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

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

Am 06.10.21 um 08:55 schrieb Lazar, Lijo:
>
>
> 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;

Yeah, that might work as well.

Potentially a good idea to add that to both the core drm function and 
the amdgpu function. and not attempt to create debugfs files in the 
first place.

Christian.

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