Hi,
On 2017-08-18 15:42:37 +0200, Niklas Söderlund wrote:
> Hi Sakari and Laurent,
>
> Thanks for your feedback.
>
> On 2017-08-18 14:20:08 +0300, Laurent Pinchart wrote:
> > Hello,
> >
> > On Tuesday 15 Aug 2017 19:09:33 Sakari Ailus wrote:
> > > On Mon, Jul 31, 2017 at 12:31:58AM +0200, Niklas Söderlund wrote:
> > > > The re-probing of subdevices when unregistering a notifier is tricky to
> > > > understand, and implemented somewhat as a hack. Add a comment trying to
> > > > explain why the re-probing is needed in the first place and why existing
> > > > helper functions can't be used in this situation.
> > > >
> > > > Signed-off-by: Niklas Söderlund <[email protected]>
> > > > ---
> > > >
> > > > drivers/media/v4l2-core/v4l2-async.c | 17 +++++++++++++++++
> > > > 1 file changed, 17 insertions(+)
> > > >
> > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > > > b/drivers/media/v4l2-core/v4l2-async.c index
> > > > d91ff0a33fd3eaff..a3c5a1f6d4d2ab03 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > > @@ -234,6 +234,23 @@ void v4l2_async_notifier_unregister(struct
> > > > v4l2_async_notifier *notifier)>
> > > > mutex_unlock(&list_lock);
> > > >
> > > > + /*
> > > > + * Try to re-probe the subdevices which where part of the
> > > > notifier.
> > > > + * This is done so subdevices which where part of the notifier
> > > > will
> > > > + * be re-probed to a pristine state and put back on the global
> > > > + * list of subdevices so they can once more be found and
> > > > associated
> > > > + * with a new notifier.
> > >
> > > Instead of tweaking the code trying to handle unhandleable error
> > > conditions
> > > in notifier unregistration and adding lengthy stories on why this is done
> > > the way it is, could we simply get rid of the driver re-probing?
> > >
> > > I can't see why drivers shouldn't simply cope with the current interfaces
> > > without re-probing to which I've never seen any reasoned cause. When a
> > > sub-device driver is unbound, simply return the sub-device node to the
> > > list
> > > of async sub-devices.
> >
> > I agree, this is a hack that we should get rid of. Reprobing has been there
> > from the very beginning, it's now 4 years and a half old, let's allow it to
> > retire :-)
>
> I would also be happy to see this code go away :-)
>
> >
> > > Or can someone come up with a valid reason why the re-probing code should
> > > stay? :-)
>
> Hans kindly dug out the original reason talking about why this code was
> added in the first place at
>
> http://lkml.iu.edu/hypermail/linux/kernel/1210.2/00713.html
>
> I would also like record here what Laurent stated about this after
> reading the above on #v4l
>
> 13:53 pinchartl : what could happen is this
> 13:53 pinchartl : the master could export resources used by the subdev
> 13:53 pinchartl : the omap3 isp driver, for instance, is a clock source
> 13:54 pinchartl : and the clock can be used by sensors
> 13:54 pinchartl : so if you remove the omap3 isp, the clock won't be
> there anymore
> 13:54 pinchartl : and that's bad for the subdev
>
>
> I don't claim I fully understand all the consequences of removing this
> reprobing now. But maybe it's safer to lave the current behavior in for
> now until the full problem is understood and move forward whit these
> patches since at least they document the behavior and removes another
> funky bit when trying to handle the situation where the memory
> allocation fails? What do you guys think?
Any thoughts about how I can move forward with this? The reason I'm
asking is that this is a dependency for the sub-notifier patches which
in turn is dependency for the R-Car CSI-2 driver :-) If someone wants to
think more about this that is fine I just don't want it to be forgotten.
As I see it these are the options open to me, but as always I'm always
open to other solutions which I'm to narrow minded to see :-)
- If after the latest discussions it feels the safest option is to keep
the re-probe logic but separating the v4l2 housekeeping from re-probe
logic move forward with this series as-is.
- Post 1/4 separately and repost patch 2/4 -- 4/4 in a v2 to allow for
more input on what is the right thing to do here.
- Post 1/4 separately, drop patch 2/4 -- 4/4 and create a new patch
which removes all re-probe related code and post that as a new patch.
I would feel a but uneasy about this without a consensus from all you
guys since I don't understand all the ramifications in doing so.
- Post 1/4 separately, drop patch 2/4 -- 4/4 and try to rework the
sub-notifier code to work the intertwined v4l2 and re-probe portions
of the code.
>
> > >
> > > > + *
> > > > + * One might be tempted to use device_reprobe() to handle the
> > > > re-
> > > > + * probing. Unfortunately this is not possible since some video
> > > > + * device drivers call v4l2_async_notifier_unregister() from
> > > > + * there remove function leading to a dead lock situation on
> > > > + * device_lock(dev->parent). This lock is held when video device
> > > > + * drivers remove function is called and device_reprobe() also
> > > > + * tries to take the same lock, so using it here could lead to a
> > > > + * dead lock situation.
> > > > + */
> > > > +
> > > > for (i = 0; i < count; i++) {
> > > >
> > > > /* If we handled USB devices, we'd have to lock the
> > > > parent too
> > */
> > > > device_release_driver(dev[i]);
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
>
> --
> Regards,
> Niklas Söderlund
--
Regards,
Niklas Söderlund
