diff mbox series

[1/7] debugfs: Add debugfs_create_xul() for hexadecimal unsigned long

Message ID 20191021143742.14487-2-geert+renesas@glider.be (mailing list archive)
State Not Applicable
Delegated to: Johannes Berg
Headers show
Series debugfs: Add and use debugfs_create_xul() | expand

Commit Message

Geert Uytterhoeven Oct. 21, 2019, 2:37 p.m. UTC
The existing debugfs_create_ulong() function supports objects of
type "unsigned long", which are 32-bit or 64-bit depending on the
platform, in decimal form.  To format objects in hexadecimal, various
debugfs_create_x*() functions exist, but all of them take fixed-size
types.

Add a debugfs helper for "unsigned long" objects in hexadecimal format.
This avoids the need for users to open-code the same, or introduce
bugs when casting the value pointer to "u32 *" or "u64 *" to call
debugfs_create_x{32,64}().

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 include/linux/debugfs.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Greg KH Oct. 21, 2019, 2:45 p.m. UTC | #1
On Mon, Oct 21, 2019 at 04:37:36PM +0200, Geert Uytterhoeven wrote:
> The existing debugfs_create_ulong() function supports objects of
> type "unsigned long", which are 32-bit or 64-bit depending on the
> platform, in decimal form.  To format objects in hexadecimal, various
> debugfs_create_x*() functions exist, but all of them take fixed-size
> types.
> 
> Add a debugfs helper for "unsigned long" objects in hexadecimal format.
> This avoids the need for users to open-code the same, or introduce
> bugs when casting the value pointer to "u32 *" or "u64 *" to call
> debugfs_create_x{32,64}().
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  include/linux/debugfs.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index 33690949b45d6904..d7b2aebcc277d65e 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -356,4 +356,14 @@ static inline ssize_t debugfs_write_file_bool(struct file *file,
>  
>  #endif
>  
> +static inline void debugfs_create_xul(const char *name, umode_t mode,
> +				      struct dentry *parent,
> +				      unsigned long *value)
> +{
> +	if (sizeof(*value) == sizeof(u32))
> +		debugfs_create_x32(name, mode, parent, (u32 *)value);
> +	else
> +		debugfs_create_x64(name, mode, parent, (u64 *)value);
> +}

Looks sane, but can you add some kernel-doc comments here so that we can
pull it into the debugfs documentation?  Also there is debugfs
documentation in Documentation/filesystems/ so maybe also add this
there?  I am going to be overhauling the debugfs documentation "soon"
but it's at the lower part of my todo list, so it will take a while,
might as well keep it up to date with new stuff added like this so that
people don't get lost.

thanks,

greg k-h
Geert Uytterhoeven Oct. 21, 2019, 2:59 p.m. UTC | #2
Hi Greg,

On Mon, Oct 21, 2019 at 4:45 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Oct 21, 2019 at 04:37:36PM +0200, Geert Uytterhoeven wrote:
> > The existing debugfs_create_ulong() function supports objects of
> > type "unsigned long", which are 32-bit or 64-bit depending on the
> > platform, in decimal form.  To format objects in hexadecimal, various
> > debugfs_create_x*() functions exist, but all of them take fixed-size
> > types.
> >
> > Add a debugfs helper for "unsigned long" objects in hexadecimal format.
> > This avoids the need for users to open-code the same, or introduce
> > bugs when casting the value pointer to "u32 *" or "u64 *" to call
> > debugfs_create_x{32,64}().
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> >  include/linux/debugfs.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> > index 33690949b45d6904..d7b2aebcc277d65e 100644
> > --- a/include/linux/debugfs.h
> > +++ b/include/linux/debugfs.h
> > @@ -356,4 +356,14 @@ static inline ssize_t debugfs_write_file_bool(struct file *file,
> >
> >  #endif
> >
> > +static inline void debugfs_create_xul(const char *name, umode_t mode,
> > +                                   struct dentry *parent,
> > +                                   unsigned long *value)
> > +{
> > +     if (sizeof(*value) == sizeof(u32))
> > +             debugfs_create_x32(name, mode, parent, (u32 *)value);
> > +     else
> > +             debugfs_create_x64(name, mode, parent, (u64 *)value);
> > +}
>
> Looks sane, but can you add some kernel-doc comments here so that we can
> pull it into the debugfs documentation?  Also there is debugfs

Sure, will do.

> documentation in Documentation/filesystems/ so maybe also add this
> there?  I am going to be overhauling the debugfs documentation "soon"
> but it's at the lower part of my todo list, so it will take a while,
> might as well keep it up to date with new stuff added like this so that
> people don't get lost.

Right. Currently it already lacks debugfs_create_ulong(). Will add both
debugfs_create_ulong() and debugfs_create_xul().

Gr{oetje,eeting}s,

                        Geert
Joe Perches Oct. 21, 2019, 3:37 p.m. UTC | #3
On Mon, 2019-10-21 at 16:37 +0200, Geert Uytterhoeven wrote:
> The existing debugfs_create_ulong() function supports objects of
> type "unsigned long", which are 32-bit or 64-bit depending on the
> platform, in decimal form.  To format objects in hexadecimal, various
> debugfs_create_x*() functions exist, but all of them take fixed-size
> types.
> 
> Add a debugfs helper for "unsigned long" objects in hexadecimal format.
> This avoids the need for users to open-code the same, or introduce
> bugs when casting the value pointer to "u32 *" or "u64 *" to call
> debugfs_create_x{32,64}().
[]
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
[]
> @@ -356,4 +356,14 @@ static inline ssize_t debugfs_write_file_bool(struct file *file,
>  
>  #endif
>  
> +static inline void debugfs_create_xul(const char *name, umode_t mode,
> +				      struct dentry *parent,
> +				      unsigned long *value)
> +{
> +	if (sizeof(*value) == sizeof(u32))
> +		debugfs_create_x32(name, mode, parent, (u32 *)value);
> +	else
> +		debugfs_create_x64(name, mode, parent, (u64 *)value);

trivia: the casts are unnecessary.

This might be more sensible using #ifdef

static inline void debugfs_create_xul(const char *name, umode_t mode,
				      struct dentry *parent,
				      unsigned long *value)
{
#if BITS_PER_LONG == 64
	debugfs_create_x64(name, mode, parent, value);
#else
	debugfs_create_x32(name, mode, parent, value);
#endif
}
Geert Uytterhoeven Oct. 22, 2019, 8:03 a.m. UTC | #4
Hi Joe,

On Mon, Oct 21, 2019 at 5:37 PM Joe Perches <joe@perches.com> wrote:
> On Mon, 2019-10-21 at 16:37 +0200, Geert Uytterhoeven wrote:
> > The existing debugfs_create_ulong() function supports objects of
> > type "unsigned long", which are 32-bit or 64-bit depending on the
> > platform, in decimal form.  To format objects in hexadecimal, various
> > debugfs_create_x*() functions exist, but all of them take fixed-size
> > types.
> >
> > Add a debugfs helper for "unsigned long" objects in hexadecimal format.
> > This avoids the need for users to open-code the same, or introduce
> > bugs when casting the value pointer to "u32 *" or "u64 *" to call
> > debugfs_create_x{32,64}().
> []
> > diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> []
> > @@ -356,4 +356,14 @@ static inline ssize_t debugfs_write_file_bool(struct file *file,
> >
> >  #endif
> >
> > +static inline void debugfs_create_xul(const char *name, umode_t mode,
> > +                                   struct dentry *parent,
> > +                                   unsigned long *value)
> > +{
> > +     if (sizeof(*value) == sizeof(u32))
> > +             debugfs_create_x32(name, mode, parent, (u32 *)value);
> > +     else
> > +             debugfs_create_x64(name, mode, parent, (u64 *)value);
>
> trivia: the casts are unnecessary.

They are necessary, in both calls (so using #ifdef as suggested below
won't help):

    include/linux/debugfs.h:375:42: error: passing argument 4 of
‘debugfs_create_x32’ from incompatible pointer type
[-Werror=incompatible-pointer-types]
       debugfs_create_x32(name, mode, parent, value);
                                              ^~~~~
    include/linux/debugfs.h:114:6: note: expected ‘u32 * {aka unsigned
int *}’ but argument is of type ‘long unsigned int *’
     void debugfs_create_x32(const char *name, umode_t mode, struct
dentry *parent,
          ^~~~~~~~~~~~~~~~~~
    include/linux/debugfs.h:377:42: error: passing argument 4 of
‘debugfs_create_x64’ from incompatible pointer type
[-Werror=incompatible-pointer-types]
       debugfs_create_x64(name, mode, parent, value);
                                              ^~~~~
    include/linux/debugfs.h:116:6: note: expected ‘u64 * {aka long
long unsigned int *}’ but argument is of type ‘long unsigned int *’
     void debugfs_create_x64(const char *name, umode_t mode, struct
dentry *parent,
          ^~~~~~~~~~~~~~~~~~

> This might be more sensible using #ifdef
>
> static inline void debugfs_create_xul(const char *name, umode_t mode,
>                                       struct dentry *parent,
>                                       unsigned long *value)
> {
> #if BITS_PER_LONG == 64
>         debugfs_create_x64(name, mode, parent, value);
> #else
>         debugfs_create_x32(name, mode, parent, value);
> #endif
> }

... at the expense of the compiler checking only one branch.

Just like "if (IS_ENABLED(CONFIG_<foo>)" (when possible) is preferred
over "#ifdef CONFIG_<foo>" because of compile-coverage, I think using
"if" here is better than using "#if".

Gr{oetje,eeting}s,

                        Geert
Joe Perches Oct. 22, 2019, 9:07 a.m. UTC | #5
On Tue, 2019-10-22 at 10:03 +0200, Geert Uytterhoeven wrote:
> Hi Joe,

Hey again Geert.

> On Mon, Oct 21, 2019 at 5:37 PM Joe Perches <joe@perches.com> wrote:
> > On Mon, 2019-10-21 at 16:37 +0200, Geert Uytterhoeven wrote:
> > > The existing debugfs_create_ulong() function supports objects of
> > > type "unsigned long", which are 32-bit or 64-bit depending on the
> > > platform, in decimal form.  To format objects in hexadecimal, various
> > > debugfs_create_x*() functions exist, but all of them take fixed-size
> > > types.
> > > 
> > > Add a debugfs helper for "unsigned long" objects in hexadecimal format.
> > > This avoids the need for users to open-code the same, or introduce
> > > bugs when casting the value pointer to "u32 *" or "u64 *" to call
> > > debugfs_create_x{32,64}().
> > []
> > > diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> > []
> > > @@ -356,4 +356,14 @@ static inline ssize_t debugfs_write_file_bool(struct file *file,
> > > 
> > >  #endif
> > > 
> > > +static inline void debugfs_create_xul(const char *name, umode_t mode,
> > > +                                   struct dentry *parent,
> > > +                                   unsigned long *value)
> > > +{
> > > +     if (sizeof(*value) == sizeof(u32))
> > > +             debugfs_create_x32(name, mode, parent, (u32 *)value);
> > > +     else
> > > +             debugfs_create_x64(name, mode, parent, (u64 *)value);
> > 
> > trivia: the casts are unnecessary.
> 
> They are necessary, in both calls (so using #ifdef as suggested below
> won't help):

Silly thinko, (I somehow thought the compiler would
eliminate the code after the branch not taken, but
of course it has to compile it first...  oops)
though the #ifdef should work.

> > This might be more sensible using #ifdef
> > 
> > static inline void debugfs_create_xul(const char *name, umode_t mode,
> >                                       struct dentry *parent,
> >                                       unsigned long *value)
> > {
> > #if BITS_PER_LONG == 64
> >         debugfs_create_x64(name, mode, parent, value);
> > #else
> >         debugfs_create_x32(name, mode, parent, value);
> > #endif
> > }
> 
> ... at the expense of the compiler checking only one branch.
> 
> Just like "if (IS_ENABLED(CONFIG_<foo>)" (when possible) is preferred
> over "#ifdef CONFIG_<foo>" because of compile-coverage, I think using
> "if" here is better than using "#if".

True if all compilers will always eliminate the unused branch.
Greg KH Oct. 22, 2019, 5:35 p.m. UTC | #6
On Tue, Oct 22, 2019 at 02:07:34AM -0700, Joe Perches wrote:
> On Tue, 2019-10-22 at 10:03 +0200, Geert Uytterhoeven wrote:
> > Hi Joe,
> 
> Hey again Geert.
> 
> > On Mon, Oct 21, 2019 at 5:37 PM Joe Perches <joe@perches.com> wrote:
> > > On Mon, 2019-10-21 at 16:37 +0200, Geert Uytterhoeven wrote:
> > > > The existing debugfs_create_ulong() function supports objects of
> > > > type "unsigned long", which are 32-bit or 64-bit depending on the
> > > > platform, in decimal form.  To format objects in hexadecimal, various
> > > > debugfs_create_x*() functions exist, but all of them take fixed-size
> > > > types.
> > > > 
> > > > Add a debugfs helper for "unsigned long" objects in hexadecimal format.
> > > > This avoids the need for users to open-code the same, or introduce
> > > > bugs when casting the value pointer to "u32 *" or "u64 *" to call
> > > > debugfs_create_x{32,64}().
> > > []
> > > > diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> > > []
> > > > @@ -356,4 +356,14 @@ static inline ssize_t debugfs_write_file_bool(struct file *file,
> > > > 
> > > >  #endif
> > > > 
> > > > +static inline void debugfs_create_xul(const char *name, umode_t mode,
> > > > +                                   struct dentry *parent,
> > > > +                                   unsigned long *value)
> > > > +{
> > > > +     if (sizeof(*value) == sizeof(u32))
> > > > +             debugfs_create_x32(name, mode, parent, (u32 *)value);
> > > > +     else
> > > > +             debugfs_create_x64(name, mode, parent, (u64 *)value);
> > > 
> > > trivia: the casts are unnecessary.
> > 
> > They are necessary, in both calls (so using #ifdef as suggested below
> > won't help):
> 
> Silly thinko, (I somehow thought the compiler would
> eliminate the code after the branch not taken, but
> of course it has to compile it first...  oops)
> though the #ifdef should work.
> 
> > > This might be more sensible using #ifdef
> > > 
> > > static inline void debugfs_create_xul(const char *name, umode_t mode,
> > >                                       struct dentry *parent,
> > >                                       unsigned long *value)
> > > {
> > > #if BITS_PER_LONG == 64
> > >         debugfs_create_x64(name, mode, parent, value);
> > > #else
> > >         debugfs_create_x32(name, mode, parent, value);
> > > #endif
> > > }
> > 
> > ... at the expense of the compiler checking only one branch.
> > 
> > Just like "if (IS_ENABLED(CONFIG_<foo>)" (when possible) is preferred
> > over "#ifdef CONFIG_<foo>" because of compile-coverage, I think using
> > "if" here is better than using "#if".
> 
> True if all compilers will always eliminate the unused branch.

Good ones will, we don't care about bad ones :)
diff mbox series

Patch

diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 33690949b45d6904..d7b2aebcc277d65e 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -356,4 +356,14 @@  static inline ssize_t debugfs_write_file_bool(struct file *file,
 
 #endif
 
+static inline void debugfs_create_xul(const char *name, umode_t mode,
+				      struct dentry *parent,
+				      unsigned long *value)
+{
+	if (sizeof(*value) == sizeof(u32))
+		debugfs_create_x32(name, mode, parent, (u32 *)value);
+	else
+		debugfs_create_x64(name, mode, parent, (u64 *)value);
+}
+
 #endif