On 21/02/2024 11:40, Sakari Ailus wrote:
> Hi Hans,
>
> Many thanks for reviewing these.
>
> On Mon, Feb 05, 2024 at 03:56:22PM +0100, Hans Verkuil wrote:
>> On 20/12/2023 11:37, Sakari Ailus wrote:
>>> The video device depends on the existence of its media device --- if there
>>> is one. Acquire a reference to it.
>>>
>>> Note that when the media device release callback is used, then the V4L2
>>> device release callback is ignored and a warning is issued if both are
>>> set.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>>> ---
>>> drivers/media/v4l2-core/v4l2-dev.c | 51 ++++++++++++++++++++----------
>>> 1 file changed, 34 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>>> index d13954bd31fd..c1e4995eaf5c 100644
>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>> @@ -176,6 +176,11 @@ static void v4l2_device_release(struct device *cd)
>>> {
>>> struct video_device *vdev = to_video_device(cd);
>>> struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
>>> + bool v4l2_dev_has_release = v4l2_dev->release;
>>> +#ifdef CONFIG_MEDIA_CONTROLLER
>>> + struct media_device *mdev = v4l2_dev->mdev;
>>> + bool mdev_has_release = mdev && mdev->ops && mdev->ops->release;
>>> +#endif
>>>
>>> mutex_lock(&videodev_lock);
>>> if (WARN_ON(video_devices[vdev->minor] != vdev)) {
>>> @@ -198,8 +203,8 @@ static void v4l2_device_release(struct device *cd)
>>>
>>> mutex_unlock(&videodev_lock);
>>>
>>> -#if defined(CONFIG_MEDIA_CONTROLLER)
>>> - if (v4l2_dev->mdev && vdev->vfl_dir != VFL_DIR_M2M) {
>>> +#ifdef CONFIG_MEDIA_CONTROLLER
>>> + if (mdev && vdev->vfl_dir != VFL_DIR_M2M) {
>>> /* Remove interfaces and interface links */
>>> media_devnode_remove(vdev->intf_devnode);
>>> if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
>>> @@ -207,23 +212,31 @@ static void v4l2_device_release(struct device *cd)
>>> }
>>> #endif
>>>
>>> - /* Do not call v4l2_device_put if there is no release callback set.
>>> - * Drivers that have no v4l2_device release callback might free the
>>> - * v4l2_dev instance in the video_device release callback below, so we
>>> - * must perform this check here.
>>> - *
>>> - * TODO: In the long run all drivers that use v4l2_device should use the
>>> - * v4l2_device release callback. This check will then be unnecessary.
>>> - */
>>> - if (v4l2_dev->release == NULL)
>>> - v4l2_dev = NULL;
>>> -
>>> /* Release video_device and perform other
>>> cleanups as needed. */
>>> vdev->release(vdev);
>>>
>>> - /* Decrease v4l2_device refcount */
>>> - if (v4l2_dev)
>>> +#ifdef CONFIG_MEDIA_CONTROLLER
>>> + if (mdev)
>>> + media_device_put(mdev);
>>> +
>>> + /*
>>> + * Generally both struct media_device and struct v4l2_device are
>>> + * embedded in the same driver's context struct so having a release
>>> + * callback in both is a bug.
>>> + */
>>> + WARN_ON(v4l2_dev_has_release && mdev_has_release);
>>
>> How about:
>>
>> if (WARN_ON(v4l2_dev_has_release && mdev_has_release))
>> v4l2_dev_has_release = false;
>>
>>> +#endif
>>> +
>>> + /*
>>> + * Decrease v4l2_device refcount, but only if the media device doesn't
>>> + * have a release callback.
>>> + */
>>> + if (v4l2_dev_has_release
>>> +#ifdef CONFIG_MEDIA_CONTROLLER
>>> + && !mdev_has_release
>>> +#endif
>>> + )
>>
>> Then this change is no longer needed.
>
> Good idea.
>
> I'll also rename v4l2_dev_has_release as v4l2_dev_call_release.
>
>>
>> General question: do we have drivers today that set both release functions?
>> Because that would now cause a WARN in the kernel log with this patch.
>
> Indeed, the intention is to be vocal about it.
>
> The only user of the v4l2_device release function I could find is
> drivers/media/radio/dsbr100.c . I may have missed some but it certainly
> isn't commonly used. Maybe we could try to drop refcounting from
> v4l2_device later on?
There are a lot more drivers that use this. A quick grep shows gspca, hackrf,
usbtv, pwc, au0828 and more.
git grep v4l2_dev.*release.*= drivers/media/
Currently it is the only way to properly release drivers that create multiple
video (or other) devices.
Regards,
Hans
>
>>
>>> v4l2_device_put(v4l2_dev);
>>> }
>>>
>>> @@ -792,11 +805,14 @@ static int video_register_media_controller(struct video_device *vdev)
>>> u32 intf_type;
>>> int ret;
>>>
>>> - /* Memory-to-memory devices are more complex and use
>>> + /*
>>> + * Memory-to-memory devices are more complex and use
>>> * their own function to register its mc entities.
>>> */
>>> - if (!vdev->v4l2_dev->mdev || vdev->vfl_dir == VFL_DIR_M2M)
>>> + if (!vdev->v4l2_dev->mdev || vdev->vfl_dir == VFL_DIR_M2M) {
>>> + media_device_get(vdev->v4l2_dev->mdev);
>>> return 0;
>>> + }
>>>
>>> vdev->entity.obj_type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE;
>>> vdev->entity.function = MEDIA_ENT_F_UNKNOWN;
>>> @@ -875,6 +891,7 @@ static int video_register_media_controller(struct video_device *vdev)
>>>
>>> /* FIXME: how to create the other interface links? */
>>>
>>> + media_device_get(vdev->v4l2_dev->mdev);
>>> #endif
>>> return 0;
>>> }
>
