[PATCH] rework pm_ops pm_disk_modes foo

[PATCH] rework pm_ops pm_disk_modes foo

[Johannes Berg]

[-- Attachment #1: 011-pm-ops-diskmode.patch --]
[-- Type: text/plain, Size: 12601 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 what pm_disk_mode is actually for by
splitting it up into default_pm_disk_mode and a bitmask of
pm_disk_modes.

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, you need to find a better
way involving Kconfig or such.

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

The patch also sets the default mode to shutdown again, but
when a new pm_ops is registered its default mode is honoured.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: Andriy Skulysh <askulsyh@gmail.com>
Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: David Brownell <david-b@pacbell.net>
Cc: Cliff Brake <cbrake@accelent.com>
Cc: Dirk Behme <dirk.behme@de.bosch.com>
Cc: Nicolas Pitre <nico@cam.org>
Cc: David Singleton <daviado@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Richard Woodruff <r-woodruff2@ti.com>
Cc: Ben Dooks <ben@simtec.co.uk>
Cc: Alexey Starikovskiy <alexey.y.starikovskiy@intel.com>
Cc: David Shaohua Li <shaohua.li@intel.com>
Cc: Patrick Mochel <mochel@osdl.org>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: linux-pm@lists.osdl.org

---
 arch/arm/common/sharpsl_pm.c |    7 ++---
 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    |    9 -------
 arch/arm/plat-s3c24xx/pm.c   |    9 -------
 arch/sh/boards/hp6xx/pm.c    |    7 -----
 drivers/acpi/sleep/main.c    |   10 +++++---
 include/linux/pm.h           |   18 ++++++++++-----
 kernel/power/disk.c          |   51 ++++++++++++++++++++++++++++++-------------
 kernel/power/main.c          |    7 +++++
 12 files changed, 65 insertions(+), 60 deletions(-)

--- linux-2.6.orig/include/linux/pm.h	2007-03-20 02:18:54.830252495 +0100
+++ linux-2.6/include/linux/pm.h	2007-03-20 02:26:32.270252495 +0100
@@ -137,24 +137,30 @@ 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,
+ * @pm_disk_modes: Set to a bitfield of the available shutdown methods
+ *	the user should be able to configure for suspend-to-disk, use
+ *	1<<%PM_DISK_PLATFORM etc. %PM_DISK_SHUTDOWN, %PM_DISK_REBOOT,
+ *	%PM_DISK_TEST and %PM_DISK_TESTPROC are always allowed regardless
+ *	of whether they are included in this mask.
+ *	If the user then choses any one other than those four,
  *	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.
+ *	Note that despite all this, you absolutely must turn off
+ *	the machine and not just suspend it.
+ *
+ * @default_pm_disk_mode: Set to the default suspend to disk method.
  */
 struct pm_ops {
 	int (*valid)(suspend_state_t state);
 	int (*prepare)(suspend_state_t state);
 	int (*enter)(suspend_state_t state);
 	int (*finish)(suspend_state_t state);
-	suspend_disk_method_t pm_disk_mode;
+	unsigned long pm_disk_modes;
+	suspend_disk_method_t default_pm_disk_mode;
 };
 
 /**
--- linux-2.6.orig/kernel/power/disk.c	2007-03-20 02:18:54.960252495 +0100
+++ linux-2.6/kernel/power/disk.c	2007-03-20 02:19:12.750252495 +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);
 	}
@@ -56,23 +62,28 @@ static inline int platform_prepare(void)
  *	there ain't no turning back.
  */
 
-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 +93,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 +184,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,12 +364,16 @@ 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))
+			    ((1<<mode) & pm_ops->pm_disk_modes))
 				pm_disk_mode = mode;
 			else
 				error = -EINVAL;
--- linux-2.6.orig/kernel/power/main.c	2007-03-20 02:18:54.850252495 +0100
+++ linux-2.6/kernel/power/main.c	2007-03-20 02:57:38.400252495 +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,11 @@ void pm_set_ops(struct pm_ops * ops)
 {
 	mutex_lock(&pm_mutex);
 	pm_ops = ops;
+	if (ops && ops->default_pm_disk_mode) {
+		WARN_ON(!( (1<<ops->default_pm_disk_mode) & ops->pm_disk_modes));
+		pm_disk_mode = ops->default_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 02:18:55.080252495 +0100
+++ linux-2.6/arch/arm/common/sharpsl_pm.c	2007-03-20 02:19:12.750252495 +0100
@@ -766,10 +766,9 @@ 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,
+	.prepare		= pxa_pm_prepare,
+	.enter			= corgi_pxa_pm_enter,
+	.finish			= pxa_pm_finish,
 };
 
 static int __init sharpsl_pm_probe(struct platform_device *pdev)
--- linux-2.6.orig/arch/arm/mach-at91/pm.c	2007-03-20 02:18:55.290252495 +0100
+++ linux-2.6/arch/arm/mach-at91/pm.c	2007-03-20 02:19:12.750252495 +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 02:18:55.180252495 +0100
+++ linux-2.6/arch/arm/mach-omap1/pm.c	2007-03-20 02:19:12.760252495 +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 02:18:55.250252495 +0100
+++ linux-2.6/arch/arm/mach-omap2/pm.c	2007-03-20 02:19:12.760252495 +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 02:18:55.350252495 +0100
+++ linux-2.6/arch/arm/mach-pxa/pm.c	2007-03-20 02:19:12.760252495 +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 02:18:55.450252495 +0100
+++ linux-2.6/arch/arm/mach-sa1100/pm.c	2007-03-20 02:19:12.760252495 +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,12 +131,8 @@ 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,
+	.enter			= sa11x0_pm_enter,
 };
 
 static int __init sa11x0_pm_init(void)
--- linux-2.6.orig/arch/arm/plat-s3c24xx/pm.c	2007-03-20 02:18:55.520252495 +0100
+++ linux-2.6/arch/arm/plat-s3c24xx/pm.c	2007-03-20 02:19:12.770252495 +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/drivers/acpi/sleep/main.c	2007-03-20 02:18:55.050252495 +0100
+++ linux-2.6/drivers/acpi/sleep/main.c	2007-03-20 02:19:12.770252495 +0100
@@ -95,7 +95,7 @@ static int acpi_pm_enter(suspend_state_t
 		break;
 
 	case PM_SUSPEND_DISK:
-		if (acpi_pm_ops.pm_disk_mode == PM_DISK_PLATFORM)
+		if (acpi_pm_ops.default_pm_disk_mode == PM_DISK_PLATFORM)
 			status = acpi_enter_sleep_state(acpi_state);
 		break;
 	case PM_SUSPEND_MAX:
@@ -219,8 +219,12 @@ int __init acpi_sleep_init(void)
 			printk(" S%d", i);
 		}
 		if (i == ACPI_STATE_S4) {
-			if (sleep_states[i])
-				acpi_pm_ops.pm_disk_mode = PM_DISK_PLATFORM;
+			if (sleep_states[i]) {
+				acpi_pm_ops.default_pm_disk_mode =
+					PM_DISK_PLATFORM;
+				acpi_pm_ops.pm_disk_modes =
+					1<<PM_DISK_PLATFORM;
+			}
 		}
 	}
 	printk(")\n");
--- linux-2.6.orig/arch/sh/boards/hp6xx/pm.c	2007-03-20 02:18:55.600252495 +0100
+++ linux-2.6/arch/sh/boards/hp6xx/pm.c	2007-03-20 02:19:12.770252495 +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 v2] rework pm_ops pm_disk_modes foo

[Johannes Berg]

[-- Attachment #1: 011-pm-ops-diskmode.patch --]
[-- Type: text/plain, Size: 11564 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, you 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 default mode is honoured.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: David Brownell <david-b@pacbell.net>
Cc: Cliff Brake <cbrake@accelent.com>
Cc: Dirk Behme <dirk.behme@de.bosch.com>
Cc: Nicolas Pitre <nico@cam.org>
Cc: David Singleton <daviado@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Richard Woodruff <r-woodruff2@ti.com>
Cc: Ben Dooks <ben@simtec.co.uk>
Cc: Alexey Starikovskiy <alexey.y.starikovskiy@intel.com>
Cc: David Shaohua Li <shaohua.li@intel.com>
Cc: Patrick Mochel <mochelp@infinity.powertie.org>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: linux-pm@lists.osdl.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           |   25 ++++++++++++---------
 kernel/power/disk.c          |   49 ++++++++++++++++++++++++++++++-------------
 kernel/power/main.c          |    6 ++++-
 11 files changed, 54 insertions(+), 57 deletions(-)

--- linux-2.6.orig/include/linux/pm.h	2007-03-20 02:18:54.830252495 +0100
+++ linux-2.6/include/linux/pm.h	2007-03-20 09:45:16.760886698 +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,18 @@ 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.
+ *
+ * @default_pm_disk_mode: Set to the default suspend to disk method.
  */
 struct pm_ops {
 	int (*valid)(suspend_state_t state);
--- linux-2.6.orig/kernel/power/disk.c	2007-03-20 02:18:54.960252495 +0100
+++ linux-2.6/kernel/power/disk.c	2007-03-20 09:40:54.120886698 +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);
 	}
@@ -56,23 +62,28 @@ static inline int platform_prepare(void)
  *	there ain't no turning back.
  */
 
-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 +93,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 +184,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 +364,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 02:18:54.850252495 +0100
+++ linux-2.6/kernel/power/main.c	2007-03-20 09:44:44.610886698 +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 02:18:55.080252495 +0100
+++ linux-2.6/arch/arm/common/sharpsl_pm.c	2007-03-20 09:42:09.100886698 +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 02:18:55.290252495 +0100
+++ linux-2.6/arch/arm/mach-at91/pm.c	2007-03-20 02:19:12.750252495 +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 02:18:55.180252495 +0100
+++ linux-2.6/arch/arm/mach-omap1/pm.c	2007-03-20 02:19:12.760252495 +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 02:18:55.250252495 +0100
+++ linux-2.6/arch/arm/mach-omap2/pm.c	2007-03-20 02:19:12.760252495 +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 02:18:55.350252495 +0100
+++ linux-2.6/arch/arm/mach-pxa/pm.c	2007-03-20 02:19:12.760252495 +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 02:18:55.450252495 +0100
+++ linux-2.6/arch/arm/mach-sa1100/pm.c	2007-03-20 09:42:32.170886698 +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 02:18:55.520252495 +0100
+++ linux-2.6/arch/arm/plat-s3c24xx/pm.c	2007-03-20 02:19:12.770252495 +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 02:18:55.600252495 +0100
+++ linux-2.6/arch/sh/boards/hp6xx/pm.c	2007-03-20 02:19:12.770252495 +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,
 };
 

--

Re: [PATCH v2] rework pm_ops pm_disk_modes foo

[Pavel Machek]

Hi!

> --- linux-2.6.orig/include/linux/pm.h	2007-03-20 02:18:54.830252495 +0100
> +++ linux-2.6/include/linux/pm.h	2007-03-20 09:45:16.760886698 +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,18 @@ 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.

Is the pm_disk_mode still bitmask? If yes, say so.

...no, it does not appear so.

> --- linux-2.6.orig/kernel/power/main.c	2007-03-20 02:18:54.850252495 +0100
> +++ linux-2.6/kernel/power/main.c	2007-03-20 09:44:44.610886698 +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;

Please don't do this. We want to keep the "use platform if available"
behaviour. [Changing platform->shutdown is really _big_ change,
independend from any cleanups, and it needs to go separate at the very
least. It will break some machines.]

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

Re: [PATCH v2] rework pm_ops pm_disk_modes foo

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 1320 bytes --]

On Tue, 2007-03-20 at 10:31 +0100, Pavel Machek wrote:

> Is the pm_disk_mode still bitmask? If yes, say so.
> 
> ...no, it does not appear so.

No, I should have explained. Since the prepare/enter/finish callbacks
aren't told what the chosen pm_disk_mode is, there is no point in
allowing multiple since they can't differentiate. Changing that is
something that could be done, but doesn't seem necessary since ACPI is
the only user.

> Please don't do this. We want to keep the "use platform if available"
> behaviour. [Changing platform->shutdown is really _big_ change,
> independend from any cleanups, and it needs to go separate at the very
> least. It will break some machines.]

As far as I can tell "use platform if available" means "use platform
when ACPI pm_ops are present" since that's the only pm_ops that has
platform. And notice that I change the default when pm_ops are
registered.

If, of course, my assumption about ACPI here is wrong, then some other
pm_ops implementations will need to be changed to have .pm_disk_mode =
PM_DISK_PLATFORM.

However, using PM_DISK_PLATFORM by default w/o pm_ops support is *wrong*
as I explained previously since it leads to user-interface
inconsistencies, the user can switch *away* from platform but *not back*
to platform.

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2] rework pm_ops pm_disk_modes foo

[Pavel Machek]

Hi!

> > Is the pm_disk_mode still bitmask? If yes, say so.
> > 
> > ...no, it does not appear so.
> 
> No, I should have explained. Since the prepare/enter/finish callbacks
> aren't told what the chosen pm_disk_mode is, there is no point in
> allowing multiple since they can't differentiate. Changing that is
> something that could be done, but doesn't seem necessary since ACPI is
> the only user.

I guess we'll have more such users in future.

> > Please don't do this. We want to keep the "use platform if available"
> > behaviour. [Changing platform->shutdown is really _big_ change,
> > independend from any cleanups, and it needs to go separate at the very
> > least. It will break some machines.]
> 
> As far as I can tell "use platform if available" means "use platform
> when ACPI pm_ops are present" since that's the only pm_ops that has
> platform. And notice that I change the default when pm_ops are
> registered.

i did not look _that_ closely. As long as platform is still default on
acpi systems, we are okay.

> However, using PM_DISK_PLATFORM by default w/o pm_ops support is *wrong*
> as I explained previously since it leads to user-interface
> inconsistencies, the user can switch *away* from platform but *not back*
> to platform.

No argument about that.



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

Re: [PATCH v2] rework pm_ops pm_disk_modes foo

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 343 bytes --]

Hi,

> I guess we'll have more such users in future.

Right, but then the question remains whether multiple disk-shutdown
modes make sense. I doubt it.

> i did not look _that_ closely. As long as platform is still default on
> acpi systems, we are okay.

It is. And there are no other users of pm_ops that can do this.

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH] add firmware disk state and clean up

[Johannes Berg]

This patch adds a new state PM_SUSPEND_FIRMWAREDISK state and cleans up
all pm_ops users to include a .valid callback which in most cases allows
only mem suspend since except ACPI they all don't differentiate between
standby/mem which are currently allowed.

This new state has no users yet but I think APM could be converted to
it. My old laptop used to be able to suspend to disk from firmware, but
it died so I couldn't test such a conversion.

In any case, having a .valid callback will be essential if more suspend
modes are added, and this makes it clearer by example.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
 arch/arm/common/sharpsl_pm.c |    6 ++++++
 arch/arm/mach-at91/pm.c      |    5 +++++
 arch/arm/mach-omap1/pm.c     |    6 +++++-
 arch/arm/mach-omap2/pm.c     |    6 ++++++
 arch/arm/mach-pnx4008/pm.c   |   34 +++-------------------------------
 arch/arm/mach-pxa/pm.c       |    6 ++++++
 arch/arm/mach-sa1100/pm.c    |    6 ++++++
 arch/arm/plat-s3c24xx/pm.c   |   18 +++---------------
 arch/sh/boards/hp6xx/pm.c    |    6 ++++++
 drivers/acpi/sleep/main.c    |   13 +++++++++++--
 include/linux/pm.h           |   16 ++++++++--------
 kernel/power/disk.c          |    3 +--
 kernel/power/main.c          |    9 ++++++---
 13 files changed, 72 insertions(+), 62 deletions(-)

--- linux-2.6.orig/include/linux/pm.h	2007-03-20 10:47:36.543214909 +0100
+++ linux-2.6/include/linux/pm.h	2007-03-20 10:48:06.643214909 +0100
@@ -108,19 +108,19 @@ typedef int __bitwise suspend_state_t;
 #define PM_SUSPEND_STANDBY	((__force suspend_state_t) 1)
 #define PM_SUSPEND_MEM		((__force suspend_state_t) 3)
 #define PM_SUSPEND_DISK		((__force suspend_state_t) 4)
-#define PM_SUSPEND_MAX		((__force suspend_state_t) 5)
+#define PM_SUSPEND_FIRMWAREDISK	((__force suspend_state_t) 5)
+#define PM_SUSPEND_MAX		((__force suspend_state_t) 6)
 
 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)
-#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/arch/arm/common/sharpsl_pm.c	2007-03-20 10:51:02.053214909 +0100
+++ linux-2.6/arch/arm/common/sharpsl_pm.c	2007-03-20 10:51:58.963214909 +0100
@@ -765,10 +765,16 @@ static void sharpsl_apm_get_power_status
 	info->battery_life = sharpsl_pm.battstat.mainbat_percent;
 }
 
+static int pxa_pm_valid(suspend_state_t state)
+{
+	return state == PM_SUSPEND_MEM;
+}
+
 static struct pm_ops sharpsl_pm_ops = {
 	.prepare	= pxa_pm_prepare,
 	.enter		= corgi_pxa_pm_enter,
 	.finish		= pxa_pm_finish,
+	.valid		= pxa_pm_valid,
 };
 
 static int __init sharpsl_pm_probe(struct platform_device *pdev)
--- linux-2.6.orig/arch/arm/mach-at91/pm.c	2007-03-20 10:55:35.433214909 +0100
+++ linux-2.6/arch/arm/mach-at91/pm.c	2007-03-20 10:56:02.243214909 +0100
@@ -199,11 +199,16 @@ error:
 	return 0;
 }
 
+static int at91_pm_valid(suspend_state_t state)
+{
+	return state == PM_SUSPEND_MEM;
+}
 
 static struct pm_ops at91_pm_ops ={
 	.valid		= at91_pm_valid_state,
 	.prepare	= at91_pm_prepare,
 	.enter		= at91_pm_enter,
+	.valid		= at91_pm_valid,
 };
 
 static int __init at91_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-omap1/pm.c	2007-03-20 10:52:17.403214909 +0100
+++ linux-2.6/arch/arm/mach-omap1/pm.c	2007-03-20 10:52:51.483214909 +0100
@@ -695,12 +695,16 @@ static struct irqaction omap_wakeup_irq 
 	.handler	= omap_wakeup_interrupt
 };
 
-
+static int omap_pm_valid(suspend_state_t state)
+{
+	return state == PM_SUSPEND_MEM;
+}
 
 static struct pm_ops omap_pm_ops ={
 	.prepare	= omap_pm_prepare,
 	.enter		= omap_pm_enter,
 	.finish		= omap_pm_finish,
+	.valid		= omap_pm_valid,
 };
 
 static int __init omap_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-omap2/pm.c	2007-03-20 10:52:56.603214909 +0100
+++ linux-2.6/arch/arm/mach-omap2/pm.c	2007-03-20 10:53:22.353214909 +0100
@@ -369,10 +369,16 @@ static int omap2_pm_finish(suspend_state
 	return 0;
 }
 
+static int omap2_pm_valid(suspend_state_t state)
+{
+	return state == PM_SUSPEND_MEM;
+}
+
 static struct pm_ops omap_pm_ops = {
 	.prepare	= omap2_pm_prepare,
 	.enter		= omap2_pm_enter,
 	.finish		= omap2_pm_finish,
+	.valid		= omap2_pm_valid,
 };
 
 int __init omap2_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-pnx4008/pm.c	2007-03-20 10:53:29.803214909 +0100
+++ linux-2.6/arch/arm/mach-pnx4008/pm.c	2007-03-20 11:03:50.133214909 +0100
@@ -115,42 +115,14 @@ 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)
+static int pnx4008_pm_valid(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;
+	return state == PM_SUSPEND_MEM;
 }
 
-/*
- * 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 = pnx4008_pm_valid,
 };
 
 static int __init pnx4008_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-pxa/pm.c	2007-03-20 10:54:29.943214909 +0100
+++ linux-2.6/arch/arm/mach-pxa/pm.c	2007-03-20 10:54:59.263214909 +0100
@@ -223,10 +223,16 @@ int pxa_pm_finish(suspend_state_t state)
 
 EXPORT_SYMBOL_GPL(pxa_pm_finish);
 
+static int pxa_pm_valid(suspend_state_t state)
+{
+	return state == PM_SUSPEND_MEM;
+}
+
 static struct pm_ops pxa_pm_ops = {
 	.prepare	= pxa_pm_prepare,
 	.enter		= pxa_pm_enter,
 	.finish		= pxa_pm_finish,
+	.valid		= pxa_pm_valid,
 };
 
 static int __init pxa_pm_init(void)
--- linux-2.6.orig/arch/arm/mach-sa1100/pm.c	2007-03-20 10:55:05.173214909 +0100
+++ linux-2.6/arch/arm/mach-sa1100/pm.c	2007-03-20 10:55:30.583214909 +0100
@@ -131,8 +131,14 @@ unsigned long sleep_phys_sp(void *sp)
 	return virt_to_phys(sp);
 }
 
+static int sa11x0_pm_valid(suspend_state_t state)
+{
+	return state == PM_STATE_MEM;
+}
+
 static struct pm_ops sa11x0_pm_ops = {
 	.enter		= sa11x0_pm_enter,
+	.valid		= sa11x0_pm_valid,
 };
 
 static int __init sa11x0_pm_init(void)
--- linux-2.6.orig/arch/arm/plat-s3c24xx/pm.c	2007-03-20 10:56:06.093214909 +0100
+++ linux-2.6/arch/arm/plat-s3c24xx/pm.c	2007-03-20 10:56:47.963214909 +0100
@@ -612,26 +612,14 @@ 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)
+static int s3c2410_pm_valid(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;
+	return state == PM_SUSPEND_MEM;
 }
 
 static struct pm_ops s3c2410_pm_ops = {
-	.prepare	= s3c2410_pm_prepare,
 	.enter		= s3c2410_pm_enter,
-	.finish		= s3c2410_pm_finish,
+	.valid		= s3c2410_pm_valid,
 };
 
 /* s3c2410_pm_init
--- linux-2.6.orig/arch/sh/boards/hp6xx/pm.c	2007-03-20 10:57:05.693214909 +0100
+++ linux-2.6/arch/sh/boards/hp6xx/pm.c	2007-03-20 10:58:04.543214909 +0100
@@ -67,8 +67,14 @@ static int hp6x0_pm_enter(suspend_state_
 	return 0;
 }
 
+static int hp6x0_pm_valid(suspend_state_t state)
+{
+	return state == PM_SUSPEND_MEM;
+}
+
 static struct pm_ops hp6x0_pm_ops = {
 	.enter		= hp6x0_pm_enter,
+	.valid		= hp6x0_pm_valid,
 };
 
 static int __init hp6x0_pm_init(void)
--- linux-2.6.orig/drivers/acpi/sleep/main.c	2007-03-20 10:58:09.733214909 +0100
+++ linux-2.6/drivers/acpi/sleep/main.c	2007-03-20 11:02:27.133214909 +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/disk.c	2007-03-20 11:11:57.793214909 +0100
+++ linux-2.6/kernel/power/disk.c	2007-03-20 11:12:13.683214909 +0100
@@ -304,7 +304,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",
@@ -357,7 +356,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/kernel/power/main.c	2007-03-20 11:13:31.173214909 +0100
+++ linux-2.6/kernel/power/main.c	2007-03-20 11:15:30.153214909 +0100
@@ -157,11 +157,14 @@ static void suspend_finish(suspend_state
 
 
 static const char * const pm_states[PM_SUSPEND_MAX] = {
-	[PM_SUSPEND_STANDBY]	= "standby",
-	[PM_SUSPEND_MEM]	= "mem",
+	"standby",
+	"mem",
 #ifdef CONFIG_SOFTWARE_SUSPEND
-	[PM_SUSPEND_DISK]	= "disk",
+	"disk",
+#else
+	NULL,
 #endif
+	"firmware-disk",
 };
 
 static inline int valid_state(suspend_state_t state)

Re: [PATCH] add firmware disk state and clean up

[Pavel Machek]

Hi!

> This patch adds a new state PM_SUSPEND_FIRMWAREDISK state and cleans up
> all pm_ops users to include a .valid callback which in most cases allows
> only mem suspend since except ACPI they all don't differentiate between
> standby/mem which are currently allowed.

> This new state has no users yet but I think APM could be converted

The person doing the conversion should add the state,
sorry... ... actually it makes sense, but I'd prefer not to have it,
yet.

> --- linux-2.6.orig/arch/arm/common/sharpsl_pm.c	2007-03-20 10:51:02.053214909 +0100
> +++ linux-2.6/arch/arm/common/sharpsl_pm.c	2007-03-20 10:51:58.963214909 +0100
> @@ -765,10 +765,16 @@ static void sharpsl_apm_get_power_status
>  	info->battery_life = sharpsl_pm.battstat.mainbat_percent;
>  }
>  
> +static int pxa_pm_valid(suspend_state_t state)
> +{
> +	return state == PM_SUSPEND_MEM;
> +}

create static inline pm_valid_only_mem() somewhere in header, then
share it around the code?

> --- linux-2.6.orig/kernel/power/main.c	2007-03-20 11:13:31.173214909 +0100
> +++ linux-2.6/kernel/power/main.c	2007-03-20 11:15:30.153214909 +0100
> @@ -157,11 +157,14 @@ static void suspend_finish(suspend_state
>  
>  
>  static const char * const pm_states[PM_SUSPEND_MAX] = {
> -	[PM_SUSPEND_STANDBY]	= "standby",
> -	[PM_SUSPEND_MEM]	= "mem",
> +	"standby",
> +	"mem",
>  #ifdef CONFIG_SOFTWARE_SUSPEND
> -	[PM_SUSPEND_DISK]	= "disk",
> +	"disk",
> +#else
> +	NULL,
>  #endif
> +	"firmware-disk",
>  };

I'd keep the c99 syntax here. (And we do not need firmware-disk just
yet).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] add firmware disk state and clean up

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 796 bytes --]

On Tue, 2007-03-20 at 11:25 +0100, Pavel Machek wrote:

> The person doing the conversion should add the state,
> sorry... ... actually it makes sense, but I'd prefer not to have it,
> yet.

That's ok. The patch is here for that person to implement it ;)

> create static inline pm_valid_only_mem() somewhere in header, then
> share it around the code?

Actually, it can't be inline if it is to be assigned to a function
pointer, but it an be an exported function. Will do that.


> I'd keep the c99 syntax here.

I prefer the c99 syntax as well, but at least this gives compile errors
when somebody adds a new state. Then again, I suppose when they add a
new state they'll want to test it and quickly notice that they need to
do something to make it accessible.

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH] remove firmware disk mode

[Johannes Berg]

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>

---
 include/linux/pm.h  |   13 ++++++-------
 kernel/power/disk.c |    3 +--
 2 files changed, 7 insertions(+), 9 deletions(-)

--- linux-2.6.orig/include/linux/pm.h	2007-03-20 11:55:22.663214909 +0100
+++ linux-2.6/include/linux/pm.h	2007-03-20 11:56:09.783214909 +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-20 11:55:23.353214909 +0100
+++ linux-2.6/kernel/power/disk.c	2007-03-20 11:56:09.783214909 +0100
@@ -304,7 +304,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",
@@ -357,7 +356,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;

Re: [PATCH] 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.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

ACK. (You may want to send it to akpm).
								Pavel

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

[PATCH] implement pm_ops.valid for everybody

[Johannes Berg]

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 ever added.

This patch implements a generic pm_valid_only_mem function that is then
exported for users as well as using it in almost all pm_ops.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

---
 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 11:56:09.783214909 +0100
+++ linux-2.6/include/linux/pm.h	2007-03-20 12:01:46.583214909 +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.
@@ -167,6 +170,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 11:55:22.693214909 +0100
+++ linux-2.6/arch/arm/common/sharpsl_pm.c	2007-03-20 11:56:34.173214909 +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 11:55:22.793214909 +0100
+++ linux-2.6/arch/arm/mach-omap1/pm.c	2007-03-20 11:56:34.173214909 +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 11:55:22.823214909 +0100
+++ linux-2.6/arch/arm/mach-omap2/pm.c	2007-03-20 11:56:34.183214909 +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 11:55:22.883214909 +0100
+++ linux-2.6/arch/arm/mach-pnx4008/pm.c	2007-03-20 11:56:34.183214909 +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 11:55:22.983214909 +0100
+++ linux-2.6/arch/arm/mach-pxa/pm.c	2007-03-20 11:56:34.183214909 +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 11:55:23.013214909 +0100
+++ linux-2.6/arch/arm/mach-sa1100/pm.c	2007-03-20 11:56:34.183214909 +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 11:55:23.143214909 +0100
+++ linux-2.6/arch/arm/plat-s3c24xx/pm.c	2007-03-20 11:56:34.223214909 +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 11:55:23.253214909 +0100
+++ linux-2.6/arch/sh/boards/hp6xx/pm.c	2007-03-20 11:56:34.223214909 +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 11:55:23.293214909 +0100
+++ linux-2.6/drivers/acpi/sleep/main.c	2007-03-20 11:56:34.223214909 +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 11:55:23.403214909 +0100
+++ linux-2.6/kernel/power/main.c	2007-03-20 11:56:34.233214909 +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] implement pm_ops.valid for everybody

[Pavel Machek]

Hi!

> 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 ever added.
> 
> This patch implements a generic pm_valid_only_mem function that is then
> exported for users as well as using it in almost all pm_ops.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

ACK. (You may want to send it to akpm).
									Pavel

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

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Tuesday 20 March 2007 4:06 am, 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 ever added.

By the way ... as a note to implementors, it should be trivial to
implement a basic "standby" state that suspends drivers, disables
many clocks, and probably puts DRAM into self-refresh mode, but
uses only the wait-for-interrupt CPU lowpower mode.

A key difference between that and STR would then be that STR does
extra magic, like switching the CPU to a slow clock and then turning
off all the clocks that drive the chip "fast".  Also, that because
it disables so many clocks, the SOC probably can't support as many
types of wakeup events in STR.


I mention this because implementing such a "standby" mode means
that all the platform drivers can start to make their suspend()
and resume() code behave, and userspace tools can be put into
place, before all that tricky/painful STR work gets done.  Also,
because driver wakeup events in such a "standby" mode tend to be
a lot more powerful ... pretty much how a driver using runtime
PM models would work (instead of user-visible "goto sleep").

That is, support for such a "standby" mode is the easiest way
to seed all the PM work on a given platform.  Even though it
doesn't save as much power, it's a useful step towards deeper
power saving modes.

And thus endeth the lesson for today.  I look forward to seeing
a lot more platforms supporting at least minimal PM, now.  ;)

- Dave



 

Re: [PATCH] implement pm_ops.valid for everybody

[Pavel Machek]

Hi!

> > 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 ever added.
> 
> By the way ... as a note to implementors, it should be trivial to
> implement a basic "standby" state that suspends drivers, disables
> many clocks, and probably puts DRAM into self-refresh mode, but
> uses only the wait-for-interrupt CPU lowpower mode.
> 
> A key difference between that and STR would then be that STR does
> extra magic, like switching the CPU to a slow clock and then turning
> off all the clocks that drive the chip "fast".  Also, that because
> it disables so many clocks, the SOC probably can't support as many
> types of wakeup events in STR.
> 
> I mention this because implementing such a "standby" mode means
> that all the platform drivers can start to make their suspend()
> and resume() code behave, and userspace tools can be put into
> place, before all that tricky/painful STR work gets done.  Also,
> because driver wakeup events in such a "standby" mode tend to be
> a lot more powerful ... pretty much how a driver using runtime
> PM models would work (instead of user-visible "goto sleep").

Actually, I second that. "standby" that spins down disks to protect
them for transport is very useful feature, even if it does not save
much power. (Okay, that's notebooks and high-end-zauruses with
spinning disks, but...)
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] implement pm_ops.valid for everybody

[Guennadi Liakhovetski]

On Tue, 20 Mar 2007, David Brownell wrote:

> On Tuesday 20 March 2007 4:06 am, 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 ever added.
> 
> By the way ... as a note to implementors, it should be trivial to
> implement a basic "standby" state that suspends drivers, disables
> many clocks, and probably puts DRAM into self-refresh mode, but
> uses only the wait-for-interrupt CPU lowpower mode.
> 
> A key difference between that and STR would then be that STR does
> extra magic, like switching the CPU to a slow clock and then turning
> off all the clocks that drive the chip "fast".  Also, that because
> it disables so many clocks, the SOC probably can't support as many
> types of wakeup events in STR.

Hm, interesting. What you described above is very similar to what I've 
just implemented for a 8241 based system (linkstation: 
http://ozlabs.org/pipermail/linuxppc-dev/2007-March/thread.html#33203).
But Paul Mackerras suggested to consider it a StR, whereas Johannes Berg 
proposed to call it a standby, which is also what seems to be more logical 
to me. May we agree on some "simple" criteria, like "CPU power on, i.e., 
CPU registers preserved"? If yes - standby, CPU off, registers lost - StR? 
I can imagine CPUs with multiple power sources allowing to switch some of 
them on and off respectively losing / keeping some register sets...

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Wednesday 21 March 2007 2:01 pm, Guennadi Liakhovetski wrote:
> On Tue, 20 Mar 2007, David Brownell wrote:
> > 
> > By the way ... as a note to implementors, it should be trivial to
> > implement a basic "standby" state that suspends drivers, disables
> > many clocks, and probably puts DRAM into self-refresh mode, but
> > uses only the wait-for-interrupt CPU lowpower mode.
> > 
> > A key difference between that and STR would then be that STR does
> > extra magic, like switching the CPU to a slow clock and then turning
> > off all the clocks that drive the chip "fast".  Also, that because
> > it disables so many clocks, the SOC probably can't support as many
> > types of wakeup events in STR.
> 
> Hm, interesting. What you described above is very similar to what I've 
> just implemented for a 8241 based system (linkstation: 
> http://ozlabs.org/pipermail/linuxppc-dev/2007-March/thread.html#33203).

Just the other day.  :)


> But Paul Mackerras suggested to consider it a StR, whereas Johannes Berg 
> proposed to call it a standby, which is also what seems to be more logical 
> to me. 

Seems more like a "standby" to me too -- at least by comparison to
APM and ACPI definitions of "standby", vs what STR involves.


> May we agree on some "simple" criteria, like "CPU power on, i.e.,  
> CPU registers preserved"? If yes - standby, CPU off, registers lost - StR? 

The ACPI spec has some verbiage on those things, which uses
roughly that distinction.

Which is very much an indication of how weak ACPI is.  It
doesn't contemplate typical SOC behavior, which have a wide
variety of system sleep states that leave the CPU on ... and
which may not even *have* (or need!) a "cpu off" state.

My own definition would be more like:  the minimal RAM-based
power-saving system state is "standby".  If the system
implements a deeper RAM-based system sleep state, that's "STR".

If Linux eventually allows more system sleep states than
just "standby", "mem", and "disk", then most of the new
states will probably fit between "standby" (ACPI S1) and
"mem" (ACPI S3).


> I can imagine CPUs with multiple power sources allowing to switch some of 
> them on and off respectively losing / keeping some register sets...

I can see that more readily with SOC designs that have multiple
power domains.  Consider:  CPU, Peripherals-1, Peripherals-2.
That implies several sleep states:

  - all domains powered
  - only peripherals-1 off
  - only peripherals-2 off
  - both peripheral domains off
  - ...
  - all three domains off

Now, I'm not sure that it would be useful to expose all those
states to userspace, but surely an implementor might find it
useful to implement more than one.  In which case, one would
be called "standby", the next "STR", and ... well, Linux PM
can't handle anything else yet.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Guennadi Liakhovetski]

On Wed, 21 Mar 2007, David Brownell wrote:

> On Wednesday 21 March 2007 2:01 pm, Guennadi Liakhovetski wrote:
> 
> > May we agree on some "simple" criteria, like "CPU power on, i.e.,  
> > CPU registers preserved"? If yes - standby, CPU off, registers lost - StR? 
> 
> The ACPI spec has some verbiage on those things, which uses
> roughly that distinction.
> 
> Which is very much an indication of how weak ACPI is.  It
> doesn't contemplate typical SOC behavior, which have a wide
> variety of system sleep states that leave the CPU on ... and
> which may not even *have* (or need!) a "cpu off" state.
> 
> My own definition would be more like:  the minimal RAM-based
> power-saving system state is "standby".  If the system
> implements a deeper RAM-based system sleep state, that's "STR".

Hmmm, this leaves the decision how to call each state COMPLETELY to the 
implementor, doesn't it? For example, in my suspend above, I do put the 
peripheral controller to sleep too, whereas a "minimal RAM-based" suspend 
could leave it on. So, does it mean I already qualify for StR?:-) I am 
sure you have more experience with power management and a better 
understanding of it too, maybe that's the reason why your definition makes 
a good sense to you and not to me. But "suspend to RAM" - I think it would 
be logical to interpret it as "only system RAM contents is preserved. All 
other volatile storage (CPU registers, peripheral internal registers and 
RAM, etc.) contents is lost."

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] implement pm_ops.valid for everybody

[Pavel Machek]

Hi!

> > Which is very much an indication of how weak ACPI is.  It
> > doesn't contemplate typical SOC behavior, which have a wide
> > variety of system sleep states that leave the CPU on ... and
> > which may not even *have* (or need!) a "cpu off" state.
> > 
> > My own definition would be more like:  the minimal RAM-based
> > power-saving system state is "standby".  If the system
> > implements a deeper RAM-based system sleep state, that's "STR".
> 
> Hmmm, this leaves the decision how to call each state COMPLETELY to the 
> implementor, doesn't it?

Is that a problem? If someone is clever enough to implement suspend, I
think we can trust them to name their states right.

(And trust me, we can flame them if not).

(Anyway, my definition would be "mem" == RAM is powered, everything
else is down, except for devices needed for wakeup; "standby" ==
something is powered that can be powered down, we'll fix that in next version).
								Pavel

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

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

[ removed crosspost to linux-arm, members-only ]

On Wednesday 21 March 2007 3:57 pm, Pavel Machek wrote:
> Hi!
> 
> > > Which is very much an indication of how weak ACPI is.  It
> > > doesn't contemplate typical SOC behavior, which have a wide
> > > variety of system sleep states that leave the CPU on ... and
> > > which may not even *have* (or need!) a "cpu off" state.
> > > 
> > > My own definition would be more like:  the minimal RAM-based
> > > power-saving system state is "standby".  If the system
> > > implements a deeper RAM-based system sleep state, that's "STR".
> > 
> > Hmmm, this leaves the decision how to call each state COMPLETELY to the 
> > implementor, doesn't it?

Not really.  Standby is not as deep a sleep state as STR, so it
would be wrong to have it save more power than STR.

And remember that the implementor must make various decisions in
any case, since the SOC probably has half a dozen distinct
low power states, but Linux can only name two of them.


> Is that a problem? If someone is clever enough to implement suspend, I
> think we can trust them to name their states right.

Modulo my earlier comment, showing that you **don't** need to be
especially clever to implement a "standby" on most systems ... !

 
> (And trust me, we can flame them if not).
> 
> (Anyway, my definition would be "mem" == RAM is powered, everything
> else is down, except for devices needed for wakeup; "standby" ==
> something is powered that can be powered down, we'll fix that in next version).

That implies that standby is less desirable, and wouldn't be used much.

That's a false implication.  Among other things, it may be more important
to have various wakeup event sources at moderate power, than to go
without them at lower power.  Simple math:  N hours at 75% power savings,
which lets the system become fully operational at any time; versus those
same hours at 0% power savings (full power), because STR (90% savings)
doesn't support some essential wakeup events.


Moreover, that assumes "powered" is the relevant issue, rather than for
example "clocked".  As in:  power is always applied everywhere, registers
are always preserved, but more clocks are gated off in deeper sleep states.

That "CPU and peripheral state is discarded" notion is NOT generally
applicable, so that it shouldn't be hard-coded into a definition of
what distinguishes the states given the "standby" and "mem" names.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Pavel Machek]

Hi!

> > (And trust me, we can flame them if not).
> > 
> > (Anyway, my definition would be "mem" == RAM is powered, everything
> > else is down, except for devices needed for wakeup; "standby" ==
> > something is powered that can be powered down, we'll fix that in next version).
> 
> That implies that standby is less desirable, and wouldn't be used much.
> 
> That's a false implication.  Among other things, it may be more important
> to have various wakeup event sources at moderate power, than to go
> without them at lower power.  Simple math:  N hours at 75% power savings,
> which lets the system become fully operational at any time; versus those
> same hours at 0% power savings (full power), because STR (90% savings)
> doesn't support some essential wakeup events.

I guess we are violently agreeing. See my "except for devices needed
for wakeup". I should not have added the "next version" note, as
"standby" is useful, too.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] implement pm_ops.valid for everybody

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 452 bytes --]

On Wed, 2007-03-21 at 16:25 -0700, David Brownell wrote:
> [ removed crosspost to linux-arm, members-only ]

My mistake, sorry about that.

> And remember that the implementor must make various decisions in
> any case, since the SOC probably has half a dozen distinct
> low power states, but Linux can only name two of them.

Actually, now that we have everybody using .valid we can add an
arbitrary amount of them if we wish :>

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Thursday 22 March 2007 3:03 am, Johannes Berg wrote:

> > And remember that the implementor must make various decisions in
> > any case, since the SOC probably has half a dozen distinct
> > low power states, but Linux can only name two of them.
> 
> Actually, now that we have everybody using .valid

Seems kind of temporary, so long as pm_set_ops() doesn't
enforce that as a requirement ...


> we can add an 
> arbitrary amount of them if we wish :>

For as many as can be #defined, yes.  But such #defines will
be rare; adding a new one would mean updating pm_states[]
plus maybe other code, and there are other structural issues
with that notion ... especially the way suspend() methods
have no way to determine the semantics of the target state.

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 790 bytes --]

On Thu, 2007-03-22 at 10:10 -0700, David Brownell wrote:

> Seems kind of temporary, so long as pm_set_ops() doesn't
> enforce that as a requirement ...

I just sent a patch to akpm to do exactly that :)

> > we can add an 
> > arbitrary amount of them if we wish :>
> 
> For as many as can be #defined, yes.  But such #defines will
> be rare; adding a new one would mean updating pm_states[]
> plus maybe other code, and there are other structural issues
> with that notion ... 

True. I think pm_states is the only thing that would need to be changed,
but I don't really advocate changing it and introducing dozens of new
states.

> especially the way suspend() methods
> have no way to determine the semantics of the target state.

I can't parse that.

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] implement pm_ops.valid for everybody

[David Brownell]

On Thursday 22 March 2007 10:18 am, Johannes Berg wrote:
> On Thu, 2007-03-22 at 10:10 -0700, David Brownell wrote:
> 
> > especially the way suspend() methods
> > have no way to determine the semantics of the target state.
> 
> I can't parse that.

Look at docs for most any SOC processor and you'll see that
not only does it have a variety of lowpower states, but that
each of them has various rules about what works there and
what doesn't.

So long as driver suspend() methods have no some way to see
how far they "must" shut down for a given target sleep state,
there is only limited utility to defining such states.

That's because the state would only really apply to the cpu
specific code ... drivers must assume the worst case, precluding
the primary reasons to use those less-deep system states.

I just reposted my clk_must_disable() patches; I think it's time
to merge them.  They address that issue for one of the most
common type of rule for system sleep states ... at least, in
terms of interface.  Platform support will still be an issue,
but at least the issue won't be "crippled by bad design".

- Dave

Re: [PATCH] implement pm_ops.valid for everybody

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 1764 bytes --]

On Thu, 2007-03-22 at 11:13 -0700, David Brownell wrote:

> Look at docs for most any SOC processor and you'll see that
> not only does it have a variety of lowpower states, but that
> each of them has various rules about what works there and
> what doesn't.
> 
> So long as driver suspend() methods have no some way to see
> how far they "must" shut down for a given target sleep state,
> there is only limited utility to defining such states.

Oh ok, yeah, I see.

> That's because the state would only really apply to the cpu
> specific code ... drivers must assume the worst case, precluding
> the primary reasons to use those less-deep system states.

Right. That's a thing we'd have to fix in the interface to drivers to
tell them which state we're entering. But that can also get hairy since
the same device driver might support devices that are built into
different systems and might support different things there by being
hooked up differently....

> I just reposted my clk_must_disable() patches; I think it's time
> to merge them.  They address that issue for one of the most
> common type of rule for system sleep states ... at least, in
> terms of interface.  Platform support will still be an issue,
> but at least the issue won't be "crippled by bad design".

I can't speak for any of that, I haven't ever worked with PM on anything
not looking like a big desktop or at least laptop.

In any case, I didn't intend to fix up all that or even make it
possible, so far all I wanted was to clean up the mess surrounding
pm_ops even as it is currently defined (though it wasn't really
well-documented which probably led to the mess, I had to read the code
to understand it, I had no part in writing it.)

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] implement pm_ops.valid for everybody

[Rafael J. Wysocki]

On Wednesday, 21 March 2007 23:57, Pavel Machek wrote:
> Hi!
> 
> > > Which is very much an indication of how weak ACPI is.  It
> > > doesn't contemplate typical SOC behavior, which have a wide
> > > variety of system sleep states that leave the CPU on ... and
> > > which may not even *have* (or need!) a "cpu off" state.
> > > 
> > > My own definition would be more like:  the minimal RAM-based
> > > power-saving system state is "standby".  If the system
> > > implements a deeper RAM-based system sleep state, that's "STR".
> > 
> > Hmmm, this leaves the decision how to call each state COMPLETELY to the 
> > implementor, doesn't it?
> 
> Is that a problem? If someone is clever enough to implement suspend, I
> think we can trust them to name their states right.
> 
> (And trust me, we can flame them if not).
> 
> (Anyway, my definition would be "mem" == RAM is powered, everything
> else is down, except for devices needed for wakeup; "standby" ==
> something is powered that can be powered down, we'll fix that in next version).

I think we can define "standby" a bit more precisely.  Something like:
- processes are frozen,
- devices are suspended,
- nonboot CPUs are down (and in low powered states, if possible),
- "system" devices may or may not be suspended, depending on the platform,
- the boot CPU may or may not be in a low power state, depending on the platform,
- RAM is powered
- wake up need not be BIOS-driven (main difference from "mem")

Greetings,
Rafael

Re: [PATCH] add firmware disk state and clean up

[David Brownell]

On Tuesday 20 March 2007 3:17 am, Johannes Berg wrote:

> --- linux-2.6.orig/arch/arm/mach-at91/pm.c	2007-03-20 10:55:35.433214909 +0100
> +++ linux-2.6/arch/arm/mach-at91/pm.c	2007-03-20 10:56:02.243214909 +0100
> @@ -199,11 +199,16 @@ error:
>  	return 0;
>  }
>  
> +static int at91_pm_valid(suspend_state_t state)
> +{
> +	return state == PM_SUSPEND_MEM;
> +}
>  
>  static struct pm_ops at91_pm_ops ={
>  	.valid		= at91_pm_valid_state,
>  	.prepare	= at91_pm_prepare,
>  	.enter		= at91_pm_enter,
> +	.valid		= at91_pm_valid,
>  };
>  
>  static int __init at91_pm_init(void)

Clearly bogus.  The STR support isn't upstream yet, true, but
you shouldn't modify that code at all.

- Dave

Re: [PATCH] add firmware disk state and clean up

[Pavel Machek]

Hi!

> > +static int at91_pm_valid(suspend_state_t state)
> > +{
> > +	return state == PM_SUSPEND_MEM;
> > +}
> >  
> >  static struct pm_ops at91_pm_ops ={
> >  	.valid		= at91_pm_valid_state,
> >  	.prepare	= at91_pm_prepare,
> >  	.enter		= at91_pm_enter,
> > +	.valid		= at91_pm_valid,
> >  };
> >  
> >  static int __init at91_pm_init(void)
> 
> Clearly bogus.  The STR support isn't upstream yet, true, but

No... look again. This tries to disable suspend-to-disk on ARM, so
that only suspend-to-RAM is enabled. I'd not call it bogus.

The series is needed to stop pm from reporting "platform" on ARM;
"platform" only makes sense on ACPI systems.

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

Re: [PATCH] add firmware disk state and clean up

[David Brownell]

On Tuesday 20 March 2007 3:09 pm, Pavel Machek wrote:
> Hi!
> 
> > > +static int at91_pm_valid(suspend_state_t state)
> > > +{
> > > +	return state == PM_SUSPEND_MEM;
> > > +}
> > >  
> > >  static struct pm_ops at91_pm_ops ={
> > >  	.valid		= at91_pm_valid_state,
         ^^^^^

> > >  	.prepare	= at91_pm_prepare,
> > >  	.enter		= at91_pm_enter,
> > > +	.valid		= at91_pm_valid,
         ^^^^^

> > >  };
> > >  
> > >  static int __init at91_pm_init(void)
> > 
> > Clearly bogus.  The STR support isn't upstream yet, true, but
> 
> No... look again. 

No, YOU look again.  If that compiles, it's a GCC bug.


> This tries to disable suspend-to-disk on ARM, so 
> that only suspend-to-RAM is enabled. I'd not call it bogus.
> 
> The series is needed to stop pm from reporting "platform" on ARM;
> "platform" only makes sense on ACPI systems.
> 
> 								Pavel

Re: [PATCH] rework pm_ops pm_disk_modes foo

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 505 bytes --]

I'll repost these three patches for inclusion once I've gotten some more
feedback, ok?

The three patches currently are:
 * rework pm_ops pm_disk_modes foo
   The pm_ops.pm_disk_mode is used in totally bogus ways, this fixes it,
   clarifies the description and kills all the users that wrongly think
   they can veto suspend to disk that way.
 * remove firmware disk mode
 * implement pm_ops.valid for everybody

After that I can go back to actually making it work on powermac...

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]