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,

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
> 
> 
> 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



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux