[PATCH 0/2] Freezer fixes for 2.6.24

[PATCH 0/2] Freezer fixes for 2.6.24

[Rafael J. Wysocki]

Hi Len,

The following two patches are intended to fix regressions related to the
freezer.

The first one fixes http://bugzilla.kernel.org/show_bug.cgi?id=9345 and has
been tested by the reporter.

The second one is intended to fix the APM emulation breakage caused before
2.6.23, but unfortunately I can't test it.  The problem is a bit academic,
though, since no one has reported this problem so far, so apparently this
driver is not very popular. ;-)

Please apply.

Thanks,
Rafael


-- 
"Premature optimization is the root of all evil." - Donald Knuth

[PATCH 1/2] Freezer: Fix s2disk resume from initrd

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Add appropriate freezer annotations to handle_initrd(), so that it's possible
to resume from disk from an initrd.

This patch fixes Bug #9345: http://bugzilla.kernel.org/show_bug.cgi?id=9345

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Nigel Cunningham <nigel@nigel.suspend2.net>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Chris Friedhoff <chris@friedhoff.org>
---
 init/do_mounts_initrd.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Index: linux-2.6/init/do_mounts_initrd.c
===================================================================
--- linux-2.6.orig/init/do_mounts_initrd.c
+++ linux-2.6/init/do_mounts_initrd.c
@@ -55,12 +55,18 @@ static void __init handle_initrd(void)
 	sys_mount(".", "/", NULL, MS_MOVE, NULL);
 	sys_chroot(".");
 
+	/*
+	 * In case that a resume from disk is carreid out by linuxrc or one of
+	 * its children, we need to tell the freezer not to wait for us
+	 */
+	current->flags |= PF_FREEZER_SKIP;
+
 	pid = kernel_thread(do_linuxrc, "/linuxrc", SIGCHLD);
 	if (pid > 0)
-		while (pid != sys_wait4(-1, NULL, 0, NULL)) {
-			try_to_freeze();
+		while (pid != sys_wait4(-1, NULL, 0, NULL))
 			yield();
-		}
+
+	current->flags &= ~PF_FREEZER_SKIP;
 
 	/* move initrd to rootfs' /old */
 	sys_fchdir(old_fd);

Re: [PATCH 1/2] Freezer: Fix s2disk resume from initrd

[Nigel Cunningham]

Hi.

Just a typo...

On Wednesday 21 November 2007 12:50:17 Rafael J. Wysocki wrote:

> +	/*
> +	 * In case that a resume from disk is carreid out by linuxrc or one of

carried

> +	 * its children, we need to tell the freezer not to wait for us

(final full stop also missing)

Regards,

Nigel
-- 
See http://www.tuxonice.net for Howtos, FAQs, mailing
lists, wiki and bugzilla info.

Re: [PATCH 1/2] Freezer: Fix s2disk resume from initrd

[Rafael J. Wysocki]

On Wednesday, 21 of November 2007, Nigel Cunningham wrote:
> Hi.
> 
> Just a typo...
> 
> On Wednesday 21 November 2007 12:50:17 Rafael J. Wysocki wrote:
> 
> > +	/*
> > +	 * In case that a resume from disk is carreid out by linuxrc or one of
> 
> carried
> 
> > +	 * its children, we need to tell the freezer not to wait for us
> 
> (final full stop also missing)

Thanks, I'll fix the comment in the next iteration.

Rafael

[PATCH 2/2] Freezer: Fix APM emulation breakage

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

The APM emulation is currently broken as a result of commit
831441862956fffa17b9801db37e6ea1650b0f69
"Freezer: make kernel threads nonfreezable by default"
that removed the PF_NOFREEZE annotations from apm_ioctl() without adding
the appropriate freezer hooks.  Fix it and remove the unnecessary variable flags
from apm_ioctl().

Special thanks to Franck Bui-Huu <vagabon.xyz@gmail.com> for pointing out the
problem.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Franck Bui-Huu <vagabon.xyz@gmail.com>
Cc: Nigel Cunningham <nigel@nigel.suspend2.net>
---
 drivers/char/apm-emulation.c |   15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/char/apm-emulation.c
===================================================================
--- linux-2.6.orig/drivers/char/apm-emulation.c
+++ linux-2.6/drivers/char/apm-emulation.c
@@ -295,7 +295,6 @@ static int
 apm_ioctl(struct inode * inode, struct file *filp, u_int cmd, u_long arg)
 {
 	struct apm_user *as = filp->private_data;
-	unsigned long flags;
 	int err = -EINVAL;
 
 	if (!as->suser || !as->writer)
@@ -331,10 +330,16 @@ apm_ioctl(struct inode * inode, struct f
 			 * Wait for the suspend/resume to complete.  If there
 			 * are pending acknowledges, we wait here for them.
 			 */
-			flags = current->flags;
+			freezer_do_not_count();
 
 			wait_event(apm_suspend_waitqueue,
 				   as->suspend_state == SUSPEND_DONE);
+
+			/*
+			 * Since we are waiting until the suspend is done, the
+			 * try_to_freeze() in freezer_count() will not trigger
+			 */
+			freezer_count();
 		} else {
 			as->suspend_state = SUSPEND_WAIT;
 			mutex_unlock(&state_lock);
@@ -362,14 +367,10 @@ apm_ioctl(struct inode * inode, struct f
 			 * Wait for the suspend/resume to complete.  If there
 			 * are pending acknowledges, we wait here for them.
 			 */
-			flags = current->flags;
-
-			wait_event_interruptible(apm_suspend_waitqueue,
+			wait_event_freezable(apm_suspend_waitqueue,
 					 as->suspend_state == SUSPEND_DONE);
 		}
 
-		current->flags = flags;
-
 		mutex_lock(&state_lock);
 		err = as->suspend_result;
 		as->suspend_state = SUSPEND_NONE;

Re: [PATCH 0/2] Freezer fixes for 2.6.24

[Franck Bui-Huu]

Rafael J. Wysocki wrote:
> The second one is intended to fix the APM emulation breakage caused before
> 2.6.23, but unfortunately I can't test it.

Why not ?

Just hack Kconfig to be able to select this driver on x86 architecture
and it should work fine, no ?

I'll give it a test today.

		Franck

Re: [PATCH 0/2] Freezer fixes for 2.6.24

[Rafael J. Wysocki]

On Wednesday, 21 of November 2007, Franck Bui-Huu wrote:
> Rafael J. Wysocki wrote:
> > The second one is intended to fix the APM emulation breakage caused before
> > 2.6.23, but unfortunately I can't test it.
> 
> Why not ?
> 
> Just hack Kconfig to be able to select this driver on x86 architecture
> and it should work fine, no ?

Well, I'm afraid I have no suitable userland here ...

> I'll give it a test today.

OK, thanks!

Rafael

Re: [PATCH 0/2] Freezer fixes for 2.6.24

[Franck Bui-Huu]

Rafael J. Wysocki wrote:
> On Wednesday, 21 of November 2007, Franck Bui-Huu wrote:
>> Rafael J. Wysocki wrote:
>>> The second one is intended to fix the APM emulation breakage caused before
>>> 2.6.23, but unfortunately I can't test it.
>> Why not ?
>>
>> Just hack Kconfig to be able to select this driver on x86 architecture
>> and it should work fine, no ?
> 
> Well, I'm afraid I have no suitable userland here ...
> 

I don't have either.

Anyway I just verified that the current code is broken. I put 2
processes waiting for a suspend evetn by reading the /dev/apm_bios
device and a third process that does the suspend event by doing an
ioctl() on the apm device. The system doesn't suspend (the kernel
dumped a list of the current tasks, instead of printing "Freezing of
tasks failed after..." BTW) and eventually the third process exits
with an incorrect value: -EINTR.

Applying your patch makes the previous test case work.

I also discovered that s2ram doesn't work on my laptop and had to use
standby suspend. I'll try to find out why.

		Franck

Re: [PATCH 0/2] Freezer fixes for 2.6.24

[Rafael J. Wysocki]

On Thursday, 22 of November 2007, Franck Bui-Huu wrote:
> Rafael J. Wysocki wrote:
> > On Wednesday, 21 of November 2007, Franck Bui-Huu wrote:
> >> Rafael J. Wysocki wrote:
> >>> The second one is intended to fix the APM emulation breakage caused before
> >>> 2.6.23, but unfortunately I can't test it.
> >> Why not ?
> >>
> >> Just hack Kconfig to be able to select this driver on x86 architecture
> >> and it should work fine, no ?
> > 
> > Well, I'm afraid I have no suitable userland here ...
> > 
> 
> I don't have either.
> 
> Anyway I just verified that the current code is broken. I put 2
> processes waiting for a suspend evetn by reading the /dev/apm_bios
> device and a third process that does the suspend event by doing an
> ioctl() on the apm device. The system doesn't suspend (the kernel
> dumped a list of the current tasks, instead of printing "Freezing of
> tasks failed after..." BTW) and eventually the third process exits
> with an incorrect value: -EINTR.
> 
> Applying your patch makes the previous test case work.

IOW, the patch helps?

> I also discovered that s2ram doesn't work on my laptop and had to use
> standby suspend. I'll try to find out why.

Yes, please.

Thanks,
Rafael

Re: [PATCH 0/2] Freezer fixes for 2.6.24

[Franck Bui-Huu]

Rafael J. Wysocki wrote:
> On Thursday, 22 of November 2007, Franck Bui-Huu wrote:
>> Anyway I just verified that the current code is broken. I put 2
>> processes waiting for a suspend evetn by reading the /dev/apm_bios
>> device and a third process that does the suspend event by doing an
>> ioctl() on the apm device. The system doesn't suspend (the kernel
>> dumped a list of the current tasks, instead of printing "Freezing of
>> tasks failed after..." BTW) and eventually the third process exits
>> with an incorrect value: -EINTR.
>>
>> Applying your patch makes the previous test case work.
> 
> IOW, the patch helps?

yes.

		Franck