Re: calling runtime PM from system PM methods

On Sunday, June 19, 2011, Alan Stern wrote:

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

quoted

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.

quoted

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

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