Hi Laurent,
Thanks for your feedback.
On 2017-05-10 16:36:03 +0300, Laurent Pinchart wrote:
> 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 see your point, and it's a good catch I missed!
>
> 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.
Would it not be simpler to remove the usage of the video device from
rvin_release()? When I check the code the only usage of vin->vdev in
paths from the rvin_release() is in relation to pm_runtime_* calls like:
pm_runtime_suspend(&vin->vdev->dev);
pm_runtime_disable(&vin->vdev->dev);
And those can just as easily (and probably should) be called like:
pm_runtime_suspend(&vin->dev);
pm_runtime_disable(&vin->dev);
I had plan to fix the usage of the PM calls at a later time when also
addressing suspend/resume for this driver, but cleaning up the PM calls
can just as easily be done now.
I think it's better to use the helper functions to manage the video
device if its possible, do you agree with this?
>
> > 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
>
--
Regards,
Niklas Söderlund
