Re: [PATCH -mm] PM: Separate hibernation code from suspend code
On Tuesday, 8 May 2007 10:41, Johannes Berg wrote: > On Mon, 2007-05-07 at 23:35 +0200, Rafael J. Wysocki wrote: > > > > Also, you could then simplify all the instances of > > > (hibernation_mode == HIBERNATION_PLATFORM && hibernation_ops) > > > to just > > > (hibernation_mode == HIBERNATION_PLATFORM) > > > in various if statements and other places. > > > > No, that's not a good idea, because of the "reduce code duplication patch" > > that I'd like to go on top of this. I'd rather use 'if (hibernation_ops)' > > here. :-) > > But if you want the user to be able to change away from 'platform' mode > you still have to have > if (hibernation_ops && hibernation_mode == HIBERNATION_PLATFORM) > in most places, no? Yes, and that's why I'm not going to change that. The code from user.c is also going to call these functions in the future, so the full test is necessary. For now, I'll only add the 'else if' check to hibernation_set_ops(). Greetings, Rafael - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] PM: Separate hibernation code from suspend code
On Mon, 2007-05-07 at 23:35 +0200, Rafael J. Wysocki wrote: > > Also, you could then simplify all the instances of > > (hibernation_mode == HIBERNATION_PLATFORM && hibernation_ops) > > to just > > (hibernation_mode == HIBERNATION_PLATFORM) > > in various if statements and other places. > > No, that's not a good idea, because of the "reduce code duplication patch" > that I'd like to go on top of this. I'd rather use 'if (hibernation_ops)' > here. :-) But if you want the user to be able to change away from 'platform' mode you still have to have if (hibernation_ops && hibernation_mode == HIBERNATION_PLATFORM) in most places, no? johannes signature.asc Description: This is a digitally signed message part
Re: [PATCH -mm] PM: Separate hibernation code from suspend code
On Monday, 7 May 2007 13:56, Johannes Berg wrote: > On Mon, 2007-05-07 at 13:46 +0200, Johannes Berg wrote: > > On Sat, 2007-05-05 at 15:50 +0200, Rafael J. Wysocki wrote: > > > > > @@ -60,6 +60,9 @@ void hibernation_set_ops(struct hibernat > > > } > > > mutex_lock(&pm_mutex); > > > hibernation_ops = ops; > > > + if (ops) > > > + hibernation_mode = HIBERNATION_PLATFORM; > > > > else if (hibernation_mode == HIBERNATION_PLATFORM) > > hibernation_mode = HIBERNATION_SHUTDOWN; > > Also, you could then simplify all the instances of > (hibernation_mode == HIBERNATION_PLATFORM && hibernation_ops) > to just > (hibernation_mode == HIBERNATION_PLATFORM) > in various if statements and other places. No, that's not a good idea, because of the "reduce code duplication patch" that I'd like to go on top of this. I'd rather use 'if (hibernation_ops)' here. :-) Greetings, Rafael - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] PM: Separate hibernation code from suspend code
On Mon, 2007-05-07 at 13:46 +0200, Johannes Berg wrote: > On Sat, 2007-05-05 at 15:50 +0200, Rafael J. Wysocki wrote: > > > @@ -60,6 +60,9 @@ void hibernation_set_ops(struct hibernat > > } > > mutex_lock(&pm_mutex); > > hibernation_ops = ops; > > + if (ops) > > + hibernation_mode = HIBERNATION_PLATFORM; > > else if (hibernation_mode == HIBERNATION_PLATFORM) > hibernation_mode = HIBERNATION_SHUTDOWN; Also, you could then simplify all the instances of (hibernation_mode == HIBERNATION_PLATFORM && hibernation_ops) to just (hibernation_mode == HIBERNATION_PLATFORM) in various if statements and other places. johannes signature.asc Description: This is a digitally signed message part
Re: [PATCH -mm] PM: Separate hibernation code from suspend code
On Sat, 2007-05-05 at 15:50 +0200, Rafael J. Wysocki wrote: > @@ -60,6 +60,9 @@ void hibernation_set_ops(struct hibernat > } > mutex_lock(&pm_mutex); > hibernation_ops = ops; > + if (ops) > + hibernation_mode = HIBERNATION_PLATFORM; else if (hibernation_mode == HIBERNATION_PLATFORM) hibernation_mode = HIBERNATION_SHUTDOWN; > + > mutex_unlock(&pm_mutex); johannes signature.asc Description: This is a digitally signed message part
Re: [PATCH -mm] PM: Separate hibernation code from suspend code
Hi! > > > Power off the system instead of halting it if the 'platform' mode of > > > hibernation > > > has been requested, but hibernation_ops is not set. > > > > Ehm, unless you made a mistake in the patch then that shouldn't be > > possible. > > Hmm, right, but the patch is correct nevertheless. :-) > > I see where the problem is. hibernation_mode is 0 by default > (HIBERNATION_INVALID) and it's not changed to anything else later if > defaults are used. > Something like this is necessary, I think: > > --- > From: Rafael J. Wysocki <[EMAIL PROTECTED]> > > Make sure that hibernation_mode is set to a reasonable value by default. > > Signed-off-by: Rafael J. Wysocki <[EMAIL PROTECTED]> ACK. -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] PM: Separate hibernation code from suspend code
> "AM" == Andrew Morton <[EMAIL PROTECTED]> writes: AM> This causes the long-suffering Vaio to fail to power off during AM> suspend to disk. It says "Please power me down manually". Interesting; I'm seeing exactly the same thing on a Vaio TXN17P, which popped up somewhere between Fedora's 2.6.21-1.2116 and 2.6.21-1.2125 kernels, but the changelog there shows little more than wireless fixes and the 2.6.21.1 patch. AM> However `halt -p' still works OK. The same for me. It's odd to think that identical symptoms are caused by something completely different, but I guess this is the kernel. - J< - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] PM: Separate hibernation code from suspend code
On Sat, 2007-05-05 at 14:21 +0200, Rafael J. Wysocki wrote: > > Power off the system instead of halting it if the 'platform' mode of > hibernation > has been requested, but hibernation_ops is not set. Ehm, unless you made a mistake in the patch then that shouldn't be possible. johannes signature.asc Description: This is a digitally signed message part
Re: [PATCH -mm] PM: Separate hibernation code from suspend code
On Saturday, 5 May 2007 08:31, Andrew Morton wrote: > On Fri, 4 May 2007 11:32:31 +0200 "Rafael J. Wysocki" <[EMAIL PROTECTED]> > wrote: > > > Separate the hibernation (aka suspend to disk code) from the other suspend > > code. > > In particular: > > * Remove the definitions related to hibernation from include/linux/pm.h > > * Introduce struct hibernation_ops and a new hibernate() function to > > hibernate > >the system, defined in include/linux/suspend.h > > * Separate suspend code in kernel/power/main.c from hibernation-related > > code > >in kernel/power/disk.c and kernel/power/user.c (with the help of > >hibernation_ops) > > * Switch ACPI (the only user of pm_ops.pm_disk_mode) to hibernation_ops > > This causes the long-suffering Vaio to fail to power off during suspend > to disk. It says "Please power me down manually". Hmm, looks like a failure of acpi_hibernation_enter(). Can you please put a printk() in there, after local_irq_restore()? > > > machine_ops.halt(); points at native_machine_halt(), which is a no-op. > > However `halt -p' still works OK. How come it is not similarly affected? 'halt -p' probably uses acpi_power_off() (which calls acpi_enter_sleep_state(ACPI_STATE_S5)) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] PM: Separate hibernation code from suspend code
On Saturday, 5 May 2007 12:16, Rafael J. Wysocki wrote: > On Saturday, 5 May 2007 08:31, Andrew Morton wrote: > > On Fri, 4 May 2007 11:32:31 +0200 "Rafael J. Wysocki" <[EMAIL PROTECTED]> > > wrote: > > > > > Separate the hibernation (aka suspend to disk code) from the other > > > suspend code. > > > In particular: > > > * Remove the definitions related to hibernation from include/linux/pm.h > > > * Introduce struct hibernation_ops and a new hibernate() function to > > > hibernate > > >the system, defined in include/linux/suspend.h > > > * Separate suspend code in kernel/power/main.c from hibernation-related > > > code > > >in kernel/power/disk.c and kernel/power/user.c (with the help of > > >hibernation_ops) > > > * Switch ACPI (the only user of pm_ops.pm_disk_mode) to hibernation_ops > > > > This causes the long-suffering Vaio to fail to power off during suspend > > to disk. It says "Please power me down manually". > > Hmm, looks like a failure of acpi_hibernation_enter(). Can you please put a > printk() in there, after local_irq_restore()? Anyway, we should power off the system rather than halt it if hibernation_ops is not set. --- From: Rafael J. Wysocki <[EMAIL PROTECTED]> Power off the system instead of halting it if the 'platform' mode of hibernation has been requested, but hibernation_ops is not set. Signed-off-by: Rafael J. Wysocki <[EMAIL PROTECTED]> --- kernel/power/disk.c |6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) Index: linux-2.6.21/kernel/power/disk.c === --- linux-2.6.21.orig/kernel/power/disk.c 2007-05-05 12:24:33.0 +0200 +++ linux-2.6.21/kernel/power/disk.c2007-05-05 13:51:45.0 +0200 @@ -99,9 +99,6 @@ static void power_down(void) case HIBERNATION_TEST: case HIBERNATION_TESTPROC: break; - case HIBERNATION_SHUTDOWN: - kernel_power_off(); - break; case HIBERNATION_REBOOT: kernel_restart(NULL); break; @@ -111,6 +108,9 @@ static void power_down(void) hibernation_ops->enter(); break; } + case HIBERNATION_SHUTDOWN: + kernel_power_off(); + break; } kernel_halt(); /* - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] PM: Separate hibernation code from suspend code
On Saturday, 5 May 2007 14:27, Johannes Berg wrote:
> On Sat, 2007-05-05 at 14:21 +0200, Rafael J. Wysocki wrote:
> >
> > Power off the system instead of halting it if the 'platform' mode of
> > hibernation
> > has been requested, but hibernation_ops is not set.
>
> Ehm, unless you made a mistake in the patch then that shouldn't be
> possible.
Hmm, right, but the patch is correct nevertheless. :-)
I see where the problem is. hibernation_mode is 0 by default
(HIBERNATION_INVALID) and it's not changed to anything else later if
defaults are used.
Something like this is necessary, I think:
---
From: Rafael J. Wysocki <[EMAIL PROTECTED]>
Make sure that hibernation_mode is set to a reasonable value by default.
Signed-off-by: Rafael J. Wysocki <[EMAIL PROTECTED]>
---
kernel/power/disk.c |7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
Index: linux-2.6.21/kernel/power/disk.c
===
--- linux-2.6.21.orig/kernel/power/disk.c 2007-05-05 13:51:45.0
+0200
+++ linux-2.6.21/kernel/power/disk.c2007-05-05 15:45:23.0 +0200
@@ -30,8 +30,6 @@ char resume_file[256] = CONFIG_PM_STD_PA
dev_t swsusp_resume_device;
sector_t swsusp_resume_block;
-static int hibernation_mode;
-
enum {
HIBERNATION_INVALID,
HIBERNATION_PLATFORM,
@@ -45,6 +43,8 @@ enum {
#define HIBERNATION_MAX (__HIBERNATION_AFTER_LAST-1)
#define HIBERNATION_FIRST (HIBERNATION_INVALID + 1)
+static int hibernation_mode = HIBERNATION_SHUTDOWN;
+
struct hibernation_ops *hibernation_ops;
/**
@@ -60,6 +60,9 @@ void hibernation_set_ops(struct hibernat
}
mutex_lock(&pm_mutex);
hibernation_ops = ops;
+ if (ops)
+ hibernation_mode = HIBERNATION_PLATFORM;
+
mutex_unlock(&pm_mutex);
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] PM: Separate hibernation code from suspend code
On Fri, 4 May 2007 11:32:31 +0200 "Rafael J. Wysocki" <[EMAIL PROTECTED]> wrote: > Separate the hibernation (aka suspend to disk code) from the other suspend > code. > In particular: > * Remove the definitions related to hibernation from include/linux/pm.h > * Introduce struct hibernation_ops and a new hibernate() function to > hibernate >the system, defined in include/linux/suspend.h > * Separate suspend code in kernel/power/main.c from hibernation-related code >in kernel/power/disk.c and kernel/power/user.c (with the help of >hibernation_ops) > * Switch ACPI (the only user of pm_ops.pm_disk_mode) to hibernation_ops This causes the long-suffering Vaio to fail to power off during suspend to disk. It says "Please power me down manually". machine_ops.halt(); points at native_machine_halt(), which is a no-op. However `halt -p' still works OK. How come it is not similarly affected? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] PM: Separate hibernation code from suspend code
On Friday, 4 May 2007 19:22, Satyam Sharma wrote:
> Trivial suggestions:
>
> On 5/4/07, Rafael J. Wysocki <[EMAIL PROTECTED]> wrote:
> > --- linux-2.6.21.orig/include/linux/suspend.h 2007-05-02
> > 22:08:58.0 +0200
> > +++ linux-2.6.21/include/linux/suspend.h2007-05-03
> > 15:22:24.0 +0200
> > [...]
> > +/**
> > + * hibernation_set_ops - set the global hibernate operations
> > + * @ops: the hibernation operations to use in subsequent hibernation
> > transitions
> > + */
> > +extern void hibernation_set_ops(struct hibernation_ops *ops);
>
> Comment must go near code, not with prototype in header.
>
> > +/**
> > + * hibernate - hibernate the system
> > + */
> > +extern int hibernate(void);
>
> Pointless comment. You've already written a doc-book comment in the
> code for this function anyway, so could just as well remove this.
The appended patch fixes these problems.
Greetings,
Rafael
---
From: Rafael J. Wysocki <[EMAIL PROTECTED]>
Remove unnecessary comments in include/linux/suspend.h and add a comment
describing hibernation_set_ops() in kernel/power/disk.c .
Signed-off-by: Rafael J. Wysocki <[EMAIL PROTECTED]>
---
include/linux/suspend.h |8
kernel/power/disk.c |5 +
2 files changed, 5 insertions(+), 8 deletions(-)
Index: linux-2.6.21/include/linux/suspend.h
===
--- linux-2.6.21.orig/include/linux/suspend.h 2007-05-05 00:39:53.0
+0200
+++ linux-2.6.21/include/linux/suspend.h2007-05-05 00:43:39.0
+0200
@@ -58,15 +58,7 @@ extern void swsusp_set_page_free(struct
extern void swsusp_unset_page_free(struct page *);
extern unsigned long get_safe_page(gfp_t gfp_mask);
-/**
- * hibernation_set_ops - set the global hibernate operations
- * @ops: the hibernation operations to use in subsequent hibernation
transitions
- */
extern void hibernation_set_ops(struct hibernation_ops *ops);
-
-/**
- * hibernate - hibernate the system
- */
extern int hibernate(void);
#else
static inline void register_nosave_region(unsigned long b, unsigned long e) {}
Index: linux-2.6.21/kernel/power/disk.c
===
--- linux-2.6.21.orig/kernel/power/disk.c 2007-05-05 00:40:02.0
+0200
+++ linux-2.6.21/kernel/power/disk.c2007-05-05 00:43:32.0 +0200
@@ -47,6 +47,11 @@ enum {
struct hibernation_ops *hibernation_ops;
+/**
+ * hibernation_set_ops - set the global hibernate operations
+ * @ops: the hibernation operations to use in subsequent hibernation
transitions
+ */
+
void hibernation_set_ops(struct hibernation_ops *ops)
{
if (ops && !(ops->prepare && ops->enter && ops->finish)) {
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -mm] PM: Separate hibernation code from suspend code
Trivial suggestions: On 5/4/07, Rafael J. Wysocki <[EMAIL PROTECTED]> wrote: --- linux-2.6.21.orig/include/linux/suspend.h 2007-05-02 22:08:58.0 +0200 +++ linux-2.6.21/include/linux/suspend.h2007-05-03 15:22:24.0 +0200 [...] +/** + * hibernation_set_ops - set the global hibernate operations + * @ops: the hibernation operations to use in subsequent hibernation transitions + */ +extern void hibernation_set_ops(struct hibernation_ops *ops); Comment must go near code, not with prototype in header. +/** + * hibernate - hibernate the system + */ +extern int hibernate(void); Pointless comment. You've already written a doc-book comment in the code for this function anyway, so could just as well remove this. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH -mm] PM: Separate hibernation code from suspend code
From: Rafael J. Wysocki <[EMAIL PROTECTED]>, Johannes Berg <[EMAIL PROTECTED]>
Separate the hibernation (aka suspend to disk code) from the other suspend code.
In particular:
* Remove the definitions related to hibernation from include/linux/pm.h
* Introduce struct hibernation_ops and a new hibernate() function to hibernate
the system, defined in include/linux/suspend.h
* Separate suspend code in kernel/power/main.c from hibernation-related code
in kernel/power/disk.c and kernel/power/user.c (with the help of
hibernation_ops)
* Switch ACPI (the only user of pm_ops.pm_disk_mode) to hibernation_ops
Signed-off-by: Rafael J. Wysocki <[EMAIL PROTECTED]>
---
Documentation/power/userland-swsusp.txt | 26 ++--
drivers/acpi/sleep/main.c | 67 +--
drivers/acpi/sleep/proc.c |2
drivers/i2c/chips/tps65010.c|2
include/linux/pm.h | 31 -
include/linux/suspend.h | 32 +
kernel/power/disk.c | 185
kernel/power/main.c | 42 ++-
kernel/power/power.h|7 -
kernel/power/user.c | 13 +-
kernel/sys.c|2
11 files changed, 224 insertions(+), 185 deletions(-)
Index: linux-2.6.21/include/linux/pm.h
===
--- linux-2.6.21.orig/include/linux/pm.h2007-05-02 22:08:58.0
+0200
+++ linux-2.6.21/include/linux/pm.h 2007-05-02 22:09:55.0 +0200
@@ -107,26 +107,11 @@ typedef int __bitwise suspend_state_t;
#define PM_SUSPEND_ON ((__force suspend_state_t) 0)
#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)
-
-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)
-#definePM_DISK_PLATFORM((__force suspend_disk_method_t) 1)
-#definePM_DISK_SHUTDOWN((__force suspend_disk_method_t) 2)
-#definePM_DISK_REBOOT ((__force suspend_disk_method_t) 3)
-#definePM_DISK_TEST((__force suspend_disk_method_t) 4)
-#definePM_DISK_TESTPROC((__force suspend_disk_method_t) 5)
-#definePM_DISK_MAX ((__force suspend_disk_method_t) 6)
+#define PM_SUSPEND_MAX ((__force suspend_state_t) 4)
/**
* struct pm_ops - Callbacks for managing platform dependent suspend states.
* @valid: Callback to determine whether the given state can be entered.
- * If %CONFIG_SOFTWARE_SUSPEND is set then %PM_SUSPEND_DISK is
- * always valid and never passed to this call. If not assigned,
- * no suspend states are valid.
* Valid states are advertised in /sys/power/state but can still
* be rejected by prepare or enter if the conditions aren't right.
* There is a %pm_valid_only_mem function available that can be assigned
@@ -140,24 +125,12 @@ 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: 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);
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;
};
/**
@@ -276,8 +249,6 @@ extern void device_power_up(void);
extern void device_resume(void);
#ifdef CONFIG_PM
-extern suspend_disk_method_t pm_disk_mode;
-
extern int device_suspend(pm_message_t state);
extern int device_prepare_suspend(pm_message_t state);
Index: linux-2.6.21/kernel/power/main.c
===
--- linux-2.6.21.orig/kernel/power/main.c 2007-05-02 22:08:58.0
+0200
+++ linux-2.6.21/kernel/power/main.c2007-05-03 12:17:51.0 +0200
@@ -30,7 +30,6 @@
DEFINE_MUTEX(pm_mutex);
struct pm_ops *pm_
