diff mbox

[linux-pm] calling runtime PM from system PM methods

Message ID 201106191604.11074.rjw@sisk.pl (mailing list archive)
State Not Applicable
Delegated to: Kevin Hilman
Headers show

Commit Message

Rafael Wysocki June 19, 2011, 2:04 p.m. UTC 
On Sunday, June 19, 2011, Alan Stern wrote:
> On Sun, 19 Jun 2011, Rafael J. Wysocki wrote:
> 
> > On Saturday, June 18, 2011, Rafael J. Wysocki wrote:
> > > On Saturday, June 18, 2011, Alan Stern wrote:
> > > > On Sat, 18 Jun 2011, Rafael J. Wysocki wrote:
> > ...
> > > 
> > > Well, assuming that https://patchwork.kernel.org/patch/893722/ is applied,
> > > which is going to be, I think we can put
> > > 
> > > +       pm_runtime_get_noresume(dev);
> > > +       pm_runtime_enable(dev);
> > > 
> > > in device_resume() after the dev->power.is_suspended check and
> > > pm_runtime_put_noidle() under the End label.  That cause them to
> > > be called under the device lock, but that shouldn't be a big deal.
> > > 
> > > Accordingly, we can call pm_runtime_disable(dev) in __device_suspend(),
> > > right next to the setting of power.is_suspended.
> > > 
> > > This is implemented by the patch below.
> > 
> > Well, it hangs suspend on my Toshiba test box, I'm not sure why exactly.
> > 
> > This happens even if the pm_runtime_disable() is replaced with a version
> > that only increments the disable depth, so it looks like something down
> > the road relies on disable_depth being zero.  Which is worrisome.
> 
> This is a sign that the PM subsystem is getting a little too 
> complicated.  :-(

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.

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.

So, the problem is either in scsi_autopm_get_host() that should check the
error code returned by pm_runtime_get_sync(), or in rpm_suspend() that should
return 0 if RPM_GET_PUT is set in flags.  I'm inclined to say that the
problem should be fixed in rpm_suspend() and hence the appended patch that
works (well, it probably should be split into three separate patches).

> > Trying to figure out what the problem is I noticed that, for example,
> > the generic PM operations use pm_runtime_suspended() to decide whether or
> > not to execute system suspend callbacks, so the patch below would break it.
> > 
> > Also, after commit e8665002477f0278f84f898145b1f141ba26ee26 the
> > pm_runtime_suspended() check in __pm_generic_call() doesn't really make
> > sense.
> 
> In light of the recent changes, we should revisit the decisions behind 
> the generic PM operations.

Certainly, although the situation is not as bad as I thought, because
__pm_generic_call() is executed after the patch below disables runtime PM
during system suspend.  Still, the pm_runtime_suspended() check in there is
pointless.

Thanks,
Rafael

---
 drivers/base/power/main.c    |    9 ++++++++-
 drivers/base/power/runtime.c |   41 ++++++++++++++++++++++++-----------------
 include/linux/pm_runtime.h   |   13 ++++++++++---
 3 files changed, 42 insertions(+), 21 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Alan Stern June 19, 2011, 3:01 p.m. UTC | #1 
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...

> So, the problem is either in scsi_autopm_get_host() that should check the
> error code returned by pm_runtime_get_sync(), or in rpm_suspend() that should
> return 0 if RPM_GET_PUT is set in flags.  I'm inclined to say that the
> problem should be fixed in rpm_suspend() and hence the appended patch that
> works (well, it probably should be split into three separate patches).

Maybe it would be good enough if the error handler ended up doing a 
get_noresume instead of get_sync?  Although I could be wrong, I don't 
think scsi_error_handler() will ever run in a situation where the host 
adapter is not runtime-active.

Tejun, does that sound right to you?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

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);
@@ -888,7 +892,10 @@  static int __device_suspend(struct devic
 	}
 
  End:
-	dev->power.is_suspended = !error;
+	if (!error) {
+		dev->power.is_suspended = true;
+		__pm_runtime_disable(dev, PRD_DEPTH);
+	}
 
  Unlock:
 	device_unlock(dev);
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
@@ -965,18 +967,23 @@  EXPORT_SYMBOL_GPL(pm_runtime_barrier);
 /**
  * __pm_runtime_disable - Disable run-time PM of a device.
  * @dev: Device to handle.
- * @check_resume: If set, check if there's a resume request for the device.
+ * @level: How much to do.
+ *
+ * Increment power.disable_depth for the device and if was zero previously.
+ *
+ * If @level is at least PRD_BARRIER, additionally cancel all pending run-time
+ * PM requests for the device and wait for all operations in progress to
+ * complete.
+ *
+ * If @level is at least PRD_CHECK_RESUME and there's a resume request pending
+ * when this function is called, and power.disable_depth is zero, the device
+ * will be woken up before disabling its run-time PM.
+ *
+ * The device can be either active or suspended after its run-time PM has been
+ * disabled.
  *
- * Increment power.disable_depth for the device and if was zero previously,
- * cancel all pending run-time PM requests for the device and wait for all
- * operations in progress to complete.  The device can be either active or
- * suspended after its run-time PM has been disabled.
- *
- * If @check_resume is set and there's a resume request pending when
- * __pm_runtime_disable() is called and power.disable_depth is zero, the
- * function will wake up the device before disabling its run-time PM.
  */
-void __pm_runtime_disable(struct device *dev, bool check_resume)
+void __pm_runtime_disable(struct device *dev, enum prd_level level)
 {
 	spin_lock_irq(&dev->power.lock);
 
@@ -990,7 +997,7 @@  void __pm_runtime_disable(struct device
 	 * means there probably is some I/O to process and disabling run-time PM
 	 * shouldn't prevent the device from processing the I/O.
 	 */
-	if (check_resume && dev->power.request_pending
+	if (level >= PRD_CHECK_RESUME && dev->power.request_pending
 	    && dev->power.request == RPM_REQ_RESUME) {
 		/*
 		 * Prevent suspends and idle notifications from being carried
@@ -1003,7 +1010,7 @@  void __pm_runtime_disable(struct device
 		pm_runtime_put_noidle(dev);
 	}
 
-	if (!dev->power.disable_depth++)
+	if (!dev->power.disable_depth++ && level >= PRD_BARRIER)
 		__pm_runtime_barrier(dev);
 
  out:
@@ -1230,7 +1237,7 @@  void pm_runtime_init(struct device *dev)
  */
 void pm_runtime_remove(struct device *dev)
 {
-	__pm_runtime_disable(dev, false);
+	__pm_runtime_disable(dev, PRD_BARRIER);
 
 	/* Change the status back to 'suspended' to match the initial status. */
 	if (dev->power.runtime_status == RPM_ACTIVE)
Index: linux-2.6/include/linux/pm_runtime.h
===================================================================
--- linux-2.6.orig/include/linux/pm_runtime.h
+++ linux-2.6/include/linux/pm_runtime.h
@@ -22,6 +22,13 @@ 
 					    usage_count */
 #define RPM_AUTO		0x08	/* Use autosuspend_delay */
 
+/* Runtime PM disable levels */
+enum prd_level {
+	PRD_DEPTH = 0,		/* Only increment disable depth */
+	PRD_BARRIER,		/* Additionally, act as a runtime PM barrier */
+	PRD_CHECK_RESUME,	/* Additionally, check if resume is pending */
+};
+
 #ifdef CONFIG_PM_RUNTIME
 
 extern struct workqueue_struct *pm_wq;
@@ -33,7 +40,7 @@  extern int pm_schedule_suspend(struct de
 extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
 extern int pm_runtime_barrier(struct device *dev);
 extern void pm_runtime_enable(struct device *dev);
-extern void __pm_runtime_disable(struct device *dev, bool check_resume);
+extern void __pm_runtime_disable(struct device *dev, enum prd_level level);
 extern void pm_runtime_allow(struct device *dev);
 extern void pm_runtime_forbid(struct device *dev);
 extern int pm_generic_runtime_idle(struct device *dev);
@@ -119,7 +126,7 @@  static inline int __pm_runtime_set_statu
 					    unsigned int status) { return 0; }
 static inline int pm_runtime_barrier(struct device *dev) { return 0; }
 static inline void pm_runtime_enable(struct device *dev) {}
-static inline void __pm_runtime_disable(struct device *dev, bool c) {}
+static inline void __pm_runtime_disable(struct device *dev, enum prd_level l) {}
 static inline void pm_runtime_allow(struct device *dev) {}
 static inline void pm_runtime_forbid(struct device *dev) {}
 
@@ -232,7 +239,7 @@  static inline void pm_runtime_set_suspen
 
 static inline void pm_runtime_disable(struct device *dev)
 {
-	__pm_runtime_disable(dev, true);
+	__pm_runtime_disable(dev, PRD_CHECK_RESUME);
 }
 
 static inline void pm_runtime_use_autosuspend(struct device *dev)