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)

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)

[Carlos Corbacho]

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).

-Carlos
---

 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) {
+		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) {
+		error = acpi_sleep_prepare(acpi_target_sleep_state);
+		if (error)
+			acpi_target_sleep_state = ACPI_STATE_S0;
+	}
 
 	return error;
 }

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]

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

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)

[Carlos Corbacho]

On Tuesday 25 December 2007 14:07:22 Rafael J. Wysocki wrote:
> OK, sorry, the approach is generally reasonable, IMO, but it needs to be a
> bit more fine grained.

I know, hence this was marked as a hack and not signed off; it's just a 
demonstration of the general idea with code instead of words.

> I'll try to prepare some patches along these lines soon.

Much appreciated - I'm much happier with someone who's more familiar with this 
code than I working on it.

-Carlos

(Now going back to unwrapping Christmas presents...)
-- 
E-Mail: carlos@strangeworlds.co.uk
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

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]

On Tuesday, 25 of December 2007, Rafael J. Wysocki wrote:
> 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.

OK, sorry, the approach is generally reasonable, IMO, but it needs to be a bit
more fine grained.

I'll try to prepare some patches along these lines soon.

Thanks,
Rafael

Re: Suspend code ordering (again)

[Robert Hancock]

Rafael J. Wysocki wrote:
> On Wednesday, 26 of December 2007, Linus Torvalds wrote:
>> On Tue, 25 Dec 2007, Rafael J. Wysocki wrote:
>>> the ACPI specification between versions 1.0x and 2.0.  Namely, while ACPI
>>> 2.0 and later wants us to put devices into low power states before calling
>>> _PTS, ACPI 1.0x wants us to do that after calling _PTS.  Since we're following
>>> the 2.0 and later specifications right now, we're not doing the right thing for
>>> the (strictly) ACPI 1.0x-compliant systems.
>>>
>>> We ought to be able to fix things on the high level, by calling _PTS earlier on
>>> systems that claim to be ACPI 1.0x-compliant.  That will require us to modify
>>> the generic susped code quite a bit and will need to be tested for some time.
>> That's insane. Are you really saying that ACPI wants totally different 
>> orderings for different versions of the spec?
> 
> Yes, I am.
> 
>> And does Windows really do that?
> 
> I don't know.
> 
>> Please don't make lots of modifications to the generic suspend code. The 
>> only thing that is worth doing is to just have a firmware callback before 
>> the "device_suspend()" thing (and then on a ACPI-1.0 system, call _PTS 
>> *there*), and on an ACPI-2.0 system, call _PTS *after* device_suspend().
> 
> Yes, that's what I'm going to do, but I need to untangle some ACPI code for
> this purpose.
> 
>> Still, the fact is, some (most, I think) drivers *should* put themselves 
>> into D3 only in "late_suspend()", so if ACPI-2.0 really expects _PTS to be 
>> called after that, we're just screwed.
> 
> Well, section 9.1.6 of ACPI 2.0 specifies the suspend ordering directly and
> says exactly that _PTS is to be executed after putting devices into respective
> D states.

I would not take those sections as gospel, they're really an example 
only. It's quite possible that Windows does not follow that ordering.

Also, as was pointed out, pre-Vista versions of Windows follow ACPI 1.0 
and Vista follows 3.0, so 2.0 doesn't really matter since BIOS people 
won't test against it. 1.0 specifies that _PTS is to be called before 
suspending devices and 3.0 says that the AML must not depend on any 
specific device power state, so in both cases it should be safe to call 
_PTS before suspending, no?

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: Suspend code ordering (again)

[Rafael J. Wysocki]

On Thursday, 27 of December 2007, Robert Hancock wrote:
> Rafael J. Wysocki wrote:
> > On Wednesday, 26 of December 2007, Linus Torvalds wrote:
> >> On Tue, 25 Dec 2007, Rafael J. Wysocki wrote:
> >>> the ACPI specification between versions 1.0x and 2.0.  Namely, while ACPI
> >>> 2.0 and later wants us to put devices into low power states before calling
> >>> _PTS, ACPI 1.0x wants us to do that after calling _PTS.  Since we're following
> >>> the 2.0 and later specifications right now, we're not doing the right thing for
> >>> the (strictly) ACPI 1.0x-compliant systems.
> >>>
> >>> We ought to be able to fix things on the high level, by calling _PTS earlier on
> >>> systems that claim to be ACPI 1.0x-compliant.  That will require us to modify
> >>> the generic susped code quite a bit and will need to be tested for some time.
> >> That's insane. Are you really saying that ACPI wants totally different 
> >> orderings for different versions of the spec?
> > 
> > Yes, I am.
> > 
> >> And does Windows really do that?
> > 
> > I don't know.
> > 
> >> Please don't make lots of modifications to the generic suspend code. The 
> >> only thing that is worth doing is to just have a firmware callback before 
> >> the "device_suspend()" thing (and then on a ACPI-1.0 system, call _PTS 
> >> *there*), and on an ACPI-2.0 system, call _PTS *after* device_suspend().
> > 
> > Yes, that's what I'm going to do, but I need to untangle some ACPI code for
> > this purpose.
> > 
> >> Still, the fact is, some (most, I think) drivers *should* put themselves 
> >> into D3 only in "late_suspend()", so if ACPI-2.0 really expects _PTS to be 
> >> called after that, we're just screwed.
> > 
> > Well, section 9.1.6 of ACPI 2.0 specifies the suspend ordering directly and
> > says exactly that _PTS is to be executed after putting devices into respective
> > D states.
> 
> I would not take those sections as gospel, they're really an example 
> only. It's quite possible that Windows does not follow that ordering.
> 
> Also, as was pointed out, pre-Vista versions of Windows follow ACPI 1.0 
> and Vista follows 3.0, so 2.0 doesn't really matter since BIOS people 
> won't test against it. 1.0 specifies that _PTS is to be called before 
> suspending devices and 3.0 says that the AML must not depend on any 
> specific device power state, so in both cases it should be safe to call 
> _PTS before suspending, no?

Well, IMO, if we take one option only (whichever that is) and there are systems
that follow the other one, they will likely break.

Apart from this, there are BIOSes that openly claim ACPI 2.0 support (for
example, the one in my HP nx6325 does that) and they may actually prefer the
post-ACPI-1.0 ordering even if they work with the pre-ACPI-2.0 one.

Greetings,
Rafael

Re: Suspend code ordering (again)

[Robert Hancock]

Rafael J. Wysocki wrote:
>> Also, as was pointed out, pre-Vista versions of Windows follow ACPI 1.0 
>> and Vista follows 3.0, so 2.0 doesn't really matter since BIOS people 
>> won't test against it. 1.0 specifies that _PTS is to be called before 
>> suspending devices and 3.0 says that the AML must not depend on any 
>> specific device power state, so in both cases it should be safe to call 
>> _PTS before suspending, no?
> 
> Well, IMO, if we take one option only (whichever that is) and there are systems
> that follow the other one, they will likely break.
> 
> Apart from this, there are BIOSes that openly claim ACPI 2.0 support (for
> example, the one in my HP nx6325 does that) and they may actually prefer the
> post-ACPI-1.0 ordering even if they work with the pre-ACPI-2.0 one.

I doubt they would prefer the later ordering in any way that matters, if 
the Windows version they were designed for uses the earlier ordering.

It would be best if somebody could manage to find out what ordering 
Windows XP (and Windows Vista, for good measure) actually use, then we 
could just use that. Virtual machine trickery might be an option - the 
only complication being that it'll be using the DSDT for the fake 
machine and not the real one..

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: Suspend code ordering (again)

[Linus Torvalds]

On Thu, 27 Dec 2007, Robert Hancock wrote:
> 
> I doubt they would prefer the later ordering in any way that matters, if the
> Windows version they were designed for uses the earlier ordering.

Well, I wouldn't say it's abotu "preferring" one over the other. It's very 
possible that the BIOS writers were *intending* to prefer ACPI 2.0, and it 
may even be likely that they thought that they wrote it that way, but the 
real issue is that it has apparently never ever been *tested* that way.

So yes, maybe the vendors actually thought they were a good ACPI-2.0 
implementation, but if Windows doesn't do the ordering that the 2.0 spec 
expects, then that is pretty much just a theoretical thing.

But yeah, it would be really nice to have this verified some way. Somebody 
must already know (whether it's a VM person or a BIOS writer, and whether 
they'd tell us, is obviously another issue).

		Linus

Re: Suspend code ordering (again)

[Shaohua Li]

[-- Attachment #1: Type: text/plain, Size: 2169 bytes --]


On Fri, 2007-12-28 at 08:25 +0800, Robert Hancock wrote:
> Rafael J. Wysocki wrote: 
> >> Also, as was pointed out, pre-Vista versions of Windows follow ACPI
> 1.0 
> >> and Vista follows 3.0, so 2.0 doesn't really matter since BIOS
> people 
> >> won't test against it. 1.0 specifies that _PTS is to be called
> before 
> >> suspending devices and 3.0 says that the AML must not depend on
> any 
> >> specific device power state, so in both cases it should be safe to
> call 
> >> _PTS before suspending, no? 
> > 
> > Well, IMO, if we take one option only (whichever that is) and there
> are systems 
> > that follow the other one, they will likely break. 
> > 
> > Apart from this, there are BIOSes that openly claim ACPI 2.0 support
> (for 
> > example, the one in my HP nx6325 does that) and they may actually
> prefer the 
> > post-ACPI-1.0 ordering even if they work with the pre-ACPI-2.0 one.
> 
> I doubt they would prefer the later ordering in any way that matters,
> if 
> the Windows version they were designed for uses the earlier ordering.
> 
> It would be best if somebody could manage to find out what ordering 
> Windows XP (and Windows Vista, for good measure) actually use, then
> we 
> could just use that. Virtual machine trickery might be an option -
> the 
> only complication being that it'll be using the DSDT for the fake 
> machine and not the real one..
I modified Qemu and use it to observe how winxp does suspend/resume. So
far, I just get some data for s4 suspend. I did have some interesting
finding.

1. xp seems not save pci config space. Or it appears just save config
PCICMD.
2. the order winxp does looks like
	a. save config (PCICMD), put device to D3 (it appears only for ne2000
NIC)
	b. _PTS
	c. write mem to disk
	d. write ACPI PM1_control register, then system shutdown
3. xp write ACPI GBL_EN bit just after _PTS (for both S4/S5), don't know
why

Attached is the log winxp does s4 suspend, it only includes pci config
read/write and ACPI register read/write.

I managed to make xp enter S3, but fails, so can't get the data for S3
so far. Anybody has other ideas which need to verify winxp, pls let me
know.

Thanks,
Shaohua

[-- Attachment #2: xplog --]
[-- Type: text/plain, Size: 2050 bytes --]

PCI NE2000 read addr 4, val 7
PCI NE2000 read addr 50, val 1
PCI NE2000 read addr 52, val c9c2
PCI NE2000 read addr 54, val 8000
PCI NE2000 write addr 54, val 8003
PCI NE2000 read addr 54, val 8003
PCI NE2000 read addr 4, val 7
PCI NE2000 write addr 4, val 0
PCI PIIX3 read addr 0, val 70008086
PCI PIIX3 read addr 4, val 7
PCI PIIX3 read addr 8, val 6010000
PCI PIIX3 read addr c, val 800000
PCI PM read addr 0, val 71138086
PCI PIIX3 read addr 0, val 70008086
PCI PIIX3 read addr 4, val 7
PCI PIIX3 read addr 8, val 6010000
PCI PIIX3 read addr c, val 800000
PCI PM read addr 64, val 8000000
PCI PIIX3 read addr 0, val 70008086
PCI PIIX3 read addr 4, val 7
PCI PIIX3 read addr 8, val 6010000
PCI PIIX3 read addr c, val 800000
PCI PM read addr 0, val 71138086
PCI PIIX3 read addr 0, val 70008086
PCI PIIX3 read addr 4, val 7
PCI PIIX3 read addr 8, val 6010000
PCI PIIX3 read addr c, val 800000
PCI PM read addr 64, val 8000000
PCI Cirrus VGA read addr 4, val 7
PCI PIIX3 read addr 0, val 70008086
PCI PIIX3 read addr 4, val 7
PCI PIIX3 read addr 8, val 6010000
PCI PIIX3 read addr c, val 800000
PCI PIIX3 IDE read addr 4, val 7
PCI PIIX3 read addr 4, val 7
PCI PIIX3 read addr 0, val 70008086
PCI PIIX3 read addr 4, val 7
PCI PIIX3 read addr 8, val 6010000
PCI PIIX3 read addr c, val 800000
PCI PM read addr 0, val 71138086
PCI PIIX3 read addr 0, val 70008086
PCI PIIX3 read addr 4, val 7
PCI PIIX3 read addr 8, val 6010000
PCI PIIX3 read addr c, val 800000
PCI PM read addr 5c, val 90000000
PCI PIIX3 read addr 0, val 70008086
PCI PIIX3 read addr 4, val 7
PCI PIIX3 read addr 8, val 6010000
PCI PIIX3 read addr c, val 800000
PCI PM read addr 0, val 71138086
PCI PIIX3 read addr 0, val 70008086
PCI PIIX3 read addr 4, val 7
PCI PIIX3 read addr 8, val 6010000
PCI PIIX3 read addr c, val 800000
PCI PM read addr 5c, val 90000000
ACPI: DBG: 0x00000002
PM readw port=0x0002 val=0xb002
PM writew port=0x0002 val=0x0020
PM readw port=0x0002 val=0xb002
PM readw port=0x0004 val=0xb004
PM readw port=0x0004 val=0xb004
PM writew port=0x0004 val=0x2801
type 2