2007-12-24 22:41:01

by Robert Hancock

[permalink] [raw]
Subject: Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

Carlos Corbacho wrote:
> On Monday 24 December 2007 18:34:21 Linus Torvalds wrote:
>> On Mon, 24 Dec 2007, Rafael J. Wysocki wrote:
>>> Well, having considered that for a longer while, I think the AML code is
>>> referring to a device that we have suspended already, and since it's in a
>>> low power state, it just can't handle the reference.
>>>
>>> If that is the case, we'll have to find the device (that should be
>>> possible using some code instrumentation) and move the suspending of it
>>> into the late stage.
>> Yes.
>
> My own experimentation (in device_suspend(), calling _PTS() in the AML after
> each suspend_device() runs, until one device causes it to hang) points to
> ohci_hcd being the culprit here (with or without any devices attached). With
> the ohci_hcd module unloaded, the machine suspends just fine[1].
>
> Of course, I'm at a complete loss as to why suspending OHCI would cause a
> problem for an IO port write.

The name of the operation region, SMIP, suggests that the BIOS has an
SMI trap on that port. In that case, writing to that port will result in
the BIOS taking control. We have little idea what it could be doing.
Could be it's trying to access the OHCI controller which has been
suspended already.

This sounds kind of like the Toshiba laptops that go nuts somewhere if
the AHCI SATA controller gets put into suspend state before the system
suspends..

The ACPI spec has the following to say about the _PTS method:

"The platform must not make any assumptions about the state of the
machine when _PTS is called. For example, operation region accesses that
require devices to be configured and enabled may not succeed, as these
devices may be in a non-decoding state due to plug and play or power
management operations."

I would guess some BIOS writers failed to heed this..

>
>> NOTE! This following patch is just for discussion, and while I think it's
>> conceptually a good thing to try, I don't think it will help Carlos'
>> problem. But removing the "pci_set_power_state()" in agp_nvidia_suspend()
>> might.
>
> nvidia-agp cannot be built on x86-64, so it's not the culprit in this case.

Yeah, and this is a PCI Express system not AGP, so it wouldn't load anyway.

--
Robert Hancock Saskatoon, SK, Canada
To email, remove "nospam" from [email protected]
Home Page: http://www.roberthancock.com/


2007-12-25 00:03:43

by Carlos Corbacho

[permalink] [raw]
Subject: Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

On Monday 24 December 2007 22:40:46 Robert Hancock wrote:
> The ACPI spec has the following to say about the _PTS method:
>
> "The platform must not make any assumptions about the state of the
> machine when _PTS is called. For example, operation region accesses that
> require devices to be configured and enabled may not succeed, as these
> devices may be in a non-decoding state due to plug and play or power
> management operations."

That is from the 3.0 spec though. And it looks like the definition changed
from pre-3.0 to 3.0 and above.

This is what the 1.0B spec says (and this is also repeated verbatim in 2.0,
2.0a, 2.0b, and 2.0c):

"The _PTS control method is executed by the operating system at the beginning
of the sleep process for S1, S2, S3, S4, and for orderly S5 shutdown. The
sleeping state value (1, 2, 3, 4, or 5) is passed to the _PTS control method.
Before the OS notifies native device drivers and prepares the system software
for a system sleeping state, it executes this ACPI control method. Thus, this
control method can be executed a relatively long time before actually
entering the desired sleeping state. In addition, the OS can abort the
sleeping operation without notification to the ACPI driver, in which case
another _PTS would occur some time before the next attempt by the OS to enter
a sleeping state. The _PTS control method cannot modify the current
configuration or power state of any device in the system. For example, _PTS
would simply store the sleep type in the embedded controller in sequencing
the system into a sleep state when the SLP_EN bit is set."

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.

> I would guess some BIOS writers failed to heed this..

No, it looks like the BIOS is obeying the older specs, and Linux is at fault
here for breaking the suspend logic of pre ACPI 3.0 systems.

-Carlos
--
E-Mail: [email protected]
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

2007-12-25 02:42:06

by Carlos Corbacho

[permalink] [raw]
Subject: 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)

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;
}

2007-12-25 13:07:16

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

On Tuesday, 25 of December 2007, Carlos Corbacho wrote:
> On Monday 24 December 2007 22:40:46 Robert Hancock wrote:
> > The ACPI spec has the following to say about the _PTS method:
> >
> > "The platform must not make any assumptions about the state of the
> > machine when _PTS is called. For example, operation region accesses that
> > require devices to be configured and enabled may not succeed, as these
> > devices may be in a non-decoding state due to plug and play or power
> > management operations."
>
> That is from the 3.0 spec though. And it looks like the definition changed
> from pre-3.0 to 3.0 and above.
>
> This is what the 1.0B spec says (and this is also repeated verbatim in 2.0,
> 2.0a, 2.0b, and 2.0c):
>
> "The _PTS control method is executed by the operating system at the beginning
> of the sleep process for S1, S2, S3, S4, and for orderly S5 shutdown. The
> sleeping state value (1, 2, 3, 4, or 5) is passed to the _PTS control method.
> Before the OS notifies native device drivers and prepares the system software
> for a system sleeping state, it executes this ACPI control method. Thus, this
> control method can be executed a relatively long time before actually
> entering the desired sleeping state. In addition, the OS can abort the
> sleeping operation without notification to the ACPI driver, in which case
> another _PTS would occur some time before the next attempt by the OS to enter
> a sleeping state. The _PTS control method cannot modify the current
> configuration or power state of any device in the system. For example, _PTS
> would simply store the sleep type in the embedded controller in sequencing
> the system into a sleep state when the SLP_EN bit is set."
>
> 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.

Well, citing from the ACPI 2.0 specification, section 9.1.6 Transitioning from
the Working to the Sleeping State (which is what we're discussing here):

3. OSPM places all device drivers into their respective Dx state. If the device
is enabled for wake, it enters the Dx state associated with the wake
capability. If the device is not enabled to wake the system, it enters the
D3 state.
4. OSPM executes the _PTS control method, passing an argument that indicates
the desired sleeping state (1, 2, 3, or 4 representing S1, S2, S3, and S4).

My opinion is that we should follow this part of the specification and so we do.

> 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.
>
> > I would guess some BIOS writers failed to heed this..
>
> No, it looks like the BIOS is obeying the older specs, and Linux is at fault
> here for breaking the suspend logic of pre ACPI 3.0 systems.

You're wrong, sorry.

Greetings,
Rafael

2007-12-25 13:12:54

by Carlos Corbacho

[permalink] [raw]
Subject: Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

On Tuesday 25 December 2007 13:26:12 Rafael J. Wysocki wrote:
> Well, citing from the ACPI 2.0 specification, section 9.1.6 Transitioning
> from the Working to the Sleeping State (which is what we're discussing
> here):
>
> 3. OSPM places all device drivers into their respective Dx state. If the
> device is enabled for wake, it enters the Dx state associated with the wake
> capability. If the device is not enabled to wake the system, it enters the
> D3 state.
> 4. OSPM executes the _PTS control method, passing an argument that
> indicates the desired sleeping state (1, 2, 3, or 4 representing S1, S2,
> S3, and S4).
>
> My opinion is that we should follow this part of the specification and so
> we do.

This is that same section from ACPI 1.0B:

3. The OS executes the Prepare To Sleep (_PTS) control method, passing an
argument that indicates the desired sleeping state (1, 2, 3, or 4 representing
S1, S2, S3, and S4).

4. The OS places all device drivers into their respective Dx state. If the
device is enabled for wakeup, it enters the Dx state associated with the
wakeup capability. If the device is not enabled to wakeup the system, it
enters the D3 state.

The DSDTs in question also claim ACPI 1.0 compatiblity.

> You're wrong, sorry.

No, I'm not entirely wrong - read the 1.0 spec, and read section 7.3.2 of the
ACPI 2.0 spec.

* ACPI 1.0 is very clear - we are breaking the 1.0 spec

* ACPI 2.0 is contradictory - section 7.3.2 repeats 1.0 ad verbatim (which is
what I quote in reply to Robert Hancock), but as you point out, 9.3.2 says
the opposite.

So, 1.0 and 3.0 are very clear and rather different on this, and 2.0 is
contradictory (and I presume this is one of the points ACPI 3.0 set out to
clean up).

I will rescind my point on ACPI 2.0 - I don't know what we should or shouldn't
be doing there, the spec is unclear.

But for ACPI 1.0, we are doing the wrong thing.

-Carlos
--
E-Mail: [email protected]
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

2007-12-25 13:17:50

by Rafael J. Wysocki

[permalink] [raw]
Subject: 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)

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

2007-12-25 13:48:36

by Rafael J. Wysocki

[permalink] [raw]
Subject: 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)

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

2007-12-25 13:52:09

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

On Tuesday, 25 of December 2007, Carlos Corbacho wrote:
> On Tuesday 25 December 2007 13:26:12 Rafael J. Wysocki wrote:
> > Well, citing from the ACPI 2.0 specification, section 9.1.6 Transitioning
> > from the Working to the Sleeping State (which is what we're discussing
> > here):
> >
> > 3. OSPM places all device drivers into their respective Dx state. If the
> > device is enabled for wake, it enters the Dx state associated with the wake
> > capability. If the device is not enabled to wake the system, it enters the
> > D3 state.
> > 4. OSPM executes the _PTS control method, passing an argument that
> > indicates the desired sleeping state (1, 2, 3, or 4 representing S1, S2,
> > S3, and S4).
> >
> > My opinion is that we should follow this part of the specification and so
> > we do.
>
> This is that same section from ACPI 1.0B:
>
> 3. The OS executes the Prepare To Sleep (_PTS) control method, passing an
> argument that indicates the desired sleeping state (1, 2, 3, or 4 representing
> S1, S2, S3, and S4).
>
> 4. The OS places all device drivers into their respective Dx state. If the
> device is enabled for wakeup, it enters the Dx state associated with the
> wakeup capability. If the device is not enabled to wakeup the system, it
> enters the D3 state.
>
> The DSDTs in question also claim ACPI 1.0 compatiblity.
>
> > You're wrong, sorry.
>
> No, I'm not entirely wrong - read the 1.0 spec, and read section 7.3.2 of the
> ACPI 2.0 spec.
>
> * ACPI 1.0 is very clear - we are breaking the 1.0 spec

By following the 2.0 and later ones. Well ...

> * ACPI 2.0 is contradictory - section 7.3.2 repeats 1.0 ad verbatim (which is
> what I quote in reply to Robert Hancock), but as you point out, 9.3.2 says
> the opposite.
>
> So, 1.0 and 3.0 are very clear and rather different on this, and 2.0 is
> contradictory (and I presume this is one of the points ACPI 3.0 set out to
> clean up).
>
> I will rescind my point on ACPI 2.0 - I don't know what we should or shouldn't
> be doing there, the spec is unclear.

I think we should follow section 9.3.2 that is explicit and has been reiterated
in the 3.0 specification.

> But for ACPI 1.0, we are doing the wrong thing.

Yes, we are.

OK, I think we can rearrange things to call _PTS early for ACPI 1.0x-compliant
systems.

Thanks,
Rafael

2007-12-25 13:52:32

by Carlos Corbacho

[permalink] [raw]
Subject: 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)

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: [email protected]
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

2007-12-25 17:17:46

by Robert Hancock

[permalink] [raw]
Subject: Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

Carlos Corbacho wrote:
> On Tuesday 25 December 2007 13:26:12 Rafael J. Wysocki wrote:
>> Well, citing from the ACPI 2.0 specification, section 9.1.6 Transitioning
>> from the Working to the Sleeping State (which is what we're discussing
>> here):
>>
>> 3. OSPM places all device drivers into their respective Dx state. If the
>> device is enabled for wake, it enters the Dx state associated with the wake
>> capability. If the device is not enabled to wake the system, it enters the
>> D3 state.
>> 4. OSPM executes the _PTS control method, passing an argument that
>> indicates the desired sleeping state (1, 2, 3, or 4 representing S1, S2,
>> S3, and S4).
>>
>> My opinion is that we should follow this part of the specification and so
>> we do.
>
> This is that same section from ACPI 1.0B:
>
> 3. The OS executes the Prepare To Sleep (_PTS) control method, passing an
> argument that indicates the desired sleeping state (1, 2, 3, or 4 representing
> S1, S2, S3, and S4).
>
> 4. The OS places all device drivers into their respective Dx state. If the
> device is enabled for wakeup, it enters the Dx state associated with the
> wakeup capability. If the device is not enabled to wakeup the system, it
> enters the D3 state.
>
> The DSDTs in question also claim ACPI 1.0 compatiblity.
>
>> You're wrong, sorry.
>
> No, I'm not entirely wrong - read the 1.0 spec, and read section 7.3.2 of the
> ACPI 2.0 spec.
>
> * ACPI 1.0 is very clear - we are breaking the 1.0 spec
>
> * ACPI 2.0 is contradictory - section 7.3.2 repeats 1.0 ad verbatim (which is
> what I quote in reply to Robert Hancock), but as you point out, 9.3.2 says
> the opposite.
>
> So, 1.0 and 3.0 are very clear and rather different on this, and 2.0 is
> contradictory (and I presume this is one of the points ACPI 3.0 set out to
> clean up).
>
> I will rescind my point on ACPI 2.0 - I don't know what we should or shouldn't
> be doing there, the spec is unclear.
>
> But for ACPI 1.0, we are doing the wrong thing.

Correct me if I'm wrong, but it appears ACPI 1.0 wants _PTS called
before any devices are suspended, ACPI 2.0 is contradictory, and ACPI
3.0 says that you can't assume anything about device state. My guess is
that unless Windows has different behavior depending on ACPI version, it
probably has called _PTS before suspending devices all along. Therefore
it would likely be safest to emulate that behavior, no?

--
Robert Hancock Saskatoon, SK, Canada
To email, remove "nospam" from [email protected]
Home Page: http://www.roberthancock.com/

2007-12-25 18:07:47

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

On Tuesday, 25 of December 2007, Robert Hancock wrote:
> Carlos Corbacho wrote:
> > On Tuesday 25 December 2007 13:26:12 Rafael J. Wysocki wrote:
> >> Well, citing from the ACPI 2.0 specification, section 9.1.6 Transitioning
> >> from the Working to the Sleeping State (which is what we're discussing
> >> here):
> >>
> >> 3. OSPM places all device drivers into their respective Dx state. If the
> >> device is enabled for wake, it enters the Dx state associated with the wake
> >> capability. If the device is not enabled to wake the system, it enters the
> >> D3 state.
> >> 4. OSPM executes the _PTS control method, passing an argument that
> >> indicates the desired sleeping state (1, 2, 3, or 4 representing S1, S2,
> >> S3, and S4).
> >>
> >> My opinion is that we should follow this part of the specification and so
> >> we do.
> >
> > This is that same section from ACPI 1.0B:
> >
> > 3. The OS executes the Prepare To Sleep (_PTS) control method, passing an
> > argument that indicates the desired sleeping state (1, 2, 3, or 4 representing
> > S1, S2, S3, and S4).
> >
> > 4. The OS places all device drivers into their respective Dx state. If the
> > device is enabled for wakeup, it enters the Dx state associated with the
> > wakeup capability. If the device is not enabled to wakeup the system, it
> > enters the D3 state.
> >
> > The DSDTs in question also claim ACPI 1.0 compatiblity.
> >
> >> You're wrong, sorry.
> >
> > No, I'm not entirely wrong - read the 1.0 spec, and read section 7.3.2 of the
> > ACPI 2.0 spec.
> >
> > * ACPI 1.0 is very clear - we are breaking the 1.0 spec
> >
> > * ACPI 2.0 is contradictory - section 7.3.2 repeats 1.0 ad verbatim (which is
> > what I quote in reply to Robert Hancock), but as you point out, 9.3.2 says
> > the opposite.
> >
> > So, 1.0 and 3.0 are very clear and rather different on this, and 2.0 is
> > contradictory (and I presume this is one of the points ACPI 3.0 set out to
> > clean up).
> >
> > I will rescind my point on ACPI 2.0 - I don't know what we should or shouldn't
> > be doing there, the spec is unclear.
> >
> > But for ACPI 1.0, we are doing the wrong thing.
>
> Correct me if I'm wrong, but it appears ACPI 1.0 wants _PTS called
> before any devices are suspended, ACPI 2.0 is contradictory, and ACPI
> 3.0 says that you can't assume anything about device state. My guess is
> that unless Windows has different behavior depending on ACPI version, it
> probably has called _PTS before suspending devices all along. Therefore
> it would likely be safest to emulate that behavior, no?

Well, I don't think so.

In fact ACPI 3.0 repeats the 2.0 wording in section 9.1.6 (so the current
requirement seems to be to put devices into low power states before calling
_PTS) and I _guess_ there was a change in the default behavior of Windows that
caused the specification to be modified (I'd bet that was between 2000 and XP
or something like this).

IMO, we should check which version of the specification we're supposed to
follow, on the basis of FADT contents, for example, and follow this one.

Thanks,
Rafael

2007-12-26 04:31:33

by Linus Torvalds

[permalink] [raw]
Subject: Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM



On Tue, 25 Dec 2007, Rafael J. Wysocki wrote:
> >
> > Correct me if I'm wrong, but it appears ACPI 1.0 wants _PTS called
> > before any devices are suspended, ACPI 2.0 is contradictory, and ACPI
> > 3.0 says that you can't assume anything about device state. My guess is
> > that unless Windows has different behavior depending on ACPI version, it
> > probably has called _PTS before suspending devices all along. Therefore
> > it would likely be safest to emulate that behavior, no?
>
> Well, I don't think so.

It would *definitely* always be safest to emulate what Windows does.

> In fact ACPI 3.0 repeats the 2.0 wording in section 9.1.6 (so the current
> requirement seems to be to put devices into low power states before calling
> _PTS) and I _guess_ there was a change in the default behavior of Windows that
> caused the specification to be modified (I'd bet that was between 2000 and XP
> or something like this).

You seem to put a lot of trust in a piece of documentation.

Do you realize how those pieces of paper are written? They are written by
people who have absolutely *nothing* to do with the actual implementation,
and whose job it is to write documentation. And while the people who
actually do the programming etc are supposed to help them, the two parties
generally detest each other.

Technical writers hate the "real engineers" for not helping them, and the
"real engineers" tend to dislike having to be pestered to explain their
stuff and have to read through some document that isn't meant for them,
but that they need to sign off on.

In other words: please do *not* expect that the documentation actually
matches reality. You seem to think that the documentation came first
and/or is quite accurate. That's not at all likely to be true.

> IMO, we should check which version of the specification we're supposed to
> follow, on the basis of FADT contents, for example, and follow this one.

No, we should try to figure out what Windows does. *If* windows checks the
version, we should do that too. But we should absolutely *not* just assume
that the documentation is an accurate picture of reality.

Does anybody know how we could find out?

Linus

2007-12-26 05:13:28

by Robert Hancock

[permalink] [raw]
Subject: Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

Linus Torvalds wrote:
>> IMO, we should check which version of the specification we're supposed to
>> follow, on the basis of FADT contents, for example, and follow this one.
>
> No, we should try to figure out what Windows does. *If* windows checks the
> version, we should do that too. But we should absolutely *not* just assume
> that the documentation is an accurate picture of reality.
>
> Does anybody know how we could find out?
>
> Linus
>

Well, it seems that if one had a checked (debug) build of Windows (or at
least the acpi.sys driver) installed, as well as a copy of the Microsoft
ASL compiler, they could compile and temporarily override the DSDT with
a hacked one that would output what the device power states were in some
fashion (maybe through the kernel debugger). Some info about this here:

http://download.microsoft.com/download/1/8/f/18f8cee2-0b64-41f2-893d-a6f2295b40c8/TW04015_WINHEC2004.ppt

I suspect that might require more Windows hacking skill and/or
motivation than one might be likely to find on this list, though :-)

--
Robert Hancock Saskatoon, SK, Canada
To email, remove "nospam" from [email protected]
Home Page: http://www.roberthancock.com/

2007-12-26 07:24:40

by Avi Kivity

[permalink] [raw]
Subject: Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

Linus Torvalds wrote:
>
>> IMO, we should check which version of the specification we're supposed to
>> follow, on the basis of FADT contents, for example, and follow this one.
>>
>
> No, we should try to figure out what Windows does. *If* windows checks the
> version, we should do that too. But we should absolutely *not* just assume
> that the documentation is an accurate picture of reality.
>
> Does anybody know how we could find out?
>
>

Run it in a virtual machine, with the ACPI methods of interest wired to
output to some port, and log that I/O port in the monitor/emulator.

--
Do not meddle in the internals of kernels, for they are subtle and quick to panic.