| Message ID | 20171208010842.20047-4-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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@ragnatech.se> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > 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@ideasonboard.com> 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. > 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));
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@ragnatech.se> >> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >> Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> >> --- >> 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@ideasonboard.com> > > 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)); >
Hi Hans, On Friday, 8 December 2017 10:46:34 EET Hans Verkuil wrote: > On 12/08/2017 08:54 AM, Laurent Pinchart wrote: > > 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@ragnatech.se> > >> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > >> Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> > >> --- > >> > >> 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@ideasonboard.com> > > > > 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. I believe that splitting initialization from registration is a good idea, but I still believe that video nodes should be registered at probe time nonetheless. Let's discuss it again after next week when you'll have had time to think about it. > >> 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));
Hi Laurent, Thanks for your comments. On 2017-12-08 09:54:31 +0200, 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. I'm not sure I fully understand this comment. My take is that .remove() indicates that the device is removed and not the driver itself, as the driver might be used by multiple devices and the .remove() function is therefor not an indication that the driver is being unloaded. So if I understood you correctly the following would be a better to go in the commit message: "If the video device was registered by the complete() callback it should be unregistered when a device is unbound from the driver." > > > 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@ragnatech.se> > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> > > --- > > 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@ideasonboard.com> Big thanks for this :-) > > 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. > > > 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)); > > -- > Regards, > > Laurent Pinchart >
Hi Niklas, On Friday, 8 December 2017 15:09:21 EET Niklas Söderlund wrote: > On 2017-12-08 09:54:31 +0200, Laurent Pinchart wrote: > > 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. > > I'm not sure I fully understand this comment. > > My take is that .remove() indicates that the device is removed and not > the driver itself, as the driver might be used by multiple devices and > the .remove() function is therefor not an indication that the driver is > being unloaded. > > So if I understood you correctly the following would be a better to go > in the commit message: > > "If the video device was registered by the complete() callback it should > be unregistered when a device is unbound from the driver." Perfect :-) > >> 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@ragnatech.se> > >> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > >> Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com> > >> --- > >> > >> 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@ideasonboard.com> > > Big thanks for this :-) > > > 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. > > > >> 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));
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); + 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));