Re: [PATCH v3 3/5] media: mc: add debugfs node to keep track of requests

Sakari Ailus
2025-06-04T21:35:25Z

Hi Nicolas, Hans,

Thanks for the update.

On Wed, Jun 04, 2025 at 04:09:37PM -0400, Nicolas Dufresne wrote:
> From: Hans Verkuil <hverkuil@xs4all.nl>
>
> Keep track of the number of requests and request objects of a media
> device. Helps to verify that all request-related memory is freed.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
> drivers/media/mc/mc-device.c | 30 ++++++++++++++++++++++++++++++
> drivers/media/mc/mc-devnode.c | 5 +++++
> drivers/media/mc/mc-request.c | 6 ++++++
> include/media/media-device.h | 9 +++++++++
> include/media/media-devnode.h | 4 ++++
> include/media/media-request.h | 2 ++
> 6 files changed, 56 insertions(+)
>
> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> index c0dd4ae5722725f1744bc6fd6282d5c765438059..5a458160200afb540d8014fed42d8bf2dab9c8c3 100644
> --- a/drivers/media/mc/mc-device.c
> +++ b/drivers/media/mc/mc-device.c
> @@ -679,6 +679,23 @@ void media_device_unregister_entity(struct media_entity *entity)
> }
> EXPORT_SYMBOL_GPL(media_device_unregister_entity);
>
> +#ifdef CONFIG_DEBUG_FS
> +/*
> + * Log the state of media requests.
> + * Very useful for debugging.
> + */

Fits on a single line.

> +static int media_device_requests(struct seq_file *file, void *priv)
> +{
> + struct media_device *dev = dev_get_drvdata(file->private);
> +
> + seq_printf(file, "number of requests: %d\n",
> + atomic_read(&dev->num_requests));
> + seq_printf(file, "number of request objects: %d\n",
> + atomic_read(&dev->num_request_objects));

Newline here?

> + return 0;
> +}
> +#endif
> +
> void media_device_init(struct media_device *mdev)
> {
> INIT_LIST_HEAD(&mdev->entities);
> @@ -697,6 +714,9 @@ void media_device_init(struct media_device *mdev)
> media_set_bus_info(mdev->bus_info, sizeof(mdev->bus_info),
> mdev->dev);
>
> + atomic_set(&mdev->num_requests, 0);
> + atomic_set(&mdev->num_request_objects, 0);
> +
> dev_dbg(mdev->dev, "Media device initialized\n");
> }
> EXPORT_SYMBOL_GPL(media_device_init);
> @@ -748,6 +768,15 @@ int __must_check __media_device_register(struct media_device *mdev,
>
> dev_dbg(mdev->dev, "Media device registered\n");
>
> +#ifdef CONFIG_DEBUG_FS
> + if (!media_debugfs_root)
> + media_debugfs_root = debugfs_create_dir("media", NULL);
> + mdev->media_dir = debugfs_create_dir(dev_name(&devnode->dev),
> + media_debugfs_root);
> + debugfs_create_devm_seqfile(&devnode->dev, "requests",
> + mdev->media_dir, media_device_requests);
> +#endif

I have no objection to this but it would have been great to have the Media
device lifetime set in first and MC device and devnode merged. But maybe
it's too late for that. Well, at least this won't change error handling...

> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(__media_device_register);
> @@ -824,6 +853,7 @@ void media_device_unregister(struct media_device *mdev)
>
> dev_dbg(mdev->dev, "Media device unregistered\n");
>
> + debugfs_remove_recursive(mdev->media_dir);
> device_remove_file(&mdev->devnode->dev, &dev_attr_model);
> media_devnode_unregister(mdev->devnode);
> /* devnode free is handled in media_devnode_*() */
> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> index 56444edaf13651874331e7c04e86b0a585067d38..d0a8bcc11dd6350fdbc04add70f62de2c5f01178 100644
> --- a/drivers/media/mc/mc-devnode.c
> +++ b/drivers/media/mc/mc-devnode.c
> @@ -45,6 +45,9 @@ static dev_t media_dev_t;
> static DEFINE_MUTEX(media_devnode_lock);
> static DECLARE_BITMAP(media_devnode_nums, MEDIA_NUM_DEVICES);
>
> +/* debugfs */
> +struct dentry *media_debugfs_root;
> +
> /* Called when the last user of the media device exits. */
> static void media_devnode_release(struct device *cd)
> {
> @@ -236,6 +239,7 @@ int __must_check media_devnode_register(struct media_device *mdev,
> if (devnode->parent)
> devnode->dev.parent = devnode->parent;
> dev_set_name(&devnode->dev, "media%d", devnode->minor);
> + dev_set_drvdata(&devnode->dev, mdev);
> device_initialize(&devnode->dev);
>
> /* Part 2: Initialize the character device */
> @@ -313,6 +317,7 @@ static int __init media_devnode_init(void)
>
> static void __exit media_devnode_exit(void)
> {
> + debugfs_remove_recursive(media_debugfs_root);
> bus_unregister(&media_bus_type);
> unregister_chrdev_region(media_dev_t, MEDIA_NUM_DEVICES);
> }
> diff --git a/drivers/media/mc/mc-request.c b/drivers/media/mc/mc-request.c
> index 398d0806d1d274eb8c454fc5c37b77476abe1e74..829e35a5d56d41c52cc583cdea1c959bcb4fce60 100644
> --- a/drivers/media/mc/mc-request.c
> +++ b/drivers/media/mc/mc-request.c
> @@ -75,6 +75,7 @@ static void media_request_release(struct kref *kref)
> mdev->ops->req_free(req);
> else
> kfree(req);
> + atomic_dec(&mdev->num_requests);
> }
>
> void media_request_put(struct media_request *req)
> @@ -326,6 +327,7 @@ int media_request_alloc(struct media_device *mdev, int *alloc_fd)
>
> snprintf(req->debug_str, sizeof(req->debug_str), "%u:%d",
> atomic_inc_return(&mdev->request_id), fd);
> + atomic_inc(&mdev->num_requests);
> dev_dbg(mdev->dev, "request: allocated %s\n", req->debug_str);
>
> fd_install(fd, filp);
> @@ -349,10 +351,12 @@ static void media_request_object_release(struct kref *kref)
> struct media_request_object *obj =
> container_of(kref, struct media_request_object, kref);
> struct media_request *req = obj->req;
> + struct media_device *mdev = obj->mdev;
>
> if (WARN_ON(req))
> media_request_object_unbind(obj);
> obj->ops->release(obj);
> + atomic_dec(&mdev->num_request_objects);
> }
>
> struct media_request_object *
> @@ -417,6 +421,7 @@ int media_request_object_bind(struct media_request *req,
> obj->req = req;
> obj->ops = ops;
> obj->priv = priv;
> + obj->mdev = req->mdev;
>
> if (is_buffer)
> list_add_tail(&obj->list, &req->objects);
> @@ -424,6 +429,7 @@ int media_request_object_bind(struct media_request *req,
> list_add(&obj->list, &req->objects);
> req->num_incomplete_objects++;
> ret = 0;
> + atomic_inc(&obj->mdev->num_request_objects);
>
> unlock:
> spin_unlock_irqrestore(&req->lock, flags);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index 53d2a16a70b0d9d6e5cc28fe1fc5d5ef384410d5..749c327e3c582c3c583e0394468321ccd6160da5 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -11,6 +11,7 @@
> #ifndef _MEDIA_DEVICE_H
> #define _MEDIA_DEVICE_H
>
> +#include <linux/atomic.h>
> #include <linux/list.h>
> #include <linux/mutex.h>
> #include <linux/pci.h>
> @@ -106,6 +107,9 @@ struct media_device_ops {
> * @ops: Operation handler callbacks
> * @req_queue_mutex: Serialise the MEDIA_REQUEST_IOC_QUEUE ioctl w.r.t.
> * other operations that stop or start streaming.
> + * @num_requests: number of associated requests
> + * @num_request_objects: number of associated request objects
> + * @media_dir: DebugFS media directory
> * @request_id: Used to generate unique request IDs
> *
> * This structure represents an abstract high-level media device. It allows easy
> @@ -179,6 +183,11 @@ struct media_device {
> const struct media_device_ops *ops;
>
> struct mutex req_queue_mutex;
> + atomic_t num_requests;
> + atomic_t num_request_objects;
> +
> + /* debugfs */
> + struct dentry *media_dir;
> atomic_t request_id;
> };
>
> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
> index d27c1c646c2805171be3997d72210dd4d1a38e32..dbcabeffcb572ae707f5fe1f51ff719d451c6784 100644
> --- a/include/media/media-devnode.h
> +++ b/include/media/media-devnode.h
> @@ -20,9 +20,13 @@
> #include <linux/fs.h>
> #include <linux/device.h>
> #include <linux/cdev.h>
> +#include <linux/debugfs.h>
>
> struct media_device;
>
> +/* debugfs top-level media directory */
> +extern struct dentry *media_debugfs_root;
> +
> /*
> * Flag to mark the media_devnode struct as registered. Drivers must not touch
> * this flag directly, it will be set and cleared by media_devnode_register and
> diff --git a/include/media/media-request.h b/include/media/media-request.h
> index 7f9af68ef19ac6de0184bbb0c0827dc59777c6dc..610ccfe8d7b20ec38e166383433f9ee208248640 100644
> --- a/include/media/media-request.h
> +++ b/include/media/media-request.h
> @@ -292,6 +292,7 @@ struct media_request_object_ops {
> * struct media_request_object - An opaque object that belongs to a media
> * request
> *
> + * @mdev: Media device this object belongs to

This deserves at least a comment what this may be used for: generally once
object is unbound, it's not related to a request anymore (nor a Media
device). This field also adds a new Media device lifetime issue: nothing
guarantees the mdev is not disappearing at a wrong time albeit this is
very, very likely not user-triggerable without physically removing
hardware.

> * @ops: object's operations
> * @priv: object's priv pointer
> * @req: the request this object belongs to (can be NULL)
> @@ -303,6 +304,7 @@ struct media_request_object_ops {
> * another struct that contains the actual data for this request object.
> */
> struct media_request_object {
> + struct media_device *mdev;
> const struct media_request_object_ops *ops;
> void *priv;
> struct media_request *req;
>

--
Regards,

Sakari Ailus