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

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Sakari Ailus <sakari.ailus@iki.fi>
Cc: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>,
	"Sakari Ailus" <sakari.ailus@linux.intel.com>,
	"Hans Verkuil" <hverkuil@xs4all.nl>,
	linux-media@vger.kernel.org,
	"Kieran Bingham" <kieran.bingham@ideasonboard.com>,
	linux-renesas-soc@vger.kernel.org,
	"Maxime Ripard" <maxime.ripard@free-electrons.com>,
	"Sylwester Nawrocki" <snawrocki@kernel.org>
Subject: Re: [PATCH 4/4] v4l: async: add comment about re-probing to v4l2_async_notifier_unregister()
Date: Fri, 18 Aug 2017 14:20:08 +0300	[thread overview]
Message-ID: <2865661.SePdnx8dyz@avalon> (raw)
In-Reply-To: <20170815160932.fizwqqkaivtz3nqu@valkosipuli.retiisi.org.uk>

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@ragnatech.se>
> > ---
> > 
> >  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 :-)

> Or can someone come up with a valid reason why the re-probing code should
> stay? :-)
> 
> > +	 *
> > +	 * 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