Debugfs APIs returns encoded error on failure instead of NULL and for drm primary/minor debugfs directories, we save the returned value in the dentry pointer and pass it on to drm drivers to further create debugfs files/directories. Error conditions are handled by debugfs APIs, so no need to check for NULL, as saved dentry pointers will either contain a valid pointer or an error code.
CC: Maarten Lankhorst maarten.lankhorst@linux.intel.com CC: Maxime Ripard mripard@kernel.org CC: Thomas Zimmermann tzimmermann@suse.de CC: David Airlie airlied@linux.ie CC: Daniel Vetter daniel@ffwll.ch
Signed-off-by: Nirmoy Das nirmoy.das@amd.com --- drivers/gpu/drm/drm_debugfs.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c index b0a826489488..0073854a4383 100644 --- a/drivers/gpu/drm/drm_debugfs.c +++ b/drivers/gpu/drm/drm_debugfs.c @@ -272,9 +272,6 @@ static void drm_debugfs_remove_all_files(struct drm_minor *minor)
void drm_debugfs_cleanup(struct drm_minor *minor) { - if (!minor->debugfs_root) - return; - drm_debugfs_remove_all_files(minor);
debugfs_remove_recursive(minor->debugfs_root); @@ -419,9 +416,6 @@ void drm_debugfs_connector_add(struct drm_connector *connector) struct drm_minor *minor = connector->dev->primary; struct dentry *root;
- if (!minor->debugfs_root) - return; - root = debugfs_create_dir(connector->name, minor->debugfs_root); connector->debugfs_entry = root;
@@ -440,9 +434,6 @@ void drm_debugfs_connector_add(struct drm_connector *connector)
void drm_debugfs_connector_remove(struct drm_connector *connector) { - if (!connector->debugfs_entry) - return; - debugfs_remove_recursive(connector->debugfs_entry);
connector->debugfs_entry = NULL; -- 2.32.0
For debugfs directory, it is recommended to save the result and pass over to next debugfs API for creating debugfs files/directories. Error conditions are handled by debugfs APIs.
CC: Christian Koenig christian.koenig@amd.com CC: Huang Rui ray.huang@amd.com CC: David Airlie airlied@linux.ie CC: Daniel Vetter daniel@ffwll.ch
Signed-off-by: Nirmoy Das nirmoy.das@amd.com --- drivers/gpu/drm/ttm/ttm_device.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c index be24bb6cefd0..225ae6b0b4c2 100644 --- a/drivers/gpu/drm/ttm/ttm_device.c +++ b/drivers/gpu/drm/ttm/ttm_device.c @@ -77,9 +77,6 @@ static int ttm_global_init(void) si_meminfo(&si);
ttm_debugfs_root = debugfs_create_dir("ttm", NULL); - if (IS_ERR(ttm_debugfs_root)) { - ttm_debugfs_root = NULL; - }
/* Limit the number of pages in the pool to about 50% of the total * system memory. @@ -108,8 +105,7 @@ static int ttm_global_init(void) debugfs_create_atomic_t("buffer_objects", 0444, ttm_debugfs_root, &glob->bo_count); out: - if (ret && ttm_debugfs_root) - debugfs_remove(ttm_debugfs_root); + debugfs_remove(ttm_debugfs_root); if (ret) --ttm_glob_use_count; mutex_unlock(&ttm_global_mutex); -- 2.32.0
Am 11.10.21 um 21:06 schrieb Nirmoy Das:
For debugfs directory, it is recommended to save the result and pass over to next debugfs API for creating debugfs files/directories. Error conditions are handled by debugfs APIs.
CC: Christian Koenig christian.koenig@amd.com CC: Huang Rui ray.huang@amd.com CC: David Airlie airlied@linux.ie CC: Daniel Vetter daniel@ffwll.ch
Signed-off-by: Nirmoy Das nirmoy.das@amd.com
Please also add a comment to ttm_debugfs_root that this can also be an ERR_PTR and should not be dereferenced.
Apart from that looks good to me.
Christian.
drivers/gpu/drm/ttm/ttm_device.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c index be24bb6cefd0..225ae6b0b4c2 100644 --- a/drivers/gpu/drm/ttm/ttm_device.c +++ b/drivers/gpu/drm/ttm/ttm_device.c @@ -77,9 +77,6 @@ static int ttm_global_init(void) si_meminfo(&si);
ttm_debugfs_root = debugfs_create_dir("ttm", NULL);
if (IS_ERR(ttm_debugfs_root)) {
ttm_debugfs_root = NULL;}
/* Limit the number of pages in the pool to about 50% of the total
- system memory.
@@ -108,8 +105,7 @@ static int ttm_global_init(void) debugfs_create_atomic_t("buffer_objects", 0444, ttm_debugfs_root, &glob->bo_count); out:
- if (ret && ttm_debugfs_root)
debugfs_remove(ttm_debugfs_root);
- debugfs_remove(ttm_debugfs_root); if (ret) --ttm_glob_use_count; mutex_unlock(&ttm_global_mutex);
-- 2.32.0
On 10/12/2021 9:09 AM, Christian König wrote:
Am 11.10.21 um 21:06 schrieb Nirmoy Das:
For debugfs directory, it is recommended to save the result and pass over to next debugfs API for creating debugfs files/directories. Error conditions are handled by debugfs APIs.
CC: Christian Koenig christian.koenig@amd.com CC: Huang Rui ray.huang@amd.com CC: David Airlie airlied@linux.ie CC: Daniel Vetter daniel@ffwll.ch
Signed-off-by: Nirmoy Das nirmoy.das@amd.com
Please also add a comment to ttm_debugfs_root that this can also be an ERR_PTR and should not be dereferenced.
Thanks, I will resend with a comment to ttm_debugfs_root.
Nirmoy
Apart from that looks good to me.
Christian.
drivers/gpu/drm/ttm/ttm_device.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c index be24bb6cefd0..225ae6b0b4c2 100644 --- a/drivers/gpu/drm/ttm/ttm_device.c +++ b/drivers/gpu/drm/ttm/ttm_device.c @@ -77,9 +77,6 @@ static int ttm_global_init(void) si_meminfo(&si);
ttm_debugfs_root = debugfs_create_dir("ttm", NULL); - if (IS_ERR(ttm_debugfs_root)) { - ttm_debugfs_root = NULL; - }
/* Limit the number of pages in the pool to about 50% of the total * system memory. @@ -108,8 +105,7 @@ static int ttm_global_init(void) debugfs_create_atomic_t("buffer_objects", 0444, ttm_debugfs_root, &glob->bo_count); out: - if (ret && ttm_debugfs_root) - debugfs_remove(ttm_debugfs_root); + debugfs_remove(ttm_debugfs_root); if (ret) --ttm_glob_use_count; mutex_unlock(&ttm_global_mutex); -- 2.32.0
Do not check for NULL value as drm.primary->debugfs_root will either contain a valid pointer or an encoded error instead of NULL.
CC: Jani Nikula jani.nikula@linux.intel.com CC: Joonas Lahtinen joonas.lahtinen@linux.intel.com CC: Rodrigo Vivi rodrigo.vivi@intel.com CC: David Airlie airlied@linux.ie CC: Daniel Vetter daniel@ffwll.ch
Signed-off-by: Nirmoy Das nirmoy.das@amd.com --- drivers/gpu/drm/i915/gt/debugfs_gt.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/debugfs_gt.c b/drivers/gpu/drm/i915/gt/debugfs_gt.c index 591eb60785db..95ca1b3ad320 100644 --- a/drivers/gpu/drm/i915/gt/debugfs_gt.c +++ b/drivers/gpu/drm/i915/gt/debugfs_gt.c @@ -16,9 +16,6 @@ void debugfs_gt_register(struct intel_gt *gt) { struct dentry *root;
- if (!gt->i915->drm.primary->debugfs_root) - return; - root = debugfs_create_dir("gt", gt->i915->drm.primary->debugfs_root); if (IS_ERR(root)) return; -- 2.32.0
Debugfs APIs returns encoded error on failure so use debugfs_lookup() instead of checking for NULL.
CC: Lukas Wunner lukas@wunner.de CC: David Airlie airlied@linux.ie CC: Daniel Vetter daniel@ffwll.ch CC: Maarten Lankhorst maarten.lankhorst@linux.intel.com CC: Maxime Ripard mripard@kernel.org CC: Thomas Zimmermann tzimmermann@suse.de
Signed-off-by: Nirmoy Das nirmoy.das@amd.com --- drivers/gpu/vga/vga_switcheroo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c index 365e6ddbe90f..a331525f0b32 100644 --- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -914,7 +914,7 @@ static void vga_switcheroo_debugfs_fini(struct vgasr_priv *priv) static void vga_switcheroo_debugfs_init(struct vgasr_priv *priv) { /* already initialised */ - if (priv->debugfs_root) + if (debugfs_lookup("vgaswitcheroo", NULL)) return;
priv->debugfs_root = debugfs_create_dir("vgaswitcheroo", NULL); -- 2.32.0
On Mon, Oct 11, 2021 at 09:06:07PM +0200, Nirmoy Das wrote:
Debugfs APIs returns encoded error on failure so use debugfs_lookup() instead of checking for NULL.
[...]
--- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -914,7 +914,7 @@ static void vga_switcheroo_debugfs_fini(struct vgasr_priv *priv) static void vga_switcheroo_debugfs_init(struct vgasr_priv *priv) { /* already initialised */
- if (priv->debugfs_root)
if (debugfs_lookup("vgaswitcheroo", NULL)) return;
priv->debugfs_root = debugfs_create_dir("vgaswitcheroo", NULL);
If debugfs_create_dir() returns an error code, it does make sense to retry the call when another vga_switcheroo client registers later. I like that.
However I'd prefer simply changing this to explicitly check for NULL, i.e.:
- if (priv->debugfs_root) + if (priv->debugfs_root == NULL)
It's just as clear as calling debugfs_lookup() and it has way less overhead. Granted, this isn't a hot path, but it's called on boot, and the less code we execute, the faster the machine boots.
Thanks,
Lukas
On Mon, Oct 11, 2021 at 10:24:29PM +0200, Lukas Wunner wrote:
On Mon, Oct 11, 2021 at 09:06:07PM +0200, Nirmoy Das wrote:
Debugfs APIs returns encoded error on failure so use debugfs_lookup() instead of checking for NULL.
[...]
--- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -914,7 +914,7 @@ static void vga_switcheroo_debugfs_fini(struct vgasr_priv *priv) static void vga_switcheroo_debugfs_init(struct vgasr_priv *priv) { /* already initialised */
- if (priv->debugfs_root)
if (debugfs_lookup("vgaswitcheroo", NULL)) return;
priv->debugfs_root = debugfs_create_dir("vgaswitcheroo", NULL);
If debugfs_create_dir() returns an error code, it does make sense to retry the call when another vga_switcheroo client registers later. I like that.
However I'd prefer simply changing this to explicitly check for NULL, i.e.:
- if (priv->debugfs_root)
- if (priv->debugfs_root == NULL)
Apologies, I meant:
- if (priv->debugfs_root) + if (priv->debugfs_root && !IS_ERR(priv->debugfs_root))
Thanks,
Lukas
It's just as clear as calling debugfs_lookup() and it has way less overhead. Granted, this isn't a hot path, but it's called on boot, and the less code we execute, the faster the machine boots.
On 10/12/2021 8:48 AM, Lukas Wunner wrote:
On Mon, Oct 11, 2021 at 10:24:29PM +0200, Lukas Wunner wrote:
On Mon, Oct 11, 2021 at 09:06:07PM +0200, Nirmoy Das wrote:
Debugfs APIs returns encoded error on failure so use debugfs_lookup() instead of checking for NULL.
[...]
--- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -914,7 +914,7 @@ static void vga_switcheroo_debugfs_fini(struct vgasr_priv *priv) static void vga_switcheroo_debugfs_init(struct vgasr_priv *priv) { /* already initialised */
- if (priv->debugfs_root)
if (debugfs_lookup("vgaswitcheroo", NULL)) return;
priv->debugfs_root = debugfs_create_dir("vgaswitcheroo", NULL);
If debugfs_create_dir() returns an error code, it does make sense to retry the call when another vga_switcheroo client registers later. I like that.
However I'd prefer simply changing this to explicitly check for NULL, i.e.:
- if (priv->debugfs_root)
- if (priv->debugfs_root == NULL)
Apologies, I meant:
- if (priv->debugfs_root)
- if (priv->debugfs_root && !IS_ERR(priv->debugfs_root))
Thanks for the suggestion, Lukas. I will update that and resend.
Nirmoy
Thanks,
Lukas
It's just as clear as calling debugfs_lookup() and it has way less overhead. Granted, this isn't a hot path, but it's called on boot, and the less code we execute, the faster the machine boots.
dri-devel@lists.freedesktop.org
-
Christian König -
Das, Nirmoy -
Lukas Wunner -
Nirmoy Das