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