* [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe() @ 2020-10-30 3:22 Deepak R Varma 2020-10-30 7:11 ` [Outreachy kernel] " Greg KH 0 siblings, 1 reply; 14+ messages in thread From: Deepak R Varma @ 2020-10-30 3:22 UTC (permalink / raw) To: outreachy-kernel, Alex Deucher, Christian König, David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel Cc: mh12gx2825, melissa.srw, daniel.vetter Using DEFINE_DEBUGFS_ATTRIBUTE macro with debugfs_create_file_unsafe() function in place of the debugfs_create_file() function will make the file operation struct "reset" aware of the file's lifetime. Additional details here: https://lists.archive.carbon60.com/linux/kernel/2369498 Issue reported by Coccinelle script: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com> --- Please Note: This is a Outreachy project task patch. drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c index 2d125b8b15ee..f076b1ba7319 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c @@ -1551,29 +1551,29 @@ static int amdgpu_debugfs_sclk_set(void *data, u64 val) return 0; } -DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL, - amdgpu_debugfs_ib_preempt, "%llu\n"); +DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL, + amdgpu_debugfs_ib_preempt, "%llu\n"); -DEFINE_SIMPLE_ATTRIBUTE(fops_sclk_set, NULL, - amdgpu_debugfs_sclk_set, "%llu\n"); +DEFINE_DEBUGFS_ATTRIBUTE(fops_sclk_set, NULL, + amdgpu_debugfs_sclk_set, "%llu\n"); int amdgpu_debugfs_init(struct amdgpu_device *adev) { int r, i; adev->debugfs_preempt = - debugfs_create_file("amdgpu_preempt_ib", 0600, - adev_to_drm(adev)->primary->debugfs_root, adev, - &fops_ib_preempt); + debugfs_create_file_unsafe("amdgpu_preempt_ib", 0600, + adev_to_drm(adev)->primary->debugfs_root, adev, + &fops_ib_preempt); if (!(adev->debugfs_preempt)) { DRM_ERROR("unable to create amdgpu_preempt_ib debugsfs file\n"); return -EIO; } adev->smu.debugfs_sclk = - debugfs_create_file("amdgpu_force_sclk", 0200, - adev_to_drm(adev)->primary->debugfs_root, adev, - &fops_sclk_set); + debugfs_create_file_unsafe("amdgpu_force_sclk", 0200, + adev_to_drm(adev)->primary->debugfs_root, adev, + &fops_sclk_set); if (!(adev->smu.debugfs_sclk)) { DRM_ERROR("unable to create amdgpu_set_sclk debugsfs file\n"); return -EIO; -- 2.25.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Outreachy kernel] [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe() 2020-10-30 3:22 [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe() Deepak R Varma @ 2020-10-30 7:11 ` Greg KH 2020-10-30 7:57 ` Deepak R Varma 0 siblings, 1 reply; 14+ messages in thread From: Greg KH @ 2020-10-30 7:11 UTC (permalink / raw) To: Deepak R Varma Cc: outreachy-kernel, Alex Deucher, Christian König, David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel, melissa.srw, daniel.vetter On Fri, Oct 30, 2020 at 08:52:45AM +0530, Deepak R Varma wrote: > Using DEFINE_DEBUGFS_ATTRIBUTE macro with debugfs_create_file_unsafe() > function in place of the debugfs_create_file() function will make the > file operation struct "reset" aware of the file's lifetime. Additional > details here: https://lists.archive.carbon60.com/linux/kernel/2369498 > > Issue reported by Coccinelle script: > scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com> > --- > Please Note: This is a Outreachy project task patch. > > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > index 2d125b8b15ee..f076b1ba7319 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > @@ -1551,29 +1551,29 @@ static int amdgpu_debugfs_sclk_set(void *data, u64 val) > return 0; > } > > -DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL, > - amdgpu_debugfs_ib_preempt, "%llu\n"); > +DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL, > + amdgpu_debugfs_ib_preempt, "%llu\n"); Are you sure this is ok? Do these devices need this additional "protection"? Do they have the problem that these macros were written for? Same for the other patches you just submitted here, I think you need to somehow "prove" that these changes are necessary, checkpatch isn't able to determine this all the time. thanks, greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Outreachy kernel] [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe() 2020-10-30 7:11 ` [Outreachy kernel] " Greg KH @ 2020-10-30 7:57 ` Deepak R Varma 2020-10-30 8:00 ` Christian König 2020-10-30 8:03 ` Greg KH 0 siblings, 2 replies; 14+ messages in thread From: Deepak R Varma @ 2020-10-30 7:57 UTC (permalink / raw) To: Greg KH Cc: outreachy-kernel, Alex Deucher, Christian König, David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel, melissa.srw, daniel.vetter, mh12gx2825 On Fri, Oct 30, 2020 at 08:11:20AM +0100, Greg KH wrote: > On Fri, Oct 30, 2020 at 08:52:45AM +0530, Deepak R Varma wrote: > > Using DEFINE_DEBUGFS_ATTRIBUTE macro with debugfs_create_file_unsafe() > > function in place of the debugfs_create_file() function will make the > > file operation struct "reset" aware of the file's lifetime. Additional > > details here: https://lists.archive.carbon60.com/linux/kernel/2369498 > > > > Issue reported by Coccinelle script: > > scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > > > Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com> > > --- > > Please Note: This is a Outreachy project task patch. > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 20 ++++++++++---------- > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > index 2d125b8b15ee..f076b1ba7319 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > @@ -1551,29 +1551,29 @@ static int amdgpu_debugfs_sclk_set(void *data, u64 val) > > return 0; > > } > > > > -DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL, > > - amdgpu_debugfs_ib_preempt, "%llu\n"); > > +DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL, > > + amdgpu_debugfs_ib_preempt, "%llu\n"); > > Are you sure this is ok? Do these devices need this additional > "protection"? Do they have the problem that these macros were written > for? > > Same for the other patches you just submitted here, I think you need to > somehow "prove" that these changes are necessary, checkpatch isn't able > to determine this all the time. Hi Greg, Based on my understanding, the current function debugfs_create_file() adds an overhead of lifetime managing proxy for such fop structs. This should be applicable to these set of drivers as well. Hence I think this change will be useful. I will wait for comments from other experts for driver specific consideration / behavior. Thanks, drv > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Outreachy kernel] [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe() 2020-10-30 7:57 ` Deepak R Varma @ 2020-10-30 8:00 ` Christian König 2020-10-30 8:25 ` Greg KH 2020-10-30 8:03 ` Greg KH 1 sibling, 1 reply; 14+ messages in thread From: Christian König @ 2020-10-30 8:00 UTC (permalink / raw) To: Deepak R Varma, Greg KH Cc: outreachy-kernel, Alex Deucher, David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel, melissa.srw, daniel.vetter Am 30.10.20 um 08:57 schrieb Deepak R Varma: > On Fri, Oct 30, 2020 at 08:11:20AM +0100, Greg KH wrote: >> On Fri, Oct 30, 2020 at 08:52:45AM +0530, Deepak R Varma wrote: >>> Using DEFINE_DEBUGFS_ATTRIBUTE macro with debugfs_create_file_unsafe() >>> function in place of the debugfs_create_file() function will make the >>> file operation struct "reset" aware of the file's lifetime. Additional >>> details here: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.archive.carbon60.com%2Flinux%2Fkernel%2F2369498&data=04%7C01%7Cchristian.koenig%40amd.com%7Cddd7a6ac8164415a639708d87ca97004%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637396414464384011%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=o6GOHvMxNMuOPlC4nhDyURCHBLqfQZhYQq%2BiIMt3D3s%3D&reserved=0 >>> >>> Issue reported by Coccinelle script: >>> scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci >>> >>> Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com> >>> --- >>> Please Note: This is a Outreachy project task patch. >>> >>> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 20 ++++++++++---------- >>> 1 file changed, 10 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>> index 2d125b8b15ee..f076b1ba7319 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>> @@ -1551,29 +1551,29 @@ static int amdgpu_debugfs_sclk_set(void *data, u64 val) >>> return 0; >>> } >>> >>> -DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL, >>> - amdgpu_debugfs_ib_preempt, "%llu\n"); >>> +DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL, >>> + amdgpu_debugfs_ib_preempt, "%llu\n"); >> Are you sure this is ok? Do these devices need this additional >> "protection"? Do they have the problem that these macros were written >> for? >> >> Same for the other patches you just submitted here, I think you need to >> somehow "prove" that these changes are necessary, checkpatch isn't able >> to determine this all the time. > Hi Greg, > Based on my understanding, the current function debugfs_create_file() > adds an overhead of lifetime managing proxy for such fop structs. This > should be applicable to these set of drivers as well. Hence I think this > change will be useful. Well since this is only created once per device instance I don't really care about this little overhead. But what exactly is debugfs doing or not doing here? Regards, Christian. > > I will wait for comments from other experts for driver specific > consideration / behavior. > > Thanks, > drv > > >> thanks, >> >> greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Outreachy kernel] [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe() 2020-10-30 8:00 ` Christian König @ 2020-10-30 8:25 ` Greg KH 2020-10-30 9:15 ` Daniel Vetter 2020-10-30 9:53 ` Christian König 0 siblings, 2 replies; 14+ messages in thread From: Greg KH @ 2020-10-30 8:25 UTC (permalink / raw) To: Christian König Cc: Deepak R Varma, outreachy-kernel, Alex Deucher, David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel, melissa.srw, daniel.vetter On Fri, Oct 30, 2020 at 09:00:04AM +0100, Christian König wrote: > Am 30.10.20 um 08:57 schrieb Deepak R Varma: > > On Fri, Oct 30, 2020 at 08:11:20AM +0100, Greg KH wrote: > > > On Fri, Oct 30, 2020 at 08:52:45AM +0530, Deepak R Varma wrote: > > > > Using DEFINE_DEBUGFS_ATTRIBUTE macro with debugfs_create_file_unsafe() > > > > function in place of the debugfs_create_file() function will make the > > > > file operation struct "reset" aware of the file's lifetime. Additional > > > > details here: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.archive.carbon60.com%2Flinux%2Fkernel%2F2369498&data=04%7C01%7Cchristian.koenig%40amd.com%7Cddd7a6ac8164415a639708d87ca97004%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637396414464384011%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=o6GOHvMxNMuOPlC4nhDyURCHBLqfQZhYQq%2BiIMt3D3s%3D&reserved=0 > > > > > > > > Issue reported by Coccinelle script: > > > > scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > > > > > > > Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com> > > > > --- > > > > Please Note: This is a Outreachy project task patch. > > > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 20 ++++++++++---------- > > > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > index 2d125b8b15ee..f076b1ba7319 100644 > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > @@ -1551,29 +1551,29 @@ static int amdgpu_debugfs_sclk_set(void *data, u64 val) > > > > return 0; > > > > } > > > > -DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL, > > > > - amdgpu_debugfs_ib_preempt, "%llu\n"); > > > > +DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL, > > > > + amdgpu_debugfs_ib_preempt, "%llu\n"); > > > Are you sure this is ok? Do these devices need this additional > > > "protection"? Do they have the problem that these macros were written > > > for? > > > > > > Same for the other patches you just submitted here, I think you need to > > > somehow "prove" that these changes are necessary, checkpatch isn't able > > > to determine this all the time. > > Hi Greg, > > Based on my understanding, the current function debugfs_create_file() > > adds an overhead of lifetime managing proxy for such fop structs. This > > should be applicable to these set of drivers as well. Hence I think this > > change will be useful. > > Well since this is only created once per device instance I don't really care > about this little overhead. > > But what exactly is debugfs doing or not doing here? It is trying to save drivers from having debugfs files open that point to memory that can go away at any time. For graphics devices, I doubt that is the case. thanks, greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Outreachy kernel] [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe() 2020-10-30 8:25 ` Greg KH @ 2020-10-30 9:15 ` Daniel Vetter 2020-10-30 9:24 ` Daniel Vetter 2020-10-30 9:53 ` Christian König 1 sibling, 1 reply; 14+ messages in thread From: Daniel Vetter @ 2020-10-30 9:15 UTC (permalink / raw) To: Greg KH Cc: Christian König, Deepak R Varma, outreachy-kernel, Alex Deucher, David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel, melissa.srw, daniel.vetter On Fri, Oct 30, 2020 at 09:25:18AM +0100, Greg KH wrote: > On Fri, Oct 30, 2020 at 09:00:04AM +0100, Christian König wrote: > > Am 30.10.20 um 08:57 schrieb Deepak R Varma: > > > On Fri, Oct 30, 2020 at 08:11:20AM +0100, Greg KH wrote: > > > > On Fri, Oct 30, 2020 at 08:52:45AM +0530, Deepak R Varma wrote: > > > > > Using DEFINE_DEBUGFS_ATTRIBUTE macro with debugfs_create_file_unsafe() > > > > > function in place of the debugfs_create_file() function will make the > > > > > file operation struct "reset" aware of the file's lifetime. Additional > > > > > details here: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.archive.carbon60.com%2Flinux%2Fkernel%2F2369498&data=04%7C01%7Cchristian.koenig%40amd.com%7Cddd7a6ac8164415a639708d87ca97004%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637396414464384011%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=o6GOHvMxNMuOPlC4nhDyURCHBLqfQZhYQq%2BiIMt3D3s%3D&reserved=0 > > > > > > > > > > Issue reported by Coccinelle script: > > > > > scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > > > > > > > > > Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com> > > > > > --- > > > > > Please Note: This is a Outreachy project task patch. > > > > > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 20 ++++++++++---------- > > > > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > > index 2d125b8b15ee..f076b1ba7319 100644 > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > > @@ -1551,29 +1551,29 @@ static int amdgpu_debugfs_sclk_set(void *data, u64 val) > > > > > return 0; > > > > > } > > > > > -DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL, > > > > > - amdgpu_debugfs_ib_preempt, "%llu\n"); > > > > > +DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL, > > > > > + amdgpu_debugfs_ib_preempt, "%llu\n"); > > > > Are you sure this is ok? Do these devices need this additional > > > > "protection"? Do they have the problem that these macros were written > > > > for? > > > > > > > > Same for the other patches you just submitted here, I think you need to > > > > somehow "prove" that these changes are necessary, checkpatch isn't able > > > > to determine this all the time. > > > Hi Greg, > > > Based on my understanding, the current function debugfs_create_file() > > > adds an overhead of lifetime managing proxy for such fop structs. This > > > should be applicable to these set of drivers as well. Hence I think this > > > change will be useful. > > > > Well since this is only created once per device instance I don't really care > > about this little overhead. > > > > But what exactly is debugfs doing or not doing here? > > It is trying to save drivers from having debugfs files open that point > to memory that can go away at any time. For graphics devices, I doubt > that is the case. So for anything we add/remove we have two-stage cleanup 1. drm_dev_unregister (or drm_connector_unregisters, those are the only two hotunpluggable things we have) unregister all the uapi interfaces. This deletes all the sysfs and debugfs files. 2. Once all the references to the underlying object disappear from the kernel, we free up the data structure. Now for chardev and similar uapi, we hold full references for open files. But for sysfs and debugfs we assume that those uapi layers will make sure that after we deleted the files in step 1 all access through these functions are guaranteed to have finished. If that's not the case, then we need to rework our refcounting and also refcount the underlying drm structure (drm_device or drm_connector) from sysfs/debugfs files. Now I tried to look at the patch Deepak references, and I'm not really clear what changes. Or whether we made a wrong assumption previously about what debugfs/sysfs guarantee when we delete the files. -Daniel > > thanks, > > greg k-h -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Outreachy kernel] [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe() 2020-10-30 9:15 ` Daniel Vetter @ 2020-10-30 9:24 ` Daniel Vetter 2020-10-30 12:55 ` Deepak R Varma 0 siblings, 1 reply; 14+ messages in thread From: Daniel Vetter @ 2020-10-30 9:24 UTC (permalink / raw) To: Greg KH, Christian König, Deepak R Varma, outreachy-kernel, Alex Deucher, David Airlie, amd-gfx, dri-devel, linux-kernel, melissa.srw On Fri, Oct 30, 2020 at 10:15:21AM +0100, Daniel Vetter wrote: > On Fri, Oct 30, 2020 at 09:25:18AM +0100, Greg KH wrote: > > On Fri, Oct 30, 2020 at 09:00:04AM +0100, Christian König wrote: > > > Am 30.10.20 um 08:57 schrieb Deepak R Varma: > > > > On Fri, Oct 30, 2020 at 08:11:20AM +0100, Greg KH wrote: > > > > > On Fri, Oct 30, 2020 at 08:52:45AM +0530, Deepak R Varma wrote: > > > > > > Using DEFINE_DEBUGFS_ATTRIBUTE macro with debugfs_create_file_unsafe() > > > > > > function in place of the debugfs_create_file() function will make the > > > > > > file operation struct "reset" aware of the file's lifetime. Additional > > > > > > details here: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.archive.carbon60.com%2Flinux%2Fkernel%2F2369498&data=04%7C01%7Cchristian.koenig%40amd.com%7Cddd7a6ac8164415a639708d87ca97004%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637396414464384011%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=o6GOHvMxNMuOPlC4nhDyURCHBLqfQZhYQq%2BiIMt3D3s%3D&reserved=0 > > > > > > > > > > > > Issue reported by Coccinelle script: > > > > > > scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > > > > > > > > > > > Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com> > > > > > > --- > > > > > > Please Note: This is a Outreachy project task patch. > > > > > > > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 20 ++++++++++---------- > > > > > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > > > index 2d125b8b15ee..f076b1ba7319 100644 > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > > > @@ -1551,29 +1551,29 @@ static int amdgpu_debugfs_sclk_set(void *data, u64 val) > > > > > > return 0; > > > > > > } > > > > > > -DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL, > > > > > > - amdgpu_debugfs_ib_preempt, "%llu\n"); > > > > > > +DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL, > > > > > > + amdgpu_debugfs_ib_preempt, "%llu\n"); > > > > > Are you sure this is ok? Do these devices need this additional > > > > > "protection"? Do they have the problem that these macros were written > > > > > for? > > > > > > > > > > Same for the other patches you just submitted here, I think you need to > > > > > somehow "prove" that these changes are necessary, checkpatch isn't able > > > > > to determine this all the time. > > > > Hi Greg, > > > > Based on my understanding, the current function debugfs_create_file() > > > > adds an overhead of lifetime managing proxy for such fop structs. This > > > > should be applicable to these set of drivers as well. Hence I think this > > > > change will be useful. > > > > > > Well since this is only created once per device instance I don't really care > > > about this little overhead. > > > > > > But what exactly is debugfs doing or not doing here? > > > > It is trying to save drivers from having debugfs files open that point > > to memory that can go away at any time. For graphics devices, I doubt > > that is the case. > > So for anything we add/remove we have two-stage cleanup > > 1. drm_dev_unregister (or drm_connector_unregisters, those are the only > two hotunpluggable things we have) unregister all the uapi interfaces. > This deletes all the sysfs and debugfs files. > > 2. Once all the references to the underlying object disappear from the > kernel, we free up the data structure. > > Now for chardev and similar uapi, we hold full references for open files. > But for sysfs and debugfs we assume that those uapi layers will make sure > that after we deleted the files in step 1 all access through these > functions are guaranteed to have finished. If that's not the case, then we > need to rework our refcounting and also refcount the underlying drm > structure (drm_device or drm_connector) from sysfs/debugfs files. > > Now I tried to look at the patch Deepak references, and I'm not really > clear what changes. Or whether we made a wrong assumption previously about > what debugfs/sysfs guarantee when we delete the files. I read some more code and kerneldoc, and I still have no idea what this new _unsafe variant is used for. Only ones I've found seem to use debugfs_file_get/put like the normal variant, to protect against concurrently removed files due to hotunplug. Which is kinda what we've been expecting debugfs to do for us. What's a use-case for _unsafe _without_ debugfs_file_get/put? Decently confused me over here doesn't get this. -Daniel > -Daniel > > > > > thanks, > > > > greg k-h > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Outreachy kernel] [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe() 2020-10-30 9:24 ` Daniel Vetter @ 2020-10-30 12:55 ` Deepak R Varma 0 siblings, 0 replies; 14+ messages in thread From: Deepak R Varma @ 2020-10-30 12:55 UTC (permalink / raw) To: Greg KH, Christian König, outreachy-kernel, Alex Deucher, David Airlie, amd-gfx, dri-devel, linux-kernel, melissa.srw Cc: mh12gx2825 On Fri, Oct 30, 2020 at 10:24:57AM +0100, Daniel Vetter wrote: > On Fri, Oct 30, 2020 at 10:15:21AM +0100, Daniel Vetter wrote: > > On Fri, Oct 30, 2020 at 09:25:18AM +0100, Greg KH wrote: > > > On Fri, Oct 30, 2020 at 09:00:04AM +0100, Christian König wrote: > > > > Am 30.10.20 um 08:57 schrieb Deepak R Varma: > > > > > On Fri, Oct 30, 2020 at 08:11:20AM +0100, Greg KH wrote: > > > > > > On Fri, Oct 30, 2020 at 08:52:45AM +0530, Deepak R Varma wrote: > > > > > > > Using DEFINE_DEBUGFS_ATTRIBUTE macro with debugfs_create_file_unsafe() > > > > > > > function in place of the debugfs_create_file() function will make the > > > > > > > file operation struct "reset" aware of the file's lifetime. Additional > > > > > > > details here: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.archive.carbon60.com%2Flinux%2Fkernel%2F2369498&data=04%7C01%7Cchristian.koenig%40amd.com%7Cddd7a6ac8164415a639708d87ca97004%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637396414464384011%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=o6GOHvMxNMuOPlC4nhDyURCHBLqfQZhYQq%2BiIMt3D3s%3D&reserved=0 > > > > > > > > > > > > > > Issue reported by Coccinelle script: > > > > > > > scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > > > > > > > > > > > > > Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com> > > > > > > > --- > > > > > > > Please Note: This is a Outreachy project task patch. > > > > > > > > > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 20 ++++++++++---------- > > > > > > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > > > > index 2d125b8b15ee..f076b1ba7319 100644 > > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > > > > @@ -1551,29 +1551,29 @@ static int amdgpu_debugfs_sclk_set(void *data, u64 val) > > > > > > > return 0; > > > > > > > } > > > > > > > -DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL, > > > > > > > - amdgpu_debugfs_ib_preempt, "%llu\n"); > > > > > > > +DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL, > > > > > > > + amdgpu_debugfs_ib_preempt, "%llu\n"); > > > > > > Are you sure this is ok? Do these devices need this additional > > > > > > "protection"? Do they have the problem that these macros were written > > > > > > for? > > > > > > > > > > > > Same for the other patches you just submitted here, I think you need to > > > > > > somehow "prove" that these changes are necessary, checkpatch isn't able > > > > > > to determine this all the time. > > > > > Hi Greg, > > > > > Based on my understanding, the current function debugfs_create_file() > > > > > adds an overhead of lifetime managing proxy for such fop structs. This > > > > > should be applicable to these set of drivers as well. Hence I think this > > > > > change will be useful. > > > > > > > > Well since this is only created once per device instance I don't really care > > > > about this little overhead. > > > > > > > > But what exactly is debugfs doing or not doing here? > > > > > > It is trying to save drivers from having debugfs files open that point > > > to memory that can go away at any time. For graphics devices, I doubt > > > that is the case. > > > > So for anything we add/remove we have two-stage cleanup > > > > 1. drm_dev_unregister (or drm_connector_unregisters, those are the only > > two hotunpluggable things we have) unregister all the uapi interfaces. > > This deletes all the sysfs and debugfs files. > > > > 2. Once all the references to the underlying object disappear from the > > kernel, we free up the data structure. > > > > Now for chardev and similar uapi, we hold full references for open files. > > But for sysfs and debugfs we assume that those uapi layers will make sure > > that after we deleted the files in step 1 all access through these > > functions are guaranteed to have finished. If that's not the case, then we > > need to rework our refcounting and also refcount the underlying drm > > structure (drm_device or drm_connector) from sysfs/debugfs files. > > > > Now I tried to look at the patch Deepak references, and I'm not really > > clear what changes. Or whether we made a wrong assumption previously about > > what debugfs/sysfs guarantee when we delete the files. > > I read some more code and kerneldoc, and I still have no idea what this > new _unsafe variant is used for. Only ones I've found seem to use > debugfs_file_get/put like the normal variant, to protect against > concurrently removed files due to hotunplug. Which is kinda what we've > been expecting debugfs to do for us. > > What's a use-case for _unsafe _without_ debugfs_file_get/put? > > Decently confused me over here doesn't get this. > -Daniel Hi All, Thank you for your comments. I was not anticipating this to be such a offbeat patch. Pardon my growing understanding. Please suggest if I can evaluate the change proposed to confirm if this is a meaningful change. Thank you again! ./drv > > > -Daniel > > > > > > > > thanks, > > > > > > greg k-h > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Outreachy kernel] [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe() 2020-10-30 8:25 ` Greg KH 2020-10-30 9:15 ` Daniel Vetter @ 2020-10-30 9:53 ` Christian König 1 sibling, 0 replies; 14+ messages in thread From: Christian König @ 2020-10-30 9:53 UTC (permalink / raw) To: Greg KH Cc: Deepak R Varma, outreachy-kernel, Alex Deucher, David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel, melissa.srw, daniel.vetter Am 30.10.20 um 09:25 schrieb Greg KH: > On Fri, Oct 30, 2020 at 09:00:04AM +0100, Christian König wrote: >> Am 30.10.20 um 08:57 schrieb Deepak R Varma: >>> On Fri, Oct 30, 2020 at 08:11:20AM +0100, Greg KH wrote: >>>> On Fri, Oct 30, 2020 at 08:52:45AM +0530, Deepak R Varma wrote: >>>>> Using DEFINE_DEBUGFS_ATTRIBUTE macro with debugfs_create_file_unsafe() >>>>> function in place of the debugfs_create_file() function will make the >>>>> file operation struct "reset" aware of the file's lifetime. Additional >>>>> details here: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.archive.carbon60.com%2Flinux%2Fkernel%2F2369498&data=04%7C01%7Cchristian.koenig%40amd.com%7Ce3fb2f2236f44f8779bc08d87cad3a2d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637396430734542672%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=CuDnWSIacKfdcv6%2B00Q9mZ%2BAcXse5mlFpuSsBybZ%2Fww%3D&reserved=0 >>>>> >>>>> Issue reported by Coccinelle script: >>>>> scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci >>>>> >>>>> Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com> >>>>> --- >>>>> Please Note: This is a Outreachy project task patch. >>>>> >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 20 ++++++++++---------- >>>>> 1 file changed, 10 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>> index 2d125b8b15ee..f076b1ba7319 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >>>>> @@ -1551,29 +1551,29 @@ static int amdgpu_debugfs_sclk_set(void *data, u64 val) >>>>> return 0; >>>>> } >>>>> -DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL, >>>>> - amdgpu_debugfs_ib_preempt, "%llu\n"); >>>>> +DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL, >>>>> + amdgpu_debugfs_ib_preempt, "%llu\n"); >>>> Are you sure this is ok? Do these devices need this additional >>>> "protection"? Do they have the problem that these macros were written >>>> for? >>>> >>>> Same for the other patches you just submitted here, I think you need to >>>> somehow "prove" that these changes are necessary, checkpatch isn't able >>>> to determine this all the time. >>> Hi Greg, >>> Based on my understanding, the current function debugfs_create_file() >>> adds an overhead of lifetime managing proxy for such fop structs. This >>> should be applicable to these set of drivers as well. Hence I think this >>> change will be useful. >> Well since this is only created once per device instance I don't really care >> about this little overhead. >> >> But what exactly is debugfs doing or not doing here? > It is trying to save drivers from having debugfs files open that point > to memory that can go away at any time. For graphics devices, I doubt > that is the case. Well we have somebody working on hot plug removal for dGPUs, but that is really not an issue here. Even if our device are removed most of the debugfs files shouldn't be affected by that. Thanks, Christian. > > thanks, > > greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Outreachy kernel] [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe() 2020-10-30 7:57 ` Deepak R Varma 2020-10-30 8:00 ` Christian König @ 2020-10-30 8:03 ` Greg KH [not found] ` <CACAkLupMiH9z4g7WLJ7t-N089_M6QGX6HkPQUATu4VCxDeFAKw@mail.gmail.com> 2020-10-31 11:52 ` Joe Perches 1 sibling, 2 replies; 14+ messages in thread From: Greg KH @ 2020-10-30 8:03 UTC (permalink / raw) To: Deepak R Varma Cc: outreachy-kernel, Alex Deucher, Christian König, David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel, melissa.srw, daniel.vetter On Fri, Oct 30, 2020 at 01:27:16PM +0530, Deepak R Varma wrote: > On Fri, Oct 30, 2020 at 08:11:20AM +0100, Greg KH wrote: > > On Fri, Oct 30, 2020 at 08:52:45AM +0530, Deepak R Varma wrote: > > > Using DEFINE_DEBUGFS_ATTRIBUTE macro with debugfs_create_file_unsafe() > > > function in place of the debugfs_create_file() function will make the > > > file operation struct "reset" aware of the file's lifetime. Additional > > > details here: https://lists.archive.carbon60.com/linux/kernel/2369498 > > > > > > Issue reported by Coccinelle script: > > > scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > > > > > Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com> > > > --- > > > Please Note: This is a Outreachy project task patch. > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 20 ++++++++++---------- > > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > index 2d125b8b15ee..f076b1ba7319 100644 > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > @@ -1551,29 +1551,29 @@ static int amdgpu_debugfs_sclk_set(void *data, u64 val) > > > return 0; > > > } > > > > > > -DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL, > > > - amdgpu_debugfs_ib_preempt, "%llu\n"); > > > +DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL, > > > + amdgpu_debugfs_ib_preempt, "%llu\n"); > > > > Are you sure this is ok? Do these devices need this additional > > "protection"? Do they have the problem that these macros were written > > for? > > > > Same for the other patches you just submitted here, I think you need to > > somehow "prove" that these changes are necessary, checkpatch isn't able > > to determine this all the time. > > Hi Greg, > Based on my understanding, the current function debugfs_create_file() > adds an overhead of lifetime managing proxy for such fop structs. This > should be applicable to these set of drivers as well. Hence I think this > change will be useful. Why do these drivers need these changes? Are these files dynamically removed from the system at random times? There is a reason we didn't just do a global search/replace for this in the kernel when the new functions were added, so I don't know why checkpatch is now saying it must be done. thanks, greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <CACAkLupMiH9z4g7WLJ7t-N089_M6QGX6HkPQUATu4VCxDeFAKw@mail.gmail.com>]
* Re: [Outreachy kernel] [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe() [not found] ` <CACAkLupMiH9z4g7WLJ7t-N089_M6QGX6HkPQUATu4VCxDeFAKw@mail.gmail.com> @ 2020-10-30 8:24 ` Greg KH 2020-10-30 10:20 ` Julia Lawall 0 siblings, 1 reply; 14+ messages in thread From: Greg KH @ 2020-10-30 8:24 UTC (permalink / raw) To: Sumera Priyadarsini Cc: Deepak R Varma, Outreachy, Alex Deucher, Christian König, David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel, melissa.srw, daniel.vetter On Fri, Oct 30, 2020 at 01:47:05PM +0530, Sumera Priyadarsini wrote: > On Fri, 30 Oct, 2020, 1:32 PM Greg KH, <gregkh@linuxfoundation.org> wrote: > > > On Fri, Oct 30, 2020 at 01:27:16PM +0530, Deepak R Varma wrote: > > > On Fri, Oct 30, 2020 at 08:11:20AM +0100, Greg KH wrote: > > > > On Fri, Oct 30, 2020 at 08:52:45AM +0530, Deepak R Varma wrote: > > > > > Using DEFINE_DEBUGFS_ATTRIBUTE macro with > > debugfs_create_file_unsafe() > > > > > function in place of the debugfs_create_file() function will make the > > > > > file operation struct "reset" aware of the file's lifetime. > > Additional > > > > > details here: > > https://lists.archive.carbon60.com/linux/kernel/2369498 > > > > > > > > > > Issue reported by Coccinelle script: > > > > > scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > > > > > > > > > Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com> > > > > > --- > > > > > Please Note: This is a Outreachy project task patch. > > > > > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 20 > > ++++++++++---------- > > > > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > > index 2d125b8b15ee..f076b1ba7319 100644 > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > > @@ -1551,29 +1551,29 @@ static int amdgpu_debugfs_sclk_set(void > > *data, u64 val) > > > > > return 0; > > > > > } > > > > > > > > > > -DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL, > > > > > - amdgpu_debugfs_ib_preempt, "%llu\n"); > > > > > +DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL, > > > > > + amdgpu_debugfs_ib_preempt, "%llu\n"); > > > > > > > > Are you sure this is ok? Do these devices need this additional > > > > "protection"? Do they have the problem that these macros were written > > > > for? > > > > > > > > Same for the other patches you just submitted here, I think you need to > > > > somehow "prove" that these changes are necessary, checkpatch isn't able > > > > to determine this all the time. > > > > > > Hi Greg, > > > Based on my understanding, the current function debugfs_create_file() > > > adds an overhead of lifetime managing proxy for such fop structs. This > > > should be applicable to these set of drivers as well. Hence I think this > > > change will be useful. > > > > Why do these drivers need these changes? Are these files dynamically > > removed from the system at random times? > > > > There is a reason we didn't just do a global search/replace for this in > > the kernel when the new functions were added, so I don't know why > > checkpatch is now saying it must be done. > > > > Hi, > > Sorry to jump in on the thread this way, but what exactly does a 'lifetime > managing proxy' for file operations mean? I am trying to understand how > DEFINE_DEBUGFS_ATTRIBUTE changes things wrt debug_ fs file operations but > can't find many resources. :( It means that the debugfs core can handle debugfs files being removed from the system while they are still open when they were created by a driver/module that is now unloaded from memory. This is only an issue for drivers that manage devices that have unknown lifespans (i.e. they can be yanked out of the system at any time, and the memory for those debugfs files can be freed). For the entire DRM/GPU subsystem, I strongly doubt this is the case. > Please let me know if I should be asking this in a different mailing > list/irc instead. > > The change seems to be suggested by a coccinelle script. I know, and I don't think that script knows the nuances behind this, as, again, we would have just done a global search/replace for this when the debugfs fixes went into the kernel. thanks, greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Outreachy kernel] [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe() 2020-10-30 8:24 ` Greg KH @ 2020-10-30 10:20 ` Julia Lawall 0 siblings, 0 replies; 14+ messages in thread From: Julia Lawall @ 2020-10-30 10:20 UTC (permalink / raw) To: Greg KH Cc: Sumera Priyadarsini, Deepak R Varma, Outreachy, Alex Deucher, Christian König, David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel, melissa.srw, daniel.vetter, nicstange On Fri, 30 Oct 2020, Greg KH wrote: > On Fri, Oct 30, 2020 at 01:47:05PM +0530, Sumera Priyadarsini wrote: > > On Fri, 30 Oct, 2020, 1:32 PM Greg KH, <gregkh@linuxfoundation.org> wrote: > > > > > On Fri, Oct 30, 2020 at 01:27:16PM +0530, Deepak R Varma wrote: > > > > On Fri, Oct 30, 2020 at 08:11:20AM +0100, Greg KH wrote: > > > > > On Fri, Oct 30, 2020 at 08:52:45AM +0530, Deepak R Varma wrote: > > > > > > Using DEFINE_DEBUGFS_ATTRIBUTE macro with > > > debugfs_create_file_unsafe() > > > > > > function in place of the debugfs_create_file() function will make the > > > > > > file operation struct "reset" aware of the file's lifetime. > > > Additional > > > > > > details here: > > > https://lists.archive.carbon60.com/linux/kernel/2369498 > > > > > > > > > > > > Issue reported by Coccinelle script: > > > > > > scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > > > > > > > > > > > Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com> > > > > > > --- > > > > > > Please Note: This is a Outreachy project task patch. > > > > > > > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 20 > > > ++++++++++---------- > > > > > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > > > index 2d125b8b15ee..f076b1ba7319 100644 > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c > > > > > > @@ -1551,29 +1551,29 @@ static int amdgpu_debugfs_sclk_set(void > > > *data, u64 val) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > -DEFINE_SIMPLE_ATTRIBUTE(fops_ib_preempt, NULL, > > > > > > - amdgpu_debugfs_ib_preempt, "%llu\n"); > > > > > > +DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL, > > > > > > + amdgpu_debugfs_ib_preempt, "%llu\n"); > > > > > > > > > > Are you sure this is ok? Do these devices need this additional > > > > > "protection"? Do they have the problem that these macros were written > > > > > for? > > > > > > > > > > Same for the other patches you just submitted here, I think you need to > > > > > somehow "prove" that these changes are necessary, checkpatch isn't able > > > > > to determine this all the time. > > > > > > > > Hi Greg, > > > > Based on my understanding, the current function debugfs_create_file() > > > > adds an overhead of lifetime managing proxy for such fop structs. This > > > > should be applicable to these set of drivers as well. Hence I think this > > > > change will be useful. > > > > > > Why do these drivers need these changes? Are these files dynamically > > > removed from the system at random times? > > > > > > There is a reason we didn't just do a global search/replace for this in > > > the kernel when the new functions were added, so I don't know why > > > checkpatch is now saying it must be done. > > > > > > > Hi, > > > > Sorry to jump in on the thread this way, but what exactly does a 'lifetime > > managing proxy' for file operations mean? I am trying to understand how > > DEFINE_DEBUGFS_ATTRIBUTE changes things wrt debug_ fs file operations but > > can't find many resources. :( > > It means that the debugfs core can handle debugfs files being removed > from the system while they are still open when they were created by a > driver/module that is now unloaded from memory. > > This is only an issue for drivers that manage devices that have unknown > lifespans (i.e. they can be yanked out of the system at any time, and > the memory for those debugfs files can be freed). > > For the entire DRM/GPU subsystem, I strongly doubt this is the case. > > > Please let me know if I should be asking this in a different mailing > > list/irc instead. > > > > The change seems to be suggested by a coccinelle script. > > I know, and I don't think that script knows the nuances behind this, as, > again, we would have just done a global search/replace for this when the > debugfs fixes went into the kernel. The script comes from Nicolai Stange <nicstange@gmail.com>. If there are some precise considerations that make the change likely to be useful, then the script can be changed. If the script is not helpful, then it can be removed. julia > > thanks, > > greg k-h > > -- > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20201030082425.GA1619669%40kroah.com. > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Outreachy kernel] [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe() 2020-10-30 8:03 ` Greg KH [not found] ` <CACAkLupMiH9z4g7WLJ7t-N089_M6QGX6HkPQUATu4VCxDeFAKw@mail.gmail.com> @ 2020-10-31 11:52 ` Joe Perches 2020-10-31 12:01 ` Julia Lawall 1 sibling, 1 reply; 14+ messages in thread From: Joe Perches @ 2020-10-31 11:52 UTC (permalink / raw) To: Greg KH, Deepak R Varma Cc: outreachy-kernel, Alex Deucher, Christian König, David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel, melissa.srw, daniel.vetter On Fri, 2020-10-30 at 09:03 +0100, Greg KH wrote: > On Fri, Oct 30, 2020 at 01:27:16PM +0530, Deepak R Varma wrote: > > On Fri, Oct 30, 2020 at 08:11:20AM +0100, Greg KH wrote: > > > On Fri, Oct 30, 2020 at 08:52:45AM +0530, Deepak R Varma wrote: > > > > Using DEFINE_DEBUGFS_ATTRIBUTE macro with debugfs_create_file_unsafe() > > > > function in place of the debugfs_create_file() function will make the > > > > file operation struct "reset" aware of the file's lifetime. Additional > > > > details here: https://lists.archive.carbon60.com/linux/kernel/2369498 > > > > > > > > Issue reported by Coccinelle script: > > > > scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci [] > There is a reason we didn't just do a global search/replace for this in > the kernel when the new functions were added, so I don't know why > checkpatch is now saying it must be done. I think it's not a checkpatch warning here. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Outreachy kernel] [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe() 2020-10-31 11:52 ` Joe Perches @ 2020-10-31 12:01 ` Julia Lawall 0 siblings, 0 replies; 14+ messages in thread From: Julia Lawall @ 2020-10-31 12:01 UTC (permalink / raw) To: Joe Perches Cc: Greg KH, Deepak R Varma, outreachy-kernel, Alex Deucher, Christian König, David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel, melissa.srw, daniel.vetter On Sat, 31 Oct 2020, Joe Perches wrote: > On Fri, 2020-10-30 at 09:03 +0100, Greg KH wrote: > > On Fri, Oct 30, 2020 at 01:27:16PM +0530, Deepak R Varma wrote: > > > On Fri, Oct 30, 2020 at 08:11:20AM +0100, Greg KH wrote: > > > > On Fri, Oct 30, 2020 at 08:52:45AM +0530, Deepak R Varma wrote: > > > > > Using DEFINE_DEBUGFS_ATTRIBUTE macro with debugfs_create_file_unsafe() > > > > > function in place of the debugfs_create_file() function will make the > > > > > file operation struct "reset" aware of the file's lifetime. Additional > > > > > details here: https://lists.archive.carbon60.com/linux/kernel/2369498 > > > > > > > > > > Issue reported by Coccinelle script: > > > > > scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > [] > > There is a reason we didn't just do a global search/replace for this in > > the kernel when the new functions were added, so I don't know why > > checkpatch is now saying it must be done. > > I think it's not a checkpatch warning here. That is correct, it's a coccinelle script. julia > > -- > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/0b818156537f354904938f437cbb9dd02e765653.camel%40perches.com. > ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-10-31 12:01 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-30 3:22 [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe() Deepak R Varma 2020-10-30 7:11 ` [Outreachy kernel] " Greg KH 2020-10-30 7:57 ` Deepak R Varma 2020-10-30 8:00 ` Christian König 2020-10-30 8:25 ` Greg KH 2020-10-30 9:15 ` Daniel Vetter 2020-10-30 9:24 ` Daniel Vetter 2020-10-30 12:55 ` Deepak R Varma 2020-10-30 9:53 ` Christian König 2020-10-30 8:03 ` Greg KH [not found] ` <CACAkLupMiH9z4g7WLJ7t-N089_M6QGX6HkPQUATu4VCxDeFAKw@mail.gmail.com> 2020-10-30 8:24 ` Greg KH 2020-10-30 10:20 ` Julia Lawall 2020-10-31 11:52 ` Joe Perches 2020-10-31 12:01 ` Julia Lawall
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).