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

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

[Alexandru Ardelean]

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.

 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)
-- 
2.20.1

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

[Jonathan Cameron]

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?


> 
>  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)

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

[Ardelean, Alexandru]

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.

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)

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

[Jonathan Cameron]

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)