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

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

On 10/6/2021 12:05 PM, Christian König wrote:
> Am 06.10.21 um 08:32 schrieb Lazar, Lijo:
>>
>>
>> On 10/6/2021 11:49 AM, Christian König wrote:
>>> 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.
>>>
>>
>> One escape route may be - add another export from debugfs like 
>> debugfs_is_valid_node() which adheres to the current logic in debugfs 
>> API and use that in client code. Whenever debugfs changes to a 
>> different logic from IS_ERR, let that be changed.
> 
> Well that would then rather be drm_is_debugfs_enabled(), because that we 
> separate debugfs handling into a drm core and individual drivers is drm 
> specific.
> 

Had one more look and looks like this will do the job. In other cases, 
API usage is allowed.

	if (!debugfs_initialized())
		return;

Thanks,
Lijo

> Christian.
> 
>>
>> Thanks,
>> Lijo
>>
>>> 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)) {
>>>>>>>
>>>>>
>>>
>