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(-)
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
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
