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

Thomas Zimmermann <tzimmermann@xxxxxxx> · Fri, 8 Oct 2021 11:35:33 +0200

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@xxxxxxxxxxxxxxx>
CC: Maxime Ripard <mripard@xxxxxxxxxx>
CC: Thomas Zimmermann <tzimmermann@xxxxxxx>
CC: David Airlie <airlied@xxxxxxxx>
CC: Daniel Vetter <daniel@xxxxxxxx>
Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxx>
---
  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:
OpenPGP_signature

Description: OpenPGP digital signature




