[PATCH]: In kernel hibernation, suspend to both

[PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

Hi Rafael,

Here is the same thing, this time with signed off. Looks like all the
memory used for hibernation is being released before power_down(), so I
guess we should be good. The patch is against linux-next.

------------------------------
Enable suspend to both for in-kernel hibernation.

It is often useful to suspend to memory after hibernation image has been
written to disk. If the battery runs out or power is otherwise lost, the
computer will resume from the hibernated image. If not, it will resume
from memory and hibernation image will be discarded.

Signed-off-by: Bojan Smojver <bojan@rexursive.com>
---
 Documentation/power/swsusp.txt |    5 +++++
 kernel/power/hibernate.c       |   36 ++++++++++++++++++++++++++++++++++++
 kernel/power/power.h           |    3 +++
 kernel/power/swap.c            |   28 ++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 0 deletions(-)

diff --git a/Documentation/power/swsusp.txt b/Documentation/power/swsusp.txt
index ac190cf..92341b8 100644
--- a/Documentation/power/swsusp.txt
+++ b/Documentation/power/swsusp.txt
@@ -33,6 +33,11 @@ echo shutdown > /sys/power/disk; echo disk > /sys/power/state
 
 echo platform > /sys/power/disk; echo disk > /sys/power/state
 
+. If you would like to write hibernation image to swap and then suspend
+to RAM (provided your platform supports it), you can try
+
+echo suspend > /sys/power/disk; echo disk > /sys/power/state
+
 . If you have SATA disks, you'll need recent kernels with SATA suspend
 support. For suspend and resume to work, make sure your disk drivers
 are built into kernel -- not modules. [There's way to make
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index e09dfbf..3b5726f 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2003 Open Source Development Lab
  * Copyright (c) 2004 Pavel Machek <pavel@ucw.cz>
  * Copyright (c) 2009 Rafael J. Wysocki, Novell Inc.
+ * Copyright (C) 2012 Bojan Smojver <bojan@rexursive.com>
  *
  * This file is released under the GPLv2.
  */
@@ -44,6 +45,9 @@ enum {
 	HIBERNATION_PLATFORM,
 	HIBERNATION_SHUTDOWN,
 	HIBERNATION_REBOOT,
+#ifdef CONFIG_SUSPEND
+	HIBERNATION_SUSPEND,
+#endif
 	/* keep last */
 	__HIBERNATION_AFTER_LAST
 };
@@ -572,6 +576,10 @@ int hibernation_platform_enter(void)
  */
 static void power_down(void)
 {
+#ifdef CONFIG_SUSPEND
+	int error;
+#endif
+
 	switch (hibernation_mode) {
 	case HIBERNATION_REBOOT:
 		kernel_restart(NULL);
@@ -581,6 +589,25 @@ static void power_down(void)
 	case HIBERNATION_SHUTDOWN:
 		kernel_power_off();
 		break;
+#ifdef CONFIG_SUSPEND
+	case HIBERNATION_SUSPEND:
+		error = suspend_devices_and_enter(PM_SUSPEND_MEM);
+		if (error) {
+			if (hibernation_ops)
+				hibernation_mode = HIBERNATION_PLATFORM;
+			else
+				hibernation_mode = HIBERNATION_SHUTDOWN;
+			power_down();
+		}
+		/*
+		 * Restore swap signature.
+		 */
+		error = swsusp_unmark();
+		if (error)
+			printk(KERN_ERR "PM: Swap will be unusable! "
+			                "Try swapon -a.\n");
+		return;
+#endif
 	}
 	kernel_halt();
 	/*
@@ -814,6 +841,9 @@ static const char * const hibernation_modes[] = {
 	[HIBERNATION_PLATFORM]	= "platform",
 	[HIBERNATION_SHUTDOWN]	= "shutdown",
 	[HIBERNATION_REBOOT]	= "reboot",
+#ifdef CONFIG_SUSPEND
+	[HIBERNATION_SUSPEND]	= "suspend",
+#endif
 };
 
 /*
@@ -854,6 +884,9 @@ static ssize_t disk_show(struct kobject *kobj, struct kobj_attribute *attr,
 		switch (i) {
 		case HIBERNATION_SHUTDOWN:
 		case HIBERNATION_REBOOT:
+#ifdef CONFIG_SUSPEND
+		case HIBERNATION_SUSPEND:
+#endif
 			break;
 		case HIBERNATION_PLATFORM:
 			if (hibernation_ops)
@@ -894,6 +927,9 @@ static ssize_t disk_store(struct kobject *kobj, struct kobj_attribute *attr,
 		switch (mode) {
 		case HIBERNATION_SHUTDOWN:
 		case HIBERNATION_REBOOT:
+#ifdef CONFIG_SUSPEND
+		case HIBERNATION_SUSPEND:
+#endif
 			hibernation_mode = mode;
 			break;
 		case HIBERNATION_PLATFORM:
diff --git a/kernel/power/power.h b/kernel/power/power.h
index b0bd4be..7d4b7ff 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -156,6 +156,9 @@ extern void swsusp_free(void);
 extern int swsusp_read(unsigned int *flags_p);
 extern int swsusp_write(unsigned int flags);
 extern void swsusp_close(fmode_t);
+#ifdef CONFIG_SUSPEND
+extern int swsusp_unmark(void);
+#endif
 
 /* kernel/power/block_io.c */
 extern struct block_device *hib_resume_bdev;
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 11e22c0..83d5051 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -1472,6 +1472,34 @@ void swsusp_close(fmode_t mode)
 	blkdev_put(hib_resume_bdev, mode);
 }
 
+/**
+ *      swsusp_unmark - Unmark swsusp signature in the resume device
+ */
+
+#ifdef CONFIG_SUSPEND
+int swsusp_unmark(void)
+{
+	int error;
+
+	hib_bio_read_page(swsusp_resume_block, swsusp_header, NULL);
+	if (!memcmp(HIBERNATE_SIG,swsusp_header->sig, 10)) {
+		memcpy(swsusp_header->sig,swsusp_header->orig_sig, 10);
+		error = hib_bio_write_page(swsusp_resume_block,
+					swsusp_header, NULL);
+	} else {
+		printk(KERN_ERR "PM: Cannot find swsusp signature!\n");
+		error = -ENODEV;
+	}
+
+	/*
+	 * We just returned from suspend, we don't need the image any more.
+	 */
+	free_all_swap_pages(root_swap);
+
+	return error;
+}
+#endif
+
 static int swsusp_header_init(void)
 {
 	swsusp_header = (struct swsusp_header*) __get_free_page(GFP_KERNEL);
------------------------------

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Srivatsa S. Bhat]

On 05/09/2012 03:52 AM, Bojan Smojver wrote:

> Hi Rafael,
> 
> Here is the same thing, this time with signed off. Looks like all the
> memory used for hibernation is being released before power_down(), so I
> guess we should be good. The patch is against linux-next.
> 
> ------------------------------
> Enable suspend to both for in-kernel hibernation.
> 
> It is often useful to suspend to memory after hibernation image has been
> written to disk. If the battery runs out or power is otherwise lost, the
> computer will resume from the hibernated image. If not, it will resume
> from memory and hibernation image will be discarded.
> 
> Signed-off-by: Bojan Smojver <bojan@rexursive.com>
> ---
>  Documentation/power/swsusp.txt |    5 +++++
>  kernel/power/hibernate.c       |   36 ++++++++++++++++++++++++++++++++++++
>  kernel/power/power.h           |    3 +++
>  kernel/power/swap.c            |   28 ++++++++++++++++++++++++++++
>  4 files changed, 72 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/power/swsusp.txt b/Documentation/power/swsusp.txt
> index ac190cf..92341b8 100644
> --- a/Documentation/power/swsusp.txt
> +++ b/Documentation/power/swsusp.txt
> @@ -33,6 +33,11 @@ echo shutdown > /sys/power/disk; echo disk > /sys/power/state
> 
>  echo platform > /sys/power/disk; echo disk > /sys/power/state
> 
> +. If you would like to write hibernation image to swap and then suspend
> +to RAM (provided your platform supports it), you can try
> +
> +echo suspend > /sys/power/disk; echo disk > /sys/power/state
> +
>  . If you have SATA disks, you'll need recent kernels with SATA suspend
>  support. For suspend and resume to work, make sure your disk drivers
>  are built into kernel -- not modules. [There's way to make
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index e09dfbf..3b5726f 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -5,6 +5,7 @@
>   * Copyright (c) 2003 Open Source Development Lab
>   * Copyright (c) 2004 Pavel Machek <pavel@ucw.cz>
>   * Copyright (c) 2009 Rafael J. Wysocki, Novell Inc.
> + * Copyright (C) 2012 Bojan Smojver <bojan@rexursive.com>
>   *
>   * This file is released under the GPLv2.
>   */
> @@ -44,6 +45,9 @@ enum {
>  	HIBERNATION_PLATFORM,
>  	HIBERNATION_SHUTDOWN,
>  	HIBERNATION_REBOOT,
> +#ifdef CONFIG_SUSPEND
> +	HIBERNATION_SUSPEND,
> +#endif
>  	/* keep last */
>  	__HIBERNATION_AFTER_LAST
>  };
> @@ -572,6 +576,10 @@ int hibernation_platform_enter(void)
>   */
>  static void power_down(void)
>  {
> +#ifdef CONFIG_SUSPEND
> +	int error;
> +#endif
> +
>  	switch (hibernation_mode) {
>  	case HIBERNATION_REBOOT:
>  		kernel_restart(NULL);
> @@ -581,6 +589,25 @@ static void power_down(void)
>  	case HIBERNATION_SHUTDOWN:
>  		kernel_power_off();
>  		break;
> +#ifdef CONFIG_SUSPEND
> +	case HIBERNATION_SUSPEND:
> +		error = suspend_devices_and_enter(PM_SUSPEND_MEM);


I can imagine running into a host of problems here, since the suspend
sequence is not carried out fully, from the beginning.

For example, this will skip sending out the PM_SUSPEND_PREPARE and the
PM_POST_SUSPEND notifiers. Worse, we actually send out the PM_HIBERNATION_PREPARE
and PM_POST_HIBERNATION notifiers and then do a suspend instead, underneath!

(Similar cases for the rest of the notifiers sent during suspend vs hibernation).

Don't we need to handle such things properly, in order to make suspend-to-both
work reliably?

Regards,
Srivatsa S. Bhat

> +		if (error) {
> +			if (hibernation_ops)
> +				hibernation_mode = HIBERNATION_PLATFORM;
> +			else
> +				hibernation_mode = HIBERNATION_SHUTDOWN;
> +			power_down();
> +		}
> +		/*
> +		 * Restore swap signature.
> +		 */
> +		error = swsusp_unmark();
> +		if (error)
> +			printk(KERN_ERR "PM: Swap will be unusable! "
> +			                "Try swapon -a.\n");
> +		return;
> +#endif

>  	}

>  	kernel_halt();
>  	/*

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Wed, 2012-05-09 at 13:40 +0530, Srivatsa S. Bhat wrote:
> > +             error = suspend_devices_and_enter(PM_SUSPEND_MEM);
> 
> 
> I can imagine running into a host of problems here, since the suspend
> sequence is not carried out fully, from the beginning.
> 
> For example, this will skip sending out the PM_SUSPEND_PREPARE and the
> PM_POST_SUSPEND notifiers. Worse, we actually send out the
> PM_HIBERNATION_PREPARE
> and PM_POST_HIBERNATION notifiers and then do a suspend instead,
> underneath!
> 
> (Similar cases for the rest of the notifiers sent during suspend vs
> hibernation).
> 
> Don't we need to handle such things properly, in order to make
> suspend-to-both
> work reliably? 

Honest answer - I have absolutely no idea. I've seen the code of
suspend-utils (i.e. user mode stuff) and it seems to me that it does
exactly this. Could be wrong of course, just like many times before.

Rafael?

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

Bojan Smojver <bojan@rexursive.com> wrote:

>Honest answer - I have absolutely no idea. I've seen the code of
>suspend-utils (i.e. user mode stuff) and it seems to me that it does
>exactly this. Could be wrong of course, just like many times before.

What makes me think that this may not be that bad is the fact that post-resume, it will actually be hibernation code that will be unwinding things. So like this: prepare for hibernation, create image, suspend to memory (equivalent to hibernation failure of some kind, really), resume from memory, unwind from unsuccessful hibernation.

No?

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Srivatsa S. Bhat]

On 05/09/2012 04:41 PM, Bojan Smojver wrote:

> Bojan Smojver <bojan@rexursive.com> wrote:
> 
>> Honest answer - I have absolutely no idea. I've seen the code of
>> suspend-utils (i.e. user mode stuff) and it seems to me that it does
>> exactly this. Could be wrong of course, just like many times before.
> 
> What makes me think that this may not be that bad is the fact that

> post-resume, it will actually be hibernation code that will be unwinding

> things. So like this: prepare for hibernation, create image, suspend to

> memory 


This is the point where your patch gets scary - Suspend is not carried out
in its fullest sense; instead you jump directly to suspend_devices_and_enter().
Luckily, most of the things that happen before this are common between
suspend and hibernation. However, one thing that really stands out is the
notifications: if you directly call suspend_devices_and_enter(), we end
up missing the PM_SUSPEND_PREPARE notifications.

And there is no guarantee that everybody implements the same thing for
both PM_SUSPEND_PREPARE and PM_HIBERNATION_PREPARE notifications. That is
the reason I don't think it is safe.

> (equivalent to hibernation failure of some kind, really), resume

>from memory, unwind from unsuccessful hibernation.
> 
> No?
> 


Regards,
Srivatsa S. Bhat
IBM Linux Technology Center

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Mon, 2012-05-14 at 05:02 +0530, Srivatsa S. Bhat wrote:
> This is the point where your patch gets scary - Suspend is not carried
> out in its fullest sense; instead you jump directly to
> suspend_devices_and_enter(). Luckily, most of the things that happen
> before this are common between suspend and hibernation. However, one
> thing that really stands out is the notifications: if you directly
> call suspend_devices_and_enter(), we end up missing the
> PM_SUSPEND_PREPARE notifications.

I'm guessing because at the point of suspend to RAM here, we are not
really doing the real, full suspend to RAM, things would be different.
At this point, not the whole system is alive - most of it is frozen.
Only devices required for image writing are alive. Or maybe I'm
misunderstanding the hibernation process...

> And there is no guarantee that everybody implements the same thing for
> both PM_SUSPEND_PREPARE and PM_HIBERNATION_PREPARE notifications. That
> is the reason I don't think it is safe.

OK.

So, you would do:

- prepare for hibernation (includes suspension of devices)
- resume devices involved in image writing
- write image
- prepare for suspend
- suspend to RAM
- resume from RAM
- post suspend from RAM
- unwind unsuccessful hibernation

Isn't that going to confuse devices involved in image writing even more
by essentially executing post-resume twice (once for suspend to RAM,
once for hibernation)?

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Alan Stern]

On Mon, 14 May 2012, Bojan Smojver wrote:

> On Mon, 2012-05-14 at 05:02 +0530, Srivatsa S. Bhat wrote:
> > This is the point where your patch gets scary - Suspend is not carried
> > out in its fullest sense; instead you jump directly to
> > suspend_devices_and_enter(). Luckily, most of the things that happen
> > before this are common between suspend and hibernation. However, one
> > thing that really stands out is the notifications: if you directly
> > call suspend_devices_and_enter(), we end up missing the
> > PM_SUSPEND_PREPARE notifications.
> 
> I'm guessing because at the point of suspend to RAM here, we are not
> really doing the real, full suspend to RAM, things would be different.
> At this point, not the whole system is alive - most of it is frozen.
> Only devices required for image writing are alive. Or maybe I'm
> misunderstanding the hibernation process...

Yes, you misunderstand it.  It's documented pretty thoroughly in
Documentation/power/devices.txt and there's a fair amount of kerneldoc
in include/linux/pm.h.  After the memory image has been prepared,
_every_ device goes back to full functionality before the image gets
written out (with maybe a few platform-specific exceptions).

> > And there is no guarantee that everybody implements the same thing for
> > both PM_SUSPEND_PREPARE and PM_HIBERNATION_PREPARE notifications. That
> > is the reason I don't think it is safe.
> 
> OK.
> 
> So, you would do:
> 
> - prepare for hibernation (includes suspension of devices)

No, devices are not suspended -- they are "frozen".  There's a
difference (frozen devices are supposed to remain at full power).

> - resume devices involved in image writing

Resume all devices.  Except that it's not called "resume"; it's called 
"thaw" -- the reverse of freezing.

> - write image
> - prepare for suspend
> - suspend to RAM
> - resume from RAM
> - post suspend from RAM

It's not clear what you mean by this last step.

> - unwind unsuccessful hibernation

Not exactly.  For example, the devices are already back at full power 
so they don't need any "unwinding".  Basically you just have to 
invalidate the hibernation image stored on disk and unfreeze the tasks.

> Isn't that going to confuse devices involved in image writing even more
> by essentially executing post-resume twice (once for suspend to RAM,
> once for hibernation)?

Not if you do it as described above.

Alan Stern

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Sun, 2012-05-13 at 22:25 -0400, Alan Stern wrote:
> Yes, you misunderstand it.

OK, thanks. I did read the rest of your response as well, thanks for the
pointers.

I'll work on redoing the patch with proper prepare to suspend to RAM.

PS. I don't really use user space suspend to both, but whoever is
familiar with it may then have a look at it as well.

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Mon, 2012-05-14 at 12:37 +1000, Bojan Smojver wrote:
> I'll work on redoing the patch with proper prepare to suspend to RAM.

If I'm reading the source of suspend.c correctly, this would then mean
that instead of calling suspend_devices_and_enter() I should be calling
pm_suspend(), correct?

This function calls enter_state(), which in turn does
prepare/suspend/finish.

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Mon, 2012-05-14 at 12:46 +1000, Bojan Smojver wrote:
> If I'm reading the source of suspend.c correctly, this would then mean
> that instead of calling suspend_devices_and_enter() I should be
> calling pm_suspend(), correct?
> 
> This function calls enter_state(), which in turn does
> prepare/suspend/finish. 

Like this?
-----------------------
 Documentation/power/swsusp.txt |    5 +++++
 kernel/power/hibernate.c       |   36 ++++++++++++++++++++++++++++++++++++
 kernel/power/power.h           |    3 +++
 kernel/power/swap.c            |   28 ++++++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 0 deletions(-)

diff --git a/Documentation/power/swsusp.txt b/Documentation/power/swsusp.txt
index ac190cf..92341b8 100644
--- a/Documentation/power/swsusp.txt
+++ b/Documentation/power/swsusp.txt
@@ -33,6 +33,11 @@ echo shutdown > /sys/power/disk; echo disk > /sys/power/state
 
 echo platform > /sys/power/disk; echo disk > /sys/power/state
 
+. If you would like to write hibernation image to swap and then suspend
+to RAM (provided your platform supports it), you can try
+
+echo suspend > /sys/power/disk; echo disk > /sys/power/state
+
 . If you have SATA disks, you'll need recent kernels with SATA suspend
 support. For suspend and resume to work, make sure your disk drivers
 are built into kernel -- not modules. [There's way to make
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index e09dfbf..a4bdf75 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2003 Open Source Development Lab
  * Copyright (c) 2004 Pavel Machek <pavel@ucw.cz>
  * Copyright (c) 2009 Rafael J. Wysocki, Novell Inc.
+ * Copyright (C) 2012 Bojan Smojver <bojan@rexursive.com>
  *
  * This file is released under the GPLv2.
  */
@@ -44,6 +45,9 @@ enum {
 	HIBERNATION_PLATFORM,
 	HIBERNATION_SHUTDOWN,
 	HIBERNATION_REBOOT,
+#ifdef CONFIG_SUSPEND
+	HIBERNATION_SUSPEND,
+#endif
 	/* keep last */
 	__HIBERNATION_AFTER_LAST
 };
@@ -572,6 +576,10 @@ int hibernation_platform_enter(void)
  */
 static void power_down(void)
 {
+#ifdef CONFIG_SUSPEND
+	int error;
+#endif
+
 	switch (hibernation_mode) {
 	case HIBERNATION_REBOOT:
 		kernel_restart(NULL);
@@ -581,6 +589,25 @@ static void power_down(void)
 	case HIBERNATION_SHUTDOWN:
 		kernel_power_off();
 		break;
+#ifdef CONFIG_SUSPEND
+	case HIBERNATION_SUSPEND:
+		error = pm_suspend(PM_SUSPEND_MEM);
+		if (error) {
+			if (hibernation_ops)
+				hibernation_mode = HIBERNATION_PLATFORM;
+			else
+				hibernation_mode = HIBERNATION_SHUTDOWN;
+			power_down();
+		}
+		/*
+		 * Restore swap signature.
+		 */
+		error = swsusp_unmark();
+		if (error)
+			printk(KERN_ERR "PM: Swap will be unusable! "
+			                "Try swapon -a.\n");
+		return;
+#endif
 	}
 	kernel_halt();
 	/*
@@ -814,6 +841,9 @@ static const char * const hibernation_modes[] = {
 	[HIBERNATION_PLATFORM]	= "platform",
 	[HIBERNATION_SHUTDOWN]	= "shutdown",
 	[HIBERNATION_REBOOT]	= "reboot",
+#ifdef CONFIG_SUSPEND
+	[HIBERNATION_SUSPEND]	= "suspend",
+#endif
 };
 
 /*
@@ -854,6 +884,9 @@ static ssize_t disk_show(struct kobject *kobj, struct kobj_attribute *attr,
 		switch (i) {
 		case HIBERNATION_SHUTDOWN:
 		case HIBERNATION_REBOOT:
+#ifdef CONFIG_SUSPEND
+		case HIBERNATION_SUSPEND:
+#endif
 			break;
 		case HIBERNATION_PLATFORM:
 			if (hibernation_ops)
@@ -894,6 +927,9 @@ static ssize_t disk_store(struct kobject *kobj, struct kobj_attribute *attr,
 		switch (mode) {
 		case HIBERNATION_SHUTDOWN:
 		case HIBERNATION_REBOOT:
+#ifdef CONFIG_SUSPEND
+		case HIBERNATION_SUSPEND:
+#endif
 			hibernation_mode = mode;
 			break;
 		case HIBERNATION_PLATFORM:
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 98f3622..07b4a68 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -156,6 +156,9 @@ extern void swsusp_free(void);
 extern int swsusp_read(unsigned int *flags_p);
 extern int swsusp_write(unsigned int flags);
 extern void swsusp_close(fmode_t);
+#ifdef CONFIG_SUSPEND
+extern int swsusp_unmark(void);
+#endif
 
 /* kernel/power/block_io.c */
 extern struct block_device *hib_resume_bdev;
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index eef311a..1027da8 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -1456,6 +1456,34 @@ void swsusp_close(fmode_t mode)
 	blkdev_put(hib_resume_bdev, mode);
 }
 
+/**
+ *      swsusp_unmark - Unmark swsusp signature in the resume device
+ */
+
+#ifdef CONFIG_SUSPEND
+int swsusp_unmark(void)
+{
+	int error;
+
+	hib_bio_read_page(swsusp_resume_block, swsusp_header, NULL);
+	if (!memcmp(HIBERNATE_SIG,swsusp_header->sig, 10)) {
+		memcpy(swsusp_header->sig,swsusp_header->orig_sig, 10);
+		error = hib_bio_write_page(swsusp_resume_block,
+					swsusp_header, NULL);
+	} else {
+		printk(KERN_ERR "PM: Cannot find swsusp signature!\n");
+		error = -ENODEV;
+	}
+
+	/*
+	 * We just returned from suspend, we don't need the image any more.
+	 */
+	free_all_swap_pages(root_swap);
+
+	return error;
+}
+#endif
+
 static int swsusp_header_init(void)
 {
 	swsusp_header = (struct swsusp_header*) __get_free_page(GFP_KERNEL);
-----------------------

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Mon, 2012-05-14 at 12:58 +1000, Bojan Smojver wrote:
> +               error = pm_suspend(PM_SUSPEND_MEM);

Er, that won't work. The lock is being held by the caller. OK, we'll
have to make it complicated, I guess...

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Mon, 2012-05-14 at 17:45 +1000, Bojan Smojver wrote:
> On Mon, 2012-05-14 at 12:58 +1000, Bojan Smojver wrote:
> > +               error = pm_suspend(PM_SUSPEND_MEM);
> 
> Er, that won't work. The lock is being held by the caller. OK, we'll
> have to make it complicated, I guess...

Totally untested...
-------------------
 Documentation/power/swsusp.txt |    5 +++
 kernel/power/hibernate.c       |   36 ++++++++++++++++++++
 kernel/power/power.h           |    6 ++++
 kernel/power/suspend.c         |   71 ++++++++++++++++++++++++++++++++--------
 kernel/power/swap.c            |   28 ++++++++++++++++
 5 files changed, 133 insertions(+), 13 deletions(-)

diff --git a/Documentation/power/swsusp.txt b/Documentation/power/swsusp.txt
index ac190cf..92341b8 100644
--- a/Documentation/power/swsusp.txt
+++ b/Documentation/power/swsusp.txt
@@ -33,6 +33,11 @@ echo shutdown > /sys/power/disk; echo disk > /sys/power/state
 
 echo platform > /sys/power/disk; echo disk > /sys/power/state
 
+. If you would like to write hibernation image to swap and then suspend
+to RAM (provided your platform supports it), you can try
+
+echo suspend > /sys/power/disk; echo disk > /sys/power/state
+
 . If you have SATA disks, you'll need recent kernels with SATA suspend
 support. For suspend and resume to work, make sure your disk drivers
 are built into kernel -- not modules. [There's way to make
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index e09dfbf..f48a9ba 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2003 Open Source Development Lab
  * Copyright (c) 2004 Pavel Machek <pavel@ucw.cz>
  * Copyright (c) 2009 Rafael J. Wysocki, Novell Inc.
+ * Copyright (C) 2012 Bojan Smojver <bojan@rexursive.com>
  *
  * This file is released under the GPLv2.
  */
@@ -44,6 +45,9 @@ enum {
 	HIBERNATION_PLATFORM,
 	HIBERNATION_SHUTDOWN,
 	HIBERNATION_REBOOT,
+#ifdef CONFIG_SUSPEND
+	HIBERNATION_SUSPEND,
+#endif
 	/* keep last */
 	__HIBERNATION_AFTER_LAST
 };
@@ -572,6 +576,10 @@ int hibernation_platform_enter(void)
  */
 static void power_down(void)
 {
+#ifdef CONFIG_SUSPEND
+	int error;
+#endif
+
 	switch (hibernation_mode) {
 	case HIBERNATION_REBOOT:
 		kernel_restart(NULL);
@@ -581,6 +589,25 @@ static void power_down(void)
 	case HIBERNATION_SHUTDOWN:
 		kernel_power_off();
 		break;
+#ifdef CONFIG_SUSPEND
+	case HIBERNATION_SUSPEND:
+		error = suspend_locked(PM_SUSPEND_MEM);
+		if (error) {
+			if (hibernation_ops)
+				hibernation_mode = HIBERNATION_PLATFORM;
+			else
+				hibernation_mode = HIBERNATION_SHUTDOWN;
+			power_down();
+		}
+		/*
+		 * Restore swap signature.
+		 */
+		error = swsusp_unmark();
+		if (error)
+			printk(KERN_ERR "PM: Swap will be unusable! "
+			                "Try swapon -a.\n");
+		return;
+#endif
 	}
 	kernel_halt();
 	/*
@@ -814,6 +841,9 @@ static const char * const hibernation_modes[] = {
 	[HIBERNATION_PLATFORM]	= "platform",
 	[HIBERNATION_SHUTDOWN]	= "shutdown",
 	[HIBERNATION_REBOOT]	= "reboot",
+#ifdef CONFIG_SUSPEND
+	[HIBERNATION_SUSPEND]	= "suspend",
+#endif
 };
 
 /*
@@ -854,6 +884,9 @@ static ssize_t disk_show(struct kobject *kobj, struct kobj_attribute *attr,
 		switch (i) {
 		case HIBERNATION_SHUTDOWN:
 		case HIBERNATION_REBOOT:
+#ifdef CONFIG_SUSPEND
+		case HIBERNATION_SUSPEND:
+#endif
 			break;
 		case HIBERNATION_PLATFORM:
 			if (hibernation_ops)
@@ -894,6 +927,9 @@ static ssize_t disk_store(struct kobject *kobj, struct kobj_attribute *attr,
 		switch (mode) {
 		case HIBERNATION_SHUTDOWN:
 		case HIBERNATION_REBOOT:
+#ifdef CONFIG_SUSPEND
+		case HIBERNATION_SUSPEND:
+#endif
 			hibernation_mode = mode;
 			break;
 		case HIBERNATION_PLATFORM:
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 98f3622..cb8e09e 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -156,6 +156,9 @@ extern void swsusp_free(void);
 extern int swsusp_read(unsigned int *flags_p);
 extern int swsusp_write(unsigned int flags);
 extern void swsusp_close(fmode_t);
+#ifdef CONFIG_SUSPEND
+extern int swsusp_unmark(void);
+#endif
 
 /* kernel/power/block_io.c */
 extern struct block_device *hib_resume_bdev;
@@ -177,6 +180,9 @@ extern const char *const pm_states[];
 
 extern bool valid_state(suspend_state_t state);
 extern int suspend_devices_and_enter(suspend_state_t state);
+#ifdef CONFIG_HIBERNATION
+extern int suspend_locked(suspend_state_t state);
+#endif
 #else /* !CONFIG_SUSPEND */
 static inline int suspend_devices_and_enter(suspend_state_t state)
 {
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 396d262..de4fedb 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -261,20 +261,15 @@ static void suspend_finish(void)
  * enter_state - Do common work needed to enter system sleep state.
  * @state: System sleep state to enter.
  *
- * Make sure that no one else is trying to put the system into a sleep state.
- * Fail if that's not the case.  Otherwise, prepare for system suspend, make the
- * system enter the given sleep state and clean up after wakeup.
+ * The system lock needs to be held already when this function is called.
  */
-static int enter_state(suspend_state_t state)
+static int enter_state_locked(suspend_state_t state)
 {
 	int error;
 
 	if (!valid_state(state))
 		return -ENODEV;
 
-	if (!mutex_trylock(&pm_mutex))
-		return -EBUSY;
-
 	printk(KERN_INFO "PM: Syncing filesystems ... ");
 	sys_sync();
 	printk("done.\n");
@@ -282,7 +277,7 @@ static int enter_state(suspend_state_t state)
 	pr_debug("PM: Preparing system for %s sleep\n", pm_states[state]);
 	error = suspend_prepare();
 	if (error)
-		goto Unlock;
+		return error;
 
 	if (suspend_test(TEST_FREEZER))
 		goto Finish;
@@ -295,26 +290,50 @@ static int enter_state(suspend_state_t state)
  Finish:
 	pr_debug("PM: Finishing wakeup.\n");
 	suspend_finish();
- Unlock:
+	return error;
+}
+
+/**
+ * enter_state - Do common work needed to enter system sleep state.
+ * @state: System sleep state to enter.
+ *
+ * Make sure that no one else is trying to put the system into a sleep state.
+ * Fail if that's not the case.  Otherwise, prepare for system suspend, make the
+ * system enter the given sleep state and clean up after wakeup.
+ */
+static int enter_state(suspend_state_t state)
+{
+	int error;
+
+	if (!valid_state(state))
+		return -ENODEV;
+
+	if (!mutex_trylock(&pm_mutex))
+		return -EBUSY;
+
+	error = enter_state_locked(state);
+
 	mutex_unlock(&pm_mutex);
 	return error;
 }
 
 /**
- * pm_suspend - Externally visible function for suspending the system.
+ * suspend - Suspend the system.
  * @state: System sleep state to enter.
+ * @lock:  Whether to acquire a system lock.
  *
  * Check if the value of @state represents one of the supported states,
- * execute enter_state() and update system suspend statistics.
+ * execute enter_state() or enter_state_locked() and update system suspend
+ * statistics.
  */
-int pm_suspend(suspend_state_t state)
+static int suspend(suspend_state_t state, int lock)
 {
 	int error;
 
 	if (state <= PM_SUSPEND_ON || state >= PM_SUSPEND_MAX)
 		return -EINVAL;
 
-	error = enter_state(state);
+	error = lock ? enter_state(state) : enter_state_locked(state);
 	if (error) {
 		suspend_stats.fail++;
 		dpm_save_failed_errno(error);
@@ -323,4 +342,30 @@ int pm_suspend(suspend_state_t state)
 	}
 	return error;
 }
+
+/**
+ * suspend_locked - Suspend the system when the system lock is already held.
+ * @state: System sleep state to enter.
+ *
+ * Check if the value of @state represents one of the supported states,
+ * execute enter_state_locked() and update system suspend statistics.
+ */
+#ifdef CONFIG_HIBERNATION
+int suspend_locked(suspend_state_t state)
+{
+	return suspend(state, 0);
+}
+#endif
+
+/**
+ * pm_suspend - Externally visible function for suspending the system.
+ * @state: System sleep state to enter.
+ *
+ * Check if the value of @state represents one of the supported states,
+ * execute enter_state() and update system suspend statistics.
+ */
+int pm_suspend(suspend_state_t state)
+{
+	return suspend(state, 1);
+}
 EXPORT_SYMBOL(pm_suspend);
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index eef311a..1027da8 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -1456,6 +1456,34 @@ void swsusp_close(fmode_t mode)
 	blkdev_put(hib_resume_bdev, mode);
 }
 
+/**
+ *      swsusp_unmark - Unmark swsusp signature in the resume device
+ */
+
+#ifdef CONFIG_SUSPEND
+int swsusp_unmark(void)
+{
+	int error;
+
+	hib_bio_read_page(swsusp_resume_block, swsusp_header, NULL);
+	if (!memcmp(HIBERNATE_SIG,swsusp_header->sig, 10)) {
+		memcpy(swsusp_header->sig,swsusp_header->orig_sig, 10);
+		error = hib_bio_write_page(swsusp_resume_block,
+					swsusp_header, NULL);
+	} else {
+		printk(KERN_ERR "PM: Cannot find swsusp signature!\n");
+		error = -ENODEV;
+	}
+
+	/*
+	 * We just returned from suspend, we don't need the image any more.
+	 */
+	free_all_swap_pages(root_swap);
+
+	return error;
+}
+#endif
+
 static int swsusp_header_init(void)
 {
 	swsusp_header = (struct swsusp_header*) __get_free_page(GFP_KERNEL);
-------------------

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Mon, 2012-05-14 at 21:11 +1000, Bojan Smojver wrote:
> Totally untested...

No. That hangs my box.

This triggers a bug in workqueues code (essentially the same as the
previous patch, except for sys_sync() not being done):
---------------
 Documentation/power/swsusp.txt |    5 +++
 kernel/power/hibernate.c       |   36 ++++++++++++++++++
 kernel/power/power.h           |    6 +++
 kernel/power/suspend.c         |   79 +++++++++++++++++++++++++++++++---------
 kernel/power/swap.c            |   28 ++++++++++++++
 5 files changed, 137 insertions(+), 17 deletions(-)

diff --git a/Documentation/power/swsusp.txt b/Documentation/power/swsusp.txt
index ac190cf..92341b8 100644
--- a/Documentation/power/swsusp.txt
+++ b/Documentation/power/swsusp.txt
@@ -33,6 +33,11 @@ echo shutdown > /sys/power/disk; echo disk > /sys/power/state
 
 echo platform > /sys/power/disk; echo disk > /sys/power/state
 
+. If you would like to write hibernation image to swap and then suspend
+to RAM (provided your platform supports it), you can try
+
+echo suspend > /sys/power/disk; echo disk > /sys/power/state
+
 . If you have SATA disks, you'll need recent kernels with SATA suspend
 support. For suspend and resume to work, make sure your disk drivers
 are built into kernel -- not modules. [There's way to make
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index e09dfbf..f48a9ba 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2003 Open Source Development Lab
  * Copyright (c) 2004 Pavel Machek <pavel@ucw.cz>
  * Copyright (c) 2009 Rafael J. Wysocki, Novell Inc.
+ * Copyright (C) 2012 Bojan Smojver <bojan@rexursive.com>
  *
  * This file is released under the GPLv2.
  */
@@ -44,6 +45,9 @@ enum {
 	HIBERNATION_PLATFORM,
 	HIBERNATION_SHUTDOWN,
 	HIBERNATION_REBOOT,
+#ifdef CONFIG_SUSPEND
+	HIBERNATION_SUSPEND,
+#endif
 	/* keep last */
 	__HIBERNATION_AFTER_LAST
 };
@@ -572,6 +576,10 @@ int hibernation_platform_enter(void)
  */
 static void power_down(void)
 {
+#ifdef CONFIG_SUSPEND
+	int error;
+#endif
+
 	switch (hibernation_mode) {
 	case HIBERNATION_REBOOT:
 		kernel_restart(NULL);
@@ -581,6 +589,25 @@ static void power_down(void)
 	case HIBERNATION_SHUTDOWN:
 		kernel_power_off();
 		break;
+#ifdef CONFIG_SUSPEND
+	case HIBERNATION_SUSPEND:
+		error = suspend_locked(PM_SUSPEND_MEM);
+		if (error) {
+			if (hibernation_ops)
+				hibernation_mode = HIBERNATION_PLATFORM;
+			else
+				hibernation_mode = HIBERNATION_SHUTDOWN;
+			power_down();
+		}
+		/*
+		 * Restore swap signature.
+		 */
+		error = swsusp_unmark();
+		if (error)
+			printk(KERN_ERR "PM: Swap will be unusable! "
+			                "Try swapon -a.\n");
+		return;
+#endif
 	}
 	kernel_halt();
 	/*
@@ -814,6 +841,9 @@ static const char * const hibernation_modes[] = {
 	[HIBERNATION_PLATFORM]	= "platform",
 	[HIBERNATION_SHUTDOWN]	= "shutdown",
 	[HIBERNATION_REBOOT]	= "reboot",
+#ifdef CONFIG_SUSPEND
+	[HIBERNATION_SUSPEND]	= "suspend",
+#endif
 };
 
 /*
@@ -854,6 +884,9 @@ static ssize_t disk_show(struct kobject *kobj, struct kobj_attribute *attr,
 		switch (i) {
 		case HIBERNATION_SHUTDOWN:
 		case HIBERNATION_REBOOT:
+#ifdef CONFIG_SUSPEND
+		case HIBERNATION_SUSPEND:
+#endif
 			break;
 		case HIBERNATION_PLATFORM:
 			if (hibernation_ops)
@@ -894,6 +927,9 @@ static ssize_t disk_store(struct kobject *kobj, struct kobj_attribute *attr,
 		switch (mode) {
 		case HIBERNATION_SHUTDOWN:
 		case HIBERNATION_REBOOT:
+#ifdef CONFIG_SUSPEND
+		case HIBERNATION_SUSPEND:
+#endif
 			hibernation_mode = mode;
 			break;
 		case HIBERNATION_PLATFORM:
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 98f3622..cb8e09e 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -156,6 +156,9 @@ extern void swsusp_free(void);
 extern int swsusp_read(unsigned int *flags_p);
 extern int swsusp_write(unsigned int flags);
 extern void swsusp_close(fmode_t);
+#ifdef CONFIG_SUSPEND
+extern int swsusp_unmark(void);
+#endif
 
 /* kernel/power/block_io.c */
 extern struct block_device *hib_resume_bdev;
@@ -177,6 +180,9 @@ extern const char *const pm_states[];
 
 extern bool valid_state(suspend_state_t state);
 extern int suspend_devices_and_enter(suspend_state_t state);
+#ifdef CONFIG_HIBERNATION
+extern int suspend_locked(suspend_state_t state);
+#endif
 #else /* !CONFIG_SUSPEND */
 static inline int suspend_devices_and_enter(suspend_state_t state)
 {
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 396d262..3402d6e 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -261,28 +261,19 @@ static void suspend_finish(void)
  * enter_state - Do common work needed to enter system sleep state.
  * @state: System sleep state to enter.
  *
- * Make sure that no one else is trying to put the system into a sleep state.
- * Fail if that's not the case.  Otherwise, prepare for system suspend, make the
- * system enter the given sleep state and clean up after wakeup.
+ * The system lock needs to be held already when this function is called.
  */
-static int enter_state(suspend_state_t state)
+static int enter_state_locked(suspend_state_t state)
 {
 	int error;
 
 	if (!valid_state(state))
 		return -ENODEV;
 
-	if (!mutex_trylock(&pm_mutex))
-		return -EBUSY;
-
-	printk(KERN_INFO "PM: Syncing filesystems ... ");
-	sys_sync();
-	printk("done.\n");
-
 	pr_debug("PM: Preparing system for %s sleep\n", pm_states[state]);
 	error = suspend_prepare();
 	if (error)
-		goto Unlock;
+		return error;
 
 	if (suspend_test(TEST_FREEZER))
 		goto Finish;
@@ -295,26 +286,54 @@ static int enter_state(suspend_state_t state)
  Finish:
 	pr_debug("PM: Finishing wakeup.\n");
 	suspend_finish();
- Unlock:
+	return error;
+}
+
+/**
+ * enter_state - Do common work needed to enter system sleep state.
+ * @state: System sleep state to enter.
+ *
+ * Make sure that no one else is trying to put the system into a sleep state.
+ * Fail if that's not the case.  Otherwise, prepare for system suspend, make the
+ * system enter the given sleep state and clean up after wakeup.
+ */
+static int enter_state(suspend_state_t state)
+{
+	int error;
+
+	if (!valid_state(state))
+		return -ENODEV;
+
+	if (!mutex_trylock(&pm_mutex))
+		return -EBUSY;
+
+	printk(KERN_INFO "PM: Syncing filesystems ... ");
+	sys_sync();
+	printk("done.\n");
+
+	error = enter_state_locked(state);
+
 	mutex_unlock(&pm_mutex);
 	return error;
 }
 
 /**
- * pm_suspend - Externally visible function for suspending the system.
+ * suspend - Suspend the system.
  * @state: System sleep state to enter.
+ * @lock:  Whether to acquire a system lock.
  *
  * Check if the value of @state represents one of the supported states,
- * execute enter_state() and update system suspend statistics.
+ * execute enter_state() or enter_state_locked() and update system suspend
+ * statistics.
  */
-int pm_suspend(suspend_state_t state)
+static int suspend(suspend_state_t state, int lock)
 {
 	int error;
 
 	if (state <= PM_SUSPEND_ON || state >= PM_SUSPEND_MAX)
 		return -EINVAL;
 
-	error = enter_state(state);
+	error = lock ? enter_state(state) : enter_state_locked(state);
 	if (error) {
 		suspend_stats.fail++;
 		dpm_save_failed_errno(error);
@@ -323,4 +342,30 @@ int pm_suspend(suspend_state_t state)
 	}
 	return error;
 }
+
+/**
+ * suspend_locked - Suspend the system when the system lock is already held.
+ * @state: System sleep state to enter.
+ *
+ * Check if the value of @state represents one of the supported states,
+ * execute enter_state_locked() and update system suspend statistics.
+ */
+#ifdef CONFIG_HIBERNATION
+int suspend_locked(suspend_state_t state)
+{
+	return suspend(state, 0);
+}
+#endif
+
+/**
+ * pm_suspend - Externally visible function for suspending the system.
+ * @state: System sleep state to enter.
+ *
+ * Check if the value of @state represents one of the supported states,
+ * execute enter_state() and update system suspend statistics.
+ */
+int pm_suspend(suspend_state_t state)
+{
+	return suspend(state, 1);
+}
 EXPORT_SYMBOL(pm_suspend);
diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index eef311a..1027da8 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -1456,6 +1456,34 @@ void swsusp_close(fmode_t mode)
 	blkdev_put(hib_resume_bdev, mode);
 }
 
+/**
+ *      swsusp_unmark - Unmark swsusp signature in the resume device
+ */
+
+#ifdef CONFIG_SUSPEND
+int swsusp_unmark(void)
+{
+	int error;
+
+	hib_bio_read_page(swsusp_resume_block, swsusp_header, NULL);
+	if (!memcmp(HIBERNATE_SIG,swsusp_header->sig, 10)) {
+		memcpy(swsusp_header->sig,swsusp_header->orig_sig, 10);
+		error = hib_bio_write_page(swsusp_resume_block,
+					swsusp_header, NULL);
+	} else {
+		printk(KERN_ERR "PM: Cannot find swsusp signature!\n");
+		error = -ENODEV;
+	}
+
+	/*
+	 * We just returned from suspend, we don't need the image any more.
+	 */
+	free_all_swap_pages(root_swap);
+
+	return error;
+}
+#endif
+
 static int swsusp_header_init(void)
 {
 	swsusp_header = (struct swsusp_header*) __get_free_page(GFP_KERNEL);
---------------

I think I'll need to break this down into even smaller steps.

PS. It seems obvious now why user space code simply did
suspend_devices_and_enter()... :-)

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Mon, 2012-05-14 at 21:47 +1000, Bojan Smojver wrote:
> No. That hangs my box.
> 
> This triggers a bug in workqueues code (essentially the same as the
> previous patch, except for sys_sync() not being done):

Alan/Srivatsa,

Coming back to the explanation of how this whole thing works, it would
seem that at the point of image writing all devices are fully functional
(not just some, as I mistakenly believed). However, the processes are
supposed to be already frozen, right? Calling suspend_prepare(), which
will essentially try to freeze the processes and kernel threads, seems
like the wrong thing to do.

Did you guys mean that we should be calling
pm_notifier_call_chain(PM_SUSPEND_PREPARE) only here?

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Alan Stern]

On Tue, 15 May 2012, Bojan Smojver wrote:

> Alan/Srivatsa,
> 
> Coming back to the explanation of how this whole thing works, it would
> seem that at the point of image writing all devices are fully functional
> (not just some, as I mistakenly believed). However, the processes are
> supposed to be already frozen, right? Calling suspend_prepare(), which
> will essentially try to freeze the processes and kernel threads, seems
> like the wrong thing to do.
> 
> Did you guys mean that we should be calling
> pm_notifier_call_chain(PM_SUSPEND_PREPARE) only here?

It looks like you've got this all figured out now.

Alan Stern

Re: [PATCH]: In kernel hibernation, suspend to both

[Srivatsa S. Bhat]

On 05/15/2012 05:29 AM, Bojan Smojver wrote:

> On Mon, 2012-05-14 at 21:47 +1000, Bojan Smojver wrote:
>> No. That hangs my box.
>>
>> This triggers a bug in workqueues code (essentially the same as the
>> previous patch, except for sys_sync() not being done):
> 
> Alan/Srivatsa,
> 
> Coming back to the explanation of how this whole thing works, it would
> seem that at the point of image writing all devices are fully functional
> (not just some, as I mistakenly believed). However, the processes are
> supposed to be already frozen, right? Calling suspend_prepare(), which
> will essentially try to freeze the processes and kernel threads, seems
> like the wrong thing to do.
> 
> Did you guys mean that we should be calling
> pm_notifier_call_chain(PM_SUSPEND_PREPARE) only here?
> 


Exactly! And also arrange for the corresponding PM_POST_SUSPEND notification
to happen at the end of suspend-to-ram stage...

Regards,
Srivatsa S. Bhat

Re: [PATCH]: In kernel hibernation, suspend to both

[Rafael J. Wysocki]

On Tuesday, May 15, 2012, Srivatsa S. Bhat wrote:
> On 05/15/2012 05:29 AM, Bojan Smojver wrote:
> 
> > On Mon, 2012-05-14 at 21:47 +1000, Bojan Smojver wrote:
> >> No. That hangs my box.
> >>
> >> This triggers a bug in workqueues code (essentially the same as the
> >> previous patch, except for sys_sync() not being done):
> > 
> > Alan/Srivatsa,
> > 
> > Coming back to the explanation of how this whole thing works, it would
> > seem that at the point of image writing all devices are fully functional
> > (not just some, as I mistakenly believed). However, the processes are
> > supposed to be already frozen, right? Calling suspend_prepare(), which
> > will essentially try to freeze the processes and kernel threads, seems
> > like the wrong thing to do.
> > 
> > Did you guys mean that we should be calling
> > pm_notifier_call_chain(PM_SUSPEND_PREPARE) only here?
> > 
> 
> 
> Exactly! And also arrange for the corresponding PM_POST_SUSPEND notification
> to happen at the end of suspend-to-ram stage...

Actually, no.  The notifiers are supposed to be called when user space is
available, otherwise some things will break badly (firmware loading for
one example IIRC).

So, I think we should pretend that this is all hibernation and, even if
the suspend to RAM phase succeeds, run PM_POST_HIBERNATION notifiers (which
I suppose is what the first Bojan's patch did, right?).

Thanks,
Rafael

Re: [PATCH]: In kernel hibernation, suspend to both

[Srivatsa S. Bhat]

On 05/15/2012 11:12 PM, Rafael J. Wysocki wrote:

> On Tuesday, May 15, 2012, Srivatsa S. Bhat wrote:
>> On 05/15/2012 05:29 AM, Bojan Smojver wrote:
>>
>>> On Mon, 2012-05-14 at 21:47 +1000, Bojan Smojver wrote:
>>>> No. That hangs my box.
>>>>
>>>> This triggers a bug in workqueues code (essentially the same as the
>>>> previous patch, except for sys_sync() not being done):
>>>
>>> Alan/Srivatsa,
>>>
>>> Coming back to the explanation of how this whole thing works, it would
>>> seem that at the point of image writing all devices are fully functional
>>> (not just some, as I mistakenly believed). However, the processes are
>>> supposed to be already frozen, right? Calling suspend_prepare(), which
>>> will essentially try to freeze the processes and kernel threads, seems
>>> like the wrong thing to do.
>>>
>>> Did you guys mean that we should be calling
>>> pm_notifier_call_chain(PM_SUSPEND_PREPARE) only here?
>>>
>>
>>
>> Exactly! And also arrange for the corresponding PM_POST_SUSPEND notification
>> to happen at the end of suspend-to-ram stage...
> 
> Actually, no.  The notifiers are supposed to be called when user space is
> available, otherwise some things will break badly (firmware loading for
> one example IIRC).
> 


Oh, you are right. The notifiers are called before freezing userspace.
Sorry, I had overlooked that.
 
Regards,
Srivatsa S. Bhat

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Tue, 2012-05-15 at 19:42 +0200, Rafael J. Wysocki wrote:
> which I suppose is what the first Bojan's patch did, right? 

Correct. No extra notifiers were sent, so whatever hibernation code did
was it.

That patch is here:

http://marc.info/?l=linux-kernel&m=133651588229850&w=2

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Wed, 2012-05-16 at 08:23 +1000, Bojan Smojver wrote:
> On Tue, 2012-05-15 at 19:42 +0200, Rafael J. Wysocki wrote:
> > which I suppose is what the first Bojan's patch did, right? 
> 
> Correct. No extra notifiers were sent, so whatever hibernation code did
> was it.
> 
> That patch is here:
> 
> http://marc.info/?l=linux-kernel&m=133651588229850&w=2

Now that 3.4 is out, just a ping to have this in the 3.5 queue.

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Borislav Petkov]

On Mon, May 21, 2012 at 02:38:27PM +1000, Bojan Smojver wrote:
> > http://marc.info/?l=linux-kernel&m=133651588229850&w=2
> 
> Now that 3.4 is out, just a ping to have this in the 3.5 queue.

Is this the final thing you guys agree on which should be tested more?

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH]: In kernel hibernation, suspend to both

[Rafael J. Wysocki]

On Monday, May 21, 2012, Bojan Smojver wrote:
> On Wed, 2012-05-16 at 08:23 +1000, Bojan Smojver wrote:
> > On Tue, 2012-05-15 at 19:42 +0200, Rafael J. Wysocki wrote:
> > > which I suppose is what the first Bojan's patch did, right? 
> > 
> > Correct. No extra notifiers were sent, so whatever hibernation code did
> > was it.
> > 
> > That patch is here:
> > 
> > http://marc.info/?l=linux-kernel&m=133651588229850&w=2
> 
> Now that 3.4 is out, just a ping to have this in the 3.5 queue.

No, I'm not going to put that into 3.5, sorry.  It can go into 3.6.

Thanks,
Rafael

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

>No, I'm not going to put that into 3.5, sorry.  It can go into 3.6.

May I ask why? I do not see why people should wait another few months to get this.

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Rafael J. Wysocki]

On Monday, May 21, 2012, Bojan Smojver wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> >No, I'm not going to put that into 3.5, sorry.  It can go into 3.6.
> 
> May I ask why? I do not see why people should wait another few months to get this.

We haven't had it for the last few years, so a couple of months won't make
a big difference I think.

The reason is that I want it to get more testing and the merge window has
already opened, so it's not a good time for adding new features to linux-next.
It's not urgent and it _can_ wait.

Thanks,
Rafael

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Mon, 2012-05-21 at 23:53 +0200, Rafael J. Wysocki wrote:
> It's not urgent and it _can_ wait.

OK. I'll go with "better late than never", in that case. :-)

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Borislav Petkov]

On Tue, May 22, 2012 at 07:55:59AM +1000, Bojan Smojver wrote:
> On Mon, 2012-05-21 at 23:53 +0200, Rafael J. Wysocki wrote:
> > It's not urgent and it _can_ wait.
> 
> OK. I'll go with "better late than never", in that case. :-)

Let me ask again, can you send the last version of that patch so that I
can give it a run pls?

Thanks.

-- 
Regards/Gruss,
Boris.

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

Borislav Petkov <bp@alien8.de> wrote:

>Let me ask again, can you send the last version of that patch so that I
>can give it a run pls?


Sorry. That patch is here (pretty much the original with signed-off):

http://marc.info/?l=linux-kernel&m=133651588229850&w=2

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Borislav Petkov]

On Fri, May 25, 2012 at 12:02:15PM +1000, Bojan Smojver wrote:
> Sorry. That patch is here (pretty much the original with signed-off):
> 
> http://marc.info/?l=linux-kernel&m=133651588229850&w=2

Ok, I finally gave the above a run, here's the dmesg after a successful
suspend-resume cycle. It looks ok except the funny characters after

"[  380.938864] PM: Compressing and saving image data (75689 pages) ..."

I'll keep running it the next couple of days/weeks and report any
instabilities.

[  372.519343] PM: Syncing filesystems ... done.
[  372.534190] Freezing user space processes ... (elapsed 0.01 seconds) done.
[  372.790881] PM: Preallocating image memory... done (allocated 71296 pages)
[  372.790978] PM: Allocated 285184 kbytes in 0.25 seconds (1140.73 MB/s)
[  372.886000] Freezing remaining freezable tasks ... (elapsed 0.09 seconds) done.
[  372.887203] Suspending console(s) (use no_console_suspend to debug)
[  372.889014] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[  372.994022] ACPI handle has no context!
[  373.654092] PM: freeze of devices complete after 767.177 msecs
[  373.654582] PM: late freeze of devices complete after 0.483 msecs
[  373.655572] PM: noirq freeze of devices complete after 0.987 msecs
[  373.655576] Disabling non-boot CPUs ...
[  373.658524] CPU 1 is now offline
[  373.658529] lockdep: fixing up alternatives.
[  373.659848] PM: Creating hibernation image:
[  373.929896] PM: Need to copy 75541 pages
[  374.779946] PM: Hibernation image created (75541 pages copied)
[  373.660536] Enabling non-boot CPUs ...
[  373.669476] lockdep: fixing up alternatives.
[  373.669489] Booting Node 0 Processor 1 APIC 0x1
[  373.680521] LVT offset 0 assigned for vector 0x400
[  373.684157] CPU1 is up
[  373.685209] PM: noirq thaw of devices complete after 0.670 msecs
[  373.685838] PM: early thaw of devices complete after 0.515 msecs
[  373.701440] [drm] PCIE GART of 512M enabled (table at 0x0000000000040000).
[  373.701756] radeon 0000:00:01.0: WB enabled
[  373.701762] radeon 0000:00:01.0: fence driver on ring 0 use gpu addr 0x0000000018000c00 and cpu addr 0xffff880118850c00
[  373.701985] snd_hda_intel 0000:00:01.1: irq 43 for MSI/MSI-X
[  373.714886] atl1c 0000:02:00.0: irq 44 for MSI/MSI-X
[  373.718033] [drm] ring test on 0 succeeded in 1 usecs
[  373.718083] [drm] ib test on ring 0 succeeded in 0 usecs
[  374.177875] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[  374.185096] ata1.00: configured for UDMA/133
[  374.202031] sd 0:0:0:0: [sda] Starting disk
[  375.154008] PM: thaw of devices complete after 1468.985 msecs
[  380.938864] PM: Using 1 thread(s) for compression.
[  380.938864] PM: Compressing and saving image data (75689 pages) ...     \x08\x08\x08\x08  0%\x08\x08\x08\x08  1%\x08\x08\x08\x08  2%\x08\x08\x08\x08  3%\x08\x08\x08\x08  4%\x08\x08\x08\x08  5%\x08\x08\x08\x08  6%\x08\x08\x08\x08  7%\x08\x08\x08\x08  8%\x08\x08\x08\x08  9%\x08\x08\x08\x08 10%\x08\x08\x08\x08 11%\x08\x08\x08\x08 12%\x08\x08\x08\x08 13%\x08\x08\x08\x08 14%\x08\x08\x08\x08 15%\x08\x08\x08\x08 16%\x08\x08\x08\x08 17%\x08\x08\x08\x08 18%\x08\x08\x08\x08 19%\x08\x08\x08\x08 20%\x08\x08\x08\x08 21%\x08\x08\x08\x08 22%\x08\x08\x08\x08 23%\x08\x08\x08\x08 24%\x08\x08\x08\x08 25%\x08\x08\x08\x08 26%\x08\x08\x08\x08 27%\x08\x08\x08\x08 28%\x08\x08\x08\x08 29%\x08\x08\x08\x08 30%\x08\x08\x08\x08 31%\x08\x08\x08\x08 32%\x08\x08\x08\x08 33%\x08\x08\x08\x08 34%\x08\x08\x08\x08 35%\x08\x08\x08\x08 36%\x08\x08\x08\x08 37%\x08\x08\x08\x08 38%\x08\x08\x08\x08 39%\x08\x08\x08\x08 40%\x08\x08\x08\x08 41%\x08\x08\x08\x08 42%\x08\x08\x08\x08 43%\x08\x08\x08\x08 44%\x08\x08\x08\x08 45%\x08\x08\x08\x08 46%\x08\x08\x08\x08 47%\x08\x08\x08\x08 48%\x08\x08\x08\x08 49%\x08\x08\x08\x08 50%\x08\x08\x08\x08 51%\x08\x08\x08\x08 52%\x08\x08\x08\x08 53%\x08\x08\x08\x08 54%\x08\x08\x08\x08 55%\x08\x08\x08\x08 56%\x08\x08\x08\x08 57%\x08\x08\x08\x08 58%\x08\x08\x08\x08 59%\x08\x08\x08\x08 60%\x08\x08\x08\x08 61%\x08\x08\x08\x08 62%\x08\x08\x08\x08 63%\x08\x08\x08\x08 64%\x08\x08\x08\x08 65%\x08\x08\x08\x08 66%\x08\x08\x08\x08 67%\x08\x08\x08\x08 68%\x08\x08\x08\x08 69%\x08\x08\x08\x08 70%\x08\x08\x08\x08 71%\x08\x08\x08\x08 72%\x08\x08\x08\x08 73%\x08\x08\x08\x08 74%\x08\x08\x08\x08 75%\x08\x08\x08\x08 76%\x08\x08\x08\x08 77%\x08\x08\x08\x08 78%\x08\x08\x08\x08 79%\x08\x08\x08\x08 80%\x08\x08\x08\x08 81%\x08\x08\x08\x08 82%\x08\x08\x08\x08 83%\x08\x08\x08\x08 84%\x08\x08\x08\x08 85%\x08\x08\x08\x08 86%\x08\x08\x08\x08 87%\x08\x08\x08\x08 88%\x08\x08\x08\x08 89%\x08\x08\x08\x08 90%\x08\x08\x08\x08 91%\x08\x08\x08\x08 92%\x08\x08\x08\x08 93%\x08\x08\x08\x08 94%\x08\x08\x08\x08 95%\x08\x08\x08\x08 96%\x08\x08\x08\x08 97%\x08\x08\x08\x08 98%\x08\x08\x08\x08 99%\x08\x08\x08\x08100%\x08\x08\x08\x08done
[  380.939433] PM: Wrote 302756 kbytes in 5.78 seconds (52.37 MB/s)
[  380.960456] PM: S|
[  381.025137] Suspending console(s) (use no_console_suspend to debug)
[  381.175895] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[  381.176891] sd 0:0:0:0: [sda] Stopping disk
[  381.369937] ACPI handle has no context!
[  381.749872] PM: suspend of devices complete after 724.410 msecs
[  381.750731] PM: late suspend of devices complete after 0.849 msecs
[  381.781749] ohci_hcd 0000:00:13.0: wake-up capability enabled by ACPI
[  381.781940] ehci_hcd 0000:00:12.2: wake-up capability enabled by ACPI
[  381.797738] ohci_hcd 0000:00:12.0: wake-up capability enabled by ACPI
[  381.798159] PM: noirq suspend of devices complete after 47.444 msecs
[  381.798225] ACPI: Preparing to enter system sleep state S3
[  381.849919] PM: Saving platform NVS memory
[  381.857244] Disabling non-boot CPUs ...
[  381.961329] CPU 1 is now offline
[  381.961335] lockdep: fixing up alternatives.
[  381.962531] ACPI: Low-level resume complete
[  381.962633] PM: Restoring platform NVS memory
[  381.963384] Enabling non-boot CPUs ...
[  381.972265] lockdep: fixing up alternatives.
[  381.972273] Booting Node 0 Processor 1 APIC 0x1
[  381.983298] LVT offset 0 assigned for vector 0x400
[  381.986425] CPU1 is up
[  381.987320] ACPI: Waking up from system sleep state S3
[  382.155684] ohci_hcd 0000:00:12.0: wake-up capability disabled by ACPI
[  382.170656] ehci_hcd 0000:00:12.2: wake-up capability disabled by ACPI
[  382.170844] ohci_hcd 0000:00:13.0: wake-up capability disabled by ACPI
[  382.187755] PM: noirq resume of devices complete after 32.977 msecs
[  382.188346] PM: early resume of devices complete after 0.476 msecs
[  382.189442] snd_hda_intel 0000:00:01.1: irq 43 for MSI/MSI-X
[  382.211320] [drm] PCIE GART of 512M enabled (table at 0x0000000000040000).
[  382.211700] radeon 0000:00:01.0: WB enabled
[  382.211706] radeon 0000:00:01.0: fence driver on ring 0 use gpu addr 0x0000000018000c00 and cpu addr 0xffff880118850c00
[  382.224190] atl1c 0000:02:00.0: irq 44 for MSI/MSI-X
[  382.228088] [drm] ring test on 0 succeeded in 1 usecs
[  382.228145] [drm] ib test on ring 0 succeeded in 0 usecs
[  384.361199] ata1: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
[  384.527745] ata1.00: configured for UDMA/133
[  384.541619] sd 0:0:0:0: [sda] Starting disk
[  384.587970] PM: resume of devices complete after 2400.966 msecs
[  384.672413] Restarting tasks ... done.
[  384.720985] video LNXVIDEO:01: Restoring backlight state
[  432.813112] atl1c 0000:02:00.0: atl1c: eth0 NIC Link is Up<100 Mbps Full Duplex>

-- 
Regards/Gruss,
Boris.

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

Borislav Petkov <bp@alien8.de> wrote:

>On Fri, May 25, 2012 at 12:02:15PM +1000, Bojan Smojver wrote:
>> Sorry. That patch is here (pretty much the original with signed-off):
>> 
>> http://marc.info/?l=linux-kernel&m=133651588229850&w=2
>
>Ok, I finally gave the above a run, here's the dmesg after a successful
>suspend-resume cycle. It looks ok except the funny characters after
>
>"[  380.938864] PM: Compressing and saving image data (75689 pages)
>..."
>
>I'll keep running it the next couple of days/weeks and report any
>instabilities.


Thanks for testing. The funny characters are backspaces printed by the progress indicator. :-)

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Borislav Petkov]

On Fri, Jun 01, 2012 at 12:03:47PM +1000, Bojan Smojver wrote:
> >Ok, I finally gave the above a run, here's the dmesg after a successful
> >suspend-resume cycle. It looks ok except the funny characters after
> >
> >"[  380.938864] PM: Compressing and saving image data (75689 pages)
> >..."
> >
> >I'll keep running it the next couple of days/weeks and report any
> >instabilities.

> Thanks for testing. The funny characters are backspaces printed by the
> progress indicator. :-)

I know, this output should probably be turned off if we're both
suspending to disk and ram so as not to pollute dmesg. Normal,
suspend-to-disk cycle doesn't put that output in dmesg.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Fri, 2012-06-01 at 10:48 +0200, Borislav Petkov wrote:
> Normal, suspend-to-disk cycle doesn't put that output in dmesg.

It does, but that kernel state never gets used again in that case. The
state before the image writing starts is the one used on thaw, so these
messages never appear anywhere.

But yeah, I do not see why these should ever go to any dmesg output.
They should just go to the screen.

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Borislav Petkov]

On Fri, Jun 01, 2012 at 06:57:20PM +1000, Bojan Smojver wrote:
> On Fri, 2012-06-01 at 10:48 +0200, Borislav Petkov wrote:
> > Normal, suspend-to-disk cycle doesn't put that output in dmesg.
> 
> It does, but that kernel state never gets used again in that case. The
> state before the image writing starts is the one used on thaw, so these
> messages never appear anywhere.

Ok, thanks for clarifying, I always wondered how that worked but was too
lazy to look :-).

> But yeah, I do not see why these should ever go to any dmesg output.
> They should just go to the screen.

Yep.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH]: In kernel hibernation, suspend to both

[Rafael J. Wysocki]

On Friday, May 25, 2012, Bojan Smojver wrote:
> Borislav Petkov <bp@alien8.de> wrote:
> 
> >Let me ask again, can you send the last version of that patch so that I
> >can give it a run pls?
> 
> 
> Sorry. That patch is here (pretty much the original with signed-off):
> 
> http://marc.info/?l=linux-kernel&m=133651588229850&w=2

Queued up for 3.6 in linux-pm/linux-next.

Thanks,
Rafael

Re: [PATCH]: In kernel hibernation, suspend to both

[Borislav Petkov]

On Sat, Jun 16, 2012 at 03:59:30PM +0200, Rafael J. Wysocki wrote:
> On Friday, May 25, 2012, Bojan Smojver wrote:
> > Borislav Petkov <bp@alien8.de> wrote:
> > 
> > >Let me ask again, can you send the last version of that patch so that I
> > >can give it a run pls?
> > 
> > 
> > Sorry. That patch is here (pretty much the original with signed-off):
> > 
> > http://marc.info/?l=linux-kernel&m=133651588229850&w=2
> 
> Queued up for 3.6 in linux-pm/linux-next.

IMHO, this one still needs a fix for the progress indicator output
landing in dmesg and filling it up with \x08 chars:

http://marc.info/?l=linux-pm&m=133848160403298&w=2

-- 
Regards/Gruss,
    Boris.

Re: [PATCH]: In kernel hibernation, suspend to both

[Rafael J. Wysocki]

On Saturday, June 16, 2012, Borislav Petkov wrote:
> On Sat, Jun 16, 2012 at 03:59:30PM +0200, Rafael J. Wysocki wrote:
> > On Friday, May 25, 2012, Bojan Smojver wrote:
> > > Borislav Petkov <bp@alien8.de> wrote:
> > > 
> > > >Let me ask again, can you send the last version of that patch so that I
> > > >can give it a run pls?
> > > 
> > > 
> > > Sorry. That patch is here (pretty much the original with signed-off):
> > > 
> > > http://marc.info/?l=linux-kernel&m=133651588229850&w=2
> > 
> > Queued up for 3.6 in linux-pm/linux-next.
> 
> IMHO, this one still needs a fix for the progress indicator output
> landing in dmesg and filling it up with \x08 chars:
> 
> http://marc.info/?l=linux-pm&m=133848160403298&w=2

Hmm.  Is there any fix, though?

Rafael

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

>Hmm.  Is there any fix, though?

How do we tell printk() to not dump stuff into dmesg? Or, how do we otherwise just print to the console?

The rest should be trivial...

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Alan Stern]

On Sun, 17 Jun 2012, Bojan Smojver wrote:

> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> >Hmm.  Is there any fix, though?
> 
> How do we tell printk() to not dump stuff into dmesg?

You can't; that's what printk() does, by definition.  If you don't want 
stuff to go into the dmesg buffer then don't call printk().

>  Or, how do we
> otherwise just print to the console?

That's a better question.  Unfortunately, I don't know the answer.

Alan Stern

Re: [PATCH]: In kernel hibernation, suspend to both

[Borislav Petkov]

On Sat, Jun 16, 2012 at 04:19:18PM -0400, Alan Stern wrote:
> On Sun, 17 Jun 2012, Bojan Smojver wrote:
> 
> > "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > 
> > >Hmm.  Is there any fix, though?
> > 
> > How do we tell printk() to not dump stuff into dmesg?
> 
> You can't; that's what printk() does, by definition.  If you don't want 
> stuff to go into the dmesg buffer then don't call printk().
> 
> >  Or, how do we
> > otherwise just print to the console?
> 
> That's a better question.  Unfortunately, I don't know the answer.

Ok, I don't know the hibernation code at all but let me give it a try:

how about teach the piece that parses

$ echo suspend > /sys/power/disk

to make a note to self that we're doing the hibrid hibernation method
and then the function that prints the progress indicator to be let known
of the aforementioned self-note so that it doesn't do any output?

Purely hypothetical, of course.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

Borislav Petkov <bp@alien8.de> wrote:

>Ok, I don't know the hibernation code at all but let me give it a try:
>
>how about teach the piece that parses
>
>$ echo suspend > /sys/power/disk
>
>to make a note to self that we're doing the hibrid hibernation method
>and then the function that prints the progress indicator to be let
>known
>of the aforementioned self-note so that it doesn't do any output?
>
>Purely hypothetical, of course.


Yeah, we already have a variable for that. But, we can just do it simpler - paint one dot every 10% or something, without any backspaces. That will fix it for sure.

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Borislav Petkov]

On Sun, Jun 17, 2012 at 01:21:29PM +1000, Bojan Smojver wrote:
> Borislav Petkov <bp@alien8.de> wrote:
> 
> >Ok, I don't know the hibernation code at all but let me give it a try:
> >
> >how about teach the piece that parses
> >
> >$ echo suspend > /sys/power/disk
> >
> >to make a note to self that we're doing the hibrid hibernation method
> >and then the function that prints the progress indicator to be let
> >known
> >of the aforementioned self-note so that it doesn't do any output?
> >
> >Purely hypothetical, of course.
>
> Yeah, we already have a variable for that. But, we can just do it
> simpler - paint one dot every 10% or something, without any backspaces.
> That will fix it for sure.

Right, this could work too. Whatever you guys decide, I'm here to test
patches :)

Thanks.

-- 
Regards/Gruss,
    Boris.

Re: [PATCH]: In kernel hibernation, suspend to both

[Rafael J. Wysocki]

On Sunday, June 17, 2012, Bojan Smojver wrote:
> Borislav Petkov <bp@alien8.de> wrote:
> 
> >Ok, I don't know the hibernation code at all but let me give it a try:
> >
> >how about teach the piece that parses
> >
> >$ echo suspend > /sys/power/disk
> >
> >to make a note to self that we're doing the hibrid hibernation method
> >and then the function that prints the progress indicator to be let
> >known
> >of the aforementioned self-note so that it doesn't do any output?
> >
> >Purely hypothetical, of course.
> 
> 
> Yeah, we already have a variable for that. But, we can just do it
> simpler - paint one dot every 10% or something, without any backspaces.
> That will fix it for sure.

Yeah, that will do.

Thanks,
Rafael

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Sun, 2012-06-17 at 22:21 +0200, Rafael J. Wysocki wrote:
> Yeah, that will do.

Something like this maybe. I didn't really test this, so Borislav,
please let me know whether this is something that may works for you.

-------------------------
 kernel/power/swap.c |   33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 11e22c0..c24d354 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -448,9 +448,9 @@ static int save_image(struct swap_map_handle *handle,
 	struct timeval start;
 	struct timeval stop;
 
-	printk(KERN_INFO "PM: Saving image data pages (%u pages) ...     ",
+	printk(KERN_INFO "PM: Saving image data pages (%u pages) ",
 		nr_to_write);
-	m = nr_to_write / 100;
+	m = nr_to_write / 10;
 	if (!m)
 		m = 1;
 	nr_pages = 0;
@@ -464,7 +464,7 @@ static int save_image(struct swap_map_handle *handle,
 		if (ret)
 			break;
 		if (!(nr_pages % m))
-			printk(KERN_CONT "\b\b\b\b%3d%%", nr_pages / m);
+			printk(KERN_CONT ".");
 		nr_pages++;
 	}
 	err2 = hib_wait_on_bio_chain(&bio);
@@ -472,7 +472,7 @@ static int save_image(struct swap_map_handle *handle,
 	if (!ret)
 		ret = err2;
 	if (!ret)
-		printk(KERN_CONT "\b\b\b\bdone\n");
+		printk(KERN_CONT " done.\n");
 	else
 		printk(KERN_CONT "\n");
 	swsusp_show_speed(&start, &stop, nr_to_write, "Wrote");
@@ -668,9 +668,9 @@ static int save_image_lzo(struct swap_map_handle *handle,
 
 	printk(KERN_INFO
 		"PM: Using %u thread(s) for compression.\n"
-		"PM: Compressing and saving image data (%u pages) ...     ",
+		"PM: Compressing and saving image data (%u pages) ",
 		nr_threads, nr_to_write);
-	m = nr_to_write / 100;
+	m = nr_to_write / 10;
 	if (!m)
 		m = 1;
 	nr_pages = 0;
@@ -690,8 +690,7 @@ static int save_image_lzo(struct swap_map_handle *handle,
 				       data_of(*snapshot), PAGE_SIZE);
 
 				if (!(nr_pages % m))
-					printk(KERN_CONT "\b\b\b\b%3d%%",
-				               nr_pages / m);
+					printk(KERN_CONT ".");
 				nr_pages++;
 			}
 			if (!off)
@@ -762,7 +761,7 @@ out_finish:
 	if (!ret)
 		ret = err2;
 	if (!ret) {
-		printk(KERN_CONT "\b\b\b\bdone\n");
+		printk(KERN_CONT " done.\n");
 	} else {
 		printk(KERN_CONT "\n");
 	}
@@ -973,9 +972,9 @@ static int load_image(struct swap_map_handle *handle,
 	int err2;
 	unsigned nr_pages;
 
-	printk(KERN_INFO "PM: Loading image data pages (%u pages) ...     ",
+	printk(KERN_INFO "PM: Loading image data pages (%u pages) ",
 		nr_to_read);
-	m = nr_to_read / 100;
+	m = nr_to_read / 10;
 	if (!m)
 		m = 1;
 	nr_pages = 0;
@@ -993,7 +992,7 @@ static int load_image(struct swap_map_handle *handle,
 		if (ret)
 			break;
 		if (!(nr_pages % m))
-			printk("\b\b\b\b%3d%%", nr_pages / m);
+			printk(".");
 		nr_pages++;
 	}
 	err2 = hib_wait_on_bio_chain(&bio);
@@ -1001,7 +1000,7 @@ static int load_image(struct swap_map_handle *handle,
 	if (!ret)
 		ret = err2;
 	if (!ret) {
-		printk("\b\b\b\bdone\n");
+		printk(" done.\n");
 		snapshot_write_finalize(snapshot);
 		if (!snapshot_image_loaded(snapshot))
 			ret = -ENODATA;
@@ -1185,9 +1184,9 @@ static int load_image_lzo(struct swap_map_handle *handle,
 
 	printk(KERN_INFO
 		"PM: Using %u thread(s) for decompression.\n"
-		"PM: Loading and decompressing image data (%u pages) ...     ",
+		"PM: Loading and decompressing image data (%u pages) ",
 		nr_threads, nr_to_read);
-	m = nr_to_read / 100;
+	m = nr_to_read / 10;
 	if (!m)
 		m = 1;
 	nr_pages = 0;
@@ -1319,7 +1318,7 @@ static int load_image_lzo(struct swap_map_handle *handle,
 				       data[thr].unc + off, PAGE_SIZE);
 
 				if (!(nr_pages % m))
-					printk("\b\b\b\b%3d%%", nr_pages / m);
+					printk(".");
 				nr_pages++;
 
 				ret = snapshot_write_next(snapshot);
@@ -1344,7 +1343,7 @@ out_finish:
 	}
 	do_gettimeofday(&stop);
 	if (!ret) {
-		printk("\b\b\b\bdone\n");
+		printk(" done.\n");
 		snapshot_write_finalize(snapshot);
 		if (!snapshot_image_loaded(snapshot))
 			ret = -ENODATA;
-------------------------

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Borislav Petkov]

On Mon, Jun 18, 2012 at 10:33:06AM +1000, Bojan Smojver wrote:
> On Sun, 2012-06-17 at 22:21 +0200, Rafael J. Wysocki wrote:
> > Yeah, that will do.
> 
> Something like this maybe. I didn't really test this, so Borislav,
> please let me know whether this is something that may works for you.

Yeah, this could probably work but it changes the progress indicator
unconditionally.

I don't know what the general consensus is about the progress indicator
but if you want to keep the old behavior too, you probably need to carve
out the printk functionality from save_image() and pass flags to the new
function which printks different output depending on how it is.

If we don't want the current output with how many pages it needs to use
on swap, then we can do the dots.

Frankly, I don't have a preference.

Hmm.

-- 
Regards/Gruss,
Boris.

Re: [PATCH]: In kernel hibernation, suspend to both

[Rafael J. Wysocki]

On Monday, June 18, 2012, Borislav Petkov wrote:
> On Mon, Jun 18, 2012 at 10:33:06AM +1000, Bojan Smojver wrote:
> > On Sun, 2012-06-17 at 22:21 +0200, Rafael J. Wysocki wrote:
> > > Yeah, that will do.
> > 
> > Something like this maybe. I didn't really test this, so Borislav,
> > please let me know whether this is something that may works for you.
> 
> Yeah, this could probably work but it changes the progress indicator
> unconditionally.
> 
> I don't know what the general consensus is about the progress indicator
> but if you want to keep the old behavior too, you probably need to carve
> out the printk functionality from save_image() and pass flags to the new
> function which printks different output depending on how it is.
> 
> If we don't want the current output with how many pages it needs to use
> on swap, then we can do the dots.
> 
> Frankly, I don't have a preference.

I'm not sure if the current behavior is worth preserving.  It's a bit hackish
and doesn't work with dmesg buffer, as you have noticed.

So, I think printing a status line every 10% would be fine.

Thanks,
Rafael

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

>So, I think printing a status line every 10% would be fine.

My patch prints a dot every 10%, which is not on a separate line. Did you want me print a full line instead or are you saying dots are fine?

PS. Yeah, the current printout with \b is a hack and I do not particularly like it either. Proper way would be with plymouth or something like that, but I got no joy with that on that list. One day, maybe...

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Rafael J. Wysocki]

On Monday, June 18, 2012, Bojan Smojver wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> >So, I think printing a status line every 10% would be fine.
> 
> My patch prints a dot every 10%, which is not on a separate line.
> Did you want me print a full line instead or are you saying dots are fine?

I think I'd prefer a full status line every 10%.  Or even maybe every 20%.

> PS. Yeah, the current printout with \b is a hack and I do not particularly
> like it either. Proper way would be with plymouth or something like that, but
> I got no joy with that on that list. One day, maybe...

I'm not sure what you mean?

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Mon, 2012-06-18 at 23:28 +0200, Rafael J. Wysocki wrote:
> I think I'd prefer a full status line every 10%.  Or even maybe every
> 20%.

Here is one that does a line every 10%:
----------------
 kernel/power/swap.c |   51 ++++++++++++++++++++++++---------------------------
 1 file changed, 24 insertions(+), 27 deletions(-)

diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index 11e22c0..314fd75 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -448,9 +448,9 @@ static int save_image(struct swap_map_handle *handle,
 	struct timeval start;
 	struct timeval stop;
 
-	printk(KERN_INFO "PM: Saving image data pages (%u pages) ...     ",
+	printk(KERN_INFO "PM: Saving image data pages (%u pages)...\n",
 		nr_to_write);
-	m = nr_to_write / 100;
+	m = nr_to_write / 10;
 	if (!m)
 		m = 1;
 	nr_pages = 0;
@@ -464,7 +464,8 @@ static int save_image(struct swap_map_handle *handle,
 		if (ret)
 			break;
 		if (!(nr_pages % m))
-			printk(KERN_CONT "\b\b\b\b%3d%%", nr_pages / m);
+			printk(KERN_INFO "PM: Saved %3d%%...\n",
+			       nr_pages / m * 10);
 		nr_pages++;
 	}
 	err2 = hib_wait_on_bio_chain(&bio);
@@ -472,9 +473,7 @@ static int save_image(struct swap_map_handle *handle,
 	if (!ret)
 		ret = err2;
 	if (!ret)
-		printk(KERN_CONT "\b\b\b\bdone\n");
-	else
-		printk(KERN_CONT "\n");
+		printk(KERN_INFO "PM: Saving image done.\n");
 	swsusp_show_speed(&start, &stop, nr_to_write, "Wrote");
 	return ret;
 }
@@ -668,9 +667,9 @@ static int save_image_lzo(struct swap_map_handle *handle,
 
 	printk(KERN_INFO
 		"PM: Using %u thread(s) for compression.\n"
-		"PM: Compressing and saving image data (%u pages) ...     ",
+		"PM: Compressing and saving image data (%u pages)...\n",
 		nr_threads, nr_to_write);
-	m = nr_to_write / 100;
+	m = nr_to_write / 10;
 	if (!m)
 		m = 1;
 	nr_pages = 0;
@@ -690,8 +689,8 @@ static int save_image_lzo(struct swap_map_handle *handle,
 				       data_of(*snapshot), PAGE_SIZE);
 
 				if (!(nr_pages % m))
-					printk(KERN_CONT "\b\b\b\b%3d%%",
-				               nr_pages / m);
+					printk(KERN_INFO "PM: Saved %3d%%...\n",
+				               nr_pages / m * 10);
 				nr_pages++;
 			}
 			if (!off)
@@ -761,11 +760,8 @@ out_finish:
 	do_gettimeofday(&stop);
 	if (!ret)
 		ret = err2;
-	if (!ret) {
-		printk(KERN_CONT "\b\b\b\bdone\n");
-	} else {
-		printk(KERN_CONT "\n");
-	}
+	if (!ret)
+		printk(KERN_INFO "PM: Saving image done.\n");
 	swsusp_show_speed(&start, &stop, nr_to_write, "Wrote");
 out_clean:
 	if (crc) {
@@ -973,9 +969,9 @@ static int load_image(struct swap_map_handle *handle,
 	int err2;
 	unsigned nr_pages;
 
-	printk(KERN_INFO "PM: Loading image data pages (%u pages) ...     ",
+	printk(KERN_INFO "PM: Loading image data pages (%u pages)...\n",
 		nr_to_read);
-	m = nr_to_read / 100;
+	m = nr_to_read / 10;
 	if (!m)
 		m = 1;
 	nr_pages = 0;
@@ -993,7 +989,8 @@ static int load_image(struct swap_map_handle *handle,
 		if (ret)
 			break;
 		if (!(nr_pages % m))
-			printk("\b\b\b\b%3d%%", nr_pages / m);
+			printk(KERN_INFO "PM: Loaded %3d%%...\n",
+			       nr_pages / m * 10);
 		nr_pages++;
 	}
 	err2 = hib_wait_on_bio_chain(&bio);
@@ -1001,12 +998,11 @@ static int load_image(struct swap_map_handle *handle,
 	if (!ret)
 		ret = err2;
 	if (!ret) {
-		printk("\b\b\b\bdone\n");
+		printk(KERN_INFO "PM: Loading image done.\n");
 		snapshot_write_finalize(snapshot);
 		if (!snapshot_image_loaded(snapshot))
 			ret = -ENODATA;
-	} else
-		printk("\n");
+	}
 	swsusp_show_speed(&start, &stop, nr_to_read, "Read");
 	return ret;
 }
@@ -1185,9 +1181,9 @@ static int load_image_lzo(struct swap_map_handle *handle,
 
 	printk(KERN_INFO
 		"PM: Using %u thread(s) for decompression.\n"
-		"PM: Loading and decompressing image data (%u pages) ...     ",
+		"PM: Loading and decompressing image data (%u pages)...\n",
 		nr_threads, nr_to_read);
-	m = nr_to_read / 100;
+	m = nr_to_read / 10;
 	if (!m)
 		m = 1;
 	nr_pages = 0;
@@ -1319,7 +1315,9 @@ static int load_image_lzo(struct swap_map_handle *handle,
 				       data[thr].unc + off, PAGE_SIZE);
 
 				if (!(nr_pages % m))
-					printk("\b\b\b\b%3d%%", nr_pages / m);
+					printk(KERN_INFO
+					       "PM: Loaded %3d%%...\n",
+					       nr_pages / m * 10);
 				nr_pages++;
 
 				ret = snapshot_write_next(snapshot);
@@ -1344,7 +1342,7 @@ out_finish:
 	}
 	do_gettimeofday(&stop);
 	if (!ret) {
-		printk("\b\b\b\bdone\n");
+		printk(KERN_INFO "PM: Loading image done.\n");
 		snapshot_write_finalize(snapshot);
 		if (!snapshot_image_loaded(snapshot))
 			ret = -ENODATA;
@@ -1357,8 +1355,7 @@ out_finish:
 				}
 			}
 		}
-	} else
-		printk("\n");
+	}
 	swsusp_show_speed(&start, &stop, nr_to_read, "Read");
 out_clean:
 	for (i = 0; i < ring_size; i++)
----------------

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Borislav Petkov]

On Tue, Jun 19, 2012 at 12:09:57PM +1000, Bojan Smojver wrote:
> On Mon, 2012-06-18 at 23:28 +0200, Rafael J. Wysocki wrote:
> > I think I'd prefer a full status line every 10%.  Or even maybe every
> > 20%.
> 
> Here is one that does a line every 10%:

Yep, here's the output:

[  168.703731] PM: thaw of devices complete after 1474.836 msecs
[  168.713213] PM: Using 1 thread(s) for compression.
[  168.713213] PM: Compressing and saving image data (75028 pages)...
[  168.713497] PM: Saved   0%...
[  169.440467] PM: Saved  10%...
[  169.775404] PM: Saved  20%...
[  170.143273] PM: Saved  30%...
[  170.677134] PM: Saved  40%...
[  171.430144] PM: Saved  50%...
[  172.279117] PM: Saved  60%...
[  173.046114] PM: Saved  70%...
[  173.581322] PM: Saved  80%...
[  174.088198] PM: Saved  90%...
[  174.584271] PM: Saved 100%...
[  174.612895] PM: Saving image done.
		^^^^^^^^^^^ This one is probably not needed - we say 100% in the line above.

[  174.614692] PM: Wrote 300112 kbytes in 5.90 seconds (50.86 MB/s)

And then the "Saved %d%%... " could probably issue on a single line using
KERN_CONT (or what was it now with the new printk changes?):

PM: Saving image: 10%... 20%... 30%... ... 100%.
PM: Wrote 300112 kbytes in 5.90 seconds (50.86 MB/s)

which, on the whole, is nice and compact and still contains the whole
info.

Thanks.

-- 
Regards/Gruss,
Boris.

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

Borislav Petkov <bp@alien8.de> wrote:

>which, on the whole, is nice and compact and still contains the whole
>info.

Yeah, that was the deal with the dots too. But, Rafael suggested we do lines (and I see his point). This way any other lines printed in the meantime (I have seen that happen) will also print properly. Ditto that last line - 100% is still not quite finished. :-)

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Rafael J. Wysocki]

On Tuesday, June 19, 2012, Bojan Smojver wrote:
> Borislav Petkov <bp@alien8.de> wrote:
> 
> >which, on the whole, is nice and compact and still contains the whole
> >info.
> 
> Yeah, that was the deal with the dots too. But, Rafael suggested we do
> lines (and I see his point). This way any other lines printed in the meantime
> (I have seen that happen) will also print properly. Ditto that last line - 100%
> is still not quite finished. :-)

Exactly.

I'd use a slightly different status line, though.  What about:

"PM: Image saving progress: %d%%"

Then, if something is printed in between those lines, it'll still be clear
what they mean.

Thanks,
Rafael

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

>I'd use a slightly different status line, though.  What about:
>
>"PM: Image saving progress: %d%%"
>
>Then, if something is printed in between those lines, it'll still be
>clear
>what they mean.

Yeah, will change and send to you with signed off.

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Mon, 2012-06-18 at 23:28 +0200, Rafael J. Wysocki wrote:
> > PS. Yeah, the current printout with \b is a hack and I do not
> particularly
> > like it either. Proper way would be with plymouth or something like
> that, but
> > I got no joy with that on that list. One day, maybe...
> 
> I'm not sure what you mean?

Sorry, I never answered your question here.

Plymouth (and other such software) provides graphical boot support. We
should be sending stuff to it to display nice, graphical
hibernation/thaw progress indicator. An exercise for the future,
perhaps.

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Rafael J. Wysocki]

On Wednesday, May 09, 2012, Bojan Smojver wrote:
> On Wed, 2012-05-09 at 13:40 +0530, Srivatsa S. Bhat wrote:
> > > +             error = suspend_devices_and_enter(PM_SUSPEND_MEM);
> > 
> > 
> > I can imagine running into a host of problems here, since the suspend
> > sequence is not carried out fully, from the beginning.
> > 
> > For example, this will skip sending out the PM_SUSPEND_PREPARE and the
> > PM_POST_SUSPEND notifiers. Worse, we actually send out the
> > PM_HIBERNATION_PREPARE
> > and PM_POST_HIBERNATION notifiers and then do a suspend instead,
> > underneath!
> > 
> > (Similar cases for the rest of the notifiers sent during suspend vs
> > hibernation).
> > 
> > Don't we need to handle such things properly, in order to make
> > suspend-to-both
> > work reliably? 
> 
> Honest answer - I have absolutely no idea. I've seen the code of
> suspend-utils (i.e. user mode stuff) and it seems to me that it does
> exactly this. Could be wrong of course, just like many times before.
> 
> Rafael?

Sorry, that has fallen out of my radar somehow.

Srivatsa is right, we should generally pay attention to those details.

I think we should generally use a different "prepare" notification for the
save-image-and-suspend case.

Thanks,
Rafael

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

>On Wednesday, May 09, 2012, Bojan Smojver wrote:
>> On Wed, 2012-05-09 at 13:40 +0530, Srivatsa S. Bhat wrote:
>> > > +             error = suspend_devices_and_enter(PM_SUSPEND_MEM);
>> > 
>> > 
>> > I can imagine running into a host of problems here, since the
>suspend
>> > sequence is not carried out fully, from the beginning.
>> > 
>> > For example, this will skip sending out the PM_SUSPEND_PREPARE and
>the
>> > PM_POST_SUSPEND notifiers. Worse, we actually send out the
>> > PM_HIBERNATION_PREPARE
>> > and PM_POST_HIBERNATION notifiers and then do a suspend instead,
>> > underneath!
>> > 
>> > (Similar cases for the rest of the notifiers sent during suspend vs
>> > hibernation).
>> > 
>> > Don't we need to handle such things properly, in order to make
>> > suspend-to-both
>> > work reliably? 
>> 
>> Honest answer - I have absolutely no idea. I've seen the code of
>> suspend-utils (i.e. user mode stuff) and it seems to me that it does
>> exactly this. Could be wrong of course, just like many times before.
>> 
>> Rafael?
>
>Sorry, that has fallen out of my radar somehow.
>
>Srivatsa is right, we should generally pay attention to those details.
>
>I think we should generally use a different "prepare" notification for
>the
>save-image-and-suspend case.
>
>Thanks,
>Rafael


OK, I will try to rework then, if that is the case.

What I don't understand is this: should the hibernation fail for some reason, we would get the same hibernation code unwind that failure, right? So, a suspend after the image write will be just one long "failure", after which hibernation code has to unwind again. No?

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Rafael J. Wysocki]

On Sunday, May 13, 2012, Bojan Smojver wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> >On Wednesday, May 09, 2012, Bojan Smojver wrote:
> >> On Wed, 2012-05-09 at 13:40 +0530, Srivatsa S. Bhat wrote:
> >> > > +             error = suspend_devices_and_enter(PM_SUSPEND_MEM);
> >> > 
> >> > 
> >> > I can imagine running into a host of problems here, since the
> >suspend
> >> > sequence is not carried out fully, from the beginning.
> >> > 
> >> > For example, this will skip sending out the PM_SUSPEND_PREPARE and
> >the
> >> > PM_POST_SUSPEND notifiers. Worse, we actually send out the
> >> > PM_HIBERNATION_PREPARE
> >> > and PM_POST_HIBERNATION notifiers and then do a suspend instead,
> >> > underneath!
> >> > 
> >> > (Similar cases for the rest of the notifiers sent during suspend vs
> >> > hibernation).
> >> > 
> >> > Don't we need to handle such things properly, in order to make
> >> > suspend-to-both
> >> > work reliably? 
> >> 
> >> Honest answer - I have absolutely no idea. I've seen the code of
> >> suspend-utils (i.e. user mode stuff) and it seems to me that it does
> >> exactly this. Could be wrong of course, just like many times before.
> >> 
> >> Rafael?
> >
> >Sorry, that has fallen out of my radar somehow.
> >
> >Srivatsa is right, we should generally pay attention to those details.
> >
> >I think we should generally use a different "prepare" notification for
> >the
> >save-image-and-suspend case.
> >
> >Thanks,
> >Rafael
> 
> 
> OK, I will try to rework then, if that is the case.
> 
> What I don't understand is this: should the hibernation fail for some reason,
> we would get the same hibernation code unwind that failure, right?

Yes, if the failure happens before we attempt to suspend.

> So, a suspend after the image write will be just one long "failure", after
> which hibernation code has to unwind again. No?

Hmm.  Good question.  It should be like this I think, although there may be
some corner cases lurking.

Thanks,
Rafael

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Sun, 2012-05-13 at 15:10 +0200, Rafael J. Wysocki wrote:
> > So, a suspend after the image write will be just one long "failure",
> after
> > which hibernation code has to unwind again. No?
> 
> Hmm.  Good question.  It should be like this I think, although there
> may be some corner cases lurking.

AFAICT, the user space suspend to both works this way.

If I understand thing correctly, at the point of suspend to memory, we
are not really running a full system any more. All the processes have
been frozen and they've been told that hibernation is in progress. Ditto
devices, minus the ones we use to write to swap. So, when hibernation
code unwinds post resume from memory, it would be doing the correct
thing.

Srivatsa, does that make sense to you?

-- 
Bojan

Re: [PATCH]: In kernel hibernation, suspend to both

[Srivatsa S. Bhat]

On 05/14/2012 04:48 AM, Bojan Smojver wrote:

> On Sun, 2012-05-13 at 15:10 +0200, Rafael J. Wysocki wrote:
>>> So, a suspend after the image write will be just one long "failure",
>> after
>>> which hibernation code has to unwind again. No?
>>
>> Hmm.  Good question.  It should be like this I think, although there
>> may be some corner cases lurking.
> 
> AFAICT, the user space suspend to both works this way.
> 


But does it carry out suspend-to-ram in full, or does it skip the
PM_SUSPEND_PREPARE notifications?

> If I understand thing correctly, at the point of suspend to memory, we
> are not really running a full system any more. All the processes have
> been frozen and they've been told that hibernation is in progress. Ditto
> devices, minus the ones we use to write to swap. So, when hibernation
> code unwinds post resume from memory, it would be doing the correct
> thing.
> 
> Srivatsa, does that make sense to you?
> 


See my concerns about the preparation stage for suspend, in my other
mail as well. I would have been less worried if we did things in full:
1. prepare for hibernation
2. create image
3. prepare for suspend
4. do suspend
5. do resume
6. unwind from hibernation code (like a failed hibernation).

Still no guarantees, but somewhat better.
(consider what will happen if some code expects completion of an operation,
successful or otherwise, before starting another.. and hence is unprepared
for something like:
hibernation prepare -> suspend prepare -> post suspend -> post hibernation)

If we don't rework this thing carefully via a special notification for
save-image-and-suspend like what Rafael suggested, and instead try to go
with existing stuff as it is, I would definitely suggest putting this feature
under something like CONFIG_EXPERIMENTAL ;-)

Regards,
Srivatsa S. Bhat

Re: [PATCH]: In kernel hibernation, suspend to both

[Bojan Smojver]

On Mon, 2012-05-14 at 05:19 +0530, Srivatsa S. Bhat wrote:
> But does it carry out suspend-to-ram in full, or does it skip the
> PM_SUSPEND_PREPARE notifications?

As far as I can see, it simply does an ioctl that suspends to RAM
immediately, without any prior notifications.

> See my concerns about the preparation stage for suspend, in my other
> mail as well. I would have been less worried if we did things in full:
> 1. prepare for hibernation
> 2. create image
> 3. prepare for suspend
> 4. do suspend
> 5. do resume
> 6. unwind from hibernation code (like a failed hibernation).
> 
> Still no guarantees, but somewhat better.
> (consider what will happen if some code expects completion of an
> operation,
> successful or otherwise, before starting another.. and hence is
> unprepared
> for something like:
> hibernation prepare -> suspend prepare -> post suspend -> post
> hibernation)

What I'm trying to say is that from the point of view of the rest of the
code, suspend to RAM never happened. Only a failed hibernation happened.

At the point where we suspend to RAM, only the devices that are required
for image writing are actually active. The rest is frozen, including all
the processes, if I'm understanding things correctly.

If these devices can survive a failed image writing and are then be
subjected to unwinding by hibernation code, how would a suspend to RAM
right in the middle change any of this? Maybe it does - don't really
know...

> If we don't rework this thing carefully via a special notification for
> save-image-and-suspend like what Rafael suggested, and instead try to
> go
> with existing stuff as it is, I would definitely suggest putting this
> feature
> under something like CONFIG_EXPERIMENTAL ;-) 

Yeah, possibly.

-- 
Bojan