Skip to contentSkip to conversation

rework pm_ops pm_disk_modes foo

PATCH Superseded Johannes Berg 5 participants 31 messages Mar 20, 2007 – Mar 22, 2007 1 message not in the archive

Subsystems (12)
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

#1 lore raw

PATCH 1/1 rework pm_ops pm_disk_modes foo

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.

Cc Andriy Skulysh <[email protected]>
Cc Richard Purdie <[email protected]>
Cc David Brownell <[email protected]>
Cc Cliff Brake <[email protected]>
Cc Dirk Behme <[email protected]>
Cc Nicolas Pitre <[email protected]>
Cc David Singleton <[email protected]>
Cc Tony Lindgren <[email protected]>
Cc Richard Woodruff <[email protected]>
Cc Ben Dooks <[email protected]>
Cc Alexey Starikovskiy <[email protected]>
Cc David Shaohua Li <[email protected]>
Cc Patrick Mochel <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Pavel Machek <[email protected]>
---
 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(-)
include/linux/pm.h +12 -6
@@ -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;
 };
 
 /**
kernel/power/disk.c +36 -15

92 lines not on this page · raw message

kernel/power/main.c +6 -1
@@ -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);
 }
 
arch/arm/common/sharpsl_pm.c +3 -4
@@ -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)
arch/arm/mach-at91/pm.c +0 -1
@@ -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,
arch/arm/mach-omap1/pm.c +0 -1
@@ -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,
arch/arm/mach-omap2/pm.c +0 -1
@@ -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,
arch/arm/mach-pxa/pm.c +0 -4
@@ -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,
arch/arm/mach-sa1100/pm.c +1 -8
@@ -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)
arch/arm/plat-s3c24xx/pm.c +0 -9
@@ -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,
drivers/acpi/sleep/main.c +7 -3
@@ -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");
arch/sh/boards/hp6xx/pm.c +0 -7
@@ -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,
 };
 

--

#2 lore raw

PATCH v2 1/1 rework pm_ops pm_disk_modes foo

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.

Cc Richard Purdie <[email protected]>
Cc David Brownell <[email protected]>
Cc Cliff Brake <[email protected]>
Cc Dirk Behme <[email protected]>
Cc Nicolas Pitre <[email protected]>
Cc David Singleton <[email protected]>
Cc Tony Lindgren <[email protected]>
Cc Richard Woodruff <[email protected]>
Cc Ben Dooks <[email protected]>
Cc Alexey Starikovskiy <[email protected]>
Cc David Shaohua Li <[email protected]>
Cc Patrick Mochel <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Pavel Machek <[email protected]>
---
 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(-)
include/linux/pm.h +14 -11
@@ -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);
kernel/power/disk.c +35 -14

89 lines not on this page · raw message

kernel/power/main.c +5 -1
@@ -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);
 }
 
arch/arm/common/sharpsl_pm.c +0 -1
@@ -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,
arch/arm/mach-at91/pm.c +0 -1
@@ -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,
arch/arm/mach-omap1/pm.c +0 -1
@@ -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,
arch/arm/mach-omap2/pm.c +0 -1
@@ -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,
arch/arm/mach-pxa/pm.c +0 -4
@@ -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,
arch/arm/mach-sa1100/pm.c +0 -7
@@ -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,
 };
 
arch/arm/plat-s3c24xx/pm.c +0 -9
@@ -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,
arch/sh/boards/hp6xx/pm.c +0 -7
@@ -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,
 };
 

--

#3 lore raw

Hi!

Quoted 26 lines
include/linux/pm.h +2 -11
@@ -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.

include/linux/pm.h typedef int __bitwise suspend_disk_metho · line 151

Quoted 10 lines
include/linux/pm.h +10 -0
+ * @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.

kernel/power/main.c line 33

Quoted 8 lines
kernel/power/main.c +1 -1
@@ -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

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#4 lore raw
Quoted 3 lines · Pavel Machek

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.

Quoted 4 lines

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

#5 lore raw

Hi!

Quoted 9 lines
Quoted 3 lines

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.

Quoted 9 lines
Quoted 4 lines

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.

Quoted 4 lines

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.

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#6 lore raw

Hi,

Quoted 1 line

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.

Quoted 2 lines

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

#7 lore raw

PATCH 1/1 add firmware disk state and clean up

In reply to Pavel Machek

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.

---
 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(-)
include/linux/pm.h +8 -8
@@ -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.
arch/arm/common/sharpsl_pm.c +6 -0
@@ -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)
arch/arm/mach-at91/pm.c +5 -0
@@ -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)
arch/arm/mach-omap1/pm.c +5 -1
@@ -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)
arch/arm/mach-omap2/pm.c +6 -0
@@ -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)
arch/arm/mach-pnx4008/pm.c +3 -31
@@ -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)
arch/arm/mach-pxa/pm.c +6 -0
@@ -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)
arch/arm/mach-sa1100/pm.c +6 -0
@@ -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)
arch/arm/plat-s3c24xx/pm.c +3 -15
@@ -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
arch/sh/boards/hp6xx/pm.c +6 -0
@@ -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)
drivers/acpi/sleep/main.c +11 -2
@@ -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 = {
kernel/power/disk.c +1 -2
@@ -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;
kernel/power/main.c +6 -3
@@ -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)
#8 lore raw

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

Hi!

Quoted 6 lines

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.

arch/arm/common/sharpsl_pm.c static void sharpsl_apm_get_power_status · line 771

Quoted 10 lines
arch/arm/common/sharpsl_pm.c +4 -0
@@ -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?

Quoted 8 lines
kernel/power/main.c +0 -2
@@ -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",

kernel/power/main.c static void suspend_finish(suspend_state · line 168

Quoted 10 lines
kernel/power/main.c +6 -1
+	"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

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#9 lore raw

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

Quoted 3 lines · Pavel Machek

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 ;)

Quoted 2 lines

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.

Quoted 1 line

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

#10 lore raw

PATCH 1/1 remove firmware disk mode

In reply to Pavel Machek

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.

---
 include/linux/pm.h  |   13 ++++++-------
 kernel/power/disk.c |    3 +--
 2 files changed, 7 insertions(+), 9 deletions(-)
include/linux/pm.h +6 -7
@@ -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.
kernel/power/disk.c +1 -2
@@ -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;
#11 lore raw

Re: [PATCH] remove firmware disk mode

Hi!

Quoted 5 lines

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.

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

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#12 lore raw

PATCH 1/1 implement pm_ops.valid for everybody

In reply to Pavel Machek

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.

---
 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(-)
include/linux/pm.h +4 -0
@@ -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
arch/arm/common/sharpsl_pm.c +1 -0
@@ -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)
arch/arm/mach-omap1/pm.c +1 -0
@@ -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)
arch/arm/mach-omap2/pm.c +1 -0
@@ -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)
arch/arm/mach-pnx4008/pm.c +1 -34
@@ -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)
arch/arm/mach-pxa/pm.c +1 -0
@@ -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)
arch/arm/mach-sa1100/pm.c +1 -0
@@ -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)
arch/arm/plat-s3c24xx/pm.c +1 -18
@@ -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
arch/sh/boards/hp6xx/pm.c +1 -0
@@ -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)
drivers/acpi/sleep/main.c +11 -2
@@ -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 = {
kernel/power/main.c +13 -0
@@ -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)
#13 lore raw

Re: [PATCH] implement pm_ops.valid for everybody

Hi!

Quoted 8 lines

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.

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

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#14 lore raw

Re: [PATCH] implement pm_ops.valid for everybody

In reply to Johannes Berg
Quoted 3 lines · 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.

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

#15 lore raw

Re: [PATCH] implement pm_ops.valid for everybody

Hi!

Quoted 15 lines
Quoted 3 lines

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.

Quoted 7 lines

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

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#16 lore raw

Re: [PATCH] implement pm_ops.valid for everybody

In reply to David Brownell
Quoted 10 lines · David Brownell
Quoted 3 lines · 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.

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.

Quoted 5 lines

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

#17 lore raw

Re: [PATCH] implement pm_ops.valid for everybody

Quoted 13 lines · Guennadi Liakhovetski
Quoted 11 lines · David Brownell

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.

Quoted 3 lines

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. :)

Quoted 3 lines

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.

Quoted 2 lines

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).

Quoted 2 lines

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

#18 lore raw

Re: [PATCH] implement pm_ops.valid for everybody

Quoted 8 lines · David Brownell
Quoted 2 lines · Guennadi Liakhovetski

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.

Quoted 8 lines

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

#19 lore raw

Re: [PATCH] implement pm_ops.valid for everybody

Hi!

Quoted 11 lines
Quoted 8 lines

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

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#20 lore raw

Re: [PATCH] implement pm_ops.valid for everybody

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

Quoted 13 lines · Pavel Machek

Hi!

Quoted 11 lines
Quoted 8 lines

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.

Quoted 2 lines

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 ... !

Quoted 5 lines

(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

#21 lore raw

Re: [PATCH] implement pm_ops.valid for everybody

Hi!

Quoted 6 lines
Quoted 5 lines

(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).

Quoted 8 lines

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

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#22 lore raw

Re: [PATCH] implement pm_ops.valid for everybody

In reply to David Brownell
Quoted 1 line · David Brownell

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

My mistake, sorry about that.

Quoted 3 lines

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

#23 lore raw

Re: [PATCH] implement pm_ops.valid for everybody

Quoted 5 lines · Johannes Berg
Quoted 3 lines

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

Quoted 2 lines

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

#24 lore raw

Re: [PATCH] implement pm_ops.valid for everybody

Quoted 2 lines · David Brownell

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 :)

Quoted 7 lines
Quoted 2 lines

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.

Quoted 2 lines

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

I can't parse that.

johannes

#25 lore raw

Re: [PATCH] implement pm_ops.valid for everybody

Quoted 6 lines · Johannes Berg
Quoted 2 lines · David Brownell

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

#26 lore raw

Re: [PATCH] implement pm_ops.valid for everybody

Quoted 8 lines · David Brownell

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.

Quoted 3 lines

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

Quoted 5 lines

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

#27 lore raw

Re: [PATCH] implement pm_ops.valid for everybody

In reply to Pavel Machek
Quoted 14 lines · Pavel Machek

Hi!

Quoted 11 lines
Quoted 8 lines

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?

Quoted 8 lines

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

#28 lore raw

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

In reply to Johannes Berg
Quoted 9 lines · Johannes Berg
arch/arm/mach-at91/pm.c +3 -0
@@ -199,11 +199,16 @@ error:
 	return 0;
 }
 
+static int at91_pm_valid(suspend_state_t state)
+{
+	return state == PM_SUSPEND_MEM;

arch/arm/mach-at91/pm.c error: · line 214

Quoted 10 lines
arch/arm/mach-at91/pm.c +2 -0
+}
 
 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

#29 lore raw

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

Hi!

Quoted 12 lines
Quoted 12 lines
arch/arm/mach-at91/pm.c +5 -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,
 };
Quoted 3 lines
Quoted 1 line

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

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#30 lore raw

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

Quoted 9 lines · Pavel Machek

Hi!

Quoted 7 lines

arch/arm/mach-at91/pm.c error: · line 208

Quoted 7 lines
arch/arm/mach-at91/pm.c +4 -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,
         ^^^^^
Quoted 3 lines
Quoted 3 lines

arch/arm/mach-at91/pm.c error: · line 211

Quoted 3 lines
arch/arm/mach-at91/pm.c +1 -0
 	.prepare	= at91_pm_prepare,
 	.enter		= at91_pm_enter,
+	.valid		= at91_pm_valid,
         ^^^^^
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

};

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.

Quoted 7 lines

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

#31 lore raw
In reply to Johannes Berg

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