Re: [PATCHv2 7/9] v4l2-subdev: handle module refcounting here

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

(CC'ing Sakari)

Thank you for the patch.

On Tue, Mar 05, 2019 at 10:58:45AM +0100, hverkuil-cisco@xs4all.nl wrote:
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> The module ownership refcounting was done in media_entity_get/put,
> but that was very confusing and it did not work either in case an
> application had a v4l-subdevX device open and the module was
> unbound. When the v4l-subdevX device was closed the media_entity_put
> was never called and the module refcount was left one too high, making
> it impossible to unload it.
> 
> Since v4l2-subdev.c was the only place where media_entity_get/put was
> called, just move the functionality to v4l2-subdev.c and drop those
> confusing entity functions.

I wonder if we will later need to refcount media entities, but we can
reintroduce a different version of those two functions then, it doesn't
prevent their removal now.

Sakari, when working on lifetime management of objects in the media and
V4L2 core, did you come across a need to refcount entities ?

> Store the module in subdev_fh so module_put no longer depends on
> the media_entity struct.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/media-entity.c          | 28 ---------------------------
>  drivers/media/v4l2-core/v4l2-subdev.c | 22 +++++++++------------
>  include/media/media-entity.h          | 24 -----------------------
>  include/media/v4l2-subdev.h           |  1 +
>  4 files changed, 10 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 7b2a2cc95530..257f20d2fb8a 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -17,7 +17,6 @@
>   */
>  
>  #include <linux/bitmap.h>
> -#include <linux/module.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
>  #include <media/media-entity.h>
> @@ -588,33 +587,6 @@ void media_pipeline_stop(struct media_entity *entity)
>  }
>  EXPORT_SYMBOL_GPL(media_pipeline_stop);
>  
> -/* -----------------------------------------------------------------------------
> - * Module use count
> - */
> -
> -struct media_entity *media_entity_get(struct media_entity *entity)
> -{
> -	if (entity == NULL)
> -		return NULL;
> -
> -	if (entity->graph_obj.mdev->dev &&
> -	    !try_module_get(entity->graph_obj.mdev->dev->driver->owner))
> -		return NULL;
> -
> -	return entity;
> -}
> -EXPORT_SYMBOL_GPL(media_entity_get);
> -
> -void media_entity_put(struct media_entity *entity)
> -{
> -	if (entity == NULL)
> -		return;
> -
> -	if (entity->graph_obj.mdev->dev)
> -		module_put(entity->graph_obj.mdev->dev->driver->owner);
> -}
> -EXPORT_SYMBOL_GPL(media_entity_put);
> -
>  /* -----------------------------------------------------------------------------
>   * Links management
>   */
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index f5f0d71ec745..d75815ab0d7b 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -18,6 +18,7 @@
>  
>  #include <linux/ioctl.h>
>  #include <linux/mm.h>
> +#include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  #include <linux/videodev2.h>
> @@ -54,9 +55,6 @@ static int subdev_open(struct file *file)
>  	struct video_device *vdev = video_devdata(file);
>  	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
>  	struct v4l2_subdev_fh *subdev_fh;
> -#if defined(CONFIG_MEDIA_CONTROLLER)
> -	struct media_entity *entity = NULL;
> -#endif
>  	int ret;
>  
>  	subdev_fh = kzalloc(sizeof(*subdev_fh), GFP_KERNEL);
> @@ -73,12 +71,15 @@ static int subdev_open(struct file *file)
>  	v4l2_fh_add(&subdev_fh->vfh);
>  	file->private_data = &subdev_fh->vfh;
>  #if defined(CONFIG_MEDIA_CONTROLLER)

On a side note, do you think it's time to select MEDIA_CONTROLLER
unconditionally ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> -	if (sd->v4l2_dev->mdev) {
> -		entity = media_entity_get(&sd->entity);
> -		if (!entity) {
> +	if (sd->v4l2_dev->mdev && sd->entity.graph_obj.mdev->dev) {
> +		struct module *owner;
> +
> +		owner = sd->entity.graph_obj.mdev->dev->driver->owner;
> +		if (!try_module_get(owner)) {
>  			ret = -EBUSY;
>  			goto err;
>  		}
> +		subdev_fh->owner = owner;
>  	}
>  #endif
>  
> @@ -91,9 +92,7 @@ static int subdev_open(struct file *file)
>  	return 0;
>  
>  err:
> -#if defined(CONFIG_MEDIA_CONTROLLER)
> -	media_entity_put(entity);
> -#endif
> +	module_put(subdev_fh->owner);
>  	v4l2_fh_del(&subdev_fh->vfh);
>  	v4l2_fh_exit(&subdev_fh->vfh);
>  	subdev_fh_free(subdev_fh);
> @@ -111,10 +110,7 @@ static int subdev_close(struct file *file)
>  
>  	if (sd->internal_ops && sd->internal_ops->close)
>  		sd->internal_ops->close(sd, subdev_fh);
> -#if defined(CONFIG_MEDIA_CONTROLLER)
> -	if (sd->v4l2_dev->mdev)
> -		media_entity_put(&sd->entity);
> -#endif
> +	module_put(subdev_fh->owner);
>  	v4l2_fh_del(vfh);
>  	v4l2_fh_exit(vfh);
>  	subdev_fh_free(subdev_fh);
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index e5f6960d92f6..3cbad42e3693 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -864,19 +864,6 @@ struct media_link *media_entity_find_link(struct media_pad *source,
>   */
>  struct media_pad *media_entity_remote_pad(const struct media_pad *pad);
>  
> -/**
> - * media_entity_get - Get a reference to the parent module
> - *
> - * @entity: The entity
> - *
> - * Get a reference to the parent media device module.
> - *
> - * The function will return immediately if @entity is %NULL.
> - *
> - * Return: returns a pointer to the entity on success or %NULL on failure.
> - */
> -struct media_entity *media_entity_get(struct media_entity *entity);
> -
>  /**
>   * media_entity_get_fwnode_pad - Get pad number from fwnode
>   *
> @@ -916,17 +903,6 @@ __must_check int media_graph_walk_init(
>   */
>  void media_graph_walk_cleanup(struct media_graph *graph);
>  
> -/**
> - * media_entity_put - Release the reference to the parent module
> - *
> - * @entity: The entity
> - *
> - * Release the reference count acquired by media_entity_get().
> - *
> - * The function will return immediately if @entity is %NULL.
> - */
> -void media_entity_put(struct media_entity *entity);
> -
>  /**
>   * media_graph_walk_start - Start walking the media graph at a
>   *	given entity
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 2f2d1c8947e6..33ba56f3dc1f 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -905,6 +905,7 @@ struct v4l2_subdev {
>   */
>  struct v4l2_subdev_fh {
>  	struct v4l2_fh vfh;
> +	struct module *owner;
>  #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>  	struct v4l2_subdev_pad_config *pad;
>  #endif

-- 
Regards,

Laurent Pinchart