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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Sat, 28 Mar 2020 12:04:53 +0530
Rohit Sarkar <[email protected]> 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
On Sun, Mar 29, 2020 at 10:38:18AM +0100, Jonathan Cameron wrote:
> On Sat, 28 Mar 2020 12:04:53 +0530
> Rohit Sarkar <[email protected]> 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
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 <[email protected]> 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
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 <[email protected]> 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
On Sun, 2020-03-29 at 10:38 +0100, Jonathan Cameron wrote:
> On Sat, 28 Mar 2020 12:04:53 +0530
> Rohit Sarkar <[email protected]> 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
Hi Rohit,
> From: [email protected] <[email protected]> On
> Behalf Of Ardelean, Alexandru
> Sent: Montag, 30. März 2020 11:20
> To: [email protected]; [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]; Bogdan, Dragos <[email protected]>;
> [email protected]; [email protected]; Hennerich, Michael
> <[email protected]>; [email protected];
> [email protected]
> 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 <[email protected]> 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
On 3/31/20 12:58 PM, Sa, Nuno wrote:
> Hi Rohit,
>
>> From: [email protected] <[email protected]> On
>> Behalf Of Ardelean, Alexandru
>> Sent: Montag, 30. März 2020 11:20
>> To: [email protected]; [email protected]
>> Cc: [email protected]; [email protected]; linux-
>> [email protected]; Bogdan, Dragos <[email protected]>;
>> [email protected]; [email protected]; Hennerich, Michael
>> <[email protected]>; [email protected];
>> [email protected]
>> 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 <[email protected]> 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
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: [email protected] <[email protected]> On
> > > Behalf Of Ardelean, Alexandru
> > > Sent: Montag, 30. M?rz 2020 11:20
> > > To: [email protected]; [email protected]
> > > Cc: [email protected]; [email protected]; linux-
> > > [email protected]; Bogdan, Dragos <[email protected]>;
> > > [email protected]; [email protected]; Hennerich, Michael
> > > <[email protected]>; [email protected];
> > > [email protected]
> > > 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 <[email protected]> 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
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
> From: [email protected] <[email protected]> On
> Behalf Of Rohit Sarkar
> Sent: Dienstag, 31. M?rz 2020 13:48
> To: [email protected]
> Cc: Bogdan, Dragos <[email protected]>; Rohit Sarkar
> <[email protected]>; Lars-Peter Clausen <[email protected]>;
> Hennerich, Michael <[email protected]>; Stefan Popa
> <[email protected]>; Jonathan Cameron <[email protected]>; Hartmut
> Knaack <[email protected]>; Peter Meerwald-Stadler
> <[email protected]>; [email protected]
> 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 <[email protected]>
> ---
> 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? <[email protected]>
> --
> 2.23.0.385.gbc12974a89
> From: [email protected] <[email protected]> On
> Behalf Of Rohit Sarkar
> Sent: Dienstag, 31. M?rz 2020 13:48
> To: [email protected]
> Cc: Bogdan, Dragos <[email protected]>; Rohit Sarkar
> <[email protected]>; Lars-Peter Clausen <[email protected]>;
> Hennerich, Michael <[email protected]>; Stefan Popa
> <[email protected]>; Jonathan Cameron <[email protected]>; Hartmut
> Knaack <[email protected]>; Peter Meerwald-Stadler
> <[email protected]>; [email protected]
> 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 <[email protected]>
> ---
> 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? <[email protected]>
> --
> 2.23.0.385.gbc12974a89
> From: Rohit Sarkar <[email protected]>
> Sent: Dienstag, 31. M?rz 2020 13:50
> To: Lars-Peter Clausen <[email protected]>
> Cc: Sa, Nuno <[email protected]>; Ardelean, Alexandru
> <[email protected]>; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; Bogdan, Dragos <[email protected]>;
> [email protected]; [email protected]; Hennerich, Michael
> <[email protected]>; [email protected]
> 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
On Tue, 31 Mar 2020 15:28:03 +0000
"Sa, Nuno" <[email protected]> wrote:
> > From: [email protected] <[email protected]> On
> > Behalf Of Rohit Sarkar
> > Sent: Dienstag, 31. März 2020 13:48
> > To: [email protected]
> > Cc: Bogdan, Dragos <[email protected]>; Rohit Sarkar
> > <[email protected]>; Lars-Peter Clausen <[email protected]>;
> > Hennerich, Michael <[email protected]>; Stefan Popa
> > <[email protected]>; Jonathan Cameron <[email protected]>; Hartmut
> > Knaack <[email protected]>; Peter Meerwald-Stadler
> > <[email protected]>; [email protected]
> > 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 <[email protected]>
> > ---
> > 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á <[email protected]>
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
>
On Tue, 31 Mar 2020 15:28:22 +0000
"Sa, Nuno" <[email protected]> wrote:
> > From: [email protected] <[email protected]> On
> > Behalf Of Rohit Sarkar
> > Sent: Dienstag, 31. März 2020 13:48
> > To: [email protected]
> > Cc: Bogdan, Dragos <[email protected]>; Rohit Sarkar
> > <[email protected]>; Lars-Peter Clausen <[email protected]>;
> > Hennerich, Michael <[email protected]>; Stefan Popa
> > <[email protected]>; Jonathan Cameron <[email protected]>; Hartmut
> > Knaack <[email protected]>; Peter Meerwald-Stadler
> > <[email protected]>; [email protected]
> > 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 <[email protected]>
> > ---
> > 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á <[email protected]>
Applied. Thanks,
Jonathan
>
> > --
> > 2.23.0.385.gbc12974a89
>