On 12/08/2017 08:54 AM, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Friday, 8 December 2017 03:08:17 EET Niklas Söderlund wrote:
>> If the video device was registered by the complete() callback it should
>> be unregistered when the driver is removed.
>
> The .remove() operation indicates device removal, not driver removal (or, the
> be more precise, it indicates that the device is unbound from the driver). I'd
> update the commit message accordingly.
>
>> Protect from printing an uninitialized video device node name by adding a
>> check in rvin_v4l2_unregister() to identify that the video device is
>> registered.
>>
>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>> Reviewed-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> ---
>> drivers/media/platform/rcar-vin/rcar-core.c | 2 ++
>> drivers/media/platform/rcar-vin/rcar-v4l2.c | 3 +++
>> 2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
>> b/drivers/media/platform/rcar-vin/rcar-core.c index
>> f7a4c21909da6923..6d99542ec74b49a7 100644
>> --- a/drivers/media/platform/rcar-vin/rcar-core.c
>> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
>> @@ -272,6 +272,8 @@ static int rcar_vin_remove(struct platform_device *pdev)
>>
>> pm_runtime_disable(&pdev->dev);
>>
>> + rvin_v4l2_unregister(vin);
>
> Unless I'm mistaken, you're unregistering the video device both here and in
> the unbound() function. That's messy, but it's not really your fault, the V4L2
> core is very messy in the first place, and registering video devices in the
> complete() handler is a bad idea. As that can't be fixed for now,
>
> Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> Hans, I still would like to hear your opinion on how this should be solved.
> You've voiced a few weeks ago that register video devices at probe() time
> isn't a good idea but you've never explained how we should fix the problem. I
> still firmly believe that video devices should be registered at probe time,
> and we need to reach an agreement on a technical solution to this problem.
I have tentatively planned to look into this next week. What will very likely
have to happen is that we need to split off allocation from the registration,
just as is done in most other subsystems. Allocation can be done at probe time,
but the final registration step should likely be in the complete().
To what extent that will resolve this specific issue I don't know. It will take
me time to understand this in more detail.
Regards,
Hans
>
>> v4l2_async_notifier_unregister(&vin->notifier);
>> v4l2_async_notifier_cleanup(&vin->notifier);
>>
>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
>> 178aecc94962abe2..32a658214f48fa49 100644
>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>> @@ -841,6 +841,9 @@ static const struct v4l2_file_operations rvin_fops = {
>>
>> void rvin_v4l2_unregister(struct rvin_dev *vin)
>> {
>> + if (!video_is_registered(&vin->vdev))
>> + return;
>> +
>> v4l2_info(&vin->v4l2_dev, "Removing %s\n",
>> video_device_node_name(&vin->vdev));
>
