+ rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

+ rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[akpm]

The patch titled
     rework pm_ops pm_disk_mode, kill misuse
has been added to the -mm tree.  Its filename is
     rework-pm_ops-pm_disk_mode-kill-misuse.patch

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
out what to do about this

------------------------------------------------------
Subject: rework pm_ops pm_disk_mode, kill misuse
From: Johannes Berg <johannes@sipsolutions.net>

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

This patch clarifies the pm_disk_mode description.

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

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

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

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: David Brownell <david-b@pacbell.net>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: <linux-pm@lists.linux-foundation.org>
Cc: Len Brown <lenb@kernel.org>
Cc: Russell King <rmk@arm.linux.org.uk>
Cc: Greg KH <greg@kroah.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: Paul Mundt <lethal@linux-sh.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

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

diff -puN arch/arm/common/sharpsl_pm.c~rework-pm_ops-pm_disk_mode-kill-misuse arch/arm/common/sharpsl_pm.c
--- a/arch/arm/common/sharpsl_pm.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/arch/arm/common/sharpsl_pm.c
@@ -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,
diff -puN arch/arm/mach-at91/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse arch/arm/mach-at91/pm.c
--- a/arch/arm/mach-at91/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/arch/arm/mach-at91/pm.c
@@ -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,
diff -puN arch/arm/mach-omap1/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse arch/arm/mach-omap1/pm.c
--- a/arch/arm/mach-omap1/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/arch/arm/mach-omap1/pm.c
@@ -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,
diff -puN arch/arm/mach-omap2/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse arch/arm/mach-omap2/pm.c
--- a/arch/arm/mach-omap2/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/arch/arm/mach-omap2/pm.c
@@ -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,
diff -puN arch/arm/mach-pxa/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse arch/arm/mach-pxa/pm.c
--- a/arch/arm/mach-pxa/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/arch/arm/mach-pxa/pm.c
@@ -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,
diff -puN arch/arm/mach-sa1100/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse arch/arm/mach-sa1100/pm.c
--- a/arch/arm/mach-sa1100/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/arch/arm/mach-sa1100/pm.c
@@ -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,
 };
 
diff -puN arch/arm/plat-s3c24xx/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse arch/arm/plat-s3c24xx/pm.c
--- a/arch/arm/plat-s3c24xx/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/arch/arm/plat-s3c24xx/pm.c
@@ -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,
diff -puN arch/sh/boards/hp6xx/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse arch/sh/boards/hp6xx/pm.c
--- a/arch/sh/boards/hp6xx/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/arch/sh/boards/hp6xx/pm.c
@@ -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,
 };
 
diff -puN include/linux/pm.h~rework-pm_ops-pm_disk_mode-kill-misuse include/linux/pm.h
--- a/include/linux/pm.h~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/include/linux/pm.h
@@ -112,6 +112,8 @@ typedef int __bitwise suspend_state_t;
 
 typedef int __bitwise suspend_disk_method_t;
 
+/* invalid must be 0 so struct pm_ops initialisers can leave it out */
+#define PM_DISK_INVALID		((__force suspend_disk_method_t) 0)
 #define	PM_DISK_FIRMWARE	((__force suspend_disk_method_t) 1)
 #define	PM_DISK_PLATFORM	((__force suspend_disk_method_t) 2)
 #define	PM_DISK_SHUTDOWN	((__force suspend_disk_method_t) 3)
@@ -137,17 +139,16 @@ typedef int __bitwise suspend_disk_metho
  * @finish: Called when the system has left the given state and all devices
  *	are resumed. The return value is ignored.
  *
- * @pm_disk_mode: Set to the disk method that the user should be able to
- *	configure for suspend-to-disk. Since %PM_DISK_SHUTDOWN,
- *	%PM_DISK_REBOOT, %PM_DISK_TEST and %PM_DISK_TESTPROC
- *	are always allowed, currently only %PM_DISK_PLATFORM
- *	makes sense. If the user then choses %PM_DISK_PLATFORM,
- *	the @prepare call will be called before suspending to disk
- *	(if present), the @enter call should be present and will
- *	be called after all state has been saved and the machine
- *	is ready to be shut down/suspended/..., and the @finish
- *	callback is called after state has been restored. All
- *	these calls are called with %PM_SUSPEND_DISK as the state.
+ * @pm_disk_mode: The generic code always allows one of the shutdown methods
+ *	%PM_DISK_SHUTDOWN, %PM_DISK_REBOOT, %PM_DISK_TEST and
+ *	%PM_DISK_TESTPROC. If this variable is set, the mode it is set
+ *	to is allowed in addition to those modes and is also made default.
+ *	When this mode is sent selected, the @prepare call will be called
+ *	before suspending to disk (if present), the @enter call should be
+ *	present and will be called after all state has been saved and the
+ *	machine is ready to be powered off; the @finish callback is called
+ *	after state has been restored. All these calls are called with
+ *	%PM_SUSPEND_DISK as the state.
  */
 struct pm_ops {
 	int (*valid)(suspend_state_t state);
diff -puN kernel/power/disk.c~rework-pm_ops-pm_disk_mode-kill-misuse kernel/power/disk.c
--- a/kernel/power/disk.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/kernel/power/disk.c
@@ -39,7 +39,13 @@ static inline int platform_prepare(void)
 {
 	int error = 0;
 
-	if (pm_disk_mode == PM_DISK_PLATFORM) {
+	switch (pm_disk_mode) {
+	case PM_DISK_TEST:
+	case PM_DISK_TESTPROC:
+	case PM_DISK_SHUTDOWN:
+	case PM_DISK_REBOOT:
+		break;
+	default:
 		if (pm_ops && pm_ops->prepare)
 			error = pm_ops->prepare(PM_SUSPEND_DISK);
 	}
@@ -48,31 +54,33 @@ static inline int platform_prepare(void)
 
 /**
  *	power_down - Shut machine down for hibernate.
- *	@mode:		Suspend-to-disk mode
  *
- *	Use the platform driver, if configured so, and return gracefully if it
- *	fails.
- *	Otherwise, try to power off and reboot. If they fail, halt the machine,
- *	there ain't no turning back.
+ *	Use the platform driver, if configured so; otherwise try
+ *	to power off or reboot.
  */
 
-static void power_down(suspend_disk_method_t mode)
+static void power_down(void)
 {
 	disable_nonboot_cpus();
-	switch(mode) {
-	case PM_DISK_PLATFORM:
-		if (pm_ops && pm_ops->enter) {
-			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
-			pm_ops->enter(PM_SUSPEND_DISK);
-			break;
-		}
+
+	switch (pm_disk_mode) {
+	case PM_DISK_TEST:
+	case PM_DISK_TESTPROC:
+		break;
 	case PM_DISK_SHUTDOWN:
 		kernel_power_off();
 		break;
 	case PM_DISK_REBOOT:
 		kernel_restart(NULL);
 		break;
+	default:
+		if (pm_ops && pm_ops->enter) {
+			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
+			pm_ops->enter(PM_SUSPEND_DISK);
+			break;
+		}
 	}
+
 	kernel_halt();
 	/* Valid image is on the disk, if we continue we risk serious data corruption
 	   after resume. */
@@ -82,7 +90,13 @@ static void power_down(suspend_disk_meth
 
 static inline void platform_finish(void)
 {
-	if (pm_disk_mode == PM_DISK_PLATFORM) {
+	switch (pm_disk_mode) {
+	case PM_DISK_TEST:
+	case PM_DISK_TESTPROC:
+	case PM_DISK_SHUTDOWN:
+	case PM_DISK_REBOOT:
+		break;
+	default:
 		if (pm_ops && pm_ops->finish)
 			pm_ops->finish(PM_SUSPEND_DISK);
 	}
@@ -177,7 +191,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;
@@ -373,10 +387,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;
diff -puN kernel/power/main.c~rework-pm_ops-pm_disk_mode-kill-misuse kernel/power/main.c
--- a/kernel/power/main.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/kernel/power/main.c
@@ -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);
 }
 
_

Patches currently in -mm which might be from johannes@sipsolutions.net are

origin.patch
git-alsa.patch
git-wireless.patch
hack-to-make-wireless-work.patch
rework-pm_ops-pm_disk_mode-kill-misuse.patch
power-management-remove-firmware-disk-mode.patch
power-management-implement-pm_opsvalid-for-everybody.patch

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Russell King]

On Wed, Mar 21, 2007 at 04:34:13PM -0700, akpm@linux-foundation.org wrote:
> It also removes all the arm and sh users that think they can veto suspend to
> disk via pm_ops; not so since the user can always do echo shutdown >
> /sys/power/disk, they need to find a better way involving Kconfig or such.

NAK.  There are ARM platforms where suspend to disk doesn't make any
sense what so ever.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Andrew Morton]

On Thu, 22 Mar 2007 09:48:26 +0000 Russell King <rmk@arm.linux.org.uk> wrote:

> On Wed, Mar 21, 2007 at 04:34:13PM -0700, akpm@linux-foundation.org wrote:
> > It also removes all the arm and sh users that think they can veto suspend to
> > disk via pm_ops; not so since the user can always do echo shutdown >
> > /sys/power/disk, they need to find a better way involving Kconfig or such.
> 
> NAK.  There are ARM platforms where suspend to disk doesn't make any
> sense what so ever.

So they shouldn't offer it in Kconfig?

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Pavel Machek]

On Thu 2007-03-22 01:56:14, Andrew Morton wrote:
> On Thu, 22 Mar 2007 09:48:26 +0000 Russell King <rmk@arm.linux.org.uk> wrote:
> 
> > On Wed, Mar 21, 2007 at 04:34:13PM -0700, akpm@linux-foundation.org wrote:
> > > It also removes all the arm and sh users that think they can veto suspend to
> > > disk via pm_ops; not so since the user can always do echo shutdown >
> > > /sys/power/disk, they need to find a better way involving Kconfig or such.
> > 
> > NAK.  There are ARM platforms where suspend to disk doesn't make any
> > sense what so ever.
> 
> So they shouldn't offer it in Kconfig?

(I thought this was a mistake, when rmk acked all three in next mail,
but maybe I'm mistaken now?)

Anyway, this patch is good, as it allows interested ARM machines to
really disable suspend-to-disk. What they were doing previously was
not effective.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Johannes Berg]

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

On Thu, 2007-03-22 at 11:03 +0100, Pavel Machek wrote:

> > So they shouldn't offer it in Kconfig?

I agree with that.

> Anyway, this patch is good, as it allows interested ARM machines to
> really disable suspend-to-disk. What they were doing previously was
> not effective.

I thought you just said you understood pm_ops now :->

No, once SOFTWARE_SUSPEND is configured there's *no way* to disable it
after the fact via pm_ops. Read valid_state() in kernel/power/main.c.

But that's a design decision, once the kernel config allows suspend to
disk runtime pm_ops have no say in it. And what they previously did by
nack'ing suspend to disk for platform mode is broken and just papers
over the problem in the default case, once the user puts shutdown
> /sys/power/disk the problem shows up again (if there is any).

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: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Russell King]

On Thu, Mar 22, 2007 at 11:08:48AM +0100, Johannes Berg wrote:
> No, once SOFTWARE_SUSPEND is configured there's *no way* to disable it
> after the fact via pm_ops. Read valid_state() in kernel/power/main.c.

So how does a platform with only flash (and an insufficient amount of
flash at that) tell the suspend code that suspend-to-disk is not
supported?

> But that's a design decision, once the kernel config allows suspend to
> disk runtime pm_ops have no say in it.

It sounds like it's a very bad design decision.  Maybe if you consider
that the entire world is desktop PCs then it might make sense.  That
isn't the case though so it doesn't.

Please fix your broken design.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Pavel Machek]

Hi!

> > No, once SOFTWARE_SUSPEND is configured there's *no way* to disable it
> > after the fact via pm_ops. Read valid_state() in kernel/power/main.c.
> 
> So how does a platform with only flash (and an insufficient amount of
> flash at that) tell the suspend code that suspend-to-disk is not
> supported?

Hohum, perhaps it can suspend to SD card? Or perhaps it can suspend
over network?

Suspend code will properly fail if there's not enough swap space.

...plus you are allowed to return -EINVAL from arch_prepare_suspend()
or how is it called.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Vitaly Wool]

On 3/22/07, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
> > > No, once SOFTWARE_SUSPEND is configured there's *no way* to disable it
> > > after the fact via pm_ops. Read valid_state() in kernel/power/main.c.
> >
> > So how does a platform with only flash (and an insufficient amount of
> > flash at that) tell the suspend code that suspend-to-disk is not
> > supported?
>
> Hohum, perhaps it can suspend to SD card? Or perhaps it can suspend
> over network?

Cool, imagine your MP3 player suspending over network... Or suspending
to SD card and switching off in the middle of the process leaving the
SD card filesystem broken. Congratulations, you've bought a nice
device...

Vitaly

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Pavel Machek]

On Thu 2007-03-22 16:41:40, Vitaly Wool wrote:
> On 3/22/07, Pavel Machek <pavel@ucw.cz> wrote:
> >Hi!
> >
> >> > No, once SOFTWARE_SUSPEND is configured there's *no way* to disable it
> >> > after the fact via pm_ops. Read valid_state() in kernel/power/main.c.
> >>
> >> So how does a platform with only flash (and an insufficient amount of
> >> flash at that) tell the suspend code that suspend-to-disk is not
> >> supported?
> >
> >Hohum, perhaps it can suspend to SD card? Or perhaps it can suspend
> >over network?
> 
> Cool, imagine your MP3 player suspending over network... Or suspending
> to SD card and switching off in the middle of the process leaving the
> SD card filesystem broken. Congratulations, you've bought a nice
> device...

Then why did you ask it to suspend, and why did its manufacturer
enable SOFTWARE_SUSPEND?

Unix lets you shoot yourself into the foot. If you do not like that,
go use something else.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Paul Mundt]

On Thu, Mar 22, 2007 at 02:31:17PM +0100, Pavel Machek wrote:
> > > No, once SOFTWARE_SUSPEND is configured there's *no way* to disable it
> > > after the fact via pm_ops. Read valid_state() in kernel/power/main.c.
> > 
> > So how does a platform with only flash (and an insufficient amount of
> > flash at that) tell the suspend code that suspend-to-disk is not
> > supported?
> 
> Hohum, perhaps it can suspend to SD card? Or perhaps it can suspend
> over network?
> 
> Suspend code will properly fail if there's not enough swap space.
> 
> ...plus you are allowed to return -EINVAL from arch_prepare_suspend()
> or how is it called.

So then how exactly is this helping anything? It's pointless to have the
sysfs file visible for platforms that are never going to support suspend
to disk, and -EINVAL'ing out in another path is simply moving the
problem.

The platforms already have a good idea of what sort of suspend they can
cope with, and while I don't see anything against allowing other forms of
suspend to be enabled by the users that really want it, forcing it on
everyone because it's a possibility is nonsense.

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Vitaly Wool]

On 3/22/07, Pavel Machek <pavel@ucw.cz> wrote:
> > Cool, imagine your MP3 player suspending over network... Or suspending
> > to SD card and switching off in the middle of the process leaving the
> > SD card filesystem broken. Congratulations, you've bought a nice
> > device...
>
> Then why did you ask it to suspend, and why did its manufacturer
> enable SOFTWARE_SUSPEND?

Well, the suspend for instance might mean suspend to SRAM, turn clocks
off, put RAM into self-refresh and so on... Sounds familiar?

> Unix lets you shoot yourself into the foot. If you do not like that,
> go use something else.

May I ask you to avoid such generalizations? Thanks!

Vitaly

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Pavel Machek]

On Thu 2007-03-22 23:24:37, Paul Mundt wrote:
> On Thu, Mar 22, 2007 at 02:31:17PM +0100, Pavel Machek wrote:
> > > > No, once SOFTWARE_SUSPEND is configured there's *no way* to disable it
> > > > after the fact via pm_ops. Read valid_state() in kernel/power/main.c.
> > > 
> > > So how does a platform with only flash (and an insufficient amount of
> > > flash at that) tell the suspend code that suspend-to-disk is not
> > > supported?
> > 
> > Hohum, perhaps it can suspend to SD card? Or perhaps it can suspend
> > over network?
> > 
> > Suspend code will properly fail if there's not enough swap space.
> > 
> > ...plus you are allowed to return -EINVAL from arch_prepare_suspend()
> > or how is it called.
> 
> So then how exactly is this helping anything? It's pointless to have the
> sysfs file visible for platforms that are never going to support suspend
> to disk, and -EINVAL'ing out in another path is simply moving the
> problem.

Platforms that are never going to support s2disk 

a) should be fixed

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

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Pavel Machek]

On Thu 2007-03-22 17:27:16, Vitaly Wool wrote:
> On 3/22/07, Pavel Machek <pavel@ucw.cz> wrote:
> >> Cool, imagine your MP3 player suspending over network... Or suspending
> >> to SD card and switching off in the middle of the process leaving the
> >> SD card filesystem broken. Congratulations, you've bought a nice
> >> device...
> >
> >Then why did you ask it to suspend, and why did its manufacturer
> >enable SOFTWARE_SUSPEND?
> 
> Well, the suspend for instance might mean suspend to SRAM, turn clocks
> off, put RAM into self-refresh and so on... Sounds familiar?

And what that has to do with SOFTWARE_SUSPEND?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Paul Mundt]

On Thu, Mar 22, 2007 at 03:33:13PM +0100, Pavel Machek wrote:
> On Thu 2007-03-22 23:24:37, Paul Mundt wrote:
> > On Thu, Mar 22, 2007 at 02:31:17PM +0100, Pavel Machek wrote:
> > > Hohum, perhaps it can suspend to SD card? Or perhaps it can suspend
> > > over network?
> > > 
> > > Suspend code will properly fail if there's not enough swap space.
> > > 
> > > ...plus you are allowed to return -EINVAL from arch_prepare_suspend()
> > > or how is it called.
> > 
> > So then how exactly is this helping anything? It's pointless to have the
> > sysfs file visible for platforms that are never going to support suspend
> > to disk, and -EINVAL'ing out in another path is simply moving the
> > problem.
> 
> Platforms that are never going to support s2disk 
> 
> a) should be fixed
> 
> b) do not have CONFIG_SOFTWARE_SUSPEND.

Ah, that's the part I was missing. Ok, As long as the memory-only thing
is explicit, then I don't see any problem with these.

Acked-by: Paul Mundt <lethal@linux-sh.org>

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Johannes Berg]

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

On Thu, 2007-03-22 at 23:24 +0900, Paul Mundt wrote:

> So then how exactly is this helping anything? It's pointless to have the
> sysfs file visible for platforms that are never going to support suspend
> to disk, and -EINVAL'ing out in another path is simply moving the
> problem.

Just another data point as Pavel has replied to all the points raised
already as far as I can tell...

If you have a platform that doesn't allow CONFIG_SOFTWARE_SUSPEND (which
is only suspend to disk) and doesn't have pm_ops either, then you end up
with an empty /sys/power/state file.

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: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Vitaly Wool]

On 3/22/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> If you have a platform that doesn't allow CONFIG_SOFTWARE_SUSPEND (which
> is only suspend to disk) and doesn't have pm_ops either, then you end up
> with an empty /sys/power/state file.

CONFIG_SOFTWARE_SUSPEND is a misleading name then.

Vitaly

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Johannes Berg]

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

On Thu, 2007-03-22 at 17:27 +0300, Vitaly Wool wrote:

> > Then why did you ask it to suspend, and why did its manufacturer
> > enable SOFTWARE_SUSPEND?
> 
> Well, the suspend for instance might mean suspend to SRAM, turn clocks
> off, put RAM into self-refresh and so on... Sounds familiar?

Reread the help text for config SOFTWARE_SUSPEND?

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: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Vitaly Wool]

On 3/22/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2007-03-22 at 17:27 +0300, Vitaly Wool wrote:
>
> > > Then why did you ask it to suspend, and why did its manufacturer
> > > enable SOFTWARE_SUSPEND?
> >
> > Well, the suspend for instance might mean suspend to SRAM, turn clocks
> > off, put RAM into self-refresh and so on... Sounds familiar?
>
> Reread the help text for config SOFTWARE_SUSPEND?

Yup, thanks, it's like calling a horse a cow.

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Johannes Berg]

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

On Thu, 2007-03-22 at 18:23 +0300, Vitaly Wool wrote:
> On 3/22/07, Johannes Berg <johannes@sipsolutions.net> wrote:
> > If you have a platform that doesn't allow CONFIG_SOFTWARE_SUSPEND (which
> > is only suspend to disk) and doesn't have pm_ops either, then you end up
> > with an empty /sys/power/state file.
> 
> CONFIG_SOFTWARE_SUSPEND is a misleading name then.

Oh, I don't disagree, but the help text for it makes it amply clear:

config SOFTWARE_SUSPEND
[...]
          Enable the suspend to disk (STD) functionality.

          You can suspend your machine with 'echo disk > /sys/power/state'.
          Alternatively, you can use the additional userland tools available
          from <http://suspend.sf.net>.
[...]

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: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Andrew Morton]

On Thu, 22 Mar 2007 23:35:56 +0900 Paul Mundt <lethal@linux-sh.org> wrote:

> Acked-by: Paul Mundt <lethal@linux-sh.org>

Whew.

Johannes, this patch series has caused a lot of confusion.  Perhaps you
could generate an additional couple of overview paragraphs for the changelog
which address the points of confusion, and which prevent the next set of
reviewers from hitting the same things?

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Johannes Berg]

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

On Thu, 2007-03-22 at 09:36 -0800, Andrew Morton wrote:

> Johannes, this patch series has caused a lot of confusion.  

Yeah, I didn't intend to cause that much confusion. Sorry about that.

> Perhaps you
> could generate an additional couple of overview paragraphs for the changelog
> which address the points of confusion, and which prevent the next set of
> reviewers from hitting the same things?

Sure. Most of this is in the pm_ops kerneldoc now. How's this?

This patch series cleans up some misconceptions about pm_ops. Some users
of the pm_ops structure attempt to use it to stop the user from entering
suspend to disk, this, however, is not possible since the user can
always use "shutdown" in /sys/power/disk and then the pm_ops are never
invoked. Also, platforms that don't support suspend to disk simply
should not allow configuring SOFTWARE_SUSPEND (read the help text on it,
it only selects suspend to disk and nothing else, all the other stuff
depends on PM).

The pm_ops structure is actually intended to provide a way to enter
platform-defined sleep states (currently supported states are "standby"
and "mem" (suspend to ram)) and additionally (if SOFTWARE_SUSPEND is
configured) allows a platform to support a platform specific way to
enter low-power mode once everything has been saved to disk. This is
currently only used by ACPI (S4).

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

+ rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[akpm]

The patch titled
     rework pm_ops pm_disk_mode, kill misuse
has been added to the -mm tree.  Its filename is
     rework-pm_ops-pm_disk_mode-kill-misuse.patch

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
out what to do about this

------------------------------------------------------
Subject: rework pm_ops pm_disk_mode, kill misuse
From: Johannes Berg <johannes@sipsolutions.net>

This patch series cleans up some misconceptions about pm_ops.  Some users of
the pm_ops structure attempt to use it to stop the user from entering suspend
to disk, this, however, is not possible since the user can always use
"shutdown" in /sys/power/disk and then the pm_ops are never invoked.  Also,
platforms that don't support suspend to disk simply should not allow
configuring SOFTWARE_SUSPEND (read the help text on it, it only selects
suspend to disk and nothing else, all the other stuff depends on PM).

The pm_ops structure is actually intended to provide a way to enter
platform-defined sleep states (currently supported states are "standby" and
"mem" (suspend to ram)) and additionally (if SOFTWARE_SUSPEND is configured)
allows a platform to support a platform specific way to enter low-power mode
once everything has been saved to disk.  This is currently only used by ACPI
(S4).



This patch:

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

This patch clarifies the pm_disk_mode description.

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

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

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

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: David Brownell <david-b@pacbell.net>
Acked-by: Pavel Machek <pavel@ucw.cz>
Cc: <linux-pm@lists.linux-foundation.org>
Cc: Len Brown <lenb@kernel.org>
Acked-by: Russell King <rmk@arm.linux.org.uk>
Cc: Greg KH <greg@kroah.com>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Acked-by: Paul Mundt <lethal@linux-sh.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

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

diff -puN arch/arm/common/sharpsl_pm.c~rework-pm_ops-pm_disk_mode-kill-misuse arch/arm/common/sharpsl_pm.c
--- a/arch/arm/common/sharpsl_pm.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/arch/arm/common/sharpsl_pm.c
@@ -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,
diff -puN arch/arm/mach-at91/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse arch/arm/mach-at91/pm.c
--- a/arch/arm/mach-at91/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/arch/arm/mach-at91/pm.c
@@ -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,
diff -puN arch/arm/mach-omap1/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse arch/arm/mach-omap1/pm.c
--- a/arch/arm/mach-omap1/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/arch/arm/mach-omap1/pm.c
@@ -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,
diff -puN arch/arm/mach-omap2/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse arch/arm/mach-omap2/pm.c
--- a/arch/arm/mach-omap2/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/arch/arm/mach-omap2/pm.c
@@ -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,
diff -puN arch/arm/mach-pxa/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse arch/arm/mach-pxa/pm.c
--- a/arch/arm/mach-pxa/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/arch/arm/mach-pxa/pm.c
@@ -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,
diff -puN arch/arm/mach-sa1100/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse arch/arm/mach-sa1100/pm.c
--- a/arch/arm/mach-sa1100/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/arch/arm/mach-sa1100/pm.c
@@ -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,
 };
 
diff -puN arch/arm/plat-s3c24xx/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse arch/arm/plat-s3c24xx/pm.c
--- a/arch/arm/plat-s3c24xx/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/arch/arm/plat-s3c24xx/pm.c
@@ -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,
diff -puN arch/sh/boards/hp6xx/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse arch/sh/boards/hp6xx/pm.c
--- a/arch/sh/boards/hp6xx/pm.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/arch/sh/boards/hp6xx/pm.c
@@ -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,
 };
 
diff -puN include/linux/pm.h~rework-pm_ops-pm_disk_mode-kill-misuse include/linux/pm.h
--- a/include/linux/pm.h~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/include/linux/pm.h
@@ -112,6 +112,8 @@ typedef int __bitwise suspend_state_t;
 
 typedef int __bitwise suspend_disk_method_t;
 
+/* invalid must be 0 so struct pm_ops initialisers can leave it out */
+#define PM_DISK_INVALID		((__force suspend_disk_method_t) 0)
 #define	PM_DISK_FIRMWARE	((__force suspend_disk_method_t) 1)
 #define	PM_DISK_PLATFORM	((__force suspend_disk_method_t) 2)
 #define	PM_DISK_SHUTDOWN	((__force suspend_disk_method_t) 3)
@@ -137,17 +139,16 @@ typedef int __bitwise suspend_disk_metho
  * @finish: Called when the system has left the given state and all devices
  *	are resumed. The return value is ignored.
  *
- * @pm_disk_mode: Set to the disk method that the user should be able to
- *	configure for suspend-to-disk. Since %PM_DISK_SHUTDOWN,
- *	%PM_DISK_REBOOT, %PM_DISK_TEST and %PM_DISK_TESTPROC
- *	are always allowed, currently only %PM_DISK_PLATFORM
- *	makes sense. If the user then choses %PM_DISK_PLATFORM,
- *	the @prepare call will be called before suspending to disk
- *	(if present), the @enter call should be present and will
- *	be called after all state has been saved and the machine
- *	is ready to be shut down/suspended/..., and the @finish
- *	callback is called after state has been restored. All
- *	these calls are called with %PM_SUSPEND_DISK as the state.
+ * @pm_disk_mode: The generic code always allows one of the shutdown methods
+ *	%PM_DISK_SHUTDOWN, %PM_DISK_REBOOT, %PM_DISK_TEST and
+ *	%PM_DISK_TESTPROC. If this variable is set, the mode it is set
+ *	to is allowed in addition to those modes and is also made default.
+ *	When this mode is sent selected, the @prepare call will be called
+ *	before suspending to disk (if present), the @enter call should be
+ *	present and will be called after all state has been saved and the
+ *	machine is ready to be powered off; the @finish callback is called
+ *	after state has been restored. All these calls are called with
+ *	%PM_SUSPEND_DISK as the state.
  */
 struct pm_ops {
 	int (*valid)(suspend_state_t state);
diff -puN kernel/power/disk.c~rework-pm_ops-pm_disk_mode-kill-misuse kernel/power/disk.c
--- a/kernel/power/disk.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/kernel/power/disk.c
@@ -39,7 +39,13 @@ static inline int platform_prepare(void)
 {
 	int error = 0;
 
-	if (pm_disk_mode == PM_DISK_PLATFORM) {
+	switch (pm_disk_mode) {
+	case PM_DISK_TEST:
+	case PM_DISK_TESTPROC:
+	case PM_DISK_SHUTDOWN:
+	case PM_DISK_REBOOT:
+		break;
+	default:
 		if (pm_ops && pm_ops->prepare)
 			error = pm_ops->prepare(PM_SUSPEND_DISK);
 	}
@@ -48,31 +54,33 @@ static inline int platform_prepare(void)
 
 /**
  *	power_down - Shut machine down for hibernate.
- *	@mode:		Suspend-to-disk mode
  *
- *	Use the platform driver, if configured so, and return gracefully if it
- *	fails.
- *	Otherwise, try to power off and reboot. If they fail, halt the machine,
- *	there ain't no turning back.
+ *	Use the platform driver, if configured so; otherwise try
+ *	to power off or reboot.
  */
 
-static void power_down(suspend_disk_method_t mode)
+static void power_down(void)
 {
 	disable_nonboot_cpus();
-	switch(mode) {
-	case PM_DISK_PLATFORM:
-		if (pm_ops && pm_ops->enter) {
-			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
-			pm_ops->enter(PM_SUSPEND_DISK);
-			break;
-		}
+
+	switch (pm_disk_mode) {
+	case PM_DISK_TEST:
+	case PM_DISK_TESTPROC:
+		break;
 	case PM_DISK_SHUTDOWN:
 		kernel_power_off();
 		break;
 	case PM_DISK_REBOOT:
 		kernel_restart(NULL);
 		break;
+	default:
+		if (pm_ops && pm_ops->enter) {
+			kernel_shutdown_prepare(SYSTEM_SUSPEND_DISK);
+			pm_ops->enter(PM_SUSPEND_DISK);
+			break;
+		}
 	}
+
 	kernel_halt();
 	/* Valid image is on the disk, if we continue we risk serious data corruption
 	   after resume. */
@@ -82,7 +90,13 @@ static void power_down(suspend_disk_meth
 
 static inline void platform_finish(void)
 {
-	if (pm_disk_mode == PM_DISK_PLATFORM) {
+	switch (pm_disk_mode) {
+	case PM_DISK_TEST:
+	case PM_DISK_TESTPROC:
+	case PM_DISK_SHUTDOWN:
+	case PM_DISK_REBOOT:
+		break;
+	default:
 		if (pm_ops && pm_ops->finish)
 			pm_ops->finish(PM_SUSPEND_DISK);
 	}
@@ -177,7 +191,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;
@@ -365,10 +379,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;
diff -puN kernel/power/main.c~rework-pm_ops-pm_disk_mode-kill-misuse kernel/power/main.c
--- a/kernel/power/main.c~rework-pm_ops-pm_disk_mode-kill-misuse
+++ a/kernel/power/main.c
@@ -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);
 }
 
_

Patches currently in -mm which might be from johannes@sipsolutions.net are

origin.patch
git-alsa.patch
git-wireless.patch
rework-pm_ops-pm_disk_mode-kill-misuse.patch
power-management-remove-firmware-disk-mode.patch
power-management-implement-pm_opsvalid-for-everybody.patch
power-management-force-pm_opsvalid-callback-to-be.patch

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Johannes Berg]

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

On Thu, 2007-03-22 at 10:00 -0800, akpm@linux-foundation.org wrote:
> The patch titled
>      rework pm_ops pm_disk_mode, kill misuse
> has been added to the -mm tree.  Its filename is
>      rework-pm_ops-pm_disk_mode-kill-misuse.patch

Will you be sending this patch for .22? I'd love to get my powermac/pmu
suspend work merged that gets /sys/power/state working instead of using
that pmu specific ioctl, but it depends on this patch to clean up the
mess surrounding pm_disk_mode [1].

johannes

[1] the generic code assumes that PM_DISK_PLATFORM is a great idea in
all cases. I'm surprised that suspend to disk is actually working for
anybody but acpi with S4 by default.

[-- 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: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Andrew Morton]

On Sat, 28 Apr 2007 00:11:20 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:

> On Thu, 2007-03-22 at 10:00 -0800, akpm@linux-foundation.org wrote:
> > The patch titled
> >      rework pm_ops pm_disk_mode, kill misuse
> > has been added to the -mm tree.  Its filename is
> >      rework-pm_ops-pm_disk_mode-kill-misuse.patch
> 
> Will you be sending this patch for .22?

yup.  It's about halfway back in the -mm queue, and I usually don't start
sending the bulk of the -mm patches until halfway through the merge window
(because I have dependencies on subsystem trees, some of which are often
distressingly sluggish).

So the ETA on this one would normally be 1.5 weeks.

> I'd love to get my powermac/pmu
> suspend work merged that gets /sys/power/state working instead of using
> that pmu specific ioctl, but it depends on this patch to clean up the
> mess surrounding pm_disk_mode [1].
> 
> johannes
> 
> [1] the generic code assumes that PM_DISK_PLATFORM is a great idea in
> all cases. I'm surprised that suspend to disk is actually working for
> anybody but acpi with S4 by default.

In which case I'll move

rework-pm_ops-pm_disk_mode-kill-misuse.patch
power-management-remove-firmware-disk-mode.patch
power-management-implement-pm_opsvalid-for-everybody.patch
power-management-force-pm_opsvalid-callback-to-be.patch

to head-of-queue.

As long as it all compiles.  It often turns out that things in this area
have dependencies upon the latest gregkh rampage.  We'll see.

Re: + rework-pm_ops-pm_disk_mode-kill-misuse.patch added to -mm tree

[Johannes Berg]

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

On Fri, 2007-04-27 at 15:55 -0700, Andrew Morton wrote:

> In which case I'll move
> 
> rework-pm_ops-pm_disk_mode-kill-misuse.patch
> power-management-remove-firmware-disk-mode.patch
> power-management-implement-pm_opsvalid-for-everybody.patch
> power-management-force-pm_opsvalid-callback-to-be.patch
> 
> to head-of-queue.

Thanks, much appreciated. If there are any problems the first one is the
only patch my other patches really depend on, the others are just
cleanups I made while in that area to ensure the mess doesn't
proliferate again.

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