[PATCH v4 2/4] drm: add debugfs support on per client-id basis

[Tvrtko Ursulin]

On 18/06/2025 14:47, Sunil Khatri wrote:
> add support to add a directory for each client-id
> with root at the dri level. Since the clients are
> unique and not just related to one single drm device,
> so it makes more sense to add all the client based
> nodes with root as dri.
> 
> Also create a symlink back to the parent drm device
> from each client.

TBH I can see an use case for both clients at DRI level and clients 
under DRM devices. I guess you have an use case for global and per 
device can be added later if it becomes needed.

> 
> Signed-off-by: Sunil Khatri <sunil.khatri at amd.com>
> ---
>   drivers/gpu/drm/drm_debugfs.c | 32 ++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/drm_file.c    | 10 ++++++++++
>   include/drm/drm_debugfs.h     | 12 ++++++++++++
>   include/drm/drm_file.h        |  7 +++++++
>   4 files changed, 61 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> index 5a33ec299c04..875276d5fb9f 100644
> --- a/drivers/gpu/drm/drm_debugfs.c
> +++ b/drivers/gpu/drm/drm_debugfs.c
> @@ -298,6 +298,38 @@ void drm_debugfs_remove_dir(void)
>   	debugfs_remove(drm_debugfs_root);
>   }
>   
> +int drm_debugfs_clients_add(struct drm_file *file)
> +{
> +	struct drm_device *dev;
> +	char *client_dir, *symlink;
> +
> +	dev = file->minor->dev;

FWIW, as dev is only used once and string locals are not overlapping, 
you could reduce to a single local variable like char *name and re-use 
it. Up to you.

> +
> +	client_dir = kasprintf(GFP_KERNEL, "client-%llu", file->client_id);
> +	if (!client_dir)
> +		return -ENOMEM;

It is a bit more work, but I think a clients/ directory with numerical 
client id subdirs would be nicer.

> +
> +	/* Create a debugfs directory for the client in root on drm debugfs */
> +	file->debugfs_client = debugfs_create_dir(client_dir, drm_debugfs_root);
> +	kfree(client_dir);
> +
> +	symlink = kasprintf(GFP_KERNEL, "../%s", dev->unique);
> +	if (!symlink)
> +		return -ENOMEM;

Worth removing the partial construction?

> +
> +	/* Create a link from client_id to the drm device this client id belongs to */
> +	debugfs_create_symlink("device", file->debugfs_client, symlink);

This can also fail.

> +	kfree(symlink);
> +
> +	return 0;
> +}
> +
> +void drm_debugfs_clients_remove(struct drm_file *file)
> +{
> +	debugfs_remove_recursive(file->debugfs_client);
> +	file->debugfs_client = NULL;
> +}
> +
>   /**
>    * drm_debugfs_dev_init - create debugfs directory for the device
>    * @dev: the device which we want to create the directory for
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 06ba6dcbf5ae..8502c5a630b1 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -39,12 +39,14 @@
>   #include <linux/poll.h>
>   #include <linux/slab.h>
>   #include <linux/vga_switcheroo.h>
> +#include <linux/debugfs.h>
>   
>   #include <drm/drm_client_event.h>
>   #include <drm/drm_drv.h>
>   #include <drm/drm_file.h>
>   #include <drm/drm_gem.h>
>   #include <drm/drm_print.h>
> +#include <drm/drm_debugfs.h>
>   
>   #include "drm_crtc_internal.h"
>   #include "drm_internal.h"
> @@ -143,6 +145,13 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>   	rcu_assign_pointer(file->pid, get_pid(task_tgid(current)));
>   	file->minor = minor;
>   
> +	ret = drm_debugfs_clients_add(file);

Slightly tricky part is that as soon as this runs userspace can enter 
debugfs. If in the future any debufs clients file is added which can 
dereference any of the drm_file fields not yet initialized it has the 
potential to explode and/or be exploited.

Hence I think to be safe the usual pattern of exposing drm_file to 
userspace at the end, only _after_ drm_file has been *fully* initialized.

Slightly annoying part with that might be undoing dev->driver->open() 
but maybe it is not that bad.

> +	if (ret) {
> +		put_pid(rcu_access_pointer(file->pid));
> +		kfree(file);
> +		return ERR_PTR(ret);

Onion unwind already exists in the function so could have used it. (Add 
a new label and here simply "goto out_put_pid".) But as above we discuss 
tweaking the order lets see how that goes first.

> +	}
> +
>   	/* for compatibility root is always authenticated */
>   	file->authenticated = capable(CAP_SYS_ADMIN);
>   
> @@ -236,6 +245,7 @@ void drm_file_free(struct drm_file *file)
>   		     atomic_read(&dev->open_count));
>   
>   	drm_events_release(file);
> +	drm_debugfs_clients_remove(file);
>   
>   	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>   		drm_fb_release(file);
> diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
> index cf06cee4343f..4bd6cc1d0900 100644
> --- a/include/drm/drm_debugfs.h
> +++ b/include/drm/drm_debugfs.h
> @@ -153,6 +153,9 @@ void drm_debugfs_add_files(struct drm_device *dev,
>   
>   int drm_debugfs_gpuva_info(struct seq_file *m,
>   			   struct drm_gpuvm *gpuvm);
> +
> +int drm_debugfs_clients_add(struct drm_file *file);
> +void drm_debugfs_clients_remove(struct drm_file *file);
>   #else
>   static inline void drm_debugfs_create_files(const struct drm_info_list *files,
>   					    int count, struct dentry *root,
> @@ -181,6 +184,15 @@ static inline int drm_debugfs_gpuva_info(struct seq_file *m,
>   {
>   	return 0;
>   }
> +
> +int drm_debugfs_clients_add(struct drm_file *file)
> +{
> +	return 0;
> +}
> +
> +void drm_debugfs_clients_remove(struct drm_file *file)
> +{
> +}

Static inline for the two above.

>   #endif
>   
>   #endif /* _DRM_DEBUGFS_H_ */
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 5c3b2aa3e69d..eab7546aad79 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -400,6 +400,13 @@ struct drm_file {
>   	 * @client_name_lock: Protects @client_name.
>   	 */
>   	struct mutex client_name_lock;
> +
> +	/**
> +	 * @debugfs_client:
> +	 *
> +	 * debugfs directory for each client under a drm node.
> +	 */
> +	struct dentry *debugfs_client;

Is it worth idefing this out if !CONFIG_DEBUG_FS?

Regards,

Tvrtko

>   };
>   
>   /**