On Thu, 12 Jan 2023, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Thu, Jan 12, 2023 at 10:50:40AM +0200, Jani Nikula wrote:
>> On Wed, 11 Jan 2023, Maíra Canal <mcanal@xxxxxxxxxx> 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@xxxxxxxxxx>
>> > ---
>> > 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@xxxxxxxxx
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
