Hi Niklas,
On Tuesday 14 Mar 2017 19:59:55 Niklas Söderlund wrote:
> Make use of the helper functions video_device_alloc() and
> video_device_release() to control the lifetime of the struct
> video_device.
It's nice to see you considering lifetime management issues, but this isn't
enough. The rvin_release() function accesses the rvin_dev structure, so you
need to keep this around until all references to the video device have been
dropped. This patch won't do so.
I would instead keep the video_device instance embedded in rvin_dev, and
implement a custom release handler that will kfree() the rvin_dev instance.
You will obviously need to replace devm_kzalloc() with kzalloc() to allocate
the rvin_dev.
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> ---
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 44 ++++++++++++++------------
> drivers/media/platform/rcar-vin/rcar-vin.h | 2 +-
> 2 files changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> be6f41bf82ac3bc5..c40f5bc3e3d26472 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -489,7 +489,7 @@ static int rvin_enum_input(struct file *file, void
> *priv, i->std = 0;
> } else {
> i->capabilities = V4L2_IN_CAP_STD;
> - i->std = vin->vdev.tvnorms;
> + i->std = vin->vdev->tvnorms;
> }
>
> strlcpy(i->name, "Camera", sizeof(i->name));
> @@ -752,8 +752,8 @@ static int rvin_initialize_device(struct file *file)
> if (ret < 0)
> return ret;
>
> - pm_runtime_enable(&vin->vdev.dev);
> - ret = pm_runtime_resume(&vin->vdev.dev);
> + pm_runtime_enable(&vin->vdev->dev);
> + ret = pm_runtime_resume(&vin->vdev->dev);
> if (ret < 0 && ret != -ENOSYS)
> goto eresume;
>
> @@ -771,7 +771,7 @@ static int rvin_initialize_device(struct file *file)
>
> return 0;
> esfmt:
> - pm_runtime_disable(&vin->vdev.dev);
> + pm_runtime_disable(&vin->vdev->dev);
> eresume:
> rvin_power_off(vin);
>
> @@ -823,8 +823,8 @@ static int rvin_release(struct file *file)
> * Then de-initialize hw module.
> */
> if (fh_singular) {
> - pm_runtime_suspend(&vin->vdev.dev);
> - pm_runtime_disable(&vin->vdev.dev);
> + pm_runtime_suspend(&vin->vdev->dev);
> + pm_runtime_disable(&vin->vdev->dev);
> rvin_power_off(vin);
> }
>
> @@ -846,13 +846,13 @@ static const struct v4l2_file_operations rvin_fops = {
> void rvin_v4l2_remove(struct rvin_dev *vin)
> {
> v4l2_info(&vin->v4l2_dev, "Removing %s\n",
> - video_device_node_name(&vin->vdev));
> + video_device_node_name(vin->vdev));
>
> /* Checks internaly if handlers have been init or not */
> v4l2_ctrl_handler_free(&vin->ctrl_handler);
>
> /* Checks internaly if vdev have been init or not */
> - video_unregister_device(&vin->vdev);
> + video_unregister_device(vin->vdev);
> }
>
> static void rvin_notify(struct v4l2_subdev *sd,
> @@ -863,7 +863,7 @@ static void rvin_notify(struct v4l2_subdev *sd,
>
> switch (notification) {
> case V4L2_DEVICE_NOTIFY_EVENT:
> - v4l2_event_queue(&vin->vdev, arg);
> + v4l2_event_queue(vin->vdev, arg);
> break;
> default:
> break;
> @@ -872,7 +872,7 @@ static void rvin_notify(struct v4l2_subdev *sd,
>
> int rvin_v4l2_probe(struct rvin_dev *vin)
> {
> - struct video_device *vdev = &vin->vdev;
> + struct video_device *vdev;
> struct v4l2_subdev *sd = vin_to_source(vin);
> int ret;
>
> @@ -880,16 +880,18 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
>
> vin->v4l2_dev.notify = rvin_notify;
>
> - ret = v4l2_subdev_call(sd, video, g_tvnorms, &vin->vdev.tvnorms);
> + vdev = video_device_alloc();
> +
> + ret = v4l2_subdev_call(sd, video, g_tvnorms, &vdev->tvnorms);
> if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> return ret;
>
> - if (vin->vdev.tvnorms == 0) {
> + if (vdev->tvnorms == 0) {
> /* Disable the STD API if there are no tvnorms defined */
> - v4l2_disable_ioctl(&vin->vdev, VIDIOC_G_STD);
> - v4l2_disable_ioctl(&vin->vdev, VIDIOC_S_STD);
> - v4l2_disable_ioctl(&vin->vdev, VIDIOC_QUERYSTD);
> - v4l2_disable_ioctl(&vin->vdev, VIDIOC_ENUMSTD);
> + v4l2_disable_ioctl(vdev, VIDIOC_G_STD);
> + v4l2_disable_ioctl(vdev, VIDIOC_S_STD);
> + v4l2_disable_ioctl(vdev, VIDIOC_QUERYSTD);
> + v4l2_disable_ioctl(vdev, VIDIOC_ENUMSTD);
> }
>
> /* Add the controls */
> @@ -913,7 +915,7 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
> vdev->v4l2_dev = &vin->v4l2_dev;
> vdev->queue = &vin->queue;
> strlcpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name));
> - vdev->release = video_device_release_empty;
> + vdev->release = video_device_release;
> vdev->ioctl_ops = &rvin_ioctl_ops;
> vdev->lock = &vin->lock;
> vdev->ctrl_handler = &vin->ctrl_handler;
> @@ -923,16 +925,18 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
> vin->format.pixelformat = RVIN_DEFAULT_FORMAT;
> rvin_reset_format(vin);
>
> - ret = video_register_device(&vin->vdev, VFL_TYPE_GRABBER, -1);
> + ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> if (ret) {
> vin_err(vin, "Failed to register video device\n");
> return ret;
> }
>
> - video_set_drvdata(&vin->vdev, vin);
> + video_set_drvdata(vdev, vin);
>
> v4l2_info(&vin->v4l2_dev, "Device registered as %s\n",
> - video_device_node_name(&vin->vdev));
> + video_device_node_name(vdev));
> +
> + vin->vdev = vdev;
>
> return ret;
> }
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> b/drivers/media/platform/rcar-vin/rcar-vin.h index
> 9bfb5a7c4dc4f215..9454ef80bc2b3961 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -122,7 +122,7 @@ struct rvin_dev {
> void __iomem *base;
> enum chip_id chip;
>
> - struct video_device vdev;
> + struct video_device *vdev;
> struct v4l2_device v4l2_dev;
> struct v4l2_ctrl_handler ctrl_handler;
> struct v4l2_async_notifier notifier;
--
Regards,
Laurent Pinchart
