[PATCH] drm: no need to check return value of debugfs_create functions
Daniel Vetter
daniel at ffwll.ch
Thu Jun 13 14:41:21 UTC 2019
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 at linux.intel.com>
> Cc: Maxime Ripard <maxime.ripard at bootlin.com>
> Cc: Sean Paul <sean at poorly.run>
> Cc: David Airlie <airlied at linux.ie>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: dri-devel at lists.freedesktop.org
> Signed-off-by: Greg Kroah-Hartman <gregkh at 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
More information about the dri-devel
mailing list