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.
--
Regards,
Sakari Ailus
