Re: [PATCH] iio: core: use debugfs_remove_recursive() on IIO unload

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 16 Dec 2019 07:43:30 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sun, 2019-12-15 at 16:25 +0000, Jonathan Cameron wrote:
> > [External]
> > 
> > On Wed, 11 Dec 2019 16:59:03 +0200
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >   
> > > From: Lars-Peter Clausen <lars@metafoo.de>
> > > 
> > > The debugfs dir may have multiple files/dirs attached to it. Use
> > > debugfs_remove_recursive() to clean it up when the IIO module unloads.
> > > 
> > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > ---
> > > 
> > > Note: note sure if this needs a Fixes tag. It's been in here for ages
> > > now,
> > >       so it may not be a big issue.  
> > 
> > There is some argument in favour of doing this as a defensive measure,
> > but
> > in current IIO debugfs directories are only created by drivers that call
> > (indirectly) iio_device_register_debugfs and those must call
> > (again indirectly) iio_device_unregister_debugfs which has called
> > the recursive free on everything in their directories.  So unless
> > something
> > very odd is going on this isn't a bug.
> > 
> > Am I missing something?  
> 
> So, I admit my note is open for interpretation.
> [ Rergarding the "Fixes" tag, I'm still not super-clear on when/why/how
> it's needed. I'll get there soon though. ]
> 
> This patch has been in our tree since 2012, and I'm not sure why it's using
> debugfs_remove_recursive() vs plain debugfs_remove().
> 
> I sometimes take a look at these old patches, try to make some sense of
> them and upstream them, or override them with the version of upstream code.
> My intent is to sync ADI code [for IIO] closer to upstream. Which would
> make it easier to track any divergence.
> 
> We're not seeing any bug with/without this.
> I guess my note about the "Fixes" tag may be a bit clumsy.
> 
> From my side, both debugfs_remove_recursive() & plain debugfs_remove() are
> fine.
I'd rather leave it be as it kind of avoids people thinking they can get away
without cleaning up properly at the higher levels.

Thanks,

Jonathan

> 
> Thanks
> Alex
> 
> > 
> >   
> > >  drivers/iio/industrialio-core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/iio/industrialio-core.c
> > > b/drivers/iio/industrialio-core.c
> > > index 9a3579943574..dab67cb69fe6 100644
> > > --- a/drivers/iio/industrialio-core.c
> > > +++ b/drivers/iio/industrialio-core.c
> > > @@ -292,7 +292,7 @@ static void __exit iio_exit(void)
> > >  	if (iio_devt)
> > >  		unregister_chrdev_region(iio_devt, IIO_DEV_MAX);
> > >  	bus_unregister(&iio_bus_type);
> > > -	debugfs_remove(iio_debugfs_dentry);
> > > +	debugfs_remove_recursive(iio_debugfs_dentry);
> > >  }
> > >  
> > >  #if defined(CONFIG_DEBUG_FS)