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

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

On Thu, 12 Jan 2023, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Jan 12, 2023 at 10:50:40AM +0200, Jani Nikula wrote:
>> 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.
>
> So the non-generics way I guess would be to
> - pass the right pointer to the functions as an explicit parameter (struct
>   drm_device|crtc|connector *, )
> - make drm_debugfs_entry and implementation detail
> - switch the pointer in there to void *, have glue show functions for each
>   case which do nothing else than cast from void * to the right type
>   (both for the parameter and the function pointer)
> - have a single function which takes that void *entry list and a pointer
>   to the debugfs director to add them all for code sharing
>
> I think this should work for ->show, but for ->fops it becomes a rather
> big mess I fear. Maybe for ->fops (and also for ->show for now) we leave
> the explicit parameter out and just rely on seq_file->private or whatever
> it was.

Similar ideas in https://lore.kernel.org/r/878ri8glee.fsf@intel.com

I think for the more general ->fops case, the question really becomes
does drm need *all* the functionality of ->fops, or can we get away with
providing just show/write functions pointers, and wrapping it all inside
drm_debugfs.c? The question *now* is avoiding to paint ourselves in the
corner and requiring a ton of *extra* work to make that happen.


BR,
Jani.




>
> Or just copypaste, it's not that much code really :-)
> -Daniel
>
>> 
>> 
>> 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

-- 
Jani Nikula, Intel Open Source Graphics Center