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