[Replacement][PATCH 2/6] PM / Domains: Fix hibernation restore of devices, v2

[Rafael J. Wysocki]

On Tuesday, March 13, 2012, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@xxxxxxx>
> 
> During resume from hibernation pm_genpd_restore_noirq() has to
> deal with software state left by pm_genpd_suspend_noirq() and
> unknown hardware state (the boot kernel may leave all PM domains and
> devices in arbitrary states).  For this reason, make it attempt to
> power cycle each domain when before resuming its first device to
> possibly get rid of any unwanted hardware state that may interfere
> with genpd_start_dev() later on.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@xxxxxxx>

I realized that this wasn't a good idea because of patch [3/6] (we
can't power cycle domains containing "always on" devices), so I
decided to only fix the really broken things in pm_genpd_restore_noirq().

Of course, patch [3/6] also needs to be updated on top of the below to
avoid starting "always on" devices in pm_genpd_restore_noirq() (it has
to assume that they will be "always on" in the boot kernel too, but that
seems to be a reasonable expectation).

Please note that those changes only affect resume from hibernation, so
they don't invalidate the testing that has already been carried out.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@xxxxxxx>
Subject: PM / Domains: Fix hibernation restore of devices, v2

During resume from hibernation pm_genpd_restore_noirq() should only
power off domains whose suspend_power_off flags are set once and
not every time it is called for a device in the given domain.
Moreover, it shouldn't decrement genpd->suspended_count, because
that field is not touched during device freezing and therefore it is
always equal to 0 when pm_genpd_restore_noirq() runs for the first
device in the given domain.

This means pm_genpd_restore_noirq() may use genpd->suspended_count
to determine whether or not it it has been called for the domain in
question already in this cycle (it only needs to increment that
field every time it runs for this purpose) and whether or not it
should check if the domain needs to be powered off.  For that to
work, though, pm_genpd_prepare() has to clear genpd->suspended_count
when it runs for the first device in the given domain (in which case
that flag need not be cleared during domain initialization).

Signed-off-by: Rafael J. Wysocki <rjw@xxxxxxx>
---
 drivers/base/power/domain.c |   31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

Index: linux/drivers/base/power/domain.c
===================================================================
--- linux.orig/drivers/base/power/domain.c
+++ linux/drivers/base/power/domain.c
@@ -764,8 +764,10 @@ static int pm_genpd_prepare(struct devic
 
 	genpd_acquire_lock(genpd);
 
-	if (genpd->prepared_count++ == 0)
+	if (genpd->prepared_count++ == 0) {
+		genpd->suspended_count = 0;
 		genpd->suspend_power_off = genpd->status == GPD_STATE_POWER_OFF;
+	}
 
 	genpd_release_lock(genpd);
 
@@ -1097,20 +1099,30 @@ static int pm_genpd_restore_noirq(struct
 	 * Since all of the "noirq" callbacks are executed sequentially, it is
 	 * guaranteed that this function will never run twice in parallel for
 	 * the same PM domain, so it is not necessary to use locking here.
+	 *
+	 * At this point suspended_count == 0 means we are being run for the
+	 * first time for the given domain in the present cycle.
 	 */
-	genpd->status = GPD_STATE_POWER_OFF;
-	if (genpd->suspend_power_off) {
+	if (genpd->suspended_count++ == 0) {
 		/*
-		 * The boot kernel might put the domain into the power on state,
-		 * so make sure it really is powered off.
+		 * The boot kernel might put the domain into arbitrary state,
+		 * so make it appear as powered off to pm_genpd_poweron(), so
+		 * that it tries to power it on in case it was really off.
 		 */
-		if (genpd->power_off)
-			genpd->power_off(genpd);
-		return 0;
+		genpd->status = GPD_STATE_POWER_OFF;
+		if (genpd->suspend_power_off) {
+			/*
+			 * If the domain was off before the hibernation, make
+			 * sure it will be off going forward.
+			 */
+			if (genpd->power_off)
+				genpd->power_off(genpd);
+
+			return 0;
+		}
 	}
 
 	pm_genpd_poweron(genpd);
-	genpd->suspended_count--;
 
 	return genpd_start_dev(genpd, dev);
 }
@@ -1617,7 +1629,6 @@ void pm_genpd_init(struct generic_pm_dom
 	genpd->poweroff_task = NULL;
 	genpd->resume_count = 0;
 	genpd->device_count = 0;
-	genpd->suspended_count = 0;
 	genpd->max_off_time_ns = -1;
 	genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend;
 	genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/