[PATCH 0/3] suspend pm_ops cleanup

[PATCH 0/3] suspend pm_ops cleanup

[Johannes Berg]

These patches clean up some misconceptions and misuse around pm_ops
and delete the unused and ill-designed "firmware" disk-suspend mode.

johannes

[PATCH 1/3] rework pm_ops pm_disk_mode, kill misuse

[Johannes Berg]

[-- Attachment #1: 011-pm-ops-diskmode.patch --]
[-- Type: text/plain, Size: 11426 bytes --]

The pm_ops.pm_disk_mode is used in totally bogus ways since
nobody really seems to understand what it actually does.

This patch clarifies the pm_disk_mode description.

It also removes all the arm and sh users that think they can veto
suspend to disk via pm_ops; not so since the user can always
do echo shutdown > /sys/power/disk, they need to find a better
way involving Kconfig or such.

ACPI is the only user left with a non-zero pm_disk_mode.

The patch also sets the default mode to shutdown again, but
when a new pm_ops is registered its pm_disk_mode is selected
as default, that way the default stays for ACPI where it is
apparently required.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: David Brownell <david-b@pacbell.net>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: linux-pm@lists.linux-foundation.org

---
 arch/arm/common/sharpsl_pm.c |    1 
 arch/arm/mach-at91/pm.c      |    1 
 arch/arm/mach-omap1/pm.c     |    1 
 arch/arm/mach-omap2/pm.c     |    1 
 arch/arm/mach-pxa/pm.c       |    4 ---
 arch/arm/mach-sa1100/pm.c    |    7 -----
 arch/arm/plat-s3c24xx/pm.c   |    9 ------
 arch/sh/boards/hp6xx/pm.c    |    7 -----
 include/linux/pm.h           |   23 +++++++++--------
 kernel/power/disk.c          |   56 ++++++++++++++++++++++++++++---------------
 kernel/power/main.c          |    6 +++-
 11 files changed, 54 insertions(+), 62 deletions(-)

--- linux-2.6.orig/include/linux/pm.h	2007-03-20 12:41:38.423214909 +0100
+++ linux-2.6/include/linux/pm.h	2007-03-20 12:42:02.253214909 +0100
@@ -112,6 +112,8 @@ typedef int __bitwise suspend_state_t;
 
 typedef int __bitwise suspend_disk_method_t;
 
+/* invalid must be 0 so struct pm_ops initialisers can leave it out */
+#define PM_DISK_INVALID		((__force suspend_disk_method_t) 0)
 #define	PM_DISK_FIRMWARE	((__force suspend_disk_method_t) 1)
 #define	PM_DISK_PLATFORM	((__force suspend_disk_method_t) 2)
 #define	PM_DISK_SHUTDOWN	((__force suspend_disk_method_t) 3)
@@ -137,17 +139,16 @@ typedef int __bitwise suspend_disk_metho
  * @finish: Called when the system has left the given state and all devices
  *	are resumed. The return value is ignored.
  *
- * @pm_disk_mode: Set to the disk method that the user should be able to
- *	configure for suspend-to-disk. Since %PM_DISK_SHUTDOWN,
- *	%PM_DISK_REBOOT, %PM_DISK_TEST and %PM_DISK_TESTPROC
- *	are always allowed, currently only %PM_DISK_PLATFORM
- *	makes sense. If the user then choses %PM_DISK_PLATFORM,
- *	the @prepare call will be called before suspending to disk
- *	(if present), the @enter call should be present and will
- *	be called after all state has been saved and the machine
- *	is ready to be shut down/suspended/..., and the @finish
- *	callback is called after state has been restored. All
- *	these calls are called with %PM_SUSPEND_DISK as the state.
+ * @pm_disk_mode: The generic code always allows one of the shutdown methods
+ *	%PM_DISK_SHUTDOWN, %PM_DISK_REBOOT, %PM_DISK_TEST and
+ *	%PM_DISK_TESTPROC. If this variable is set, the mode it is set
+ *	to is allowed in addition to those modes and is also made default.
+ *	When this mode is sent selected, the @prepare call will be called
+ *	before suspending to disk (if present), the @enter call should be
+ *	present and will be called after all state has been saved and the
+ *	machine is ready to be powered off; the @finish callback is called
+ *	after state has been restored. All these calls are called with
+ *	%PM_SUSPEND_DISK as the state.
  */
 struct pm_ops {
 	int (*valid)(suspend_state_t state);
--- linux-2.6.orig/kernel/power/disk.c	2007-03-20 12:41:38.463214909 +0100
+++ linux-2.6/kernel/power/disk.c	2007-03-20 12:42:02.263214909 +0100
@@ -39,7 +39,13 @@ static inline int platform_prepare(void)
 {
 	int error = 0;
 
-	if (pm_disk_mode == PM_DISK_PLATFORM) {
+	switch (pm_disk_mode) {
+	case PM_DISK_TEST:
+	case PM_DISK_TESTPROC:
+	case PM_DISK_SHUTDOWN:
+	case PM_DISK_REBOOT:
+		break;
+	default:
 		if (pm_ops && pm_ops->prepare)
 			error = pm_ops->prepare(PM_SUSPEND_DISK);
 	}
@@ -48,31 +54,33 @@ static inline int platform_prepare(void)
 
 /**
  *	power_down - Shut machine down for hibernate.
- *	@mode:		Suspend-to-disk mode
  *
- *	Use the platform driver, if configured so, and return gracefully if it
- *	fails.
- *	Otherwise, try to power off and reboot. If they fail, halt the machine,
- *	there ain't no turning back.
+ *	Use the platform driver, if configured so; otherwise try
+ *	to power off or reboot.
  */
 
-static void power_down(suspend_disk_method_t mode)
+static void power_down(void)
 {
 	disable_nonboot_cpus();
-	switch(mode) {
-	case PM_DISK_PLATFORM:
-		if (pm_ops && pm_ops->enter) {
-			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
-			pm_ops->enter(PM_SUSPEND_DISK);
-			break;
-		}
+
+	switch (pm_disk_mode) {
+	case PM_DISK_TEST:
+	case PM_DISK_TESTPROC:
+		break;
 	case PM_DISK_SHUTDOWN:
 		kernel_power_off();
 		break;
 	case PM_DISK_REBOOT:
 		kernel_restart(NULL);
 		break;
+	default:
+		if (pm_ops && pm_ops->enter) {
+			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
+			pm_ops->enter(PM_SUSPEND_DISK);
+			break;
+		}
 	}
+
 	kernel_halt();
 	/* Valid image is on the disk, if we continue we risk serious data corruption
 	   after resume. */
@@ -82,7 +90,13 @@ static void power_down(suspend_disk_meth
 
 static inline void platform_finish(void)
 {
-	if (pm_disk_mode == PM_DISK_PLATFORM) {
+	switch (pm_disk_mode) {
+	case PM_DISK_TEST:
+	case PM_DISK_TESTPROC:
+	case PM_DISK_SHUTDOWN:
+	case PM_DISK_REBOOT:
+		break;
+	default:
 		if (pm_ops && pm_ops->finish)
 			pm_ops->finish(PM_SUSPEND_DISK);
 	}
@@ -167,7 +181,7 @@ int pm_suspend_disk(void)
 		pr_debug("PM: writing image.\n");
 		error = swsusp_write();
 		if (!error)
-			power_down(pm_disk_mode);
+			power_down();
 		else {
 			swsusp_free();
 			goto Thaw;
@@ -347,10 +361,14 @@ static ssize_t disk_store(struct subsyst
 		}
 	}
 	if (mode) {
-		if (mode == PM_DISK_SHUTDOWN || mode == PM_DISK_REBOOT ||
-		     mode == PM_DISK_TEST || mode == PM_DISK_TESTPROC) {
+		switch (mode) {
+		case PM_DISK_SHUTDOWN:
+		case PM_DISK_REBOOT:
+		case PM_DISK_TEST:
+		case PM_DISK_TESTPROC:
 			pm_disk_mode = mode;
-		} else {
+			break;
+		default:
 			if (pm_ops && pm_ops->enter &&
 			    (mode == pm_ops->pm_disk_mode))
 				pm_disk_mode = mode;
--- linux-2.6.orig/kernel/power/main.c	2007-03-20 12:41:38.543214909 +0100
+++ linux-2.6/kernel/power/main.c	2007-03-20 12:42:02.263214909 +0100
@@ -30,7 +30,7 @@
 DEFINE_MUTEX(pm_mutex);
 
 struct pm_ops *pm_ops;
-suspend_disk_method_t pm_disk_mode = PM_DISK_PLATFORM;
+suspend_disk_method_t pm_disk_mode = PM_DISK_SHUTDOWN;
 
 /**
  *	pm_set_ops - Set the global power method table. 
@@ -41,6 +41,10 @@ void pm_set_ops(struct pm_ops * ops)
 {
 	mutex_lock(&pm_mutex);
 	pm_ops = ops;
+	if (ops && ops->pm_disk_mode != PM_DISK_INVALID) {
+		pm_disk_mode = ops->pm_disk_mode;
+	} else
+		pm_disk_mode = PM_DISK_SHUTDOWN;
 	mutex_unlock(&pm_mutex);
 }
 
--- linux-2.6.orig/arch/arm/common/sharpsl_pm.c	2007-03-20 12:41:38.673214909 +0100
+++ linux-2.6/arch/arm/common/sharpsl_pm.c	2007-03-20 12:42:02.263214909 +0100
@@ -766,7 +766,6 @@ static void sharpsl_apm_get_power_status
 }
 
 static struct pm_ops sharpsl_pm_ops = {
-	.pm_disk_mode	= PM_DISK_FIRMWARE,
 	.prepare	= pxa_pm_prepare,
 	.enter		= corgi_pxa_pm_enter,
 	.finish		= pxa_pm_finish,
--- linux-2.6.orig/arch/arm/mach-at91/pm.c	2007-03-20 12:41:38.743214909 +0100
+++ linux-2.6/arch/arm/mach-at91/pm.c	2007-03-20 12:42:02.263214909 +0100
@@ -201,7 +201,6 @@ error:
 
 
 static struct pm_ops at91_pm_ops ={
-	.pm_disk_mode	= 0,
 	.valid		= at91_pm_valid_state,
 	.prepare	= at91_pm_prepare,
 	.enter		= at91_pm_enter,
--- linux-2.6.orig/arch/arm/mach-omap1/pm.c	2007-03-20 12:41:38.763214909 +0100
+++ linux-2.6/arch/arm/mach-omap1/pm.c	2007-03-20 12:42:02.263214909 +0100
@@ -698,7 +698,6 @@ static struct irqaction omap_wakeup_irq 
 
 
 static struct pm_ops omap_pm_ops ={
-	.pm_disk_mode	= 0,
 	.prepare	= omap_pm_prepare,
 	.enter		= omap_pm_enter,
 	.finish		= omap_pm_finish,
--- linux-2.6.orig/arch/arm/mach-omap2/pm.c	2007-03-20 12:41:38.833214909 +0100
+++ linux-2.6/arch/arm/mach-omap2/pm.c	2007-03-20 12:42:02.263214909 +0100
@@ -370,7 +370,6 @@ static int omap2_pm_finish(suspend_state
 }
 
 static struct pm_ops omap_pm_ops = {
-	.pm_disk_mode	= 0,
 	.prepare	= omap2_pm_prepare,
 	.enter		= omap2_pm_enter,
 	.finish		= omap2_pm_finish,
--- linux-2.6.orig/arch/arm/mach-pxa/pm.c	2007-03-20 12:41:38.853214909 +0100
+++ linux-2.6/arch/arm/mach-pxa/pm.c	2007-03-20 12:42:02.263214909 +0100
@@ -223,11 +223,7 @@ int pxa_pm_finish(suspend_state_t state)
 
 EXPORT_SYMBOL_GPL(pxa_pm_finish);
 
-/*
- * Set to PM_DISK_FIRMWARE so we can quickly veto suspend-to-disk.
- */
 static struct pm_ops pxa_pm_ops = {
-	.pm_disk_mode	= PM_DISK_FIRMWARE,
 	.prepare	= pxa_pm_prepare,
 	.enter		= pxa_pm_enter,
 	.finish		= pxa_pm_finish,
--- linux-2.6.orig/arch/arm/mach-sa1100/pm.c	2007-03-20 12:41:38.903214909 +0100
+++ linux-2.6/arch/arm/mach-sa1100/pm.c	2007-03-20 12:42:02.273214909 +0100
@@ -59,9 +59,6 @@ static int sa11x0_pm_enter(suspend_state
 	unsigned long gpio, sleep_save[SLEEP_SAVE_SIZE];
 	struct timespec delta, rtc;
 
-	if (state != PM_SUSPEND_MEM)
-		return -EINVAL;
-
 	/* preserve current time */
 	rtc.tv_sec = RCNR;
 	rtc.tv_nsec = 0;
@@ -134,11 +131,7 @@ unsigned long sleep_phys_sp(void *sp)
 	return virt_to_phys(sp);
 }
 
-/*
- * Set to PM_DISK_FIRMWARE so we can quickly veto suspend-to-disk.
- */
 static struct pm_ops sa11x0_pm_ops = {
-	.pm_disk_mode	= PM_DISK_FIRMWARE,
 	.enter		= sa11x0_pm_enter,
 };
 
--- linux-2.6.orig/arch/arm/plat-s3c24xx/pm.c	2007-03-20 12:41:38.953214909 +0100
+++ linux-2.6/arch/arm/plat-s3c24xx/pm.c	2007-03-20 12:42:02.273214909 +0100
@@ -511,11 +511,6 @@ static int s3c2410_pm_enter(suspend_stat
 		return -EINVAL;
 	}
 
-	if (state != PM_SUSPEND_MEM) {
-		printk(KERN_ERR PFX "error: only PM_SUSPEND_MEM supported\n");
-		return -EINVAL;
-	}
-
 	/* check if we have anything to wake-up with... bad things seem
 	 * to happen if you suspend with no wakeup (system will often
 	 * require a full power-cycle)
@@ -633,11 +628,7 @@ static int s3c2410_pm_finish(suspend_sta
 	return 0;
 }
 
-/*
- * Set to PM_DISK_FIRMWARE so we can quickly veto suspend-to-disk.
- */
 static struct pm_ops s3c2410_pm_ops = {
-	.pm_disk_mode	= PM_DISK_FIRMWARE,
 	.prepare	= s3c2410_pm_prepare,
 	.enter		= s3c2410_pm_enter,
 	.finish		= s3c2410_pm_finish,
--- linux-2.6.orig/arch/sh/boards/hp6xx/pm.c	2007-03-20 12:41:39.163214909 +0100
+++ linux-2.6/arch/sh/boards/hp6xx/pm.c	2007-03-20 12:42:02.273214909 +0100
@@ -27,9 +27,6 @@ static int hp6x0_pm_enter(suspend_state_
 	u16 hd64461_stbcr;
 #endif
 
-	if (state != PM_SUSPEND_MEM)
-		return -EINVAL;
-
 #ifdef CONFIG_HD64461_ENABLER
 	outb(0, HD64461_PCC1CSCIER);
 
@@ -70,11 +67,7 @@ static int hp6x0_pm_enter(suspend_state_
 	return 0;
 }
 
-/*
- * Set to PM_DISK_FIRMWARE so we can quickly veto suspend-to-disk.
- */
 static struct pm_ops hp6x0_pm_ops = {
-	.pm_disk_mode	= PM_DISK_FIRMWARE,
 	.enter		= hp6x0_pm_enter,
 };
 

--

[PATCH 2/3] power management: remove firmware disk mode

[Johannes Berg]

[-- Attachment #1: 012-remove-firmware-disk-mode.patch --]
[-- Type: text/plain, Size: 9812 bytes --]

This patch removes the firmware disk suspend mode which is the wrong
approach, it is supposed to be used for implementing firmware-based disk
suspend but cannot actually be used for that.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: linux-pm@lists.linux-foundation.org

---
 Documentation/power/interface.txt |   21 +++++----------------
 Documentation/power/states.txt    |   13 +++++++------
 Documentation/power/swsusp.txt    |   14 +++++---------
 include/linux/pm.h                |   13 ++++++-------
 kernel/power/disk.c               |   27 +++++++++++----------------
 5 files changed, 34 insertions(+), 54 deletions(-)

--- linux-2.6.orig/include/linux/pm.h	2007-03-21 15:44:54.663148946 +0100
+++ linux-2.6/include/linux/pm.h	2007-03-21 15:45:04.403148946 +0100
@@ -114,13 +114,12 @@ typedef int __bitwise suspend_disk_metho
 
 /* invalid must be 0 so struct pm_ops initialisers can leave it out */
 #define PM_DISK_INVALID		((__force suspend_disk_method_t) 0)
-#define	PM_DISK_FIRMWARE	((__force suspend_disk_method_t) 1)
-#define	PM_DISK_PLATFORM	((__force suspend_disk_method_t) 2)
-#define	PM_DISK_SHUTDOWN	((__force suspend_disk_method_t) 3)
-#define	PM_DISK_REBOOT		((__force suspend_disk_method_t) 4)
-#define	PM_DISK_TEST		((__force suspend_disk_method_t) 5)
-#define	PM_DISK_TESTPROC	((__force suspend_disk_method_t) 6)
-#define	PM_DISK_MAX		((__force suspend_disk_method_t) 7)
+#define	PM_DISK_PLATFORM	((__force suspend_disk_method_t) 1)
+#define	PM_DISK_SHUTDOWN	((__force suspend_disk_method_t) 2)
+#define	PM_DISK_REBOOT		((__force suspend_disk_method_t) 3)
+#define	PM_DISK_TEST		((__force suspend_disk_method_t) 4)
+#define	PM_DISK_TESTPROC	((__force suspend_disk_method_t) 5)
+#define	PM_DISK_MAX		((__force suspend_disk_method_t) 6)
 
 /**
  * struct pm_ops - Callbacks for managing platform dependent suspend states.
--- linux-2.6.orig/kernel/power/disk.c	2007-03-21 15:44:54.693148946 +0100
+++ linux-2.6/kernel/power/disk.c	2007-03-21 15:48:06.073148946 +0100
@@ -123,8 +123,6 @@ static int prepare_processes(void)
 /**
  *	pm_suspend_disk - The granpappy of hibernation power management.
  *
- *	If we're going through the firmware, then get it over with quickly.
- *
  *	If not, then call swsusp to do its thing, then figure out how
  *	to power down the system.
  */
@@ -301,7 +299,6 @@ late_initcall(software_resume);
 
 
 static const char * const pm_disk_modes[] = {
-	[PM_DISK_FIRMWARE]	= "firmware",
 	[PM_DISK_PLATFORM]	= "platform",
 	[PM_DISK_SHUTDOWN]	= "shutdown",
 	[PM_DISK_REBOOT]	= "reboot",
@@ -312,27 +309,25 @@ static const char * const pm_disk_modes[
 /**
  *	disk - Control suspend-to-disk mode
  *
- *	Suspend-to-disk can be handled in several ways. The greatest
- *	distinction is who writes memory to disk - the firmware or the OS.
- *	If the firmware does it, we assume that it also handles suspending
- *	the system.
- *	If the OS does it, then we have three options for putting the system
- *	to sleep - using the platform driver (e.g. ACPI or other PM registers),
- *	powering off the system or rebooting the system (for testing).
+ *	Suspend-to-disk can be handled in several ways. We have a few options
+ *	for putting the system to sleep - using the platform driver (e.g. ACPI
+ *	or other pm_ops), powering off the system or rebooting the system
+ *	(for testing) as well as the two test modes.
  *
- *	The system will support either 'firmware' or 'platform', and that is
- *	known a priori (and encoded in pm_ops). But, the user may choose
- *	'shutdown' or 'reboot' as alternatives.
+ *	The system can support 'platform', and that is known a priori (and
+ *	encoded in pm_ops). However, the user may choose 'shutdown' or 'reboot'
+ *	as alternatives, as well as the test modes 'test' and 'testproc'.
  *
  *	show() will display what the mode is currently set to.
  *	store() will accept one of
  *
- *	'firmware'
  *	'platform'
  *	'shutdown'
  *	'reboot'
+ *	'test'
+ *	'testproc'
  *
- *	It will only change to 'firmware' or 'platform' if the system
+ *	It will only change to 'platform' if the system
  *	supports it (as determined from pm_ops->pm_disk_mode).
  */
 
@@ -354,7 +349,7 @@ static ssize_t disk_store(struct subsyst
 	len = p ? p - buf : n;
 
 	mutex_lock(&pm_mutex);
-	for (i = PM_DISK_FIRMWARE; i < PM_DISK_MAX; i++) {
+	for (i = PM_DISK_PLATFORM; i < PM_DISK_MAX; i++) {
 		if (!strncmp(buf, pm_disk_modes[i], len)) {
 			mode = i;
 			break;
--- linux-2.6.orig/Documentation/power/interface.txt	2007-03-21 15:45:17.873148946 +0100
+++ linux-2.6/Documentation/power/interface.txt	2007-03-21 15:49:26.673148946 +0100
@@ -18,17 +18,10 @@ states.
 
 
 /sys/power/disk controls the operating mode of the suspend-to-disk
-mechanism. Suspend-to-disk can be handled in several ways. The
-greatest distinction is who writes memory to disk - the firmware or
-the kernel. If the firmware does it, we assume that it also handles
-suspending the system. 
-
-If the kernel does it, then we have three options for putting the system
-to sleep - using the platform driver (e.g. ACPI or other PM
-registers), powering off the system or rebooting the system (for
-testing). The system will support either 'firmware' or 'platform', and
-that is known a priori. But, the user may choose 'shutdown' or
-'reboot' as alternatives. 
+mechanism. Suspend-to-disk can be handled in several ways. We have a
+few options for putting the system to sleep - using the platform driver
+(e.g. ACPI or other pm_ops), powering off the system or rebooting the
+system (for testing).
 
 Additionally, /sys/power/disk can be used to turn on one of the two testing
 modes of the suspend-to-disk mechanism: 'testproc' or 'test'.  If the
@@ -44,16 +37,12 @@ is being slow and which device drivers a
 Reading from this file will display what the mode is currently set
 to. Writing to this file will accept one of
 
-       'firmware'
-       'platform'
+       'platform' (only if the platform supports it)
        'shutdown'
        'reboot'
        'testproc'
        'test'
 
-It will only change to 'firmware' or 'platform' if the system supports
-it. 
-
 /sys/power/image_size controls the size of the image created by
 the suspend-to-disk mechanism.  It can be written a string
 representing a non-negative integer that will be used as an upper
--- linux-2.6.orig/Documentation/power/states.txt	2007-03-21 15:45:17.993148946 +0100
+++ linux-2.6/Documentation/power/states.txt	2007-03-21 15:51:18.763148946 +0100
@@ -62,17 +62,18 @@ setup via another operating system for i
 inconvenience, this method requires minimal work by the kernel, since
 the firmware will also handle restoring memory contents on resume. 
 
-If the kernel is responsible for persistently saving state, a mechanism
-called 'swsusp' (Swap Suspend) is used to write memory contents to
-free swap space. swsusp has some restrictive requirements, but should
-work in most cases. Some, albeit outdated, documentation can be found
-in Documentation/power/swsusp.txt. 
+For suspend-to-disk, a mechanism called swsusp called 'swsusp' (Swap
+Suspend) is used to write memory contents to free swap space.
+swsusp has some restrictive requirements, but should work in most
+cases. Some, albeit outdated, documentation can be found in
+Documentation/power/swsusp.txt. Alternatively, userspace can do most
+of the actual suspend to disk work, see userland-swsusp.txt.
 
 Once memory state is written to disk, the system may either enter a
 low-power state (like ACPI S4), or it may simply power down. Powering
 down offers greater savings, and allows this mechanism to work on any
 system. However, entering a real low-power state allows the user to
-trigger wake up events (e.g. pressing a key or opening a laptop lid). 
+trigger wake up events (e.g. pressing a key or opening a laptop lid).
 
 A transition from Suspend-to-Disk to the On state should take about 30
 seconds, though it's typically a bit more with the current
--- linux-2.6.orig/Documentation/power/swsusp.txt	2007-03-21 15:45:18.133148946 +0100
+++ linux-2.6/Documentation/power/swsusp.txt	2007-03-21 15:52:20.423148946 +0100
@@ -156,8 +156,7 @@ instead set the PF_NOFREEZE process flag
 be very careful).
 
 
-Q: What is the difference between "platform", "shutdown" and
-"firmware" in /sys/power/disk?
+Q: What is the difference between "platform" and "shutdown"?
 
 A:
 
@@ -166,11 +165,8 @@ shutdown: save state in linux, then tell
 platform: save state in linux, then tell bios to powerdown and blink
           "suspended led"
 
-firmware: tell bios to save state itself [needs BIOS-specific suspend
-	  partition, and has very little to do with swsusp]
-
-"platform" is actually right thing to do, but "shutdown" is most
-reliable.
+"platform" is actually right thing to do where supported, but
+"shutdown" is most reliable (except on ACPI systems).
 
 Q: I do not understand why you have such strong objections to idea of
 selective suspend.
@@ -388,8 +384,8 @@ while the system is asleep, maintaining 
 modes like "suspend-to-RAM" or "standby".  (Don't write "disk" to the
 /sys/power/state file; write "standby" or "mem".)  We've not seen any
 hardware that can use these modes through software suspend, although in
-theory some systems might support "platform" or "firmware" modes that
-won't break the USB connections.
+theory some systems might support "platform" modes that won't break the
+USB connections.
 
 Remember that it's always a bad idea to unplug a disk drive containing a
 mounted filesystem.  That's true even when your system is asleep!  The

--

[PATCH 3/3] power management: implement pm_ops.valid for everybody

[Johannes Berg]

[-- Attachment #1: 013-implement-valid.patch --]
[-- Type: text/plain, Size: 7606 bytes --]

Almost all users of pm_ops only support mem sleep, don't check in .valid
and don't reject any others in .prepare so users can be confused if they
check /sys/power/state, especially when new states are added (these would
then result in s-t-r although they're supposed to be something different).

This patch implements a generic pm_valid_only_mem function that is then
exported for users and puts it to use in almost all existing pm_ops.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: David Brownell <david-b@pacbell.net>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: linux-pm@lists.linux-foundation.org

---
 arch/arm/common/sharpsl_pm.c |    1 +
 arch/arm/mach-omap1/pm.c     |    1 +
 arch/arm/mach-omap2/pm.c     |    1 +
 arch/arm/mach-pnx4008/pm.c   |   35 +----------------------------------
 arch/arm/mach-pxa/pm.c       |    1 +
 arch/arm/mach-sa1100/pm.c    |    1 +
 arch/arm/plat-s3c24xx/pm.c   |   19 +------------------
 arch/sh/boards/hp6xx/pm.c    |    1 +
 drivers/acpi/sleep/main.c    |   13 +++++++++++--
 include/linux/pm.h           |    4 ++++
 kernel/power/main.c          |   13 +++++++++++++
 11 files changed, 36 insertions(+), 54 deletions(-)

--- linux-2.6.orig/include/linux/pm.h	2007-03-20 12:42:04.813214909 +0100
+++ linux-2.6/include/linux/pm.h	2007-03-20 12:42:05.693214909 +0100
@@ -128,6 +128,9 @@ typedef int __bitwise suspend_disk_metho
  *	always valid and never passed to this call.
  *	If not assigned, all suspend states are advertised as valid
  *	in /sys/power/state (but can still be rejected by prepare or enter.)
+ *	Since new states can be added for other platforms, you should
+ *	assign this callback. There is a %pm_valid_only_mem function
+ *	available if you only implemented mem sleep.
  *
  * @prepare: Prepare the platform for the given suspend state. Can return a
  *	negative error code if necessary.
@@ -165,6 +168,7 @@ extern void pm_set_ops(struct pm_ops *pm
 extern struct pm_ops *pm_ops;
 extern int pm_suspend(suspend_state_t state);
 
+extern int pm_valid_only_mem(suspend_state_t state);
 
 /*
  * Device power management
--- linux-2.6.orig/arch/arm/common/sharpsl_pm.c	2007-03-20 12:42:02.263214909 +0100
+++ linux-2.6/arch/arm/common/sharpsl_pm.c	2007-03-20 12:42:05.693214909 +0100
@@ -769,6 +769,7 @@ static struct pm_ops sharpsl_pm_ops = {
 	.prepare	= pxa_pm_prepare,
 	.enter		= corgi_pxa_pm_enter,
 	.finish		= pxa_pm_finish,
+	.valid		= pm_valid_only_mem,
 };
 
 static int __init sharpsl_pm_probe(struct platform_device *pdev)
--- linux-2.6.orig/arch/arm/mach-omap1/pm.c	2007-03-20 12:42:02.263214909 +0100
+++ linux-2.6/arch/arm/mach-omap1/pm.c	2007-03-20 12:42:05.703214909 +0100
@@ -701,6 +701,7 @@ static struct pm_ops omap_pm_ops ={
 	.prepare	= omap_pm_prepare,
 	.enter		= omap_pm_enter,
 	.finish		= omap_pm_finish,
+	.valid		= pm_valid_only_mem,
 };
 
 static int __init omap_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-omap2/pm.c	2007-03-20 12:42:02.263214909 +0100
+++ linux-2.6/arch/arm/mach-omap2/pm.c	2007-03-20 12:42:05.703214909 +0100
@@ -373,6 +373,7 @@ static struct pm_ops omap_pm_ops = {
 	.prepare	= omap2_pm_prepare,
 	.enter		= omap2_pm_enter,
 	.finish		= omap2_pm_finish,
+	.valid		= pm_valid_only_mem,
 };
 
 int __init omap2_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-pnx4008/pm.c	2007-03-20 12:41:36.133214909 +0100
+++ linux-2.6/arch/arm/mach-pnx4008/pm.c	2007-03-20 12:42:05.703214909 +0100
@@ -115,42 +115,9 @@ static int pnx4008_pm_enter(suspend_stat
 	return 0;
 }
 
-/*
- * Called after processes are frozen, but before we shut down devices.
- */
-static int pnx4008_pm_prepare(suspend_state_t state)
-{
-	switch (state) {
-	case PM_SUSPEND_STANDBY:
-	case PM_SUSPEND_MEM:
-		break;
-
-	case PM_SUSPEND_DISK:
-		return -ENOTSUPP;
-		break;
-
-	default:
-		return -EINVAL;
-		break;
-	}
-	return 0;
-}
-
-/*
- * Called after devices are re-setup, but before processes are thawed.
- */
-static int pnx4008_pm_finish(suspend_state_t state)
-{
-	return 0;
-}
-
-/*
- * Set to PM_DISK_FIRMWARE so we can quickly veto suspend-to-disk.
- */
 static struct pm_ops pnx4008_pm_ops = {
-	.prepare = pnx4008_pm_prepare,
 	.enter = pnx4008_pm_enter,
-	.finish = pnx4008_pm_finish,
+	.valid = pm_valid_only_mem,
 };
 
 static int __init pnx4008_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-pxa/pm.c	2007-03-20 12:42:02.263214909 +0100
+++ linux-2.6/arch/arm/mach-pxa/pm.c	2007-03-20 12:42:05.703214909 +0100
@@ -227,6 +227,7 @@ static struct pm_ops pxa_pm_ops = {
 	.prepare	= pxa_pm_prepare,
 	.enter		= pxa_pm_enter,
 	.finish		= pxa_pm_finish,
+	.valid		= pm_valid_only_mem,
 };
 
 static int __init pxa_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-sa1100/pm.c	2007-03-20 12:42:02.273214909 +0100
+++ linux-2.6/arch/arm/mach-sa1100/pm.c	2007-03-20 12:42:05.703214909 +0100
@@ -133,6 +133,7 @@ unsigned long sleep_phys_sp(void *sp)
 
 static struct pm_ops sa11x0_pm_ops = {
 	.enter		= sa11x0_pm_enter,
+	.valid		= pm_valid_only_mem,
 };
 
 static int __init sa11x0_pm_init(void)
--- linux-2.6.orig/arch/arm/plat-s3c24xx/pm.c	2007-03-20 12:42:02.273214909 +0100
+++ linux-2.6/arch/arm/plat-s3c24xx/pm.c	2007-03-20 12:42:05.703214909 +0100
@@ -612,26 +612,9 @@ static int s3c2410_pm_enter(suspend_stat
 	return 0;
 }
 
-/*
- * Called after processes are frozen, but before we shut down devices.
- */
-static int s3c2410_pm_prepare(suspend_state_t state)
-{
-	return 0;
-}
-
-/*
- * Called after devices are re-setup, but before processes are thawed.
- */
-static int s3c2410_pm_finish(suspend_state_t state)
-{
-	return 0;
-}
-
 static struct pm_ops s3c2410_pm_ops = {
-	.prepare	= s3c2410_pm_prepare,
 	.enter		= s3c2410_pm_enter,
-	.finish		= s3c2410_pm_finish,
+	.valid		= pm_valid_only_mem,
 };
 
 /* s3c2410_pm_init
--- linux-2.6.orig/arch/sh/boards/hp6xx/pm.c	2007-03-20 12:42:02.273214909 +0100
+++ linux-2.6/arch/sh/boards/hp6xx/pm.c	2007-03-20 12:42:05.713214909 +0100
@@ -69,6 +69,7 @@ static int hp6x0_pm_enter(suspend_state_
 
 static struct pm_ops hp6x0_pm_ops = {
 	.enter		= hp6x0_pm_enter,
+	.valid		= pm_valid_only_mem,
 };
 
 static int __init hp6x0_pm_init(void)
--- linux-2.6.orig/drivers/acpi/sleep/main.c	2007-03-20 12:41:36.863214909 +0100
+++ linux-2.6/drivers/acpi/sleep/main.c	2007-03-20 12:42:05.713214909 +0100
@@ -168,9 +168,18 @@ int acpi_suspend(u32 acpi_state)
 
 static int acpi_pm_state_valid(suspend_state_t pm_state)
 {
-	u32 acpi_state = acpi_suspend_states[pm_state];
+	u32 acpi_state;
 
-	return sleep_states[acpi_state];
+	switch (pm_state) {
+	case PM_SUSPEND_ON:
+	case PM_SUSPEND_STANDBY:
+	case PM_SUSPEND_MEM:
+		acpi_state = acpi_suspend_states[pm_state];
+
+		return sleep_states[acpi_state];
+	default:
+		return 0;
+	}
 }
 
 static struct pm_ops acpi_pm_ops = {
--- linux-2.6.orig/kernel/power/main.c	2007-03-20 12:42:02.263214909 +0100
+++ linux-2.6/kernel/power/main.c	2007-03-20 12:42:05.713214909 +0100
@@ -48,6 +48,19 @@ void pm_set_ops(struct pm_ops * ops)
 	mutex_unlock(&pm_mutex);
 }
 
+/**
+ * pm_valid_only_mem - generic memory-only valid callback
+ *
+ * pm_ops drivers that implement mem suspend only and only need
+ * to check for that in their .valid callback can use this instead
+ * of rolling their own .valid callback.
+ */
+int pm_valid_only_mem(suspend_state_t state)
+{
+	return state == PM_SUSPEND_MEM;
+}
+
+
 static inline void pm_finish(suspend_state_t state)
 {
 	if (pm_ops->finish)

--

Re: [PATCH 1/3] rework pm_ops pm_disk_mode, kill misuse

[Pavel Machek]

Hi!

> The pm_ops.pm_disk_mode is used in totally bogus ways since
> nobody really seems to understand what it actually does.

Well, I believe me and Rafael understand it by now...

> ACPI is the only user left with a non-zero pm_disk_mode.
> 
> The patch also sets the default mode to shutdown again, but
> when a new pm_ops is registered its pm_disk_mode is selected
> as default, that way the default stays for ACPI where it is
> apparently required.

Ok.

> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> Cc: David Brownell <david-b@pacbell.net>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: linux-pm@lists.linux-foundation.org

ACK, not 2.6.21 material.
									Pavel

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

Re: [PATCH 2/3] power management: remove firmware disk mode

[Pavel Machek]

Hi!

> This patch removes the firmware disk suspend mode which is the wrong
> approach, it is supposed to be used for implementing firmware-based disk
> suspend but cannot actually be used for that.

Actually, it _can_ be used for that. It worked... once, on machine I
no longer have and patch is lost anyway.

ACK.
								Pavel

>  5 files changed, 34 insertions(+), 54 deletions(-)
> 
> --- linux-2.6.orig/include/linux/pm.h	2007-03-21 15:44:54.663148946 +0100
> +++ linux-2.6/include/linux/pm.h	2007-03-21 15:45:04.403148946
                                                             ~~~~~~~~
								wow :-)



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

Re: [PATCH 3/3] power management: implement pm_ops.valid for everybody

[Pavel Machek]

On Wed 2007-03-21 15:53:31, Johannes Berg wrote:
> Almost all users of pm_ops only support mem sleep, don't check in .valid
> and don't reject any others in .prepare so users can be confused if they
> check /sys/power/state, especially when new states are added (these would
> then result in s-t-r although they're supposed to be something different).
> 
> This patch implements a generic pm_valid_only_mem function that is then
> exported for users and puts it to use in almost all existing pm_ops.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> Cc: David Brownell <david-b@pacbell.net>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: linux-pm@lists.linux-foundation.org

Looks ok to me. ACK. (But I guess it would be nice to cc rmk or
something, so we don't conflict with his patches etc?)

								Pavel

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

Re: [PATCH 3/3] power management: implement pm_ops.valid for everybody

[Andrew Morton]

On Thu, 22 Mar 2007 00:51:54 +0100 Pavel Machek <pavel@ucw.cz> wrote:

> On Wed 2007-03-21 15:53:31, Johannes Berg wrote:
> > Almost all users of pm_ops only support mem sleep, don't check in .valid
> > and don't reject any others in .prepare so users can be confused if they
> > check /sys/power/state, especially when new states are added (these would
> > then result in s-t-r although they're supposed to be something different).
> > 
> > This patch implements a generic pm_valid_only_mem function that is then
> > exported for users and puts it to use in almost all existing pm_ops.
> > 
> > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> > Cc: David Brownell <david-b@pacbell.net>
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Cc: linux-pm@lists.linux-foundation.org
> 
> Looks ok to me. ACK. (But I guess it would be nice to cc rmk or
> something, so we don't conflict with his patches etc?)
> 

I copied Russell and Paul on all three patches.