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

["Das, Nirmoy" <nirmoy.das@amd.com>]

On 10/6/2021 8:59 AM, Christian König wrote:
> 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.


Sounds good, I will send patches to add this check.


Thanks,

Nirmoy


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