[PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Rohit Sarkar]

The debugfs_create_file_unsafe method does not protect the fops given to
it from file removal. It must be used with DEFINE_DEBUGFS_ATTRIBUTE
which makes the fops aware of the file lifetime.

Further using DEFINE_DEBUGFS_ATTRIBUTE along with
debugfs_create_file_unsafe significantly reduces the overhead introduced by
debugfs_create_file which creates a lifetime managing proxy around each
fops handed in. Refer [1] for more on this.

Fixes the following warnings reported by coccinelle:
drivers/iio/imu//adis16460.c:126:0-23: WARNING: adis16460_flash_count_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
drivers/iio/imu//adis16460.c:108:0-23: WARNING: adis16460_product_id_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
drivers/iio/imu//adis16460.c:90:0-23: WARNING: adis16460_serial_number_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
drivers/iio/imu//adis16400.c:278:0-23: WARNING: adis16400_flash_count_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
drivers/iio/imu//adis16400.c:261:0-23: WARNING: adis16400_product_id_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE

[1]: https://lists.gt.net/linux/kernel/2369498

Rohit Sarkar (2):
  iio: imu: adis16400: use DEFINE_DEBUGFS_ATTRIBUTE instead of
    DEFINE_SIMPLE_ATTRIBUTE
  iio: imu: adis16460: use DEFINE_DEBUGFS_ATTRIBUTE instead of
    DEFINE_SIMPLE_ATTRIBUTE

 drivers/iio/imu/adis16400.c | 4 ++--
 drivers/iio/imu/adis16460.c | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.23.0.385.gbc12974a89

[PATCH 1/2] iio: imu: adis16400: use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Rohit Sarkar]

debugfs_create_file_unsafe does not protect the fops handed to it
against file removal. DEFINE_DEBUGFS_ATTRIBUTE makes the fops aware of
the file lifetime and thus protects it against removal.

Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
---
 drivers/iio/imu/adis16400.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
index cfb1c19eb930..19a35967f385 100644
--- a/drivers/iio/imu/adis16400.c
+++ b/drivers/iio/imu/adis16400.c
@@ -258,7 +258,7 @@ static int adis16400_show_product_id(void *arg, u64 *val)
 
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(adis16400_product_id_fops,
+DEFINE_DEBUGFS_ATTRIBUTE(adis16400_product_id_fops,
 	adis16400_show_product_id, NULL, "%lld\n");
 
 static int adis16400_show_flash_count(void *arg, u64 *val)
@@ -275,7 +275,7 @@ static int adis16400_show_flash_count(void *arg, u64 *val)
 
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(adis16400_flash_count_fops,
+DEFINE_DEBUGFS_ATTRIBUTE(adis16400_flash_count_fops,
 	adis16400_show_flash_count, NULL, "%lld\n");
 
 static int adis16400_debugfs_init(struct iio_dev *indio_dev)
-- 
2.23.0.385.gbc12974a89

[PATCH 2/2] iio: imu: adis16460: use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Rohit Sarkar]

debugfs_create_file_unsafe does not protect the fops handed to it
against file removal. DEFINE_DEBUGFS_ATTRIBUTE makes the fops aware of
the file lifetime and thus protects it against removal.

Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
---
 drivers/iio/imu/adis16460.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/imu/adis16460.c b/drivers/iio/imu/adis16460.c
index 9539cfe4a259..ef22de5c0211 100644
--- a/drivers/iio/imu/adis16460.c
+++ b/drivers/iio/imu/adis16460.c
@@ -87,7 +87,7 @@ static int adis16460_show_serial_number(void *arg, u64 *val)
 
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(adis16460_serial_number_fops,
+DEFINE_DEBUGFS_ATTRIBUTE(adis16460_serial_number_fops,
 	adis16460_show_serial_number, NULL, "0x%.4llx\n");
 
 static int adis16460_show_product_id(void *arg, u64 *val)
@@ -105,7 +105,7 @@ static int adis16460_show_product_id(void *arg, u64 *val)
 
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(adis16460_product_id_fops,
+DEFINE_DEBUGFS_ATTRIBUTE(adis16460_product_id_fops,
 	adis16460_show_product_id, NULL, "%llu\n");
 
 static int adis16460_show_flash_count(void *arg, u64 *val)
@@ -123,7 +123,7 @@ static int adis16460_show_flash_count(void *arg, u64 *val)
 
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(adis16460_flash_count_fops,
+DEFINE_DEBUGFS_ATTRIBUTE(adis16460_flash_count_fops,
 	adis16460_show_flash_count, NULL, "%lld\n");
 
 static int adis16460_debugfs_init(struct iio_dev *indio_dev)
-- 
2.23.0.385.gbc12974a89

Re: [PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Jonathan Cameron]

On Sat, 28 Mar 2020 12:04:53 +0530
Rohit Sarkar <rohitsarkar5398@gmail.com> wrote:

> The debugfs_create_file_unsafe method does not protect the fops given to
> it from file removal. It must be used with DEFINE_DEBUGFS_ATTRIBUTE
> which makes the fops aware of the file lifetime.
> 
> Further using DEFINE_DEBUGFS_ATTRIBUTE along with
> debugfs_create_file_unsafe significantly reduces the overhead introduced by
> debugfs_create_file which creates a lifetime managing proxy around each
> fops handed in. Refer [1] for more on this.
> 
> Fixes the following warnings reported by coccinelle:
> drivers/iio/imu//adis16460.c:126:0-23: WARNING: adis16460_flash_count_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> drivers/iio/imu//adis16460.c:108:0-23: WARNING: adis16460_product_id_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> drivers/iio/imu//adis16460.c:90:0-23: WARNING: adis16460_serial_number_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> drivers/iio/imu//adis16400.c:278:0-23: WARNING: adis16400_flash_count_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> drivers/iio/imu//adis16400.c:261:0-23: WARNING: adis16400_product_id_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> 
> [1]: https://lists.gt.net/linux/kernel/2369498
> 
> Rohit Sarkar (2):
>   iio: imu: adis16400: use DEFINE_DEBUGFS_ATTRIBUTE instead of
>     DEFINE_SIMPLE_ATTRIBUTE
>   iio: imu: adis16460: use DEFINE_DEBUGFS_ATTRIBUTE instead of
>     DEFINE_SIMPLE_ATTRIBUTE
> 
>  drivers/iio/imu/adis16400.c | 4 ++--
>  drivers/iio/imu/adis16460.c | 6 +++---
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
Hi Rohit,

You've opened a can of worms with this one.  There as a previous series
posted doing exactly this change back in 2019 by Zhong Jiang (cc'd)

At the time I did a bit of looking into why this had been universally taken
up cross tree and turned out there are some potential issues.

Alexandru added it to the list of things to test, but I guess it got
buried under other work and is still outstanding.

https://lkml.org/lkml/2019/10/30/144

Has Greg KH raising the point that file reference counting is changed (as you
mention) but that can cause subtle bugs.  It 'might' be fine but is
definitely one that needs a tested-by from someone with the hardware.

Jonathan

Re: [PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Rohit Sarkar]

On Sun, Mar 29, 2020 at 10:38:18AM +0100, Jonathan Cameron wrote:
> On Sat, 28 Mar 2020 12:04:53 +0530
> Rohit Sarkar <rohitsarkar5398@gmail.com> wrote:
> 
> > The debugfs_create_file_unsafe method does not protect the fops given to
> > it from file removal. It must be used with DEFINE_DEBUGFS_ATTRIBUTE
> > which makes the fops aware of the file lifetime.
> > 
> > Further using DEFINE_DEBUGFS_ATTRIBUTE along with
> > debugfs_create_file_unsafe significantly reduces the overhead introduced by
> > debugfs_create_file which creates a lifetime managing proxy around each
> > fops handed in. Refer [1] for more on this.
> > 
> > Fixes the following warnings reported by coccinelle:
> > drivers/iio/imu//adis16460.c:126:0-23: WARNING: adis16460_flash_count_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > drivers/iio/imu//adis16460.c:108:0-23: WARNING: adis16460_product_id_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > drivers/iio/imu//adis16460.c:90:0-23: WARNING: adis16460_serial_number_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > drivers/iio/imu//adis16400.c:278:0-23: WARNING: adis16400_flash_count_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > drivers/iio/imu//adis16400.c:261:0-23: WARNING: adis16400_product_id_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > 
> > [1]: https://lists.gt.net/linux/kernel/2369498
> > 
> > Rohit Sarkar (2):
> >   iio: imu: adis16400: use DEFINE_DEBUGFS_ATTRIBUTE instead of
> >     DEFINE_SIMPLE_ATTRIBUTE
> >   iio: imu: adis16460: use DEFINE_DEBUGFS_ATTRIBUTE instead of
> >     DEFINE_SIMPLE_ATTRIBUTE
> > 
> >  drivers/iio/imu/adis16400.c | 4 ++--
> >  drivers/iio/imu/adis16460.c | 6 +++---
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> Hi Rohit,
Hey, 
> You've opened a can of worms with this one.  There as a previous series
> posted doing exactly this change back in 2019 by Zhong Jiang (cc'd)
> 
> At the time I did a bit of looking into why this had been universally taken
> up cross tree and turned out there are some potential issues.
> 
> Alexandru added it to the list of things to test, but I guess it got
> buried under other work and is still outstanding.
> 
> https://lkml.org/lkml/2019/10/30/144
Acc. to the patch by Zhong this change kind of comes off as a cosmetic
change as in the commit message he mentions "it is more clear". 

But there is certainly more to it than that:
In the current scenario since we are using debugfs_create_file_unsafe
the file has no protection whatsoever against removal. 
> Has Greg KH raising the point that file reference counting is changed (as you
> mention) but that can cause subtle bugs.  It 'might' be fine but is
> definitely one that needs a tested-by from someone with the hardware.
Sure that makes sense. 
> Jonathan

Thanks,
Rohit

Re: [PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Lars-Peter Clausen]

On 3/29/20 1:34 PM, Rohit Sarkar wrote:
> On Sun, Mar 29, 2020 at 10:38:18AM +0100, Jonathan Cameron wrote:
>> On Sat, 28 Mar 2020 12:04:53 +0530
>> Rohit Sarkar <rohitsarkar5398@gmail.com> wrote:
>>
>>> The debugfs_create_file_unsafe method does not protect the fops given to
>>> it from file removal. It must be used with DEFINE_DEBUGFS_ATTRIBUTE
>>> which makes the fops aware of the file lifetime.
>>>
>>> Further using DEFINE_DEBUGFS_ATTRIBUTE along with
>>> debugfs_create_file_unsafe significantly reduces the overhead introduced by
>>> debugfs_create_file which creates a lifetime managing proxy around each
>>> fops handed in. Refer [1] for more on this.
>>>
>>> Fixes the following warnings reported by coccinelle:
>>> drivers/iio/imu//adis16460.c:126:0-23: WARNING: adis16460_flash_count_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>> drivers/iio/imu//adis16460.c:108:0-23: WARNING: adis16460_product_id_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>> drivers/iio/imu//adis16460.c:90:0-23: WARNING: adis16460_serial_number_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>> drivers/iio/imu//adis16400.c:278:0-23: WARNING: adis16400_flash_count_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>> drivers/iio/imu//adis16400.c:261:0-23: WARNING: adis16400_product_id_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>>
>>> [1]: https://lists.gt.net/linux/kernel/2369498
>>>
>>> Rohit Sarkar (2):
>>>    iio: imu: adis16400: use DEFINE_DEBUGFS_ATTRIBUTE instead of
>>>      DEFINE_SIMPLE_ATTRIBUTE
>>>    iio: imu: adis16460: use DEFINE_DEBUGFS_ATTRIBUTE instead of
>>>      DEFINE_SIMPLE_ATTRIBUTE
>>>
>>>   drivers/iio/imu/adis16400.c | 4 ++--
>>>   drivers/iio/imu/adis16460.c | 6 +++---
>>>   2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>> Hi Rohit,
> Hey,
>> You've opened a can of worms with this one.  There as a previous series
>> posted doing exactly this change back in 2019 by Zhong Jiang (cc'd)
>>
>> At the time I did a bit of looking into why this had been universally taken
>> up cross tree and turned out there are some potential issues.
>>
>> Alexandru added it to the list of things to test, but I guess it got
>> buried under other work and is still outstanding.
>>
>> https://lkml.org/lkml/2019/10/30/144
> Acc. to the patch by Zhong this change kind of comes off as a cosmetic
> change as in the commit message he mentions "it is more clear".
>
> But there is certainly more to it than that:
> In the current scenario since we are using debugfs_create_file_unsafe
> the file has no protection whatsoever against removal.

The drivers you are patching all use debugfs_create_file() as far as I 
can see.

The way I understand it using DEFINE_DEBUGFS_ATTRIBUTE without switching 
to debugfs_create_file_unsafe() will not make a difference. There will 
only be more overhead since the files are protected twice.

- Lars

Re: [PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Rohit Sarkar]

On Sun, Mar 29, 2020 at 03:46:17PM +0200, Lars-Peter Clausen wrote:
> On 3/29/20 1:34 PM, Rohit Sarkar wrote:
> > On Sun, Mar 29, 2020 at 10:38:18AM +0100, Jonathan Cameron wrote:
> > > On Sat, 28 Mar 2020 12:04:53 +0530
> > > Rohit Sarkar <rohitsarkar5398@gmail.com> wrote:
> > > 
> > > > The debugfs_create_file_unsafe method does not protect the fops given to
> > > > it from file removal. It must be used with DEFINE_DEBUGFS_ATTRIBUTE
> > > > which makes the fops aware of the file lifetime.
> > > > 
> > > > Further using DEFINE_DEBUGFS_ATTRIBUTE along with
> > > > debugfs_create_file_unsafe significantly reduces the overhead introduced by
> > > > debugfs_create_file which creates a lifetime managing proxy around each
> > > > fops handed in. Refer [1] for more on this.
> > > > 
> > > > Fixes the following warnings reported by coccinelle:
> > > > drivers/iio/imu//adis16460.c:126:0-23: WARNING: adis16460_flash_count_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > > drivers/iio/imu//adis16460.c:108:0-23: WARNING: adis16460_product_id_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > > drivers/iio/imu//adis16460.c:90:0-23: WARNING: adis16460_serial_number_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > > drivers/iio/imu//adis16400.c:278:0-23: WARNING: adis16400_flash_count_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > > drivers/iio/imu//adis16400.c:261:0-23: WARNING: adis16400_product_id_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > > 
> > > > [1]: https://lists.gt.net/linux/kernel/2369498
> > > > 
> > > > Rohit Sarkar (2):
> > > >    iio: imu: adis16400: use DEFINE_DEBUGFS_ATTRIBUTE instead of
> > > >      DEFINE_SIMPLE_ATTRIBUTE
> > > >    iio: imu: adis16460: use DEFINE_DEBUGFS_ATTRIBUTE instead of
> > > >      DEFINE_SIMPLE_ATTRIBUTE
> > > > 
> > > >   drivers/iio/imu/adis16400.c | 4 ++--
> > > >   drivers/iio/imu/adis16460.c | 6 +++---
> > > >   2 files changed, 5 insertions(+), 5 deletions(-)
> > > > 
> > > Hi Rohit,
> > Hey,
> > > You've opened a can of worms with this one.  There as a previous series
> > > posted doing exactly this change back in 2019 by Zhong Jiang (cc'd)
> > > 
> > > At the time I did a bit of looking into why this had been universally taken
> > > up cross tree and turned out there are some potential issues.
> > > 
> > > Alexandru added it to the list of things to test, but I guess it got
> > > buried under other work and is still outstanding.
> > > 
> > > https://lkml.org/lkml/2019/10/30/144
> > Acc. to the patch by Zhong this change kind of comes off as a cosmetic
> > change as in the commit message he mentions "it is more clear".
> > 
> > But there is certainly more to it than that:
> > In the current scenario since we are using debugfs_create_file_unsafe
> > the file has no protection whatsoever against removal.
> 
> The drivers you are patching all use debugfs_create_file() as far as I can
> see.
Ah, You are right. I dont know why I assumed that
debugfs_create_file_unsafe was being used. Was probably sleepy when I
sent this out :/
> The way I understand it using DEFINE_DEBUGFS_ATTRIBUTE without switching to
> debugfs_create_file_unsafe() will not make a difference. There will only be
> more overhead since the files are protected twice.
That's right.
it should either be ...unsafe() with DEFINE_DEBUGFS_ATTRIBUTE or what it
is currently.

In the current scenario the file is protected against removal but there
is extra overhead because debugfs_create_file creates a lifetime
managing proxy AFAIK.

I can send a v2 changing the debugfs_create_file function to unsafe if
someone can test it out on h/w as Jonathan suggested.
> - Lars
> 
Thanks,
Rohit

Re: [PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Ardelean, Alexandru]

On Sun, 2020-03-29 at 10:38 +0100, Jonathan Cameron wrote:
> On Sat, 28 Mar 2020 12:04:53 +0530
> Rohit Sarkar <rohitsarkar5398@gmail.com> wrote:
> 
> > The debugfs_create_file_unsafe method does not protect the fops given to
> > it from file removal. It must be used with DEFINE_DEBUGFS_ATTRIBUTE
> > which makes the fops aware of the file lifetime.
> > 
> > Further using DEFINE_DEBUGFS_ATTRIBUTE along with
> > debugfs_create_file_unsafe significantly reduces the overhead introduced by
> > debugfs_create_file which creates a lifetime managing proxy around each
> > fops handed in. Refer [1] for more on this.
> > 
> > Fixes the following warnings reported by coccinelle:
> > drivers/iio/imu//adis16460.c:126:0-23: WARNING: adis16460_flash_count_fops
> > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > drivers/iio/imu//adis16460.c:108:0-23: WARNING: adis16460_product_id_fops
> > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > drivers/iio/imu//adis16460.c:90:0-23: WARNING: adis16460_serial_number_fops
> > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > drivers/iio/imu//adis16400.c:278:0-23: WARNING: adis16400_flash_count_fops
> > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > drivers/iio/imu//adis16400.c:261:0-23: WARNING: adis16400_product_id_fops
> > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > 
> > [1]: https://lists.gt.net/linux/kernel/2369498
> > 
> > Rohit Sarkar (2):
> >   iio: imu: adis16400: use DEFINE_DEBUGFS_ATTRIBUTE instead of
> >     DEFINE_SIMPLE_ATTRIBUTE
> >   iio: imu: adis16460: use DEFINE_DEBUGFS_ATTRIBUTE instead of
> >     DEFINE_SIMPLE_ATTRIBUTE
> > 
> >  drivers/iio/imu/adis16400.c | 4 ++--
> >  drivers/iio/imu/adis16460.c | 6 +++---
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> Hi Rohit,
> 
> You've opened a can of worms with this one.  There as a previous series
> posted doing exactly this change back in 2019 by Zhong Jiang (cc'd)
> 
> At the time I did a bit of looking into why this had been universally taken
> up cross tree and turned out there are some potential issues.
> 
> Alexandru added it to the list of things to test, but I guess it got
> buried under other work and is still outstanding.
> 

yep
my bad;
will try to make room these days for that old one


> https://lkml.org/lkml/2019/10/30/144
> 
> Has Greg KH raising the point that file reference counting is changed (as you
> mention) but that can cause subtle bugs.  It 'might' be fine but is
> definitely one that needs a tested-by from someone with the hardware.
> 
> Jonathan

RE: [PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Sa, Nuno]

Hi Rohit,

> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On
> Behalf Of Ardelean, Alexandru
> Sent: Montag, 30. März 2020 11:20
> To: jic23@kernel.org; rohitsarkar5398@gmail.com
> Cc: stefan.popa@analog.com; zhongjiang@huawei.com; linux-
> iio@vger.kernel.org; Bogdan, Dragos <Dragos.Bogdan@analog.com>;
> pmeerw@pmeerw.net; knaack.h@gmx.de; Hennerich, Michael
> <Michael.Hennerich@analog.com>; linux-kernel@vger.kernel.org;
> lars@metafoo.de
> Subject: Re: [PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of
> DEFINE_SIMPLE_ATTRIBUTE
> 
> On Sun, 2020-03-29 at 10:38 +0100, Jonathan Cameron wrote:
> > On Sat, 28 Mar 2020 12:04:53 +0530
> > Rohit Sarkar <rohitsarkar5398@gmail.com> wrote:
> >
> > > The debugfs_create_file_unsafe method does not protect the fops given to
> > > it from file removal. It must be used with DEFINE_DEBUGFS_ATTRIBUTE
> > > which makes the fops aware of the file lifetime.
> > >
> > > Further using DEFINE_DEBUGFS_ATTRIBUTE along with
> > > debugfs_create_file_unsafe significantly reduces the overhead introduced
> by
> > > debugfs_create_file which creates a lifetime managing proxy around each
> > > fops handed in. Refer [1] for more on this.
> > >
> > > Fixes the following warnings reported by coccinelle:
> > > drivers/iio/imu//adis16460.c:126:0-23: WARNING:
> adis16460_flash_count_fops
> > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > drivers/iio/imu//adis16460.c:108:0-23: WARNING:
> adis16460_product_id_fops
> > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > drivers/iio/imu//adis16460.c:90:0-23: WARNING:
> adis16460_serial_number_fops
> > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > drivers/iio/imu//adis16400.c:278:0-23: WARNING:
> adis16400_flash_count_fops
> > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > drivers/iio/imu//adis16400.c:261:0-23: WARNING:
> adis16400_product_id_fops
> > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > >
> > > [1]: https://lists.gt.net/linux/kernel/2369498
> > >
> > > Rohit Sarkar (2):
> > >   iio: imu: adis16400: use DEFINE_DEBUGFS_ATTRIBUTE instead of
> > >     DEFINE_SIMPLE_ATTRIBUTE
> > >   iio: imu: adis16460: use DEFINE_DEBUGFS_ATTRIBUTE instead of
> > >     DEFINE_SIMPLE_ATTRIBUTE
> > >
> > >  drivers/iio/imu/adis16400.c | 4 ++--
> > >  drivers/iio/imu/adis16460.c | 6 +++---
> > >  2 files changed, 5 insertions(+), 5 deletions(-)
> > >
> > Hi Rohit,
> >
> > You've opened a can of worms with this one.  There as a previous series
> > posted doing exactly this change back in 2019 by Zhong Jiang (cc'd)
> >
> > At the time I did a bit of looking into why this had been universally taken
> > up cross tree and turned out there are some potential issues.
> >
> > Alexandru added it to the list of things to test, but I guess it got
> > buried under other work and is still outstanding.
> >
> 
> yep
> my bad;
> will try to make room these days for that old one
> 
> 

I don't have the exact parts that this patch is touching but I have other parts where this patch
applies and should be same. So, the idea to test this is to read the files in debugfs? Maybe also
some unbind + binding?

I will try to test this still today...

- Nuno Sá
> > https://lkml.org/lkml/2019/10/30/144
> >
> > Has Greg KH raising the point that file reference counting is changed (as you
> > mention) but that can cause subtle bugs.  It 'might' be fine but is
> > definitely one that needs a tested-by from someone with the hardware.
> >
> > Jonathan

Re: [PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Lars-Peter Clausen]

On 3/31/20 12:58 PM, Sa, Nuno wrote:
> Hi Rohit,
>
>> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On
>> Behalf Of Ardelean, Alexandru
>> Sent: Montag, 30. März 2020 11:20
>> To: jic23@kernel.org; rohitsarkar5398@gmail.com
>> Cc: stefan.popa@analog.com; zhongjiang@huawei.com; linux-
>> iio@vger.kernel.org; Bogdan, Dragos <Dragos.Bogdan@analog.com>;
>> pmeerw@pmeerw.net; knaack.h@gmx.de; Hennerich, Michael
>> <Michael.Hennerich@analog.com>; linux-kernel@vger.kernel.org;
>> lars@metafoo.de
>> Subject: Re: [PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of
>> DEFINE_SIMPLE_ATTRIBUTE
>>
>> On Sun, 2020-03-29 at 10:38 +0100, Jonathan Cameron wrote:
>>> On Sat, 28 Mar 2020 12:04:53 +0530
>>> Rohit Sarkar <rohitsarkar5398@gmail.com> wrote:
>>>
>>>> The debugfs_create_file_unsafe method does not protect the fops given to
>>>> it from file removal. It must be used with DEFINE_DEBUGFS_ATTRIBUTE
>>>> which makes the fops aware of the file lifetime.
>>>>
>>>> Further using DEFINE_DEBUGFS_ATTRIBUTE along with
>>>> debugfs_create_file_unsafe significantly reduces the overhead introduced
>> by
>>>> debugfs_create_file which creates a lifetime managing proxy around each
>>>> fops handed in. Refer [1] for more on this.
>>>>
>>>> Fixes the following warnings reported by coccinelle:
>>>> drivers/iio/imu//adis16460.c:126:0-23: WARNING:
>> adis16460_flash_count_fops
>>>> should be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>>> drivers/iio/imu//adis16460.c:108:0-23: WARNING:
>> adis16460_product_id_fops
>>>> should be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>>> drivers/iio/imu//adis16460.c:90:0-23: WARNING:
>> adis16460_serial_number_fops
>>>> should be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>>> drivers/iio/imu//adis16400.c:278:0-23: WARNING:
>> adis16400_flash_count_fops
>>>> should be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>>> drivers/iio/imu//adis16400.c:261:0-23: WARNING:
>> adis16400_product_id_fops
>>>> should be defined with DEFINE_DEBUGFS_ATTRIBUTE
>>>>
>>>> [1]: https://lists.gt.net/linux/kernel/2369498
>>>>
>>>> Rohit Sarkar (2):
>>>>    iio: imu: adis16400: use DEFINE_DEBUGFS_ATTRIBUTE instead of
>>>>      DEFINE_SIMPLE_ATTRIBUTE
>>>>    iio: imu: adis16460: use DEFINE_DEBUGFS_ATTRIBUTE instead of
>>>>      DEFINE_SIMPLE_ATTRIBUTE
>>>>
>>>>   drivers/iio/imu/adis16400.c | 4 ++--
>>>>   drivers/iio/imu/adis16460.c | 6 +++---
>>>>   2 files changed, 5 insertions(+), 5 deletions(-)
>>>>
>>> Hi Rohit,
>>>
>>> You've opened a can of worms with this one.  There as a previous series
>>> posted doing exactly this change back in 2019 by Zhong Jiang (cc'd)
>>>
>>> At the time I did a bit of looking into why this had been universally taken
>>> up cross tree and turned out there are some potential issues.
>>>
>>> Alexandru added it to the list of things to test, but I guess it got
>>> buried under other work and is still outstanding.
>>>
>> yep
>> my bad;
>> will try to make room these days for that old one
>>
>>
> I don't have the exact parts that this patch is touching but I have other parts where this patch
> applies and should be same. So, the idea to test this is to read the files in debugfs? Maybe also
> some unbind + binding?
>
> I will try to test this still today...

The stress test is to open the debugfs file, then unbind the device and 
then read from the still open debugfs file.

- Lars

Re: [PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Rohit Sarkar]

On Tue, Mar 31, 2020 at 01:08:00PM +0200, Lars-Peter Clausen wrote:
> On 3/31/20 12:58 PM, Sa, Nuno wrote:
> > Hi Rohit,
> > 
> > > From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On
> > > Behalf Of Ardelean, Alexandru
> > > Sent: Montag, 30. März 2020 11:20
> > > To: jic23@kernel.org; rohitsarkar5398@gmail.com
> > > Cc: stefan.popa@analog.com; zhongjiang@huawei.com; linux-
> > > iio@vger.kernel.org; Bogdan, Dragos <Dragos.Bogdan@analog.com>;
> > > pmeerw@pmeerw.net; knaack.h@gmx.de; Hennerich, Michael
> > > <Michael.Hennerich@analog.com>; linux-kernel@vger.kernel.org;
> > > lars@metafoo.de
> > > Subject: Re: [PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of
> > > DEFINE_SIMPLE_ATTRIBUTE
> > > 
> > > On Sun, 2020-03-29 at 10:38 +0100, Jonathan Cameron wrote:
> > > > On Sat, 28 Mar 2020 12:04:53 +0530
> > > > Rohit Sarkar <rohitsarkar5398@gmail.com> wrote:
> > > > 
> > > > > The debugfs_create_file_unsafe method does not protect the fops given to
> > > > > it from file removal. It must be used with DEFINE_DEBUGFS_ATTRIBUTE
> > > > > which makes the fops aware of the file lifetime.
> > > > > 
> > > > > Further using DEFINE_DEBUGFS_ATTRIBUTE along with
> > > > > debugfs_create_file_unsafe significantly reduces the overhead introduced
> > > by
> > > > > debugfs_create_file which creates a lifetime managing proxy around each
> > > > > fops handed in. Refer [1] for more on this.
> > > > > 
> > > > > Fixes the following warnings reported by coccinelle:
> > > > > drivers/iio/imu//adis16460.c:126:0-23: WARNING:
> > > adis16460_flash_count_fops
> > > > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > > > drivers/iio/imu//adis16460.c:108:0-23: WARNING:
> > > adis16460_product_id_fops
> > > > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > > > drivers/iio/imu//adis16460.c:90:0-23: WARNING:
> > > adis16460_serial_number_fops
> > > > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > > > drivers/iio/imu//adis16400.c:278:0-23: WARNING:
> > > adis16400_flash_count_fops
> > > > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > > > drivers/iio/imu//adis16400.c:261:0-23: WARNING:
> > > adis16400_product_id_fops
> > > > > should be defined with DEFINE_DEBUGFS_ATTRIBUTE
> > > > > 
> > > > > [1]: https://lists.gt.net/linux/kernel/2369498
> > > > > 
> > > > > Rohit Sarkar (2):
> > > > >    iio: imu: adis16400: use DEFINE_DEBUGFS_ATTRIBUTE instead of
> > > > >      DEFINE_SIMPLE_ATTRIBUTE
> > > > >    iio: imu: adis16460: use DEFINE_DEBUGFS_ATTRIBUTE instead of
> > > > >      DEFINE_SIMPLE_ATTRIBUTE
> > > > > 
> > > > >   drivers/iio/imu/adis16400.c | 4 ++--
> > > > >   drivers/iio/imu/adis16460.c | 6 +++---
> > > > >   2 files changed, 5 insertions(+), 5 deletions(-)
> > > > > 
> > > > Hi Rohit,
> > > > 
> > > > You've opened a can of worms with this one.  There as a previous series
> > > > posted doing exactly this change back in 2019 by Zhong Jiang (cc'd)
> > > > 
> > > > At the time I did a bit of looking into why this had been universally taken
> > > > up cross tree and turned out there are some potential issues.
> > > > 
> > > > Alexandru added it to the list of things to test, but I guess it got
> > > > buried under other work and is still outstanding.
> > > > 
> > > yep
> > > my bad;
> > > will try to make room these days for that old one
> > > 
> > > 
> > I don't have the exact parts that this patch is touching but I have other parts where this patch
> > applies and should be same. So, the idea to test this is to read the files in debugfs? Maybe also
> > some unbind + binding?
> > 
> > I will try to test this still today...
> 
> The stress test is to open the debugfs file, then unbind the device and then
> read from the still open debugfs file.
Yes, also just to be sure, we need to test DEFINE_DEBUGFS_ATTRIBUTE
along with debugfs_create_file_unsafe. I will send out another patch
that changes debugfs_create_file to debugfs_create_file_unsafe and then
that can be tested.

> - Lars

Thanks,
Rohit

[PATCH v2 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Rohit Sarkar]

The debugfs_create_file_unsafe method does not protect the fops given to
it from file removal. It must be used with DEFINE_DEBUGFS_ATTRIBUTE
which makes the fops aware of the file lifetime.

Further using DEFINE_DEBUGFS_ATTRIBUTE along with
debugfs_create_file_unsafe significantly reduces the overhead introduced by
debugfs_create_file which creates a lifetime managing proxy around each
fops handed in. Refer [1] for more on this.

Fixes the following warnings reported by coccinelle:
drivers/iio/imu//adis16460.c:126:0-23: WARNING: adis16460_flash_count_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
drivers/iio/imu//adis16460.c:108:0-23: WARNING: adis16460_product_id_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
drivers/iio/imu//adis16460.c:90:0-23: WARNING: adis16460_serial_number_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
drivers/iio/imu//adis16400.c:278:0-23: WARNING: adis16400_flash_count_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE
drivers/iio/imu//adis16400.c:261:0-23: WARNING: adis16400_product_id_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE

[1]: https://lists.gt.net/linux/kernel/2369498

Changelog v1 -> v2
* Use debugfs_create_file_unsafe instead of debugfs_create_file

Rohit Sarkar (2):
  iio: imu: adis16400: use DEFINE_DEBUGFS_ATTRIBUTE instead of
    DEFINE_SIMPLE_ATTRIBUTE
  iio: imu: adis16460: use DEFINE_DEBUGFS_ATTRIBUTE instead of
    DEFINE_SIMPLE_ATTRIBUTE

 drivers/iio/imu/adis16400.c | 19 ++++++++++---------
 drivers/iio/imu/adis16460.c | 27 +++++++++++++++------------
 2 files changed, 25 insertions(+), 21 deletions(-)

-- 
2.23.0.385.gbc12974a89

[PATCH v2 1/2] iio: imu: adis16400: use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Rohit Sarkar]

debugfs_create_file_unsafe does not protect the fops handed to it
against file removal. DEFINE_DEBUGFS_ATTRIBUTE makes the fops aware of
the file lifetime and thus protects it against removal.

Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
---
 drivers/iio/imu/adis16400.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
index cfb1c19eb930..c8fcd40f58c0 100644
--- a/drivers/iio/imu/adis16400.c
+++ b/drivers/iio/imu/adis16400.c
@@ -258,7 +258,7 @@ static int adis16400_show_product_id(void *arg, u64 *val)
 
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(adis16400_product_id_fops,
+DEFINE_DEBUGFS_ATTRIBUTE(adis16400_product_id_fops,
 	adis16400_show_product_id, NULL, "%lld\n");
 
 static int adis16400_show_flash_count(void *arg, u64 *val)
@@ -275,7 +275,7 @@ static int adis16400_show_flash_count(void *arg, u64 *val)
 
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(adis16400_flash_count_fops,
+DEFINE_DEBUGFS_ATTRIBUTE(adis16400_flash_count_fops,
 	adis16400_show_flash_count, NULL, "%lld\n");
 
 static int adis16400_debugfs_init(struct iio_dev *indio_dev)
@@ -283,15 +283,16 @@ static int adis16400_debugfs_init(struct iio_dev *indio_dev)
 	struct adis16400_state *st = iio_priv(indio_dev);
 
 	if (st->variant->flags & ADIS16400_HAS_SERIAL_NUMBER)
-		debugfs_create_file("serial_number", 0400,
-			indio_dev->debugfs_dentry, st,
-			&adis16400_serial_number_fops);
+		debugfs_create_file_unsafe("serial_number", 0400,
+				indio_dev->debugfs_dentry, st,
+				&adis16400_serial_number_fops);
 	if (st->variant->flags & ADIS16400_HAS_PROD_ID)
-		debugfs_create_file("product_id", 0400,
+		debugfs_create_file_unsafe("product_id", 0400,
+				indio_dev->debugfs_dentry, st,
+				&adis16400_product_id_fops);
+	debugfs_create_file_unsafe("flash_count", 0400,
 			indio_dev->debugfs_dentry, st,
-			&adis16400_product_id_fops);
-	debugfs_create_file("flash_count", 0400, indio_dev->debugfs_dentry,
-		st, &adis16400_flash_count_fops);
+			&adis16400_flash_count_fops);
 
 	return 0;
 }
-- 
2.23.0.385.gbc12974a89

[PATCH v2 2/2] iio: imu: adis16460: use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Rohit Sarkar]

debugfs_create_file_unsafe does not protect the fops handed to it
against file removal. DEFINE_DEBUGFS_ATTRIBUTE makes the fops aware of
the file lifetime and thus protects it against removal.

Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
---
 drivers/iio/imu/adis16460.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/imu/adis16460.c b/drivers/iio/imu/adis16460.c
index 9539cfe4a259..f96cfd007957 100644
--- a/drivers/iio/imu/adis16460.c
+++ b/drivers/iio/imu/adis16460.c
@@ -87,8 +87,8 @@ static int adis16460_show_serial_number(void *arg, u64 *val)
 
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(adis16460_serial_number_fops,
-	adis16460_show_serial_number, NULL, "0x%.4llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(adis16460_serial_number_fops,
+		adis16460_show_serial_number, NULL, "0x%.4llx\n");
 
 static int adis16460_show_product_id(void *arg, u64 *val)
 {
@@ -105,8 +105,8 @@ static int adis16460_show_product_id(void *arg, u64 *val)
 
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(adis16460_product_id_fops,
-	adis16460_show_product_id, NULL, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(adis16460_product_id_fops,
+		adis16460_show_product_id, NULL, "%llu\n");
 
 static int adis16460_show_flash_count(void *arg, u64 *val)
 {
@@ -123,19 +123,22 @@ static int adis16460_show_flash_count(void *arg, u64 *val)
 
 	return 0;
 }
-DEFINE_SIMPLE_ATTRIBUTE(adis16460_flash_count_fops,
-	adis16460_show_flash_count, NULL, "%lld\n");
+DEFINE_DEBUGFS_ATTRIBUTE(adis16460_flash_count_fops,
+		adis16460_show_flash_count, NULL, "%lld\n");
 
 static int adis16460_debugfs_init(struct iio_dev *indio_dev)
 {
 	struct adis16460 *adis16460 = iio_priv(indio_dev);
 
-	debugfs_create_file("serial_number", 0400, indio_dev->debugfs_dentry,
-		adis16460, &adis16460_serial_number_fops);
-	debugfs_create_file("product_id", 0400, indio_dev->debugfs_dentry,
-		adis16460, &adis16460_product_id_fops);
-	debugfs_create_file("flash_count", 0400, indio_dev->debugfs_dentry,
-		adis16460, &adis16460_flash_count_fops);
+	debugfs_create_file_unsafe("serial_number", 0400,
+			indio_dev->debugfs_dentry, adis16460,
+			&adis16460_serial_number_fops);
+	debugfs_create_file_unsafe("product_id", 0400,
+			indio_dev->debugfs_dentry, adis16460,
+			&adis16460_product_id_fops);
+	debugfs_create_file_unsafe("flash_count", 0400,
+			indio_dev->debugfs_dentry, adis16460,
+			&adis16460_flash_count_fops);
 
 	return 0;
 }
-- 
2.23.0.385.gbc12974a89

Re: [PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Rohit Sarkar]

Hey Nuno,
> > > > 
> > > I don't have the exact parts that this patch is touching but I have other parts where this patch
> > > applies and should be same. So, the idea to test this is to read the files in debugfs? Maybe also
> > > some unbind + binding?
> > > 
> > > I will try to test this still today...
> > 
> > The stress test is to open the debugfs file, then unbind the device and then
> > read from the still open debugfs file.
> Yes, also just to be sure, we need to test DEFINE_DEBUGFS_ATTRIBUTE
> along with debugfs_create_file_unsafe. I will send out another patch
> that changes debugfs_create_file to debugfs_create_file_unsafe and then
> that can be tested.
Have sent out a v2 that is ready to be tested
> > - Lars
> 
> Thanks,
> Rohit
Thanks,
Rohit

RE: [PATCH v2 1/2] iio: imu: adis16400: use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Sa, Nuno]

> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On
> Behalf Of Rohit Sarkar
> Sent: Dienstag, 31. März 2020 13:48
> To: linux-iio@vger.kernel.org
> Cc: Bogdan, Dragos <Dragos.Bogdan@analog.com>; Rohit Sarkar
> <rohitsarkar5398@gmail.com>; Lars-Peter Clausen <lars@metafoo.de>;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Stefan Popa
> <stefan.popa@analog.com>; Jonathan Cameron <jic23@kernel.org>; Hartmut
> Knaack <knaack.h@gmx.de>; Peter Meerwald-Stadler
> <pmeerw@pmeerw.net>; linux-kernel@vger.kernel.org
> Subject: [PATCH v2 1/2] iio: imu: adis16400: use
> DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE
> 
> debugfs_create_file_unsafe does not protect the fops handed to it
> against file removal. DEFINE_DEBUGFS_ATTRIBUTE makes the fops aware of
> the file lifetime and thus protects it against removal.
> 
> Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
> ---
>  drivers/iio/imu/adis16400.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
> index cfb1c19eb930..c8fcd40f58c0 100644
> --- a/drivers/iio/imu/adis16400.c
> +++ b/drivers/iio/imu/adis16400.c
> @@ -258,7 +258,7 @@ static int adis16400_show_product_id(void *arg, u64
> *val)
> 
>  	return 0;
>  }
> -DEFINE_SIMPLE_ATTRIBUTE(adis16400_product_id_fops,
> +DEFINE_DEBUGFS_ATTRIBUTE(adis16400_product_id_fops,
>  	adis16400_show_product_id, NULL, "%lld\n");
> 
>  static int adis16400_show_flash_count(void *arg, u64 *val)
> @@ -275,7 +275,7 @@ static int adis16400_show_flash_count(void *arg, u64
> *val)
> 
>  	return 0;
>  }
> -DEFINE_SIMPLE_ATTRIBUTE(adis16400_flash_count_fops,
> +DEFINE_DEBUGFS_ATTRIBUTE(adis16400_flash_count_fops,
>  	adis16400_show_flash_count, NULL, "%lld\n");
> 
>  static int adis16400_debugfs_init(struct iio_dev *indio_dev)
> @@ -283,15 +283,16 @@ static int adis16400_debugfs_init(struct iio_dev
> *indio_dev)
>  	struct adis16400_state *st = iio_priv(indio_dev);
> 
>  	if (st->variant->flags & ADIS16400_HAS_SERIAL_NUMBER)
> -		debugfs_create_file("serial_number", 0400,
> -			indio_dev->debugfs_dentry, st,
> -			&adis16400_serial_number_fops);
> +		debugfs_create_file_unsafe("serial_number", 0400,
> +				indio_dev->debugfs_dentry, st,
> +				&adis16400_serial_number_fops);
>  	if (st->variant->flags & ADIS16400_HAS_PROD_ID)
> -		debugfs_create_file("product_id", 0400,
> +		debugfs_create_file_unsafe("product_id", 0400,
> +				indio_dev->debugfs_dentry, st,
> +				&adis16400_product_id_fops);
> +	debugfs_create_file_unsafe("flash_count", 0400,
>  			indio_dev->debugfs_dentry, st,
> -			&adis16400_product_id_fops);
> -	debugfs_create_file("flash_count", 0400, indio_dev->debugfs_dentry,
> -		st, &adis16400_flash_count_fops);
> +			&adis16400_flash_count_fops);
> 
>  	return 0;
>  }

Tested-by Nuno Sá <nuno.sa@analog.com>

> --
> 2.23.0.385.gbc12974a89

RE: [PATCH v2 2/2] iio: imu: adis16460: use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Sa, Nuno]

> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On
> Behalf Of Rohit Sarkar
> Sent: Dienstag, 31. März 2020 13:48
> To: linux-iio@vger.kernel.org
> Cc: Bogdan, Dragos <Dragos.Bogdan@analog.com>; Rohit Sarkar
> <rohitsarkar5398@gmail.com>; Lars-Peter Clausen <lars@metafoo.de>;
> Hennerich, Michael <Michael.Hennerich@analog.com>; Stefan Popa
> <stefan.popa@analog.com>; Jonathan Cameron <jic23@kernel.org>; Hartmut
> Knaack <knaack.h@gmx.de>; Peter Meerwald-Stadler
> <pmeerw@pmeerw.net>; linux-kernel@vger.kernel.org
> Subject: [PATCH v2 2/2] iio: imu: adis16460: use
> DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE
> 
> debugfs_create_file_unsafe does not protect the fops handed to it
> against file removal. DEFINE_DEBUGFS_ATTRIBUTE makes the fops aware of
> the file lifetime and thus protects it against removal.
> 
> Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
> ---
>  drivers/iio/imu/adis16460.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16460.c b/drivers/iio/imu/adis16460.c
> index 9539cfe4a259..f96cfd007957 100644
> --- a/drivers/iio/imu/adis16460.c
> +++ b/drivers/iio/imu/adis16460.c
> @@ -87,8 +87,8 @@ static int adis16460_show_serial_number(void *arg,
> u64 *val)
> 
>  	return 0;
>  }
> -DEFINE_SIMPLE_ATTRIBUTE(adis16460_serial_number_fops,
> -	adis16460_show_serial_number, NULL, "0x%.4llx\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_serial_number_fops,
> +		adis16460_show_serial_number, NULL, "0x%.4llx\n");
> 
>  static int adis16460_show_product_id(void *arg, u64 *val)
>  {
> @@ -105,8 +105,8 @@ static int adis16460_show_product_id(void *arg, u64
> *val)
> 
>  	return 0;
>  }
> -DEFINE_SIMPLE_ATTRIBUTE(adis16460_product_id_fops,
> -	adis16460_show_product_id, NULL, "%llu\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_product_id_fops,
> +		adis16460_show_product_id, NULL, "%llu\n");
> 
>  static int adis16460_show_flash_count(void *arg, u64 *val)
>  {
> @@ -123,19 +123,22 @@ static int adis16460_show_flash_count(void *arg,
> u64 *val)
> 
>  	return 0;
>  }
> -DEFINE_SIMPLE_ATTRIBUTE(adis16460_flash_count_fops,
> -	adis16460_show_flash_count, NULL, "%lld\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_flash_count_fops,
> +		adis16460_show_flash_count, NULL, "%lld\n");
> 
>  static int adis16460_debugfs_init(struct iio_dev *indio_dev)
>  {
>  	struct adis16460 *adis16460 = iio_priv(indio_dev);
> 
> -	debugfs_create_file("serial_number", 0400, indio_dev-
> >debugfs_dentry,
> -		adis16460, &adis16460_serial_number_fops);
> -	debugfs_create_file("product_id", 0400, indio_dev->debugfs_dentry,
> -		adis16460, &adis16460_product_id_fops);
> -	debugfs_create_file("flash_count", 0400, indio_dev->debugfs_dentry,
> -		adis16460, &adis16460_flash_count_fops);
> +	debugfs_create_file_unsafe("serial_number", 0400,
> +			indio_dev->debugfs_dentry, adis16460,
> +			&adis16460_serial_number_fops);
> +	debugfs_create_file_unsafe("product_id", 0400,
> +			indio_dev->debugfs_dentry, adis16460,
> +			&adis16460_product_id_fops);
> +	debugfs_create_file_unsafe("flash_count", 0400,
> +			indio_dev->debugfs_dentry, adis16460,
> +			&adis16460_flash_count_fops);
> 
>  	return 0;
>  }

Tested-by Nuno Sá <nuno.sa@analog.com>

> --
> 2.23.0.385.gbc12974a89

RE: [PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Sa, Nuno]

> From: Rohit Sarkar <rohitsarkar5398@gmail.com>
> Sent: Dienstag, 31. März 2020 13:50
> To: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Sa, Nuno <Nuno.Sa@analog.com>; Ardelean, Alexandru
> <alexandru.Ardelean@analog.com>; jic23@kernel.org;
> stefan.popa@analog.com; zhongjiang@huawei.com; linux-
> iio@vger.kernel.org; Bogdan, Dragos <Dragos.Bogdan@analog.com>;
> pmeerw@pmeerw.net; knaack.h@gmx.de; Hennerich, Michael
> <Michael.Hennerich@analog.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 0/2] use DEFINE_DEBUGFS_ATTRIBUTE instead of
> DEFINE_SIMPLE_ATTRIBUTE
> 
> Hey Nuno,
> > > > >
> > > > I don't have the exact parts that this patch is touching but I have other
> parts where this patch
> > > > applies and should be same. So, the idea to test this is to read the files in
> debugfs? Maybe also
> > > > some unbind + binding?
> > > >
> > > > I will try to test this still today...
> > >
> > > The stress test is to open the debugfs file, then unbind the device and then
> > > read from the still open debugfs file.
> > Yes, also just to be sure, we need to test DEFINE_DEBUGFS_ATTRIBUTE
> > along with debugfs_create_file_unsafe. I will send out another patch
> > that changes debugfs_create_file to debugfs_create_file_unsafe and then
> > that can be tested.
> Have sent out a v2 that is ready to be tested

So I tested your changes in a different HW but the principle is the same.
It worked as expected. Unbinding the device and doing a read on an opened file
descriptor afterwards returns error...

- Nuno Sá
> > > - Lars
> >
> > Thanks,
> > Rohit
> Thanks,
> Rohit

Re: [PATCH v2 1/2] iio: imu: adis16400: use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Jonathan Cameron]

On Tue, 31 Mar 2020 15:28:03 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On
> > Behalf Of Rohit Sarkar
> > Sent: Dienstag, 31. März 2020 13:48
> > To: linux-iio@vger.kernel.org
> > Cc: Bogdan, Dragos <Dragos.Bogdan@analog.com>; Rohit Sarkar
> > <rohitsarkar5398@gmail.com>; Lars-Peter Clausen <lars@metafoo.de>;
> > Hennerich, Michael <Michael.Hennerich@analog.com>; Stefan Popa
> > <stefan.popa@analog.com>; Jonathan Cameron <jic23@kernel.org>; Hartmut
> > Knaack <knaack.h@gmx.de>; Peter Meerwald-Stadler
> > <pmeerw@pmeerw.net>; linux-kernel@vger.kernel.org
> > Subject: [PATCH v2 1/2] iio: imu: adis16400: use
> > DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE
> > 
> > debugfs_create_file_unsafe does not protect the fops handed to it
> > against file removal. DEFINE_DEBUGFS_ATTRIBUTE makes the fops aware of
> > the file lifetime and thus protects it against removal.
> > 
> > Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
> > ---
> >  drivers/iio/imu/adis16400.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
> > index cfb1c19eb930..c8fcd40f58c0 100644
> > --- a/drivers/iio/imu/adis16400.c
> > +++ b/drivers/iio/imu/adis16400.c
> > @@ -258,7 +258,7 @@ static int adis16400_show_product_id(void *arg, u64
> > *val)
> > 
> >  	return 0;
> >  }
> > -DEFINE_SIMPLE_ATTRIBUTE(adis16400_product_id_fops,
> > +DEFINE_DEBUGFS_ATTRIBUTE(adis16400_product_id_fops,
> >  	adis16400_show_product_id, NULL, "%lld\n");
> > 
> >  static int adis16400_show_flash_count(void *arg, u64 *val)
> > @@ -275,7 +275,7 @@ static int adis16400_show_flash_count(void *arg, u64
> > *val)
> > 
> >  	return 0;
> >  }
> > -DEFINE_SIMPLE_ATTRIBUTE(adis16400_flash_count_fops,
> > +DEFINE_DEBUGFS_ATTRIBUTE(adis16400_flash_count_fops,
> >  	adis16400_show_flash_count, NULL, "%lld\n");
> > 
> >  static int adis16400_debugfs_init(struct iio_dev *indio_dev)
> > @@ -283,15 +283,16 @@ static int adis16400_debugfs_init(struct iio_dev
> > *indio_dev)
> >  	struct adis16400_state *st = iio_priv(indio_dev);
> > 
> >  	if (st->variant->flags & ADIS16400_HAS_SERIAL_NUMBER)
> > -		debugfs_create_file("serial_number", 0400,
> > -			indio_dev->debugfs_dentry, st,
> > -			&adis16400_serial_number_fops);
> > +		debugfs_create_file_unsafe("serial_number", 0400,
> > +				indio_dev->debugfs_dentry, st,
> > +				&adis16400_serial_number_fops);
> >  	if (st->variant->flags & ADIS16400_HAS_PROD_ID)
> > -		debugfs_create_file("product_id", 0400,
> > +		debugfs_create_file_unsafe("product_id", 0400,
> > +				indio_dev->debugfs_dentry, st,
> > +				&adis16400_product_id_fops);
> > +	debugfs_create_file_unsafe("flash_count", 0400,
> >  			indio_dev->debugfs_dentry, st,
> > -			&adis16400_product_id_fops);
> > -	debugfs_create_file("flash_count", 0400, indio_dev->debugfs_dentry,
> > -		st, &adis16400_flash_count_fops);
> > +			&adis16400_flash_count_fops);
> > 
> >  	return 0;
> >  }  
> 
> Tested-by Nuno Sá <nuno.sa@analog.com>
Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to try it out.

Thanks,

Jonathan

> 
> > --
> > 2.23.0.385.gbc12974a89  
> 

Re: [PATCH v2 2/2] iio: imu: adis16460: use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE

[Jonathan Cameron]

On Tue, 31 Mar 2020 15:28:22 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On
> > Behalf Of Rohit Sarkar
> > Sent: Dienstag, 31. März 2020 13:48
> > To: linux-iio@vger.kernel.org
> > Cc: Bogdan, Dragos <Dragos.Bogdan@analog.com>; Rohit Sarkar
> > <rohitsarkar5398@gmail.com>; Lars-Peter Clausen <lars@metafoo.de>;
> > Hennerich, Michael <Michael.Hennerich@analog.com>; Stefan Popa
> > <stefan.popa@analog.com>; Jonathan Cameron <jic23@kernel.org>; Hartmut
> > Knaack <knaack.h@gmx.de>; Peter Meerwald-Stadler
> > <pmeerw@pmeerw.net>; linux-kernel@vger.kernel.org
> > Subject: [PATCH v2 2/2] iio: imu: adis16460: use
> > DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE
> > 
> > debugfs_create_file_unsafe does not protect the fops handed to it
> > against file removal. DEFINE_DEBUGFS_ATTRIBUTE makes the fops aware of
> > the file lifetime and thus protects it against removal.
> > 
> > Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
> > ---
> >  drivers/iio/imu/adis16460.c | 27 +++++++++++++++------------
> >  1 file changed, 15 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/adis16460.c b/drivers/iio/imu/adis16460.c
> > index 9539cfe4a259..f96cfd007957 100644
> > --- a/drivers/iio/imu/adis16460.c
> > +++ b/drivers/iio/imu/adis16460.c
> > @@ -87,8 +87,8 @@ static int adis16460_show_serial_number(void *arg,
> > u64 *val)
> > 
> >  	return 0;
> >  }
> > -DEFINE_SIMPLE_ATTRIBUTE(adis16460_serial_number_fops,
> > -	adis16460_show_serial_number, NULL, "0x%.4llx\n");
> > +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_serial_number_fops,
> > +		adis16460_show_serial_number, NULL, "0x%.4llx\n");
> > 
> >  static int adis16460_show_product_id(void *arg, u64 *val)
> >  {
> > @@ -105,8 +105,8 @@ static int adis16460_show_product_id(void *arg, u64
> > *val)
> > 
> >  	return 0;
> >  }
> > -DEFINE_SIMPLE_ATTRIBUTE(adis16460_product_id_fops,
> > -	adis16460_show_product_id, NULL, "%llu\n");
> > +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_product_id_fops,
> > +		adis16460_show_product_id, NULL, "%llu\n");
> > 
> >  static int adis16460_show_flash_count(void *arg, u64 *val)
> >  {
> > @@ -123,19 +123,22 @@ static int adis16460_show_flash_count(void *arg,
> > u64 *val)
> > 
> >  	return 0;
> >  }
> > -DEFINE_SIMPLE_ATTRIBUTE(adis16460_flash_count_fops,
> > -	adis16460_show_flash_count, NULL, "%lld\n");
> > +DEFINE_DEBUGFS_ATTRIBUTE(adis16460_flash_count_fops,
> > +		adis16460_show_flash_count, NULL, "%lld\n");
> > 
> >  static int adis16460_debugfs_init(struct iio_dev *indio_dev)
> >  {
> >  	struct adis16460 *adis16460 = iio_priv(indio_dev);
> > 
> > -	debugfs_create_file("serial_number", 0400, indio_dev-  
> > >debugfs_dentry,  
> > -		adis16460, &adis16460_serial_number_fops);
> > -	debugfs_create_file("product_id", 0400, indio_dev->debugfs_dentry,
> > -		adis16460, &adis16460_product_id_fops);
> > -	debugfs_create_file("flash_count", 0400, indio_dev->debugfs_dentry,
> > -		adis16460, &adis16460_flash_count_fops);
> > +	debugfs_create_file_unsafe("serial_number", 0400,
> > +			indio_dev->debugfs_dentry, adis16460,
> > +			&adis16460_serial_number_fops);
> > +	debugfs_create_file_unsafe("product_id", 0400,
> > +			indio_dev->debugfs_dentry, adis16460,
> > +			&adis16460_product_id_fops);
> > +	debugfs_create_file_unsafe("flash_count", 0400,
> > +			indio_dev->debugfs_dentry, adis16460,
> > +			&adis16460_flash_count_fops);
> > 
> >  	return 0;
> >  }  
> 
> Tested-by Nuno Sá <nuno.sa@analog.com>
Applied.  Thanks,

Jonathan

> 
> > --
> > 2.23.0.385.gbc12974a89  
>