Re: [PATCH] drm/amdgpu: use ARRAY_SIZE() to add amdgpu debugfs files

["Christian König" <christian.koenig@amd.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5102 bytes --]

Am 13.07.20 um 15:34 schrieb Yuan, Xiaojie:
> [AMD Official Use Only - Internal Distribution Only]
>
> Hi Chris,
>
> This was observed when I was trying to add a new debugfs file.

In this case please add the new file using debugfs_create_file() 
directly and don't touch this old code.

>   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

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:

> void  debugfs_remove 
> <https://elixir.bootlin.com/linux/latest/C/ident/debugfs_remove>(struct  dentry <https://elixir.bootlin.com/linux/latest/C/ident/dentry>  *dentry <https://elixir.bootlin.com/linux/latest/C/ident/dentry>);
> #define debugfs_remove_recursive 
> <https://elixir.bootlin.com/linux/latest/C/ident/debugfs_remove_recursive> 
> debugfs_remove 
> <https://elixir.bootlin.com/linux/latest/C/ident/debugfs_remove>

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.

>
> If so, we need a separate patch to cleanup them in a batch.
>
> BR,
> Xiaojie
>
> ________________________________________
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Monday, July 13, 2020 4:38 PM
> To: Yuan, Xiaojie; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: use ARRAY_SIZE() to add amdgpu debugfs files
>
> Am 13.07.20 um 07:59 schrieb Xiaojie Yuan:
>> to easily add new debugfs file w/o changing the hardcoded list count.
> 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.
>
>> Signed-off-by: Xiaojie Yuan <xiaojie.yuan@amd.com>
>> ---
>>    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


[-- Attachment #1.2: Type: text/html, Size: 6757 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx