| Message ID | 20230111173748.752659-2-mcanal@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/debugfs: Create a debugfs infrastructure for kms objects | expand |
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);
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. 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
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
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) + /** * 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);
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(-)