Re: ACPI: _PTS ordering needs fixing for pre ACPI 3.0 systems (was: Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM)

Rafael J. Wysocki
2007-12-25T13:17:41Z

On Tuesday, 25 of December 2007, Carlos Corbacho wrote:
> Adding Linux-ACPI to CC.
>
> On Tuesday 25 December 2007 00:03:25 Carlos Corbacho wrote:
> > According to the earlier versions of the ACPI spec, Linux is doing the
> > wrong thing - we should call _PTS() before we start powerding down devices,
> > or notifying device drivers to start suspending.
> >
> > So, my limited understanding of what we currently do for ACPI
> > suspend-to-RAM is:
> >
> > 1) Freeze processes/ devices
> > 2) Put all devices into low power mode
> > 3) Execute _PTS()
> > 4) Suspend system
> >
> > So the problem is - our current suspend order is fine for ACPI 3.0 and
> > above, but for pre-3.0 systems, this violates the older specs, where 2) and
> > 3) should be reversed.
>
> The following is a hack to illustrate what I'm getting at (this is
> tested on x86-64) (it's a hack since it does all the ACPI prepare bits
> during set_target() for the pre ACPI 3.0 systems, rather than prepare() -
> whether this can be cleaned up to move out just the _PTS() call, I don't
> know).
>
> It abuses suspend_ops->set_target(), but was the easiest way to quickly
> demonstrate this (since the kerneldoc for set_target() says it will always
> be executed before we suspend the devices).

Please, don't do that.

The current code is following the ACPI 2.0 specification (and later) quite
closely and while we can add a special case for the 1.0-copmpilant systems,
the failing ones tend to be supposed to follow ACPI 2.0 (or later).

> drivers/acpi/sleep/main.c | 26 ++++++++++++++++++++++----
> 1 files changed, 22 insertions(+), 4 deletions(-)
>
>
> diff --git a/drivers/acpi/sleep/main.c b/drivers/acpi/sleep/main.c
> index 96d23b3..89e708b 100644
> --- a/drivers/acpi/sleep/main.c
> +++ b/drivers/acpi/sleep/main.c
> @@ -77,8 +77,19 @@ static int acpi_pm_set_target(suspend_state_t pm_state)
> } else {
> printk(KERN_ERR "ACPI does not support this state: %d\n",
> pm_state);
> - error = -ENOSYS;
> + return -ENOSYS;
> }
> +
> + /*
> + * For ACPI 1.0 and 2.0 systems, we must run the preparation methods
> + * before we put the devices into low power mode.
> + */
> + if (acpi_gbl_FADT.header.revision < 3) {

acpi_gbl_FADT.header.revision is equal to 3 for ACPI 2.0-compilant systems
(section 5.2.8 of the specification).

> + error = acpi_sleep_prepare(acpi_target_sleep_state);
> + if (error)
> + acpi_target_sleep_state = ACPI_STATE_S0;
> + }
> +
> return error;
> }
>
> @@ -91,10 +102,17 @@ static int acpi_pm_set_target(suspend_state_t pm_state)
>
> static int acpi_pm_prepare(void)
> {
> - int error = acpi_sleep_prepare(acpi_target_sleep_state);
> + int error = 0;
>
> - if (error)
> - acpi_target_sleep_state = ACPI_STATE_S0;
> + /*
> + * For ACPI 3.0 or newer systems, we must run the preparation methods
> + * after we put the devices into low power mode.
> + */
> + if (acpi_gbl_FADT.header.revision >= 3) {

Same here (so the comment is wrong).

> + error = acpi_sleep_prepare(acpi_target_sleep_state);
> + if (error)
> + acpi_target_sleep_state = ACPI_STATE_S0;
> + }
>
> return error;
> }

Thanks,
Rafael