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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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








[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux