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

  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(-)
  

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.

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.

>  		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;
>  }

Regards,

	Hans
  

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.

Why is that ? The two are distinct objects, why can't they both have a
release function ?

> > 
> > 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.
> 
> 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.
> 
> >  		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 you fix the comment style as a drive-by change, you could as well
reflow it to 80 columns.

> >  	 */
> > -	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;
> >  }
  

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?

> 
> >  		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;
> >  }
  

Hi Laurent,

Thank you for reviewing the set!

On Wed, Feb 07, 2024 at 01:13:44PM +0200, Laurent Pinchart wrote:
> 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.
> 
> Why is that ? The two are distinct objects, why can't they both have a
> release function ?

You could, in principle, but in practice both of the structs are part of
the same driver's device context struct which is a single allocation. You
can only have a single release callback for it.

> 
> > > 
> > > 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.
> > 
> > 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.
> > 
> > >  		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 you fix the comment style as a drive-by change, you could as well
> reflow it to 80 columns.

I'll update this for the next version.

> 
> > >  	 */
> > > -	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;
> > >  }
>
  

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.

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;
>>>  }
>
  

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.
  

On Wed, Feb 21, 2024 at 10:43:47AM +0000, Sakari Ailus wrote:
> Hi Laurent,
> 
> Thank you for reviewing the set!
> 
> On Wed, Feb 07, 2024 at 01:13:44PM +0200, Laurent Pinchart wrote:
> > 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.
> > 
> > Why is that ? The two are distinct objects, why can't they both have a
> > release function ?
> 
> You could, in principle, but in practice both of the structs are part of
> the same driver's device context struct which is a single allocation. You
> can only have a single release callback for it.

If both release callbacks freed the same data structure, that would
indeed be a problem. There could be other use cases though. For
instance, in the uvcvideo driver, the top-level structure is
reference-counted, and the release callbacks of the video devices
decrement that reference count. I don't expect drivers to do something
similar with media_device and v4l2_device, but I'm not sure if we should
forbid it completely. If we do, I would then rather deprecate the
release callback of v4l2_device completely.

> > > > 
> > > > 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.
> > > 
> > > 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.
> > > 
> > > >  		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 you fix the comment style as a drive-by change, you could as well
> > reflow it to 80 columns.
> 
> I'll update this for the next version.
> 
> > > >  	 */
> > > > -	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;
> > > >  }
  

Hi Laurent,

On Wed, Feb 21, 2024 at 02:19:56PM +0200, Laurent Pinchart wrote:
> On Wed, Feb 21, 2024 at 10:43:47AM +0000, Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > Thank you for reviewing the set!
> > 
> > On Wed, Feb 07, 2024 at 01:13:44PM +0200, Laurent Pinchart wrote:
> > > 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.
> > > 
> > > Why is that ? The two are distinct objects, why can't they both have a
> > > release function ?
> > 
> > You could, in principle, but in practice both of the structs are part of
> > the same driver's device context struct which is a single allocation. You
> > can only have a single release callback for it.
> 
> If both release callbacks freed the same data structure, that would
> indeed be a problem. There could be other use cases though. For
> instance, in the uvcvideo driver, the top-level structure is
> reference-counted, and the release callbacks of the video devices
> decrement that reference count. I don't expect drivers to do something
> similar with media_device and v4l2_device, but I'm not sure if we should
> forbid it completely. If we do, I would then rather deprecate the
> release callback of v4l2_device completely.

There are quite a few drivers using the struct v4l2_device's release
callback, it seems. We very probably can't get rid of it except possibly in
the long term.

The documentation already takes this into account, see patch "media:
Documentation: Document how Media device resources are released".
  

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.
  

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

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