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

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

Am 06.10.21 um 06:51 schrieb Lazar, Lijo:
>
>
> 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.

Yeah, but that is horrible API design and should be avoided.

ERR_PTR(), PTR_ERR(), IS_ERR() and similar are supposed to be used as 
alternative to signaling errors as return values from functions and 
should *never* ever be used to signal errors in pointer members.

Regards,
Christian.

>
> 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)) {
>>>>
>>