Re: [PATCH] drm: no need to check return value of debugfs_create functions

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

 



On Thu, Jun 13, 2019 at 03:34:39PM +0200, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.
> 
> Because there is no need to check these functions, a number of local
> functions can be made to return void to simplify things as nothing can
> fail.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Maxime Ripard <maxime.ripard@xxxxxxxxxxx>
> Cc: Sean Paul <sean@xxxxxxxxxx>
> Cc: David Airlie <airlied@xxxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>

Ah there was more, kinda wondered. Applied to drm-misc-next, thanks.

btw wrt changing function signatures, there's a few more as a quick

$ git grep debugfs -- include/drm

shows. Do you plan to do the s/int/void/ on all these functions/hooks too
once the driver patches have landed? Otherwise could put that as a nice
todo item into Documentation/gpu/todo.rst for outreachy/gsoc and other
bored people.
-Daniel

> ---
>  drivers/gpu/drm/drm_connector.c   |  6 +---
>  drivers/gpu/drm/drm_crtc.c        |  4 +--
>  drivers/gpu/drm/drm_debugfs.c     | 53 +++++++------------------------
>  drivers/gpu/drm/drm_debugfs_crc.c | 28 ++++------------
>  drivers/gpu/drm/drm_drv.c         |  5 ---
>  drivers/gpu/drm/drm_internal.h    | 20 +++++-------
>  6 files changed, 29 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index b34c3d38bf15..5e9e0c2a9e5c 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -464,10 +464,7 @@ int drm_connector_register(struct drm_connector *connector)
>  	if (ret)
>  		goto unlock;
>  
> -	ret = drm_debugfs_connector_add(connector);
> -	if (ret) {
> -		goto err_sysfs;
> -	}
> +	drm_debugfs_connector_add(connector);
>  
>  	if (connector->funcs->late_register) {
>  		ret = connector->funcs->late_register(connector);
> @@ -482,7 +479,6 @@ int drm_connector_register(struct drm_connector *connector)
>  
>  err_debugfs:
>  	drm_debugfs_connector_remove(connector);
> -err_sysfs:
>  	drm_sysfs_connector_remove(connector);
>  unlock:
>  	mutex_unlock(&connector->mutex);
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 790ba5941954..4936e1080e41 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -122,9 +122,7 @@ int drm_crtc_register_all(struct drm_device *dev)
>  	int ret = 0;
>  
>  	drm_for_each_crtc(crtc, dev) {
> -		if (drm_debugfs_crtc_add(crtc))
> -			DRM_ERROR("Failed to initialize debugfs entry for CRTC '%s'.\n",
> -				  crtc->name);
> +		drm_debugfs_crtc_add(crtc);
>  
>  		if (crtc->funcs->late_register)
>  			ret = crtc->funcs->late_register(crtc);
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index f8468eae0503..6f2802e9bfb5 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -226,10 +226,6 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  	mutex_init(&minor->debugfs_lock);
>  	sprintf(name, "%d", minor_id);
>  	minor->debugfs_root = debugfs_create_dir(name, root);
> -	if (!minor->debugfs_root) {
> -		DRM_ERROR("Cannot create /sys/kernel/debug/dri/%s\n", name);
> -		return -1;
> -	}
>  
>  	ret = drm_debugfs_create_files(drm_debugfs_list, DRM_DEBUGFS_ENTRIES,
>  				       minor->debugfs_root, minor);
> @@ -310,17 +306,15 @@ static void drm_debugfs_remove_all_files(struct drm_minor *minor)
>  	mutex_unlock(&minor->debugfs_lock);
>  }
>  
> -int drm_debugfs_cleanup(struct drm_minor *minor)
> +void drm_debugfs_cleanup(struct drm_minor *minor)
>  {
>  	if (!minor->debugfs_root)
> -		return 0;
> +		return;
>  
>  	drm_debugfs_remove_all_files(minor);
>  
>  	debugfs_remove_recursive(minor->debugfs_root);
>  	minor->debugfs_root = NULL;
> -
> -	return 0;
>  }
>  
>  static int connector_show(struct seq_file *m, void *data)
> @@ -438,38 +432,24 @@ static const struct file_operations drm_connector_fops = {
>  	.write = connector_write
>  };
>  
> -int drm_debugfs_connector_add(struct drm_connector *connector)
> +void drm_debugfs_connector_add(struct drm_connector *connector)
>  {
>  	struct drm_minor *minor = connector->dev->primary;
> -	struct dentry *root, *ent;
> +	struct dentry *root;
>  
>  	if (!minor->debugfs_root)
> -		return -1;
> +		return;
>  
>  	root = debugfs_create_dir(connector->name, minor->debugfs_root);
> -	if (!root)
> -		return -ENOMEM;
> -
>  	connector->debugfs_entry = root;
>  
>  	/* force */
> -	ent = debugfs_create_file("force", S_IRUGO | S_IWUSR, root, connector,
> -				  &drm_connector_fops);
> -	if (!ent)
> -		goto error;
> +	debugfs_create_file("force", S_IRUGO | S_IWUSR, root, connector,
> +			    &drm_connector_fops);
>  
>  	/* edid */
> -	ent = debugfs_create_file("edid_override", S_IRUGO | S_IWUSR, root,
> -				  connector, &drm_edid_fops);
> -	if (!ent)
> -		goto error;
> -
> -	return 0;
> -
> -error:
> -	debugfs_remove_recursive(connector->debugfs_entry);
> -	connector->debugfs_entry = NULL;
> -	return -ENOMEM;
> +	debugfs_create_file("edid_override", S_IRUGO | S_IWUSR, root, connector,
> +			    &drm_edid_fops);
>  }
>  
>  void drm_debugfs_connector_remove(struct drm_connector *connector)
> @@ -482,7 +462,7 @@ void drm_debugfs_connector_remove(struct drm_connector *connector)
>  	connector->debugfs_entry = NULL;
>  }
>  
> -int drm_debugfs_crtc_add(struct drm_crtc *crtc)
> +void drm_debugfs_crtc_add(struct drm_crtc *crtc)
>  {
>  	struct drm_minor *minor = crtc->dev->primary;
>  	struct dentry *root;
> @@ -490,23 +470,14 @@ int drm_debugfs_crtc_add(struct drm_crtc *crtc)
>  
>  	name = kasprintf(GFP_KERNEL, "crtc-%d", crtc->index);
>  	if (!name)
> -		return -ENOMEM;
> +		return;
>  
>  	root = debugfs_create_dir(name, minor->debugfs_root);
>  	kfree(name);
> -	if (!root)
> -		return -ENOMEM;
>  
>  	crtc->debugfs_entry = root;
>  
> -	if (drm_debugfs_crtc_crc_add(crtc))
> -		goto error;
> -
> -	return 0;
> -
> -error:
> -	drm_debugfs_crtc_remove(crtc);
> -	return -ENOMEM;
> +	drm_debugfs_crtc_crc_add(crtc);
>  }
>  
>  void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
> diff --git a/drivers/gpu/drm/drm_debugfs_crc.c b/drivers/gpu/drm/drm_debugfs_crc.c
> index 00e743153e94..f9e317046551 100644
> --- a/drivers/gpu/drm/drm_debugfs_crc.c
> +++ b/drivers/gpu/drm/drm_debugfs_crc.c
> @@ -344,33 +344,19 @@ static const struct file_operations drm_crtc_crc_data_fops = {
>  	.release = crtc_crc_release,
>  };
>  
> -int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
> +void drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
>  {
> -	struct dentry *crc_ent, *ent;
> +	struct dentry *crc_ent;
>  
>  	if (!crtc->funcs->set_crc_source || !crtc->funcs->verify_crc_source)
> -		return 0;
> +		return;
>  
>  	crc_ent = debugfs_create_dir("crc", crtc->debugfs_entry);
> -	if (!crc_ent)
> -		return -ENOMEM;
> -
> -	ent = debugfs_create_file("control", S_IRUGO, crc_ent, crtc,
> -				  &drm_crtc_crc_control_fops);
> -	if (!ent)
> -		goto error;
> -
> -	ent = debugfs_create_file("data", S_IRUGO, crc_ent, crtc,
> -				  &drm_crtc_crc_data_fops);
> -	if (!ent)
> -		goto error;
> -
> -	return 0;
> -
> -error:
> -	debugfs_remove_recursive(crc_ent);
>  
> -	return -ENOMEM;
> +	debugfs_create_file("control", S_IRUGO, crc_ent, crtc,
> +			    &drm_crtc_crc_control_fops);
> +	debugfs_create_file("data", S_IRUGO, crc_ent, crtc,
> +			    &drm_crtc_crc_data_fops);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 862621494a93..da9a83da37eb 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -1161,11 +1161,6 @@ static int __init drm_core_init(void)
>  	}
>  
>  	drm_debugfs_root = debugfs_create_dir("dri", NULL);
> -	if (!drm_debugfs_root) {
> -		ret = -ENOMEM;
> -		DRM_ERROR("Cannot create debugfs-root: %d\n", ret);
> -		goto error;
> -	}
>  
>  	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 e19ac7ca602d..938a6f06d7c7 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -126,12 +126,12 @@ void drm_gem_print_info(struct drm_printer *p, unsigned int indent,
>  #if defined(CONFIG_DEBUG_FS)
>  int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  		     struct dentry *root);
> -int drm_debugfs_cleanup(struct drm_minor *minor);
> -int drm_debugfs_connector_add(struct drm_connector *connector);
> +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);
> -int drm_debugfs_crtc_add(struct drm_crtc *crtc);
> +void drm_debugfs_crtc_add(struct drm_crtc *crtc);
>  void drm_debugfs_crtc_remove(struct drm_crtc *crtc);
> -int drm_debugfs_crtc_crc_add(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,
>  				   struct dentry *root)
> @@ -139,30 +139,26 @@ static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
>  	return 0;
>  }
>  
> -static inline int drm_debugfs_cleanup(struct drm_minor *minor)
> +static inline void drm_debugfs_cleanup(struct drm_minor *minor)
>  {
> -	return 0;
>  }
>  
> -static inline int drm_debugfs_connector_add(struct drm_connector *connector)
> +static inline void drm_debugfs_connector_add(struct drm_connector *connector)
>  {
> -	return 0;
>  }
>  static inline void drm_debugfs_connector_remove(struct drm_connector *connector)
>  {
>  }
>  
> -static inline int drm_debugfs_crtc_add(struct drm_crtc *crtc)
> +static inline void drm_debugfs_crtc_add(struct drm_crtc *crtc)
>  {
> -	return 0;
>  }
>  static inline void drm_debugfs_crtc_remove(struct drm_crtc *crtc)
>  {
>  }
>  
> -static inline int drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
> +static inline void drm_debugfs_crtc_crc_add(struct drm_crtc *crtc)
>  {
> -	return 0;
>  }
>  
>  #endif
> -- 
> 2.22.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux