On 3/5/19 8:52 PM, Laurent Pinchart wrote:
> Hi Hans,
>
> (CC'ing Sakari)
>
> Thank you for the patch.
>
> On Tue, Mar 05, 2019 at 10:58:45AM +0100, hverkuil-cisco@xxxxxxxxx wrote:
>> From: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> ---
>> 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 ?
I wouldn't be opposed to that.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Thanks!
Hans
>
>> - 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
>
