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

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

 





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;

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