On 13/08/18 17:15, Mauro Carvalho Chehab wrote:
> Em Sat, 4 Aug 2018 14:45:26 +0200
> Hans Verkuil <hverk...@xs4all.nl> escreveu:
>
>> From: Hans Verkuil <hans.verk...@cisco.com>
>>
>> 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 <hans.verk...@cisco.com>
>
>
>
>> ---
>> drivers/media/media-device.c | 41 +++++++++++++++++++++++++++++++++++
>> drivers/media/media-devnode.c | 17 +++++++++++++++
>> drivers/media/media-request.c | 5 +++++
>> include/media/media-device.h | 11 ++++++++++
>> include/media/media-devnode.h | 4 ++++
>> include/media/media-request.h | 2 ++
>> 6 files changed, 80 insertions(+)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index 4b9a8de05562..28a891b53886 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -691,6 +691,23 @@ void media_device_unregister_entity(struct media_entity
>> *entity)
>> }
>> EXPORT_SYMBOL_GPL(media_device_unregister_entity);
>>
>> +#ifdef CONFIG_DEBUG_FS
>
> Patch itself looks good. Yet, perhaps we could request both
> CONFIG_DEBUG_FS and CONFIG_VIDEO_ADV_DEBUG.
>
> Also, instead of ifdef, please use IS_ENABLED for DEBUG_FS. That tends
> to be safer long term.
>
> With that:
>
> Reviewed-by: Mauro Carvalho Chehab <mchehab+sams...@kernel.org>
I don't intend to merge this patch yet. I'm not happy with it: I would
really like to count the objects per-request instead of globally, and
provide some more information about requests and their objects.
It was really a quick hack so that I could verify that all requests and
objects are properly freed.
Regards,
Hans
>
>> +/*
>> + * Log the state of media requests.
>> + * Very useful for debugging.
>> + */
>> +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));
>> + return 0;
>> +}
>> +#endif
>> +
>> /**
>> * media_device_init() - initialize a media device
>> * @mdev: The media device
>> @@ -713,6 +730,9 @@ void media_device_init(struct media_device *mdev)
>> mutex_init(&mdev->graph_mutex);
>> ida_init(&mdev->entity_internal_idx);
>>
>> + 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);
>> @@ -764,6 +784,26 @@ int __must_check __media_device_register(struct
>> media_device *mdev,
>>
>> dev_dbg(mdev->dev, "Media device registered\n");
>>
>> +#ifdef CONFIG_DEBUG_FS
>> + if (!media_top_dir)
>> + return 0;
>> +
>> + mdev->media_dir = debugfs_create_dir(dev_name(&devnode->dev),
>> + media_top_dir);
>> + if (IS_ERR_OR_NULL(mdev->media_dir)) {
>> + dev_warn(mdev->dev, "Failed to create debugfs dir\n");
>> + return 0;
>> + }
>> + mdev->requests_file = debugfs_create_devm_seqfile(&devnode->dev,
>> + "requests", mdev->media_dir, media_device_requests);
>> + if (IS_ERR_OR_NULL(mdev->requests_file)) {
>> + dev_warn(mdev->dev, "Failed to create requests file\n");
>> + debugfs_remove_recursive(mdev->media_dir);
>> + mdev->media_dir = NULL;
>> + return 0;
>> + }
>> +#endif
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(__media_device_register);
>> @@ -841,6 +881,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/media-devnode.c b/drivers/media/media-devnode.c
>> index 6b87a721dc49..4358ed22f208 100644
>> --- a/drivers/media/media-devnode.c
>> +++ b/drivers/media/media-devnode.c
>> @@ -53,6 +53,12 @@ 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_top_dir;
>> +
>> +
>> /* Called when the last user of the media device exits. */
>> static void media_devnode_release(struct device *cd)
>> {
>> @@ -259,6 +265,8 @@ int __must_check media_devnode_register(struct
>> media_device *mdev,
>> goto cdev_add_error;
>> }
>>
>> + dev_set_drvdata(&devnode->dev, mdev);
>> +
>> /* Part 4: Activate this minor. The char device can now be used. */
>> set_bit(MEDIA_FLAG_REGISTERED, &devnode->flags);
>>
>> @@ -310,6 +318,14 @@ static int __init media_devnode_init(void)
>> return ret;
>> }
>>
>> +#ifdef CONFIG_DEBUG_FS
>> + media_top_dir = debugfs_create_dir("media", NULL);
>> + if (IS_ERR_OR_NULL(media_top_dir)) {
>> + pr_warn("Failed to create debugfs media dir\n");
>> + media_top_dir = NULL;
>> + }
>> +#endif
>> +
>> ret = bus_register(&media_bus_type);
>> if (ret < 0) {
>> unregister_chrdev_region(media_dev_t, MEDIA_NUM_DEVICES);
>> @@ -322,6 +338,7 @@ static int __init media_devnode_init(void)
>>
>> static void __exit media_devnode_exit(void)
>> {
>> + debugfs_remove_recursive(media_top_dir);
>> bus_unregister(&media_bus_type);
>> unregister_chrdev_region(media_dev_t, MEDIA_NUM_DEVICES);
>> }
>> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
>> index a5b70a4e613b..8ba97a9c4bf1 100644
>> --- a/drivers/media/media-request.c
>> +++ b/drivers/media/media-request.c
>> @@ -72,6 +72,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)
>> @@ -318,6 +319,7 @@ int media_request_alloc(struct media_device *mdev,
>> get_task_comm(comm, current);
>> snprintf(req->debug_str, sizeof(req->debug_str), "%s:%d",
>> comm, fd);
>> + atomic_inc(&mdev->num_requests);
>> dev_dbg(mdev->dev, "request: allocated %s\n", req->debug_str);
>>
>> fd_install(fd, filp);
>> @@ -342,6 +344,7 @@ static void media_request_object_release(struct kref
>> *kref)
>> if (WARN_ON(req))
>> media_request_object_unbind(obj);
>> obj->ops->release(obj);
>> + atomic_dec(&obj->mdev->num_request_objects);
>> }
>>
>> struct media_request_object *
>> @@ -405,10 +408,12 @@ int media_request_object_bind(struct media_request
>> *req,
>> obj->req = req;
>> obj->ops = ops;
>> obj->priv = priv;
>> + obj->mdev = req->mdev;
>>
>> list_add_tail(&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 110b89567671..2de0606938d4 100644
>> --- a/include/media/media-device.h
>> +++ b/include/media/media-device.h
>> @@ -21,6 +21,7 @@
>>
>> #include <linux/list.h>
>> #include <linux/mutex.h>
>> +#include <linux/atomic.h>
>>
>> #include <media/media-devnode.h>
>> #include <media/media-entity.h>
>> @@ -107,6 +108,10 @@ 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
>> + * @requests_file: DebugFS requests file
>> *
>> * This structure represents an abstract high-level media device. It allows
>> easy
>> * access to entities and provides basic media device-level support. The
>> @@ -179,6 +184,12 @@ 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;
>> + struct dentry *requests_file;
>> };
>>
>> /* We don't need to include pci.h or usb.h here */
>> diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h
>> index dc2f64e1b08f..984b62b634d3 100644
>> --- a/include/media/media-devnode.h
>> +++ b/include/media/media-devnode.h
>> @@ -28,9 +28,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_top_dir;
>> +
>> /*
>> * 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 fd08d7a431a1..76727d4a89c3 100644
>> --- a/include/media/media-request.h
>> +++ b/include/media/media-request.h
>> @@ -210,6 +210,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
>> * @ops: object's operations
>> * @priv: object's priv pointer
>> * @req: the request this object belongs to (can be NULL)
>> @@ -221,6 +222,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;
>
>
>
> Thanks,
> Mauro
>
