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)) {
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)) {
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)) {
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)) {
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)) {
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)) { >
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)) { >>
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)) { >>> >
dri-devel@lists.freedesktop.org
-
Christian König -
Das, Nirmoy -
Lazar, Lijo