[PATCH] ACPI PM: Restore the 2.6.24 suspend ordering

[PATCH] ACPI PM: Restore the 2.6.24 suspend ordering

[Rafael J. Wysocki]

Hi Len,

Please consider pushing the appended patch for 2.6.25.

It fixed the regression described at:
https://bugzilla.novell.com/show_bug.cgi?id=374217
http://bugzilla.kernel.org/show_bug.cgi?id=10340

details in the changelog.

Thanks,
Rafael


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

Some time ago it turned out that our suspend code ordering broke
some NVidia-based systems that hung if _PTS was executed with one of
the PCI devices, specifically a USB controller, in a low power state.
Then, it was noticed that the suspend code ordering was not compliant
with ACPI 1.0, although it was compliant with ACPI 2.0 (and later),
and it was argued that the code had to be changed for that reason
(ref. http://bugzilla.kernel.org/show_bug.cgi?id=9528).  So we did,
but evidently we did wrong, because it's now turning out that some
systems have been broken by this change (refs.
http://bugzilla.kernel.org/show_bug.cgi?id=10340 ,
https://bugzilla.novell.com/show_bug.cgi?id=374217#c16).  [I said
at that time that something like this might happend, but the majority
of people involved thought that it was improbable due to the
necessity to preserve the compliance of hardware with ACPI 1.0.]
This actually is a quite serious regression from 2.6.24.

Moreover, the ACPI 1.0 ordering of suspend code introduced another
issue that I have only noticed recently.  Namely, if the suspend of
one of devices fails, the already suspended devices will be resumed
without executing _WAK before, which leads to problems on some
systems (for example, in such situations thermal management is
broken on my HP nx6325).  Consequently, it also breaks suspend
debugging on the affected systems.

Note also, that the requirement to execute _PTS before suspending
devices does not really make sense, because the device in question
may be put into a low power state at run time for a reason unrelated
to a system-wide suspend.

For the reasons outlined above, the change of the suspend ordering
should be reverted, which is done by the patch below.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 Documentation/kernel-parameters.txt |    5 --
 drivers/acpi/sleep/main.c           |   71 +++++++-----------------------------
 2 files changed, 14 insertions(+), 62 deletions(-)

Index: linux-2.6/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.orig/drivers/acpi/sleep/main.c
+++ linux-2.6/drivers/acpi/sleep/main.c
@@ -26,21 +26,6 @@ u8 sleep_states[ACPI_S_STATE_COUNT];
 
 #ifdef CONFIG_PM_SLEEP
 static u32 acpi_target_sleep_state = ACPI_STATE_S0;
-static bool acpi_sleep_finish_wake_up;
-
-/*
- * ACPI 2.0 and later want us to execute _PTS after suspending devices, so we
- * allow the user to request that behavior by using the 'acpi_new_pts_ordering'
- * kernel command line option that causes the following variable to be set.
- */
-static bool new_pts_ordering;
-
-static int __init acpi_new_pts_ordering(char *str)
-{
-	new_pts_ordering = true;
-	return 1;
-}
-__setup("acpi_new_pts_ordering", acpi_new_pts_ordering);
 #endif
 
 static int acpi_sleep_prepare(u32 acpi_state)
@@ -91,14 +76,6 @@ static int acpi_pm_begin(suspend_state_t
 
 	if (sleep_states[acpi_state]) {
 		acpi_target_sleep_state = acpi_state;
-		if (new_pts_ordering)
-			return 0;
-
-		error = acpi_sleep_prepare(acpi_state);
-		if (error)
-			acpi_target_sleep_state = ACPI_STATE_S0;
-		else
-			acpi_sleep_finish_wake_up = true;
 	} else {
 		printk(KERN_ERR "ACPI does not support this state: %d\n",
 			pm_state);
@@ -116,14 +93,11 @@ static int acpi_pm_begin(suspend_state_t
 
 static int acpi_pm_prepare(void)
 {
-	if (new_pts_ordering) {
-		int error = acpi_sleep_prepare(acpi_target_sleep_state);
+	int error = acpi_sleep_prepare(acpi_target_sleep_state);
 
-		if (error) {
-			acpi_target_sleep_state = ACPI_STATE_S0;
-			return error;
-		}
-		acpi_sleep_finish_wake_up = true;
+	if (error) {
+		acpi_target_sleep_state = ACPI_STATE_S0;
+		return error;
 	}
 
 	return ACPI_SUCCESS(acpi_hw_disable_all_gpes()) ? 0 : -EFAULT;
@@ -212,7 +186,6 @@ static void acpi_pm_finish(void)
 	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
 
 	acpi_target_sleep_state = ACPI_STATE_S0;
-	acpi_sleep_finish_wake_up = false;
 
 #ifdef CONFIG_X86
 	if (init_8259A_after_S1) {
@@ -229,11 +202,10 @@ static void acpi_pm_finish(void)
 static void acpi_pm_end(void)
 {
 	/*
-	 * This is necessary in case acpi_pm_finish() is not called directly
-	 * during a failing transition to a sleep state.
+	 * This is necessary in case acpi_pm_finish() is not called during a
+	 * failing transition to a sleep state.
 	 */
-	if (acpi_sleep_finish_wake_up)
-		acpi_pm_finish();
+	acpi_target_sleep_state = ACPI_STATE_S0;
 }
 
 static int acpi_pm_state_valid(suspend_state_t pm_state)
@@ -296,31 +268,18 @@ __setup("acpi_s4_nosigcheck", acpi_s4_no
 
 static int acpi_hibernation_begin(void)
 {
-	int error;
-
 	acpi_target_sleep_state = ACPI_STATE_S4;
-	if (new_pts_ordering)
-		return 0;
 
-	error = acpi_sleep_prepare(ACPI_STATE_S4);
-	if (error)
-		acpi_target_sleep_state = ACPI_STATE_S0;
-	else
-		acpi_sleep_finish_wake_up = true;
-
-	return error;
+	return 0;
 }
 
 static int acpi_hibernation_prepare(void)
 {
-	if (new_pts_ordering) {
-		int error = acpi_sleep_prepare(ACPI_STATE_S4);
+	int error = acpi_sleep_prepare(ACPI_STATE_S4);
 
-		if (error) {
-			acpi_target_sleep_state = ACPI_STATE_S0;
-			return error;
-		}
-		acpi_sleep_finish_wake_up = true;
+	if (error) {
+		acpi_target_sleep_state = ACPI_STATE_S0;
+		return error;
 	}
 
 	return ACPI_SUCCESS(acpi_hw_disable_all_gpes()) ? 0 : -EFAULT;
@@ -370,17 +329,15 @@ static void acpi_hibernation_finish(void
 	acpi_set_firmware_waking_vector((acpi_physical_address) 0);
 
 	acpi_target_sleep_state = ACPI_STATE_S0;
-	acpi_sleep_finish_wake_up = false;
 }
 
 static void acpi_hibernation_end(void)
 {
 	/*
 	 * This is necessary in case acpi_hibernation_finish() is not called
-	 * directly during a failing transition to the sleep state.
+	 * during a failing transition to the sleep state.
 	 */
-	if (acpi_sleep_finish_wake_up)
-		acpi_hibernation_finish();
+	acpi_target_sleep_state = ACPI_STATE_S0;
 }
 
 static int acpi_hibernation_pre_restore(void)
Index: linux-2.6/Documentation/kernel-parameters.txt
===================================================================
--- linux-2.6.orig/Documentation/kernel-parameters.txt
+++ linux-2.6/Documentation/kernel-parameters.txt
@@ -170,11 +170,6 @@ and is between 256 and 4096 characters. 
 	acpi_irq_isa=	[HW,ACPI] If irq_balance, mark listed IRQs used by ISA
 			Format: <irq>,<irq>...
 
-	acpi_new_pts_ordering [HW,ACPI]
-			Enforce the ACPI 2.0 ordering of the _PTS control
-			method wrt putting devices into low power states
-			default: pre ACPI 2.0 ordering of _PTS
-
 	acpi_no_auto_ssdt	[HW,ACPI] Disable automatic loading of SSDT
 
 	acpi_os_name=	[HW,ACPI] Tell ACPI BIOS the name of the OS

Re: [PATCH] ACPI PM: Restore the 2.6.24 suspend ordering

[Pavel Machek]

Hi!

> Please consider pushing the appended patch for 2.6.25.
> 
> It fixed the regression described at:
> https://bugzilla.novell.com/show_bug.cgi?id=374217
> http://bugzilla.kernel.org/show_bug.cgi?id=10340
> 
> details in the changelog.

> 
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Some time ago it turned out that our suspend code ordering broke
> some NVidia-based systems that hung if _PTS was executed with one of
> the PCI devices, specifically a USB controller, in a low power state.
> Then, it was noticed that the suspend code ordering was not compliant
> with ACPI 1.0, although it was compliant with ACPI 2.0 (and later),
> and it was argued that the code had to be changed for that reason
> (ref. http://bugzilla.kernel.org/show_bug.cgi?id=9528).  So we did,
> but evidently we did wrong, because it's now turning out that some
> systems have been broken by this change (refs.
> http://bugzilla.kernel.org/show_bug.cgi?id=10340 ,
> https://bugzilla.novell.com/show_bug.cgi?id=374217#c16).  [I said
> at that time that something like this might happend, but the majority
> of people involved thought that it was improbable due to the
> necessity to preserve the compliance of hardware with ACPI 1.0.]
> This actually is a quite serious regression from 2.6.24.
> 
> Moreover, the ACPI 1.0 ordering of suspend code introduced another
> issue that I have only noticed recently.  Namely, if the suspend of
> one of devices fails, the already suspended devices will be resumed
> without executing _WAK before, which leads to problems on some
> systems (for example, in such situations thermal management is
> broken on my HP nx6325).  Consequently, it also breaks suspend
> debugging on the affected systems.
> 
> Note also, that the requirement to execute _PTS before suspending
> devices does not really make sense, because the device in question
> may be put into a low power state at run time for a reason unrelated
> to a system-wide suspend.
> 
> For the reasons outlined above, the change of the suspend ordering
> should be reverted, which is done by the patch below.

But this will break those few nvidia-based systems, no?

this may have been a good idea in -rc1 days, but we are in -rc7
now... and the patch is slightly big.

What about something like: (hand-edited patch, sorry)



 Index: linux-2.6/drivers/acpi/sleep/main.c
 ===================================================================
 --- linux-2.6.orig/drivers/acpi/sleep/main.c
 +++ linux-2.6/drivers/acpi/sleep/main.c
 @@ -26,21 +26,6 @@ u8 sleep_states[ACPI_S_STATE_COUNT];
  
  #ifdef CONFIG_PM_SLEEP
  static u32 acpi_target_sleep_state = ACPI_STATE_S0;
 static bool acpi_sleep_finish_wake_up;
 
- /*
-  * ACPI 2.0 and later want us to execute _PTS after suspending devices, so we
-  * allow the user to request that behavior by using the 'acpi_new_pts_ordering'
-  * kernel command line option that causes the following variable to be set.
-  */
 static bool new_pts_ordering = true;
 
 -static int __init acpi_new_pts_ordering(char *str)
 +static int __init acpi_old_pts_ordering(char *str)
 {
 	new_pts_ordering = false;
 	return 1;
 }
 -__setup("acpi_old_pts_ordering", acpi_old_pts_ordering);
 +__setup("acpi_new_pts_ordering", acpi_new_pts_ordering);
  #endif
 
  static int acpi_sleep_prepare(u32 acpi_state)
 Index: linux-2.6/Documentation/kernel-parameters.txt
 ===================================================================
 --- linux-2.6.orig/Documentation/kernel-parameters.txt
 +++ linux-2.6/Documentation/kernel-parameters.txt
 @@ -170,11 +170,6 @@ and is between 256 and 4096 characters. 
  	acpi_irq_isa=	[HW,ACPI] If irq_balance, mark listed IRQs used by ISA
  			Format: <irq>,<irq>...
  
 -	acpi_new_pts_ordering [HW,ACPI]
 +	acpi_old_pts_ordering [HW,ACPI]
 -			Enforce the ACPI 2.0 ordering of the _PTS control
 +			Enforce the ACPI 1.0 ordering of the _PTS control
 			method wrt putting devices into low power states
 -			default: pre ACPI 2.0 ordering of _PTS
 +			default: ACPI 2.0 ordering of _PTS
 
  	acpi_no_auto_ssdt	[HW,ACPI] Disable automatic loading of SSDT
  
  	acpi_os_name=	[HW,ACPI] Tell ACPI BIOS the name of the OS

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

Re: [PATCH] ACPI PM: Restore the 2.6.24 suspend ordering

[Rafael J. Wysocki]

On Sunday, 30 of March 2008, Pavel Machek wrote:
> Hi!

Hi,

> > Please consider pushing the appended patch for 2.6.25.
> > 
> > It fixed the regression described at:
> > https://bugzilla.novell.com/show_bug.cgi?id=374217
> > http://bugzilla.kernel.org/show_bug.cgi?id=10340
> > 
> > details in the changelog.
> 
> > 
> > ---
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Some time ago it turned out that our suspend code ordering broke
> > some NVidia-based systems that hung if _PTS was executed with one of
> > the PCI devices, specifically a USB controller, in a low power state.
> > Then, it was noticed that the suspend code ordering was not compliant
> > with ACPI 1.0, although it was compliant with ACPI 2.0 (and later),
> > and it was argued that the code had to be changed for that reason
> > (ref. http://bugzilla.kernel.org/show_bug.cgi?id=9528).  So we did,
> > but evidently we did wrong, because it's now turning out that some
> > systems have been broken by this change (refs.
> > http://bugzilla.kernel.org/show_bug.cgi?id=10340 ,
> > https://bugzilla.novell.com/show_bug.cgi?id=374217#c16).  [I said
> > at that time that something like this might happend, but the majority
> > of people involved thought that it was improbable due to the
> > necessity to preserve the compliance of hardware with ACPI 1.0.]
> > This actually is a quite serious regression from 2.6.24.
> > 
> > Moreover, the ACPI 1.0 ordering of suspend code introduced another
> > issue that I have only noticed recently.  Namely, if the suspend of
> > one of devices fails, the already suspended devices will be resumed
> > without executing _WAK before, which leads to problems on some
> > systems (for example, in such situations thermal management is
> > broken on my HP nx6325).  Consequently, it also breaks suspend
> > debugging on the affected systems.
> > 
> > Note also, that the requirement to execute _PTS before suspending
> > devices does not really make sense, because the device in question
> > may be put into a low power state at run time for a reason unrelated
> > to a system-wide suspend.
> > 
> > For the reasons outlined above, the change of the suspend ordering
> > should be reverted, which is done by the patch below.
> 
> But this will break those few nvidia-based systems, no?
> 
> this may have been a good idea in -rc1 days, but we are in -rc7
> now... and the patch is slightly big.

It's quite obvious, though.

> What about something like: (hand-edited patch, sorry)

Well, I think that would be confusing.

The NVidia systems are broken anyway on 2.6.24.x, so we just don't fix them
rather than break them and there are more reasons to do what the patch does
(as pointed out in the changelog).  For example, your suggested patch doesn't
 fix the error paths/debugging breakage described in the changelog.

I think we _can_ do something about the failing NVidia systems in the 2.6.26
time frame, but that will require some more consideration.

>  Index: linux-2.6/drivers/acpi/sleep/main.c
>  ===================================================================
>  --- linux-2.6.orig/drivers/acpi/sleep/main.c
>  +++ linux-2.6/drivers/acpi/sleep/main.c
>  @@ -26,21 +26,6 @@ u8 sleep_states[ACPI_S_STATE_COUNT];
>   
>   #ifdef CONFIG_PM_SLEEP
>   static u32 acpi_target_sleep_state = ACPI_STATE_S0;
>  static bool acpi_sleep_finish_wake_up;
>  
> - /*
> -  * ACPI 2.0 and later want us to execute _PTS after suspending devices, so we
> -  * allow the user to request that behavior by using the 'acpi_new_pts_ordering'
> -  * kernel command line option that causes the following variable to be set.
> -  */
>  static bool new_pts_ordering = true;
>  
>  -static int __init acpi_new_pts_ordering(char *str)
>  +static int __init acpi_old_pts_ordering(char *str)
>  {
>  	new_pts_ordering = false;
>  	return 1;
>  }
>  -__setup("acpi_old_pts_ordering", acpi_old_pts_ordering);
>  +__setup("acpi_new_pts_ordering", acpi_new_pts_ordering);
>   #endif
>  
>   static int acpi_sleep_prepare(u32 acpi_state)
>  Index: linux-2.6/Documentation/kernel-parameters.txt
>  ===================================================================
>  --- linux-2.6.orig/Documentation/kernel-parameters.txt
>  +++ linux-2.6/Documentation/kernel-parameters.txt
>  @@ -170,11 +170,6 @@ and is between 256 and 4096 characters. 
>   	acpi_irq_isa=	[HW,ACPI] If irq_balance, mark listed IRQs used by ISA
>   			Format: <irq>,<irq>...
>   
>  -	acpi_new_pts_ordering [HW,ACPI]
>  +	acpi_old_pts_ordering [HW,ACPI]
>  -			Enforce the ACPI 2.0 ordering of the _PTS control
>  +			Enforce the ACPI 1.0 ordering of the _PTS control
>  			method wrt putting devices into low power states
>  -			default: pre ACPI 2.0 ordering of _PTS
>  +			default: ACPI 2.0 ordering of _PTS
>  
>   	acpi_no_auto_ssdt	[HW,ACPI] Disable automatic loading of SSDT
>   
>   	acpi_os_name=	[HW,ACPI] Tell ACPI BIOS the name of the OS
> 

Thanks,
Rafael

Re: [PATCH] ACPI PM: Restore the 2.6.24 suspend ordering

[Pavel Machek]

Hi!

> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > Some time ago it turned out that our suspend code ordering broke
> > > some NVidia-based systems that hung if _PTS was executed with one of
> > > the PCI devices, specifically a USB controller, in a low power state.
> > > Then, it was noticed that the suspend code ordering was not compliant
> > > with ACPI 1.0, although it was compliant with ACPI 2.0 (and later),
> > > and it was argued that the code had to be changed for that reason
> > > (ref. http://bugzilla.kernel.org/show_bug.cgi?id=9528).  So we did,
> > > but evidently we did wrong, because it's now turning out that some
> > > systems have been broken by this change (refs.
> > > http://bugzilla.kernel.org/show_bug.cgi?id=10340 ,
> > > https://bugzilla.novell.com/show_bug.cgi?id=374217#c16).  [I said
> > > at that time that something like this might happend, but the majority
> > > of people involved thought that it was improbable due to the
> > > necessity to preserve the compliance of hardware with ACPI 1.0.]
> > > This actually is a quite serious regression from 2.6.24.
> > > 
> > > Moreover, the ACPI 1.0 ordering of suspend code introduced another
> > > issue that I have only noticed recently.  Namely, if the suspend of
> > > one of devices fails, the already suspended devices will be resumed
> > > without executing _WAK before, which leads to problems on some
> > > systems (for example, in such situations thermal management is
> > > broken on my HP nx6325).  Consequently, it also breaks suspend
> > > debugging on the affected systems.
> > > 
> > > Note also, that the requirement to execute _PTS before suspending
> > > devices does not really make sense, because the device in question
> > > may be put into a low power state at run time for a reason unrelated
> > > to a system-wide suspend.

Yes, but if we are putting them into lowpower state ourselves, we
should probably be doing that "by hand", without calling acpi
methods. _PTS may prepare something for acpi methods (which tell us
which PCI Dx state to put the device in at the very least).

> > > For the reasons outlined above, the change of the suspend ordering
> > > should be reverted, which is done by the patch below.
> > 
> > But this will break those few nvidia-based systems, no?
> > 
> > this may have been a good idea in -rc1 days, but we are in -rc7
> > now... and the patch is slightly big.
> 
> It's quite obvious, though.

Yes, but breaking systems between -rc7 and final is _very_ unnice.

> > What about something like: (hand-edited patch, sorry)
> 
> Well, I think that would be confusing.
> 
> The NVidia systems are broken anyway on 2.6.24.x, so we just don't fix them
> rather than break them and there are more reasons to do what the patch does
> (as pointed out in the changelog).  For example, your suggested patch doesn't
>  fix the error paths/debugging breakage described in the changelog.

But that should not be impossible to fix, right?

> I think we _can_ do something about the failing NVidia systems in the 2.6.26
> time frame, but that will require some more consideration.

We could simply blacklist them, no?
								Pavel

> >  Index: linux-2.6/drivers/acpi/sleep/main.c
> >  ===================================================================
> >  --- linux-2.6.orig/drivers/acpi/sleep/main.c
> >  +++ linux-2.6/drivers/acpi/sleep/main.c
> >  @@ -26,21 +26,6 @@ u8 sleep_states[ACPI_S_STATE_COUNT];
> >   
> >   #ifdef CONFIG_PM_SLEEP
> >   static u32 acpi_target_sleep_state = ACPI_STATE_S0;
> >  static bool acpi_sleep_finish_wake_up;
> >  
> > - /*
> > -  * ACPI 2.0 and later want us to execute _PTS after suspending devices, so we
> > -  * allow the user to request that behavior by using the 'acpi_new_pts_ordering'
> > -  * kernel command line option that causes the following variable to be set.
> > -  */
> >  static bool new_pts_ordering = true;
> >  
> >  -static int __init acpi_new_pts_ordering(char *str)
> >  +static int __init acpi_old_pts_ordering(char *str)
> >  {
> >  	new_pts_ordering = false;
> >  	return 1;
> >  }
> >  -__setup("acpi_old_pts_ordering", acpi_old_pts_ordering);
> >  +__setup("acpi_new_pts_ordering", acpi_new_pts_ordering);
> >   #endif
> >  
> >   static int acpi_sleep_prepare(u32 acpi_state)
> >  Index: linux-2.6/Documentation/kernel-parameters.txt
> >  ===================================================================
> >  --- linux-2.6.orig/Documentation/kernel-parameters.txt
> >  +++ linux-2.6/Documentation/kernel-parameters.txt
> >  @@ -170,11 +170,6 @@ and is between 256 and 4096 characters. 
> >   	acpi_irq_isa=	[HW,ACPI] If irq_balance, mark listed IRQs used by ISA
> >   			Format: <irq>,<irq>...
> >   
> >  -	acpi_new_pts_ordering [HW,ACPI]
> >  +	acpi_old_pts_ordering [HW,ACPI]
> >  -			Enforce the ACPI 2.0 ordering of the _PTS control
> >  +			Enforce the ACPI 1.0 ordering of the _PTS control
> >  			method wrt putting devices into low power states
> >  -			default: pre ACPI 2.0 ordering of _PTS
> >  +			default: ACPI 2.0 ordering of _PTS
> >  
> >   	acpi_no_auto_ssdt	[HW,ACPI] Disable automatic loading of SSDT
> >   
> >   	acpi_os_name=	[HW,ACPI] Tell ACPI BIOS the name of the OS
> > 
> 
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

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

Re: [PATCH] ACPI PM: Restore the 2.6.24 suspend ordering

[Rafael J. Wysocki]

On Sunday, 30 of March 2008, Pavel Machek wrote:
> Hi!

Hi,

> > > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > > 
> > > > Some time ago it turned out that our suspend code ordering broke
> > > > some NVidia-based systems that hung if _PTS was executed with one of
> > > > the PCI devices, specifically a USB controller, in a low power state.
> > > > Then, it was noticed that the suspend code ordering was not compliant
> > > > with ACPI 1.0, although it was compliant with ACPI 2.0 (and later),
> > > > and it was argued that the code had to be changed for that reason
> > > > (ref. http://bugzilla.kernel.org/show_bug.cgi?id=9528).  So we did,
> > > > but evidently we did wrong, because it's now turning out that some
> > > > systems have been broken by this change (refs.
> > > > http://bugzilla.kernel.org/show_bug.cgi?id=10340 ,
> > > > https://bugzilla.novell.com/show_bug.cgi?id=374217#c16).  [I said
> > > > at that time that something like this might happend, but the majority
> > > > of people involved thought that it was improbable due to the
> > > > necessity to preserve the compliance of hardware with ACPI 1.0.]
> > > > This actually is a quite serious regression from 2.6.24.
> > > > 
> > > > Moreover, the ACPI 1.0 ordering of suspend code introduced another
> > > > issue that I have only noticed recently.  Namely, if the suspend of
> > > > one of devices fails, the already suspended devices will be resumed
> > > > without executing _WAK before, which leads to problems on some
> > > > systems (for example, in such situations thermal management is
> > > > broken on my HP nx6325).  Consequently, it also breaks suspend
> > > > debugging on the affected systems.
> > > > 
> > > > Note also, that the requirement to execute _PTS before suspending
> > > > devices does not really make sense, because the device in question
> > > > may be put into a low power state at run time for a reason unrelated
> > > > to a system-wide suspend.
> 
> Yes, but if we are putting them into lowpower state ourselves, we
> should probably be doing that "by hand", without calling acpi
> methods. _PTS may prepare something for acpi methods (which tell us
> which PCI Dx state to put the device in at the very least).

I meant "the requirement to execute _PTS before suspending devices, because
it would hang otherwise".

> > > > For the reasons outlined above, the change of the suspend ordering
> > > > should be reverted, which is done by the patch below.
> > > 
> > > But this will break those few nvidia-based systems, no?
> > > 
> > > this may have been a good idea in -rc1 days, but we are in -rc7
> > > now... and the patch is slightly big.
> > 
> > It's quite obvious, though.
> 
> Yes, but breaking systems between -rc7 and final is _very_ unnice.

Breaking systems between 2.6.24 and 2.6.25 is even worse, which is why
I've posted this patch.

IOW, we tried to fix systems that were broken with 2.6.24, but it didn't work,
because our "fix" broke systems that were OK with 2.6.24.  Solution: revert
the "fix" and go back to the design board.  That's all we can do so late in
the release cycle, IMO.

> > > What about something like: (hand-edited patch, sorry)
> > 
> > Well, I think that would be confusing.
> > 
> > The NVidia systems are broken anyway on 2.6.24.x, so we just don't fix them
> > rather than break them and there are more reasons to do what the patch does
> > (as pointed out in the changelog).  For example, your suggested patch doesn't
> >  fix the error paths/debugging breakage described in the changelog.
> 
> But that should not be impossible to fix, right?

No, it shouldn't, but it would be more complicated than it seemed to be.

> > I think we _can_ do something about the failing NVidia systems in the 2.6.26
> > time frame, but that will require some more consideration.
> 
> We could simply blacklist them, no?

Yes, but for this purpose we'll have to redesign the core so that everything
(including debugging and the error paths) works if _PTS is executed before
suspending devices.  _That_, however, is not a 2.6.25 thing.

Thanks,
Rafael

Re: [PATCH] ACPI PM: Restore the 2.6.24 suspend ordering

[Pavel Machek]

Hi!

> > > > > For the reasons outlined above, the change of the suspend ordering
> > > > > should be reverted, which is done by the patch below.
> > > > 
> > > > But this will break those few nvidia-based systems, no?
> > > > 
> > > > this may have been a good idea in -rc1 days, but we are in -rc7
> > > > now... and the patch is slightly big.
> > > 
> > > It's quite obvious, though.
> > 
> > Yes, but breaking systems between -rc7 and final is _very_ unnice.
> 
> Breaking systems between 2.6.24 and 2.6.25 is even worse, which is why
> I've posted this patch.
> 
> IOW, we tried to fix systems that were broken with 2.6.24, but it didn't work,
> because our "fix" broke systems that were OK with 2.6.24.  Solution: revert
> the "fix" and go back to the design board.  That's all we can do so late in
> the release cycle, IMO.

Well, I agree that regression from 2.6.24 is worse, but it is
_slightly_ worse... -rcs are really expected to improve...

...plus it no longer looks like macbook regression is caused by _PTS
ordering?

> > > I think we _can_ do something about the failing NVidia systems in the 2.6.26
> > > time frame, but that will require some more consideration.
> > 
> > We could simply blacklist them, no?
> 
> Yes, but for this purpose we'll have to redesign the core so that everything
> (including debugging and the error paths) works if _PTS is executed before
> suspending devices.  _That_, however, is not a 2.6.25 thing.

So we have solution that fixes 2.6.24 systems, makes system that
worked in 2.6.25-rc5 work with command line option, but gets error
handling wrong.

I guess we could use that?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] ACPI PM: Restore the 2.6.24 suspend ordering

[Felix Möller]

Hi,
>>>>>> For the reasons outlined above, the change of the suspend ordering
>>>>>> should be reverted, which is done by the patch below.
>>>>> But this will break those few nvidia-based systems, no?
>>>>>
>>>>> this may have been a good idea in -rc1 days, but we are in -rc7
>>>>> now... and the patch is slightly big.
>>>> It's quite obvious, though.
>>> Yes, but breaking systems between -rc7 and final is _very_ unnice.
>> Breaking systems between 2.6.24 and 2.6.25 is even worse, which is why
>> I've posted this patch.
>>
>> IOW, we tried to fix systems that were broken with 2.6.24, but it didn't work,
>> because our "fix" broke systems that were OK with 2.6.24.  Solution: revert
>> the "fix" and go back to the design board.  That's all we can do so late in
>> the release cycle, IMO.
> 
> Well, I agree that regression from 2.6.24 is worse, but it is
> _slightly_ worse... -rcs are really expected to improve...
> 
> ...plus it no longer looks like macbook regression is caused by _PTS
> ordering?
I am the reporter from the original Novell Bug: 
https://bugzilla.novell.com/show_bug.cgi?id=374217

I just tried current git head (two hours ago) with the patch (the one 
from the beginning of this thread) from Rafael and without it. With the 
patch my MacBook does suspend without it does not.

HTH
Felix Möller

Re: [PATCH] ACPI PM: Restore the 2.6.24 suspend ordering

[Rafael J. Wysocki]

On Tuesday, 1 of April 2008, Pavel Machek wrote:
> Hi!

Hi,

> > > > > > For the reasons outlined above, the change of the suspend ordering
> > > > > > should be reverted, which is done by the patch below.
> > > > > 
> > > > > But this will break those few nvidia-based systems, no?
> > > > > 
> > > > > this may have been a good idea in -rc1 days, but we are in -rc7
> > > > > now... and the patch is slightly big.
> > > > 
> > > > It's quite obvious, though.
> > > 
> > > Yes, but breaking systems between -rc7 and final is _very_ unnice.
> > 
> > Breaking systems between 2.6.24 and 2.6.25 is even worse, which is why
> > I've posted this patch.
> > 
> > IOW, we tried to fix systems that were broken with 2.6.24, but it didn't work,
> > because our "fix" broke systems that were OK with 2.6.24.  Solution: revert
> > the "fix" and go back to the design board.  That's all we can do so late in
> > the release cycle, IMO.
> 
> Well, I agree that regression from 2.6.24 is worse, but it is
> _slightly_ worse... -rcs are really expected to improve...
> 
> ...plus it no longer looks like macbook regression is caused by _PTS
> ordering?
> 
> > > > I think we _can_ do something about the failing NVidia systems in the 2.6.26
> > > > time frame, but that will require some more consideration.
> > > 
> > > We could simply blacklist them, no?
> > 
> > Yes, but for this purpose we'll have to redesign the core so that everything
> > (including debugging and the error paths) works if _PTS is executed before
> > suspending devices.  _That_, however, is not a 2.6.25 thing.
> 
> So we have solution that fixes 2.6.24 systems, makes system that
> worked in 2.6.25-rc5 work with command line option, but gets error
> handling wrong.
> 
> I guess we could use that?

IMO we should not use that, because it's broken.  That's why I posted the
patch.

Thanks,
Rafael

Re: [PATCH] ACPI PM: Restore the 2.6.24 suspend ordering

[Pavel Machek]

Hi!

> > > > We could simply blacklist them, no?
> > > 
> > > Yes, but for this purpose we'll have to redesign the core so that everything
> > > (including debugging and the error paths) works if _PTS is executed before
> > > suspending devices.  _That_, however, is not a 2.6.25 thing.
> > 
> > So we have solution that fixes 2.6.24 systems, makes system that
> > worked in 2.6.25-rc5 work with command line option, but gets error
> > handling wrong.
> > 
> > I guess we could use that?
> 
> IMO we should not use that, because it's broken.  That's why I posted the
> patch.

I understand that. World will not fall down one way or another, but
I'd slightly prefer the 'only change default' version. That's why I
objected to your patch :-).
							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html