PCI / PM: Block races between runtime PM and system sleep

Commit Message

Rafael Wysocki June 20, 2011, 9:28 p.m. UTC

On Monday, June 20, 2011, Alan Stern wrote:
> On Mon, 20 Jun 2011, Rafael J. Wysocki wrote:
> 
> > > Furthermore, since we're going to disable runtime PM as soon as the
> > > suspend callback returns anyway, why not increment usage_count before
> > > invoking the callback?  This will prevent runtime suspends from 
> > > occurring while the callback runs, so no changes will be needed in the 
> > > PCI or USB subsystems.
> > 
> > The PCI case is different from the USB one.  PCI needs to resume devices
> > before calling their drivers' .suspend() callbacks, so it does that in
> > .prepare().  If the core acquired a reference to every device  before executing
> > the subsystem .suspend(), then pm_runtime_resume() could be moved from
> > pci_pm_prepare() to pci_prepare_suspend(), but then additionally it would
> > have to be called from pci_pm_freeze() and pci_pm_poweroff().  It simply is
> > more efficient to call it once from pci_pm_prepare(), but then PCI needs to
> > take the reference by itself.
> 
> Ah, okay.  The PCI part makes sense then.

OK, so the appended patch is a modification of the $subject one using
pm_runtime_put_sync() instead of pm_runtime_put_noidle().

> > > It also will prevent Kevin from calling pm_runtime_suspend from within
> > > his suspend callbacks, but you have already determined that subsystems
> > > and drivers should never do that in any case.
> > 
> > Then reverting commit e8665002477f0278f84f898145b1f141ba26ee26 would be
> > even better. :-)
> 
> See below.
> 
> 
> > > As I see it, we never want a suspend or suspend_noirq callback to call 
> > > pm_runtime_suspend().  However it's okay for the suspend callback to 
> > > invoke pm_runtime_resume(), as long as this is all done in subsystem 
> > > code.
> > 
> > First off, I don't really see a reason for a subsystem to call
> > pm_runtime_resume() from its .suspend_noirq() callback.
> 
> I was referring to .suspend(), not .suspend_noirq().
> 
> >  Now, if
> > pm_runtime_resume() is to be called concurrently with the subsystem's
> > .suspend_noirq() callback, I'd rather won't let that happen. :-)
> 
> Me too.  But I see no reason to prevent pm_runtime_resume() from being 
> called by .suspend().

Hmm.

> > > And in between the prepare and suspend callbacks, runtime PM should be
> > > more or less fully functional, right?  For most devices it will never
> > > be triggered, because it has to run in process context and both
> > > userspace and pm_wq are frozen.  It may trigger for devices marked as
> > > IRQ-safe, though.
> > 
> > It also may trigger for drivers using non-freezable workqueues and calling
> > runtime PM synchronously from there.
> 
> Right.  So we shouldn't ignore this window.

So, your point is that while .suspend() or .resume() are running, the
synchronization between runtime PM and system suspend/resume should be the
subsystem's problem, right?

> > > Maybe the barrier should be moved into __device_suspend().
> > 
> > I _really_ think that the initial approach, i.e. before commit
> > e8665002477f0278f84f898145b1f141ba26ee26, made the most sense.  It didn't
> > cover the "pm_runtime_resume() called during system suspend" case, but
> > it did cover everything else.
> 
> But it prevented runtime PM from working during the window between 
> .prepare() and .suspend(), and also between .resume() and .complete().
> If a subsystem like PCI wants to rule out runtime PM during those 
> windows, then fine -- it can do whatever it wants.  But the PM core 
> shouldn't do this.

I actually see a reason for doing this.  Namely, I don't really think
driver writers should be bothered with preventing races between different
PM callbacks from happening.  Runtime PM takes care of that at run time,
the design of the system suspend/resume code ensures that the callbacks
for the same device are executed sequentially, but if we allow runtime PM
callbacks to be executed in parallel with system suspend/resume callbacks,
someone has to prevent those callbacks from racing with each other.

Now, if you agree that that shouldn't be a driver's task, then it has to
be the subsystem's one and I'm not sure what a subsystem can do other than
disabling runtime PM or at least taking a reference on every device before
calling device drivers' .suspend() callbacks.

Please note, I think that .prepare() and .complete() are somewhat special,
so perhaps we should allow those to race with runtime PM callbacks, but IMO
allowing .suspend() and .resume() to race with .runtime_suspend() and
.runtime_resume() is not a good idea.

> > So, I think there are serious technical arguments for reverting that commit.
> > 
> > I think we went really far trying to avoid that, but I'm not sure I want to go
> > any further.
> 
> What I'm suggesting is to revert the commit but at the same time,
> move the get_noresume() into __device_suspend() and the put_sync() into 
> device_resume().

What about doing pm_runtime_get_noresume() and the pm_runtime_barrier()
in dpm_prepare(), but _after_ calling device_prepare() and doing
pm_runtime_put_noidle() in dpm_complete() _before_ calling .complete()
from the subsystem (a _put_sync() at this point will likely invoke
.runtime_idle() from the subsystem before executing .complete(), which may
not be desirable)?

Rafael

---
From: Rafael J. Wysocki <[e-mail]>
Subject: PCI / PM: Block races between runtime PM and system sleep

After commit e8665002477f0278f84f898145b1f141ba26ee26
(PM: Allow pm_runtime_suspend() to succeed during system suspend) it
is possible that a device resumed by the pm_runtime_resume(dev) in
pci_pm_prepare() will be suspended immediately from a work item,
timer function or otherwise, defeating the very purpose of calling
pm_runtime_resume(dev) from there.  To prevent that from happening
it is necessary to increment the runtime PM usage counter of the
device by replacing pm_runtime_resume() with pm_runtime_get_sync().
Moreover, the incremented runtime PM usage counter has to be
decremented by the corresponding pci_pm_complete(), via
pm_runtime_put_sync().

Signed-off-by: Rafael J. Wysocki <[e-mail]>
Cc: [e-mail]
---
 drivers/pci/pci-driver.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Alan Stern June 21, 2011, 2:52 p.m. UTC | #1

On Mon, 20 Jun 2011, Rafael J. Wysocki wrote:

> > Ah, okay.  The PCI part makes sense then.
> 
> OK, so the appended patch is a modification of the $subject one using
> pm_runtime_put_sync() instead of pm_runtime_put_noidle().

Yes, it looks good.


> So, your point is that while .suspend() or .resume() are running, the
> synchronization between runtime PM and system suspend/resume should be the
> subsystem's problem, right?

Almost but not quite.  I was talking about the time period between 
.prepare() and .suspend() (and also the time period between .resume() 
and .complete()).

It's probably okay to prevent pm_runtime_suspend() from working during
.suspend() or .resume(), but it's not a good idea to prevent
pm_runtime_resume() from working then.

> I actually see a reason for doing this.  Namely, I don't really think
> driver writers should be bothered with preventing races between different
> PM callbacks from happening.  Runtime PM takes care of that at run time,
> the design of the system suspend/resume code ensures that the callbacks
> for the same device are executed sequentially, but if we allow runtime PM
> callbacks to be executed in parallel with system suspend/resume callbacks,
> someone has to prevent those callbacks from racing with each other.
> 
> Now, if you agree that that shouldn't be a driver's task, then it has to
> be the subsystem's one and I'm not sure what a subsystem can do other than
> disabling runtime PM or at least taking a reference on every device before
> calling device drivers' .suspend() callbacks.
> 
> Please note, I think that .prepare() and .complete() are somewhat special,
> so perhaps we should allow those to race with runtime PM callbacks, but IMO
> allowing .suspend() and .resume() to race with .runtime_suspend() and
> .runtime_resume() is not a good idea.

Races in the period after .suspend() and before .resume() will be 
handled by disabling runtime PM when .suspend() returns and enabling it 
before calling .resume().

During the .suspend and .resume callbacks, races with
.runtime_suspend() can be prevented by calling
pm_runtime_get_noresume() just before .suspend() and then calling
pm_runtime_put_sync() just after .resume().

Races with .runtime_resume() can be handled to some extent by putting a
runtime barrier immediately after the pm_runtime_get_noresume() call,
but that's not a perfect solution.  Is it good enough?

> > What I'm suggesting is to revert the commit but at the same time,
> > move the get_noresume() into __device_suspend() and the put_sync() into 
> > device_resume().
> 
> What about doing pm_runtime_get_noresume() and the pm_runtime_barrier()
> in dpm_prepare(), but _after_ calling device_prepare() and doing
> pm_runtime_put_noidle() in dpm_complete() _before_ calling .complete()
> from the subsystem

This does not address the issue of allowing runtime suspends in the 
windows between .prepare() - .suspend() and .resume() - .complete().

>  (a _put_sync() at this point will likely invoke
> .runtime_idle() from the subsystem before executing .complete(), which may
> not be desirable)?

It should be allowed.  The purpose of .complete() is not to re-enable
runtime power management of the device; it is to release resources
(like memory) allocated during .prepare() and perhaps also to allow new
children to be registered under the device.

Alan Stern

Rafael Wysocki June 21, 2011, 11:49 p.m. UTC | #2

On Tuesday, June 21, 2011, Alan Stern wrote:
> On Mon, 20 Jun 2011, Rafael J. Wysocki wrote:
> 
> > > Ah, okay.  The PCI part makes sense then.
> > 
> > OK, so the appended patch is a modification of the $subject one using
> > pm_runtime_put_sync() instead of pm_runtime_put_noidle().
> 
> Yes, it looks good.

Cool, thanks!

> > So, your point is that while .suspend() or .resume() are running, the
> > synchronization between runtime PM and system suspend/resume should be the
> > subsystem's problem, right?
> 
> Almost but not quite.  I was talking about the time period between 
> .prepare() and .suspend() (and also the time period between .resume() 
> and .complete()).
> 
> It's probably okay to prevent pm_runtime_suspend() from working during
> .suspend() or .resume(), but it's not a good idea to prevent
> pm_runtime_resume() from working then.

OK, but taking a reference by means of pm_runtime_get_noresume() won't
block pm_runtime_resume().

> > I actually see a reason for doing this.  Namely, I don't really think
> > driver writers should be bothered with preventing races between different
> > PM callbacks from happening.  Runtime PM takes care of that at run time,
> > the design of the system suspend/resume code ensures that the callbacks
> > for the same device are executed sequentially, but if we allow runtime PM
> > callbacks to be executed in parallel with system suspend/resume callbacks,
> > someone has to prevent those callbacks from racing with each other.
> > 
> > Now, if you agree that that shouldn't be a driver's task, then it has to
> > be the subsystem's one and I'm not sure what a subsystem can do other than
> > disabling runtime PM or at least taking a reference on every device before
> > calling device drivers' .suspend() callbacks.
> > 
> > Please note, I think that .prepare() and .complete() are somewhat special,
> > so perhaps we should allow those to race with runtime PM callbacks, but IMO
> > allowing .suspend() and .resume() to race with .runtime_suspend() and
> > .runtime_resume() is not a good idea.
> 
> Races in the period after .suspend() and before .resume() will be 
> handled by disabling runtime PM when .suspend() returns and enabling it 
> before calling .resume().

OK

> During the .suspend and .resume callbacks, races with
> .runtime_suspend() can be prevented by calling
> pm_runtime_get_noresume() just before .suspend() and then calling
> pm_runtime_put_sync() just after .resume().

So, you seem to suggest to call pm_runtime_get_noresume() in
__device_suspend() and pm_runtime_put_sync() in device_resume().
That would be fine by me, perhaps up to the "sync" part of the "put".

> Races with .runtime_resume() can be handled to some extent by putting a
> runtime barrier immediately after the pm_runtime_get_noresume() call,
> but that's not a perfect solution.  Is it good enough?

It's not worse than what we had before, so I guess it should be enough.

> > > What I'm suggesting is to revert the commit but at the same time,
> > > move the get_noresume() into __device_suspend() and the put_sync() into 
> > > device_resume().
> > 
> > What about doing pm_runtime_get_noresume() and the pm_runtime_barrier()
> > in dpm_prepare(), but _after_ calling device_prepare() and doing
> > pm_runtime_put_noidle() in dpm_complete() _before_ calling .complete()
> > from the subsystem
> 
> This does not address the issue of allowing runtime suspends in the 
> windows between .prepare() - .suspend() and .resume() - .complete().

OK

> >  (a _put_sync() at this point will likely invoke
> > .runtime_idle() from the subsystem before executing .complete(), which may
> > not be desirable)?
> 
> It should be allowed.  The purpose of .complete() is not to re-enable
> runtime power management of the device; it is to release resources
> (like memory) allocated during .prepare() and perhaps also to allow new
> children to be registered under the device.

Right.  But does "allowed" mean the core _should_ do it at this point?
We may as well call pm_runtime_idle() directly from rpm_complete(), but
perhaps it's better to call it from device_resume(), so that it runs in
parallel for async devices.

Thanks,
Rafael

Alan Stern June 22, 2011, 2:20 p.m. UTC | #3

On Wed, 22 Jun 2011, Rafael J. Wysocki wrote:

> > It's probably okay to prevent pm_runtime_suspend() from working during
> > .suspend() or .resume(), but it's not a good idea to prevent
> > pm_runtime_resume() from working then.
> 
> OK, but taking a reference by means of pm_runtime_get_noresume() won't
> block pm_runtime_resume().

Exactly my point -- we don't need to (and don't want to) block 
pm_runtime_resume during the .suspend() and .resume() callbacks.

> > During the .suspend and .resume callbacks, races with
> > .runtime_suspend() can be prevented by calling
> > pm_runtime_get_noresume() just before .suspend() and then calling
> > pm_runtime_put_sync() just after .resume().
> 
> So, you seem to suggest to call pm_runtime_get_noresume() in
> __device_suspend() and pm_runtime_put_sync() in device_resume().

Yes.  Also perhaps call pm_runtime_barrier() immediately after 
get_noresume.

> That would be fine by me, perhaps up to the "sync" part of the "put".

The main feature of this design is that it allows runtime PM to work 
between .resume() and .complete().  If you do a put_noidle instead of 
put_sync then you may prevent runtime PM from working properly.

> > >  (a _put_sync() at this point will likely invoke
> > > .runtime_idle() from the subsystem before executing .complete(), which may
> > > not be desirable)?
> > 
> > It should be allowed.  The purpose of .complete() is not to re-enable
> > runtime power management of the device; it is to release resources
> > (like memory) allocated during .prepare() and perhaps also to allow new
> > children to be registered under the device.
> 
> Right.  But does "allowed" mean the core _should_ do it at this point?
> We may as well call pm_runtime_idle() directly from rpm_complete(), but
> perhaps it's better to call it from device_resume(), so that it runs in
> parallel for async devices.

Calling pm_runtime_put_noidle() followed by pm_runtime_idle() is 
essentially the same as calling pm_runtime_put_sync() anyway.

If a subsystem really does want to block runtime PM between the 
.resume() and .complete() callbacks, it can do its own get_noresume and 
put_sync -- just as you have done with PCI.

Alan Stern

Patch

Index: linux-2.6/drivers/pci/pci-driver.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci-driver.c
+++ linux-2.6/drivers/pci/pci-driver.c
@@ -624,7 +624,7 @@  static int pci_pm_prepare(struct device
 	 * system from the sleep state, we'll have to prevent it from signaling
 	 * wake-up.
 	 */
-	pm_runtime_resume(dev);
+	pm_runtime_get_sync(dev);
 
 	if (drv && drv->pm && drv->pm->prepare)
 		error = drv->pm->prepare(dev);
@@ -638,6 +638,8 @@  static void pci_pm_complete(struct devic
 
 	if (drv && drv->pm && drv->pm->complete)
 		drv->pm->complete(dev);
+
+	pm_runtime_put_sync(dev);
 }
 
 #else /* !CONFIG_PM_SLEEP */