Re: [PATCH 4/4] v4l: async: add comment about re-probing to v4l2_async_notifier_unregister()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hans,

On Thu, Aug 24, 2017 at 09:59:41AM +0200, Hans Verkuil wrote:
> On 08/23/17 21:03, Niklas Söderlund wrote:
> > 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 <niklas.soderlund+renesas@xxxxxxxxxxxx>
> >>>>> ---
> >>>>>
> >>>>>  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

Re-probing never helped anything with omap3isp driver as the clock is
removed *after* unregistering async notifier. This means that the
re-probing sub-device driver will get the same clock which is about to be
removed and continues with that happily, only to find the clock gone in a
brief moment.

This could be fixed in the omap3isp driver but it is telling that _no-one
ever complained_.

> >>
> >>
> >> 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.
> 
> I prefer this. We can always remove the reprobe code later once we have
> a better understanding. I see no downside to this cleanup series and it
> doesn't block any future development.

One thing we could do is to remove the memory allocation there. After that
it couldn't fail anymore, leaving the device in an unknown state.

> 
> > - 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.
> 
> I'm OK with this as well, we missed the 4.14 merge window anyway.

Agreed.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@xxxxxx



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux