* Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs [not found] ` <182c1454-102e-cf2f-80eb-09baa57f8e82@amd.com> @ 2021-10-05 16:45 ` Christian König 2021-10-06 4:51 ` Lazar, Lijo 0 siblings, 1 reply; 8+ messages in thread From: Christian König @ 2021-10-05 16:45 UTC (permalink / raw) To: Das, Nirmoy, amd-gfx, dri-devel, Daniel Vetter; +Cc: shashank.sharma 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. 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)) { >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs 2021-10-05 16:45 ` [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs Christian König @ 2021-10-06 4:51 ` Lazar, Lijo 2021-10-06 6:19 ` Christian König 0 siblings, 1 reply; 8+ messages in thread From: Lazar, Lijo @ 2021-10-06 4:51 UTC (permalink / raw) To: Christian König, Das, Nirmoy, amd-gfx, dri-devel, Daniel Vetter Cc: shashank.sharma 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. 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)) { >>> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs 2021-10-06 4:51 ` Lazar, Lijo @ 2021-10-06 6:19 ` Christian König 2021-10-06 6:32 ` Lazar, Lijo 0 siblings, 1 reply; 8+ messages in thread From: Christian König @ 2021-10-06 6:19 UTC (permalink / raw) To: Lazar, Lijo, Das, Nirmoy, amd-gfx, dri-devel, Daniel Vetter Cc: shashank.sharma 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)) { >>>> >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs 2021-10-06 6:19 ` Christian König @ 2021-10-06 6:32 ` Lazar, Lijo 2021-10-06 6:35 ` Christian König 0 siblings, 1 reply; 8+ messages in thread From: Lazar, Lijo @ 2021-10-06 6:32 UTC (permalink / raw) To: Christian König, Das, Nirmoy, amd-gfx, dri-devel, Daniel Vetter Cc: shashank.sharma 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. 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)) { >>>>> >>> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs 2021-10-06 6:32 ` Lazar, Lijo @ 2021-10-06 6:35 ` Christian König 2021-10-06 6:55 ` Lazar, Lijo 0 siblings, 1 reply; 8+ messages in thread From: Christian König @ 2021-10-06 6:35 UTC (permalink / raw) To: Lazar, Lijo, Das, Nirmoy, amd-gfx, dri-devel, Daniel Vetter Cc: shashank.sharma 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. 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)) { >>>>>> >>>> >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs 2021-10-06 6:35 ` Christian König @ 2021-10-06 6:55 ` Lazar, Lijo 2021-10-06 6:59 ` Christian König 0 siblings, 1 reply; 8+ messages in thread From: Lazar, Lijo @ 2021-10-06 6:55 UTC (permalink / raw) To: Christian König, Das, Nirmoy, amd-gfx, dri-devel, Daniel Vetter Cc: shashank.sharma 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)) { >>>>>>> >>>>> >>> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs 2021-10-06 6:55 ` Lazar, Lijo @ 2021-10-06 6:59 ` Christian König 2021-10-06 8:52 ` Das, Nirmoy 0 siblings, 1 reply; 8+ messages in thread From: Christian König @ 2021-10-06 6:59 UTC (permalink / raw) To: Lazar, Lijo, Das, Nirmoy, amd-gfx, dri-devel, Daniel Vetter Cc: shashank.sharma 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)) { >>>>>>>> >>>>>> >>>> >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs 2021-10-06 6:59 ` Christian König @ 2021-10-06 8:52 ` Das, Nirmoy 0 siblings, 0 replies; 8+ messages in thread From: Das, Nirmoy @ 2021-10-06 8:52 UTC (permalink / raw) To: Christian König, Lazar, Lijo, amd-gfx, dri-devel, Daniel Vetter Cc: shashank.sharma 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)) { >>>>>>>>> >>>>>>> >>>>> >>> > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-10-06 8:52 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20211005131115.25595-1-nirmoy.das@amd.com>
[not found] ` <ab673b77-e8b7-94a4-76f6-588ad9e472f7@amd.com>
[not found] ` <182c1454-102e-cf2f-80eb-09baa57f8e82@amd.com>
2021-10-05 16:45 ` [PATCH 1/1] drm/amdgpu: ignore -EPERM error from debugfs Christian König
2021-10-06 4:51 ` Lazar, Lijo
2021-10-06 6:19 ` Christian König
2021-10-06 6:32 ` Lazar, Lijo
2021-10-06 6:35 ` Christian König
2021-10-06 6:55 ` Lazar, Lijo
2021-10-06 6:59 ` Christian König
2021-10-06 8:52 ` Das, Nirmoy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox