Re: [PATCH 14/16] rcar-vin: make use of video_device_alloc() and video_device_release()

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

Hi Niklas,

On Saturday 20 May 2017 20:27:41 Niklas Söderlund wrote:
> On 2017-05-10 16:36:03 +0300, Laurent Pinchart wrote:
> > 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.

You would still access the vin structure, so you need to refcount that one 
anyway. Refcounting of video_device then comes for free if you embed it in the 
vin structure. It's actually simpler to embed the video_device instance than 
managing its lifetime separately.

> I think it's better to use the helper functions to manage the video
> device if its possible, do you agree with this?

Only when it makes sense :-) The video_device_release_empty() and 
video_device_release() functions were bad idea, they completely circumvent 
lifetime management. We need to go in the other direction in the V4L2 core.

> >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >> ---
> >> 
> >>  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(-)


-- 
Regards,

Laurent Pinchart