On Wed, Feb 08, 2023 at 07:06:19PM +0100, Daniel Vetter wrote:
> On Tue, Jan 31, 2023 at 04:58:21PM -0300, Maíra Canal wrote:
> > Introduce a struct wrapper for all the debugfs-related stuff: the list
> > of debugfs files and the mutex that protects it. This will make it
> > easier to initialize all the debugfs list in a DRM object and will
> > create a good abstraction for a possible implementation of the debugfs
> > infrastructure for KMS objects.
> >
> > Signed-off-by: Maíra Canal <mcanal@xxxxxxxxxx>
> > ---
> > drivers/gpu/drm/drm_debugfs.c | 18 ++++++++++++++++++
> > drivers/gpu/drm/drm_internal.h | 12 ++++++++++++
> > include/drm/drm_debugfs.h | 16 ++++++++++++++++
> > 3 files changed, 46 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> > index 4f643a490dc3..8658d3929ea5 100644
> > --- a/drivers/gpu/drm/drm_debugfs.c
> > +++ b/drivers/gpu/drm/drm_debugfs.c
> > @@ -218,6 +218,24 @@ void drm_debugfs_create_files(const struct drm_info_list *files, int count,
> > }
> > EXPORT_SYMBOL(drm_debugfs_create_files);
> >
> > +struct drm_debugfs_files *drm_debugfs_files_init(void)
> > +{
> > + struct drm_debugfs_files *debugfs_files;
> > +
> > + debugfs_files = kzalloc(sizeof(*debugfs_files), GFP_KERNEL);
> > +
> > + INIT_LIST_HEAD(&debugfs_files->list);
> > + mutex_init(&debugfs_files->mutex);
> > +
> > + return debugfs_files;
> > +}
> > +
> > +void drm_debugfs_files_destroy(struct drm_debugfs_files *debugfs_files)
> > +{
> > + mutex_destroy(&debugfs_files->mutex);
> > + kfree(debugfs_files);
> > +}
> > +
> > int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> > struct dentry *root)
> > {
> > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> > index ed2103ee272c..f1c8766ed828 100644
> > --- a/drivers/gpu/drm/drm_internal.h
> > +++ b/drivers/gpu/drm/drm_internal.h
> > @@ -23,6 +23,7 @@
> >
> > #include <linux/kthread.h>
> >
> > +#include <drm/drm_debugfs.h>
> > #include <drm/drm_ioctl.h>
> > #include <drm/drm_vblank.h>
> >
> > @@ -183,6 +184,8 @@ int drm_gem_dumb_destroy(struct drm_file *file, struct drm_device *dev,
> >
> > /* drm_debugfs.c drm_debugfs_crc.c */
> > #if defined(CONFIG_DEBUG_FS)
> > +struct drm_debugfs_files *drm_debugfs_files_init(void);
> > +void drm_debugfs_files_destroy(struct drm_debugfs_files *debugfs_files);
> > int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> > struct dentry *root);
> > void drm_debugfs_cleanup(struct drm_minor *minor);
> > @@ -193,6 +196,15 @@ void drm_debugfs_crtc_add(struct drm_crtc *crtc);
> > void drm_debugfs_crtc_remove(struct drm_crtc *crtc);
> > void drm_debugfs_crtc_crc_add(struct drm_crtc *crtc);
> > #else
> > +static inline struct drm_debugfs_files *drm_debugfs_files_init(void)
> > +{
> > + return NULL;
> > +}
> > +
> > +static inline void drm_debugfs_files_destroy(struct drm_debugfs_files *debugfs_files)
> > +{
> > +}
> > +
> > static inline int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> > struct dentry *root)
> > {
> > diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
> > index 7616f457ce70..423aa3de506a 100644
> > --- a/include/drm/drm_debugfs.h
> > +++ b/include/drm/drm_debugfs.h
> > @@ -32,6 +32,8 @@
> > #ifndef _DRM_DEBUGFS_H_
> > #define _DRM_DEBUGFS_H_
> >
> > +#include <linux/list.h>
> > +#include <linux/mutex.h>
> > #include <linux/types.h>
> > #include <linux/seq_file.h>
> > /**
> > @@ -79,6 +81,20 @@ struct drm_info_node {
> > struct dentry *dent;
> > };
> >
> > +/**
> > + * struct drm_debugfs_files - Encapsulates the debugfs list and its mutex
> > + *
> > + * This structure represents the debugfs list of files and is encapsulated
> > + * with a mutex to protect the access of the list.
> > + */
> > +struct drm_debugfs_files {
> > + /** @list: List of debugfs files to be created by the DRM object. */
> > + struct list_head list;
> > +
> > + /** @mutex: Protects &list access. */
> > + struct mutex mutex;
>
> I'm not seeing any use for the mutex here? Also unless you also plan to
> put like the debugfs directory pointers in this struct, I'm not sure we
> need this abstraction since it's purely internal to debugfs code (so also
> should really be in the headers where drivers could perhaps come up with
> funny ideas).
To clarify, I think any struct or code which is potentially type unsafe,
like this one here or the drm_debugfs_entry one, should be moved into
drm_debugfs.c. That way drivers do not ever see the potentially dangerous
pieces, and only have a type-safe interface for everything.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
