On 05/03/2024 8:43 am, Sakari Ailus wrote:
> Hi Hans,
>
> On Wed, Feb 21, 2024 at 11:44:59AM +0000, Sakari Ailus wrote:
>> Hi Hans,
>>
>> On Wed, Feb 21, 2024 at 11:51:08AM +0100, Hans Verkuil wrote:
>>> 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.
>>
>> I mistakenly grepped for ->release, .release is actually more common. I'll
>> check how this is currently being used.
>
> Getting back to the topic---indeed the V4L2 device release function is used
> by a number of drivers today. Moving to the Media device release function
> is no small task: I checked some drivers and while releasing the resources
> is centralised in this case, unregistering the interfaces and releasing
> actual resources may be intertwined so that fixing this requires reworking
> much of the driver code. It's better to leave this for driver authors or at
> least someone who has the hardware.
>
Indeed. Most if not all of these drivers do not create a media device anyway.
Now, if you see drivers that have a media device AND use the v4l2_device
release callback, then let me know. If I have the hardware, then I can try
and switch it to using the media device callback since that would make more
sense.
Regards,
Hans
