Re: [PATCHv17 34/34] RFC: media-requests: add debugfs node
Em Tue, 14 Aug 2018 09:33:16 +0200
Hans Verkuil escreveu:
> On 13/08/18 17:15, Mauro Carvalho Chehab wrote:
> > Em Sat, 4 Aug 2018 14:45:26 +0200
> > Hans Verkuil escreveu:
> >
> >> From: Hans Verkuil
> >>
> >> Keep track of the number of requests and request objects of a media
> >> device. Helps to verify that all request-related memory is freed.
Ok. So, let's skip it on your next patchset. Debugfs can
be added any time after the requests API main patchset gets merged.
> >>
> >> Signed-off-by: Hans Verkuil
> >
> >
> >
> >> ---
> >> 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
>
> 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(>num_requests));
> >> + seq_printf(file, "number of request objects: %d\n",
> >> + atomic_read(>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(>graph_mutex);
> >>ida_init(>entity_internal_idx);
> >>
> >> + atomic_set(>num_requests, 0);
> >> + atomic_set(>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(>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(>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(>devnode->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
Re: [PATCHv17 34/34] RFC: media-requests: add debugfs node
On 13/08/18 17:15, Mauro Carvalho Chehab wrote:
> Em Sat, 4 Aug 2018 14:45:26 +0200
> Hans Verkuil escreveu:
>
>> From: Hans Verkuil
>>
>> 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
>
>
>
>> ---
>> 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
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(>num_requests));
>> +seq_printf(file, "number of request objects: %d\n",
>> + atomic_read(>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(>graph_mutex);
>> ida_init(>entity_internal_idx);
>>
>> +atomic_set(>num_requests, 0);
>> +atomic_set(>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(>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(>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(>devnode->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(>dev, mdev);
>> +
>> /* Part 4: Activate this minor. The char device can now be used. */
>> set_bit(MEDIA_FLAG_REGISTERED, >flags);
>>
>> @@ -310,6 +318,14 @@ static int __init media_devnode_init(void)
>> return ret;
>> }
>>
>> +#ifdef CONFIG_DEBUG_FS
>> +
Re: [PATCHv17 34/34] RFC: media-requests: add debugfs node
Em Sat, 4 Aug 2018 14:45:26 +0200
Hans Verkuil escreveu:
> From: Hans Verkuil
>
> 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
> ---
> 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
> +/*
> + * 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(>num_requests));
> + seq_printf(file, "number of request objects: %d\n",
> +atomic_read(>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(>graph_mutex);
> ida_init(>entity_internal_idx);
>
> + atomic_set(>num_requests, 0);
> + atomic_set(>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(>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(>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(>devnode->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(>dev, mdev);
> +
> /* Part 4: Activate this minor. The char device can now be used. */
> set_bit(MEDIA_FLAG_REGISTERED, >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(_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)
> {
> +
[PATCHv17 34/34] RFC: media-requests: add debugfs node
From: Hans Verkuil
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
---
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
+/*
+ * 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(>num_requests));
+ seq_printf(file, "number of request objects: %d\n",
+ atomic_read(>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(>graph_mutex);
ida_init(>entity_internal_idx);
+ atomic_set(>num_requests, 0);
+ atomic_set(>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(>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(>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(>devnode->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(>dev, mdev);
+
/* Part 4: Activate this minor. The char device can now be used. */
set_bit(MEDIA_FLAG_REGISTERED, >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(_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(_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);
+
