calling runtime PM from system PM methods

Commit Message

Rafael Wysocki June 19, 2011, 7:36 p.m. UTC

On Sunday, June 19, 2011, Alan Stern wrote:
> On Sun, 19 Jun 2011, Rafael J. Wysocki wrote:
> 
> > Well, that was kind of difficult to debug, but not impossible. :-)
> > 
> > The problem here turns out to be related to the SCSI subsystem.
> > Namely, when the AHCI controller is suspended, it uses the SCSI error
> > handling mechanism for scheduling the suspend operation (I'm still at a little
> > loss why that is necessary, but Tejun says it is :-)).  This (after several
> > convoluted operations) causes scsi_error_handler() to be woken up and
> > it calls scsi_autopm_get_host(shost), which returns error code (-EAGAIN),
> > because the runtime PM has been disabled at the host level.
> 
> Oh no.  I was afraid something like this was going to happen 
> eventually.
> 
> It's clear that we don't want runtime PM kicking in while the SCSI 
> error handler is running.  That's why I added the 
> scsi_autopm_get_host().  But this also means we will run into trouble 
> if the error handler needs to be used during a power transition.
> 
> > This happens because scsi_autopm_get_host() uses
> > pm_runtime_get_sync(&shost->shost_gendev) and returns error code when
> > shost_gendev.power.disable_depth is nonzero.
> 
> Maybe get_sync doesn't need to return an error if the runtime status is 
> already ACTIVE.  I'm not sure about this; it's just an idea...

Well, if disable_depth > 0, ACTIVE isn't really well defined.  As I said,
though, I think it makes sense for pm_runtime_get_sync() to return 0 when
disable_depth > 0, because the grabbing of a reference is successful anyway and
the caller may assume that the device is accessible in that case.

In the meantime I rethought the __pm_runtime_disable() part of my previous
patch and I now think it's not necessary to complicate it any more.  Of course,
we need not check if runtime resume is pending in __device_suspend(), because
we've done it already in dpm_prepare(), but the barrier part should better be
done in there too.

Updated patch is appended.

Thanks,
Rafael

---
 drivers/base/power/main.c    |    6 ++++++
 drivers/base/power/runtime.c |   10 ++++++----
 2 files changed, 12 insertions(+), 4 deletions(-)

Comments

Alan Stern June 20, 2011, 2:39 p.m. UTC | #1

On Sun, 19 Jun 2011, Rafael J. Wysocki wrote:

> In the meantime I rethought the __pm_runtime_disable() part of my previous
> patch and I now think it's not necessary to complicate it any more.  Of course,
> we need not check if runtime resume is pending in __device_suspend(), because
> we've done it already in dpm_prepare(), but the barrier part should better be
> done in there too.

Does this really make sense?  What use is a barrier in dpm_prepare() if 
runtime PM is allowed to continue functioning up to the 
suspend callback?

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.

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.

Maybe the barrier should be moved into __device_suspend().

Alan Stern

Rafael Wysocki June 20, 2011, 7:53 p.m. UTC | #2

On Monday, June 20, 2011, Alan Stern wrote:
> On Sun, 19 Jun 2011, Rafael J. Wysocki wrote:
> 
> > In the meantime I rethought the __pm_runtime_disable() part of my previous
> > patch and I now think it's not necessary to complicate it any more.  Of course,
> > we need not check if runtime resume is pending in __device_suspend(), because
> > we've done it already in dpm_prepare(), but the barrier part should better be
> > done in there too.
> 
> Does this really make sense?  What use is a barrier in dpm_prepare() if 
> runtime PM is allowed to continue functioning up to the 
> suspend callback?

It checks if a resume request is pending and executes runtime resume in that
case.

> 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.  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. :-)

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

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

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.

Thanks,
Rafael

Patch

Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -521,6 +521,9 @@  static int device_resume(struct device *
 	if (!dev->power.is_suspended)
 		goto Unlock;
 
+	pm_runtime_get_noresume(dev);
+	pm_runtime_enable(dev);
+
 	if (dev->pwr_domain) {
 		pm_dev_dbg(dev, state, "power domain ");
 		error = pm_op(dev, &dev->pwr_domain->ops, state);
@@ -557,6 +560,7 @@  static int device_resume(struct device *
 
  End:
 	dev->power.is_suspended = false;
+	pm_runtime_put_noidle(dev);
 
  Unlock:
 	device_unlock(dev);
@@ -896,6 +900,8 @@  static int __device_suspend(struct devic
 
 	if (error)
 		async_error = error;
+	else if (dev->power.is_suspended)
+		__pm_runtime_disable(dev, false);
 
 	return error;
 }
Index: linux-2.6/drivers/base/power/runtime.c
===================================================================
--- linux-2.6.orig/drivers/base/power/runtime.c
+++ linux-2.6/drivers/base/power/runtime.c
@@ -455,12 +455,14 @@  static int rpm_resume(struct device *dev
 	dev_dbg(dev, "%s flags 0x%x\n", __func__, rpmflags);
 
  repeat:
-	if (dev->power.runtime_error)
+	if (dev->power.runtime_error) {
 		retval = -EINVAL;
-	else if (dev->power.disable_depth > 0)
-		retval = -EAGAIN;
-	if (retval)
 		goto out;
+	} else if (dev->power.disable_depth > 0) {
+		if (!(rpmflags & RPM_GET_PUT))
+			retval = -EAGAIN;
+		goto out;
+	}
 
 	/*
 	 * Other scheduled or pending requests need to be canceled.  Small