Re: [PATCH v2 17/29] media: v4l: Acquire a reference to the media device for every video device

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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@linux.intel.com>
> >>> ---
> >>>  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