[PATCH 1/5] dri: cleanup debugfs error handling

[PATCH 1/5] dri: cleanup debugfs error handling

[Nirmoy Das]

Debugfs API returns encoded error instead of NULL.
This patch cleanups drm debugfs error handling to
properly set dri and its minor's root dentry to NULL.

Also do not error out if dri/minor debugfs directory
creation fails as a debugfs error is not a fatal error.

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  | 25 +++++++++++++++++++++++--
 drivers/gpu/drm/drm_drv.c      | 16 ++++++++++------
 drivers/gpu/drm/drm_internal.h |  7 +++----
 3 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
index b0a826489488..af275a0c09b4 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -180,6 +180,9 @@ void drm_debugfs_create_files(const struct drm_info_list *files, int count,
 	struct drm_info_node *tmp;
 	int i;

+	if (!minor->debugfs_root)
+		return;
+
 	for (i = 0; i < count; i++) {
 		u32 features = files[i].driver_features;

@@ -203,7 +206,7 @@ void drm_debugfs_create_files(const struct drm_info_list *files, int count,
 }
 EXPORT_SYMBOL(drm_debugfs_create_files);

-int drm_debugfs_init(struct drm_minor *minor, int minor_id,
+void drm_debugfs_init(struct drm_minor *minor, int minor_id,
 		     struct dentry *root)
 {
 	struct drm_device *dev = minor->dev;
@@ -212,8 +215,16 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 	INIT_LIST_HEAD(&minor->debugfs_list);
 	mutex_init(&minor->debugfs_lock);
 	sprintf(name, "%d", minor_id);
+
+	if (!root)
+		goto error;
+
 	minor->debugfs_root = debugfs_create_dir(name, root);

+	if (IS_ERR(minor->debugfs_root))
+		goto error;
+
+
 	drm_debugfs_create_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES,
 				 minor->debugfs_root, minor);

@@ -230,7 +241,11 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
 	if (dev->driver->debugfs_init)
 		dev->driver->debugfs_init(minor);

-	return 0;
+	return;
+
+error:
+	minor->debugfs_root = NULL;
+	return;
 }


@@ -241,6 +256,9 @@ int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
 	struct drm_info_node *tmp;
 	int i;

+	if (!minor->debugfs_root)
+		return 0;
+
 	mutex_lock(&minor->debugfs_lock);
 	for (i = 0; i < count; i++) {
 		list_for_each_safe(pos, q, &minor->debugfs_list) {
@@ -261,6 +279,9 @@ static void drm_debugfs_remove_all_files(struct drm_minor *minor)
 {
 	struct drm_info_node *node, *tmp;

+	if (!minor->debugfs_root)
+		return;
+
 	mutex_lock(&minor->debugfs_lock);
 	list_for_each_entry_safe(node, tmp, &minor->debugfs_list, list) {
 		debugfs_remove(node->dent);
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 7a5097467ba5..fa57ec2d49bf 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -160,11 +160,7 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
 	if (!minor)
 		return 0;

-	ret = drm_debugfs_init(minor, minor->index, drm_debugfs_root);
-	if (ret) {
-		DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
-		goto err_debugfs;
-	}
+	drm_debugfs_init(minor, minor->index, drm_debugfs_root);

 	ret = device_add(minor->kdev);
 	if (ret)
@@ -1050,7 +1046,15 @@ static int __init drm_core_init(void)
 		goto error;
 	}

-	drm_debugfs_root = debugfs_create_dir("dri", NULL);
+	if (!debugfs_initialized()) {
+		drm_debugfs_root = NULL;
+	} else {
+		drm_debugfs_root = debugfs_create_dir("dri", NULL);
+		if (IS_ERR(drm_debugfs_root)) {
+			DRM_WARN("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
+			drm_debugfs_root = NULL;
+		}
+	}

 	ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops);
 	if (ret < 0)
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 17f3548c8ed2..e27a40166178 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -182,8 +182,8 @@ int drm_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev,

 /* drm_debugfs.c drm_debugfs_crc.c */
 #if defined(CONFIG_DEBUG_FS)
-int drm_debugfs_init(struct drm_minor *minor, int minor_id,
-		     struct dentry *root);
+void drm_debugfs_init(struct drm_minor *minor, int minor_id,
+		      struct dentry *root);
 void drm_debugfs_cleanup(struct drm_minor *minor);
 void drm_debugfs_connector_add(struct drm_connector *connector);
 void drm_debugfs_connector_remove(struct drm_connector *connector);
@@ -191,10 +191,9 @@ void drm_debugfs_crtc_add(struct drm_crtc *crtc);
 void drm_debugfs_crtc_remove(struct drm_crtc *crtc);
 void drm_debugfs_crtc_crc_add(struct drm_crtc *crtc);
 #else
-static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
+static inline void drm_debugfs_init(struct drm_minor *minor, int minor_id,
 				   struct dentry *root)
 {
-	return 0;
 }

 static inline void drm_debugfs_cleanup(struct drm_minor *minor)
--
2.32.0

[PATCH 2/5] drm/i915: check dri root before debugfs init

[Nirmoy Das]

Return early if dri minor root dentry is NULL.

CC: Zhenyu Wang <zhenyuw@linux.intel.com>
CC: Zhi Wang <zhi.a.wang@intel.com>
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/gvt/debugfs.c  | 3 +++
 drivers/gpu/drm/i915/i915_debugfs.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/gvt/debugfs.c b/drivers/gpu/drm/i915/gvt/debugfs.c
index 9f1c209d9251..2d47acaa03ee 100644
--- a/drivers/gpu/drm/i915/gvt/debugfs.c
+++ b/drivers/gpu/drm/i915/gvt/debugfs.c
@@ -187,6 +187,9 @@ void intel_gvt_debugfs_init(struct intel_gvt *gvt)
 {
 	struct drm_minor *minor = gvt->gt->i915->drm.primary;

+	if (!minor->debugfs_root)
+		return;
+
 	gvt->debugfs_root = debugfs_create_dir("gvt", minor->debugfs_root);

 	debugfs_create_ulong("num_tracked_mmio", 0444, gvt->debugfs_root,
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 44969f5dde50..d572b686edeb 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1012,6 +1012,9 @@ void i915_debugfs_register(struct drm_i915_private *dev_priv)
 	struct drm_minor *minor = dev_priv->drm.primary;
 	int i;

+	if (!minor->debugfs_root)
+		return;
+
 	i915_debugfs_params(dev_priv);

 	debugfs_create_file("i915_forcewake_user", S_IRUSR, minor->debugfs_root,
--
2.32.0

Re: [PATCH 2/5] drm/i915: check dri root before debugfs init

[Wang, Zhi A]

On 10/8/21 9:17 AM, Nirmoy Das wrote:
> Return early if dri minor root dentry is NULL.
> 
> CC: Zhenyu Wang <zhenyuw@linux.intel.com>
> CC: Zhi Wang <zhi.a.wang@intel.com>
> 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/gvt/debugfs.c  | 3 +++
>   drivers/gpu/drm/i915/i915_debugfs.c | 3 +++
>   2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/debugfs.c b/drivers/gpu/drm/i915/gvt/debugfs.c
> index 9f1c209d9251..2d47acaa03ee 100644
> --- a/drivers/gpu/drm/i915/gvt/debugfs.c
> +++ b/drivers/gpu/drm/i915/gvt/debugfs.c
> @@ -187,6 +187,9 @@ void intel_gvt_debugfs_init(struct intel_gvt *gvt)
>   {
>   	struct drm_minor *minor = gvt->gt->i915->drm.primary;
> 
> +	if (!minor->debugfs_root)
> +		return;
> +
>   	gvt->debugfs_root = debugfs_create_dir("gvt", minor->debugfs_root);
> 
>   	debugfs_create_ulong("num_tracked_mmio", 0444, gvt->debugfs_root,
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 44969f5dde50..d572b686edeb 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1012,6 +1012,9 @@ void i915_debugfs_register(struct drm_i915_private *dev_priv)
>   	struct drm_minor *minor = dev_priv->drm.primary;
>   	int i;
> 
> +	if (!minor->debugfs_root)
> +		return;
> +
>   	i915_debugfs_params(dev_priv);
> 
>   	debugfs_create_file("i915_forcewake_user", S_IRUSR, minor->debugfs_root,
> --
> 2.32.0
> 
Thanks for the patch. queued.
Reviewed-by: Zhi Wang <zhi.a.wang@intel.com>

Re: [PATCH 2/5] drm/i915: check dri root before debugfs init

[Das, Nirmoy]

Hi Zhi,


Please discard this patch,  review 
https://patchwork.freedesktop.org/patch/458554/?series=95690&rev=1 instead.

minor->debugfs_root wont be NULl as we save debugfs_create_dir()'s return value in that.

Regards,
Nirmoy

On 10/12/2021 11:59 AM, Wang, Zhi A wrote:
> On 10/8/21 9:17 AM, Nirmoy Das wrote:
>> Return early if dri minor root dentry is NULL.
>>
>> CC: Zhenyu Wang <zhenyuw@linux.intel.com>
>> CC: Zhi Wang <zhi.a.wang@intel.com>
>> 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/gvt/debugfs.c  | 3 +++
>>    drivers/gpu/drm/i915/i915_debugfs.c | 3 +++
>>    2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/debugfs.c b/drivers/gpu/drm/i915/gvt/debugfs.c
>> index 9f1c209d9251..2d47acaa03ee 100644
>> --- a/drivers/gpu/drm/i915/gvt/debugfs.c
>> +++ b/drivers/gpu/drm/i915/gvt/debugfs.c
>> @@ -187,6 +187,9 @@ void intel_gvt_debugfs_init(struct intel_gvt *gvt)
>>    {
>>    	struct drm_minor *minor = gvt->gt->i915->drm.primary;
>>
>> +	if (!minor->debugfs_root)
>> +		return;
>> +
>>    	gvt->debugfs_root = debugfs_create_dir("gvt", minor->debugfs_root);
>>
>>    	debugfs_create_ulong("num_tracked_mmio", 0444, gvt->debugfs_root,
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 44969f5dde50..d572b686edeb 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1012,6 +1012,9 @@ void i915_debugfs_register(struct drm_i915_private *dev_priv)
>>    	struct drm_minor *minor = dev_priv->drm.primary;
>>    	int i;
>>
>> +	if (!minor->debugfs_root)
>> +		return;
>> +
>>    	i915_debugfs_params(dev_priv);
>>
>>    	debugfs_create_file("i915_forcewake_user", S_IRUSR, minor->debugfs_root,
>> --
>> 2.32.0
>>
> Thanks for the patch. queued.
> Reviewed-by: Zhi Wang <zhi.a.wang@intel.com>

[PATCH 3/5] drm/radeon: check dri root before debugfs init

[Nirmoy Das]

Return early if dri minor root dentry is NULL.

CC: Alex Deucher <alexander.deucher@amd.com>
CC: "Christian König" <christian.koenig@amd.com>
CC: "Pan, Xinhui" <Xinhui.Pan@amd.com>

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/radeon/r100.c          | 9 +++++++++
 drivers/gpu/drm/radeon/r300.c          | 3 +++
 drivers/gpu/drm/radeon/r420.c          | 3 +++
 drivers/gpu/drm/radeon/r600.c          | 3 +++
 drivers/gpu/drm/radeon/radeon_dp_mst.c | 3 +++
 drivers/gpu/drm/radeon/radeon_fence.c  | 3 +++
 drivers/gpu/drm/radeon/radeon_gem.c    | 3 +++
 drivers/gpu/drm/radeon/radeon_ib.c     | 3 +++
 drivers/gpu/drm/radeon/radeon_pm.c     | 5 ++++-
 drivers/gpu/drm/radeon/radeon_ring.c   | 3 +++
 drivers/gpu/drm/radeon/radeon_ttm.c    | 3 +++
 drivers/gpu/drm/radeon/rs400.c         | 3 +++
 drivers/gpu/drm/radeon/rv515.c         | 3 +++
 13 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
index 2dd85ba1faa2..ae6c95b34013 100644
--- a/drivers/gpu/drm/radeon/r100.c
+++ b/drivers/gpu/drm/radeon/r100.c
@@ -3059,6 +3059,9 @@ void  r100_debugfs_rbbm_init(struct radeon_device *rdev)
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry *root = rdev->ddev->primary->debugfs_root;

+	if (!root)
+		return;
+
 	debugfs_create_file("r100_rbbm_info", 0444, root, rdev,
 			    &r100_debugfs_rbbm_info_fops);
 #endif
@@ -3069,6 +3072,9 @@ void r100_debugfs_cp_init(struct radeon_device *rdev)
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry *root = rdev->ddev->primary->debugfs_root;

+	if (!root)
+		return;
+
 	debugfs_create_file("r100_cp_ring_info", 0444, root, rdev,
 			    &r100_debugfs_cp_ring_info_fops);
 	debugfs_create_file("r100_cp_csq_fifo", 0444, root, rdev,
@@ -3081,6 +3087,9 @@ void  r100_debugfs_mc_info_init(struct radeon_device *rdev)
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry *root = rdev->ddev->primary->debugfs_root;

+	if (!root)
+		return;
+
 	debugfs_create_file("r100_mc_info", 0444, root, rdev,
 			    &r100_debugfs_mc_info_fops);
 #endif
diff --git a/drivers/gpu/drm/radeon/r300.c b/drivers/gpu/drm/radeon/r300.c
index 621ff174dff3..b22969e2394f 100644
--- a/drivers/gpu/drm/radeon/r300.c
+++ b/drivers/gpu/drm/radeon/r300.c
@@ -618,6 +618,9 @@ static void rv370_debugfs_pcie_gart_info_init(struct radeon_device *rdev)
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry *root = rdev->ddev->primary->debugfs_root;

+	if (!root)
+		return;
+
 	debugfs_create_file("rv370_pcie_gart_info", 0444, root, rdev,
 			    &rv370_debugfs_pcie_gart_info_fops);
 #endif
diff --git a/drivers/gpu/drm/radeon/r420.c b/drivers/gpu/drm/radeon/r420.c
index 7e6320e8c6a0..cdb4ac3e346b 100644
--- a/drivers/gpu/drm/radeon/r420.c
+++ b/drivers/gpu/drm/radeon/r420.c
@@ -494,6 +494,9 @@ void r420_debugfs_pipes_info_init(struct radeon_device *rdev)
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry *root = rdev->ddev->primary->debugfs_root;

+	if (!root)
+		return;
+
 	debugfs_create_file("r420_pipes_info", 0444, root, rdev,
 			    &r420_debugfs_pipes_info_fops);
 #endif
diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
index ca3fcae2adb5..d8f525cf0c3b 100644
--- a/drivers/gpu/drm/radeon/r600.c
+++ b/drivers/gpu/drm/radeon/r600.c
@@ -4360,6 +4360,9 @@ static void r600_debugfs_mc_info_init(struct radeon_device *rdev)
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry *root = rdev->ddev->primary->debugfs_root;

+	if (!root)
+		return;
+
 	debugfs_create_file("r600_mc_info", 0444, root, rdev,
 			    &r600_debugfs_mc_info_fops);

diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c
index ec867fa880a4..cf06da89bb7c 100644
--- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
+++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
@@ -771,6 +771,9 @@ void radeon_mst_debugfs_init(struct radeon_device *rdev)
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry *root = rdev->ddev->primary->debugfs_root;

+	if (!root)
+		return;
+
 	debugfs_create_file("radeon_mst_info", 0444, root, rdev,
 			    &radeon_debugfs_mst_info_fops);

diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c
index 73e3117420bf..11f30349de46 100644
--- a/drivers/gpu/drm/radeon/radeon_fence.c
+++ b/drivers/gpu/drm/radeon/radeon_fence.c
@@ -1006,6 +1006,9 @@ void radeon_debugfs_fence_init(struct radeon_device *rdev)
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry *root = rdev->ddev->primary->debugfs_root;

+	if (!root)
+		return;
+
 	debugfs_create_file("radeon_gpu_reset", 0444, root, rdev,
 			    &radeon_debugfs_gpu_reset_fops);
 	debugfs_create_file("radeon_fence_info", 0444, root, rdev,
diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c
index 458f92a70887..e6df1451af37 100644
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -890,6 +890,9 @@ void radeon_gem_debugfs_init(struct radeon_device *rdev)
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry *root = rdev->ddev->primary->debugfs_root;

+	if (!root)
+		return;
+
 	debugfs_create_file("radeon_gem_info", 0444, root, rdev,
 			    &radeon_debugfs_gem_info_fops);

diff --git a/drivers/gpu/drm/radeon/radeon_ib.c b/drivers/gpu/drm/radeon/radeon_ib.c
index 62b116727b4f..28316eb4fd49 100644
--- a/drivers/gpu/drm/radeon/radeon_ib.c
+++ b/drivers/gpu/drm/radeon/radeon_ib.c
@@ -311,6 +311,9 @@ static void radeon_debugfs_sa_init(struct radeon_device *rdev)
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry *root = rdev->ddev->primary->debugfs_root;

+	if (!root)
+		return;
+
 	debugfs_create_file("radeon_sa_info", 0444, root, rdev,
 			    &radeon_debugfs_sa_info_fops);
 #endif
diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
index c67b6ddb29a4..c09e574d04f0 100644
--- a/drivers/gpu/drm/radeon/radeon_pm.c
+++ b/drivers/gpu/drm/radeon/radeon_pm.c
@@ -1958,6 +1958,9 @@ static void radeon_debugfs_pm_init(struct radeon_device *rdev)
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry *root = rdev->ddev->primary->debugfs_root;

+	if (!root)
+		return;
+
 	debugfs_create_file("radeon_pm_info", 0444, root, rdev,
 			    &radeon_debugfs_pm_info_fops);

diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c
index 7e207276df37..31a5b1ebf7c9 100644
--- a/drivers/gpu/drm/radeon/radeon_ring.c
+++ b/drivers/gpu/drm/radeon/radeon_ring.c
@@ -550,6 +550,9 @@ static void radeon_debugfs_ring_init(struct radeon_device *rdev, struct radeon_r
 	const char *ring_name = radeon_debugfs_ring_idx_to_name(ring->idx);
 	struct dentry *root = rdev->ddev->primary->debugfs_root;

+	if (!root)
+		return;
+
 	if (ring_name)
 		debugfs_create_file(ring_name, 0444, root, ring,
 				    &radeon_debugfs_ring_info_fops);
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 11b21d605584..2e18ec93768d 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -942,6 +942,9 @@ static void radeon_ttm_debugfs_init(struct radeon_device *rdev)
 	struct drm_minor *minor = rdev->ddev->primary;
 	struct dentry *root = minor->debugfs_root;

+	if (!root)
+		return;
+
 	debugfs_create_file("radeon_vram", 0444, root, rdev,
 			    &radeon_ttm_vram_fops);

diff --git a/drivers/gpu/drm/radeon/rs400.c b/drivers/gpu/drm/radeon/rs400.c
index 6383f7a34bd8..b41a903a29c3 100644
--- a/drivers/gpu/drm/radeon/rs400.c
+++ b/drivers/gpu/drm/radeon/rs400.c
@@ -380,6 +380,9 @@ static void rs400_debugfs_pcie_gart_info_init(struct radeon_device *rdev)
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry *root = rdev->ddev->primary->debugfs_root;

+	if (!root)
+		return;
+
 	debugfs_create_file("rs400_gart_info", 0444, root, rdev,
 			    &rs400_debugfs_gart_info_fops);
 #endif
diff --git a/drivers/gpu/drm/radeon/rv515.c b/drivers/gpu/drm/radeon/rv515.c
index 63fb06e8e2d7..f39b6ab554f2 100644
--- a/drivers/gpu/drm/radeon/rv515.c
+++ b/drivers/gpu/drm/radeon/rv515.c
@@ -257,6 +257,9 @@ void rv515_debugfs(struct radeon_device *rdev)
 #if defined(CONFIG_DEBUG_FS)
 	struct dentry *root = rdev->ddev->primary->debugfs_root;

+	if (!root)
+		return;
+
 	debugfs_create_file("rv515_pipes_info", 0444, root, rdev,
 			    &rv515_debugfs_pipes_info_fops);
 	debugfs_create_file("rv515_ga_info", 0444, root, rdev,
--
2.32.0

Re: [PATCH 3/5] drm/radeon: check dri root before debugfs init

[Christian König]

Am 08.10.21 um 11:17 schrieb Nirmoy Das:
> Return early if dri minor root dentry is NULL.
>
> CC: Alex Deucher <alexander.deucher@amd.com>
> CC: "Christian König" <christian.koenig@amd.com>
> CC: "Pan, Xinhui" <Xinhui.Pan@amd.com>
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>

Acked-by: Christian König <christian.koenig@amd.com>

Where are the other patches from the series?

Thanks,
Christian.

> ---
>   drivers/gpu/drm/radeon/r100.c          | 9 +++++++++
>   drivers/gpu/drm/radeon/r300.c          | 3 +++
>   drivers/gpu/drm/radeon/r420.c          | 3 +++
>   drivers/gpu/drm/radeon/r600.c          | 3 +++
>   drivers/gpu/drm/radeon/radeon_dp_mst.c | 3 +++
>   drivers/gpu/drm/radeon/radeon_fence.c  | 3 +++
>   drivers/gpu/drm/radeon/radeon_gem.c    | 3 +++
>   drivers/gpu/drm/radeon/radeon_ib.c     | 3 +++
>   drivers/gpu/drm/radeon/radeon_pm.c     | 5 ++++-
>   drivers/gpu/drm/radeon/radeon_ring.c   | 3 +++
>   drivers/gpu/drm/radeon/radeon_ttm.c    | 3 +++
>   drivers/gpu/drm/radeon/rs400.c         | 3 +++
>   drivers/gpu/drm/radeon/rv515.c         | 3 +++
>   13 files changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
> index 2dd85ba1faa2..ae6c95b34013 100644
> --- a/drivers/gpu/drm/radeon/r100.c
> +++ b/drivers/gpu/drm/radeon/r100.c
> @@ -3059,6 +3059,9 @@ void  r100_debugfs_rbbm_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>   	struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +	if (!root)
> +		return;
> +
>   	debugfs_create_file("r100_rbbm_info", 0444, root, rdev,
>   			    &r100_debugfs_rbbm_info_fops);
>   #endif
> @@ -3069,6 +3072,9 @@ void r100_debugfs_cp_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>   	struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +	if (!root)
> +		return;
> +
>   	debugfs_create_file("r100_cp_ring_info", 0444, root, rdev,
>   			    &r100_debugfs_cp_ring_info_fops);
>   	debugfs_create_file("r100_cp_csq_fifo", 0444, root, rdev,
> @@ -3081,6 +3087,9 @@ void  r100_debugfs_mc_info_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>   	struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +	if (!root)
> +		return;
> +
>   	debugfs_create_file("r100_mc_info", 0444, root, rdev,
>   			    &r100_debugfs_mc_info_fops);
>   #endif
> diff --git a/drivers/gpu/drm/radeon/r300.c b/drivers/gpu/drm/radeon/r300.c
> index 621ff174dff3..b22969e2394f 100644
> --- a/drivers/gpu/drm/radeon/r300.c
> +++ b/drivers/gpu/drm/radeon/r300.c
> @@ -618,6 +618,9 @@ static void rv370_debugfs_pcie_gart_info_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>   	struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +	if (!root)
> +		return;
> +
>   	debugfs_create_file("rv370_pcie_gart_info", 0444, root, rdev,
>   			    &rv370_debugfs_pcie_gart_info_fops);
>   #endif
> diff --git a/drivers/gpu/drm/radeon/r420.c b/drivers/gpu/drm/radeon/r420.c
> index 7e6320e8c6a0..cdb4ac3e346b 100644
> --- a/drivers/gpu/drm/radeon/r420.c
> +++ b/drivers/gpu/drm/radeon/r420.c
> @@ -494,6 +494,9 @@ void r420_debugfs_pipes_info_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>   	struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +	if (!root)
> +		return;
> +
>   	debugfs_create_file("r420_pipes_info", 0444, root, rdev,
>   			    &r420_debugfs_pipes_info_fops);
>   #endif
> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
> index ca3fcae2adb5..d8f525cf0c3b 100644
> --- a/drivers/gpu/drm/radeon/r600.c
> +++ b/drivers/gpu/drm/radeon/r600.c
> @@ -4360,6 +4360,9 @@ static void r600_debugfs_mc_info_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>   	struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +	if (!root)
> +		return;
> +
>   	debugfs_create_file("r600_mc_info", 0444, root, rdev,
>   			    &r600_debugfs_mc_info_fops);
>
> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> index ec867fa880a4..cf06da89bb7c 100644
> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> @@ -771,6 +771,9 @@ void radeon_mst_debugfs_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>   	struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +	if (!root)
> +		return;
> +
>   	debugfs_create_file("radeon_mst_info", 0444, root, rdev,
>   			    &radeon_debugfs_mst_info_fops);
>
> diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c
> index 73e3117420bf..11f30349de46 100644
> --- a/drivers/gpu/drm/radeon/radeon_fence.c
> +++ b/drivers/gpu/drm/radeon/radeon_fence.c
> @@ -1006,6 +1006,9 @@ void radeon_debugfs_fence_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>   	struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +	if (!root)
> +		return;
> +
>   	debugfs_create_file("radeon_gpu_reset", 0444, root, rdev,
>   			    &radeon_debugfs_gpu_reset_fops);
>   	debugfs_create_file("radeon_fence_info", 0444, root, rdev,
> diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c
> index 458f92a70887..e6df1451af37 100644
> --- a/drivers/gpu/drm/radeon/radeon_gem.c
> +++ b/drivers/gpu/drm/radeon/radeon_gem.c
> @@ -890,6 +890,9 @@ void radeon_gem_debugfs_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>   	struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +	if (!root)
> +		return;
> +
>   	debugfs_create_file("radeon_gem_info", 0444, root, rdev,
>   			    &radeon_debugfs_gem_info_fops);
>
> diff --git a/drivers/gpu/drm/radeon/radeon_ib.c b/drivers/gpu/drm/radeon/radeon_ib.c
> index 62b116727b4f..28316eb4fd49 100644
> --- a/drivers/gpu/drm/radeon/radeon_ib.c
> +++ b/drivers/gpu/drm/radeon/radeon_ib.c
> @@ -311,6 +311,9 @@ static void radeon_debugfs_sa_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>   	struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +	if (!root)
> +		return;
> +
>   	debugfs_create_file("radeon_sa_info", 0444, root, rdev,
>   			    &radeon_debugfs_sa_info_fops);
>   #endif
> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
> index c67b6ddb29a4..c09e574d04f0 100644
> --- a/drivers/gpu/drm/radeon/radeon_pm.c
> +++ b/drivers/gpu/drm/radeon/radeon_pm.c
> @@ -1958,6 +1958,9 @@ static void radeon_debugfs_pm_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>   	struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +	if (!root)
> +		return;
> +
>   	debugfs_create_file("radeon_pm_info", 0444, root, rdev,
>   			    &radeon_debugfs_pm_info_fops);
>
> diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c
> index 7e207276df37..31a5b1ebf7c9 100644
> --- a/drivers/gpu/drm/radeon/radeon_ring.c
> +++ b/drivers/gpu/drm/radeon/radeon_ring.c
> @@ -550,6 +550,9 @@ static void radeon_debugfs_ring_init(struct radeon_device *rdev, struct radeon_r
>   	const char *ring_name = radeon_debugfs_ring_idx_to_name(ring->idx);
>   	struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +	if (!root)
> +		return;
> +
>   	if (ring_name)
>   		debugfs_create_file(ring_name, 0444, root, ring,
>   				    &radeon_debugfs_ring_info_fops);
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 11b21d605584..2e18ec93768d 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -942,6 +942,9 @@ static void radeon_ttm_debugfs_init(struct radeon_device *rdev)
>   	struct drm_minor *minor = rdev->ddev->primary;
>   	struct dentry *root = minor->debugfs_root;
>
> +	if (!root)
> +		return;
> +
>   	debugfs_create_file("radeon_vram", 0444, root, rdev,
>   			    &radeon_ttm_vram_fops);
>
> diff --git a/drivers/gpu/drm/radeon/rs400.c b/drivers/gpu/drm/radeon/rs400.c
> index 6383f7a34bd8..b41a903a29c3 100644
> --- a/drivers/gpu/drm/radeon/rs400.c
> +++ b/drivers/gpu/drm/radeon/rs400.c
> @@ -380,6 +380,9 @@ static void rs400_debugfs_pcie_gart_info_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>   	struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +	if (!root)
> +		return;
> +
>   	debugfs_create_file("rs400_gart_info", 0444, root, rdev,
>   			    &rs400_debugfs_gart_info_fops);
>   #endif
> diff --git a/drivers/gpu/drm/radeon/rv515.c b/drivers/gpu/drm/radeon/rv515.c
> index 63fb06e8e2d7..f39b6ab554f2 100644
> --- a/drivers/gpu/drm/radeon/rv515.c
> +++ b/drivers/gpu/drm/radeon/rv515.c
> @@ -257,6 +257,9 @@ void rv515_debugfs(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>   	struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +	if (!root)
> +		return;
> +
>   	debugfs_create_file("rv515_pipes_info", 0444, root, rdev,
>   			    &rv515_debugfs_pipes_info_fops);
>   	debugfs_create_file("rv515_ga_info", 0444, root, rdev,
> --
> 2.32.0
>

Re: [PATCH 3/5] drm/radeon: check dri root before debugfs init

[Das, Nirmoy]

[-- Attachment #1: Type: text/plain, Size: 10593 bytes --]

[AMD Official Use Only]

I sent all the patches to dr-devel. I think there is an issue with our email server. Thunderbird is asking for a password every few minutes.

https://patchwork.freedesktop.org/series/95603/

Nirmoy
[sending this from my browser]
________________________________
From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Friday, October 8, 2021 12:23 PM
To: Das, Nirmoy <Nirmoy.Das@amd.com>; dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org>
Cc: intel-gfx@lists.freedesktop.org <intel-gfx@lists.freedesktop.org>; Deucher, Alexander <Alexander.Deucher@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>
Subject: Re: [PATCH 3/5] drm/radeon: check dri root before debugfs init

Am 08.10.21 um 11:17 schrieb Nirmoy Das:
> Return early if dri minor root dentry is NULL.
>
> CC: Alex Deucher <alexander.deucher@amd.com>
> CC: "Christian König" <christian.koenig@amd.com>
> CC: "Pan, Xinhui" <Xinhui.Pan@amd.com>
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>

Acked-by: Christian König <christian.koenig@amd.com>

Where are the other patches from the series?

Thanks,
Christian.

> ---
>   drivers/gpu/drm/radeon/r100.c          | 9 +++++++++
>   drivers/gpu/drm/radeon/r300.c          | 3 +++
>   drivers/gpu/drm/radeon/r420.c          | 3 +++
>   drivers/gpu/drm/radeon/r600.c          | 3 +++
>   drivers/gpu/drm/radeon/radeon_dp_mst.c | 3 +++
>   drivers/gpu/drm/radeon/radeon_fence.c  | 3 +++
>   drivers/gpu/drm/radeon/radeon_gem.c    | 3 +++
>   drivers/gpu/drm/radeon/radeon_ib.c     | 3 +++
>   drivers/gpu/drm/radeon/radeon_pm.c     | 5 ++++-
>   drivers/gpu/drm/radeon/radeon_ring.c   | 3 +++
>   drivers/gpu/drm/radeon/radeon_ttm.c    | 3 +++
>   drivers/gpu/drm/radeon/rs400.c         | 3 +++
>   drivers/gpu/drm/radeon/rv515.c         | 3 +++
>   13 files changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
> index 2dd85ba1faa2..ae6c95b34013 100644
> --- a/drivers/gpu/drm/radeon/r100.c
> +++ b/drivers/gpu/drm/radeon/r100.c
> @@ -3059,6 +3059,9 @@ void  r100_debugfs_rbbm_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>        struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +     if (!root)
> +             return;
> +
>        debugfs_create_file("r100_rbbm_info", 0444, root, rdev,
>                            &r100_debugfs_rbbm_info_fops);
>   #endif
> @@ -3069,6 +3072,9 @@ void r100_debugfs_cp_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>        struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +     if (!root)
> +             return;
> +
>        debugfs_create_file("r100_cp_ring_info", 0444, root, rdev,
>                            &r100_debugfs_cp_ring_info_fops);
>        debugfs_create_file("r100_cp_csq_fifo", 0444, root, rdev,
> @@ -3081,6 +3087,9 @@ void  r100_debugfs_mc_info_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>        struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +     if (!root)
> +             return;
> +
>        debugfs_create_file("r100_mc_info", 0444, root, rdev,
>                            &r100_debugfs_mc_info_fops);
>   #endif
> diff --git a/drivers/gpu/drm/radeon/r300.c b/drivers/gpu/drm/radeon/r300.c
> index 621ff174dff3..b22969e2394f 100644
> --- a/drivers/gpu/drm/radeon/r300.c
> +++ b/drivers/gpu/drm/radeon/r300.c
> @@ -618,6 +618,9 @@ static void rv370_debugfs_pcie_gart_info_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>        struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +     if (!root)
> +             return;
> +
>        debugfs_create_file("rv370_pcie_gart_info", 0444, root, rdev,
>                            &rv370_debugfs_pcie_gart_info_fops);
>   #endif
> diff --git a/drivers/gpu/drm/radeon/r420.c b/drivers/gpu/drm/radeon/r420.c
> index 7e6320e8c6a0..cdb4ac3e346b 100644
> --- a/drivers/gpu/drm/radeon/r420.c
> +++ b/drivers/gpu/drm/radeon/r420.c
> @@ -494,6 +494,9 @@ void r420_debugfs_pipes_info_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>        struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +     if (!root)
> +             return;
> +
>        debugfs_create_file("r420_pipes_info", 0444, root, rdev,
>                            &r420_debugfs_pipes_info_fops);
>   #endif
> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
> index ca3fcae2adb5..d8f525cf0c3b 100644
> --- a/drivers/gpu/drm/radeon/r600.c
> +++ b/drivers/gpu/drm/radeon/r600.c
> @@ -4360,6 +4360,9 @@ static void r600_debugfs_mc_info_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>        struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +     if (!root)
> +             return;
> +
>        debugfs_create_file("r600_mc_info", 0444, root, rdev,
>                            &r600_debugfs_mc_info_fops);
>
> diff --git a/drivers/gpu/drm/radeon/radeon_dp_mst.c b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> index ec867fa880a4..cf06da89bb7c 100644
> --- a/drivers/gpu/drm/radeon/radeon_dp_mst.c
> +++ b/drivers/gpu/drm/radeon/radeon_dp_mst.c
> @@ -771,6 +771,9 @@ void radeon_mst_debugfs_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>        struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +     if (!root)
> +             return;
> +
>        debugfs_create_file("radeon_mst_info", 0444, root, rdev,
>                            &radeon_debugfs_mst_info_fops);
>
> diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c
> index 73e3117420bf..11f30349de46 100644
> --- a/drivers/gpu/drm/radeon/radeon_fence.c
> +++ b/drivers/gpu/drm/radeon/radeon_fence.c
> @@ -1006,6 +1006,9 @@ void radeon_debugfs_fence_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>        struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +     if (!root)
> +             return;
> +
>        debugfs_create_file("radeon_gpu_reset", 0444, root, rdev,
>                            &radeon_debugfs_gpu_reset_fops);
>        debugfs_create_file("radeon_fence_info", 0444, root, rdev,
> diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c
> index 458f92a70887..e6df1451af37 100644
> --- a/drivers/gpu/drm/radeon/radeon_gem.c
> +++ b/drivers/gpu/drm/radeon/radeon_gem.c
> @@ -890,6 +890,9 @@ void radeon_gem_debugfs_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>        struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +     if (!root)
> +             return;
> +
>        debugfs_create_file("radeon_gem_info", 0444, root, rdev,
>                            &radeon_debugfs_gem_info_fops);
>
> diff --git a/drivers/gpu/drm/radeon/radeon_ib.c b/drivers/gpu/drm/radeon/radeon_ib.c
> index 62b116727b4f..28316eb4fd49 100644
> --- a/drivers/gpu/drm/radeon/radeon_ib.c
> +++ b/drivers/gpu/drm/radeon/radeon_ib.c
> @@ -311,6 +311,9 @@ static void radeon_debugfs_sa_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>        struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +     if (!root)
> +             return;
> +
>        debugfs_create_file("radeon_sa_info", 0444, root, rdev,
>                            &radeon_debugfs_sa_info_fops);
>   #endif
> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c
> index c67b6ddb29a4..c09e574d04f0 100644
> --- a/drivers/gpu/drm/radeon/radeon_pm.c
> +++ b/drivers/gpu/drm/radeon/radeon_pm.c
> @@ -1958,6 +1958,9 @@ static void radeon_debugfs_pm_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>        struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +     if (!root)
> +             return;
> +
>        debugfs_create_file("radeon_pm_info", 0444, root, rdev,
>                            &radeon_debugfs_pm_info_fops);
>
> diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c
> index 7e207276df37..31a5b1ebf7c9 100644
> --- a/drivers/gpu/drm/radeon/radeon_ring.c
> +++ b/drivers/gpu/drm/radeon/radeon_ring.c
> @@ -550,6 +550,9 @@ static void radeon_debugfs_ring_init(struct radeon_device *rdev, struct radeon_r
>        const char *ring_name = radeon_debugfs_ring_idx_to_name(ring->idx);
>        struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +     if (!root)
> +             return;
> +
>        if (ring_name)
>                debugfs_create_file(ring_name, 0444, root, ring,
>                                    &radeon_debugfs_ring_info_fops);
> diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> index 11b21d605584..2e18ec93768d 100644
> --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> @@ -942,6 +942,9 @@ static void radeon_ttm_debugfs_init(struct radeon_device *rdev)
>        struct drm_minor *minor = rdev->ddev->primary;
>        struct dentry *root = minor->debugfs_root;
>
> +     if (!root)
> +             return;
> +
>        debugfs_create_file("radeon_vram", 0444, root, rdev,
>                            &radeon_ttm_vram_fops);
>
> diff --git a/drivers/gpu/drm/radeon/rs400.c b/drivers/gpu/drm/radeon/rs400.c
> index 6383f7a34bd8..b41a903a29c3 100644
> --- a/drivers/gpu/drm/radeon/rs400.c
> +++ b/drivers/gpu/drm/radeon/rs400.c
> @@ -380,6 +380,9 @@ static void rs400_debugfs_pcie_gart_info_init(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>        struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +     if (!root)
> +             return;
> +
>        debugfs_create_file("rs400_gart_info", 0444, root, rdev,
>                            &rs400_debugfs_gart_info_fops);
>   #endif
> diff --git a/drivers/gpu/drm/radeon/rv515.c b/drivers/gpu/drm/radeon/rv515.c
> index 63fb06e8e2d7..f39b6ab554f2 100644
> --- a/drivers/gpu/drm/radeon/rv515.c
> +++ b/drivers/gpu/drm/radeon/rv515.c
> @@ -257,6 +257,9 @@ void rv515_debugfs(struct radeon_device *rdev)
>   #if defined(CONFIG_DEBUG_FS)
>        struct dentry *root = rdev->ddev->primary->debugfs_root;
>
> +     if (!root)
> +             return;
> +
>        debugfs_create_file("rv515_pipes_info", 0444, root, rdev,
>                            &rv515_debugfs_pipes_info_fops);
>        debugfs_create_file("rv515_ga_info", 0444, root, rdev,
> --
> 2.32.0
>


[-- Attachment #2: Type: text/html, Size: 19750 bytes --]

[PATCH 4/5] drm/armada: check dri/crtc root before debugfs init

[Nirmoy Das]

Return early if dri minor root dentry is NULL.

CC: Russell King <linux@armlinux.org.uk>
CC: David Airlie <airlied@linux.ie>
CC: Daniel Vetter <daniel@ffwll.ch>

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/armada/armada_debugfs.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/armada/armada_debugfs.c b/drivers/gpu/drm/armada/armada_debugfs.c
index 29f4b52e3c8d..b40003fe4a89 100644
--- a/drivers/gpu/drm/armada/armada_debugfs.c
+++ b/drivers/gpu/drm/armada/armada_debugfs.c
@@ -93,6 +93,9 @@ static const struct file_operations armada_debugfs_crtc_reg_fops = {

 void armada_drm_crtc_debugfs_init(struct armada_crtc *dcrtc)
 {
+	if (!dcrtc->crtc.debugfs_entry)
+		return;
+
 	debugfs_create_file("armada-regs", 0600, dcrtc->crtc.debugfs_entry,
 			    dcrtc, &armada_debugfs_crtc_reg_fops);
 }
@@ -104,6 +107,9 @@ static struct drm_info_list armada_debugfs_list[] = {

 int armada_drm_debugfs_init(struct drm_minor *minor)
 {
+	if (!minor->debugfs_root)
+		return;
+
 	drm_debugfs_create_files(armada_debugfs_list, ARMADA_DEBUGFS_ENTRIES,
 				 minor->debugfs_root, minor);

--
2.32.0

Re: [PATCH 4/5] drm/armada: check dri/crtc root before debugfs init

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2433 bytes --]

Hi Nirmoy,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on tegra/for-next drm-tip/drm-tip linus/master v5.15-rc5 next-20211012]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nirmoy-Das/dri-cleanup-debugfs-error-handling/20211008-173312
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: arm-randconfig-r034-20211012 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c3dcf39554dbea780d6cb7e12239451ba47a2668)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/0c9a39f1265783134650d360585c60e0b1f64efe
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nirmoy-Das/dri-cleanup-debugfs-error-handling/20211008-173312
        git checkout 0c9a39f1265783134650d360585c60e0b1f64efe
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/armada/armada_debugfs.c:111:3: error: non-void function 'armada_drm_debugfs_init' should return a value [-Wreturn-type]
                   return;
                   ^
   1 error generated.

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for QCOM_SCM
   Depends on (ARM || ARM64) && HAVE_ARM_SMCCC
   Selected by
   - ARM_QCOM_SPM_CPUIDLE && CPU_IDLE && (ARM || ARM64) && (ARCH_QCOM || COMPILE_TEST && !ARM64 && MMU


vim +/armada_drm_debugfs_init +111 drivers/gpu/drm/armada/armada_debugfs.c

   107	
   108	int armada_drm_debugfs_init(struct drm_minor *minor)
   109	{
   110		if (!minor->debugfs_root)
 > 111			return;

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35834 bytes --]

[PATCH 5/5] drm/tegra: check root dentry before debugfs init

[Nirmoy Das]

Return early if crtc or connector's debugfs root dentries are NULL.

CC: Thierry Reding <thierry.reding@gmail.com>
CC: David Airlie <airlied@linux.ie>
CC: Daniel Vetter <daniel@ffwll.ch>
CC: Jonathan Hunter <jonathanh@nvidia.com>

Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/gpu/drm/tegra/dc.c   | 5 +++++
 drivers/gpu/drm/tegra/dsi.c  | 4 ++++
 drivers/gpu/drm/tegra/hdmi.c | 5 +++++
 drivers/gpu/drm/tegra/sor.c  | 4 ++++
 4 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 16c7aabb94d3..87eeda68d231 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1623,6 +1623,11 @@ static int tegra_dc_late_register(struct drm_crtc *crtc)
 	struct dentry *root;
 	struct tegra_dc *dc = to_tegra_dc(crtc);

+	if (!crtc->debugfs_entry) {
+		dc->debugfs_files = NULL;
+		return 0;
+	}
+
 #ifdef CONFIG_DEBUG_FS
 	root = crtc->debugfs_entry;
 #else
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index f46d377f0c30..f0263165e261 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -236,6 +236,10 @@ static int tegra_dsi_late_register(struct drm_connector *connector)
 	struct dentry *root = connector->debugfs_entry;
 	struct tegra_dsi *dsi = to_dsi(output);

+	if (!root) {
+		dsi->debugfs_files = NULL;
+		return 0;
+	}
 	dsi->debugfs_files = kmemdup(debugfs_files, sizeof(debugfs_files),
 				     GFP_KERNEL);
 	if (!dsi->debugfs_files)
diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
index e5d2a4026028..400319db0afc 100644
--- a/drivers/gpu/drm/tegra/hdmi.c
+++ b/drivers/gpu/drm/tegra/hdmi.c
@@ -1065,6 +1065,11 @@ static int tegra_hdmi_late_register(struct drm_connector *connector)
 	struct dentry *root = connector->debugfs_entry;
 	struct tegra_hdmi *hdmi = to_hdmi(output);

+	if (!root) {
+		hdmi->debugfs_files = NULL;
+		return 0;
+	}
+
 	hdmi->debugfs_files = kmemdup(debugfs_files, sizeof(debugfs_files),
 				      GFP_KERNEL);
 	if (!hdmi->debugfs_files)
diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c
index 0ea320c1092b..a8a3b0a587d9 100644
--- a/drivers/gpu/drm/tegra/sor.c
+++ b/drivers/gpu/drm/tegra/sor.c
@@ -1687,6 +1687,10 @@ static int tegra_sor_late_register(struct drm_connector *connector)
 	struct dentry *root = connector->debugfs_entry;
 	struct tegra_sor *sor = to_sor(output);

+	if (!root) {
+		sor->debugfs_files = NULL;
+		return 0;
+	}
 	sor->debugfs_files = kmemdup(debugfs_files, sizeof(debugfs_files),
 				     GFP_KERNEL);
 	if (!sor->debugfs_files)
--
2.32.0

Re: [PATCH 5/5] drm/tegra: check root dentry before debugfs init

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3242 bytes --]

Hi Nirmoy,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on tegra/for-next drm-tip/drm-tip linus/master v5.15 next-20211108]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nirmoy-Das/dri-cleanup-debugfs-error-handling/20211008-173312
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: arm64-randconfig-r012-20211008 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/313e208057ad9c5d5de6c0fdb03fb575f71270e3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nirmoy-Das/dri-cleanup-debugfs-error-handling/20211008-173312
        git checkout 313e208057ad9c5d5de6c0fdb03fb575f71270e3
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/gpu/drm/tegra/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/gpu/drm/tegra/dc.c: In function 'tegra_dc_late_register':
>> drivers/gpu/drm/tegra/dc.c:1626:18: error: 'struct drm_crtc' has no member named 'debugfs_entry'
    1626 |         if (!crtc->debugfs_entry) {
         |                  ^~
   drivers/gpu/drm/tegra/dc.c: In function 'tegra_crtc_update_memory_bandwidth':
   drivers/gpu/drm/tegra/dc.c:1853:53: warning: variable 'new_dc_state' set but not used [-Wunused-but-set-variable]
    1853 |         const struct tegra_dc_state *old_dc_state, *new_dc_state;
         |                                                     ^~~~~~~~~~~~
   drivers/gpu/drm/tegra/dc.c:1853:38: warning: variable 'old_dc_state' set but not used [-Wunused-but-set-variable]
    1853 |         const struct tegra_dc_state *old_dc_state, *new_dc_state;
         |                                      ^~~~~~~~~~~~
   drivers/gpu/drm/tegra/dc.c: In function 'tegra_crtc_calculate_memory_bandwidth':
   drivers/gpu/drm/tegra/dc.c:2233:38: warning: variable 'old_state' set but not used [-Wunused-but-set-variable]
    2233 |         const struct drm_crtc_state *old_state;
         |                                      ^~~~~~~~~


vim +1626 drivers/gpu/drm/tegra/dc.c

  1618	
  1619	static int tegra_dc_late_register(struct drm_crtc *crtc)
  1620	{
  1621		unsigned int i, count = ARRAY_SIZE(debugfs_files);
  1622		struct drm_minor *minor = crtc->dev->primary;
  1623		struct dentry *root;
  1624		struct tegra_dc *dc = to_tegra_dc(crtc);
  1625	
> 1626		if (!crtc->debugfs_entry) {
  1627			dc->debugfs_files = NULL;
  1628			return 0;
  1629		}
  1630	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 43752 bytes --]

Re: [PATCH 1/5] dri: cleanup debugfs error handling

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 6145 bytes --]

Hi

Am 08.10.21 um 11:17 schrieb Nirmoy Das:
> Debugfs API returns encoded error instead of NULL.
> This patch cleanups drm debugfs error handling to
> properly set dri and its minor's root dentry to NULL.
> 
> Also do not error out if dri/minor debugfs directory
> creation fails as a debugfs error is not a fatal error.
> 
> 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  | 25 +++++++++++++++++++++++--
>   drivers/gpu/drm/drm_drv.c      | 16 ++++++++++------
>   drivers/gpu/drm/drm_internal.h |  7 +++----
>   3 files changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index b0a826489488..af275a0c09b4 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -180,6 +180,9 @@ void drm_debugfs_create_files(const struct drm_info_list *files, int count,
>   	struct drm_info_node *tmp;
>   	int i;
> 
> +	if (!minor->debugfs_root)
> +		return;
> +
>   	for (i = 0; i < count; i++) {
>   		u32 features = files[i].driver_features;
> 
> @@ -203,7 +206,7 @@ void drm_debugfs_create_files(const struct drm_info_list *files, int count,
>   }
>   EXPORT_SYMBOL(drm_debugfs_create_files);
> 
> -int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> +void drm_debugfs_init(struct drm_minor *minor, int minor_id,
>   		     struct dentry *root)
>   {
>   	struct drm_device *dev = minor->dev;
> @@ -212,8 +215,16 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>   	INIT_LIST_HEAD(&minor->debugfs_list);
>   	mutex_init(&minor->debugfs_lock);
>   	sprintf(name, "%d", minor_id);
> +
> +	if (!root)
> +		goto error;
> +
>   	minor->debugfs_root = debugfs_create_dir(name, root);
> 
> +	if (IS_ERR(minor->debugfs_root))
> +		goto error;
> +
> +
>   	drm_debugfs_create_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES,
>   				 minor->debugfs_root, minor);
> 
> @@ -230,7 +241,11 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>   	if (dev->driver->debugfs_init)
>   		dev->driver->debugfs_init(minor);
> 
> -	return 0;
> +	return;
> +
> +error:
> +	minor->debugfs_root = NULL;
> +	return;
>   }
> 
> 
> @@ -241,6 +256,9 @@ int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
>   	struct drm_info_node *tmp;
>   	int i;
> 
> +	if (!minor->debugfs_root)
> +		return 0;
> +
>   	mutex_lock(&minor->debugfs_lock);
>   	for (i = 0; i < count; i++) {
>   		list_for_each_safe(pos, q, &minor->debugfs_list) {
> @@ -261,6 +279,9 @@ static void drm_debugfs_remove_all_files(struct drm_minor *minor)
>   {
>   	struct drm_info_node *node, *tmp;
> 
> +	if (!minor->debugfs_root)
> +		return;
> +
>   	mutex_lock(&minor->debugfs_lock);
>   	list_for_each_entry_safe(node, tmp, &minor->debugfs_list, list) {
>   		debugfs_remove(node->dent);
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 7a5097467ba5..fa57ec2d49bf 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -160,11 +160,7 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
>   	if (!minor)
>   		return 0;
> 
> -	ret = drm_debugfs_init(minor, minor->index, drm_debugfs_root);
> -	if (ret) {
> -		DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");

Rather than deleting the error message, return an error code from 
drm_debugfs_init() and print it here. I'd change DRM_ERROR() to 
drm_dbg_core(NULL, ...).


> -		goto err_debugfs;
> -	}
> +	drm_debugfs_init(minor, minor->index, drm_debugfs_root);
> 
>   	ret = device_add(minor->kdev);
>   	if (ret)
> @@ -1050,7 +1046,15 @@ static int __init drm_core_init(void)
>   		goto error;
>   	}
> 
> -	drm_debugfs_root = debugfs_create_dir("dri", NULL);
> +	if (!debugfs_initialized()) {
> +		drm_debugfs_root = NULL;
> +	} else {
> +		drm_debugfs_root = debugfs_create_dir("dri", NULL);
> +		if (IS_ERR(drm_debugfs_root)) {
> +			DRM_WARN("DRM: Failed to initialize /sys/kernel/debug/dri.\n");

This should also print the error code. I'd also change the call to 
drm_dbg_core(). The message should say 'failed to create', so it's 
differnt from the other one.

Best regards
Thomas

> +			drm_debugfs_root = NULL;
> +		}
> +	}
> 
>   	ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops);
>   	if (ret < 0)
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 17f3548c8ed2..e27a40166178 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -182,8 +182,8 @@ int drm_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev,
> 
>   /* drm_debugfs.c drm_debugfs_crc.c */
>   #if defined(CONFIG_DEBUG_FS)
> -int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> -		     struct dentry *root);
> +void drm_debugfs_init(struct drm_minor *minor, int minor_id,
> +		      struct dentry *root);
>   void drm_debugfs_cleanup(struct drm_minor *minor);
>   void drm_debugfs_connector_add(struct drm_connector *connector);
>   void drm_debugfs_connector_remove(struct drm_connector *connector);
> @@ -191,10 +191,9 @@ void drm_debugfs_crtc_add(struct drm_crtc *crtc);
>   void drm_debugfs_crtc_remove(struct drm_crtc *crtc);
>   void drm_debugfs_crtc_crc_add(struct drm_crtc *crtc);
>   #else
> -static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> +static inline void drm_debugfs_init(struct drm_minor *minor, int minor_id,
>   				   struct dentry *root)
>   {
> -	return 0;
>   }
> 
>   static inline void drm_debugfs_cleanup(struct drm_minor *minor)
> --
> 2.32.0
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Intel-gfx] [PATCH 1/5] dri: cleanup debugfs error handling

[Jani Nikula]

On Fri, 08 Oct 2021, Nirmoy Das <nirmoy.das@amd.com> wrote:
> Debugfs API returns encoded error instead of NULL.
> This patch cleanups drm debugfs error handling to
> properly set dri and its minor's root dentry to NULL.
>
> Also do not error out if dri/minor debugfs directory
> creation fails as a debugfs error is not a fatal error.

Cc: Greg

I thought this is the opposite of what Greg's been telling everyone to
do with debugfs.

BR,
Jani.

>
> 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  | 25 +++++++++++++++++++++++--
>  drivers/gpu/drm/drm_drv.c      | 16 ++++++++++------
>  drivers/gpu/drm/drm_internal.h |  7 +++----
>  3 files changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index b0a826489488..af275a0c09b4 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -180,6 +180,9 @@ void drm_debugfs_create_files(const struct drm_info_list *files, int count,
>  	struct drm_info_node *tmp;
>  	int i;
>
> +	if (!minor->debugfs_root)
> +		return;
> +
>  	for (i = 0; i < count; i++) {
>  		u32 features = files[i].driver_features;
>
> @@ -203,7 +206,7 @@ void drm_debugfs_create_files(const struct drm_info_list *files, int count,
>  }
>  EXPORT_SYMBOL(drm_debugfs_create_files);
>
> -int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> +void drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  		     struct dentry *root)
>  {
>  	struct drm_device *dev = minor->dev;
> @@ -212,8 +215,16 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  	INIT_LIST_HEAD(&minor->debugfs_list);
>  	mutex_init(&minor->debugfs_lock);
>  	sprintf(name, "%d", minor_id);
> +
> +	if (!root)
> +		goto error;
> +
>  	minor->debugfs_root = debugfs_create_dir(name, root);
>
> +	if (IS_ERR(minor->debugfs_root))
> +		goto error;
> +
> +
>  	drm_debugfs_create_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES,
>  				 minor->debugfs_root, minor);
>
> @@ -230,7 +241,11 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  	if (dev->driver->debugfs_init)
>  		dev->driver->debugfs_init(minor);
>
> -	return 0;
> +	return;
> +
> +error:
> +	minor->debugfs_root = NULL;
> +	return;
>  }
>
>
> @@ -241,6 +256,9 @@ int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
>  	struct drm_info_node *tmp;
>  	int i;
>
> +	if (!minor->debugfs_root)
> +		return 0;
> +
>  	mutex_lock(&minor->debugfs_lock);
>  	for (i = 0; i < count; i++) {
>  		list_for_each_safe(pos, q, &minor->debugfs_list) {
> @@ -261,6 +279,9 @@ static void drm_debugfs_remove_all_files(struct drm_minor *minor)
>  {
>  	struct drm_info_node *node, *tmp;
>
> +	if (!minor->debugfs_root)
> +		return;
> +
>  	mutex_lock(&minor->debugfs_lock);
>  	list_for_each_entry_safe(node, tmp, &minor->debugfs_list, list) {
>  		debugfs_remove(node->dent);
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 7a5097467ba5..fa57ec2d49bf 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -160,11 +160,7 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
>  	if (!minor)
>  		return 0;
>
> -	ret = drm_debugfs_init(minor, minor->index, drm_debugfs_root);
> -	if (ret) {
> -		DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
> -		goto err_debugfs;
> -	}
> +	drm_debugfs_init(minor, minor->index, drm_debugfs_root);
>
>  	ret = device_add(minor->kdev);
>  	if (ret)
> @@ -1050,7 +1046,15 @@ static int __init drm_core_init(void)
>  		goto error;
>  	}
>
> -	drm_debugfs_root = debugfs_create_dir("dri", NULL);
> +	if (!debugfs_initialized()) {
> +		drm_debugfs_root = NULL;
> +	} else {
> +		drm_debugfs_root = debugfs_create_dir("dri", NULL);
> +		if (IS_ERR(drm_debugfs_root)) {
> +			DRM_WARN("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
> +			drm_debugfs_root = NULL;
> +		}
> +	}
>
>  	ret = register_chrdev(DRM_MAJOR, "drm", &drm_stub_fops);
>  	if (ret < 0)
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 17f3548c8ed2..e27a40166178 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -182,8 +182,8 @@ int drm_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev,
>
>  /* drm_debugfs.c drm_debugfs_crc.c */
>  #if defined(CONFIG_DEBUG_FS)
> -int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> -		     struct dentry *root);
> +void drm_debugfs_init(struct drm_minor *minor, int minor_id,
> +		      struct dentry *root);
>  void drm_debugfs_cleanup(struct drm_minor *minor);
>  void drm_debugfs_connector_add(struct drm_connector *connector);
>  void drm_debugfs_connector_remove(struct drm_connector *connector);
> @@ -191,10 +191,9 @@ void drm_debugfs_crtc_add(struct drm_crtc *crtc);
>  void drm_debugfs_crtc_remove(struct drm_crtc *crtc);
>  void drm_debugfs_crtc_crc_add(struct drm_crtc *crtc);
>  #else
> -static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> +static inline void drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  				   struct dentry *root)
>  {
> -	return 0;
>  }
>
>  static inline void drm_debugfs_cleanup(struct drm_minor *minor)
> --
> 2.32.0
>

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH 1/5] dri: cleanup debugfs error handling

[Greg KH]

On Fri, Oct 08, 2021 at 12:40:47PM +0300, Jani Nikula wrote:
> On Fri, 08 Oct 2021, Nirmoy Das <nirmoy.das@amd.com> wrote:
> > Debugfs API returns encoded error instead of NULL.
> > This patch cleanups drm debugfs error handling to
> > properly set dri and its minor's root dentry to NULL.
> >
> > Also do not error out if dri/minor debugfs directory
> > creation fails as a debugfs error is not a fatal error.
> 
> Cc: Greg
> 
> I thought this is the opposite of what Greg's been telling everyone to
> do with debugfs.

Yes, that is not good.

You should never care about the result of a debugfs_create* call.  Just
take the result, and if it is a directory, save it off to use it for
creating a file, no need to check anything.

And then throw it away, later, when you want to remove the directory,
look it up with a call to debugfs_lookup() and pass that to
debugfs_remove() (which does so recursively).

There should never be a need to save, or check, the result of any
debugfs call.  If so, odds are it is being used incorrectly.

thanks,

greg k-h

Re: [Intel-gfx] [PATCH 1/5] dri: cleanup debugfs error handling

[Das, Nirmoy]

[-- Attachment #1: Type: text/plain, Size: 1882 bytes --]

[AMD Official Use Only]

Thanks, Greg and Jani. So I have to do the exact opposite.

We do have some NULL dentry check in the drm code. I will remove those instead.

Regards,
Nirmoy
________________________________
From: Greg KH <gregkh@linuxfoundation.org>
Sent: Friday, October 8, 2021 1:07 PM
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Das, Nirmoy <Nirmoy.Das@amd.com>; dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org>; intel-gfx@lists.freedesktop.org <intel-gfx@lists.freedesktop.org>; Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; David Airlie <airlied@linux.ie>; Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [Intel-gfx] [PATCH 1/5] dri: cleanup debugfs error handling

On Fri, Oct 08, 2021 at 12:40:47PM +0300, Jani Nikula wrote:
> On Fri, 08 Oct 2021, Nirmoy Das <nirmoy.das@amd.com> wrote:
> > Debugfs API returns encoded error instead of NULL.
> > This patch cleanups drm debugfs error handling to
> > properly set dri and its minor's root dentry to NULL.
> >
> > Also do not error out if dri/minor debugfs directory
> > creation fails as a debugfs error is not a fatal error.
>
> Cc: Greg
>
> I thought this is the opposite of what Greg's been telling everyone to
> do with debugfs.

Yes, that is not good.

You should never care about the result of a debugfs_create* call.  Just
take the result, and if it is a directory, save it off to use it for
creating a file, no need to check anything.

And then throw it away, later, when you want to remove the directory,
look it up with a call to debugfs_lookup() and pass that to
debugfs_remove() (which does so recursively).

There should never be a need to save, or check, the result of any
debugfs call.  If so, odds are it is being used incorrectly.

thanks,

greg k-h

[-- Attachment #2: Type: text/html, Size: 3756 bytes --]

Fw: [Intel-gfx] [PATCH 1/5] dri: cleanup debugfs error handling

[Das, Nirmoy]

[-- Attachment #1: Type: text/plain, Size: 1974 bytes --]

[AMD Official Use Only]

Trying with Christian's Gmail, as he still didn't receive previous emails.

________________________________
From: Das, Nirmoy <nirmoy.das@amd.com>
Sent: Friday, October 8, 2021 2:17 PM
To: Koenig, Christian <Christian.Koenig@amd.com>
Cc: dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org>; intel-gfx@lists.freedesktop.org <intel-gfx@lists.freedesktop.org>; Maarten Lankhorst <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>; Thomas Zimmermann <tzimmermann@suse.de>; David Airlie <airlied@linux.ie>; Daniel Vetter <daniel@ffwll.ch>; Greg KH <gregkh@linuxfoundation.org>; Jani Nikula <jani.nikula@linux.intel.com>
Subject: Re: [Intel-gfx] [PATCH 1/5] dri: cleanup debugfs error handling

Adding Christian as he didn't receive the full thread yet.


On 10/8/2021 1:07 PM, Greg KH wrote:
> On Fri, Oct 08, 2021 at 12:40:47PM +0300, Jani Nikula wrote:
>> On Fri, 08 Oct 2021, Nirmoy Das <nirmoy.das@amd.com> wrote:
>>> Debugfs API returns encoded error instead of NULL.
>>> This patch cleanups drm debugfs error handling to
>>> properly set dri and its minor's root dentry to NULL.
>>>
>>> Also do not error out if dri/minor debugfs directory
>>> creation fails as a debugfs error is not a fatal error.
>> Cc: Greg
>>
>> I thought this is the opposite of what Greg's been telling everyone to
>> do with debugfs.
> Yes, that is not good.
>
> You should never care about the result of a debugfs_create* call.  Just
> take the result, and if it is a directory, save it off to use it for
> creating a file, no need to check anything.
>
> And then throw it away, later, when you want to remove the directory,
> look it up with a call to debugfs_lookup() and pass that to
> debugfs_remove() (which does so recursively).
>
> There should never be a need to save, or check, the result of any
> debugfs call.  If so, odds are it is being used incorrectly.
>
> thanks,
>
> greg k-h

[-- Attachment #2: Type: text/html, Size: 3314 bytes --]

Re: Fw: [Intel-gfx] [PATCH 1/5] dri: cleanup debugfs error handling

[Christian König]

Am 08.10.21 um 17:11 schrieb Greg KH:
> On Fri, Oct 08, 2021 at 04:22:06PM +0200, Christian König wrote:
>> Hi guys,
>>
>> thanks Nirmoy for forwarding this, there is seriously something wrong with
>> the AMD mail servers.
>>
>>> On 10/8/2021 1:07 PM, Greg KH wrote:
>>>> On Fri, Oct 08, 2021 at 12:40:47PM +0300, Jani Nikula wrote:
>>>>> On Fri, 08 Oct 2021, Nirmoy Das <nirmoy.das@amd.com> wrote:
>>>>>> Debugfs API returns encoded error instead of NULL.
>>>>>> This patch cleanups drm debugfs error handling to
>>>>>> properly set dri and its minor's root dentry to NULL.
>>>>>>
>>>>>> Also do not error out if dri/minor debugfs directory
>>>>>> creation fails as a debugfs error is not a fatal error.
>>>>> Cc: Greg
>>>>>
>>>>> I thought this is the opposite of what Greg's been telling everyone to
>>>>> do with debugfs.
>>>> Yes, that is not good.
>>>>
>>>> You should never care about the result of a debugfs_create* call.  Just
>>>> take the result, and if it is a directory, save it off to use it for
>>>> creating a file, no need to check anything.
>> While I totally agree to the intention behind that I'm pretty sure there are
>> some consequences which are a rather bad idea.
>>
>> First of all the debugfs functions return a normal struct dentry pointer and
>> keeping that around unchecked means that we now have pointers in structure
>> members which can suddenly be an ERR_PTR() without any documentation that
>> they are not real pointers.
>>
>> What we could do instead is something like returning a typedef or a
>> structure with the dentry pointer embedded and then document that those are
>> special, can be ERR_PTR() and should never be touched except for the debugfs
>> functions itself.
> I agree, and am slowly working toward that, but am not there yet.  If
> you look, I have removed the return values for almost all
> debugfs_create_* calls, only a few remain.
>
> For now, just treat it like a "void" pointer that you have no context
> for and all will be fine.

Ok in that case we should just document on the saved dentry that this 
pointer is not necessary valid.

Nirmoy, can you take care of that?

>> The other issue is that adding the same file twice is unfortunately a rather
>> common coding error which we don't catch or complain about any more if we
>> don't look at the return value at all.
> How can you add the same debugfs file twice?  You have different
> directories.

We had multiple occasions triggering that:

1. Code accidentally initializing a component twice.

Except for the debugfs entry and a bit memory leak we had no negative 
effect otherwise and wouldn't had detected that without the error 
message from debugfs.

2. Driver not cleaning up properly on hotplug. E.g. old subdirectory not 
properly removed and re-plug.

3. Driver trying to use the same debugfs file for different devices.

> And really, who cares, it's debugging code!  :)

Well especially cause 3 once took me a day to figure out that I'm 
looking at the wrong hardware state because the driver was handling two 
devices, but only one showed up under debugfs.

>>>> And then throw it away, later, when you want to remove the directory,
>>>> look it up with a call to debugfs_lookup() and pass that to
>>>> debugfs_remove() (which does so recursively).
>>>>
>>>> There should never be a need to save, or check, the result of any
>>>> debugfs call.  If so, odds are it is being used incorrectly.
>> Yeah, exactly that's the problem I see here.
>>
>> We save the return value because the DRM subsystem is creating a debugfs
>> directory for the drivers to use.
> That's fine for now, not a big deal.  And even if there is an error,
> again, you can always feed that error back into the debugfs subsystem on
> another call and it will handle it correctly.

Problem is it isn't, we have a crash because the member isn't a pointer 
but an ERR_PTR instead.

And adding IS_ERR checks all around is even worse than adding NULL checks.

Regards,
Christian.

>
> thanks,
>
> greg k-h

Re: Fw: [Intel-gfx] [PATCH 1/5] dri: cleanup debugfs error handling

[Greg KH]

On Mon, Oct 11, 2021 at 04:19:58PM +0200, Christian König wrote:
> > > > > And then throw it away, later, when you want to remove the directory,
> > > > > look it up with a call to debugfs_lookup() and pass that to
> > > > > debugfs_remove() (which does so recursively).
> > > > > 
> > > > > There should never be a need to save, or check, the result of any
> > > > > debugfs call.  If so, odds are it is being used incorrectly.
> > > Yeah, exactly that's the problem I see here.
> > > 
> > > We save the return value because the DRM subsystem is creating a debugfs
> > > directory for the drivers to use.
> > That's fine for now, not a big deal.  And even if there is an error,
> > again, you can always feed that error back into the debugfs subsystem on
> > another call and it will handle it correctly.
> 
> Problem is it isn't, we have a crash because the member isn't a pointer but
> an ERR_PTR instead.

Again, that is fine, you can feed that into debugfs and it will "just
work".  Treat it as an opaque pointer, not a *dentry and you will be
fine.

thanks,

greg k-h

Re: Fw: [Intel-gfx] [PATCH 1/5] dri: cleanup debugfs error handling

[Jani Nikula]

On Mon, 11 Oct 2021, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Oct 11, 2021 at 04:19:58PM +0200, Christian König wrote:
>> > > > > And then throw it away, later, when you want to remove the directory,
>> > > > > look it up with a call to debugfs_lookup() and pass that to
>> > > > > debugfs_remove() (which does so recursively).
>> > > > > 
>> > > > > There should never be a need to save, or check, the result of any
>> > > > > debugfs call.  If so, odds are it is being used incorrectly.
>> > > Yeah, exactly that's the problem I see here.
>> > > 
>> > > We save the return value because the DRM subsystem is creating a debugfs
>> > > directory for the drivers to use.
>> > That's fine for now, not a big deal.  And even if there is an error,
>> > again, you can always feed that error back into the debugfs subsystem on
>> > another call and it will handle it correctly.
>> 
>> Problem is it isn't, we have a crash because the member isn't a pointer but
>> an ERR_PTR instead.
>
> Again, that is fine, you can feed that into debugfs and it will "just
> work".  Treat it as an opaque pointer, not a *dentry and you will be
> fine.

Hmm, some of the patches add things like:

+
+	if (!root)
+		goto error;
+
	minor->debugfs_root = debugfs_create_dir(name, root);

Superficially this seems okay, as it looks like debugfs_create_dir()
doesn't actually cope with NULL values. However, since ->debugfs_root
comes from debugfs_create_dir() I presume it'll never be NULL on errors
anyway but rather an error pointer!

So I think we probably need to go through the drm subsystem and look for
existing similar patterns in fix them.

BR,
Jani.



>
> thanks,
>
> greg k-h

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: Fw: [Intel-gfx] [PATCH 1/5] dri: cleanup debugfs error handling

[Greg KH]

On Mon, Oct 11, 2021 at 07:38:22PM +0300, Jani Nikula wrote:
> On Mon, 11 Oct 2021, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Mon, Oct 11, 2021 at 04:19:58PM +0200, Christian König wrote:
> >> > > > > And then throw it away, later, when you want to remove the directory,
> >> > > > > look it up with a call to debugfs_lookup() and pass that to
> >> > > > > debugfs_remove() (which does so recursively).
> >> > > > > 
> >> > > > > There should never be a need to save, or check, the result of any
> >> > > > > debugfs call.  If so, odds are it is being used incorrectly.
> >> > > Yeah, exactly that's the problem I see here.
> >> > > 
> >> > > We save the return value because the DRM subsystem is creating a debugfs
> >> > > directory for the drivers to use.
> >> > That's fine for now, not a big deal.  And even if there is an error,
> >> > again, you can always feed that error back into the debugfs subsystem on
> >> > another call and it will handle it correctly.
> >> 
> >> Problem is it isn't, we have a crash because the member isn't a pointer but
> >> an ERR_PTR instead.
> >
> > Again, that is fine, you can feed that into debugfs and it will "just
> > work".  Treat it as an opaque pointer, not a *dentry and you will be
> > fine.
> 
> Hmm, some of the patches add things like:
> 
> +
> +	if (!root)
> +		goto error;
> +
> 	minor->debugfs_root = debugfs_create_dir(name, root);
> 
> Superficially this seems okay, as it looks like debugfs_create_dir()
> doesn't actually cope with NULL values.

Yes it does, it puts things at the root of debugfs.

But why are you checking for NULL here, as the return value of a debugfs
call can never be NULL?

> However, since ->debugfs_root
> comes from debugfs_create_dir() I presume it'll never be NULL on errors
> anyway but rather an error pointer!

That is correct.

> So I think we probably need to go through the drm subsystem and look for
> existing similar patterns in fix them.

Please do.  I know I made one pass at it a while ago but I think someone
else went through and cleaned them up again.

thanks,

greg k-h

Re: Fw: [Intel-gfx] [PATCH 1/5] dri: cleanup debugfs error handling

[Jani Nikula]

On Mon, 11 Oct 2021, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Oct 11, 2021 at 07:38:22PM +0300, Jani Nikula wrote:
>> On Mon, 11 Oct 2021, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Mon, Oct 11, 2021 at 04:19:58PM +0200, Christian König wrote:
>> >> > > > > And then throw it away, later, when you want to remove the directory,
>> >> > > > > look it up with a call to debugfs_lookup() and pass that to
>> >> > > > > debugfs_remove() (which does so recursively).
>> >> > > > > 
>> >> > > > > There should never be a need to save, or check, the result of any
>> >> > > > > debugfs call.  If so, odds are it is being used incorrectly.
>> >> > > Yeah, exactly that's the problem I see here.
>> >> > > 
>> >> > > We save the return value because the DRM subsystem is creating a debugfs
>> >> > > directory for the drivers to use.
>> >> > That's fine for now, not a big deal.  And even if there is an error,
>> >> > again, you can always feed that error back into the debugfs subsystem on
>> >> > another call and it will handle it correctly.
>> >> 
>> >> Problem is it isn't, we have a crash because the member isn't a pointer but
>> >> an ERR_PTR instead.
>> >
>> > Again, that is fine, you can feed that into debugfs and it will "just
>> > work".  Treat it as an opaque pointer, not a *dentry and you will be
>> > fine.
>> 
>> Hmm, some of the patches add things like:
>> 
>> +
>> +	if (!root)
>> +		goto error;
>> +
>> 	minor->debugfs_root = debugfs_create_dir(name, root);
>> 
>> Superficially this seems okay, as it looks like debugfs_create_dir()
>> doesn't actually cope with NULL values.
>
> Yes it does, it puts things at the root of debugfs.

Oh, thanks for the correction.

> But why are you checking for NULL here, as the return value of a debugfs
> call can never be NULL?

Just musing on what is going on in the patch, and why such changes
initially seem like good ideas and get through review.


Thanks,
Jani.


>
>> However, since ->debugfs_root
>> comes from debugfs_create_dir() I presume it'll never be NULL on errors
>> anyway but rather an error pointer!
>
> That is correct.
>
>> So I think we probably need to go through the drm subsystem and look for
>> existing similar patterns in fix them.
>
> Please do.  I know I made one pass at it a while ago but I think someone
> else went through and cleaned them up again.
>
> thanks,
>
> greg k-h

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: Fw: [Intel-gfx] [PATCH 1/5] dri: cleanup debugfs error handling

[Das, Nirmoy]

On 10/11/2021 4:19 PM, Christian König wrote:
> Am 08.10.21 um 17:11 schrieb Greg KH:
>> On Fri, Oct 08, 2021 at 04:22:06PM +0200, Christian König wrote:
>>> Hi guys,
>>>
>>> thanks Nirmoy for forwarding this, there is seriously something 
>>> wrong with
>>> the AMD mail servers.
>>>
>>>> On 10/8/2021 1:07 PM, Greg KH wrote:
>>>>> On Fri, Oct 08, 2021 at 12:40:47PM +0300, Jani Nikula wrote:
>>>>>> On Fri, 08 Oct 2021, Nirmoy Das <nirmoy.das@amd.com> wrote:
>>>>>>> Debugfs API returns encoded error instead of NULL.
>>>>>>> This patch cleanups drm debugfs error handling to
>>>>>>> properly set dri and its minor's root dentry to NULL.
>>>>>>>
>>>>>>> Also do not error out if dri/minor debugfs directory
>>>>>>> creation fails as a debugfs error is not a fatal error.
>>>>>> Cc: Greg
>>>>>>
>>>>>> I thought this is the opposite of what Greg's been telling 
>>>>>> everyone to
>>>>>> do with debugfs.
>>>>> Yes, that is not good.
>>>>>
>>>>> You should never care about the result of a debugfs_create* call.  
>>>>> Just
>>>>> take the result, and if it is a directory, save it off to use it for
>>>>> creating a file, no need to check anything.
>>> While I totally agree to the intention behind that I'm pretty sure 
>>> there are
>>> some consequences which are a rather bad idea.
>>>
>>> First of all the debugfs functions return a normal struct dentry 
>>> pointer and
>>> keeping that around unchecked means that we now have pointers in 
>>> structure
>>> members which can suddenly be an ERR_PTR() without any documentation 
>>> that
>>> they are not real pointers.
>>>
>>> What we could do instead is something like returning a typedef or a
>>> structure with the dentry pointer embedded and then document that 
>>> those are
>>> special, can be ERR_PTR() and should never be touched except for the 
>>> debugfs
>>> functions itself.
>> I agree, and am slowly working toward that, but am not there yet.  If
>> you look, I have removed the return values for almost all
>> debugfs_create_* calls, only a few remain.
>>
>> For now, just treat it like a "void" pointer that you have no context
>> for and all will be fine.
>
> Ok in that case we should just document on the saved dentry that this 
> pointer is not necessary valid.
>
> Nirmoy, can you take care of that?


Sure, I will send patches to document and clean our drm core debugfs code.


Thanks,

Nirmoy


>
>>> The other issue is that adding the same file twice is unfortunately 
>>> a rather
>>> common coding error which we don't catch or complain about any more 
>>> if we
>>> don't look at the return value at all.
>> How can you add the same debugfs file twice?  You have different
>> directories.
>
> We had multiple occasions triggering that:
>
> 1. Code accidentally initializing a component twice.
>
> Except for the debugfs entry and a bit memory leak we had no negative 
> effect otherwise and wouldn't had detected that without the error 
> message from debugfs.
>
> 2. Driver not cleaning up properly on hotplug. E.g. old subdirectory 
> not properly removed and re-plug.
>
> 3. Driver trying to use the same debugfs file for different devices.
>
>> And really, who cares, it's debugging code!  :)
>
> Well especially cause 3 once took me a day to figure out that I'm 
> looking at the wrong hardware state because the driver was handling 
> two devices, but only one showed up under debugfs.
>
>>>>> And then throw it away, later, when you want to remove the directory,
>>>>> look it up with a call to debugfs_lookup() and pass that to
>>>>> debugfs_remove() (which does so recursively).
>>>>>
>>>>> There should never be a need to save, or check, the result of any
>>>>> debugfs call.  If so, odds are it is being used incorrectly.
>>> Yeah, exactly that's the problem I see here.
>>>
>>> We save the return value because the DRM subsystem is creating a 
>>> debugfs
>>> directory for the drivers to use.
>> That's fine for now, not a big deal.  And even if there is an error,
>> again, you can always feed that error back into the debugfs subsystem on
>> another call and it will handle it correctly.
>
> Problem is it isn't, we have a crash because the member isn't a 
> pointer but an ERR_PTR instead.
>
> And adding IS_ERR checks all around is even worse than adding NULL 
> checks.
>
> Regards,
> Christian.
>
>>
>> thanks,
>>
>> greg k-h
>