[PATCH v2 1/4] dri: do not check for NULL debugfs dentry

[PATCH v2 1/4] dri: do not check for NULL debugfs dentry

[Nirmoy Das]

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.

Also document this for future reference.

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 ---------
 drivers/gpu/drm/drm_drv.c     |  1 +
 include/drm/drm_file.h        | 28 ++++++++++++++++++++++++----
 3 files changed, 25 insertions(+), 13 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;
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 7a5097467ba5..918f302d9c43 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -64,6 +64,7 @@ static struct idr drm_minors_idr;
  */
 static bool drm_core_init_complete;

+/* Do not deference this pointer as it will contain ERR_PTR on error. */
 static struct dentry *drm_debugfs_root;

 DEFINE_STATIC_SRCU(drm_unplug_srcu);
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index a3acb7ac3550..3a30fc4d8905 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -69,15 +69,35 @@ enum drm_minor_type {
  */
 struct drm_minor {
 	/* private: */
-	int index;			/* Minor device number */
-	int type;                       /* Control or render */
-	struct device *kdev;		/* Linux device */
+	/** @index: minor device number. */
+	int index;
+
+	/** @type: minor device type: primary, control, render. */
+	int type;
+
+	/** @kdev: Linux device pointer. */
+	struct device *kdev;
+
+	/** @dev: drm device pointer. */
 	struct drm_device *dev;

+
+	/** @debugfs_root:
+	 *
+	 * Dentry for /sys/kernel/debug/dri/@index debugfs dir. Do not
+	 * deference this pointer as it will contain ERR_PTR on error.
+	 */
 	struct dentry *debugfs_root;

+	/** @debugfs_list:
+	 *
+	 * A list to keep track of debugfs dentries created using
+	 * drm_debugfs_create_files() by drm drivers.
+	 */
 	struct list_head debugfs_list;
-	struct mutex debugfs_lock; /* Protects debugfs_list. */
+
+	/** @debugfs_lock: Protects debugfs_list. */
+	struct mutex debugfs_lock;
 };

 /**
--
2.32.0

[PATCH 2/4] drm/ttm: do not set NULL to debugfs dentry

[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>
---
 drivers/gpu/drm/ttm/ttm_device.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c
index be24bb6cefd0..2c66f06198e9 100644
--- a/drivers/gpu/drm/ttm/ttm_device.c
+++ b/drivers/gpu/drm/ttm/ttm_device.c
@@ -44,6 +44,7 @@ static unsigned ttm_glob_use_count;
 struct ttm_global ttm_glob;
 EXPORT_SYMBOL(ttm_glob);
 
+/* Do not deference this pointer as it will contain ERR_PTR on error. */
 struct dentry *ttm_debugfs_root;
 
 static void ttm_global_release(void)
@@ -77,9 +78,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 +106,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

[PATCH 3/4] drm/i915/gt: do not check for NULL debugfs dentry

[Nirmoy Das]

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

[PATCH v2 4/4] vgaswitcheroo: do not check for NULL debugfs dentry

[Nirmoy Das]

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..07ab8d85e899 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 (priv->debugfs_root && !IS_ERR(priv->debugfs_root))
 		return;

 	priv->debugfs_root = debugfs_create_dir("vgaswitcheroo", NULL);
--
2.32.0

Re: [PATCH v2 1/4] dri: do not check for NULL debugfs dentry

[Das, Nirmoy]

Ah there are three typos :/

s/deference/dereference for this one and for the 2nd patch as well.


Regards,

Nirmoy

On 10/13/2021 8:35 PM, Nirmoy Das wrote:
> 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.
>
> Also document this for future reference.
>
> 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 ---------
>   drivers/gpu/drm/drm_drv.c     |  1 +
>   include/drm/drm_file.h        | 28 ++++++++++++++++++++++++----
>   3 files changed, 25 insertions(+), 13 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;
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 7a5097467ba5..918f302d9c43 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -64,6 +64,7 @@ static struct idr drm_minors_idr;
>    */
>   static bool drm_core_init_complete;
>
> +/* Do not deference this pointer as it will contain ERR_PTR on error. */
>   static struct dentry *drm_debugfs_root;
>
>   DEFINE_STATIC_SRCU(drm_unplug_srcu);
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index a3acb7ac3550..3a30fc4d8905 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -69,15 +69,35 @@ enum drm_minor_type {
>    */
>   struct drm_minor {
>   	/* private: */
> -	int index;			/* Minor device number */
> -	int type;                       /* Control or render */
> -	struct device *kdev;		/* Linux device */
> +	/** @index: minor device number. */
> +	int index;
> +
> +	/** @type: minor device type: primary, control, render. */
> +	int type;
> +
> +	/** @kdev: Linux device pointer. */
> +	struct device *kdev;
> +
> +	/** @dev: drm device pointer. */
>   	struct drm_device *dev;
>
> +
> +	/** @debugfs_root:
> +	 *
> +	 * Dentry for /sys/kernel/debug/dri/@index debugfs dir. Do not
> +	 * deference this pointer as it will contain ERR_PTR on error.
> +	 */
>   	struct dentry *debugfs_root;
>
> +	/** @debugfs_list:
> +	 *
> +	 * A list to keep track of debugfs dentries created using
> +	 * drm_debugfs_create_files() by drm drivers.
> +	 */
>   	struct list_head debugfs_list;
> -	struct mutex debugfs_lock; /* Protects debugfs_list. */
> +
> +	/** @debugfs_lock: Protects debugfs_list. */
> +	struct mutex debugfs_lock;
>   };
>
>   /**
> --
> 2.32.0
>

Re: [PATCH v2 4/4] vgaswitcheroo: do not check for NULL debugfs dentry

[Lukas Wunner]

On Wed, Oct 13, 2021 at 08:36:01PM +0200, Nirmoy Das wrote:
> Debugfs APIs returns encoded error on failure so use
> debugfs_lookup() instead of checking for NULL.

The commit message no longer matches up with the patch itself
(debugfs_lookup() isn't called).

My suggestion would be something like:

  Retry creation of the vga_switcheroo debugfs if a previous
  invocation of debugfs_create_dir() returned an error code.

With that addressed,
Reviewed-by: Lukas Wunner <lukas@wunner.de>

Thanks,

Lukas

> 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..07ab8d85e899 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 (priv->debugfs_root && !IS_ERR(priv->debugfs_root))
>  		return;
> 
>  	priv->debugfs_root = debugfs_create_dir("vgaswitcheroo", NULL);
> --
> 2.32.0

Re: [PATCH v2 4/4] vgaswitcheroo: do not check for NULL debugfs dentry

[Das, Nirmoy]

On 10/17/2021 10:03 PM, Lukas Wunner wrote:
> On Wed, Oct 13, 2021 at 08:36:01PM +0200, Nirmoy Das wrote:
>> Debugfs APIs returns encoded error on failure so use
>> debugfs_lookup() instead of checking for NULL.
> The commit message no longer matches up with the patch itself
> (debugfs_lookup() isn't called).
>
> My suggestion would be something like:
>
>    Retry creation of the vga_switcheroo debugfs if a previous
>    invocation of debugfs_create_dir() returned an error code.
>
> With that addressed,
> Reviewed-by: Lukas Wunner <lukas@wunner.de>


Thanks, Lukas. Yes, I missed that commit message modification.


Nirmoy

>
> Thanks,
>
> Lukas
>
>> 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..07ab8d85e899 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 (priv->debugfs_root && !IS_ERR(priv->debugfs_root))
>>   		return;
>>
>>   	priv->debugfs_root = debugfs_create_dir("vgaswitcheroo", NULL);
>> --
>> 2.32.0