[PATCH v3 1/6] drm/debugfs: Introduce wrapper for debugfs list

[Maíra Canal]

On 2/8/23 15:06, 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 at igalia.com>
>> ---
>>   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).

Isn't this mutex needed to guarantee race-conditions safety when adding new
files to the list, as drm_debugfs_add_file() is called by the drivers? [1]

[1] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/drm_debugfs.c#n343

Best Regards,
- Maíra Canal

> -Daniel
> 
>> +};
>> +
>>   /**
>>    * struct drm_debugfs_info - debugfs info list entry
>>    *
>> -- 
>> 2.39.1
>>
>