| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <29c200ff-1fe8-4026-ad08-fc1da845622e@xs4all.nl>
Date: Mon, 9 Jun 2025 12:46:13 +0200
From: Hans Verkuil <hverkuil@...all.nl>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>,
Nicolas Dufresne <nicolas.dufresne@...labora.com>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Tiffany Lin <tiffany.lin@...iatek.com>,
Andrew-CT Chen <andrew-ct.chen@...iatek.com>,
Yunfei Dong <yunfei.dong@...iatek.com>,
Matthias Brugger <matthias.bgg@...il.com>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, kernel@...labora.com,
linux-media@...r.kernel.org
Subject: Re: [PATCH v3 3/5] media: mc: add debugfs node to keep track of
requests
On 09/06/2025 12:28, Sakari Ailus wrote:
> Hi Nicolas,
>
> On Wed, Jun 04, 2025 at 07:08:53PM -0400, Nicolas Dufresne wrote:
>> Le mercredi 04 juin 2025 à 21:33 +0000, Sakari Ailus a écrit :
>>> Hi Nicolas, Hans,
>>>
>>> Thanks for the update.
>>
>> thanks for the review, these things are precious.
>>
>>>
>>> On Wed, Jun 04, 2025 at 04:09:37PM -0400, Nicolas Dufresne wrote:
>>>> From: Hans Verkuil <hverkuil@...all.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@...all.nl>
>>>> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@...labora.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.
>>
>> Ack.
>>
>>>
>>>> +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?
>>
>> I prefer that too.
>>
>>>
>>>> + 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...
>>
>> Since this specific patch is not required to fix the MTK VCODEC issue, I can
>> delay this a little. Is that comment related to an existing patch ?
>
> Yes.
>
> I've pushed the current set here:
> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=media-ref>. I've
> rebased it recently but it's still WiP.
Since this patch series has been going on for many years now, I do not believe
that that should prevent this specific patch from being merged. It is generally
a bad idea to do that, unless it is absolutely certain that such a patch series
will be merged in a few weeks tops.
I've carried this patch around out-of-tree ever since I started the first
request implementation because without it it is very hard to check that all
requests are properly freed. So getting this in is actually quite useful.
And when it is in I will probably add a test in test-media to improve the stateless
decoder regression test, verifying that all requests are freed.
Regards,
Hans
>
> ...
>
>>>> 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
>>
>> We could make it explicit by clearing the mdev pointer ?
>
> That would probably be out of scope of this patch(set). Also see the
> patchset I referred to earlier.
>
>>
>>> guarantees the mdev is not disappearing at a wrong time albeit this is
>>> very, very likely not user-triggerable without physically removing
>>> hardware.
>>
>> I'm not too familiar with the subject, if the MC knows it has open request
>> FD(s), why would it allow userspace from unloading its module ?
>
> Drivers nor MC currently have a list of request file handles.
>
> Apart from the file handles, that was the original thinking, yes, but
> devices can be also unbound without touching the driver (or other) modules.
>
>>
>>>
>>>> * @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;
>>>>
>
Powered by blists - more mailing lists