[PATCH -mm 0/3] PM: Improve ACPI handling during suspend and hibernation (rev. 3)

[PATCH -mm 0/3] PM: Improve ACPI handling during suspend and hibernation (rev. 3)

[Rafael J. Wysocki]

Hi,

The patches in this series are intended to improve the handling of the ACPI
platform during suspend and hibernation.

They do the following things:
* make hibernation_platform_enter() consistent with the sleep state entering
  code in kernel/power/main.c
* introduce global platform callbacks allowing us to handle ACPI in a more fine
  grained way during suspend and hibernation
* improve the handling of the system status indicator during suspend and
  hibernation.

Greetings,
Rafael

[PATCH -mm 1/3] Hibernation: Enter platform hibernation state in a consistent way (rev. 3)

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Make hibernation_platform_enter() execute the enter-a-sleep-state sequence
instead of the mixed shutdown-with-entering-S4 thing.

Replace the shutting down of devices done by kernel_shutdown_prepare(), before
entering the ACPI S4 sleep state, with suspending them and the shutting down of
sysdevs with calling device_power_down(PMSG_SUSPEND) (just like before entering
S1 or S3, but the target state is now S4).  Also, disable the nonboot CPUs
before entering the sleep state (S4), which generally always is a good idea.

This is known to fix the "double disk spin down during hibernation" on some
machines, eg. HPC nx6325 (ref. http://lkml.org/lkml/2007/8/7/316 and the
following thread).  It also generally causes the hibernation state (ACPI S4) to
be entered faster.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 kernel/power/disk.c |   55 ++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 14 deletions(-)

Index: linux-2.6.23-rc3/kernel/power/disk.c
===================================================================
--- linux-2.6.23-rc3.orig/kernel/power/disk.c
+++ linux-2.6.23-rc3/kernel/power/disk.c
@@ -222,21 +222,48 @@ int hibernation_platform_enter(void)
 {
 	int error;
 
-	if (hibernation_ops) {
-		kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
-		/*
-		 * We have cancelled the power transition by running
-		 * hibernation_ops->finish() before saving the image, so we
-		 * should let the firmware know that we're going to enter the
-		 * sleep state after all
-		 */
-		error = hibernation_ops->prepare();
-		sysdev_shutdown();
-		if (!error)
-			error = hibernation_ops->enter();
-	} else {
-		error = -ENOSYS;
+	if (!hibernation_ops)
+		return -ENOSYS;
+
+	/*
+	 * We have cancelled the power transition by running
+	 * hibernation_ops->finish() before saving the image, so we should let
+	 * the firmware know that we're going to enter the sleep state after all
+	 */
+	error = hibernation_ops->start();
+	if (error)
+		return error;
+
+	suspend_console();
+	error = device_suspend(PMSG_SUSPEND);
+	if (error)
+		return error;
+
+	error = hibernation_ops->prepare();
+	if (error)
+		goto Resume_devices;
+
+	error = disable_nonboot_cpus();
+	if (error)
+		goto Finish;
+
+	local_irq_disable();
+	error = device_power_down(PMSG_SUSPEND);
+	if (!error) {
+		hibernation_ops->enter();
+		/* We should never get here */
+		while (1);
 	}
+	local_irq_enable();
+
+	/*
+	 * We don't need to reenable the nonboot CPUs or resume consoles, since
+	 * the system is going to be halted anyway.
+	 */
+ Finish:
+	hibernation_ops->finish();
+ Resume_devices:
+	device_resume();
 	return error;
 }
 

[PATCH -mm 2/3] PM: More fine grained ACPI handling during suspend and hibernation (rev. 3)

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

According to the ACPI specification (eg. ACPI 2.0c, sec. 7.3.1, 7.3.3,
ACPI 3.0b, sec. 7.3.1, 7.3.3) the _GTS and _BFS global control methods should
be executed, respectively, right before entering a sleep state (S1-S4) and right
after leaving it, but we don't follow this reqirement.  Namely, in our
implementation the nonboot CPUs are disabled after executing _GTS and enabled
before executing _BFS, which doesn't seem to be correct.  [In fact, the ACPI
specification requires that no physical I/O and interrupt servicing be performed
after the sleep state has been left and before _BFS is executed as well as after
executing _GTS and before the sleep state is entered, but we can't follow this
requirement literally, since our AML interpreter needs to run with interrupts
enabled and we need to carry out some operations with interrupts disabled before
entering the sleep state and after leaving it.]  Moreover, acpi_enable() called
after restoring the system memory state from a hibernation image should really
be executed before enabling the nonboot CPUs, since functional ACPI may be
needed for that.  All of this means that we need to handle ACPI in a more fine
grained manner during suspend and hibernation.

For this reason we can introduce new global platform callbacks prepare_late()
and finish_early() to be executed, respectively, between disabling the nonboot
CPUs and entering a sleep state and between leaving the sleep state and enabling
the nonboot CPUs.  For ACPI systems they can be used to execute the _GTS and
_BFS global control methods, respectively.

It also seems to be a good idea to introduce new hibernation-related callback
post_snapshot() to be executed after creating a hibernation image, instead of
finish() (for now, both these callbacks are defined to point to the same
function, but that will be changed in the future).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/acpi/hardware/hwsleep.c |   98 ++++++++++++++++++++++++++++++++--------
 drivers/acpi/sleep/main.c       |   57 +++++++++++++++++++++--
 include/acpi/acpixf.h           |    4 +
 include/linux/suspend.h         |   57 +++++++++++++++++++----
 kernel/power/disk.c             |   50 +++++++++++++++++---
 kernel/power/main.c             |   16 +++++-
 6 files changed, 241 insertions(+), 41 deletions(-)

Index: linux-2.6.23-rc3/include/linux/suspend.h
===================================================================
--- linux-2.6.23-rc3.orig/include/linux/suspend.h
+++ linux-2.6.23-rc3/include/linux/suspend.h
@@ -49,7 +49,7 @@ typedef int __bitwise suspend_state_t;
  *	passed to @enter() is meaningless and should be ignored.
  *
  * @prepare: Prepare the platform for entering the system sleep state indicated
- *	by @set_target().
+ *	by @set_target() (first stage).
  *	@prepare() is called right after devices have been suspended (ie. the
  *	appropriate .suspend() method has been executed for each device) and
  *	before the nonboot CPUs are disabled (it is executed with IRQs enabled).
@@ -57,12 +57,27 @@ typedef int __bitwise suspend_state_t;
  *	error code otherwise, in which case the system cannot enter the desired
  *	sleep state (@enter() and @finish() will not be called in that case).
  *
+ * @prepare_late: Prepare the platform for entering the system sleep state
+ *	indicated by @set_target() (second stage).
+ *	@prepare_late() is called right after the nonboot CPUs have been
+ *	disabled (it is executed with on CPU on line and with IRQs enabled).
+ *	This callback is optional.  It returns 0 on success or a negative
+ *	error code otherwise, in which case the system cannot enter the desired
+ *	sleep state (@enter() will not be called in that case).
+ *
  * @enter: Enter the system sleep state indicated by @set_target() or
  *	represented by the argument if @set_target() is not implemented.
  *	This callback is mandatory.  It returns 0 on success or a negative
  *	error code otherwise, in which case the system cannot enter the desired
  *	sleep state.
  *
+ * @finish_early: Called when the system has just left a sleep state, right
+ *	prior to enabling the nonboot CPUs (it is executed with one CPU on line
+ *	and with IRQs enabled).
+ *	This callback is optional, but should be implemented by the platforms
+ *	that implement @prepare_late().  If @enter() fails, this callback will
+ *	not be executed.
+ *
  * @finish: Called when the system has just left a sleep state, right after
  *	the nonboot CPUs have been enabled and before devices are resumed (it is
  *	executed with IRQs enabled).
@@ -74,7 +89,9 @@ struct platform_suspend_ops {
 	int (*valid)(suspend_state_t state);
 	int (*set_target)(suspend_state_t state);
 	int (*prepare)(void);
+	int (*prepare_late)(void);
 	int (*enter)(suspend_state_t state);
+	void (*finish_early)(void);
 	void (*finish)(void);
 };
 
@@ -141,15 +158,32 @@ extern void mark_free_pages(struct zone 
  *	Called right after devices have been frozen and before the nonboot
  *	CPUs are disabled (runs with IRQs on).
  *
- * @finish: Restore the previous state of the platform after the hibernation
- *	image has been created *or* put the platform into the normal operation
- *	mode after the hibernation (the same method is executed in both cases).
- *	Called right after the nonboot CPUs have been enabled and before
- *	thawing devices (runs with IRQs on).
- *
- * @prepare: Prepare the platform for entering the low power state.
- *	Called right after the hibernation image has been saved and before
- *	devices are prepared for entering the low power state.
+ * @post_snapshot: Prepare the platform for saving the hibernation image.
+ *	Called right prior to thawing devices in order to save the hibernation
+ *	image, after the nonboot CPUs have been enabled (runs with IRQs on).
+ *
+ * @finish_early: Restore the previous state of the platform putting it into the
+ *	normal operation mode after hibernation (first stage).
+ *	Called only during restore, prior to enabling the nonboot CPUs (runs
+ *	with one CPU on line and with IRQs on).
+ *
+ * @finish: Restore the previous state of the platform putting it into the
+ *	normal operation mode after hibernation (second stage).
+ *	Called during restore after the nonboot CPUs have been enabled and prior
+ *	to thawing devices.  Also called during hibernation if there's an error
+ *	making it impossible to hibernate, but only after @pre_snapshot() have
+ *	been successfully executed.  Runs with IRQs on.
+ *
+ * @prepare: Prepare the platform for entering the hibernation state (first
+ *	stage).
+ *	Called right after the hibernation image has been saved and devices have
+ *	been prepared for entering the low power states.
+ *
+ * @prepare_late: Prepare the platform for entering the hibernation state
+ *	(second stage).
+ *	Called after the hibernation image has been saved, devices have been
+ *	prepared for entering the low power states and the nonboot CPUs have
+ *	been disabled.
  *
  * @enter: Put the system into the low power state after the hibernation image
  *	has been saved to disk.
@@ -167,8 +201,11 @@ extern void mark_free_pages(struct zone 
 struct platform_hibernation_ops {
 	int (*start)(void);
 	int (*pre_snapshot)(void);
+	void (*post_snapshot)(void);
+	void (*finish_early)(void);
 	void (*finish)(void);
 	int (*prepare)(void);
+	int (*prepare_late)(void);
 	int (*enter)(void);
 	int (*pre_restore)(void);
 	void (*restore_cleanup)(void);
Index: linux-2.6.23-rc3/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.23-rc3.orig/drivers/acpi/sleep/main.c
+++ linux-2.6.23-rc3/drivers/acpi/sleep/main.c
@@ -58,7 +58,7 @@ static int acpi_pm_set_target(suspend_st
 }
 
 /**
- *	acpi_pm_prepare - Do preliminary suspend work.
+ *	acpi_pm_prepare - Do preliminary suspend work (first stage).
  *
  *	If necessary, set the firmware waking vector and do arch-specific
  *	nastiness to get the wakeup code to the waking vector.
@@ -75,6 +75,24 @@ static int acpi_pm_prepare(void)
 }
 
 /**
+ *	acpi_pm_prepare_late - Do preliminary suspend work (second stage).
+ */
+
+static int acpi_pm_prepare_late(void)
+{
+	acpi_status status;
+
+	status = acpi_enter_sleep_state_prep_late(acpi_target_sleep_state);
+
+	if (ACPI_SUCCESS(status))
+		return 0;
+
+	acpi_target_sleep_state = ACPI_STATE_S0;
+
+	return -EFAULT;
+}
+
+/**
  *	acpi_pm_enter - Actually enter a sleep state.
  *	@pm_state: ignored
  *
@@ -139,10 +157,22 @@ static int acpi_pm_enter(suspend_state_t
 }
 
 /**
- *	acpi_pm_finish - Finish up suspend sequence.
+ *	acpi_pm_finish_early - Finish up suspend sequence (first stage).
  *
  *	This is called after we wake back up (or if entering the sleep state
- *	failed). 
+ *	has failed).
+ */
+
+static void acpi_pm_finish_early(void)
+{
+	acpi_leave_sleep_state_prep(acpi_target_sleep_state);
+}
+
+/**
+ *	acpi_pm_finish - Finish up suspend sequence (second stage).
+ *
+ *	This is called after we wake back up (or if entering the sleep state
+ *	has failed).
  */
 
 static void acpi_pm_finish(void)
@@ -185,7 +215,9 @@ static struct platform_suspend_ops acpi_
 	.valid = acpi_pm_state_valid,
 	.set_target = acpi_pm_set_target,
 	.prepare = acpi_pm_prepare,
+	.prepare_late = acpi_pm_prepare_late,
 	.enter = acpi_pm_enter,
+	.finish_early = acpi_pm_finish_early,
 	.finish = acpi_pm_finish,
 };
 
@@ -222,6 +254,15 @@ static int acpi_hibernation_prepare(void
 	return acpi_sleep_prepare(ACPI_STATE_S4);
 }
 
+static int acpi_hibernation_prepare_late(void)
+{
+	acpi_status status;
+
+	status = acpi_enter_sleep_state_prep_late(ACPI_STATE_S4);
+
+	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
+}
+
 static int acpi_hibernation_enter(void)
 {
 	acpi_status status = AE_OK;
@@ -238,13 +279,18 @@ static int acpi_hibernation_enter(void)
 	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
 }
 
-static void acpi_hibernation_finish(void)
+static void acpi_hibernation_finish_early(void)
 {
 	/*
 	 * If ACPI is not enabled by the BIOS and the boot kernel, we need to
 	 * enable it here.
 	 */
 	acpi_enable();
+	acpi_leave_sleep_state_prep(ACPI_STATE_S4);
+}
+
+static void acpi_hibernation_finish(void)
+{
 	acpi_leave_sleep_state(ACPI_STATE_S4);
 	acpi_disable_wakeup_device(ACPI_STATE_S4);
 
@@ -271,7 +317,10 @@ static void acpi_hibernation_restore_cle
 static struct platform_hibernation_ops acpi_hibernation_ops = {
 	.start = acpi_hibernation_start,
 	.pre_snapshot = acpi_hibernation_prepare,
+	.post_snapshot = acpi_hibernation_finish,
 	.finish = acpi_hibernation_finish,
+	.finish_early = acpi_hibernation_finish_early,
+	.prepare_late = acpi_hibernation_prepare_late,
 	.prepare = acpi_hibernation_prepare,
 	.enter = acpi_hibernation_enter,
 	.pre_restore = acpi_hibernation_pre_restore,
Index: linux-2.6.23-rc3/kernel/power/main.c
===================================================================
--- linux-2.6.23-rc3.orig/kernel/power/main.c
+++ linux-2.6.23-rc3/kernel/power/main.c
@@ -173,9 +173,21 @@ int suspend_devices_and_enter(suspend_st
 			goto Resume_devices;
 	}
 	error = disable_nonboot_cpus();
-	if (!error)
-		suspend_enter(state);
+	if (error)
+		goto Enable_cpus;
+
+	if (suspend_ops->prepare_late) {
+		error = suspend_ops->prepare_late();
+		if (error)
+			goto Enable_cpus;
+	}
+
+	error = suspend_enter(state);
+
+	if (!error && suspend_ops->finish_early)
+		suspend_ops->finish_early();
 
+ Enable_cpus:
 	enable_nonboot_cpus();
 	if (suspend_ops->finish)
 		suspend_ops->finish();
Index: linux-2.6.23-rc3/drivers/acpi/hardware/hwsleep.c
===================================================================
--- linux-2.6.23-rc3.orig/drivers/acpi/hardware/hwsleep.c
+++ linux-2.6.23-rc3/drivers/acpi/hardware/hwsleep.c
@@ -162,8 +162,8 @@ ACPI_EXPORT_SYMBOL(acpi_get_firmware_wak
  *
  * DESCRIPTION: Prepare to enter a system sleep state (see ACPI 2.0 spec p 231)
  *              This function must execute with interrupts enabled.
- *              We break sleeping into 2 stages so that OSPM can handle
- *              various OS-specific tasks between the two steps.
+ *              We break sleeping into 3 stages so that OSPM can handle
+ *              various OS-specific tasks between the three steps.
  *
  ******************************************************************************/
 acpi_status acpi_enter_sleep_state_prep(u8 sleep_state)
@@ -192,18 +192,13 @@ acpi_status acpi_enter_sleep_state_prep(
 	arg.type = ACPI_TYPE_INTEGER;
 	arg.integer.value = sleep_state;
 
-	/* Run the _PTS and _GTS methods */
+	/* Run the _PTS method */
 
 	status = acpi_evaluate_object(NULL, METHOD_NAME__PTS, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		return_ACPI_STATUS(status);
 	}
 
-	status = acpi_evaluate_object(NULL, METHOD_NAME__GTS, &arg_list, NULL);
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		return_ACPI_STATUS(status);
-	}
-
 	/* Setup the argument to _SST */
 
 	switch (sleep_state) {
@@ -245,6 +240,43 @@ ACPI_EXPORT_SYMBOL(acpi_enter_sleep_stat
 
 /*******************************************************************************
  *
+ * FUNCTION:    acpi_enter_sleep_state_prep_late
+ *
+ * PARAMETERS:  sleep_state         - Which sleep state to enter
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Execute the _GTS ACPI global control method.
+ *              This function must execute with interrupts enabled.
+ *
+ ******************************************************************************/
+acpi_status acpi_enter_sleep_state_prep_late(u8 sleep_state)
+{
+	acpi_status status;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+
+	ACPI_FUNCTION_TRACE(acpi_enter_sleep_state_prep_late);
+
+	/* Setup parameter object */
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = sleep_state;
+
+	/* Run the _GTS method */
+
+	status = acpi_evaluate_object(NULL, METHOD_NAME__GTS, &arg_list, NULL);
+
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
+		return_ACPI_STATUS(status);
+	}
+	return_ACPI_STATUS(AE_OK);
+}
+/*******************************************************************************
+ *
  * FUNCTION:    acpi_enter_sleep_state
  *
  * PARAMETERS:  sleep_state         - Which sleep state to enter
@@ -478,17 +510,17 @@ ACPI_EXPORT_SYMBOL(acpi_enter_sleep_stat
 
 /*******************************************************************************
  *
- * FUNCTION:    acpi_leave_sleep_state
+ * FUNCTION:    acpi_leave_sleep_state_prep
  *
- * PARAMETERS:  sleep_state         - Which sleep state we just exited
+ * PARAMETERS:  sleep_state         - Which sleep state we are exiting
  *
  * RETURN:      Status
  *
- * DESCRIPTION: Perform OS-independent ACPI cleanup after a sleep
+ * DESCRIPTION: Perform OS-independent ACPI cleanup after a sleep (first stage)
  *              Called with interrupts ENABLED.
  *
  ******************************************************************************/
-acpi_status acpi_leave_sleep_state(u8 sleep_state)
+acpi_status acpi_leave_sleep_state_prep(u8 sleep_state)
 {
 	struct acpi_object_list arg_list;
 	union acpi_object arg;
@@ -498,7 +530,7 @@ acpi_status acpi_leave_sleep_state(u8 sl
 	u32 PM1Acontrol;
 	u32 PM1Bcontrol;
 
-	ACPI_FUNCTION_TRACE(acpi_leave_sleep_state);
+	ACPI_FUNCTION_TRACE(acpi_leave_sleep_state_prep);
 
 	/*
 	 * Set SLP_TYPE and SLP_EN to state S0.
@@ -560,17 +592,40 @@ acpi_status acpi_leave_sleep_state(u8 sl
 
 	/* Ignore any errors from these methods */
 
+	arg.integer.value = sleep_state;
+	status = acpi_evaluate_object(NULL, METHOD_NAME__BFS, &arg_list, NULL);
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
+		ACPI_EXCEPTION((AE_INFO, status, "During Method _BFS"));
+	}
+
 	arg.integer.value = ACPI_SST_WAKING;
 	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		ACPI_EXCEPTION((AE_INFO, status, "During Method _SST"));
 	}
 
-	arg.integer.value = sleep_state;
-	status = acpi_evaluate_object(NULL, METHOD_NAME__BFS, &arg_list, NULL);
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		ACPI_EXCEPTION((AE_INFO, status, "During Method _BFS"));
-	}
+	return_ACPI_STATUS(AE_OK);
+}
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_leave_sleep_state
+ *
+ * PARAMETERS:  sleep_state         - Which sleep state we just exited
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Perform OS-independent ACPI cleanup after a sleep (second stage)
+ *              Called with interrupts ENABLED.
+ *
+ ******************************************************************************/
+acpi_status acpi_leave_sleep_state(u8 sleep_state)
+{
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+	acpi_status status;
+
+	ACPI_FUNCTION_TRACE(acpi_leave_sleep_state);
 
 	/*
 	 * GPEs must be enabled before _WAK is called as GPEs
@@ -589,6 +644,13 @@ acpi_status acpi_leave_sleep_state(u8 sl
 		return_ACPI_STATUS(status);
 	}
 
+	/* Setup parameter object */
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = sleep_state;
+
 	status = acpi_evaluate_object(NULL, METHOD_NAME__WAK, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		ACPI_EXCEPTION((AE_INFO, status, "During Method _WAK"));
Index: linux-2.6.23-rc3/include/acpi/acpixf.h
===================================================================
--- linux-2.6.23-rc3.orig/include/acpi/acpixf.h
+++ linux-2.6.23-rc3/include/acpi/acpixf.h
@@ -329,10 +329,14 @@ acpi_get_sleep_type_data(u8 sleep_state,
 
 acpi_status acpi_enter_sleep_state_prep(u8 sleep_state);
 
+acpi_status acpi_enter_sleep_state_prep_late(u8 sleep_state);
+
 acpi_status asmlinkage acpi_enter_sleep_state(u8 sleep_state);
 
 acpi_status asmlinkage acpi_enter_sleep_state_s4bios(void);
 
+acpi_status acpi_leave_sleep_state_prep(u8 sleep_state);
+
 acpi_status acpi_leave_sleep_state(u8 sleep_state);
 
 #endif				/* __ACXFACE_H__ */
Index: linux-2.6.23-rc3/kernel/power/disk.c
===================================================================
--- linux-2.6.23-rc3.orig/kernel/power/disk.c
+++ linux-2.6.23-rc3/kernel/power/disk.c
@@ -54,9 +54,10 @@ static struct platform_hibernation_ops *
 
 void hibernation_set_ops(struct platform_hibernation_ops *ops)
 {
-	if (ops && !(ops->start && ops->pre_snapshot && ops->finish
-	    && ops->prepare && ops->enter && ops->pre_restore
-	    && ops->restore_cleanup)) {
+	if (ops && !(ops->start && ops->pre_snapshot && ops->post_snapshot
+	    && ops->finish_early && ops->finish
+	    && ops->prepare && ops->prepare_late && ops->enter
+	    && ops->pre_restore && ops->restore_cleanup)) {
 		WARN_ON(1);
 		return;
 	}
@@ -93,8 +94,32 @@ static int platform_pre_snapshot(int pla
 }
 
 /**
+ *	platform_post_snapshot - use the platform driver, if so configured, to
+ *	prepare the machine for saving the hibernation image.
+ */
+
+static void platform_post_snapshot(int platform_mode)
+{
+	if (platform_mode && hibernation_ops)
+		hibernation_ops->post_snapshot();
+}
+
+/**
+ *	platform_finish_early - switch the machine to the normal mode of
+ *	operation using the platform driver (first stage; called only after
+ *	image restoration)
+ */
+
+static void platform_finish_early(int platform_mode)
+{
+	if (platform_mode && hibernation_ops)
+		hibernation_ops->finish_early();
+}
+
+/**
  *	platform_finish - switch the machine to the normal mode of operation
- *	using the platform driver (must be called after platform_prepare())
+ *	using the platform driver (second stage; called only after image
+ *	restoration)
  */
 
 static void platform_finish(int platform_mode)
@@ -165,14 +190,20 @@ int hibernation_snapshot(int platform_mo
 			in_suspend = 1;
 			error = swsusp_suspend();
 			/* Control returns here after successful restore */
+			if (!in_suspend)
+				platform_finish_early(platform_mode);
 		} else {
 			printk("swsusp debug: Waiting for 5 seconds.\n");
 			mdelay(5000);
 		}
 	}
 	enable_nonboot_cpus();
+
+	if (in_suspend && !error)
+		platform_post_snapshot(platform_mode);
+	else
+		platform_finish(platform_mode);
  Resume_devices:
-	platform_finish(platform_mode);
 	device_resume();
  Resume_console:
 	resume_console();
@@ -227,8 +258,9 @@ int hibernation_platform_enter(void)
 
 	/*
 	 * We have cancelled the power transition by running
-	 * hibernation_ops->finish() before saving the image, so we should let
-	 * the firmware know that we're going to enter the sleep state after all
+	 * hibernation_ops->post_snapshot() before saving the image, so we
+	 * should let the firmware know that we're going to enter the sleep
+	 * state after all
 	 */
 	error = hibernation_ops->start();
 	if (error)
@@ -247,6 +279,10 @@ int hibernation_platform_enter(void)
 	if (error)
 		goto Finish;
 
+	error = hibernation_ops->prepare_late();
+	if (error)
+		goto Finish;
+
 	local_irq_disable();
 	error = device_power_down(PMSG_SUSPEND);
 	if (!error) {

[PATCH -mm 3/3] PM: Improve handling of ACPI system state indicator (rev. 3)

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Introduce a separate ACPI function for setting the system status indicator and
use it in the right places in the suspend and hibernation related ACPI callbacks
instead of setting the system status indicator implicitly in
acpi_enter_sleep_state_prep() and acpi_leave_sleep_state().

This allows us to avoid turning off the sleep state indicator during hibernation
on Thinkpads (currently, it is turned off before saving the image, because we
need to call finish() before unfreezing devices).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/acpi/hardware/hwsleep.c |   51 +++++++++++++++++++++++++++-------------
 drivers/acpi/sleep/main.c       |   40 ++++++++++++++++++++++++-------
 include/acpi/acpixf.h           |    2 +
 3 files changed, 68 insertions(+), 25 deletions(-)

Index: linux-2.6.23-rc3/drivers/acpi/hardware/hwsleep.c
===================================================================
--- linux-2.6.23-rc3.orig/drivers/acpi/hardware/hwsleep.c
+++ linux-2.6.23-rc3/drivers/acpi/hardware/hwsleep.c
@@ -199,15 +199,46 @@ acpi_status acpi_enter_sleep_state_prep(
 		return_ACPI_STATUS(status);
 	}
 
-	/* Setup the argument to _SST */
+	/* Disable/Clear all GPEs */
+
+	status = acpi_hw_disable_all_gpes();
+
+	return_ACPI_STATUS(status);
+}
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_set_sleep_state_indicator
+ *
+ * PARAMETERS:  sleep_state         - Which sleep state to enter
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Make the system status indicator reflect the sleep state being
+ *              entered.
+ *              This function must execute with interrupts enabled.
+ *
+ ******************************************************************************/
+acpi_status acpi_set_sleep_state_indicator(u8 sleep_state)
+{
+	acpi_status status;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+
+	ACPI_FUNCTION_TRACE(acpi_set_sleep_state_indicator);
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+
+	arg.type = ACPI_TYPE_INTEGER;
 
+	/* Setup the argument to _SST */
 	switch (sleep_state) {
 	case ACPI_STATE_S0:
 		arg.integer.value = ACPI_SST_WORKING;
 		break;
 
 	case ACPI_STATE_S1:
-	case ACPI_STATE_S2:
 	case ACPI_STATE_S3:
 		arg.integer.value = ACPI_SST_SLEEPING;
 		break;
@@ -217,27 +248,21 @@ acpi_status acpi_enter_sleep_state_prep(
 		break;
 
 	default:
-		arg.integer.value = ACPI_SST_INDICATOR_OFF;	/* Default is off */
+		/* Default is off */
+		arg.integer.value = ACPI_SST_INDICATOR_OFF;
 		break;
 	}
 
 	/* Set the system indicators to show the desired sleep state. */
-
 	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		ACPI_EXCEPTION((AE_INFO, status,
 				"While executing method _SST"));
 	}
 
-	/* Disable/Clear all GPEs */
-
-	status = acpi_hw_disable_all_gpes();
-
 	return_ACPI_STATUS(status);
 }
 
-ACPI_EXPORT_SYMBOL(acpi_enter_sleep_state_prep)
-
 /*******************************************************************************
  *
  * FUNCTION:    acpi_enter_sleep_state_prep_late
@@ -669,12 +694,6 @@ acpi_status acpi_leave_sleep_state(u8 sl
 	    acpi_set_register(acpi_gbl_fixed_event_info
 			      [ACPI_EVENT_POWER_BUTTON].status_register_id, 1);
 
-	arg.integer.value = ACPI_SST_WORKING;
-	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		ACPI_EXCEPTION((AE_INFO, status, "During Method _SST"));
-	}
-
 	return_ACPI_STATUS(status);
 }
 
Index: linux-2.6.23-rc3/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.23-rc3.orig/drivers/acpi/sleep/main.c
+++ linux-2.6.23-rc3/drivers/acpi/sleep/main.c
@@ -70,6 +70,8 @@ static int acpi_pm_prepare(void)
 
 	if (error)
 		acpi_target_sleep_state = ACPI_STATE_S0;
+	else
+		acpi_set_sleep_state_indicator(acpi_target_sleep_state);
 
 	return error;
 }
@@ -186,6 +188,7 @@ static void acpi_pm_finish(void)
 	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
 
 	acpi_target_sleep_state = ACPI_STATE_S0;
+	acpi_set_sleep_state_indicator(ACPI_STATE_S0);
 
 #ifdef CONFIG_X86
 	if (init_8259A_after_S1) {
@@ -246,9 +249,33 @@ static struct dmi_system_id __initdata a
 static int acpi_hibernation_start(void)
 {
 	acpi_target_sleep_state = ACPI_STATE_S4;
+
 	return 0;
 }
 
+static int acpi_hibernation_pre_snapshot(void)
+{
+	int error = acpi_sleep_prepare(ACPI_STATE_S4);
+
+	if (error)
+		acpi_target_sleep_state = ACPI_STATE_S0;
+	else
+		acpi_set_sleep_state_indicator(ACPI_STATE_S4);
+
+	return error;
+}
+
+static void acpi_hibernation_post_snapshot(void)
+{
+	acpi_leave_sleep_state(ACPI_STATE_S4);
+	acpi_disable_wakeup_device(ACPI_STATE_S4);
+
+	/* reset firmware waking vector */
+	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
+
+	acpi_target_sleep_state = ACPI_STATE_S0;
+}
+
 static int acpi_hibernation_prepare(void)
 {
 	return acpi_sleep_prepare(ACPI_STATE_S4);
@@ -291,13 +318,8 @@ static void acpi_hibernation_finish_earl
 
 static void acpi_hibernation_finish(void)
 {
-	acpi_leave_sleep_state(ACPI_STATE_S4);
-	acpi_disable_wakeup_device(ACPI_STATE_S4);
-
-	/* reset firmware waking vector */
-	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
-
-	acpi_target_sleep_state = ACPI_STATE_S0;
+	acpi_hibernation_post_snapshot();
+	acpi_set_sleep_state_indicator(ACPI_STATE_S0);
 }
 
 static int acpi_hibernation_pre_restore(void)
@@ -316,8 +338,8 @@ static void acpi_hibernation_restore_cle
 
 static struct platform_hibernation_ops acpi_hibernation_ops = {
 	.start = acpi_hibernation_start,
-	.pre_snapshot = acpi_hibernation_prepare,
-	.post_snapshot = acpi_hibernation_finish,
+	.pre_snapshot = acpi_hibernation_pre_snapshot,
+	.post_snapshot = acpi_hibernation_post_snapshot,
 	.finish = acpi_hibernation_finish,
 	.finish_early = acpi_hibernation_finish_early,
 	.prepare_late = acpi_hibernation_prepare_late,
Index: linux-2.6.23-rc3/include/acpi/acpixf.h
===================================================================
--- linux-2.6.23-rc3.orig/include/acpi/acpixf.h
+++ linux-2.6.23-rc3/include/acpi/acpixf.h
@@ -327,6 +327,8 @@ acpi_get_firmware_waking_vector(acpi_phy
 acpi_status
 acpi_get_sleep_type_data(u8 sleep_state, u8 * slp_typ_a, u8 * slp_typ_b);
 
+acpi_status acpi_set_sleep_state_indicator(u8 sleep_state);
+
 acpi_status acpi_enter_sleep_state_prep(u8 sleep_state);
 
 acpi_status acpi_enter_sleep_state_prep_late(u8 sleep_state);

Re: [PATCH -mm 2/3] PM: More fine grained ACPI handling during suspend and hibernation (rev. 3)

[Len Brown]

On Monday 27 August 2007 17:51, Rafael J. Wysocki wrote:
> According to the ACPI specification (eg. ACPI 2.0c, sec. 7.3.1, 7.3.3,
> ACPI 3.0b, sec. 7.3.1, 7.3.3) the _GTS and _BFS global control methods should
> be executed, respectively, right before entering a sleep state (S1-S4) and right
> after leaving it, but we don't follow this reqirement.  Namely, in our
> implementation the nonboot CPUs are disabled after executing _GTS and enabled
> before executing _BFS, which doesn't seem to be correct.

I've never encountered a BIOS that actually implements _GTS or _BFS,
so I expect that changing how they are invoked may be somewhat academic.

> [In fact, the ACPI 
> specification requires that no physical I/O and interrupt servicing be performed
> after the sleep state has been left and before _BFS is executed as well as after
> executing _GTS and before the sleep state is entered, but we can't follow this
> requirement literally, 

> since our AML interpreter needs to run with interrupts 
> enabled and we need to carry out some operations with interrupts disabled before
> entering the sleep state and after leaving it.]

This is sort of a myth.

The real requirement is that the ACPI interpreter must be able to call kmalloc().
It does this today via acpi_os_allocate(), which does this:

kmalloc(size, irqs_disabled()? GFP_ATOMIC : GFP_KERNEL);

No, we don't actually run the interpreter during device interrupts,
but we need to be able to run it with interrupts off for boot,
suspend, and resume.

So how did boot work before this hack was added?
kmalloc() does a might_sleep(), but deep down in
cond_sleep, there is a handy little check for
if (system_state == SYSTEM_RUNNING)
to disable the run-time oops.

I suggested that since it works during boot, and resume is in many
ways similar to boot, we should just re-use system_state for early resume.
But at the time, akpm told me not to use system_state, and so we have the hack above.

I don't recall his reasoning -- it might be something that should
be re-visited.  I don't like disabling the may_sleep() check all the time,
I'd rather just disable it during the critical boot/suspend/resume states.

> Moreover, acpi_enable() called 
> after restoring the system memory state from a hibernation image should really
> be executed before enabling the nonboot CPUs, since functional ACPI may be
> needed for that.  All of this means that we need to handle ACPI in a more fine
> grained manner during suspend and hibernation.

I don't follow the requirement to boot an ACPI-enabled resume image
from a non-ACPI-enabled boot kernel.  Certainly this isn't a scenario
described by the ACPI spec, which transitions between G1(S4) and G0(S0) without
going through an ACPI-disabled state.

-Len

RE: [PATCH -mm 3/3] PM: Improve handling of ACPI system state indicator (rev. 3)

[Moore, Robert]

Since these changes appear to affect the ACPICA core in a fairly big
way, I would like to see a short, concise description of each change and
why it is necessary.

Thanks,
Bob


-----Original Message-----
From: linux-acpi-owner@vger.kernel.org
[mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
Sent: Monday, August 27, 2007 2:53 PM
To: Andrew Morton
Cc: ACPI Devel Maling List; Len Brown; LKML; Pavel Machek; pm list
Subject: [PATCH -mm 3/3] PM: Improve handling of ACPI system state
indicator (rev. 3)

From: Rafael J. Wysocki <rjw@sisk.pl>

Re: [PATCH -mm 2/3] PM: More fine grained ACPI handling during suspend and hibernation (rev. 3)

[Rafael J. Wysocki]

On Tuesday, 28 August 2007 21:48, Len Brown wrote:
> On Monday 27 August 2007 17:51, Rafael J. Wysocki wrote:
> > According to the ACPI specification (eg. ACPI 2.0c, sec. 7.3.1, 7.3.3,
> > ACPI 3.0b, sec. 7.3.1, 7.3.3) the _GTS and _BFS global control methods should
> > be executed, respectively, right before entering a sleep state (S1-S4) and right
> > after leaving it, but we don't follow this reqirement.  Namely, in our
> > implementation the nonboot CPUs are disabled after executing _GTS and enabled
> > before executing _BFS, which doesn't seem to be correct.
> 
> I've never encountered a BIOS that actually implements _GTS or _BFS,
> so I expect that changing how they are invoked may be somewhat academic.

It is for now, but once we have a system that implements them, we'd most
probably need to change the current code, so I think it's better to consider
that in advance.

> > [In fact, the ACPI 
> > specification requires that no physical I/O and interrupt servicing be performed
> > after the sleep state has been left and before _BFS is executed as well as after
> > executing _GTS and before the sleep state is entered, but we can't follow this
> > requirement literally, 
> 
> > since our AML interpreter needs to run with interrupts 
> > enabled and we need to carry out some operations with interrupts disabled before
> > entering the sleep state and after leaving it.]
> 
> This is sort of a myth.
> 
> The real requirement is that the ACPI interpreter must be able to call kmalloc().
> It does this today via acpi_os_allocate(), which does this:
> 
> kmalloc(size, irqs_disabled()? GFP_ATOMIC : GFP_KERNEL);
> 
> No, we don't actually run the interpreter during device interrupts,
> but we need to be able to run it with interrupts off for boot,
> suspend, and resume.

At present, during suspend and resume we always call the AML interpreter
with interrupts enabled.

Frankly, I'd like _BFS and _GTS to be executed with interrupts disabled,
just as the specification tells us to do.  If you think that's safe, I'll
change the patch to work this way.

> So how did boot work before this hack was added?
> kmalloc() does a might_sleep(), but deep down in
> cond_sleep, there is a handy little check for
> if (system_state == SYSTEM_RUNNING)
> to disable the run-time oops.
> 
> I suggested that since it works during boot, and resume is in many
> ways similar to boot, we should just re-use system_state for early resume.
> But at the time, akpm told me not to use system_state, and so we have the hack above.
> 
> I don't recall his reasoning -- it might be something that should
> be re-visited.  I don't like disabling the may_sleep() check all the time,
> I'd rather just disable it during the critical boot/suspend/resume states.
> 
> > Moreover, acpi_enable() called 
> > after restoring the system memory state from a hibernation image should really
> > be executed before enabling the nonboot CPUs, since functional ACPI may be
> > needed for that.  All of this means that we need to handle ACPI in a more fine
> > grained manner during suspend and hibernation.
> 
> I don't follow the requirement to boot an ACPI-enabled resume image
> from a non-ACPI-enabled boot kernel.  Certainly this isn't a scenario
> described by the ACPI spec, which transitions between G1(S4) and G0(S0) without
> going through an ACPI-disabled state.

That actually depends on which version of the ACPI specification you consider.

In ACPI 3.0 (and later) there's section 15 "Waking and Sleeping" that
describes, among other things, the supposed system start sequence (in 15.3.3).
It clearly states that we're supposed to check if ACPI is enabled (and enable
if not), only _after_ the hibernation image has been loaded.  After that, in
turn, we should execute _BFS and subsequently _WAK, so my interpretation is
that we should not execute any ACPI methods before that point.

Anyway, however, if the user passes acpi=off to the boot kernel, ACPI may not
be enabled until the image kernel gets control.  Thus, it should always check
if ACPI is enabled (and enable it, if need be) before doing anything
ACPI-related and that should happen before the nonboot CPUs are enabled.
Preferrably, with interrupts off, as that should be done before we attempt to
execute _BFS.

Greetings,
Rafael

Re: [PATCH -mm 3/3] PM: Improve handling of ACPI system state indicator (rev. 3)

[Rafael J. Wysocki]

On Tuesday, 28 August 2007 21:57, Moore, Robert wrote:
> Since these changes appear to affect the ACPICA core in a fairly big
> way, I would like to see a short, concise description of each change and
> why it is necessary.

All right.  I'll describe the changes made by the current version of the
patches, but please note that if it's safe to run the AML interpreter with
IRQs disabled, it's better to do some of them in a different way.

1. Remove the execution of _GTS from acpi_enter_sleep_state_prep()

	acpi_enter_sleep_state_prep() is called before disabling the nonboot
	CPUs and _GTS should be executed after that, according to the spec.

2. Introduce acpi_enter_sleep_state_prep_late() that will execute _GTS

	Necessary because of 1.

3. Split acpi_leave_sleep_state() into two functions:
   acpi_leave_sleep_state_prep() and acpi_leave_sleep_state().

	acpi_leave_sleep_state_prep() contains the code that should be executed
	before enabling the nonboot CPUs, most importantly the execution of
	_BFS, and acpi_leave_sleep_state() contains the remaining code (the
	enabling of GPEs, the execution of _WAK and the enabling of power
	buttons)

4. Change the code ordering in acpi_leave_sleep_state_prep() (introduced
   in 3.) so that _SST is executed after _BFS

	According to the spec, _BFS should be the first ACPI method executed
	after leaving a sleep state

5. Introduce acpi_set_sleep_state_indicator() that will execute _SST for given
   ACPI sleep state

	Needed so that we can set the state indicator independently of the
	other lower-level operations.

6. Remove the execution of _SST from acpi_leave_sleep_state()

	No longer needed, because we can use acpi_set_sleep_state_indicator()
	to set the state indicator appropriately from higher level routines.

The other changes affect only drivers/acpi/sleep/main.c and the files in
kernel/power .

Greetings,
Rafael

Re: [PATCH -mm 2/3] PM: More fine grained ACPI handling during suspend and hibernation (rev. 3)

[Rafael J. Wysocki]

On Tuesday, 28 August 2007 23:35, Rafael J. Wysocki wrote:
> On Tuesday, 28 August 2007 21:48, Len Brown wrote:
> > On Monday 27 August 2007 17:51, Rafael J. Wysocki wrote:
> > > According to the ACPI specification (eg. ACPI 2.0c, sec. 7.3.1, 7.3.3,
> > > ACPI 3.0b, sec. 7.3.1, 7.3.3) the _GTS and _BFS global control methods should
> > > be executed, respectively, right before entering a sleep state (S1-S4) and right
> > > after leaving it, but we don't follow this reqirement.  Namely, in our
> > > implementation the nonboot CPUs are disabled after executing _GTS and enabled
> > > before executing _BFS, which doesn't seem to be correct.
> > 
> > I've never encountered a BIOS that actually implements _GTS or _BFS,
> > so I expect that changing how they are invoked may be somewhat academic.
> 
> It is for now, but once we have a system that implements them, we'd most
> probably need to change the current code, so I think it's better to consider
> that in advance.
> 
> > > [In fact, the ACPI 
> > > specification requires that no physical I/O and interrupt servicing be performed
> > > after the sleep state has been left and before _BFS is executed as well as after
> > > executing _GTS and before the sleep state is entered, but we can't follow this
> > > requirement literally, 
> > 
> > > since our AML interpreter needs to run with interrupts 
> > > enabled and we need to carry out some operations with interrupts disabled before
> > > entering the sleep state and after leaving it.]
> > 
> > This is sort of a myth.
> > 
> > The real requirement is that the ACPI interpreter must be able to call kmalloc().
> > It does this today via acpi_os_allocate(), which does this:
> > 
> > kmalloc(size, irqs_disabled()? GFP_ATOMIC : GFP_KERNEL);
> > 
> > No, we don't actually run the interpreter during device interrupts,
> > but we need to be able to run it with interrupts off for boot,
> > suspend, and resume.
> 
> At present, during suspend and resume we always call the AML interpreter
> with interrupts enabled.
> 
> Frankly, I'd like _BFS and _GTS to be executed with interrupts disabled,
> just as the specification tells us to do.  If you think that's safe, I'll
> change the patch to work this way.

Appended is an alternative version of the $subject patch, in which the _GTS and
_BFS methods are executed with interrupts disabled.  It is simpler than the
previous one, so I prefer it. :-)

Greetings,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>

According to the ACPI specification (eg. ACPI 2.0c, sec. 7.3.1, 7.3.3,
ACPI 3.0b, sec. 7.3.1, 7.3.3) the _GTS and _BFS global control methods should
be executed, respectively, right before entering a sleep state (S1-S4) and right
after leaving it, but we don't follow this reqirement.  Namely, in our
implementation the nonboot CPUs are disabled after executing _GTS and enabled
before executing _BFS, which doesn't seem to be correct.  Moreover,
acpi_enable() called after restoring the system memory state from a hibernation
image should really be executed before enabling the nonboot CPUs, since
functional ACPI layer may be needed for that.  Thus, it seems reasonable to do
the following changes:
* Move the execution of _GTS from acpi_enter_sleep_state_prep() to
  acpi_enter_sleep_state()
* Move the execution of _BFS to before the execution of _SST in
  acpi_leave_sleep_state()
* Move the enabling of GPS, the execution of _WAK and the enabling of power
  buttons from acpi_leave_sleep_state() to a new function
  acpi_leave_sleep_state_finish()
* Make acpi_leave_sleep_state() be called with interrupts disabled from
  acpi_pm_enter(), right after leaving the sleep state
* Make acpi_pm_finish() call acpi_leave_sleep_state_finish() instead of
  acpi_leave_sleep_state()
* Introduce a new global hibernation callback ->leave() to be executed with
  interrupts disabled just after transferring control from the boot kernel to
  the image kernel and make it call acpi_enable() and
  acpi_leave_sleep_state(ACPI_STATE_S4) on ACPI systems
* Make acpi_hibernation_finish() call acpi_leave_sleep_state_finish() instead of
  acpi_leave_sleep_state()

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/hardware/hwsleep.c |   73 +++++++++++++++++++++++++++++++---------
 drivers/acpi/sleep/main.c       |   13 ++++++-
 include/acpi/acpixf.h           |    2 +
 include/linux/suspend.h         |    7 +++
 kernel/power/disk.c             |   62 ++++++++++++++++++++++++++++++++-
 kernel/power/power.h            |    1 
 kernel/power/swsusp.c           |   33 ------------------
 7 files changed, 137 insertions(+), 54 deletions(-)

Index: linux-2.6.23-rc4/drivers/acpi/hardware/hwsleep.c
===================================================================
--- linux-2.6.23-rc4.orig/drivers/acpi/hardware/hwsleep.c
+++ linux-2.6.23-rc4/drivers/acpi/hardware/hwsleep.c
@@ -192,18 +192,13 @@ acpi_status acpi_enter_sleep_state_prep(
 	arg.type = ACPI_TYPE_INTEGER;
 	arg.integer.value = sleep_state;
 
-	/* Run the _PTS and _GTS methods */
+	/* Run the _PTS method */
 
 	status = acpi_evaluate_object(NULL, METHOD_NAME__PTS, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		return_ACPI_STATUS(status);
 	}
 
-	status = acpi_evaluate_object(NULL, METHOD_NAME__GTS, &arg_list, NULL);
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		return_ACPI_STATUS(status);
-	}
-
 	/* Setup the argument to _SST */
 
 	switch (sleep_state) {
@@ -263,6 +258,8 @@ acpi_status asmlinkage acpi_enter_sleep_
 	struct acpi_bit_register_info *sleep_enable_reg_info;
 	u32 in_value;
 	acpi_status status;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
 
 	ACPI_FUNCTION_TRACE(acpi_enter_sleep_state);
 
@@ -317,6 +314,19 @@ acpi_status asmlinkage acpi_enter_sleep_
 	ACPI_DEBUG_PRINT((ACPI_DB_INIT,
 			  "Entering sleep state [S%d]\n", sleep_state));
 
+	/* Execute the _GTS method */
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = sleep_state;
+
+	status = acpi_evaluate_object(NULL, METHOD_NAME__GTS, &arg_list, NULL);
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
+		return_ACPI_STATUS(status);
+	}
+
 	/* Clear SLP_EN and SLP_TYP fields */
 
 	PM1Acontrol &= ~(sleep_type_reg_info->access_bit_mask |
@@ -484,8 +494,9 @@ ACPI_EXPORT_SYMBOL(acpi_enter_sleep_stat
  *
  * RETURN:      Status
  *
- * DESCRIPTION: Perform OS-independent ACPI cleanup after a sleep
- *              Called with interrupts ENABLED.
+ * DESCRIPTION: Perform the first stage of OS-independent ACPI cleanup after a
+ *              sleep.
+ *              Called with one CPU on line and with interrupts DISABLED.
  *
  ******************************************************************************/
 acpi_status acpi_leave_sleep_state(u8 sleep_state)
@@ -560,17 +571,43 @@ acpi_status acpi_leave_sleep_state(u8 sl
 
 	/* Ignore any errors from these methods */
 
+	arg.integer.value = sleep_state;
+	status = acpi_evaluate_object(NULL, METHOD_NAME__BFS, &arg_list, NULL);
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
+		ACPI_EXCEPTION((AE_INFO, status, "During Method _BFS"));
+	}
+
 	arg.integer.value = ACPI_SST_WAKING;
 	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		ACPI_EXCEPTION((AE_INFO, status, "During Method _SST"));
 	}
 
-	arg.integer.value = sleep_state;
-	status = acpi_evaluate_object(NULL, METHOD_NAME__BFS, &arg_list, NULL);
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		ACPI_EXCEPTION((AE_INFO, status, "During Method _BFS"));
-	}
+	return_ACPI_STATUS(AE_OK);
+}
+
+ACPI_EXPORT_SYMBOL(acpi_leave_sleep_state)
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_leave_sleep_state_finish
+ *
+ * PARAMETERS:  sleep_state         - Which sleep state we just exited
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Perform the second stage of OS-independent ACPI cleanup after a
+ *              sleep.
+ *              Called with interrupts ENABLED.
+ *
+ ******************************************************************************/
+acpi_status acpi_leave_sleep_state_finish(u8 sleep_state)
+{
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+	acpi_status status;
+
+	ACPI_FUNCTION_TRACE(acpi_leave_sleep_state_finish);
 
 	/*
 	 * GPEs must be enabled before _WAK is called as GPEs
@@ -589,6 +626,14 @@ acpi_status acpi_leave_sleep_state(u8 sl
 		return_ACPI_STATUS(status);
 	}
 
+	/* Execute the _WAK method */
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = sleep_state;
+
 	status = acpi_evaluate_object(NULL, METHOD_NAME__WAK, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		ACPI_EXCEPTION((AE_INFO, status, "During Method _WAK"));
@@ -615,5 +660,3 @@ acpi_status acpi_leave_sleep_state(u8 sl
 
 	return_ACPI_STATUS(status);
 }
-
-ACPI_EXPORT_SYMBOL(acpi_leave_sleep_state)
Index: linux-2.6.23-rc4/include/acpi/acpixf.h
===================================================================
--- linux-2.6.23-rc4.orig/include/acpi/acpixf.h
+++ linux-2.6.23-rc4/include/acpi/acpixf.h
@@ -335,4 +335,6 @@ acpi_status asmlinkage acpi_enter_sleep_
 
 acpi_status acpi_leave_sleep_state(u8 sleep_state);
 
+acpi_status acpi_leave_sleep_state_finish(u8 sleep_state);
+
 #endif				/* __ACXFACE_H__ */
Index: linux-2.6.23-rc4/include/linux/suspend.h
===================================================================
--- linux-2.6.23-rc4.orig/include/linux/suspend.h
+++ linux-2.6.23-rc4/include/linux/suspend.h
@@ -156,6 +156,12 @@ extern void mark_free_pages(struct zone 
  *	Called after the nonboot CPUs have been disabled and all of the low
  *	level devices have been shut down (runs with IRQs off).
  *
+ * @leave: Perform the first stage of the cleanup after the system sleep state
+ *	indicated by @set_target() has been left.
+ *	Called right after contol has been passed from the boot kernel to the
+ *	image kernel, before the nonboot CPUs are enabled and before devices are
+ *	resumed.  Executed with interrupts disabled.
+ *
  * @pre_restore: Prepare system for the restoration from a hibernation image.
  *	Called right after devices have been frozen and before the nonboot
  *	CPUs are disabled (runs with IRQs on).
@@ -170,6 +176,7 @@ struct platform_hibernation_ops {
 	void (*finish)(void);
 	int (*prepare)(void);
 	int (*enter)(void);
+	void (*leave)(void);
 	int (*pre_restore)(void);
 	void (*restore_cleanup)(void);
 };
Index: linux-2.6.23-rc4/kernel/power/disk.c
===================================================================
--- linux-2.6.23-rc4.orig/kernel/power/disk.c
+++ linux-2.6.23-rc4/kernel/power/disk.c
@@ -55,7 +55,7 @@ static struct platform_hibernation_ops *
 void hibernation_set_ops(struct platform_hibernation_ops *ops)
 {
 	if (ops && !(ops->start && ops->pre_snapshot && ops->finish
-	    && ops->prepare && ops->enter && ops->pre_restore
+	    && ops->prepare && ops->enter && ops->leave && ops->pre_restore
 	    && ops->restore_cleanup)) {
 		WARN_ON(1);
 		return;
@@ -93,8 +93,19 @@ static int platform_pre_snapshot(int pla
 }
 
 /**
+ *	platform_leave - prepare the machine for switching to the normal mode
+ *	of operation using the platform driver (called with interrupts disabled)
+ */
+
+static void platform_leave(int platform_mode)
+{
+	if (platform_mode && hibernation_ops)
+		hibernation_ops->leave();
+}
+
+/**
  *	platform_finish - switch the machine to the normal mode of operation
- *	using the platform driver (must be called after platform_prepare())
+ *	using the platform driver (must be called after platform_leave())
  */
 
 static void platform_finish(int platform_mode)
@@ -129,6 +140,51 @@ static void platform_restore_cleanup(int
 }
 
 /**
+ *	create_image - freeze devices that need to be frozen with interrupts
+ *	off, create the hibernation image and thaw those devices.  Control
+ *	reappears in this routine after a restore.
+ */
+
+int create_image(int platform_mode)
+{
+	int error;
+
+	error = arch_prepare_suspend();
+	if (error)
+		return error;
+
+	local_irq_disable();
+	/* At this point, device_suspend() has been called, but *not*
+	 * device_power_down(). We *must* call device_power_down() now.
+	 * Otherwise, drivers for some devices (e.g. interrupt controllers)
+	 * become desynchronized with the actual state of the hardware
+	 * at resume time, and evil weirdness ensues.
+	 */
+	error = device_power_down(PMSG_FREEZE);
+	if (error) {
+		printk(KERN_ERR "Some devices failed to power down, "
+			KERN_ERR "aborting suspend\n");
+		goto Enable_irqs;
+	}
+
+	save_processor_state();
+	error = swsusp_arch_suspend();
+	if (error)
+		printk(KERN_ERR "Error %d while creating the image\n", error);
+	/* Restore control flow magically appears here */
+	restore_processor_state();
+	if (!error && !in_suspend)
+		platform_leave(platform_mode);
+	/* NOTE:  device_power_up() is just a resume() for devices
+	 * that suspended with irqs off ... no overall powerup.
+	 */
+	device_power_up();
+ Enable_irqs:
+	local_irq_enable();
+	return error;
+}
+
+/**
  *	hibernation_snapshot - quiesce devices and create the hibernation
  *	snapshot image.
  *	@platform_mode - if set, use the platform driver, if available, to
@@ -163,7 +219,7 @@ int hibernation_snapshot(int platform_mo
 	if (!error) {
 		if (hibernation_mode != HIBERNATION_TEST) {
 			in_suspend = 1;
-			error = swsusp_suspend();
+			error = create_image(platform_mode);
 			/* Control returns here after successful restore */
 		} else {
 			printk("swsusp debug: Waiting for 5 seconds.\n");
Index: linux-2.6.23-rc4/kernel/power/swsusp.c
===================================================================
--- linux-2.6.23-rc4.orig/kernel/power/swsusp.c
+++ linux-2.6.23-rc4/kernel/power/swsusp.c
@@ -270,39 +270,6 @@ int swsusp_shrink_memory(void)
 	return 0;
 }
 
-int swsusp_suspend(void)
-{
-	int error;
-
-	if ((error = arch_prepare_suspend()))
-		return error;
-
-	local_irq_disable();
-	/* At this point, device_suspend() has been called, but *not*
-	 * device_power_down(). We *must* device_power_down() now.
-	 * Otherwise, drivers for some devices (e.g. interrupt controllers)
-	 * become desynchronized with the actual state of the hardware
-	 * at resume time, and evil weirdness ensues.
-	 */
-	if ((error = device_power_down(PMSG_FREEZE))) {
-		printk(KERN_ERR "Some devices failed to power down, aborting suspend\n");
-		goto Enable_irqs;
-	}
-
-	save_processor_state();
-	if ((error = swsusp_arch_suspend()))
-		printk(KERN_ERR "Error %d suspending\n", error);
-	/* Restore control flow magically appears here */
-	restore_processor_state();
-	/* NOTE:  device_power_up() is just a resume() for devices
-	 * that suspended with irqs off ... no overall powerup.
-	 */
-	device_power_up();
- Enable_irqs:
-	local_irq_enable();
-	return error;
-}
-
 int swsusp_resume(void)
 {
 	int error;
Index: linux-2.6.23-rc4/kernel/power/power.h
===================================================================
--- linux-2.6.23-rc4.orig/kernel/power/power.h
+++ linux-2.6.23-rc4/kernel/power/power.h
@@ -183,7 +183,6 @@ extern int swsusp_swap_in_use(void);
 extern int swsusp_check(void);
 extern int swsusp_shrink_memory(void);
 extern void swsusp_free(void);
-extern int swsusp_suspend(void);
 extern int swsusp_resume(void);
 extern int swsusp_read(unsigned int *flags_p);
 extern int swsusp_write(unsigned int flags);
Index: linux-2.6.23-rc4/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.23-rc4.orig/drivers/acpi/sleep/main.c
+++ linux-2.6.23-rc4/drivers/acpi/sleep/main.c
@@ -114,6 +114,9 @@ static int acpi_pm_enter(suspend_state_t
 		break;
 	}
 
+	/* Execute _BFS */
+	acpi_leave_sleep_state(acpi_state);
+
 	/* ACPI 3.0 specs (P62) says that it's the responsabilty
 	 * of the OSPM to clear the status bit [ implying that the
 	 * POWER_BUTTON event should not reach userspace ]
@@ -149,7 +152,7 @@ static void acpi_pm_finish(void)
 {
 	u32 acpi_state = acpi_target_sleep_state;
 
-	acpi_leave_sleep_state(acpi_state);
+	acpi_leave_sleep_state_finish(acpi_state);
 	acpi_disable_wakeup_device(acpi_state);
 
 	/* reset firmware waking vector */
@@ -238,7 +241,7 @@ static int acpi_hibernation_enter(void)
 	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
 }
 
-static void acpi_hibernation_finish(void)
+static void acpi_hibernation_leave(void)
 {
 	/*
 	 * If ACPI is not enabled by the BIOS and the boot kernel, we need to
@@ -246,6 +249,11 @@ static void acpi_hibernation_finish(void
 	 */
 	acpi_enable();
 	acpi_leave_sleep_state(ACPI_STATE_S4);
+}
+
+static void acpi_hibernation_finish(void)
+{
+	acpi_leave_sleep_state_finish(ACPI_STATE_S4);
 	acpi_disable_wakeup_device(ACPI_STATE_S4);
 
 	/* reset firmware waking vector */
@@ -274,6 +282,7 @@ static struct platform_hibernation_ops a
 	.finish = acpi_hibernation_finish,
 	.prepare = acpi_hibernation_prepare,
 	.enter = acpi_hibernation_enter,
+	.leave = acpi_hibernation_leave,
 	.pre_restore = acpi_hibernation_pre_restore,
 	.restore_cleanup = acpi_hibernation_restore_cleanup,
 };
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH -mm 2/3] PM: More fine grained ACPI handling during suspend and hibernation (rev. 3)

[Rafael J. Wysocki]

On Tuesday, 28 August 2007 23:35, Rafael J. Wysocki wrote:
> On Tuesday, 28 August 2007 21:48, Len Brown wrote:
> > On Monday 27 August 2007 17:51, Rafael J. Wysocki wrote:
> > > According to the ACPI specification (eg. ACPI 2.0c, sec. 7.3.1, 7.3.3,
> > > ACPI 3.0b, sec. 7.3.1, 7.3.3) the _GTS and _BFS global control methods should
> > > be executed, respectively, right before entering a sleep state (S1-S4) and right
> > > after leaving it, but we don't follow this reqirement.  Namely, in our
> > > implementation the nonboot CPUs are disabled after executing _GTS and enabled
> > > before executing _BFS, which doesn't seem to be correct.
> > 
> > I've never encountered a BIOS that actually implements _GTS or _BFS,
> > so I expect that changing how they are invoked may be somewhat academic.
> 
> It is for now, but once we have a system that implements them, we'd most
> probably need to change the current code, so I think it's better to consider
> that in advance.
> 
> > > [In fact, the ACPI 
> > > specification requires that no physical I/O and interrupt servicing be performed
> > > after the sleep state has been left and before _BFS is executed as well as after
> > > executing _GTS and before the sleep state is entered, but we can't follow this
> > > requirement literally, 
> > 
> > > since our AML interpreter needs to run with interrupts 
> > > enabled and we need to carry out some operations with interrupts disabled before
> > > entering the sleep state and after leaving it.]
> > 
> > This is sort of a myth.
> > 
> > The real requirement is that the ACPI interpreter must be able to call kmalloc().
> > It does this today via acpi_os_allocate(), which does this:
> > 
> > kmalloc(size, irqs_disabled()? GFP_ATOMIC : GFP_KERNEL);
> > 
> > No, we don't actually run the interpreter during device interrupts,
> > but we need to be able to run it with interrupts off for boot,
> > suspend, and resume.
> 
> At present, during suspend and resume we always call the AML interpreter
> with interrupts enabled.
> 
> Frankly, I'd like _BFS and _GTS to be executed with interrupts disabled,
> just as the specification tells us to do.  If you think that's safe, I'll
> change the patch to work this way.

Appended is an alternative version of the $subject patch, in which the _GTS and
_BFS methods are executed with interrupts disabled.  It is simpler than the
previous one, so I prefer it. :-)

Greetings,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>

According to the ACPI specification (eg. ACPI 2.0c, sec. 7.3.1, 7.3.3,
ACPI 3.0b, sec. 7.3.1, 7.3.3) the _GTS and _BFS global control methods should
be executed, respectively, right before entering a sleep state (S1-S4) and right
after leaving it, but we don't follow this reqirement.  Namely, in our
implementation the nonboot CPUs are disabled after executing _GTS and enabled
before executing _BFS, which doesn't seem to be correct.  Moreover,
acpi_enable() called after restoring the system memory state from a hibernation
image should really be executed before enabling the nonboot CPUs, since
functional ACPI layer may be needed for that.  Thus, it seems reasonable to do
the following changes:
* Move the execution of _GTS from acpi_enter_sleep_state_prep() to
  acpi_enter_sleep_state()
* Move the execution of _BFS to before the execution of _SST in
  acpi_leave_sleep_state()
* Move the enabling of GPS, the execution of _WAK and the enabling of power
  buttons from acpi_leave_sleep_state() to a new function
  acpi_leave_sleep_state_finish()
* Make acpi_leave_sleep_state() be called with interrupts disabled from
  acpi_pm_enter(), right after leaving the sleep state
* Make acpi_pm_finish() call acpi_leave_sleep_state_finish() instead of
  acpi_leave_sleep_state()
* Introduce a new global hibernation callback ->leave() to be executed with
  interrupts disabled just after transferring control from the boot kernel to
  the image kernel and make it call acpi_enable() and
  acpi_leave_sleep_state(ACPI_STATE_S4) on ACPI systems
* Make acpi_hibernation_finish() call acpi_leave_sleep_state_finish() instead of
  acpi_leave_sleep_state()

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/hardware/hwsleep.c |   73 +++++++++++++++++++++++++++++++---------
 drivers/acpi/sleep/main.c       |   13 ++++++-
 include/acpi/acpixf.h           |    2 +
 include/linux/suspend.h         |    7 +++
 kernel/power/disk.c             |   62 ++++++++++++++++++++++++++++++++-
 kernel/power/power.h            |    1 
 kernel/power/swsusp.c           |   33 ------------------
 7 files changed, 137 insertions(+), 54 deletions(-)

Index: linux-2.6.23-rc4/drivers/acpi/hardware/hwsleep.c
===================================================================
--- linux-2.6.23-rc4.orig/drivers/acpi/hardware/hwsleep.c
+++ linux-2.6.23-rc4/drivers/acpi/hardware/hwsleep.c
@@ -192,18 +192,13 @@ acpi_status acpi_enter_sleep_state_prep(
 	arg.type = ACPI_TYPE_INTEGER;
 	arg.integer.value = sleep_state;
 
-	/* Run the _PTS and _GTS methods */
+	/* Run the _PTS method */
 
 	status = acpi_evaluate_object(NULL, METHOD_NAME__PTS, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		return_ACPI_STATUS(status);
 	}
 
-	status = acpi_evaluate_object(NULL, METHOD_NAME__GTS, &arg_list, NULL);
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		return_ACPI_STATUS(status);
-	}
-
 	/* Setup the argument to _SST */
 
 	switch (sleep_state) {
@@ -263,6 +258,8 @@ acpi_status asmlinkage acpi_enter_sleep_
 	struct acpi_bit_register_info *sleep_enable_reg_info;
 	u32 in_value;
 	acpi_status status;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
 
 	ACPI_FUNCTION_TRACE(acpi_enter_sleep_state);
 
@@ -317,6 +314,19 @@ acpi_status asmlinkage acpi_enter_sleep_
 	ACPI_DEBUG_PRINT((ACPI_DB_INIT,
 			  "Entering sleep state [S%d]\n", sleep_state));
 
+	/* Execute the _GTS method */
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = sleep_state;
+
+	status = acpi_evaluate_object(NULL, METHOD_NAME__GTS, &arg_list, NULL);
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
+		return_ACPI_STATUS(status);
+	}
+
 	/* Clear SLP_EN and SLP_TYP fields */
 
 	PM1Acontrol &= ~(sleep_type_reg_info->access_bit_mask |
@@ -484,8 +494,9 @@ ACPI_EXPORT_SYMBOL(acpi_enter_sleep_stat
  *
  * RETURN:      Status
  *
- * DESCRIPTION: Perform OS-independent ACPI cleanup after a sleep
- *              Called with interrupts ENABLED.
+ * DESCRIPTION: Perform the first stage of OS-independent ACPI cleanup after a
+ *              sleep.
+ *              Called with one CPU on line and with interrupts DISABLED.
  *
  ******************************************************************************/
 acpi_status acpi_leave_sleep_state(u8 sleep_state)
@@ -560,17 +571,43 @@ acpi_status acpi_leave_sleep_state(u8 sl
 
 	/* Ignore any errors from these methods */
 
+	arg.integer.value = sleep_state;
+	status = acpi_evaluate_object(NULL, METHOD_NAME__BFS, &arg_list, NULL);
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
+		ACPI_EXCEPTION((AE_INFO, status, "During Method _BFS"));
+	}
+
 	arg.integer.value = ACPI_SST_WAKING;
 	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		ACPI_EXCEPTION((AE_INFO, status, "During Method _SST"));
 	}
 
-	arg.integer.value = sleep_state;
-	status = acpi_evaluate_object(NULL, METHOD_NAME__BFS, &arg_list, NULL);
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		ACPI_EXCEPTION((AE_INFO, status, "During Method _BFS"));
-	}
+	return_ACPI_STATUS(AE_OK);
+}
+
+ACPI_EXPORT_SYMBOL(acpi_leave_sleep_state)
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_leave_sleep_state_finish
+ *
+ * PARAMETERS:  sleep_state         - Which sleep state we just exited
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Perform the second stage of OS-independent ACPI cleanup after a
+ *              sleep.
+ *              Called with interrupts ENABLED.
+ *
+ ******************************************************************************/
+acpi_status acpi_leave_sleep_state_finish(u8 sleep_state)
+{
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+	acpi_status status;
+
+	ACPI_FUNCTION_TRACE(acpi_leave_sleep_state_finish);
 
 	/*
 	 * GPEs must be enabled before _WAK is called as GPEs
@@ -589,6 +626,14 @@ acpi_status acpi_leave_sleep_state(u8 sl
 		return_ACPI_STATUS(status);
 	}
 
+	/* Execute the _WAK method */
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = sleep_state;
+
 	status = acpi_evaluate_object(NULL, METHOD_NAME__WAK, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		ACPI_EXCEPTION((AE_INFO, status, "During Method _WAK"));
@@ -615,5 +660,3 @@ acpi_status acpi_leave_sleep_state(u8 sl
 
 	return_ACPI_STATUS(status);
 }
-
-ACPI_EXPORT_SYMBOL(acpi_leave_sleep_state)
Index: linux-2.6.23-rc4/include/acpi/acpixf.h
===================================================================
--- linux-2.6.23-rc4.orig/include/acpi/acpixf.h
+++ linux-2.6.23-rc4/include/acpi/acpixf.h
@@ -335,4 +335,6 @@ acpi_status asmlinkage acpi_enter_sleep_
 
 acpi_status acpi_leave_sleep_state(u8 sleep_state);
 
+acpi_status acpi_leave_sleep_state_finish(u8 sleep_state);
+
 #endif				/* __ACXFACE_H__ */
Index: linux-2.6.23-rc4/include/linux/suspend.h
===================================================================
--- linux-2.6.23-rc4.orig/include/linux/suspend.h
+++ linux-2.6.23-rc4/include/linux/suspend.h
@@ -156,6 +156,12 @@ extern void mark_free_pages(struct zone 
  *	Called after the nonboot CPUs have been disabled and all of the low
  *	level devices have been shut down (runs with IRQs off).
  *
+ * @leave: Perform the first stage of the cleanup after the system sleep state
+ *	indicated by @set_target() has been left.
+ *	Called right after contol has been passed from the boot kernel to the
+ *	image kernel, before the nonboot CPUs are enabled and before devices are
+ *	resumed.  Executed with interrupts disabled.
+ *
  * @pre_restore: Prepare system for the restoration from a hibernation image.
  *	Called right after devices have been frozen and before the nonboot
  *	CPUs are disabled (runs with IRQs on).
@@ -170,6 +176,7 @@ struct platform_hibernation_ops {
 	void (*finish)(void);
 	int (*prepare)(void);
 	int (*enter)(void);
+	void (*leave)(void);
 	int (*pre_restore)(void);
 	void (*restore_cleanup)(void);
 };
Index: linux-2.6.23-rc4/kernel/power/disk.c
===================================================================
--- linux-2.6.23-rc4.orig/kernel/power/disk.c
+++ linux-2.6.23-rc4/kernel/power/disk.c
@@ -55,7 +55,7 @@ static struct platform_hibernation_ops *
 void hibernation_set_ops(struct platform_hibernation_ops *ops)
 {
 	if (ops && !(ops->start && ops->pre_snapshot && ops->finish
-	    && ops->prepare && ops->enter && ops->pre_restore
+	    && ops->prepare && ops->enter && ops->leave && ops->pre_restore
 	    && ops->restore_cleanup)) {
 		WARN_ON(1);
 		return;
@@ -93,8 +93,19 @@ static int platform_pre_snapshot(int pla
 }
 
 /**
+ *	platform_leave - prepare the machine for switching to the normal mode
+ *	of operation using the platform driver (called with interrupts disabled)
+ */
+
+static void platform_leave(int platform_mode)
+{
+	if (platform_mode && hibernation_ops)
+		hibernation_ops->leave();
+}
+
+/**
  *	platform_finish - switch the machine to the normal mode of operation
- *	using the platform driver (must be called after platform_prepare())
+ *	using the platform driver (must be called after platform_leave())
  */
 
 static void platform_finish(int platform_mode)
@@ -129,6 +140,51 @@ static void platform_restore_cleanup(int
 }
 
 /**
+ *	create_image - freeze devices that need to be frozen with interrupts
+ *	off, create the hibernation image and thaw those devices.  Control
+ *	reappears in this routine after a restore.
+ */
+
+int create_image(int platform_mode)
+{
+	int error;
+
+	error = arch_prepare_suspend();
+	if (error)
+		return error;
+
+	local_irq_disable();
+	/* At this point, device_suspend() has been called, but *not*
+	 * device_power_down(). We *must* call device_power_down() now.
+	 * Otherwise, drivers for some devices (e.g. interrupt controllers)
+	 * become desynchronized with the actual state of the hardware
+	 * at resume time, and evil weirdness ensues.
+	 */
+	error = device_power_down(PMSG_FREEZE);
+	if (error) {
+		printk(KERN_ERR "Some devices failed to power down, "
+			KERN_ERR "aborting suspend\n");
+		goto Enable_irqs;
+	}
+
+	save_processor_state();
+	error = swsusp_arch_suspend();
+	if (error)
+		printk(KERN_ERR "Error %d while creating the image\n", error);
+	/* Restore control flow magically appears here */
+	restore_processor_state();
+	if (!error && !in_suspend)
+		platform_leave(platform_mode);
+	/* NOTE:  device_power_up() is just a resume() for devices
+	 * that suspended with irqs off ... no overall powerup.
+	 */
+	device_power_up();
+ Enable_irqs:
+	local_irq_enable();
+	return error;
+}
+
+/**
  *	hibernation_snapshot - quiesce devices and create the hibernation
  *	snapshot image.
  *	@platform_mode - if set, use the platform driver, if available, to
@@ -163,7 +219,7 @@ int hibernation_snapshot(int platform_mo
 	if (!error) {
 		if (hibernation_mode != HIBERNATION_TEST) {
 			in_suspend = 1;
-			error = swsusp_suspend();
+			error = create_image(platform_mode);
 			/* Control returns here after successful restore */
 		} else {
 			printk("swsusp debug: Waiting for 5 seconds.\n");
Index: linux-2.6.23-rc4/kernel/power/swsusp.c
===================================================================
--- linux-2.6.23-rc4.orig/kernel/power/swsusp.c
+++ linux-2.6.23-rc4/kernel/power/swsusp.c
@@ -270,39 +270,6 @@ int swsusp_shrink_memory(void)
 	return 0;
 }
 
-int swsusp_suspend(void)
-{
-	int error;
-
-	if ((error = arch_prepare_suspend()))
-		return error;
-
-	local_irq_disable();
-	/* At this point, device_suspend() has been called, but *not*
-	 * device_power_down(). We *must* device_power_down() now.
-	 * Otherwise, drivers for some devices (e.g. interrupt controllers)
-	 * become desynchronized with the actual state of the hardware
-	 * at resume time, and evil weirdness ensues.
-	 */
-	if ((error = device_power_down(PMSG_FREEZE))) {
-		printk(KERN_ERR "Some devices failed to power down, aborting suspend\n");
-		goto Enable_irqs;
-	}
-
-	save_processor_state();
-	if ((error = swsusp_arch_suspend()))
-		printk(KERN_ERR "Error %d suspending\n", error);
-	/* Restore control flow magically appears here */
-	restore_processor_state();
-	/* NOTE:  device_power_up() is just a resume() for devices
-	 * that suspended with irqs off ... no overall powerup.
-	 */
-	device_power_up();
- Enable_irqs:
-	local_irq_enable();
-	return error;
-}
-
 int swsusp_resume(void)
 {
 	int error;
Index: linux-2.6.23-rc4/kernel/power/power.h
===================================================================
--- linux-2.6.23-rc4.orig/kernel/power/power.h
+++ linux-2.6.23-rc4/kernel/power/power.h
@@ -183,7 +183,6 @@ extern int swsusp_swap_in_use(void);
 extern int swsusp_check(void);
 extern int swsusp_shrink_memory(void);
 extern void swsusp_free(void);
-extern int swsusp_suspend(void);
 extern int swsusp_resume(void);
 extern int swsusp_read(unsigned int *flags_p);
 extern int swsusp_write(unsigned int flags);
Index: linux-2.6.23-rc4/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.23-rc4.orig/drivers/acpi/sleep/main.c
+++ linux-2.6.23-rc4/drivers/acpi/sleep/main.c
@@ -114,6 +114,9 @@ static int acpi_pm_enter(suspend_state_t
 		break;
 	}
 
+	/* Execute _BFS */
+	acpi_leave_sleep_state(acpi_state);
+
 	/* ACPI 3.0 specs (P62) says that it's the responsabilty
 	 * of the OSPM to clear the status bit [ implying that the
 	 * POWER_BUTTON event should not reach userspace ]
@@ -149,7 +152,7 @@ static void acpi_pm_finish(void)
 {
 	u32 acpi_state = acpi_target_sleep_state;
 
-	acpi_leave_sleep_state(acpi_state);
+	acpi_leave_sleep_state_finish(acpi_state);
 	acpi_disable_wakeup_device(acpi_state);
 
 	/* reset firmware waking vector */
@@ -238,7 +241,7 @@ static int acpi_hibernation_enter(void)
 	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
 }
 
-static void acpi_hibernation_finish(void)
+static void acpi_hibernation_leave(void)
 {
 	/*
 	 * If ACPI is not enabled by the BIOS and the boot kernel, we need to
@@ -246,6 +249,11 @@ static void acpi_hibernation_finish(void
 	 */
 	acpi_enable();
 	acpi_leave_sleep_state(ACPI_STATE_S4);
+}
+
+static void acpi_hibernation_finish(void)
+{
+	acpi_leave_sleep_state_finish(ACPI_STATE_S4);
 	acpi_disable_wakeup_device(ACPI_STATE_S4);
 
 	/* reset firmware waking vector */
@@ -274,6 +282,7 @@ static struct platform_hibernation_ops a
 	.finish = acpi_hibernation_finish,
 	.prepare = acpi_hibernation_prepare,
 	.enter = acpi_hibernation_enter,
+	.leave = acpi_hibernation_leave,
 	.pre_restore = acpi_hibernation_pre_restore,
 	.restore_cleanup = acpi_hibernation_restore_cleanup,
 };

Re: [PATCH -mm 2/3] PM: More fine grained ACPI handling during suspend and hibernation (rev. 3)

[Rafael J. Wysocki]

On Tuesday, 28 August 2007 23:35, Rafael J. Wysocki wrote:
> On Tuesday, 28 August 2007 21:48, Len Brown wrote:
> > On Monday 27 August 2007 17:51, Rafael J. Wysocki wrote:
> > > According to the ACPI specification (eg. ACPI 2.0c, sec. 7.3.1, 7.3.3,
> > > ACPI 3.0b, sec. 7.3.1, 7.3.3) the _GTS and _BFS global control methods should
> > > be executed, respectively, right before entering a sleep state (S1-S4) and right
> > > after leaving it, but we don't follow this reqirement.  Namely, in our
> > > implementation the nonboot CPUs are disabled after executing _GTS and enabled
> > > before executing _BFS, which doesn't seem to be correct.
> > 
> > I've never encountered a BIOS that actually implements _GTS or _BFS,
> > so I expect that changing how they are invoked may be somewhat academic.
> 
> It is for now, but once we have a system that implements them, we'd most
> probably need to change the current code, so I think it's better to consider
> that in advance.
> 
> > > [In fact, the ACPI 
> > > specification requires that no physical I/O and interrupt servicing be performed
> > > after the sleep state has been left and before _BFS is executed as well as after
> > > executing _GTS and before the sleep state is entered, but we can't follow this
> > > requirement literally, 
> > 
> > > since our AML interpreter needs to run with interrupts 
> > > enabled and we need to carry out some operations with interrupts disabled before
> > > entering the sleep state and after leaving it.]
> > 
> > This is sort of a myth.
> > 
> > The real requirement is that the ACPI interpreter must be able to call kmalloc().
> > It does this today via acpi_os_allocate(), which does this:
> > 
> > kmalloc(size, irqs_disabled()? GFP_ATOMIC : GFP_KERNEL);
> > 
> > No, we don't actually run the interpreter during device interrupts,
> > but we need to be able to run it with interrupts off for boot,
> > suspend, and resume.
> 
> At present, during suspend and resume we always call the AML interpreter
> with interrupts enabled.
> 
> Frankly, I'd like _BFS and _GTS to be executed with interrupts disabled,
> just as the specification tells us to do.  If you think that's safe, I'll
> change the patch to work this way.

Appended is an alternative version of the $subject patch, in which the _GTS and
_BFS methods are executed with interrupts disabled.  It is simpler than the
previous one, so I prefer it. :-)

Greetings,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>

According to the ACPI specification (eg. ACPI 2.0c, sec. 7.3.1, 7.3.3,
ACPI 3.0b, sec. 7.3.1, 7.3.3) the _GTS and _BFS global control methods should
be executed, respectively, right before entering a sleep state (S1-S4) and right
after leaving it, but we don't follow this reqirement.  Namely, in our
implementation the nonboot CPUs are disabled after executing _GTS and enabled
before executing _BFS, which doesn't seem to be correct.  Moreover,
acpi_enable() called after restoring the system memory state from a hibernation
image should really be executed before enabling the nonboot CPUs, since
functional ACPI layer may be needed for that.  Thus, it seems reasonable to do
the following changes:
* Move the execution of _GTS from acpi_enter_sleep_state_prep() to
  acpi_enter_sleep_state()
* Move the execution of _BFS to before the execution of _SST in
  acpi_leave_sleep_state()
* Move the enabling of GPS, the execution of _WAK and the enabling of power
  buttons from acpi_leave_sleep_state() to a new function
  acpi_leave_sleep_state_finish()
* Make acpi_leave_sleep_state() be called with interrupts disabled from
  acpi_pm_enter(), right after leaving the sleep state
* Make acpi_pm_finish() call acpi_leave_sleep_state_finish() instead of
  acpi_leave_sleep_state()
* Introduce a new global hibernation callback ->leave() to be executed with
  interrupts disabled just after transferring control from the boot kernel to
  the image kernel and make it call acpi_enable() and
  acpi_leave_sleep_state(ACPI_STATE_S4) on ACPI systems
* Make acpi_hibernation_finish() call acpi_leave_sleep_state_finish() instead of
  acpi_leave_sleep_state()

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/hardware/hwsleep.c |   73 +++++++++++++++++++++++++++++++---------
 drivers/acpi/sleep/main.c       |   13 ++++++-
 include/acpi/acpixf.h           |    2 +
 include/linux/suspend.h         |    7 +++
 kernel/power/disk.c             |   62 ++++++++++++++++++++++++++++++++-
 kernel/power/power.h            |    1 
 kernel/power/swsusp.c           |   33 ------------------
 7 files changed, 137 insertions(+), 54 deletions(-)

Index: linux-2.6.23-rc4/drivers/acpi/hardware/hwsleep.c
===================================================================
--- linux-2.6.23-rc4.orig/drivers/acpi/hardware/hwsleep.c
+++ linux-2.6.23-rc4/drivers/acpi/hardware/hwsleep.c
@@ -192,18 +192,13 @@ acpi_status acpi_enter_sleep_state_prep(
 	arg.type = ACPI_TYPE_INTEGER;
 	arg.integer.value = sleep_state;
 
-	/* Run the _PTS and _GTS methods */
+	/* Run the _PTS method */
 
 	status = acpi_evaluate_object(NULL, METHOD_NAME__PTS, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		return_ACPI_STATUS(status);
 	}
 
-	status = acpi_evaluate_object(NULL, METHOD_NAME__GTS, &arg_list, NULL);
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		return_ACPI_STATUS(status);
-	}
-
 	/* Setup the argument to _SST */
 
 	switch (sleep_state) {
@@ -263,6 +258,8 @@ acpi_status asmlinkage acpi_enter_sleep_
 	struct acpi_bit_register_info *sleep_enable_reg_info;
 	u32 in_value;
 	acpi_status status;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
 
 	ACPI_FUNCTION_TRACE(acpi_enter_sleep_state);
 
@@ -317,6 +314,19 @@ acpi_status asmlinkage acpi_enter_sleep_
 	ACPI_DEBUG_PRINT((ACPI_DB_INIT,
 			  "Entering sleep state [S%d]\n", sleep_state));
 
+	/* Execute the _GTS method */
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = sleep_state;
+
+	status = acpi_evaluate_object(NULL, METHOD_NAME__GTS, &arg_list, NULL);
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
+		return_ACPI_STATUS(status);
+	}
+
 	/* Clear SLP_EN and SLP_TYP fields */
 
 	PM1Acontrol &= ~(sleep_type_reg_info->access_bit_mask |
@@ -484,8 +494,9 @@ ACPI_EXPORT_SYMBOL(acpi_enter_sleep_stat
  *
  * RETURN:      Status
  *
- * DESCRIPTION: Perform OS-independent ACPI cleanup after a sleep
- *              Called with interrupts ENABLED.
+ * DESCRIPTION: Perform the first stage of OS-independent ACPI cleanup after a
+ *              sleep.
+ *              Called with one CPU on line and with interrupts DISABLED.
  *
  ******************************************************************************/
 acpi_status acpi_leave_sleep_state(u8 sleep_state)
@@ -560,17 +571,43 @@ acpi_status acpi_leave_sleep_state(u8 sl
 
 	/* Ignore any errors from these methods */
 
+	arg.integer.value = sleep_state;
+	status = acpi_evaluate_object(NULL, METHOD_NAME__BFS, &arg_list, NULL);
+	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
+		ACPI_EXCEPTION((AE_INFO, status, "During Method _BFS"));
+	}
+
 	arg.integer.value = ACPI_SST_WAKING;
 	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		ACPI_EXCEPTION((AE_INFO, status, "During Method _SST"));
 	}
 
-	arg.integer.value = sleep_state;
-	status = acpi_evaluate_object(NULL, METHOD_NAME__BFS, &arg_list, NULL);
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		ACPI_EXCEPTION((AE_INFO, status, "During Method _BFS"));
-	}
+	return_ACPI_STATUS(AE_OK);
+}
+
+ACPI_EXPORT_SYMBOL(acpi_leave_sleep_state)
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_leave_sleep_state_finish
+ *
+ * PARAMETERS:  sleep_state         - Which sleep state we just exited
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Perform the second stage of OS-independent ACPI cleanup after a
+ *              sleep.
+ *              Called with interrupts ENABLED.
+ *
+ ******************************************************************************/
+acpi_status acpi_leave_sleep_state_finish(u8 sleep_state)
+{
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+	acpi_status status;
+
+	ACPI_FUNCTION_TRACE(acpi_leave_sleep_state_finish);
 
 	/*
 	 * GPEs must be enabled before _WAK is called as GPEs
@@ -589,6 +626,14 @@ acpi_status acpi_leave_sleep_state(u8 sl
 		return_ACPI_STATUS(status);
 	}
 
+	/* Execute the _WAK method */
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+
+	arg.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = sleep_state;
+
 	status = acpi_evaluate_object(NULL, METHOD_NAME__WAK, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		ACPI_EXCEPTION((AE_INFO, status, "During Method _WAK"));
@@ -615,5 +660,3 @@ acpi_status acpi_leave_sleep_state(u8 sl
 
 	return_ACPI_STATUS(status);
 }
-
-ACPI_EXPORT_SYMBOL(acpi_leave_sleep_state)
Index: linux-2.6.23-rc4/include/acpi/acpixf.h
===================================================================
--- linux-2.6.23-rc4.orig/include/acpi/acpixf.h
+++ linux-2.6.23-rc4/include/acpi/acpixf.h
@@ -335,4 +335,6 @@ acpi_status asmlinkage acpi_enter_sleep_
 
 acpi_status acpi_leave_sleep_state(u8 sleep_state);
 
+acpi_status acpi_leave_sleep_state_finish(u8 sleep_state);
+
 #endif				/* __ACXFACE_H__ */
Index: linux-2.6.23-rc4/include/linux/suspend.h
===================================================================
--- linux-2.6.23-rc4.orig/include/linux/suspend.h
+++ linux-2.6.23-rc4/include/linux/suspend.h
@@ -156,6 +156,12 @@ extern void mark_free_pages(struct zone 
  *	Called after the nonboot CPUs have been disabled and all of the low
  *	level devices have been shut down (runs with IRQs off).
  *
+ * @leave: Perform the first stage of the cleanup after the system sleep state
+ *	indicated by @set_target() has been left.
+ *	Called right after contol has been passed from the boot kernel to the
+ *	image kernel, before the nonboot CPUs are enabled and before devices are
+ *	resumed.  Executed with interrupts disabled.
+ *
  * @pre_restore: Prepare system for the restoration from a hibernation image.
  *	Called right after devices have been frozen and before the nonboot
  *	CPUs are disabled (runs with IRQs on).
@@ -170,6 +176,7 @@ struct platform_hibernation_ops {
 	void (*finish)(void);
 	int (*prepare)(void);
 	int (*enter)(void);
+	void (*leave)(void);
 	int (*pre_restore)(void);
 	void (*restore_cleanup)(void);
 };
Index: linux-2.6.23-rc4/kernel/power/disk.c
===================================================================
--- linux-2.6.23-rc4.orig/kernel/power/disk.c
+++ linux-2.6.23-rc4/kernel/power/disk.c
@@ -55,7 +55,7 @@ static struct platform_hibernation_ops *
 void hibernation_set_ops(struct platform_hibernation_ops *ops)
 {
 	if (ops && !(ops->start && ops->pre_snapshot && ops->finish
-	    && ops->prepare && ops->enter && ops->pre_restore
+	    && ops->prepare && ops->enter && ops->leave && ops->pre_restore
 	    && ops->restore_cleanup)) {
 		WARN_ON(1);
 		return;
@@ -93,8 +93,19 @@ static int platform_pre_snapshot(int pla
 }
 
 /**
+ *	platform_leave - prepare the machine for switching to the normal mode
+ *	of operation using the platform driver (called with interrupts disabled)
+ */
+
+static void platform_leave(int platform_mode)
+{
+	if (platform_mode && hibernation_ops)
+		hibernation_ops->leave();
+}
+
+/**
  *	platform_finish - switch the machine to the normal mode of operation
- *	using the platform driver (must be called after platform_prepare())
+ *	using the platform driver (must be called after platform_leave())
  */
 
 static void platform_finish(int platform_mode)
@@ -129,6 +140,51 @@ static void platform_restore_cleanup(int
 }
 
 /**
+ *	create_image - freeze devices that need to be frozen with interrupts
+ *	off, create the hibernation image and thaw those devices.  Control
+ *	reappears in this routine after a restore.
+ */
+
+int create_image(int platform_mode)
+{
+	int error;
+
+	error = arch_prepare_suspend();
+	if (error)
+		return error;
+
+	local_irq_disable();
+	/* At this point, device_suspend() has been called, but *not*
+	 * device_power_down(). We *must* call device_power_down() now.
+	 * Otherwise, drivers for some devices (e.g. interrupt controllers)
+	 * become desynchronized with the actual state of the hardware
+	 * at resume time, and evil weirdness ensues.
+	 */
+	error = device_power_down(PMSG_FREEZE);
+	if (error) {
+		printk(KERN_ERR "Some devices failed to power down, "
+			KERN_ERR "aborting suspend\n");
+		goto Enable_irqs;
+	}
+
+	save_processor_state();
+	error = swsusp_arch_suspend();
+	if (error)
+		printk(KERN_ERR "Error %d while creating the image\n", error);
+	/* Restore control flow magically appears here */
+	restore_processor_state();
+	if (!error && !in_suspend)
+		platform_leave(platform_mode);
+	/* NOTE:  device_power_up() is just a resume() for devices
+	 * that suspended with irqs off ... no overall powerup.
+	 */
+	device_power_up();
+ Enable_irqs:
+	local_irq_enable();
+	return error;
+}
+
+/**
  *	hibernation_snapshot - quiesce devices and create the hibernation
  *	snapshot image.
  *	@platform_mode - if set, use the platform driver, if available, to
@@ -163,7 +219,7 @@ int hibernation_snapshot(int platform_mo
 	if (!error) {
 		if (hibernation_mode != HIBERNATION_TEST) {
 			in_suspend = 1;
-			error = swsusp_suspend();
+			error = create_image(platform_mode);
 			/* Control returns here after successful restore */
 		} else {
 			printk("swsusp debug: Waiting for 5 seconds.\n");
Index: linux-2.6.23-rc4/kernel/power/swsusp.c
===================================================================
--- linux-2.6.23-rc4.orig/kernel/power/swsusp.c
+++ linux-2.6.23-rc4/kernel/power/swsusp.c
@@ -270,39 +270,6 @@ int swsusp_shrink_memory(void)
 	return 0;
 }
 
-int swsusp_suspend(void)
-{
-	int error;
-
-	if ((error = arch_prepare_suspend()))
-		return error;
-
-	local_irq_disable();
-	/* At this point, device_suspend() has been called, but *not*
-	 * device_power_down(). We *must* device_power_down() now.
-	 * Otherwise, drivers for some devices (e.g. interrupt controllers)
-	 * become desynchronized with the actual state of the hardware
-	 * at resume time, and evil weirdness ensues.
-	 */
-	if ((error = device_power_down(PMSG_FREEZE))) {
-		printk(KERN_ERR "Some devices failed to power down, aborting suspend\n");
-		goto Enable_irqs;
-	}
-
-	save_processor_state();
-	if ((error = swsusp_arch_suspend()))
-		printk(KERN_ERR "Error %d suspending\n", error);
-	/* Restore control flow magically appears here */
-	restore_processor_state();
-	/* NOTE:  device_power_up() is just a resume() for devices
-	 * that suspended with irqs off ... no overall powerup.
-	 */
-	device_power_up();
- Enable_irqs:
-	local_irq_enable();
-	return error;
-}
-
 int swsusp_resume(void)
 {
 	int error;
Index: linux-2.6.23-rc4/kernel/power/power.h
===================================================================
--- linux-2.6.23-rc4.orig/kernel/power/power.h
+++ linux-2.6.23-rc4/kernel/power/power.h
@@ -183,7 +183,6 @@ extern int swsusp_swap_in_use(void);
 extern int swsusp_check(void);
 extern int swsusp_shrink_memory(void);
 extern void swsusp_free(void);
-extern int swsusp_suspend(void);
 extern int swsusp_resume(void);
 extern int swsusp_read(unsigned int *flags_p);
 extern int swsusp_write(unsigned int flags);
Index: linux-2.6.23-rc4/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.23-rc4.orig/drivers/acpi/sleep/main.c
+++ linux-2.6.23-rc4/drivers/acpi/sleep/main.c
@@ -114,6 +114,9 @@ static int acpi_pm_enter(suspend_state_t
 		break;
 	}
 
+	/* Execute _BFS */
+	acpi_leave_sleep_state(acpi_state);
+
 	/* ACPI 3.0 specs (P62) says that it's the responsabilty
 	 * of the OSPM to clear the status bit [ implying that the
 	 * POWER_BUTTON event should not reach userspace ]
@@ -149,7 +152,7 @@ static void acpi_pm_finish(void)
 {
 	u32 acpi_state = acpi_target_sleep_state;
 
-	acpi_leave_sleep_state(acpi_state);
+	acpi_leave_sleep_state_finish(acpi_state);
 	acpi_disable_wakeup_device(acpi_state);
 
 	/* reset firmware waking vector */
@@ -238,7 +241,7 @@ static int acpi_hibernation_enter(void)
 	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
 }
 
-static void acpi_hibernation_finish(void)
+static void acpi_hibernation_leave(void)
 {
 	/*
 	 * If ACPI is not enabled by the BIOS and the boot kernel, we need to
@@ -246,6 +249,11 @@ static void acpi_hibernation_finish(void
 	 */
 	acpi_enable();
 	acpi_leave_sleep_state(ACPI_STATE_S4);
+}
+
+static void acpi_hibernation_finish(void)
+{
+	acpi_leave_sleep_state_finish(ACPI_STATE_S4);
 	acpi_disable_wakeup_device(ACPI_STATE_S4);
 
 	/* reset firmware waking vector */
@@ -274,6 +282,7 @@ static struct platform_hibernation_ops a
 	.finish = acpi_hibernation_finish,
 	.prepare = acpi_hibernation_prepare,
 	.enter = acpi_hibernation_enter,
+	.leave = acpi_hibernation_leave,
 	.pre_restore = acpi_hibernation_pre_restore,
 	.restore_cleanup = acpi_hibernation_restore_cleanup,
 };

Re: [linux-pm] [PATCH -mm 3/3] PM: Improve handling of ACPI system state indicator (rev. 3)

[Rafael J. Wysocki]

Hi,

Since I've just sent a new version of the 2/3 patch, below is the alternatove
version of $subject applying on top of that one.

Greetings,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>

To be able to avoid turning off the sleep state indicator during hibernation
on Thinkpads (currently, it is turned off before saving the image, because we
need to call finish() before unfreezing devices), we need to move the invocation
of the _SST global ACPI method from acpi_enter_sleep_state_prep() to a separate
function, acpi_set_sleep_state_indicator(), that will be called wherever
necessary by higher-level routines.  Also, once acpi_set_sleep_state_indicator()
has been introduced, the execution of _SST can be removed from
acpi_leave_sleep_state_finish().

acpi_set_sleep_state_indicator() should be called by acpi_pm_prepare() as well
as by acpi_pm_finish() and by acpi_hibernation_finish().  Moreover, it is
necessary to introduce a new hibernation callback ->post_snapshot(), that won't
change the state indicator status and will be executed instead of ->finish()
after creating a hibernation image.  Of course, the status indicator should be
set before the hibernation, so the ->pre_snapshot() callback also has to call
acpi_set_sleep_state_indicator() on ACPI systems.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/hardware/hwsleep.c |   51 +++++++++++++++++++++++++++-------------
 drivers/acpi/sleep/main.c       |   39 ++++++++++++++++++++++++------
 include/acpi/acpixf.h           |    2 +
 include/linux/suspend.h         |   19 ++++++++++----
 kernel/power/disk.c             |   23 ++++++++++++++----
 5 files changed, 100 insertions(+), 34 deletions(-)

Index: linux-2.6.23-rc4/drivers/acpi/hardware/hwsleep.c
===================================================================
--- linux-2.6.23-rc4.orig/drivers/acpi/hardware/hwsleep.c
+++ linux-2.6.23-rc4/drivers/acpi/hardware/hwsleep.c
@@ -199,15 +199,46 @@ acpi_status acpi_enter_sleep_state_prep(
 		return_ACPI_STATUS(status);
 	}
 
-	/* Setup the argument to _SST */
+	/* Disable/Clear all GPEs */
+
+	status = acpi_hw_disable_all_gpes();
+
+	return_ACPI_STATUS(status);
+}
 
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_set_sleep_state_indicator
+ *
+ * PARAMETERS:  sleep_state         - Which sleep state to enter
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Make the system status indicator reflect the sleep state being
+ *              entered.
+ *              This function must execute with interrupts enabled.
+ *
+ ******************************************************************************/
+acpi_status acpi_set_sleep_state_indicator(u8 sleep_state)
+{
+	acpi_status status;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+
+	ACPI_FUNCTION_TRACE(acpi_set_sleep_state_indicator);
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+
+	arg.type = ACPI_TYPE_INTEGER;
+
+	/* Setup the argument to _SST */
 	switch (sleep_state) {
 	case ACPI_STATE_S0:
 		arg.integer.value = ACPI_SST_WORKING;
 		break;
 
 	case ACPI_STATE_S1:
-	case ACPI_STATE_S2:
 	case ACPI_STATE_S3:
 		arg.integer.value = ACPI_SST_SLEEPING;
 		break;
@@ -217,27 +248,21 @@ acpi_status acpi_enter_sleep_state_prep(
 		break;
 
 	default:
-		arg.integer.value = ACPI_SST_INDICATOR_OFF;	/* Default is off */
+		/* Default is off */
+		arg.integer.value = ACPI_SST_INDICATOR_OFF;
 		break;
 	}
 
 	/* Set the system indicators to show the desired sleep state. */
-
 	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		ACPI_EXCEPTION((AE_INFO, status,
 				"While executing method _SST"));
 	}
 
-	/* Disable/Clear all GPEs */
-
-	status = acpi_hw_disable_all_gpes();
-
 	return_ACPI_STATUS(status);
 }
 
-ACPI_EXPORT_SYMBOL(acpi_enter_sleep_state_prep)
-
 /*******************************************************************************
  *
  * FUNCTION:    acpi_enter_sleep_state
@@ -652,11 +677,5 @@ acpi_status acpi_leave_sleep_state_finis
 	    acpi_set_register(acpi_gbl_fixed_event_info
 			      [ACPI_EVENT_POWER_BUTTON].status_register_id, 1);
 
-	arg.integer.value = ACPI_SST_WORKING;
-	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		ACPI_EXCEPTION((AE_INFO, status, "During Method _SST"));
-	}
-
 	return_ACPI_STATUS(status);
 }
Index: linux-2.6.23-rc4/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.23-rc4.orig/drivers/acpi/sleep/main.c
+++ linux-2.6.23-rc4/drivers/acpi/sleep/main.c
@@ -70,6 +70,8 @@ static int acpi_pm_prepare(void)
 
 	if (error)
 		acpi_target_sleep_state = ACPI_STATE_S0;
+	else
+		acpi_set_sleep_state_indicator(acpi_target_sleep_state);
 
 	return error;
 }
@@ -159,6 +161,7 @@ static void acpi_pm_finish(void)
 	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
 
 	acpi_target_sleep_state = ACPI_STATE_S0;
+	acpi_set_sleep_state_indicator(ACPI_STATE_S0);
 
 #ifdef CONFIG_X86
 	if (init_8259A_after_S1) {
@@ -217,9 +220,33 @@ static struct dmi_system_id __initdata a
 static int acpi_hibernation_start(void)
 {
 	acpi_target_sleep_state = ACPI_STATE_S4;
+
 	return 0;
 }
 
+static int acpi_hibernation_pre_snapshot(void)
+{
+	int error = acpi_sleep_prepare(ACPI_STATE_S4);
+
+	if (error)
+		acpi_target_sleep_state = ACPI_STATE_S0;
+	else
+		acpi_set_sleep_state_indicator(ACPI_STATE_S4);
+
+	return error;
+}
+
+static void acpi_hibernation_post_snapshot(void)
+{
+	acpi_leave_sleep_state_finish(ACPI_STATE_S4);
+	acpi_disable_wakeup_device(ACPI_STATE_S4);
+
+	/* reset firmware waking vector */
+	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
+
+	acpi_target_sleep_state = ACPI_STATE_S0;
+}
+
 static int acpi_hibernation_prepare(void)
 {
 	return acpi_sleep_prepare(ACPI_STATE_S4);
@@ -253,13 +280,8 @@ static void acpi_hibernation_leave(void)
 
 static void acpi_hibernation_finish(void)
 {
-	acpi_leave_sleep_state_finish(ACPI_STATE_S4);
-	acpi_disable_wakeup_device(ACPI_STATE_S4);
-
-	/* reset firmware waking vector */
-	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
-
-	acpi_target_sleep_state = ACPI_STATE_S0;
+	acpi_hibernation_post_snapshot();
+	acpi_set_sleep_state_indicator(ACPI_STATE_S0);
 }
 
 static int acpi_hibernation_pre_restore(void)
@@ -278,7 +300,8 @@ static void acpi_hibernation_restore_cle
 
 static struct platform_hibernation_ops acpi_hibernation_ops = {
 	.start = acpi_hibernation_start,
-	.pre_snapshot = acpi_hibernation_prepare,
+	.pre_snapshot = acpi_hibernation_pre_snapshot,
+	.post_snapshot = acpi_hibernation_post_snapshot,
 	.finish = acpi_hibernation_finish,
 	.prepare = acpi_hibernation_prepare,
 	.enter = acpi_hibernation_enter,
Index: linux-2.6.23-rc4/include/acpi/acpixf.h
===================================================================
--- linux-2.6.23-rc4.orig/include/acpi/acpixf.h
+++ linux-2.6.23-rc4/include/acpi/acpixf.h
@@ -327,6 +327,8 @@ acpi_get_firmware_waking_vector(acpi_phy
 acpi_status
 acpi_get_sleep_type_data(u8 sleep_state, u8 * slp_typ_a, u8 * slp_typ_b);
 
+acpi_status acpi_set_sleep_state_indicator(u8 sleep_state);
+
 acpi_status acpi_enter_sleep_state_prep(u8 sleep_state);
 
 acpi_status asmlinkage acpi_enter_sleep_state(u8 sleep_state);
Index: linux-2.6.23-rc4/include/linux/suspend.h
===================================================================
--- linux-2.6.23-rc4.orig/include/linux/suspend.h
+++ linux-2.6.23-rc4/include/linux/suspend.h
@@ -139,13 +139,19 @@ extern void mark_free_pages(struct zone 
  *
  * @pre_snapshot: Prepare the platform for creating the hibernation image.
  *	Called right after devices have been frozen and before the nonboot
- *	CPUs are disabled (runs with IRQs on).
+ *	CPUs are disabled (runs with interrupts enabled).
  *
- * @finish: Restore the previous state of the platform after the hibernation
- *	image has been created *or* put the platform into the normal operation
- *	mode after the hibernation (the same method is executed in both cases).
- *	Called right after the nonboot CPUs have been enabled and before
- *	thawing devices (runs with IRQs on).
+ * @post_snapshot: Restore the previous state of the platform after creating a
+ *	hibernation image.
+ *	Called when the hibernation image has been created, right after the
+ *	nonboot CPUs have been enabled and before thawing devices (runs with
+ *	interrupts enabled).
+ *
+ * @finish: Put the platform into the normal operation mode after the
+ *	hibernation.
+ *	Called only after @leave() has been executed, right after the nonboot
+ *	CPUs have been enabled and before thawing devices (runs with interrupts
+ *	enabled).
  *
  * @prepare: Prepare the platform for entering the low power state.
  *	Called right after the hibernation image has been saved and before
@@ -173,6 +179,7 @@ extern void mark_free_pages(struct zone 
 struct platform_hibernation_ops {
 	int (*start)(void);
 	int (*pre_snapshot)(void);
+	void (*post_snapshot)(void);
 	void (*finish)(void);
 	int (*prepare)(void);
 	int (*enter)(void);
Index: linux-2.6.23-rc4/kernel/power/disk.c
===================================================================
--- linux-2.6.23-rc4.orig/kernel/power/disk.c
+++ linux-2.6.23-rc4/kernel/power/disk.c
@@ -54,9 +54,9 @@ static struct platform_hibernation_ops *
 
 void hibernation_set_ops(struct platform_hibernation_ops *ops)
 {
-	if (ops && !(ops->start && ops->pre_snapshot && ops->finish
-	    && ops->prepare && ops->enter && ops->leave && ops->pre_restore
-	    && ops->restore_cleanup)) {
+	if (ops && !(ops->start && ops->pre_snapshot && ops->post_snapshot
+	    && ops->finish && ops->prepare && ops->enter && ops->leave
+	    && ops->pre_restore && ops->restore_cleanup)) {
 		WARN_ON(1);
 		return;
 	}
@@ -93,6 +93,18 @@ static int platform_pre_snapshot(int pla
 }
 
 /**
+ *	platform_post_snapshot - switch the machine to the normal mode of
+ *	operation using the platform driver after creating the hibernation
+ *	image
+ */
+
+static void platform_post_snapshot(int platform_mode)
+{
+	if (platform_mode && hibernation_ops)
+		hibernation_ops->post_snapshot();
+}
+
+/**
  *	platform_leave - prepare the machine for switching to the normal mode
  *	of operation using the platform driver (called with interrupts disabled)
  */
@@ -228,7 +240,10 @@ int hibernation_snapshot(int platform_mo
 	}
 	enable_nonboot_cpus();
  Resume_devices:
-	platform_finish(platform_mode);
+	if (!error && !in_suspend)
+		platform_finish(platform_mode);
+	else
+		platform_post_snapshot(platform_mode);
 	device_resume();
  Resume_console:
 	resume_console();

Re: [PATCH -mm 3/3] PM: Improve handling of ACPI system state indicator (rev. 3)

[Rafael J. Wysocki]

Hi,

Since I've just sent a new version of the 2/3 patch, below is the alternatove
version of $subject applying on top of that one.

Greetings,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>

To be able to avoid turning off the sleep state indicator during hibernation
on Thinkpads (currently, it is turned off before saving the image, because we
need to call finish() before unfreezing devices), we need to move the invocation
of the _SST global ACPI method from acpi_enter_sleep_state_prep() to a separate
function, acpi_set_sleep_state_indicator(), that will be called wherever
necessary by higher-level routines.  Also, once acpi_set_sleep_state_indicator()
has been introduced, the execution of _SST can be removed from
acpi_leave_sleep_state_finish().

acpi_set_sleep_state_indicator() should be called by acpi_pm_prepare() as well
as by acpi_pm_finish() and by acpi_hibernation_finish().  Moreover, it is
necessary to introduce a new hibernation callback ->post_snapshot(), that won't
change the state indicator status and will be executed instead of ->finish()
after creating a hibernation image.  Of course, the status indicator should be
set before the hibernation, so the ->pre_snapshot() callback also has to call
acpi_set_sleep_state_indicator() on ACPI systems.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/hardware/hwsleep.c |   51 +++++++++++++++++++++++++++-------------
 drivers/acpi/sleep/main.c       |   39 ++++++++++++++++++++++++------
 include/acpi/acpixf.h           |    2 +
 include/linux/suspend.h         |   19 ++++++++++----
 kernel/power/disk.c             |   23 ++++++++++++++----
 5 files changed, 100 insertions(+), 34 deletions(-)

Index: linux-2.6.23-rc4/drivers/acpi/hardware/hwsleep.c
===================================================================
--- linux-2.6.23-rc4.orig/drivers/acpi/hardware/hwsleep.c
+++ linux-2.6.23-rc4/drivers/acpi/hardware/hwsleep.c
@@ -199,15 +199,46 @@ acpi_status acpi_enter_sleep_state_prep(
 		return_ACPI_STATUS(status);
 	}
 
-	/* Setup the argument to _SST */
+	/* Disable/Clear all GPEs */
+
+	status = acpi_hw_disable_all_gpes();
+
+	return_ACPI_STATUS(status);
+}
 
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_set_sleep_state_indicator
+ *
+ * PARAMETERS:  sleep_state         - Which sleep state to enter
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: Make the system status indicator reflect the sleep state being
+ *              entered.
+ *              This function must execute with interrupts enabled.
+ *
+ ******************************************************************************/
+acpi_status acpi_set_sleep_state_indicator(u8 sleep_state)
+{
+	acpi_status status;
+	struct acpi_object_list arg_list;
+	union acpi_object arg;
+
+	ACPI_FUNCTION_TRACE(acpi_set_sleep_state_indicator);
+
+	arg_list.count = 1;
+	arg_list.pointer = &arg;
+
+	arg.type = ACPI_TYPE_INTEGER;
+
+	/* Setup the argument to _SST */
 	switch (sleep_state) {
 	case ACPI_STATE_S0:
 		arg.integer.value = ACPI_SST_WORKING;
 		break;
 
 	case ACPI_STATE_S1:
-	case ACPI_STATE_S2:
 	case ACPI_STATE_S3:
 		arg.integer.value = ACPI_SST_SLEEPING;
 		break;
@@ -217,27 +248,21 @@ acpi_status acpi_enter_sleep_state_prep(
 		break;
 
 	default:
-		arg.integer.value = ACPI_SST_INDICATOR_OFF;	/* Default is off */
+		/* Default is off */
+		arg.integer.value = ACPI_SST_INDICATOR_OFF;
 		break;
 	}
 
 	/* Set the system indicators to show the desired sleep state. */
-
 	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
 		ACPI_EXCEPTION((AE_INFO, status,
 				"While executing method _SST"));
 	}
 
-	/* Disable/Clear all GPEs */
-
-	status = acpi_hw_disable_all_gpes();
-
 	return_ACPI_STATUS(status);
 }
 
-ACPI_EXPORT_SYMBOL(acpi_enter_sleep_state_prep)
-
 /*******************************************************************************
  *
  * FUNCTION:    acpi_enter_sleep_state
@@ -652,11 +677,5 @@ acpi_status acpi_leave_sleep_state_finis
 	    acpi_set_register(acpi_gbl_fixed_event_info
 			      [ACPI_EVENT_POWER_BUTTON].status_register_id, 1);
 
-	arg.integer.value = ACPI_SST_WORKING;
-	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
-	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
-		ACPI_EXCEPTION((AE_INFO, status, "During Method _SST"));
-	}
-
 	return_ACPI_STATUS(status);
 }
Index: linux-2.6.23-rc4/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.23-rc4.orig/drivers/acpi/sleep/main.c
+++ linux-2.6.23-rc4/drivers/acpi/sleep/main.c
@@ -70,6 +70,8 @@ static int acpi_pm_prepare(void)
 
 	if (error)
 		acpi_target_sleep_state = ACPI_STATE_S0;
+	else
+		acpi_set_sleep_state_indicator(acpi_target_sleep_state);
 
 	return error;
 }
@@ -159,6 +161,7 @@ static void acpi_pm_finish(void)
 	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
 
 	acpi_target_sleep_state = ACPI_STATE_S0;
+	acpi_set_sleep_state_indicator(ACPI_STATE_S0);
 
 #ifdef CONFIG_X86
 	if (init_8259A_after_S1) {
@@ -217,9 +220,33 @@ static struct dmi_system_id __initdata a
 static int acpi_hibernation_start(void)
 {
 	acpi_target_sleep_state = ACPI_STATE_S4;
+
 	return 0;
 }
 
+static int acpi_hibernation_pre_snapshot(void)
+{
+	int error = acpi_sleep_prepare(ACPI_STATE_S4);
+
+	if (error)
+		acpi_target_sleep_state = ACPI_STATE_S0;
+	else
+		acpi_set_sleep_state_indicator(ACPI_STATE_S4);
+
+	return error;
+}
+
+static void acpi_hibernation_post_snapshot(void)
+{
+	acpi_leave_sleep_state_finish(ACPI_STATE_S4);
+	acpi_disable_wakeup_device(ACPI_STATE_S4);
+
+	/* reset firmware waking vector */
+	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
+
+	acpi_target_sleep_state = ACPI_STATE_S0;
+}
+
 static int acpi_hibernation_prepare(void)
 {
 	return acpi_sleep_prepare(ACPI_STATE_S4);
@@ -253,13 +280,8 @@ static void acpi_hibernation_leave(void)
 
 static void acpi_hibernation_finish(void)
 {
-	acpi_leave_sleep_state_finish(ACPI_STATE_S4);
-	acpi_disable_wakeup_device(ACPI_STATE_S4);
-
-	/* reset firmware waking vector */
-	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
-
-	acpi_target_sleep_state = ACPI_STATE_S0;
+	acpi_hibernation_post_snapshot();
+	acpi_set_sleep_state_indicator(ACPI_STATE_S0);
 }
 
 static int acpi_hibernation_pre_restore(void)
@@ -278,7 +300,8 @@ static void acpi_hibernation_restore_cle
 
 static struct platform_hibernation_ops acpi_hibernation_ops = {
 	.start = acpi_hibernation_start,
-	.pre_snapshot = acpi_hibernation_prepare,
+	.pre_snapshot = acpi_hibernation_pre_snapshot,
+	.post_snapshot = acpi_hibernation_post_snapshot,
 	.finish = acpi_hibernation_finish,
 	.prepare = acpi_hibernation_prepare,
 	.enter = acpi_hibernation_enter,
Index: linux-2.6.23-rc4/include/acpi/acpixf.h
===================================================================
--- linux-2.6.23-rc4.orig/include/acpi/acpixf.h
+++ linux-2.6.23-rc4/include/acpi/acpixf.h
@@ -327,6 +327,8 @@ acpi_get_firmware_waking_vector(acpi_phy
 acpi_status
 acpi_get_sleep_type_data(u8 sleep_state, u8 * slp_typ_a, u8 * slp_typ_b);
 
+acpi_status acpi_set_sleep_state_indicator(u8 sleep_state);
+
 acpi_status acpi_enter_sleep_state_prep(u8 sleep_state);
 
 acpi_status asmlinkage acpi_enter_sleep_state(u8 sleep_state);
Index: linux-2.6.23-rc4/include/linux/suspend.h
===================================================================
--- linux-2.6.23-rc4.orig/include/linux/suspend.h
+++ linux-2.6.23-rc4/include/linux/suspend.h
@@ -139,13 +139,19 @@ extern void mark_free_pages(struct zone 
  *
  * @pre_snapshot: Prepare the platform for creating the hibernation image.
  *	Called right after devices have been frozen and before the nonboot
- *	CPUs are disabled (runs with IRQs on).
+ *	CPUs are disabled (runs with interrupts enabled).
  *
- * @finish: Restore the previous state of the platform after the hibernation
- *	image has been created *or* put the platform into the normal operation
- *	mode after the hibernation (the same method is executed in both cases).
- *	Called right after the nonboot CPUs have been enabled and before
- *	thawing devices (runs with IRQs on).
+ * @post_snapshot: Restore the previous state of the platform after creating a
+ *	hibernation image.
+ *	Called when the hibernation image has been created, right after the
+ *	nonboot CPUs have been enabled and before thawing devices (runs with
+ *	interrupts enabled).
+ *
+ * @finish: Put the platform into the normal operation mode after the
+ *	hibernation.
+ *	Called only after @leave() has been executed, right after the nonboot
+ *	CPUs have been enabled and before thawing devices (runs with interrupts
+ *	enabled).
  *
  * @prepare: Prepare the platform for entering the low power state.
  *	Called right after the hibernation image has been saved and before
@@ -173,6 +179,7 @@ extern void mark_free_pages(struct zone 
 struct platform_hibernation_ops {
 	int (*start)(void);
 	int (*pre_snapshot)(void);
+	void (*post_snapshot)(void);
 	void (*finish)(void);
 	int (*prepare)(void);
 	int (*enter)(void);
Index: linux-2.6.23-rc4/kernel/power/disk.c
===================================================================
--- linux-2.6.23-rc4.orig/kernel/power/disk.c
+++ linux-2.6.23-rc4/kernel/power/disk.c
@@ -54,9 +54,9 @@ static struct platform_hibernation_ops *
 
 void hibernation_set_ops(struct platform_hibernation_ops *ops)
 {
-	if (ops && !(ops->start && ops->pre_snapshot && ops->finish
-	    && ops->prepare && ops->enter && ops->leave && ops->pre_restore
-	    && ops->restore_cleanup)) {
+	if (ops && !(ops->start && ops->pre_snapshot && ops->post_snapshot
+	    && ops->finish && ops->prepare && ops->enter && ops->leave
+	    && ops->pre_restore && ops->restore_cleanup)) {
 		WARN_ON(1);
 		return;
 	}
@@ -93,6 +93,18 @@ static int platform_pre_snapshot(int pla
 }
 
 /**
+ *	platform_post_snapshot - switch the machine to the normal mode of
+ *	operation using the platform driver after creating the hibernation
+ *	image
+ */
+
+static void platform_post_snapshot(int platform_mode)
+{
+	if (platform_mode && hibernation_ops)
+		hibernation_ops->post_snapshot();
+}
+
+/**
  *	platform_leave - prepare the machine for switching to the normal mode
  *	of operation using the platform driver (called with interrupts disabled)
  */
@@ -228,7 +240,10 @@ int hibernation_snapshot(int platform_mo
 	}
 	enable_nonboot_cpus();
  Resume_devices:
-	platform_finish(platform_mode);
+	if (!error && !in_suspend)
+		platform_finish(platform_mode);
+	else
+		platform_post_snapshot(platform_mode);
 	device_resume();
  Resume_console:
 	resume_console();

RE: [PATCH -mm 3/3] PM: Improve handling of ACPI system state indicator (rev. 3)

[Moore, Robert]

No, it's not safe to run the AML interpreter with interrupts disabled.

I don't have any problem with introducing finer granularity enter/exit
sleep interfaces if they are required.

I would suggest that we rename things a bit however.

Currently:
acpi_enter_sleep_state_prep
acpi_enter_sleep_state_prep_late
acpi_enter_sleep_state

acpi_leave_sleep_state_prep
acpi_leave_sleep_state

I think we can truncate and clarify:

acpi_sleep_setup1
acpi_sleep_setup2
acpi_sleep

acpi_wake_setup1
acpi_wake


acpi_set_sleep_state_indicator:

I'm not sure if we have any external interfaces that simply execute a
control method, seems like overkill.

Please give me more information as to why _SSI needs to be moved (other
than executing it after _BFS)

Thanks,
Bob





-----Original Message-----
From: Rafael J. Wysocki [mailto:rjw@sisk.pl] 
Sent: Tuesday, August 28, 2007 3:05 PM
To: Moore, Robert
Cc: Andrew Morton; ACPI Devel Maling List; Len Brown; LKML; Pavel
Machek; pm list
Subject: Re: [PATCH -mm 3/3] PM: Improve handling of ACPI system state
indicator (rev. 3)

On Tuesday, 28 August 2007 21:57, Moore, Robert wrote:
> Since these changes appear to affect the ACPICA core in a fairly big
> way, I would like to see a short, concise description of each change
and
> why it is necessary.

All right.  I'll describe the changes made by the current version of the
patches, but please note that if it's safe to run the AML interpreter
with
IRQs disabled, it's better to do some of them in a different way.

1. Remove the execution of _GTS from acpi_enter_sleep_state_prep()

	acpi_enter_sleep_state_prep() is called before disabling the
nonboot
	CPUs and _GTS should be executed after that, according to the
spec.

2. Introduce acpi_enter_sleep_state_prep_late() that will execute _GTS

	Necessary because of 1.

3. Split acpi_leave_sleep_state() into two functions:
   acpi_leave_sleep_state_prep() and acpi_leave_sleep_state().

	acpi_leave_sleep_state_prep() contains the code that should be
executed
	before enabling the nonboot CPUs, most importantly the execution
of
	_BFS, and acpi_leave_sleep_state() contains the remaining code
(the
	enabling of GPEs, the execution of _WAK and the enabling of
power
	buttons)

4. Change the code ordering in acpi_leave_sleep_state_prep() (introduced
   in 3.) so that _SST is executed after _BFS

	According to the spec, _BFS should be the first ACPI method
executed
	after leaving a sleep state

5. Introduce acpi_set_sleep_state_indicator() that will execute _SST for
given
   ACPI sleep state

	Needed so that we can set the state indicator independently of
the
	other lower-level operations.

6. Remove the execution of _SST from acpi_leave_sleep_state()

	No longer needed, because we can use
acpi_set_sleep_state_indicator()
	to set the state indicator appropriately from higher level
routines.

The other changes affect only drivers/acpi/sleep/main.c and the files in
kernel/power .

Greetings,
Rafael

RE: [PATCH -mm 3/3] PM: Improve handling of ACPI system state indicator (rev. 3)

[Moore, Robert]

No, it's not safe to run the AML interpreter with interrupts disabled.

I don't have any problem with introducing finer granularity enter/exit
sleep interfaces if they are required.

I would suggest that we rename things a bit however.

Currently:
acpi_enter_sleep_state_prep
acpi_enter_sleep_state_prep_late
acpi_enter_sleep_state

acpi_leave_sleep_state_prep
acpi_leave_sleep_state

I think we can truncate and clarify:

acpi_sleep_setup1
acpi_sleep_setup2
acpi_sleep

acpi_wake_setup1
acpi_wake


acpi_set_sleep_state_indicator:

I'm not sure if we have any external interfaces that simply execute a
control method, seems like overkill.

Please give me more information as to why _SSI needs to be moved (other
than executing it after _BFS)

Thanks,
Bob





-----Original Message-----
From: Rafael J. Wysocki [mailto:rjw@sisk.pl] 
Sent: Tuesday, August 28, 2007 3:05 PM
To: Moore, Robert
Cc: Andrew Morton; ACPI Devel Maling List; Len Brown; LKML; Pavel
Machek; pm list
Subject: Re: [PATCH -mm 3/3] PM: Improve handling of ACPI system state
indicator (rev. 3)

On Tuesday, 28 August 2007 21:57, Moore, Robert wrote:
> Since these changes appear to affect the ACPICA core in a fairly big
> way, I would like to see a short, concise description of each change
and
> why it is necessary.

All right.  I'll describe the changes made by the current version of the
patches, but please note that if it's safe to run the AML interpreter
with
IRQs disabled, it's better to do some of them in a different way.

1. Remove the execution of _GTS from acpi_enter_sleep_state_prep()

	acpi_enter_sleep_state_prep() is called before disabling the
nonboot
	CPUs and _GTS should be executed after that, according to the
spec.

2. Introduce acpi_enter_sleep_state_prep_late() that will execute _GTS

	Necessary because of 1.

3. Split acpi_leave_sleep_state() into two functions:
   acpi_leave_sleep_state_prep() and acpi_leave_sleep_state().

	acpi_leave_sleep_state_prep() contains the code that should be
executed
	before enabling the nonboot CPUs, most importantly the execution
of
	_BFS, and acpi_leave_sleep_state() contains the remaining code
(the
	enabling of GPEs, the execution of _WAK and the enabling of
power
	buttons)

4. Change the code ordering in acpi_leave_sleep_state_prep() (introduced
   in 3.) so that _SST is executed after _BFS

	According to the spec, _BFS should be the first ACPI method
executed
	after leaving a sleep state

5. Introduce acpi_set_sleep_state_indicator() that will execute _SST for
given
   ACPI sleep state

	Needed so that we can set the state indicator independently of
the
	other lower-level operations.

6. Remove the execution of _SST from acpi_leave_sleep_state()

	No longer needed, because we can use
acpi_set_sleep_state_indicator()
	to set the state indicator appropriately from higher level
routines.

The other changes affect only drivers/acpi/sleep/main.c and the files in
kernel/power .

Greetings,
Rafael

Re: [PATCH -mm 3/3] PM: Improve handling of ACPI system state indicator (rev. 3)

[Rafael J. Wysocki]

On Wednesday, 29 August 2007 18:54, Moore, Robert wrote:
> No, it's not safe to run the AML interpreter with interrupts disabled.

OK

> I don't have any problem with introducing finer granularity enter/exit
> sleep interfaces if they are required.
> 
> I would suggest that we rename things a bit however.
> 
> Currently:
> acpi_enter_sleep_state_prep
> acpi_enter_sleep_state_prep_late
> acpi_enter_sleep_state
> 
> acpi_leave_sleep_state_prep
> acpi_leave_sleep_state
> 
> I think we can truncate and clarify:
> 
> acpi_sleep_setup1
> acpi_sleep_setup2
> acpi_sleep
> 
> acpi_wake_setup1
> acpi_wake

That's perfectly fine by me.  I'll update the 2/3 patch to use these names.

Also, I think we can remove acpi_enter_sleep_state_s4bios() entirely (in a
separate patch).

> acpi_set_sleep_state_indicator:
> 
> I'm not sure if we have any external interfaces that simply execute a
> control method, seems like overkill.
> 
> Please give me more information as to why _SSI needs to be moved (other
> than executing it after _BFS)

The _SST after _BFS is okay, but the invocation of _SST in acpi_wake()
(currently acpi_leave_sleep_state) is problematic, since it causes the
indicator to be set to "working" during hibernation, before the image is
saved.  Thus, during hibernation _SST shouldn't be called from acpi_wake().

For this reason, I thought it would be a good idea to call _SST from a separate
routine that might be invoked by higher level functions as desired.

Greetings,
Rafael

Re: [PATCH -mm 3/3] PM: Improve handling of ACPI system state indicator (rev. 3)

[Rafael J. Wysocki]

On Wednesday, 29 August 2007 18:54, Moore, Robert wrote:
> No, it's not safe to run the AML interpreter with interrupts disabled.

OK

> I don't have any problem with introducing finer granularity enter/exit
> sleep interfaces if they are required.
> 
> I would suggest that we rename things a bit however.
> 
> Currently:
> acpi_enter_sleep_state_prep
> acpi_enter_sleep_state_prep_late
> acpi_enter_sleep_state
> 
> acpi_leave_sleep_state_prep
> acpi_leave_sleep_state
> 
> I think we can truncate and clarify:
> 
> acpi_sleep_setup1
> acpi_sleep_setup2
> acpi_sleep
> 
> acpi_wake_setup1
> acpi_wake

That's perfectly fine by me.  I'll update the 2/3 patch to use these names.

Also, I think we can remove acpi_enter_sleep_state_s4bios() entirely (in a
separate patch).

> acpi_set_sleep_state_indicator:
> 
> I'm not sure if we have any external interfaces that simply execute a
> control method, seems like overkill.
> 
> Please give me more information as to why _SSI needs to be moved (other
> than executing it after _BFS)

The _SST after _BFS is okay, but the invocation of _SST in acpi_wake()
(currently acpi_leave_sleep_state) is problematic, since it causes the
indicator to be set to "working" during hibernation, before the image is
saved.  Thus, during hibernation _SST shouldn't be called from acpi_wake().

For this reason, I thought it would be a good idea to call _SST from a separate
routine that might be invoked by higher level functions as desired.

Greetings,
Rafael

Re: [linux-pm] [PATCH -mm 0/3] PM: Improve ACPI handling during suspend and hibernation (rev. 3)

[Rafael J. Wysocki]

On Monday, 27 August 2007 23:47, Rafael J. Wysocki wrote:
> Hi,
> 
> The patches in this series are intended to improve the handling of the ACPI
> platform during suspend and hibernation.
> 
> They do the following things:
> * make hibernation_platform_enter() consistent with the sleep state entering
>   code in kernel/power/main.c
> * introduce global platform callbacks allowing us to handle ACPI in a more fine
>   grained way during suspend and hibernation
> * improve the handling of the system status indicator during suspend and
>   hibernation.

Please regard this patch series as withdrawn.

I have found a corner case that requires special treatment clashing with the
2/3 and 3/3 patches and the 1/3 patch will be resubmitted separately.

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

Re: [linux-pm] [PATCH -mm 0/3] PM: Improve ACPI handling during suspend and hibernation (rev. 3)

[Rafael J. Wysocki]

On Monday, 27 August 2007 23:47, Rafael J. Wysocki wrote:
> Hi,
> 
> The patches in this series are intended to improve the handling of the ACPI
> platform during suspend and hibernation.
> 
> They do the following things:
> * make hibernation_platform_enter() consistent with the sleep state entering
>   code in kernel/power/main.c
> * introduce global platform callbacks allowing us to handle ACPI in a more fine
>   grained way during suspend and hibernation
> * improve the handling of the system status indicator during suspend and
>   hibernation.

Please regard this patch series as withdrawn.

I have found a corner case that requires special treatment clashing with the
2/3 and 3/3 patches and the 1/3 patch will be resubmitted separately.

Greetings,
Rafael

Re: [PATCH -mm 0/3] PM: Improve ACPI handling during suspend and hibernation (rev. 3)

[Rafael J. Wysocki]

On Monday, 27 August 2007 23:47, Rafael J. Wysocki wrote:
> Hi,
> 
> The patches in this series are intended to improve the handling of the ACPI
> platform during suspend and hibernation.
> 
> They do the following things:
> * make hibernation_platform_enter() consistent with the sleep state entering
>   code in kernel/power/main.c
> * introduce global platform callbacks allowing us to handle ACPI in a more fine
>   grained way during suspend and hibernation
> * improve the handling of the system status indicator during suspend and
>   hibernation.

Please regard this patch series as withdrawn.

I have found a corner case that requires special treatment clashing with the
2/3 and 3/3 patches and the 1/3 patch will be resubmitted separately.

Greetings,
Rafael