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: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@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.

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.

-- 
Sakari Ailus