Re: Fixes to suspend-to-RAM

Re: Fixes to suspend-to-RAM

[Patrick Mochel]

>  void __init acpi_reserve_bootmem(void)
>  {
>  	acpi_wakeup_address = (unsigned long)alloc_bootmem_low(PAGE_SIZE);
> +	if (!acpi_wakeup_address)
> +		printk(KERN_ERR "ACPI: Cannot allocate lowmem. S3 disabled.\n");
>  	if ((&wakeup_end - &wakeup_start) > PAGE_SIZE)
>  		printk(KERN_CRIT "ACPI: Wakeup code way too big, will crash on attempt to suspend\n");
> -	printk(KERN_DEBUG "ACPI: have wakeup address 0x%8.8lx\n", acpi_wakeup_address);

If you say you're disabling S3, then please really do so and flip the bit 
in the sleep_states[] array.

> --- clean/drivers/acpi/sleep/main.c	2003-02-15 18:51:17.000000000 +0100
> +++ linux/drivers/acpi/sleep/main.c	2003-02-15 18:57:27.000000000 +0100
> @@ -103,6 +103,10 @@
>  			return error;
>  		}
>  
> +		error = device_suspend(state, SUSPEND_DISABLE);
> +		if (error)
> +			panic("Sorry, devices really should know how to disable\n");
> +

Why is every error condition a panic()? That certainly does not add 
robustness to the code..

Also, you say that the APIC needs this state. I wonder if that should be
done in the SUSPEND_POWER_DOWN stage with interrupts disabled?

	-pat

Fixes to suspend-to-RAM

[Pavel Machek]

Hi!

Clean up comments in sleep.c and make code robust against
out-of-memory. In wakeup.S, reload segment registers: bios might have
changed them. In main.c -- we really need to disable devices before
going to sleep -- its critical for APIC (driver model for that will be
supplied later). Please apply,
								Pavel

--- clean/arch/i386/kernel/acpi/sleep.c	2003-02-15 18:51:10.000000000 +0100
+++ linux/arch/i386/kernel/acpi/sleep.c	2003-02-15 19:00:15.000000000 +0100
@@ -2,6 +2,7 @@
  * sleep.c - x86-specific ACPI sleep support.
  *
  *  Copyright (C) 2001-2003 Patrick Mochel
+ *  Copyright (C) 2001-2003 Pavel Machek <pavel@suse.cz>
  */
 
 #include <linux/acpi.h>
@@ -38,6 +39,8 @@
 	panic("S3 and PAE do not like each other for now.");
 	return 1;
 #endif
+	if (!acpi_wakeup_address)
+		return 1;
 	init_low_mapping(swapper_pg_dir, USER_PTRS_PER_PGD);
 	memcpy((void *) acpi_wakeup_address, &wakeup_start, &wakeup_end - &wakeup_start);
 	acpi_copy_wakeup_routine(acpi_wakeup_address);
@@ -65,17 +68,18 @@
 /**
  * acpi_reserve_bootmem - do _very_ early ACPI initialisation
  *
- * We allocate a page in low memory for the wakeup
+ * We allocate a page from the first 1MB of memory for the wakeup
  * routine for when we come back from a sleep state. The
- * runtime allocator allows specification of <16M pages, but not
- * <1M pages.
+ * runtime allocator allows specification of <16MB pages, but not
+ * <1MB pages.
  */
 void __init acpi_reserve_bootmem(void)
 {
 	acpi_wakeup_address = (unsigned long)alloc_bootmem_low(PAGE_SIZE);
+	if (!acpi_wakeup_address)
+		printk(KERN_ERR "ACPI: Cannot allocate lowmem. S3 disabled.\n");
 	if ((&wakeup_end - &wakeup_start) > PAGE_SIZE)
 		printk(KERN_CRIT "ACPI: Wakeup code way too big, will crash on attempt to suspend\n");
-	printk(KERN_DEBUG "ACPI: have wakeup address 0x%8.8lx\n", acpi_wakeup_address);
 }
 
 static int __init acpi_sleep_setup(char *str)
--- clean/arch/i386/kernel/acpi/wakeup.S	2003-02-15 18:51:10.000000000 +0100
+++ linux/arch/i386/kernel/acpi/wakeup.S	2003-02-18 00:58:24.000000000 +0100
@@ -44,6 +44,9 @@
 	testl	$1, video_flags - wakeup_code
 	jz	1f
 	lcall   $0xc000,$3
+	movw	%cs, %ax
+	movw	%ax, %ds					# Bios might have played with that
+	movw	%ax, %ss
 1:
 
 	testl	$2, video_flags - wakeup_code
--- clean/drivers/acpi/sleep/main.c	2003-02-15 18:51:17.000000000 +0100
+++ linux/drivers/acpi/sleep/main.c	2003-02-15 18:57:27.000000000 +0100
@@ -103,6 +103,10 @@
 			return error;
 		}
 
+		error = device_suspend(state, SUSPEND_DISABLE);
+		if (error)
+			panic("Sorry, devices really should know how to disable\n");
+
 		/* flush caches */
 		ACPI_FLUSH_CPU_CACHE();
 

-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: Fixes to suspend-to-RAM

[Patrick Mochel]

On Tue, 18 Feb 2003, Pavel Machek wrote:

> Hi!
> 
> > >  void __init acpi_reserve_bootmem(void)
> > >  {
> > >  	acpi_wakeup_address = (unsigned long)alloc_bootmem_low(PAGE_SIZE);
> > > +	if (!acpi_wakeup_address)
> > > +		printk(KERN_ERR "ACPI: Cannot allocate lowmem. S3 disabled.\n");
> > >  	if ((&wakeup_end - &wakeup_start) > PAGE_SIZE)
> > >  		printk(KERN_CRIT "ACPI: Wakeup code way too big, will crash on attempt to suspend\n");
> > > -	printk(KERN_DEBUG "ACPI: have wakeup address 0x%8.8lx\n", acpi_wakeup_address);
> > 
> > If you say you're disabling S3, then please really do so and flip the bit 
> > in the sleep_states[] array.
> 
> Check the code, there's a conditional former in the file and S3 will
> correctly fail. I don't feel like flipping bits from here
> (arch-dep-code) in generic code.

Fine. But, how about a call that the drivers/acpi/sleep/main.c::sleep_init()
can call to arch/i386/kernel/acpi/ to validate that it can do S3 from the 
beginning. That way, we're more certain of whether or not we're going to 
succeed or not. 

Doing this can also get rid of this crap, too:

int acpi_save_state_mem (void)
{
#if CONFIG_X86_PAE
        panic("S3 and PAE do not like each other for now.");
        return 1;
#endif

> Good idea. So acpi_system_save_state can no longer be called with S5,
> right? That allows quite a lot of cleanup.

Correct.

> Hmm, how do you propose error handling when SUSPEND_DISABLE fails?
> Call SUSPEND_ENABLE or not? Once that is decided  panic can go. But I
> still think that just should not happen.

I propose that we don't even call SUSPEND_DISABLE. Based on recent
conversations, it appears that we can and should do the entire device
suspend sequence in two passes - SUSPEND_SAVE_STATE with interrupts
enabled, and SUSPEND_POWER_DOWN with interrupts disabled.


	-pat

Re: Fixes to suspend-to-RAM

[Pavel Machek]

Hi!

> >  void __init acpi_reserve_bootmem(void)
> >  {
> >  	acpi_wakeup_address = (unsigned long)alloc_bootmem_low(PAGE_SIZE);
> > +	if (!acpi_wakeup_address)
> > +		printk(KERN_ERR "ACPI: Cannot allocate lowmem. S3 disabled.\n");
> >  	if ((&wakeup_end - &wakeup_start) > PAGE_SIZE)
> >  		printk(KERN_CRIT "ACPI: Wakeup code way too big, will crash on attempt to suspend\n");
> > -	printk(KERN_DEBUG "ACPI: have wakeup address 0x%8.8lx\n", acpi_wakeup_address);
> 
> If you say you're disabling S3, then please really do so and flip the bit 
> in the sleep_states[] array.

Check the code, there's a conditional former in the file and S3 will
correctly fail. I don't feel like flipping bits from here
(arch-dep-code) in generic code.

> > --- clean/drivers/acpi/sleep/main.c	2003-02-15 18:51:17.000000000 +0100
> > +++ linux/drivers/acpi/sleep/main.c	2003-02-15 18:57:27.000000000 +0100
> > @@ -103,6 +103,10 @@
> >  			return error;
> >  		}
> >  
> > +		error = device_suspend(state, SUSPEND_DISABLE);
> > +		if (error)
> > +			panic("Sorry, devices really should know how to disable\n");
> > +
> 
> Why is every error condition a panic()? That certainly does not add 
> robustness to the code..
> 
> Also, you say that the APIC needs this state. I wonder if that should be
> done in the SUSPEND_POWER_DOWN stage with interrupts disabled?

Good idea. So acpi_system_save_state can no longer be called with S5,
right? That allows quite a lot of cleanup.

Hmm, how do you propose error handling when SUSPEND_DISABLE fails?
Call SUSPEND_ENABLE or not? Once that is decided  panic can go. But I
still think that just should not happen.

								Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: Fixes to suspend-to-RAM

[Patrick Mochel]

> Bootmem needs to be reserved pretty soon in the boot process, that
> might be a problem.

That's not the issue. The call to the arch code would only check if the 
bootmem had been reserved, and as far the arch code knew, it was OK to 
enable S3. 


> Based on recent talk... Will you act as S3 maintainer so that I can
> submit patches to you and you'll take care of forwarding to Linus?

Yes, but please don't flood me with patches yet. I'm getting reacquainted 
with some of the more esoteric details of suspend states, and verifying 
that we have a working PM model for 2.6.

	-pat

Re: Fixes to suspend-to-RAM

[Pavel Machek]

Hi!

> > > >  void __init acpi_reserve_bootmem(void)
> > > >  {
> > > >  	acpi_wakeup_address = (unsigned long)alloc_bootmem_low(PAGE_SIZE);
> > > > +	if (!acpi_wakeup_address)
> > > > +		printk(KERN_ERR "ACPI: Cannot allocate lowmem. S3 disabled.\n");
> > > >  	if ((&wakeup_end - &wakeup_start) > PAGE_SIZE)
> > > >  		printk(KERN_CRIT "ACPI: Wakeup code way too big, will crash on attempt to suspend\n");
> > > > -	printk(KERN_DEBUG "ACPI: have wakeup address 0x%8.8lx\n", acpi_wakeup_address);
> > > 
> > > If you say you're disabling S3, then please really do so and flip the bit 
> > > in the sleep_states[] array.
> > 
> > Check the code, there's a conditional former in the file and S3 will
> > correctly fail. I don't feel like flipping bits from here
> > (arch-dep-code) in generic code.
> 
> Fine. But, how about a call that the drivers/acpi/sleep/main.c::sleep_init()
> can call to arch/i386/kernel/acpi/ to validate that it can do S3 from the 
> beginning. That way, we're more certain of whether or not we're going to 
> succeed or not. 

Bootmem needs to be reserved pretty soon in the boot process, that
might be a problem.

> Doing this can also get rid of this crap, too:
> 
> int acpi_save_state_mem (void)
> {
> #if CONFIG_X86_PAE
>         panic("S3 and PAE do not like each other for now.");
>         return 1;
> #endif
> 
> > Good idea. So acpi_system_save_state can no longer be called with S5,
> > right? That allows quite a lot of cleanup.
> 
> Correct.
> 
> > Hmm, how do you propose error handling when SUSPEND_DISABLE fails?
> > Call SUSPEND_ENABLE or not? Once that is decided  panic can go. But I
> > still think that just should not happen.
> 
> I propose that we don't even call SUSPEND_DISABLE. Based on recent
> conversations, it appears that we can and should do the entire device
> suspend sequence in two passes - SUSPEND_SAVE_STATE with interrupts
> enabled, and SUSPEND_POWER_DOWN with interrupts disabled.

Yes, that crossed my mind, too. That's probably best.

Based on recent talk... Will you act as S3 maintainer so that I can
submit patches to you and you'll take care of forwarding to Linus?

								Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: Fixes to suspend-to-RAM

[Pavel Machek]

Hi!

> > Bootmem needs to be reserved pretty soon in the boot process, that
> > might be a problem.
> 
> That's not the issue. The call to the arch code would only check if the 
> bootmem had been reserved, and as far the arch code knew, it was OK to 
> enable S3.

Like this?

> > Based on recent talk... Will you act as S3 maintainer so that I can
> > submit patches to you and you'll take care of forwarding to Linus?
> 
> Yes, but please don't flood me with patches yet. I'm getting reacquainted 
> with some of the more esoteric details of suspend states, and verifying 
> that we have a working PM model for 2.6.

I have maybe two more patches around this size (i.e. small). Will you
take this?
								Pavel

--- clean/arch/i386/kernel/acpi/sleep.c	2003-02-15 18:51:10.000000000 +0100
+++ linux/arch/i386/kernel/acpi/sleep.c	2003-02-18 23:09:01.000000000 +0100
@@ -2,6 +2,7 @@
  * sleep.c - x86-specific ACPI sleep support.
  *
  *  Copyright (C) 2001-2003 Patrick Mochel
+ *  Copyright (C) 2001-2003 Pavel Machek <pavel@suse.cz>
  */
 
 #include <linux/acpi.h>
@@ -34,10 +35,8 @@
  */
 int acpi_save_state_mem (void)
 {
-#if CONFIG_X86_PAE
-	panic("S3 and PAE do not like each other for now.");
-	return 1;
-#endif
+	if (!acpi_wakeup_address)
+		return 1;
 	init_low_mapping(swapper_pg_dir, USER_PTRS_PER_PGD);
 	memcpy((void *) acpi_wakeup_address, &wakeup_start, &wakeup_end - &wakeup_start);
 	acpi_copy_wakeup_routine(acpi_wakeup_address);
@@ -65,17 +64,24 @@
 /**
  * acpi_reserve_bootmem - do _very_ early ACPI initialisation
  *
- * We allocate a page in low memory for the wakeup
+ * We allocate a page from the first 1MB of memory for the wakeup
  * routine for when we come back from a sleep state. The
- * runtime allocator allows specification of <16M pages, but not
- * <1M pages.
+ * runtime allocator allows specification of <16MB pages, but not
+ * <1MB pages.
  */
 void __init acpi_reserve_bootmem(void)
 {
+	if ((&wakeup_end - &wakeup_start) > PAGE_SIZE) {
+		printk(KERN_ERR "ACPI: Wakeup code way too big, S3 disabled.\n");
+		return;
+	}
+#if CONFIG_X86_PAE
+	printk(KERN_ERR "ACPI: S3 and PAE do not like each other for now, S3 disabled.\n");
+	return;
+#endif
 	acpi_wakeup_address = (unsigned long)alloc_bootmem_low(PAGE_SIZE);
-	if ((&wakeup_end - &wakeup_start) > PAGE_SIZE)
-		printk(KERN_CRIT "ACPI: Wakeup code way too big, will crash on attempt to suspend\n");
-	printk(KERN_DEBUG "ACPI: have wakeup address 0x%8.8lx\n", acpi_wakeup_address);
+	if (!acpi_wakeup_address)
+		printk(KERN_ERR "ACPI: Cannot allocate lowmem, S3 disabled.\n");
 }
 
 static int __init acpi_sleep_setup(char *str)


-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: [ACPI] Re: Fixes to suspend-to-RAM

[Benjamin Herrenschmidt]

On Tue, 2003-02-18 at 22:41, Patrick Mochel wrote:

> I propose that we don't even call SUSPEND_DISABLE. Based on recent
> conversations, it appears that we can and should do the entire device
> suspend sequence in two passes - SUSPEND_SAVE_STATE with interrupts
> enabled, and SUSPEND_POWER_DOWN with interrupts disabled.

Right... though I still care about my early pet SUSPEND_NOTIFY for
the few things that need to care about allocations issues (that is
that need to know they can no longer rely on GFP_KERNEL & friends
not blocking until wakeup).

Regarding failure, what I did with success on pmac (I had occasional
failure to suspend from the OHCI controller or video drivers during
tests, though those seem to be quite rare in real life) is to call
back the wakeup calls for devices that got the matching suspend
call.

I beleive it's as safe to just call all wakeup calls in order
instead though, and it makes things simpler (as the individual
drivers should really know in what state they really are).

Ben.