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@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(-) -- Regards, Laurent Pinchart
