Hi Hans,
Thank you for the patch.
On Tue, Mar 05, 2019 at 10:58:44AM +0100, hverkuil-cisco@xxxxxxxxx wrote:
> From: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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
