[PATCH] docs: debugfs: do not recommend debugfs_remove_recursive

[PATCH] docs: debugfs: do not recommend debugfs_remove_recursive

[Timur Tabi]

Update the debugfs documentation to indicate that debugfs_remove()
should be used to clean up debugfs entries.

In commit a3d1e7eb5abe ("simple_recursive_removal(): kernel-side rm -rf
for ramfs-style filesystems"), function debugfs_remove_recursive()
was made into an alias for debugfs_remove():

    #define debugfs_remove_recursive debugfs_remove

Therefore, drivers should just use debugfs_remove() going forward.

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 Documentation/filesystems/debugfs.rst         | 19 ++++++-------------
 .../driver_development_debugging_guide.rst    |  2 +-
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/Documentation/filesystems/debugfs.rst b/Documentation/filesystems/debugfs.rst
index 610f718ef8b5..55f807293924 100644
--- a/Documentation/filesystems/debugfs.rst
+++ b/Documentation/filesystems/debugfs.rst
@@ -229,22 +229,15 @@ module is unloaded without explicitly removing debugfs entries, the result
 will be a lot of stale pointers and no end of highly antisocial behavior.
 So all debugfs users - at least those which can be built as modules - must
 be prepared to remove all files and directories they create there.  A file
-can be removed with::
+or directory can be removed with::
 
     void debugfs_remove(struct dentry *dentry);
 
 The dentry value can be NULL or an error value, in which case nothing will
-be removed.
-
-Once upon a time, debugfs users were required to remember the dentry
-pointer for every debugfs file they created so that all files could be
-cleaned up.  We live in more civilized times now, though, and debugfs users
-can call::
-
-    void debugfs_remove_recursive(struct dentry *dentry);
-
-If this function is passed a pointer for the dentry corresponding to the
-top-level directory, the entire hierarchy below that directory will be
-removed.
+be removed.  Note that this function will recursively remove all files and
+directories underneath it.  Previously, debugfs_remove_recursive() was used
+to perform that task, but this function is now just an alias to
+debugfs_remove().  debugfs_remove_recursive() should be considered
+deprecated.
 
 .. [1] http://lwn.net/Articles/309298/
diff --git a/Documentation/process/debugging/driver_development_debugging_guide.rst b/Documentation/process/debugging/driver_development_debugging_guide.rst
index 46becda8764b..aca08f457793 100644
--- a/Documentation/process/debugging/driver_development_debugging_guide.rst
+++ b/Documentation/process/debugging/driver_development_debugging_guide.rst
@@ -155,7 +155,7 @@ The general idea is:
     ``my_variable``
 
 - Clean up the directory when removing the device
-  (``debugfs_remove_recursive(parent);``)
+  (``debugfs_remove(parent);``)
 
 For the full documentation see :doc:`/filesystems/debugfs`.
 
-- 
2.43.0

Re: [PATCH] docs: debugfs: do not recommend debugfs_remove_recursive

[Greg Kroah-Hartman]

On Tue, Apr 29, 2025 at 12:39:58PM -0500, Timur Tabi wrote:
> Update the debugfs documentation to indicate that debugfs_remove()
> should be used to clean up debugfs entries.
> 
> In commit a3d1e7eb5abe ("simple_recursive_removal(): kernel-side rm -rf
> for ramfs-style filesystems"), function debugfs_remove_recursive()
> was made into an alias for debugfs_remove():
> 
>     #define debugfs_remove_recursive debugfs_remove
> 
> Therefore, drivers should just use debugfs_remove() going forward.

No, the other way around, we should be telling people to use
debugfs_remove_recursive() instead please, and getting rid of
debugfs_remove() entirely.

thanks,

greg k-h

Re: [PATCH] docs: debugfs: do not recommend debugfs_remove_recursive

[Timur Tabi]

On Tue, 2025-04-29 at 19:47 +0200, Greg Kroah-Hartman wrote:
> No, the other way around, we should be telling people to use
> debugfs_remove_recursive() instead please, and getting rid of
> debugfs_remove() entirely.

Then why was 

	#define debugfs_remove_recursive debugfs_remove

added back in 2019, and why was that functionality *added* to debugfs_remove?

I recently added a patch to Nouveau that used debugfs_remove() to clean up all debugfs entries
specifically because it operates recursively.

Re: [PATCH] docs: debugfs: do not recommend debugfs_remove_recursive

[gregkh]

On Tue, Apr 29, 2025 at 06:24:40PM +0000, Timur Tabi wrote:
> On Tue, 2025-04-29 at 19:47 +0200, Greg Kroah-Hartman wrote:
> > No, the other way around, we should be telling people to use
> > debugfs_remove_recursive() instead please, and getting rid of
> > debugfs_remove() entirely.
> 
> Then why was 
> 
> 	#define debugfs_remove_recursive debugfs_remove
> 
> added back in 2019, and why was that functionality *added* to debugfs_remove?

So we didn't have 2 functions that did the same thing and no one wanted
to sweep the tree and rename everything at that time?  I honestly don't
remember, that was tens of thousands of patches ago :)

> I recently added a patch to Nouveau that used debugfs_remove() to clean up all debugfs entries
> specifically because it operates recursively.

That's great, I'm not objecting to that, just that we need to stick with
one or the other and I'd prefer the "recursive" name as that makes it
blindingly obvious what is happening here while without it, people can
get confused.

thanks,

greg k-h

Re: [PATCH] docs: debugfs: do not recommend debugfs_remove_recursive

[Timur Tabi]

On Wed, 2025-04-30 at 09:30 +0200, gregkh@linuxfoundation.org wrote:
> > added back in 2019, and why was that functionality *added* to
> > debugfs_remove?
> 
> So we didn't have 2 functions that did the same thing and no one wanted
> to sweep the tree and rename everything at that time?  I honestly don't
> remember, that was tens of thousands of patches ago :)

I get that, but it seems pretty clear that at the time, the intent was to
replace debugfs_remove_recursive() with debugfs_remove().  The C function is
named debugfs_remove() and the macro is called debugfs_remove_recursive().

What you're saying now is that the C function should be renamed to
debugfs_remove_recursive() and the macro should be swapped.  I don't think
that's a good idea.

> > I recently added a patch to Nouveau that used debugfs_remove() to clean up
> > all debugfs entries
> > specifically because it operates recursively.
> 
> That's great, I'm not objecting to that, just that we need to stick with
> one or the other and I'd prefer the "recursive" name as that makes it
> blindingly obvious what is happening here while without it, people can
> get confused.

Well, wouldn't you think it's confusing to call a function named
debugfs_remove_recursive() in order to remove a single file?

If you want, I can change the documentation to say, "please use
debugfs_remove_recursive() to remove directories, and debugfs_remove() to
remove files".  

We could also modify debugfs_remove_recursive() to issue a WARN if it is
called on a file.

Re: [PATCH] docs: debugfs: do not recommend debugfs_remove_recursive

[gregkh]

On Wed, Apr 30, 2025 at 02:27:18PM +0000, Timur Tabi wrote:
> On Wed, 2025-04-30 at 09:30 +0200, gregkh@linuxfoundation.org wrote:
> > > added back in 2019, and why was that functionality *added* to
> > > debugfs_remove?
> > 
> > So we didn't have 2 functions that did the same thing and no one wanted
> > to sweep the tree and rename everything at that time?  I honestly don't
> > remember, that was tens of thousands of patches ago :)
> 
> I get that, but it seems pretty clear that at the time, the intent was to
> replace debugfs_remove_recursive() with debugfs_remove().  The C function is
> named debugfs_remove() and the macro is called debugfs_remove_recursive().
> 
> What you're saying now is that the C function should be renamed to
> debugfs_remove_recursive() and the macro should be swapped.  I don't think
> that's a good idea.

I don't really remember what we were talking about in 2019 for this, but
look at how many of each there are in the tree:
	402	debugfs_remove
	627	debugfs_remove_recursive

so we need to pick one and just stick to it.  Majority wins?  Shortest
function name wins?  Most descriptive winse?

Naming is hard.

> > > I recently added a patch to Nouveau that used debugfs_remove() to clean up
> > > all debugfs entries
> > > specifically because it operates recursively.
> > 
> > That's great, I'm not objecting to that, just that we need to stick with
> > one or the other and I'd prefer the "recursive" name as that makes it
> > blindingly obvious what is happening here while without it, people can
> > get confused.
> 
> Well, wouldn't you think it's confusing to call a function named
> debugfs_remove_recursive() in order to remove a single file?

As debugfs doesn't really care about files vs. directories it just
doesn't matter.

> If you want, I can change the documentation to say, "please use
> debugfs_remove_recursive() to remove directories, and debugfs_remove() to
> remove files".  

Let's just pick one and be done with it please.  Especially now that we
are reviewing the rust bindings for it, let's not end up duplicating
that mess there.

> We could also modify debugfs_remove_recursive() to issue a WARN if it is
> called on a file.

Never call WARN().  If you do, you just rebooted the box because a few
billion Linux machined have panic-on-warn enabled.

So I'll defer to you, which one do you want?  You originally said
debugfs_remove(), which is fine, but you get to send a patch touching
all of those files :)

thanks,

greg k-h

Re: [PATCH] docs: debugfs: do not recommend debugfs_remove_recursive

[Timur Tabi]

On Wed, 2025-04-30 at 16:57 +0200, gregkh@linuxfoundation.org wrote:
> I don't really remember what we were talking about in 2019 for this, but
> look at how many of each there are in the tree:
> 	402	debugfs_remove
> 	627	debugfs_remove_recursive
> 
> so we need to pick one and just stick to it.  Majority wins?  Shortest
> function name wins?  Most descriptive winse?

How about "intent of the original patch wins"?  I'm pretty sure that if my patch had been merged in
2019, these numbers would be swapped.

> Naming is hard.

Sure, but it just seems pretty clear that this has already been decided, and the only mistake was in
not updating the documentation when that decision was made in 2019.

> > > 
> > Well, wouldn't you think it's confusing to call a function named
> > debugfs_remove_recursive() in order to remove a single file?
> 
> As debugfs doesn't really care about files vs. directories it just
> doesn't matter.

Well, the word "recursive" should mean something.

> > If you want, I can change the documentation to say, "please use
> > debugfs_remove_recursive() to remove directories, and debugfs_remove() to
> > remove files".  
> 
> Let's just pick one and be done with it please.  

That's what my patch does!

> Especially now that we
> are reviewing the rust bindings for it, let's not end up duplicating
> that mess there.

Coincidentally, I noticed this because I'm doing some Rust debugfs work.

> > We could also modify debugfs_remove_recursive() to issue a WARN if it is
> > called on a file.
> 
> Never call WARN().  If you do, you just rebooted the box because a few
> billion Linux machined have panic-on-warn enabled.

Fine, pr_warn() then.  But like you said, you want one of these functions removed, not enhanced.

> So I'll defer to you, which one do you want?  You originally said
> debugfs_remove(), which is fine, but you get to send a patch touching
> all of those files :)

If you really want me to send out a patch modifying 600+ calls, I will, but I think that will cause
more harm than good, and I'll just make more enemies than friends.  

All I was trying to do with my patch was have the documentation align with the code.  This would be
a low-impact first step to replacing debugfs_remove_recursive with debugfs_remove everywhere.

Re: [PATCH] docs: debugfs: do not recommend debugfs_remove_recursive

[gregkh]

On Wed, Apr 30, 2025 at 04:36:04PM +0000, Timur Tabi wrote:
> On Wed, 2025-04-30 at 16:57 +0200, gregkh@linuxfoundation.org wrote:
> > I don't really remember what we were talking about in 2019 for this, but
> > look at how many of each there are in the tree:
> > 	402	debugfs_remove
> > 	627	debugfs_remove_recursive
> > 
> > so we need to pick one and just stick to it.  Majority wins?  Shortest
> > function name wins?  Most descriptive winse?
> 
> How about "intent of the original patch wins"?  I'm pretty sure that if my patch had been merged in
> 2019, these numbers would be swapped.

5.4.0 came out in 2019, and the numbers there are:
	402	debugfs_remove
	461	debugfs_remove_recursive
So not quite.


But we are both ignoring debugfs_lookup_and_remove(), which I do
recommend people using over _both_ of those as there's no need to keep
anything around in the driver at all.

So that proves that you are right, and I'm wrong, as there is no
debugfs_lookup_and_remove_recursive()

> > So I'll defer to you, which one do you want?  You originally said
> > debugfs_remove(), which is fine, but you get to send a patch touching
> > all of those files :)
> 
> If you really want me to send out a patch modifying 600+ calls, I will, but I think that will cause
> more harm than good, and I'll just make more enemies than friends.  

A sed script for Linus at -rc1 is usually a good way to do this, but I
can throw some interns at it as well.  Or I can script it on my side and
just do a big push in one of my trees.

> All I was trying to do with my patch was have the documentation align with the code.  This would be
> a low-impact first step to replacing debugfs_remove_recursive with debugfs_remove everywhere.

Ok, fair enough, I'm now convinced, I'll go take your patch now, sorry
for the noise, but thanks for the discussion.

greg k-h