dri-devel Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>,
	dri-devel@lists.freedesktop.org, David Airlie <airlied@linux.ie>,
	Sean Paul <sean@poorly.run>
Subject: Re: [PATCH] drm: no need to check return value of debugfs_create functions
Date: Thu, 13 Jun 2019 16:41:21 +0200	[thread overview]
Message-ID: <20190613144121.GP23020@phenom.ffwll.local> (raw)
In-Reply-To: <20190613133439.GA6715@kroah.com>

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@linux.intel.com>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

      reply	other threads:[~2019-06-13 14:41 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-13 13:34 [PATCH] drm: no need to check return value of debugfs_create functions Greg Kroah-Hartman
2019-06-13 14:41 ` Daniel Vetter [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190613144121.GP23020@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=airlied@linux.ie \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=maxime.ripard@bootlin.com \
    --cc=sean@poorly.run \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox