Re: [PATCH 01/13] drm/debugfs: Create helper to add debugfs files to device's list

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 11 Jan 2023, Maíra Canal <mcanal@igalia.com> wrote:
> Create a helper to encapsulate the code that adds a new debugfs file to
> a linked list related to a object. Moreover, the helper also provides
> more flexibily on the type of the object, allowing to use the helper for
> other types of drm_debugfs_entry.
>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  drivers/gpu/drm/drm_debugfs.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 4f643a490dc3..255d2068ac16 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -316,6 +316,17 @@ void drm_debugfs_cleanup(struct drm_minor *minor)
>  	minor->debugfs_root = NULL;
>  }
>  
> +#define drm_debugfs_add_file_to_list(component) do {			\
> +		entry->file.name = name;				\
> +		entry->file.show = show;				\
> +		entry->file.data = data;				\
> +		entry->component = (component);				\
> +									\
> +		mutex_lock(&(component)->debugfs_mutex);		\
> +		list_add(&entry->list, &(component)->debugfs_list);	\
> +		mutex_unlock(&(component)->debugfs_mutex);		\
> +	} while (0)

In general, please don't add macros that implicitly depend on certain
local variable names. In this case, "entry".

But I'm also not convinced about the usefulness of adding this kind of
"generics". Sure, it'll save you a few lines here and there, but I think
overall it's just confusing more than it's useful.


BR,
Jani.

> +
>  /**
>   * drm_debugfs_add_file - Add a given file to the DRM device debugfs file list
>   * @dev: drm device for the ioctl
> @@ -334,14 +345,7 @@ void drm_debugfs_add_file(struct drm_device *dev, const char *name,
>  	if (!entry)
>  		return;
>  
> -	entry->file.name = name;
> -	entry->file.show = show;
> -	entry->file.data = data;
> -	entry->dev = dev;
> -
> -	mutex_lock(&dev->debugfs_mutex);
> -	list_add(&entry->list, &dev->debugfs_list);
> -	mutex_unlock(&dev->debugfs_mutex);
> +	drm_debugfs_add_file_to_list(dev);
>  }
>  EXPORT_SYMBOL(drm_debugfs_add_file);

-- 
Jani Nikula, Intel Open Source Graphics Center