Message-ID: <20150911120345.GB16323@ulmo>
Date:	Fri, 11 Sep 2015 14:03:46 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:	Alan Stern <stern@...land.harvard.edu>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Grygorii Strashko <grygorii.strashko@...com>,
	Tomeu Vizoso <tomeu.vizoso@...labora.com>,
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] driver core: Ensure proper suspend/resume ordering

On Fri, Sep 11, 2015 at 12:08:02AM +0200, Rafael J. Wysocki wrote:
> On Thursday, September 10, 2015 12:19:03 PM Thierry Reding wrote:
> > From: Thierry Reding <treding@...dia.com>
> > 
> > Deferred probe can lead to strange situations where a device that is a
> > dependency for others will be moved to the end of the dpm_list. At the
> > same time the dependers may not be moved because at the time they will
> > be probed the dependee may already have been successfully reprobed and
> > they will not have to defer the probe themselves.
> 
> So there's a bug in the implementation of deferred probing IMO.

Well, yeah. The root problem here is that we don't have dependency
information and deferred probing is supposed to fix that. It does so
fairly well, but it breaks in this particular case.

> > One example where this happens is the Jetson TK1 board (Tegra124). The
> > gpio-keys driver exposes the power key of the board as an input device
> > that can also be used as a wakeup source. Commit 17cdddf0fb68 ("ARM:
> > tegra: Add gpio-ranges property") results in the gpio-tegra driver
> > deferring probe because one of its dependencies, the pinctrl-tegra
> > driver, has not successfully completed probing. Currently the deferred
> > probe code will move the corresponding gpio-tegra device to the end of
> > the dpm_list, but by the time the gpio-keys device, depending on the
> > gpio-tegra device, is probed, gpio-tegra has already been reprobed, so
> > the gpio-keys device is not moved to the end of dpm_list itself. As a
> > result, the suspend ordering becomes pinctrl-tegra -> gpio-keys ->
> > gpio-tegra. That's problematic because the gpio-keys driver requests
> > the power key to be a wakeup source. However, the programming of the
> > wakeup interrupt registers happens in the gpio-tegra driver's suspend
> > callback, which is now called before that of the gpio-keys driver. The
> > result is that the wrong values are programmed and leaves the system
> > unable to be resumed using the power key.
> > 
> > To fix this situation, always move devices to the end of the dpm_list
> > before probing them. Technically this should only be done for devices
> > that have been successfully probed, but that won't work for recursive
> > probing of devices (think an I2C master that instantiates children in
> > its ->probe()). Effectively the dpm_list will end up ordered the same
> > way that devices were probed, hence taking care of dependencies.
> > 
> > Signed-off-by: Thierry Reding <treding@...dia.com>
> > ---
> > Note that this commit is kind of the PM equivalent of 52cdbdd49853
> > ("driver core: correct device's shutdown order) and that we have two
> > lists that are essentially the same (dpm_list and devices_kset). I'm
> > wondering if it would be worth looking into getting rid of one of
> > them? I don't see any reason why the ordering for shutdown and
> > suspend/resume should be different, and having a single list would
> > help keep this in sync.
> 
> We move away things from dpm_list during suspend and add them back to it
> during resume to handle the situations in which some devices go away or
> appear during suspend/resume.  That makes this idea potentially problematic.

Okay, I see. If they are used for different purposes it's fine to keep
them both.

> >  drivers/base/dd.c | 33 +++++++++++++++++++++++----------
> >  1 file changed, 23 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index be0eb4639128..56291b11049b 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -88,16 +88,6 @@ static void deferred_probe_work_func(struct work_struct *work)
> >  		 */
> >  		mutex_unlock(&deferred_probe_mutex);
> >  
> > -		/*
> > -		 * Force the device to the end of the dpm_list since
> > -		 * the PM code assumes that the order we add things to
> > -		 * the list is a good order for suspend but deferred
> > -		 * probe makes that very unsafe.
> > -		 */
> > -		device_pm_lock();
> > -		device_pm_move_last(dev);
> > -		device_pm_unlock();
> > -
> >  		dev_dbg(dev, "Retrying from deferred list\n");
> >  		bus_probe_device(dev);
> >  
> > @@ -312,6 +302,29 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> >  	 */
> >  	devices_kset_move_last(dev);
> >  
> > +	/*
> > +	 * Force the device to the end of the dpm_list since the PM code
> > +	 * assumes that the order we add things to the list is a good order
> > +	 * for suspend but deferred probe makes that very unsafe.
> > +	 *
> > +	 * Deferred probe can also cause situations in which a device that is
> > +	 * a dependency for others gets moved further down the dpm_list as a
> > +	 * result of probe deferral. In that case the dependee will end up
> > +	 * getting suspended before any of its dependers.
> > +	 *
> > +	 * To ensure proper ordering of suspend/resume, move every device that
> > +	 * is being probed to the end of the dpm_list. Note that technically
> > +	 * only successfully probed devices need to be moved, but that breaks
> > +	 * for recursively added devices because they would end up in the list
> > +	 * in reverse of the desired order, so we simply do it unconditionally
> > +	 * for all devices before they are being probed. In the worst case the
> > +	 * list will be reordered a couple more times than necessary, which
> > +	 * should be an insignificant amount of work.
> > +	 */
> > +	device_pm_lock();
> > +	device_pm_move_last(dev);
> > +	device_pm_unlock();
> 
> So I don't agree with doing that for every driver being probed against the
> same device.  That's just wasteful IMO.

I don't understand. At this point driver matching has already taken
place, so this will only every happen for one particular driver and the
corresponding device. In the most common case this will happen exactly
once, when the device is probed. Worst case it will happen a second or
more times if a driver defers probing for a specific device. But in that
case moving the device to the end of the list is absolutely required to
keep it properly ordered for suspend/resume. Note that this is already
done by the original code in deferred_probe_work_func() that is removed
in the first hunk. The fix here is to do it for every device to ensure
that inter-device dependencies are properly dealt with.

I agree that it's a little wasteful, but that's completely in line with
deferred probe. It's a simple solution to a very difficult problem, so
naturally it comes at a cost. But it's also fairly elegant in how it
correctly solves the ordering problem with very little code. The only
other way that I can think of to avoid reordering the list for every
device probe would be to sort it in advance using dependency information
which we don't have. So we'd need to first add all that dependency
information, and using that information is likely to be more work than
simply reordering the list for each probe.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists