Message-Id: <200803301358.39831.rjw@sisk.pl>
Date:	Sun, 30 Mar 2008 13:58:38 +0200
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Pavel Machek <pavel@...e.cz>
Cc:	Len Brown <lenb@...nel.org>,
	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Carlos Corbacho <carlos@...angeworlds.co.uk>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	pm list <linux-pm@...ts.linux-foundation.org>,
	Shaohua Li <shaohua.li@...el.com>,
	Felix Möller <fm@...nsuse.org>,
	Arthur Erhardt <erhardt@....physik.uni-tuebingen.de>,
	Matthew Garrett <mjg59@...f.ucam.org>
Subject: Re: [PATCH] ACPI PM: Restore the 2.6.24 suspend ordering

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@...k.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists