[RFC][PATCH 0/4] swsusp: Fix hibernation/restore code ordering

[RFC][PATCH 0/4] swsusp: Fix hibernation/restore code ordering

[Rafael J. Wysocki]

Hi,

In the face of this change:

http://marc.info/?l=linux-acpi&m=117938245931603&w=2

I think we should also modify the code ordering in swsusp so that
hibernation_ops->prepare() is executed after device_suspend().

However, for this purpose it seems reasonable to eliminate the code duplication
between kernel/power/disk.c and kernel/power/user.c first.  By eliminating it
we can reduce the size of user.c quite substantially and remove the
maintenance difficulty with making essentially the same changes in two
different places.

Moreover, we should also remove the calls to "platform" functions from the
restore code path, since it doesn't carry out any power transition of the
system, but we generally need to disable the GPEs before the restore if the
'platform' hibernation mode has been used.  To do this, we can introduce
two new hibernation_ops to be used in the restore code.

Comments welcome.

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

[RFC][PATCH 1/4] swsusp: Remove incorrect code from user.c

[Rafael J. Wysocki]

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

Make the code hibernation code in kernel/power/user.c be functionally equivalent
to the corresponding code in kernel/power/disk.c , as it should be.

The calls to the platform functions removed by this patch are incorrect.  They
should be replaced with some other "platform" operations that will be
introduced in one of the subsequent patches.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/user.c |   17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

Index: linux-2.6.21/kernel/power/user.c
===================================================================
--- linux-2.6.21.orig/kernel/power/user.c	2007-05-08 22:10:36.000000000 +0200
+++ linux-2.6.21/kernel/power/user.c	2007-05-08 23:03:17.000000000 +0200
@@ -179,34 +179,25 @@ static inline int snapshot_suspend(int p
 	return error;
 }
 
-static inline int snapshot_restore(int platform_suspend)
+static inline int snapshot_restore(void)
 {
 	int error;
 
 	mutex_lock(&pm_mutex);
 	pm_prepare_console();
-	if (platform_suspend) {
-		error = platform_prepare();
-		if (error)
-			goto Finish;
-	}
 	suspend_console();
 	error = device_suspend(PMSG_PRETHAW);
 	if (error)
-		goto Resume_devices;
+		goto Finish;
 
 	error = disable_nonboot_cpus();
 	if (!error)
 		error = swsusp_resume();
 
 	enable_nonboot_cpus();
- Resume_devices:
-	if (platform_suspend)
-		platform_finish();
-
+ Finish:
 	device_resume();
 	resume_console();
- Finish:
 	pm_restore_console();
 	mutex_unlock(&pm_mutex);
 	return error;
@@ -272,7 +263,7 @@ static int snapshot_ioctl(struct inode *
 			error = -EPERM;
 			break;
 		}
-		error = snapshot_restore(data->platform_suspend);
+		error = snapshot_restore();
 		break;
 
 	case SNAPSHOT_FREE:

Re: [RFC][PATCH 1/4] swsusp: Remove incorrect code from user.c

[Pavel Machek]

On Fri 2007-05-18 00:21:21, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Make the code hibernation code in kernel/power/user.c be functionally equivalent
> to the corresponding code in kernel/power/disk.c , as it should be.
> 
> The calls to the platform functions removed by this patch are incorrect.  They
> should be replaced with some other "platform" operations that will be
> introduced in one of the subsequent patches.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

ACK.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[RFC][PATCH 2/4] swsusp: Remove code duplication between disk.c and user.c

[Rafael J. Wysocki]

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

Currently, much of the code in kernel/power/disk.c is duplicated in
kernel/power/user.c , mainly for historical reasons.  By eliminating this code
duplication we can reduce the size of user.c quite substantially and remove the
maintenance difficulty resulting from it.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/disk.c  |  184 ++++++++++++++++++++++++++++-----------------------
 kernel/power/power.h |    5 +
 kernel/power/user.c  |   96 +-------------------------
 3 files changed, 115 insertions(+), 170 deletions(-)

Index: linux-2.6.22-rc1/kernel/power/disk.c
===================================================================
--- linux-2.6.22-rc1.orig/kernel/power/disk.c	2007-05-13 13:48:49.000000000 +0200
+++ linux-2.6.22-rc1/kernel/power/disk.c	2007-05-13 13:50:42.000000000 +0200
@@ -74,9 +74,9 @@ void hibernation_set_ops(struct hibernat
  *	platform driver if so configured and return an error code if it fails
  */
 
-static int platform_prepare(void)
+static int platform_prepare(int platform_mode)
 {
-	return (hibernation_mode == HIBERNATION_PLATFORM && hibernation_ops) ?
+	return (platform_mode && hibernation_ops) ?
 		hibernation_ops->prepare() : 0;
 }
 
@@ -85,20 +85,111 @@ static int platform_prepare(void)
  *	using the platform driver (must be called after platform_prepare())
  */
 
-static void platform_finish(void)
+static void platform_finish(int platform_mode)
 {
-	if (hibernation_mode == HIBERNATION_PLATFORM && hibernation_ops)
+	if (platform_mode && hibernation_ops)
 		hibernation_ops->finish();
 }
 
 /**
+ *	hibernation_snapshot - quiesce devices and create the hibernation
+ *	snapshot image.
+ *	@platform_mode - if set, use the platform driver, if available, to
+ *			 prepare the platform frimware for the power transition.
+ *
+ *	Must be called with pm_mutex held
+ */
+
+int hibernation_snapshot(int platform_mode)
+{
+	int error;
+
+	/* Free memory before shutting down devices. */
+	error = swsusp_shrink_memory();
+	if (error)
+		goto Finish;
+
+	error = platform_prepare(platform_mode);
+	if (error)
+		goto Finish;
+
+	suspend_console();
+	error = device_suspend(PMSG_FREEZE);
+	if (error)
+		goto Resume_devices;
+
+	error = disable_nonboot_cpus();
+	if (!error) {
+		if (hibernation_mode != HIBERNATION_TEST) {
+			in_suspend = 1;
+			error = swsusp_suspend();
+			/* Control returns here after successful restore */
+		} else {
+			printk("swsusp debug: Waiting for 5 seconds.\n");
+			mdelay(5000);
+		}
+	}
+	enable_nonboot_cpus();
+ Resume_devices:
+	platform_finish(platform_mode);
+	device_resume();
+	resume_console();
+ Finish:
+	return error;
+}
+
+/**
+ *	hibernation_restore - quiesce devices and restore the hibernation
+ *	snapshot image.  If successful, control returns in hibernation_snaphot()
+ *
+ *	Must be called with pm_mutex held
+ */
+
+int hibernation_restore(void)
+{
+	int error;
+
+	pm_prepare_console();
+	suspend_console();
+	error = device_suspend(PMSG_PRETHAW);
+	if (error)
+		goto Finish;
+
+	error = disable_nonboot_cpus();
+	if (!error)
+		error = swsusp_resume();
+
+	enable_nonboot_cpus();
+ Finish:
+	device_resume();
+	resume_console();
+	pm_restore_console();
+	return error;
+}
+
+/**
+ *	hibernation_platform_enter - enter the hibernation state using the
+ *	platform driver (if available)
+ */
+
+int hibernation_platform_enter(void)
+{
+	if (hibernation_ops) {
+		kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
+		return hibernation_ops->enter();
+	} else {
+		return -ENOSYS;
+	}
+}
+
+/**
  *	power_down - Shut the machine down for hibernation.
  *
  *	Use the platform driver, if configured so; otherwise try
  *	to power off or reboot.
  */
 
-static void power_down(void)
+void power_down(void)
 {
 	switch (hibernation_mode) {
 	case HIBERNATION_TEST:
@@ -111,11 +202,7 @@ static void power_down(void)
 		kernel_restart(NULL);
 		break;
 	case HIBERNATION_PLATFORM:
-		if (hibernation_ops) {
-			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
-			hibernation_ops->enter();
-			break;
-		}
+		hibernation_platform_enter();
 	}
 	kernel_halt();
 	/*
@@ -171,62 +258,17 @@ int hibernate(void)
 		mdelay(5000);
 		goto Thaw;
 	}
-
-	/* Free memory before shutting down devices. */
-	error = swsusp_shrink_memory();
-	if (error)
-		goto Thaw;
-
-	error = platform_prepare();
-	if (error)
-		goto Thaw;
-
-	suspend_console();
-	error = device_suspend(PMSG_FREEZE);
-	if (error) {
-		printk(KERN_ERR "PM: Some devices failed to suspend\n");
-		goto Resume_devices;
-	}
-	error = disable_nonboot_cpus();
-	if (error)
-		goto Enable_cpus;
-
-	if (hibernation_mode == HIBERNATION_TEST) {
-		printk("swsusp debug: Waiting for 5 seconds.\n");
-		mdelay(5000);
-		goto Enable_cpus;
-	}
-
-	pr_debug("PM: snapshotting memory.\n");
-	in_suspend = 1;
-	error = swsusp_suspend();
-	if (error)
-		goto Enable_cpus;
-
-	if (in_suspend) {
-		enable_nonboot_cpus();
-		platform_finish();
-		device_resume();
-		resume_console();
+	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
+	if (in_suspend && !error) {
 		pr_debug("PM: writing image.\n");
 		error = swsusp_write();
+		swsusp_free();
 		if (!error)
 			power_down();
-		else {
-			swsusp_free();
-			goto Thaw;
-		}
 	} else {
 		pr_debug("PM: Image restored successfully.\n");
+		swsusp_free();
 	}
-
-	swsusp_free();
- Enable_cpus:
-	enable_nonboot_cpus();
- Resume_devices:
-	platform_finish();
-	device_resume();
-	resume_console();
  Thaw:
 	mutex_unlock(&pm_mutex);
 	unprepare_processes();
@@ -301,29 +343,11 @@ static int software_resume(void)
 	pr_debug("PM: Reading swsusp image.\n");
 
 	error = swsusp_read();
-	if (error) {
-		swsusp_free();
-		goto Thaw;
-	}
-
-	pr_debug("PM: Preparing devices for restore.\n");
-
-	suspend_console();
-	error = device_suspend(PMSG_PRETHAW);
-	if (error)
-		goto Free;
-
-	error = disable_nonboot_cpus();
 	if (!error)
-		swsusp_resume();
+		hibernation_restore();
 
-	enable_nonboot_cpus();
- Free:
-	swsusp_free();
-	device_resume();
-	resume_console();
- Thaw:
 	printk(KERN_ERR "PM: Restore failed, recovering.\n");
+	swsusp_free();
 	unprepare_processes();
  Done:
 	free_basic_memory_bitmaps();
@@ -333,7 +357,7 @@ static int software_resume(void)
  Unlock:
 	mutex_unlock(&pm_mutex);
 	pr_debug("PM: Resume from disk failed.\n");
-	return 0;
+	return error;
 }
 
 late_initcall(software_resume);
Index: linux-2.6.22-rc1/kernel/power/power.h
===================================================================
--- linux-2.6.22-rc1.orig/kernel/power/power.h	2007-05-13 12:23:27.000000000 +0200
+++ linux-2.6.22-rc1/kernel/power/power.h	2007-05-13 13:50:42.000000000 +0200
@@ -25,7 +25,10 @@ struct swsusp_info {
  */
 #define SPARE_PAGES	((1024 * 1024) >> PAGE_SHIFT)
 
-extern struct hibernation_ops *hibernation_ops;
+/* kernel/power/disk.c */
+extern int hibernation_snapshot(int platform_mode);
+extern int hibernation_restore(void);
+extern int hibernation_platform_enter(void);
 #endif
 
 extern int pfn_is_nosave(unsigned long);
Index: linux-2.6.22-rc1/kernel/power/user.c
===================================================================
--- linux-2.6.22-rc1.orig/kernel/power/user.c	2007-05-13 13:50:01.000000000 +0200
+++ linux-2.6.22-rc1/kernel/power/user.c	2007-05-13 13:50:42.000000000 +0200
@@ -126,83 +126,6 @@ static ssize_t snapshot_write(struct fil
 	return res;
 }
 
-static inline int platform_prepare(void)
-{
-	int error = 0;
-
-	if (hibernation_ops)
-		error = hibernation_ops->prepare();
-
-	return error;
-}
-
-static inline void platform_finish(void)
-{
-	if (hibernation_ops)
-		hibernation_ops->finish();
-}
-
-static inline int snapshot_suspend(int platform_suspend)
-{
-	int error;
-
-	mutex_lock(&pm_mutex);
-	/* Free memory before shutting down devices. */
-	error = swsusp_shrink_memory();
-	if (error)
-		goto Finish;
-
-	if (platform_suspend) {
-		error = platform_prepare();
-		if (error)
-			goto Finish;
-	}
-	suspend_console();
-	error = device_suspend(PMSG_FREEZE);
-	if (error)
-		goto Resume_devices;
-
-	error = disable_nonboot_cpus();
-	if (!error) {
-		in_suspend = 1;
-		error = swsusp_suspend();
-	}
-	enable_nonboot_cpus();
- Resume_devices:
-	if (platform_suspend)
-		platform_finish();
-
-	device_resume();
-	resume_console();
- Finish:
-	mutex_unlock(&pm_mutex);
-	return error;
-}
-
-static inline int snapshot_restore(void)
-{
-	int error;
-
-	mutex_lock(&pm_mutex);
-	pm_prepare_console();
-	suspend_console();
-	error = device_suspend(PMSG_PRETHAW);
-	if (error)
-		goto Finish;
-
-	error = disable_nonboot_cpus();
-	if (!error)
-		error = swsusp_resume();
-
-	enable_nonboot_cpus();
- Finish:
-	device_resume();
-	resume_console();
-	pm_restore_console();
-	mutex_unlock(&pm_mutex);
-	return error;
-}
-
 static int snapshot_ioctl(struct inode *inode, struct file *filp,
                           unsigned int cmd, unsigned long arg)
 {
@@ -249,7 +172,7 @@ static int snapshot_ioctl(struct inode *
 			error = -EPERM;
 			break;
 		}
-		error = snapshot_suspend(data->platform_suspend);
+		error = hibernation_snapshot(data->platform_suspend);
 		if (!error)
 			error = put_user(in_suspend, (unsigned int __user *)arg);
 		if (!error)
@@ -263,7 +186,7 @@ static int snapshot_ioctl(struct inode *
 			error = -EPERM;
 			break;
 		}
-		error = snapshot_restore();
+		error = hibernation_restore();
 		break;
 
 	case SNAPSHOT_FREE:
@@ -375,19 +298,14 @@ static int snapshot_ioctl(struct inode *
 		switch (arg) {
 
 		case PMOPS_PREPARE:
-			if (hibernation_ops) {
-				data->platform_suspend = 1;
-				error = 0;
-			} else {
-				error = -ENOSYS;
-			}
+			data->platform_suspend = 1;
+			error = 0;
 			break;
 
 		case PMOPS_ENTER:
-			if (data->platform_suspend) {
-				kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
-				error = hibernation_ops->enter();
-			}
+			if (data->platform_suspend)
+				error = hibernation_platform_enter();
+
 			break;
 
 		case PMOPS_FINISH:

Re: [RFC][PATCH 2/4] swsusp: Remove code duplication between disk.c and user.c

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Currently, much of the code in kernel/power/disk.c is duplicated in
> kernel/power/user.c , mainly for historical reasons.  By eliminating this code
> duplication we can reduce the size of user.c quite substantially and remove the
> maintenance difficulty resulting from it.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

ACK.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[RFC][PATCH 3/4] swsusp: Introduce restore platform operations

[Rafael J. Wysocki]

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

At least on some machines it is necessary to prepare the ACPI firmware for the
restoration of the system memory state from the hibernation image if the
"platform" mode of hibernation has been used.  Namely, in that cases we need to
disable the GPEs before replacing the "boot" kernel with the "frozen" kernel.
After the restore they will be re-enabled by hibernation_ops->finish(), but if
the restore fails, they have to be re-enabled by the restore code explicitly.

For this purpose we can introduce two additional hibernation operations, called
pre_restore() and restore_cleanup() and call them from the restore code path.
Still, they should be called if the "platform" mode of hibernation has been
used, so we need to pass the information about the hibernation mode from the
"frozen" kernel to the "boot" kernel in the image header.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/sleep/main.c |   16 +++++++++++++
 include/linux/suspend.h   |    4 +++
 kernel/power/disk.c       |   56 +++++++++++++++++++++++++++++++++++++---------
 kernel/power/power.h      |   13 ++++++++--
 kernel/power/swap.c       |   20 ++++++++++++----
 kernel/power/user.c       |    2 -
 6 files changed, 92 insertions(+), 19 deletions(-)

Index: linux-2.6.22-rc1/kernel/power/disk.c
===================================================================
--- linux-2.6.22-rc1.orig/kernel/power/disk.c	2007-05-17 21:36:45.000000000 +0200
+++ linux-2.6.22-rc1/kernel/power/disk.c	2007-05-17 23:19:24.000000000 +0200
@@ -54,7 +54,8 @@ struct hibernation_ops *hibernation_ops;
 
 void hibernation_set_ops(struct hibernation_ops *ops)
 {
-	if (ops && !(ops->prepare && ops->enter && ops->finish)) {
+	if (ops && !(ops->prepare && ops->enter && ops->finish
+	    && ops->pre_restore && ops->restore_cleanup)) {
 		WARN_ON(1);
 		return;
 	}
@@ -92,6 +93,31 @@ static void platform_finish(int platform
 }
 
 /**
+ *	platform_pre_restore - prepare the platform for the restoration from a
+ *	hibernation image.  If the restore fails after this function has been
+ *	called, platform_restore_cleanup() must be called.
+ */
+
+static int platform_pre_restore(int platform_mode)
+{
+	return (platform_mode && hibernation_ops) ?
+		hibernation_ops->pre_restore() : 0;
+}
+
+/**
+ *	platform_restore_cleanup - switch the platform to the normal mode of
+ *	operation after a failing restore.  If platform_pre_restore() has been
+ *	called before the failing restore, this function must be called too,
+ *	regardless of the result of platform_pre_restore().
+ */
+
+static void platform_restore_cleanup(int platform_mode)
+{
+	if (platform_mode && hibernation_ops)
+		hibernation_ops->restore_cleanup();
+}
+
+/**
  *	hibernation_snapshot - quiesce devices and create the hibernation
  *	snapshot image.
  *	@platform_mode - if set, use the platform driver, if available, to
@@ -141,11 +167,13 @@ int hibernation_snapshot(int platform_mo
 /**
  *	hibernation_restore - quiesce devices and restore the hibernation
  *	snapshot image.  If successful, control returns in hibernation_snaphot()
+ *	@platform_mode - if set, use the platform driver, if available, to
+ *			 prepare the platform frimware for the transition.
  *
  *	Must be called with pm_mutex held
  */
 
-int hibernation_restore(void)
+int hibernation_restore(int platform_mode)
 {
 	int error;
 
@@ -155,11 +183,14 @@ int hibernation_restore(void)
 	if (error)
 		goto Finish;
 
-	error = disable_nonboot_cpus();
-	if (!error)
-		error = swsusp_resume();
-
-	enable_nonboot_cpus();
+	error = platform_pre_restore(platform_mode);
+	if (!error) {
+		error = disable_nonboot_cpus();
+		if (!error)
+			error = swsusp_resume();
+		enable_nonboot_cpus();
+	}
+	platform_restore_cleanup(platform_mode);
  Finish:
 	device_resume();
 	resume_console();
@@ -260,8 +291,12 @@ int hibernate(void)
 	}
 	error = hibernation_snapshot(hibernation_mode == HIBERNATION_PLATFORM);
 	if (in_suspend && !error) {
+		unsigned int flags = 0;
+
+		if (hibernation_mode == HIBERNATION_PLATFORM)
+			flags |= SF_PLATFORM_MODE;
 		pr_debug("PM: writing image.\n");
-		error = swsusp_write();
+		error = swsusp_write(flags);
 		swsusp_free();
 		if (!error)
 			power_down();
@@ -295,6 +330,7 @@ int hibernate(void)
 static int software_resume(void)
 {
 	int error;
+	unsigned int flags;
 
 	mutex_lock(&pm_mutex);
 	if (!swsusp_resume_device) {
@@ -342,9 +378,9 @@ static int software_resume(void)
 
 	pr_debug("PM: Reading swsusp image.\n");
 
-	error = swsusp_read();
+	error = swsusp_read(&flags);
 	if (!error)
-		hibernation_restore();
+		hibernation_restore(flags & SF_PLATFORM_MODE);
 
 	printk(KERN_ERR "PM: Restore failed, recovering.\n");
 	swsusp_free();
Index: linux-2.6.22-rc1/kernel/power/power.h
===================================================================
--- linux-2.6.22-rc1.orig/kernel/power/power.h	2007-05-13 13:51:45.000000000 +0200
+++ linux-2.6.22-rc1/kernel/power/power.h	2007-05-17 22:29:18.000000000 +0200
@@ -27,7 +27,7 @@ struct swsusp_info {
 
 /* kernel/power/disk.c */
 extern int hibernation_snapshot(int platform_mode);
-extern int hibernation_restore(void);
+extern int hibernation_restore(int platform_mode);
 extern int hibernation_platform_enter(void);
 #endif
 
@@ -155,13 +155,20 @@ extern sector_t alloc_swapdev_block(int 
 extern void free_all_swap_pages(int swap);
 extern int swsusp_swap_in_use(void);
 
+/*
+ * Flags that can be passed from the hibernatig hernel to the "boot" kernel in
+ * the image header.
+ */
+#define SF_PLATFORM_MODE	1
+
+/* kernel/power/disk.c */
 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(void);
-extern int swsusp_write(void);
+extern int swsusp_read(unsigned int *flags_p);
+extern int swsusp_write(unsigned int flags);
 extern void swsusp_close(void);
 extern int suspend_enter(suspend_state_t state);
 
Index: linux-2.6.22-rc1/kernel/power/swap.c
===================================================================
--- linux-2.6.22-rc1.orig/kernel/power/swap.c	2007-05-13 12:23:27.000000000 +0200
+++ linux-2.6.22-rc1/kernel/power/swap.c	2007-05-17 22:30:21.000000000 +0200
@@ -33,8 +33,9 @@ extern char resume_file[];
 #define SWSUSP_SIG	"S1SUSPEND"
 
 struct swsusp_header {
-	char reserved[PAGE_SIZE - 20 - sizeof(sector_t)];
+	char reserved[PAGE_SIZE - 20 - sizeof(sector_t) - sizeof(int)];
 	sector_t image;
+	unsigned int flags;	/* Flags to pass to the "boot" kernel */
 	char	orig_sig[10];
 	char	sig[10];
 } __attribute__((packed));
@@ -138,7 +139,7 @@ static int wait_on_bio_chain(struct bio 
  * Saving part
  */
 
-static int mark_swapfiles(sector_t start)
+static int mark_swapfiles(sector_t start, unsigned int flags)
 {
 	int error;
 
@@ -148,6 +149,7 @@ static int mark_swapfiles(sector_t start
 		memcpy(swsusp_header->orig_sig,swsusp_header->sig, 10);
 		memcpy(swsusp_header->sig,SWSUSP_SIG, 10);
 		swsusp_header->image = start;
+		swsusp_header->flags = flags;
 		error = bio_write_page(swsusp_resume_block,
 					swsusp_header, NULL);
 	} else {
@@ -369,6 +371,7 @@ static int enough_swap(unsigned int nr_p
 
 /**
  *	swsusp_write - Write entire image and metadata.
+ *	@flags: flags to pass to the "boot" kernel in the image header
  *
  *	It is important _NOT_ to umount filesystems at this point. We want
  *	them synced (in case something goes wrong) but we DO not want to mark
@@ -376,7 +379,7 @@ static int enough_swap(unsigned int nr_p
  *	correctly, we'll mark system clean, anyway.)
  */
 
-int swsusp_write(void)
+int swsusp_write(unsigned int flags)
 {
 	struct swap_map_handle handle;
 	struct snapshot_handle snapshot;
@@ -415,7 +418,7 @@ int swsusp_write(void)
 		if (!error) {
 			flush_swap_writer(&handle);
 			printk("S");
-			error = mark_swapfiles(start);
+			error = mark_swapfiles(start, flags);
 			printk("|\n");
 		}
 	}
@@ -540,13 +543,20 @@ static int load_image(struct swap_map_ha
 	return error;
 }
 
-int swsusp_read(void)
+/**
+ *	swsusp_read - read the hibernation image.
+ *	@flags_p: flags passed by the "frozen" kernel in the image header should
+ *		  be written into this memeory location
+ */
+
+int swsusp_read(unsigned int *flags_p)
 {
 	int error;
 	struct swap_map_handle handle;
 	struct snapshot_handle snapshot;
 	struct swsusp_info *header;
 
+	*flags_p = swsusp_header->flags;
 	if (IS_ERR(resume_bdev)) {
 		pr_debug("swsusp: block device not initialised\n");
 		return PTR_ERR(resume_bdev);
Index: linux-2.6.22-rc1/kernel/power/user.c
===================================================================
--- linux-2.6.22-rc1.orig/kernel/power/user.c	2007-05-13 13:51:45.000000000 +0200
+++ linux-2.6.22-rc1/kernel/power/user.c	2007-05-17 22:00:08.000000000 +0200
@@ -186,7 +186,7 @@ static int snapshot_ioctl(struct inode *
 			error = -EPERM;
 			break;
 		}
-		error = hibernation_restore();
+		error = hibernation_restore(data->platform_suspend);
 		break;
 
 	case SNAPSHOT_FREE:
Index: linux-2.6.22-rc1/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.22-rc1.orig/drivers/acpi/sleep/main.c	2007-05-17 21:36:45.000000000 +0200
+++ linux-2.6.22-rc1/drivers/acpi/sleep/main.c	2007-05-17 22:41:44.000000000 +0200
@@ -217,10 +217,26 @@ static void acpi_hibernation_finish(void
 	}
 }
 
+static int acpi_hibernation_pre_restore(void)
+{
+	acpi_status status;
+
+	status = acpi_hw_disable_all_gpes();
+
+	return ACPI_SUCCESS(status) ? 0 : -EFAULT;
+}
+
+static void acpi_hibernation_restore_cleanup(void)
+{
+	acpi_hw_enable_all_runtime_gpes();
+}
+
 static struct hibernation_ops acpi_hibernation_ops = {
 	.prepare = acpi_hibernation_prepare,
 	.enter = acpi_hibernation_enter,
 	.finish = acpi_hibernation_finish,
+	.pre_restore = acpi_hibernation_pre_restore,
+	.restore_cleanup = acpi_hibernation_restore_cleanup,
 };
 #endif				/* CONFIG_SOFTWARE_SUSPEND */
 
Index: linux-2.6.22-rc1/include/linux/suspend.h
===================================================================
--- linux-2.6.22-rc1.orig/include/linux/suspend.h	2007-05-17 21:36:45.000000000 +0200
+++ linux-2.6.22-rc1/include/linux/suspend.h	2007-05-17 22:42:47.000000000 +0200
@@ -43,11 +43,15 @@ static inline void pm_restore_console(vo
  * @prepare: prepare system for hibernation
  * @enter: shut down system after state has been saved to disk
  * @finish: finish/clean up after state has been reloaded
+ * @pre_restore: prepare system for the restoration from a hibernation image
+ * @restore_cleanup: clean up after a failing image restoration
  */
 struct hibernation_ops {
 	int (*prepare)(void);
 	int (*enter)(void);
 	void (*finish)(void);
+	int (*pre_restore)(void);
+	void (*restore_cleanup)(void);
 };
 
 #if defined(CONFIG_PM) && defined(CONFIG_SOFTWARE_SUSPEND)

Re: [RFC][PATCH 3/4] swsusp: Introduce restore platform operations

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> At least on some machines it is necessary to prepare the ACPI firmware for the
> restoration of the system memory state from the hibernation image if the
> "platform" mode of hibernation has been used.  Namely, in that cases we need to
> disable the GPEs before replacing the "boot" kernel with the "frozen" kernel.
> After the restore they will be re-enabled by hibernation_ops->finish(), but if
> the restore fails, they have to be re-enabled by the restore code explicitly.
> 
> For this purpose we can introduce two additional hibernation operations, called
> pre_restore() and restore_cleanup() and call them from the restore code path.
> Still, they should be called if the "platform" mode of hibernation has been
> used, so we need to pass the information about the hibernation mode from the
> "frozen" kernel to the "boot" kernel in the image header.

I don't quite like this one... passing flags from frozen to boot
kernel makes it more complex.

Is it really neccessary?

Could we do the same steps as the first thing when the frozen kernel
wakes up?

What machines does it fix?  

> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC][PATCH 3/4] swsusp: Introduce restore platform operations

[Rafael J. Wysocki]

On Tuesday, 22 May 2007 01:38, Pavel Machek wrote:
> Hi!
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > At least on some machines it is necessary to prepare the ACPI firmware for the
> > restoration of the system memory state from the hibernation image if the
> > "platform" mode of hibernation has been used.  Namely, in that cases we need to
> > disable the GPEs before replacing the "boot" kernel with the "frozen" kernel.
> > After the restore they will be re-enabled by hibernation_ops->finish(), but if
> > the restore fails, they have to be re-enabled by the restore code explicitly.
> > 
> > For this purpose we can introduce two additional hibernation operations, called
> > pre_restore() and restore_cleanup() and call them from the restore code path.
> > Still, they should be called if the "platform" mode of hibernation has been
> > used, so we need to pass the information about the hibernation mode from the
> > "frozen" kernel to the "boot" kernel in the image header.
> 
> I don't quite like this one... passing flags from frozen to boot
> kernel makes it more complex.
> 
> Is it really neccessary?

I think so.

> Could we do the same steps as the first thing when the frozen kernel
> wakes up?

At that time we do hibernate_ops->finish() which enables the GPEs anyway
in the right place.

> What machines does it fix?

http://bugzilla.kernel.org/show_bug.cgi?id=7887

Greetings,
Rafael

Re: [RFC][PATCH 3/4] swsusp: Introduce restore platform operations

[Rafael J. Wysocki]

On Tuesday, 22 May 2007 01:55, Rafael J. Wysocki wrote:
> On Tuesday, 22 May 2007 01:38, Pavel Machek wrote:
> > Hi!
> > 
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > At least on some machines it is necessary to prepare the ACPI firmware for the
> > > restoration of the system memory state from the hibernation image if the
> > > "platform" mode of hibernation has been used.  Namely, in that cases we need to
> > > disable the GPEs before replacing the "boot" kernel with the "frozen" kernel.
> > > After the restore they will be re-enabled by hibernation_ops->finish(), but if
> > > the restore fails, they have to be re-enabled by the restore code explicitly.
> > > 
> > > For this purpose we can introduce two additional hibernation operations, called
> > > pre_restore() and restore_cleanup() and call them from the restore code path.
> > > Still, they should be called if the "platform" mode of hibernation has been
> > > used, so we need to pass the information about the hibernation mode from the
> > > "frozen" kernel to the "boot" kernel in the image header.
> > 
> > I don't quite like this one... passing flags from frozen to boot
> > kernel makes it more complex.

BTW, we've been doing this for months in the userland version, so why not to do
it here?
 
> > Is it really neccessary?
> 
> I think so.
> 
> > Could we do the same steps as the first thing when the frozen kernel
> > wakes up?
> 
> At that time we do hibernate_ops->finish() which enables the GPEs anyway
> in the right place.
> 
> > What machines does it fix?
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=7887

Some more clarification is necessary, I think.

Apparently, we can't drop the disabling of GPEs before the restore because of
Bug #7887 .  We also can't do it unconditionally, because the GPEs wouldn't be
enabled after a successful restore if the suspend was done in the 'shutdown' or
'reboot' mode.

In principle we could (and probably should) unconditionally disable the GPEs
before each snapshot creation *and* before the restore, but then we'd have to
unconditionally enable them after the snapshot creation as well as after the
restore (or restore failure)   Still, for this purpose we'd need to modify
acpi_enter_sleep_state_prep() and acpi_leave_sleep_state() and we'd have to
introduce some mechanism synchronizing the disablind/enabling of the GPEs with
the device drivers' .suspend()/.resume() routines and with
disable_/enable_nonboot_cpus().  The problem with this approach is that it
affects the suspend (ie. s2ram) code as well as the hibernation, which I'd like
to avoid, for now.

Moreover, I'm not quite certain in which points the GPEs should actually be
disabled and enabled.  For example, there are some testing data indicating that
the enabling of GPEs should be done after all of the device drivers' .resume()
callbacks have run.  For this reason we need to have a (temporary) flexible
design that would allow us to try different variants until we are sure which
one is correct.  I regard this particular patch as a step in that direction.

Greetings,
Rafael

Re: [RFC][PATCH 3/4] swsusp: Introduce restore platform operations

[Alexey Starikovskiy]

Rafael J. Wysocki ?????:
> On Tuesday, 22 May 2007 01:55, Rafael J. Wysocki wrote:
> Moreover, I'm not quite certain in which points the GPEs should actually be
>   
> disabled and enabled.  For example, there are some testing data indicating that
> the enabling of GPEs should be done after all of the device drivers' .resume()
> callbacks have run.
Opposite, ACPI devices require enabled GPEs to do their resume, so 
enabling of GPEs
should happen at least before .resume() of ACPI devices is called.

Regards,
Alex.

Re: [RFC][PATCH 3/4] swsusp: Introduce restore platform operations

[Rafael J. Wysocki]

On Tuesday, 22 May 2007 22:10, Alexey Starikovskiy wrote:
> Rafael J. Wysocki ?????:
> > On Tuesday, 22 May 2007 01:55, Rafael J. Wysocki wrote:
> > Moreover, I'm not quite certain in which points the GPEs should actually be
> >   
> > disabled and enabled.  For example, there are some testing data indicating that
> > the enabling of GPEs should be done after all of the device drivers' .resume()
> > callbacks have run.
> Opposite, ACPI devices require enabled GPEs to do their resume, so 
> enabling of GPEs
> should happen at least before .resume() of ACPI devices is called.

Theoretically, that surely is correct, and which is what the patch actually
does. :-)

Still, we have users for whom hibernation was broken by moving the execution
of acpi_leave_sleep_state() before enable_nonboot_cpus().  At least on one
of these systems the hibernation can be fixed by removing the execution of
acpi_leave_sleep_state() from before enable_nonboot_cpus() and adding a
simplified version of it that only executes _WAK and enables the GPEs *after*
device_resume().  Unfortunately, I was unable to check if _WAK was necessary
for this (I hope it wasn't).

Greetings,
Rafael

[RFC][PATCH 4/4] swsusp: Fix hibernation code ordering

[Rafael J. Wysocki]

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

Change the code ordering so that hibernation_ops->prepare() is called after
device_suspend().  This is needed so that we don't violate the ACPI
specification, which states that the _PTS and _GTS system-control methods,
executed from acpi_sleep_prepare(), ought to be called after devices have been
put in low power states.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/disk.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Index: linux-2.6.22-rc1/kernel/power/disk.c
===================================================================
--- linux-2.6.22-rc1.orig/kernel/power/disk.c	2007-05-17 22:53:27.000000000 +0200
+++ linux-2.6.22-rc1/kernel/power/disk.c	2007-05-17 22:56:24.000000000 +0200
@@ -135,15 +135,15 @@ int hibernation_snapshot(int platform_mo
 	if (error)
 		goto Finish;
 
-	error = platform_prepare(platform_mode);
-	if (error)
-		goto Finish;
-
 	suspend_console();
 	error = device_suspend(PMSG_FREEZE);
 	if (error)
 		goto Resume_devices;
 
+	error = platform_prepare(platform_mode);
+	if (error)
+		goto Resume_devices;
+
 	error = disable_nonboot_cpus();
 	if (!error) {
 		if (hibernation_mode != HIBERNATION_TEST) {

[RFC][PATCH 4/4] swsusp: Fix hibernation code ordering (updated)

[Rafael J. Wysocki]

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

Change the code ordering so that hibernation_ops->prepare() is called after
device_suspend().  This is needed so that we don't violate the ACPI
specification, which states that the _PTS and _GTS system-control methods,
executed from acpi_sleep_prepare(), ought to be called after devices have been
put in low power states.

The "Finish" label in hibernation_restore() is moved, because device_suspend()
resumes devices if the suspending of them fails and the restore code ordering
should reflect the hibernation code ordering.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 kernel/power/disk.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Index: linux-2.6.22-rc1/kernel/power/disk.c
===================================================================
--- linux-2.6.22-rc1.orig/kernel/power/disk.c	2007-05-18 23:49:33.000000000 +0200
+++ linux-2.6.22-rc1/kernel/power/disk.c	2007-05-18 23:56:47.000000000 +0200
@@ -133,15 +133,15 @@ int hibernation_snapshot(int platform_mo
 	/* Free memory before shutting down devices. */
 	error = swsusp_shrink_memory();
 	if (error)
-		goto Finish;
-
-	error = platform_prepare(platform_mode);
-	if (error)
-		goto Finish;
+		return error;
 
 	suspend_console();
 	error = device_suspend(PMSG_FREEZE);
 	if (error)
+		goto Resume_console;
+
+	error = platform_prepare(platform_mode);
+	if (error)
 		goto Resume_devices;
 
 	error = disable_nonboot_cpus();
@@ -159,8 +159,8 @@ int hibernation_snapshot(int platform_mo
  Resume_devices:
 	platform_finish(platform_mode);
 	device_resume();
+ Resume_console:
 	resume_console();
- Finish:
 	return error;
 }
 
@@ -191,8 +191,8 @@ int hibernation_restore(int platform_mod
 		enable_nonboot_cpus();
 	}
 	platform_restore_cleanup(platform_mode);
- Finish:
 	device_resume();
+ Finish:
 	resume_console();
 	pm_restore_console();
 	return error;

Re: [RFC][PATCH 4/4] swsusp: Fix hibernation code ordering (updated)

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Change the code ordering so that hibernation_ops->prepare() is called after
> device_suspend().  This is needed so that we don't violate the ACPI
> specification, which states that the _PTS and _GTS system-control methods,
> executed from acpi_sleep_prepare(), ought to be called after devices have been
> put in low power states.
> 
> The "Finish" label in hibernation_restore() is moved, because device_suspend()
> resumes devices if the suspending of them fails and the restore code ordering
> should reflect the hibernation code ordering.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

ACK.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html