| Message ID | 20200328063456.24012-1-rohitsarkar5398@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | use DEFINE_DEBUGFS_ATTRIBUTE instead of DEFINE_SIMPLE_ATTRIBUTE | expand |
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
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
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
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
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
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
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
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
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: 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
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(-)