[PATCH] debugfs: remove return value of debugfs_create_bool()

[PATCH] debugfs: remove return value of debugfs_create_bool()

[Greg Kroah-Hartman]

No one checks the return value of debugfs_create_bool(), as it's not
needed, so make the return value void, so that no one tries to do so in
the future.

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 Documentation/filesystems/debugfs.rst |  4 ++--
 fs/debugfs/file.c                     | 15 +++------------
 include/linux/debugfs.h               | 12 ++++--------
 3 files changed, 9 insertions(+), 22 deletions(-)

diff --git a/Documentation/filesystems/debugfs.rst b/Documentation/filesystems/debugfs.rst
index 0f2292e367e6..71b1fee56d2a 100644
--- a/Documentation/filesystems/debugfs.rst
+++ b/Documentation/filesystems/debugfs.rst
@@ -120,8 +120,8 @@ and hexadecimal::
 
 Boolean values can be placed in debugfs with::
 
-    struct dentry *debugfs_create_bool(const char *name, umode_t mode,
-				       struct dentry *parent, bool *value);
+    void debugfs_create_bool(const char *name, umode_t mode,
+                             struct dentry *parent, bool *value);
 
 A read on the resulting file will yield either Y (for non-zero values) or
 N, followed by a newline.  If written to, it will accept either upper- or
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index d513d5465c89..6ede714cb551 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -836,20 +836,11 @@ static const struct file_operations fops_bool_wo = {
  * This function creates a file in debugfs with the given name that
  * contains the value of the variable @value.  If the @mode variable is so
  * set, it can be read from, and written to.
- *
- * This function will return a pointer to a dentry if it succeeds.  This
- * pointer must be passed to the debugfs_remove() function when the file is
- * to be removed (no automatic cleanup happens if your module is unloaded,
- * you are responsible here.)  If an error occurs, ERR_PTR(-ERROR) will be
- * returned.
- *
- * If debugfs is not enabled in the kernel, the value ERR_PTR(-ENODEV) will
- * be returned.
  */
-struct dentry *debugfs_create_bool(const char *name, umode_t mode,
-				   struct dentry *parent, bool *value)
+void debugfs_create_bool(const char *name, umode_t mode, struct dentry *parent,
+			 bool *value)
 {
-	return debugfs_create_mode_unsafe(name, mode, parent, value, &fops_bool,
+	debugfs_create_mode_unsafe(name, mode, parent, value, &fops_bool,
 				   &fops_bool_ro, &fops_bool_wo);
 }
 EXPORT_SYMBOL_GPL(debugfs_create_bool);
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 4e82f2b1fa62..c869f1e73d75 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -126,8 +126,8 @@ void debugfs_create_size_t(const char *name, umode_t mode,
 			   struct dentry *parent, size_t *value);
 void debugfs_create_atomic_t(const char *name, umode_t mode,
 			     struct dentry *parent, atomic_t *value);
-struct dentry *debugfs_create_bool(const char *name, umode_t mode,
-				  struct dentry *parent, bool *value);
+void debugfs_create_bool(const char *name, umode_t mode, struct dentry *parent,
+			 bool *value);
 void debugfs_create_str(const char *name, umode_t mode,
 			struct dentry *parent, char **value);
 
@@ -291,12 +291,8 @@ static inline void debugfs_create_atomic_t(const char *name, umode_t mode,
 					   atomic_t *value)
 { }
 
-static inline struct dentry *debugfs_create_bool(const char *name, umode_t mode,
-						 struct dentry *parent,
-						 bool *value)
-{
-	return ERR_PTR(-ENODEV);
-}
+static inline void debugfs_create_bool(const char *name, umode_t mode,
+				       struct dentry *parent, bool *value) { }
 
 static inline void debugfs_create_str(const char *name, umode_t mode,
 				      struct dentry *parent,
-- 
2.31.1

Re: [PATCH] debugfs: remove return value of debugfs_create_bool()

[Geert Uytterhoeven]

Hi Greg,

Thanks for your patch!

On Fri, May 21, 2021 at 10:28 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> No one checks the return value of debugfs_create_bool(), as it's not
> needed, so make the return value void, so that no one tries to do so in

Please explain in the patch description why it is not needed.

> the future.
>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -836,20 +836,11 @@ static const struct file_operations fops_bool_wo = {
>   * This function creates a file in debugfs with the given name that
>   * contains the value of the variable @value.  If the @mode variable is so
>   * set, it can be read from, and written to.
> - *
> - * This function will return a pointer to a dentry if it succeeds.  This
> - * pointer must be passed to the debugfs_remove() function when the file is
> - * to be removed (no automatic cleanup happens if your module is unloaded,

Why isn't the above no longer true?
Are we no longer allowed to remove individual debugfs entries?
Do we always have to remove the whole parent directory and all its
contents together?

> - * you are responsible here.)  If an error occurs, ERR_PTR(-ERROR) will be
> - * returned.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] debugfs: remove return value of debugfs_create_bool()

[Greg Kroah-Hartman]

On Mon, May 24, 2021 at 11:11:32AM +0200, Geert Uytterhoeven wrote:
> Hi Greg,
> 
> Thanks for your patch!
> 
> On Fri, May 21, 2021 at 10:28 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > No one checks the return value of debugfs_create_bool(), as it's not
> > needed, so make the return value void, so that no one tries to do so in
> 
> Please explain in the patch description why it is not needed.

Because you just do not need it, like almost all other debugfs calls
now.

> > the future.
> >
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> > --- a/fs/debugfs/file.c
> > +++ b/fs/debugfs/file.c
> > @@ -836,20 +836,11 @@ static const struct file_operations fops_bool_wo = {
> >   * This function creates a file in debugfs with the given name that
> >   * contains the value of the variable @value.  If the @mode variable is so
> >   * set, it can be read from, and written to.
> > - *
> > - * This function will return a pointer to a dentry if it succeeds.  This
> > - * pointer must be passed to the debugfs_remove() function when the file is
> > - * to be removed (no automatic cleanup happens if your module is unloaded,
> 
> Why isn't the above no longer true?

Because there is no return value.

> Are we no longer allowed to remove individual debugfs entries?

It's not something that is almost ever needed.

> Do we always have to remove the whole parent directory and all its
> contents together?

For 99% of all debugfs usages, yes, that is true.

If you really do need the file dentry, there is still a call to create
it, and you can always query debugfs for the dentry after it is created
if you need it later on.

thanks,

greg k-h

Re: [PATCH] debugfs: remove return value of debugfs_create_bool()

[Geert Uytterhoeven]

Hi Greg,

On Mon, May 24, 2021 at 11:41 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, May 24, 2021 at 11:11:32AM +0200, Geert Uytterhoeven wrote:
> > On Fri, May 21, 2021 at 10:28 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > No one checks the return value of debugfs_create_bool(), as it's not
> > > needed, so make the return value void, so that no one tries to do so in
> >
> > Please explain in the patch description why it is not needed.
>
> Because you just do not need it, like almost all other debugfs calls
> now.

Why do I just not need it?

> > > the future.
> > >
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > > --- a/fs/debugfs/file.c
> > > +++ b/fs/debugfs/file.c
> > > @@ -836,20 +836,11 @@ static const struct file_operations fops_bool_wo = {
> > >   * This function creates a file in debugfs with the given name that
> > >   * contains the value of the variable @value.  If the @mode variable is so
> > >   * set, it can be read from, and written to.
> > > - *
> > > - * This function will return a pointer to a dentry if it succeeds.  This
> > > - * pointer must be passed to the debugfs_remove() function when the file is
> > > - * to be removed (no automatic cleanup happens if your module is unloaded,
> >
> > Why isn't the above no longer true?
>
> Because there is no return value.

Why is there no longer a return value?
Please stop giving circular answers as justifications.

> > Are we no longer allowed to remove individual debugfs entries?
>
> It's not something that is almost ever needed.

Why not?

> > Do we always have to remove the whole parent directory and all its
> > contents together?
>
> For 99% of all debugfs usages, yes, that is true.

So 1% of the users still need it...

> If you really do need the file dentry, there is still a call to create
> it, and you can always query debugfs for the dentry after it is created

... and will have to duplicate debugfs_create_bool() and friends, but
with a return value.  This may introduce bugs, and will complicate
maintenance, as any change to debugfs_create_bool() means all those
copies need to be found and updated, too.

> if you need it later on.

Which involves looking up something again...

Seriously, what's the added value of removing the return value from
debugfs_create_bool() and friends?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] debugfs: remove return value of debugfs_create_bool()

[Greg Kroah-Hartman]

On Mon, May 24, 2021 at 11:51:42AM +0200, Geert Uytterhoeven wrote:
> Hi Greg,
> 
> On Mon, May 24, 2021 at 11:41 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, May 24, 2021 at 11:11:32AM +0200, Geert Uytterhoeven wrote:
> > > On Fri, May 21, 2021 at 10:28 PM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > > No one checks the return value of debugfs_create_bool(), as it's not
> > > > needed, so make the return value void, so that no one tries to do so in
> > >
> > > Please explain in the patch description why it is not needed.
> >
> > Because you just do not need it, like almost all other debugfs calls
> > now.
> 
> Why do I just not need it?

Let me flip it around, why do you need it?  There are no in-kernel users
of the return value anymore so what code requires this pointer now?

The goal of removing these dentry pointers was that users were somehow
using the return value to determine code paths (like erroring out of
files were not created).  Debugfs code working or not working should
never matter, this is only for debugging features and we had a number of
cases where if debugfs was acting up, other "real" things would stop
working.

Yes, there are a few exceptions that some of the perf/trace people point
out, and they still check the return value of creating individual
debugfs files for good reasons.  But for any driver or a "normal" kernel
subsystem, they should not be doing that as it's wasteful and pointless.

debugfs is supposed to be "simple" and almost "fire and forget" as
possible.  By removing the ability to check return values, it helps
achieve this as I have seen all sorts of errors when trying to check the
return values of debugfs calls, mostly where people were thinking they
were checking for an error, yet they really were not.

So for the past few years, I've been slowly cleaning this all up,
removing the ability to get using the debugfs api wrong, which is the
end-goal here.  By allowing a return value to be forced to be checked,
developers have the ability to get it wrong (and they did.)

> > If you really do need the file dentry, there is still a call to create
> > it, and you can always query debugfs for the dentry after it is created
> 
> ... and will have to duplicate debugfs_create_bool() and friends, but
> with a return value.  This may introduce bugs, and will complicate
> maintenance, as any change to debugfs_create_bool() means all those
> copies need to be found and updated, too.

There are no in-kernel users that need to check this return value, so
what code are we talking about here?

thanks,

greg k-h

Re: [PATCH] debugfs: remove return value of debugfs_create_bool()

[Geert Uytterhoeven]

Hi Greg,

On Mon, May 24, 2021 at 12:18 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, May 24, 2021 at 11:51:42AM +0200, Geert Uytterhoeven wrote:
> > On Mon, May 24, 2021 at 11:41 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Mon, May 24, 2021 at 11:11:32AM +0200, Geert Uytterhoeven wrote:
> > > > On Fri, May 21, 2021 at 10:28 PM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > > No one checks the return value of debugfs_create_bool(), as it's not
> > > > > needed, so make the return value void, so that no one tries to do so in
> > > >
> > > > Please explain in the patch description why it is not needed.
> > >
> > > Because you just do not need it, like almost all other debugfs calls
> > > now.
> >
> > Why do I just not need it?
>
> Let me flip it around, why do you need it?  There are no in-kernel users
> of the return value anymore so what code requires this pointer now?

There still are a few users of other members in the family, and some
of them are meant to be removed without removing the full parent
directory.  Having all debugfs_create_*() functions behave the same
is a bonus.

> The goal of removing these dentry pointers was that users were somehow
> using the return value to determine code paths (like erroring out of
> files were not created).  Debugfs code working or not working should
> never matter, this is only for debugging features and we had a number of
> cases where if debugfs was acting up, other "real" things would stop
> working.
>
> Yes, there are a few exceptions that some of the perf/trace people point
> out, and they still check the return value of creating individual
> debugfs files for good reasons.  But for any driver or a "normal" kernel
> subsystem, they should not be doing that as it's wasteful and pointless.
>
> debugfs is supposed to be "simple" and almost "fire and forget" as
> possible.  By removing the ability to check return values, it helps
> achieve this as I have seen all sorts of errors when trying to check the
> return values of debugfs calls, mostly where people were thinking they
> were checking for an error, yet they really were not.
>
> So for the past few years, I've been slowly cleaning this all up,
> removing the ability to get using the debugfs api wrong, which is the
> end-goal here.  By allowing a return value to be forced to be checked,
> developers have the ability to get it wrong (and they did.)

"to be forced to be checked" applies to _must_check only.

> > > If you really do need the file dentry, there is still a call to create
> > > it, and you can always query debugfs for the dentry after it is created
> >
> > ... and will have to duplicate debugfs_create_bool() and friends, but
> > with a return value.  This may introduce bugs, and will complicate
> > maintenance, as any change to debugfs_create_bool() means all those
> > copies need to be found and updated, too.
>
> There are no in-kernel users that need to check this return value, so
> what code are we talking about here?

Yeah, you removed the last user in commit 1be4ec2456a7d110 ("scsi:
snic: debugfs: remove local storage of debugfs files") ;-)
As I said, there are a few more for other similar functions.

But if other people are fine with having to call
debugfs_remove(debugfs_lookup(...)), well, let it be like that...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] debugfs: remove return value of debugfs_create_bool()

[Greg Kroah-Hartman]

On Mon, May 24, 2021 at 01:44:38PM +0200, Geert Uytterhoeven wrote:
> Hi Greg,
> 
> On Mon, May 24, 2021 at 12:18 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, May 24, 2021 at 11:51:42AM +0200, Geert Uytterhoeven wrote:
> > > On Mon, May 24, 2021 at 11:41 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > > On Mon, May 24, 2021 at 11:11:32AM +0200, Geert Uytterhoeven wrote:
> > > > > On Fri, May 21, 2021 at 10:28 PM Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > No one checks the return value of debugfs_create_bool(), as it's not
> > > > > > needed, so make the return value void, so that no one tries to do so in
> > > > >
> > > > > Please explain in the patch description why it is not needed.
> > > >
> > > > Because you just do not need it, like almost all other debugfs calls
> > > > now.
> > >
> > > Why do I just not need it?
> >
> > Let me flip it around, why do you need it?  There are no in-kernel users
> > of the return value anymore so what code requires this pointer now?
> 
> There still are a few users of other members in the family, and some
> of them are meant to be removed without removing the full parent
> directory.  Having all debugfs_create_*() functions behave the same
> is a bonus.

I agree, and we are almost there, all that is left is:
	debugfs_create_blob()
	debugfs_create_file()
	debugfs_create_file_unsafe()
for creating debugfs files.

There is still:
	debugfs_create_dir()
	debugfs_create_symlink()
	debugfs_create_automount()
for non-file creations that do not return void.

The majority of the debugfs_create_* functions now do not return
anything.

> But if other people are fine with having to call
> debugfs_remove(debugfs_lookup(...)), well, let it be like that...

It saves at least a static variable, so what's not to like?  :)

thanks,

greg k-h

Re: [PATCH] debugfs: remove return value of debugfs_create_bool()

[Geert Uytterhoeven]

Hi Greg,

On Mon, May 24, 2021 at 2:39 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, May 24, 2021 at 01:44:38PM +0200, Geert Uytterhoeven wrote:
> > On Mon, May 24, 2021 at 12:18 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Mon, May 24, 2021 at 11:51:42AM +0200, Geert Uytterhoeven wrote:
> > > > On Mon, May 24, 2021 at 11:41 AM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > > On Mon, May 24, 2021 at 11:11:32AM +0200, Geert Uytterhoeven wrote:
> > > > > > On Fri, May 21, 2021 at 10:28 PM Greg Kroah-Hartman
> > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > > No one checks the return value of debugfs_create_bool(), as it's not
> > > > > > > needed, so make the return value void, so that no one tries to do so in
> > > > > >
> > > > > > Please explain in the patch description why it is not needed.
> > > > >
> > > > > Because you just do not need it, like almost all other debugfs calls
> > > > > now.
> > > >
> > > > Why do I just not need it?
> > >
> > > Let me flip it around, why do you need it?  There are no in-kernel users
> > > of the return value anymore so what code requires this pointer now?
> >
> > There still are a few users of other members in the family, and some
> > of them are meant to be removed without removing the full parent
> > directory.  Having all debugfs_create_*() functions behave the same
> > is a bonus.
>
> I agree, and we are almost there, all that is left is:
>         debugfs_create_blob()
>         debugfs_create_file()
>         debugfs_create_file_unsafe()
> for creating debugfs files.
>
> There is still:
>         debugfs_create_dir()
>         debugfs_create_symlink()
>         debugfs_create_automount()
> for non-file creations that do not return void.
>
> The majority of the debugfs_create_* functions now do not return
> anything.
>
> > But if other people are fine with having to call
> > debugfs_remove(debugfs_lookup(...)), well, let it be like that...
>
> It saves at least a static variable, so what's not to like?  :)

Which is more than offset by the cost of the new debugfs_lookup() call...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] debugfs: remove return value of debugfs_create_bool()

[Greg Kroah-Hartman]

On Tue, May 25, 2021 at 09:26:42AM +0200, Geert Uytterhoeven wrote:
> Hi Greg,
> 
> On Mon, May 24, 2021 at 2:39 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, May 24, 2021 at 01:44:38PM +0200, Geert Uytterhoeven wrote:
> > > On Mon, May 24, 2021 at 12:18 PM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > > On Mon, May 24, 2021 at 11:51:42AM +0200, Geert Uytterhoeven wrote:
> > > > > On Mon, May 24, 2021 at 11:41 AM Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > On Mon, May 24, 2021 at 11:11:32AM +0200, Geert Uytterhoeven wrote:
> > > > > > > On Fri, May 21, 2021 at 10:28 PM Greg Kroah-Hartman
> > > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > > > No one checks the return value of debugfs_create_bool(), as it's not
> > > > > > > > needed, so make the return value void, so that no one tries to do so in
> > > > > > >
> > > > > > > Please explain in the patch description why it is not needed.
> > > > > >
> > > > > > Because you just do not need it, like almost all other debugfs calls
> > > > > > now.
> > > > >
> > > > > Why do I just not need it?
> > > >
> > > > Let me flip it around, why do you need it?  There are no in-kernel users
> > > > of the return value anymore so what code requires this pointer now?
> > >
> > > There still are a few users of other members in the family, and some
> > > of them are meant to be removed without removing the full parent
> > > directory.  Having all debugfs_create_*() functions behave the same
> > > is a bonus.
> >
> > I agree, and we are almost there, all that is left is:
> >         debugfs_create_blob()
> >         debugfs_create_file()
> >         debugfs_create_file_unsafe()
> > for creating debugfs files.
> >
> > There is still:
> >         debugfs_create_dir()
> >         debugfs_create_symlink()
> >         debugfs_create_automount()
> > for non-file creations that do not return void.
> >
> > The majority of the debugfs_create_* functions now do not return
> > anything.
> >
> > > But if other people are fine with having to call
> > > debugfs_remove(debugfs_lookup(...)), well, let it be like that...
> >
> > It saves at least a static variable, so what's not to like?  :)
> 
> Which is more than offset by the cost of the new debugfs_lookup() call...

Not when people were keeping a dentry-per-entry in lots of structures.
That's from the heap, this dentry pointer sits on the stack, or if we
are lucky, only in a register :)

thanks,

greg k-h