Re: [PATCHv2 6/9] v4l2-subdev: add release() internal op

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

Hi Hans,

Thank you for the patch.

On Tue, Mar 05, 2019 at 10:58:44AM +0100, hverkuil-cisco@xs4all.nl wrote:
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> If the subdevice created a device node, then the v4l2_subdev cannot
> be freed until the last user of the device node closes it.
> 
> This means that we need a release() callback in v4l2_subdev_internal_ops
> that is called from the video_device release function so the subdevice
> driver can postpone freeing memory until the that callback is called.
> 
> If no video device node was created then the release callback can
> be called immediately when the subdev is unregistered.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/v4l2-core/v4l2-device.c | 19 ++++++++++++++-----
>  include/media/v4l2-subdev.h           |  3 +++
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> index e0ddb9a52bd1..7cca0de1b730 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -216,10 +216,18 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_device_register_subdev);
>  
> +static void v4l2_subdev_release(struct v4l2_subdev *sd)
> +{
> +	struct module *owner = !sd->owner_v4l2_dev ? sd->owner : NULL;
> +
> +	if (sd->internal_ops && sd->internal_ops->release)
> +		sd->internal_ops->release(sd);
> +	module_put(owner);
> +}
> +
>  static void v4l2_device_release_subdev_node(struct video_device *vdev)
>  {
> -	struct v4l2_subdev *sd = video_get_drvdata(vdev);
> -	sd->devnode = NULL;
> +	v4l2_subdev_release(video_get_drvdata(vdev));
>  	kfree(vdev);
>  }
>  
> @@ -318,8 +326,9 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
>  		media_device_unregister_entity(&sd->entity);
>  	}
>  #endif
> -	video_unregister_device(sd->devnode);
> -	if (!sd->owner_v4l2_dev)
> -		module_put(sd->owner);
> +	if (sd->devnode)
> +		video_unregister_device(sd->devnode);
> +	else
> +		v4l2_subdev_release(sd);
>  }

Don't we also need to handle the error path in
v4l2_device_register_subdev_nodes() ?

>  EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 349e1c18cf48..2f2d1c8947e6 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -757,6 +757,8 @@ struct v4l2_subdev_ops {
>   *
>   * @close: called when the subdev device node is closed.
>   *
> + * @release: called when the subdev device node is released.
> + *

I think this should be expanded a bit. First of all, we should mention
what happens when the subdev doesn't have a device node, and then we
should also explain what drivers are supposed to do in this operation.

At what point do you think we should add a WARN_ON(!sd->internal_ops ||
!sd->internal_ops->release) ?

I expect we'll need to refcount the subdev structure, with the
video_device only having one of the multiple references to the subdev,
but that can be implemented later. Overall this moves us in the right
direction, thank you for your work.

>   * .. note::
>   *	Never call this from drivers, only the v4l2 framework can call
>   *	these ops.
> @@ -766,6 +768,7 @@ struct v4l2_subdev_internal_ops {
>  	void (*unregistered)(struct v4l2_subdev *sd);
>  	int (*open)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
>  	int (*close)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh);
> +	void (*release)(struct v4l2_subdev *sd);
>  };
>  
>  #define V4L2_SUBDEV_NAME_SIZE 32

-- 
Regards,

Laurent Pinchart