Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs

["Lazar, Lijo" <lijo.lazar@amd.com>]

On 10/5/2021 10:15 PM, Christian König wrote:
> Am 05.10.21 um 15:49 schrieb Das, Nirmoy:
>>
>> On 10/5/2021 3:22 PM, Christian König wrote:
>>>
>>>
>>> Am 05.10.21 um 15:11 schrieb Nirmoy Das:
>>>> Debugfs core APIs will throw -EPERM when user disables debugfs
>>>> using CONFIG_DEBUG_FS_ALLOW_NONE or with kernel param. We shouldn't
>>>> see that as an error. Also validate drm root dentry before creating
>>>> amdgpu debugfs files.
>>>>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 10 ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> index 6611b3c7c149..d786072e918b 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> @@ -1617,6 +1617,16 @@ int amdgpu_debugfs_init(struct amdgpu_device 
>>>> *adev)
>>>>       struct dentry *ent;
>>>>       int r, i;
>>>>   +    if (IS_ERR(root)) {
>>>> +        /* When debugfs is disabled we get -EPERM which is not an
>>>> +         * error as this is user controllable.
>>>> +         */
>>>
>>> Well setting primary->debugfs_root to an error code is probably not a 
>>> good idea to begin with.
>>>
>>> When debugfs is disabled that should most likely be NULL.
>>
>>
>> If we set primary->debugfs_root to  NULL then we need to add bunch of 
>> NULL checks everywhere before creating any debugfs files
>>
>> because debugfs_create_{file|dir}() with NULL root is still valid.  I 
>> am assuming a hypothetical case when debugfs_root dir creation fails 
>> even with debugfs enabled
>>
>> but further calls are successful.  This wont be a problem if we 
>> propagate the error code.
> 
> Yeah, but an error code in members is ugly like hell and potentially 
> causes crashes instead.
> 
> I strongly suggest to fix this so that root is NULL when debugfs isn't 
> available and we add proper checks for that instead.

This shouldn't be done. A NULL is a valid parent for debugfs API. An 
invalid parent is always checked like this
	      if (IS_ERR(parent))
                 return parent;

Instead of adding redundant work like NULL checks, let the API do its 
work and don't break the API contract. For ex: usage of sample client, 
you may look at the drm usage; it does the same.

Thanks,
Lijo

> 
> Regards,
> Christian.
> 
>>
>>
>> Regards,
>>
>> Nirmoy
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>> +        if (PTR_ERR(root) == -EPERM)
>>>> +            return 0;
>>>> +
>>>> +        return PTR_ERR(ent);
>>>> +    }
>>>> +
>>>>       ent = debugfs_create_file("amdgpu_preempt_ib", 0600, root, adev,
>>>>                     &fops_ib_preempt);
>>>>       if (IS_ERR(ent)) {
>>>
>