<html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">Am 13.07.20 um 15:34 schrieb Yuan, Xiaojie: </div> <blockquote type="cite" cite="mid:MW2PR12MB2586787CBB596190ECDF754789600@MW2PR12MB2586.namprd12.prod.outlook.com"> <pre class="moz-quote-pre" wrap="">[AMD Official Use Only - Internal Distribution Only] Hi Chris, This was observed when I was trying to add a new debugfs file.</pre> </blockquote> In this case please add the new file using debugfs_create_file() directly and don't touch this old code. <blockquote type="cite" cite="mid:MW2PR12MB2586787CBB596190ECDF754789600@MW2PR12MB2586.namprd12.prod.outlook.com"> <pre class="moz-quote-pre" wrap=""> Some similar occurrences using ARRAY_SIZE() are: - amdgpu_kms.c :: amdgpu_firmware_info_list - amdgpu_pm.c :: amdgpu_debugfs_pm_info - amdgpu_ttm.c :: amdgpu_ttm_debugfs_list - amdgpu_dm_debugfs.c :: amdgpu_dm_debugfs_list This patch simply unified the usage of amdgpu_debugfs_add_files(). BTW, do you intended to use: debugfs_create_file() - need to call debugfs_remove() explicitly or the drm helper drm_debugfs_create_files() - debugfs files will be removed automatically</pre> </blockquote> No, exactly that's the point. All debugfs files are automatically removed when the driver unloads because the parent directory is removed. See the debugfs.h file in the Linux source code: <blockquote type="cite"> <pre>void <a href="https://elixir.bootlin.com/linux/latest/C/ident/debugfs_remove">debugfs_remove</a>(struct <a href="https://elixir.bootlin.com/linux/latest/C/ident/dentry">dentry</a> *<a href="https://elixir.bootlin.com/linux/latest/C/ident/dentry">dentry</a>); #define <a href="https://elixir.bootlin.com/linux/latest/C/ident/debugfs_remove_recursive">debugfs_remove_recursive</a> <a href="https://elixir.bootlin.com/linux/latest/C/ident/debugfs_remove">debugfs_remove</a></pre> </blockquote> The whole tracking amdgpu_debugfs_add_files() and the underlying DRM function do are completely nonsense and was only added because somebody didn't knew that this stuff is automatically removed. The only functionality amdgpu_debugfs_add_files() still provides is protecting to not try to add files twice. And that in turn is a coding bug we should probably fix :) Regards, Christian. <blockquote type="cite" cite="mid:MW2PR12MB2586787CBB596190ECDF754789600@MW2PR12MB2586.namprd12.prod.outlook.com"> <pre class="moz-quote-pre" wrap=""> If so, we need a separate patch to cleanup them in a batch. BR, Xiaojie ________________________________________ From: Christian König <a class="moz-txt-link-rfc2396E" href="mailto:ckoenig.leichtzumerken@gmail.com"><ckoenig.leichtzumerken@gmail.com></a> Sent: Monday, July 13, 2020 4:38 PM To: Yuan, Xiaojie; <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> Subject: Re: [PATCH] drm/amdgpu: use ARRAY_SIZE() to add amdgpu debugfs files Am 13.07.20 um 07:59 schrieb Xiaojie Yuan: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">to easily add new debugfs file w/o changing the hardcoded list count. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> In general a good idea, but I would rather like to see amdgpu_debugfs_add_files() completely removed and debugfs_create_file() used directly instead. Christian. </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> Signed-off-by: Xiaojie Yuan <a class="moz-txt-link-rfc2396E" href="mailto:xiaojie.yuan@amd.com"><xiaojie.yuan@amd.com></a> --- drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 6 ++++-- drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 3 ++- drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 3 ++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index b8ce43c28116..58d4c219178a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -781,8 +781,10 @@ int amdgpu_debugfs_fence_init(struct amdgpu_device *adev) { #if defined(CONFIG_DEBUG_FS) if (amdgpu_sriov_vf(adev)) - return amdgpu_debugfs_add_files(adev, amdgpu_debugfs_fence_list_sriov, 1); - return amdgpu_debugfs_add_files(adev, amdgpu_debugfs_fence_list, 2); + return amdgpu_debugfs_add_files(adev, amdgpu_debugfs_fence_list_sriov, + ARRAY_SIZE(amdgpu_debugfs_fence_list_sriov)); + return amdgpu_debugfs_add_files(adev, amdgpu_debugfs_fence_list, + ARRAY_SIZE(amdgpu_debugfs_fence_list)); #else return 0; #endif diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c index 77d988a0033f..8c64d8d6cb82 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c @@ -928,7 +928,8 @@ static const struct drm_info_list amdgpu_debugfs_gem_list[] = { int amdgpu_debugfs_gem_init(struct amdgpu_device *adev) { #if defined(CONFIG_DEBUG_FS) - return amdgpu_debugfs_add_files(adev, amdgpu_debugfs_gem_list, 1); + return amdgpu_debugfs_add_files(adev, amdgpu_debugfs_gem_list, + ARRAY_SIZE(amdgpu_debugfs_gem_list)); #endif return 0; } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c index 4ffc32b78745..dcd492170598 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c @@ -468,7 +468,8 @@ static const struct drm_info_list amdgpu_debugfs_sa_list[] = { int amdgpu_debugfs_sa_init(struct amdgpu_device *adev) { #if defined(CONFIG_DEBUG_FS) - return amdgpu_debugfs_add_files(adev, amdgpu_debugfs_sa_list, 1); + return amdgpu_debugfs_add_files(adev, amdgpu_debugfs_sa_list, + ARRAY_SIZE(amdgpu_debugfs_sa_list)); #else return 0; #endif </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> </pre> </blockquote> </body> </html>