Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Rafael J. Wysocki]

On Sunday, 23 of December 2007, Carlos Corbacho wrote:
> Fix suspend-to-RAM on nForce 4 (CK804) boards by increasing
> PCIBIOS_MIN_IO.
> 
> Fixes kernel bugzilla #9528
> 
> Problem:
> 
> Linus' patch (52ade9b3b97fd3bea42842a056fe0786c28d0555) to re-order
> suspend (and fix fall out from Rafael's earlier suspend reordering work)
> broke suspend-to-RAM on nForce 4 (CK804) boards.
> 
> Why:
> 
> After debugging _PTS() in the DSDT, it turns out these nVidia boards are
> trying to write to an IO port > 0x1000 (0x142E) during suspend. Before the
> re-ordering, we got away with this.
> 
> After the afore mentioned commit, we started hitting the PCIBIOS_MIN_IO
> limit and suspend then broke on these machines (the machine simply hangs
> when it reaches the 0x142E IO port write during suspend-to-RAM).
> 
> There was some previous work in the PCIBIOS_MIN_IO area over two years ago
> (71db63acff69618b3d9d3114bd061938150e146b) which bumped this to 0x4000,
> but this was reverted (2ba84684e8cf6f980e4e95a2300f53a505eb794e) after
> causing new and entirely different problems on another nForce board.
> 
> 0x1500 has been picked here as a nice, round and more conservative value
> than 0x4000, and which covers 0x142E.

The patch is fine by me, so if anyone has objections, please speak up.

Thanks,
Rafael


> Tested on x86-64.
> 
> Signed-off-by: Carlos Corbacho <carlos@strangeworlds.co.uk>
> CC: Rafael J. Wysocki <rjw@sisk.pl>
> CC: Linus Torvalds <torvalds@linux-foundation.org>
> CC: Greg KH <gregkh@suse.de>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Len Brown <lenb@kernel.org>
> ---
> Since it's not entirely clear who is responsible for what in this file,
> and given what it fixes, I'm CC'ing you all in the hope that someone
> will handle this.
> 
>  include/asm-x86/pci.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> 
> diff --git a/include/asm-x86/pci.h b/include/asm-x86/pci.h
> index e883619..03cb123 100644
> --- a/include/asm-x86/pci.h
> +++ b/include/asm-x86/pci.h
> @@ -46,7 +46,7 @@ extern unsigned int pcibios_assign_all_busses(void);
>  #define pcibios_scan_all_fns(a, b)	0
>  
>  extern unsigned long pci_mem_start;
> -#define PCIBIOS_MIN_IO		0x1000
> +#define PCIBIOS_MIN_IO		0x1500
>  #define PCIBIOS_MIN_MEM		(pci_mem_start)
>  
>  #define PCIBIOS_MIN_CARDBUS_IO	0x4000

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Ingo Molnar]

* Rafael J. Wysocki <rjw@sisk.pl> wrote:

> > 0x1500 has been picked here as a nice, round and more conservative 
> > value than 0x4000, and which covers 0x142E.
> 
> The patch is fine by me, so if anyone has objections, please speak up.

i'm quite nervous about that approach, partly due to the "black magic" 
interaction of commit 2ba84684e8c. Could we try to figure out what's 
really going on here, instead of playing with a general limit (which 
might break other boxes)?

	Ingo

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Linus Torvalds]

On Sun, 23 Dec 2007, Rafael J. Wysocki wrote:
> 
> The patch is fine by me, so if anyone has objections, please speak up.

There is absolutely *no* way I will apply this in an -rc6 release.

The number of machines this will break is totally unknown. It might be 
zero. It might be hundreds.  We just don't know. We might hit another 
unlucky allocation that we just happened to avoid before.

		Linus

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Rafael J. Wysocki]

On Sunday, 23 of December 2007, Linus Torvalds wrote:
> 
> On Sun, 23 Dec 2007, Rafael J. Wysocki wrote:
> > 
> > The patch is fine by me, so if anyone has objections, please speak up.
> 
> There is absolutely *no* way I will apply this in an -rc6 release.
> 
> The number of machines this will break is totally unknown. It might be 
> zero. It might be hundreds.  We just don't know. We might hit another 
> unlucky allocation that we just happened to avoid before.

I was rather thinking of putting it into -mm for some time and target for
2.6.25 if possible.

If it breaks systems, we can always revert before 2.6.25 final.

Thanks,
Rafael

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[H. Peter Anvin]

Rafael J. Wysocki wrote:
> On Sunday, 23 of December 2007, Linus Torvalds wrote:
>> On Sun, 23 Dec 2007, Rafael J. Wysocki wrote:
>>> The patch is fine by me, so if anyone has objections, please speak up.
>> There is absolutely *no* way I will apply this in an -rc6 release.
>>
>> The number of machines this will break is totally unknown. It might be 
>> zero. It might be hundreds.  We just don't know. We might hit another 
>> unlucky allocation that we just happened to avoid before.
> 
> I was rather thinking of putting it into -mm for some time and target for
> 2.6.25 if possible.
> 
> If it breaks systems, we can always revert before 2.6.25 final.
> 

This is totally the wrong way to go about it.

Instead, it should detect this particular chipset and reserve relevant 
ports.  Even better would be if we can find out what reserves these 
ports and mark it as a quirk.

That being said, I have seen other chipsets allocate ports in the low 
0x1XXX range using non-BAR methods.  They *should* reserve them in ACPI, 
of course.

	-hpa

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Carlos Corbacho]

On Sunday 23 December 2007 23:12:47 H. Peter Anvin wrote:
> Rafael J. Wysocki wrote:
> > On Sunday, 23 of December 2007, Linus Torvalds wrote:
> >> On Sun, 23 Dec 2007, Rafael J. Wysocki wrote:
> >>> The patch is fine by me, so if anyone has objections, please speak up.
> >>
> >> There is absolutely *no* way I will apply this in an -rc6 release.
> >>
> This is totally the wrong way to go about it.

Please disregard the patch anyway - my test system was still using the custom 
DSDT - it doesn't fix anything.

Regardless, Linus' patch in question (in combination with Rafael's suspend 
reordering work) still broke suspend, and the port 0x142E write is still the 
offender, so something is still not playing nice - I'm just now at a complete 
loss as to what.

(PNPACPI came to mind as a suspect, but even with that disabled, this board/ 
chipset still wedges on suspend).

-Carlos
-- 
E-Mail: carlos@strangeworlds.co.uk
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Linus Torvalds]

On Mon, 24 Dec 2007, Carlos Corbacho wrote:
> 
> Please disregard the patch anyway - my test system was still using the custom 
> DSDT - it doesn't fix anything.

Ok, so it's not a simple IO port conflict.

And the range 0x1400-0x147f (which is apparently the ACPI block) is 
properly marked as reserved.

So the IO write to 1428 is a red herring. It's just part of the normal 
sequence to suspend-to-ram, and while it failing probably has something to 
do with the failure to s2ram, it's simply a result of ACPI doing something 
insane, and probably making some assumptions that we've broken by mistake 
when we do the higher-level device_suspend() before we do the low-level 
one.

IOW, it looks like the normal kind of ACPI mess. Color me not in the least 
surprised, and it needs somebody who understands AML and what the heck is 
supposed to happen to figure out.

The kernel doesn't do anything at all to port 1428 (since it is reserved), 
so if any write to it "fails" (how?) it's probably because the writer made 
some assumptions about system state.

			Linus

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Linus Torvalds]

On Sun, 23 Dec 2007, Linus Torvalds wrote:
> 
> IOW, it looks like the normal kind of ACPI mess. Color me not in the least 
> surprised, and it needs somebody who understands AML and what the heck is 
> supposed to happen to figure out.

Side note: we could obviously undo the commit that triggered this for you 
(ie 52ade9b3b97fd3bea42842a056fe0786c28d0555), but then we have to undo 
also the commit that caused us to do that commit in the first place, and 
change the ordering on resume too (that would be commit 
e3c7db621bed4afb8e231cb005057f2feb5db557 - the commit that moved the 
"pm_ops->finish()" call to before the call to device_resume())

In other words, we'd have to go back to our original ordering, which Len 
said was fundamentally wrong. I don't think anybody really wants that.

It would be better to figure out why "device_suspend()" apparently causes 
problems for your AML crud.

Oh, and why is linux-kernel cc'd, but not linux-pm? Are all the relevant 
people from linux-pm cc'd, or should somebody who is on that list please 
try to condense this down?

(For linux-pm: see 

        http://bugzilla.kernel.org/show_bug.cgi?id=9528

for some more details, including a few red herrings like the whole subject 
line of this email thread which turned out to not be valid after all).

		Linus

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Carlos Corbacho]

On Monday 24 December 2007 01:14:34 Linus Torvalds wrote:
> Side note: we could obviously undo the commit that triggered this for you
> [..]
> In other words, we'd have to go back to our original ordering, which Len
> said was fundamentally wrong. I don't think anybody really wants that.

Nor would I argue to do so.

> It would be better to figure out why "device_suspend()" apparently causes
> problems for your AML crud.

Will do - thanks for the pointer.

> Oh, and why is linux-kernel cc'd, but not linux-pm?

Because I like to compound my errors (I mean, if you're going to screw up, you 
might as well _really_ go for it).

-Carlos
-- 
E-Mail: carlos@strangeworlds.co.uk
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Rafael J. Wysocki]

On Monday, 24 of December 2007, Carlos Corbacho wrote:
> On Monday 24 December 2007 01:14:34 Linus Torvalds wrote:
> > Side note: we could obviously undo the commit that triggered this for you
> > [..]
> > In other words, we'd have to go back to our original ordering, which Len
> > said was fundamentally wrong. I don't think anybody really wants that.
> 
> Nor would I argue to do so.
> 
> > It would be better to figure out why "device_suspend()" apparently causes
> > problems for your AML crud.
> 
> Will do - thanks for the pointer.

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.

> > Oh, and why is linux-kernel cc'd, but not linux-pm?
> 
> Because I like to compound my errors (I mean, if you're going to screw up, you 
> might as well _really_ go for it).

BTW, linux-pm is on linux-foundation, changed the CC. ;-)

Thanks,
Rafael

Re: [Bug 9528] x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Robert Hancock]

Linus Torvalds wrote:
> 
> On Sun, 23 Dec 2007, Carlos Corbacho wrote:
>> Fix suspend-to-RAM on nForce 4 (CK804) boards by increasing
>> PCIBIOS_MIN_IO.
>>
>> Fixes kernel bugzilla #9528
>>
>> Problem:
>>
>> Linus' patch (52ade9b3b97fd3bea42842a056fe0786c28d0555) to re-order
>> suspend (and fix fall out from Rafael's earlier suspend reordering work)
>> broke suspend-to-RAM on nForce 4 (CK804) boards.
>>
>> Why:
>>
>> After debugging _PTS() in the DSDT, it turns out these nVidia boards are
>> trying to write to an IO port > 0x1000 (0x142E) during suspend. Before the
>> re-ordering, we got away with this.
> 
> Very interesting.
> 
> HOWEVER.
> 
> I'd much rather figure out what the magic IO resource is that clashes. 
> 
> It's almost certainly some hidden and undocumented (or badly documented) 
> ACPI IO area that the kernel doesn't know about, because it's not a 
> regular PCI BAR resource, but some northbridge (or southbridge) magic 
> register range.
> 
> Those ranges *should* be reserved by the BIOS in the ACPI tables, but this 
> would definitely not be the first time that doesn't happen.

I'm having trouble sorting out which report is for which BIOS (and some 
of them don't have any dmesg posted), but I believe in these cases that 
memory region is indeed reported as reserved by the BIOS, and no PCI 
resources should end up allocated there. So I'm not sure why fiddling 
with PCIBIOS_MIN_IO would have any effect (other than by accident).

I wonder if this is the culprit (from Arthur Erhardt's dmesg):

pnpacpi: exceeded the max number of mem resources: 12
pnpacpi: exceeded the max number of mem resources: 12

which means we're ignoring some of the memory reservations. I wonder if 
some IO reservations are also being ignored?

Why do we have this silly hard limit of number of resources anyway? If 
we just ignore random reservations provided by the BIOS, we shouldn't be 
surprised if things break randomly. This warning at the very least 
should be much louder (i.e. "Warning: This problem may break your system")..

> 
> But the right fix would be for us to just figure out what the range is ass 
> a PCI quirk, and just know to avoid it on purpose, ratehr than just being 
> lucky and happen to avoid it because PCIBIOS_MIN_IO just happens to be 
> bigger than the particular address.
> 
> So can you:
>  - show what your /proc/ioports contains (*with* the bug triggering, ie 
>    non-working suspend, so we see what it is that actually ends up using 
>    that area)
>  - send out 'dmesg' for a boot (same deal)
>  - add "lspci -xxxvv" output to the deal too.
> 
> and also make them part of the bugzilla history (I'm cc'ing bugzilla here, 
> and added the bug number to the subject, so hopefully this thread ends up 
> being archived there too).
> 
>> There was some previous work in the PCIBIOS_MIN_IO area over two years ago
>> (71db63acff69618b3d9d3114bd061938150e146b) which bumped this to 0x4000,
>> but this was reverted (2ba84684e8cf6f980e4e95a2300f53a505eb794e) after
>> causing new and entirely different problems on another nForce board.
> 
> The problem here is classic: these magic ranges tend to be *different* on 
> different boards (because they don't tend to be fixed by hardware, they 
> are programmed regions set up by firmware), so trying to change 
> PCIBIOS_MIN_IO to avoid a problem on one board is almost certain to just 
> introduce it on another board instead.
> 
> On *your* particular board, 0x142E is used for something, but on somebody 
> elses board it might be 0x162E, and now changing PCIBIOS_MIN_IO to 0x1500 
> might make that other board hang instead.
> 
> So you seem to have debugged this very successfully, and I'm wondering if 
> you might be able to find out where that 0x142e comes from, and we could 
> fix it for *all* boards using that chipset by just figuring out what the 
> *hardware* rules (rather than the random firmware setup that will be 
> different on different boards) for that chipset actually are!

I suspect it's board specific. Looking at the DSDT for my A8N-SLI 
Deluxe, that SMIP region is defined at 0x442E (and is reported as 
reserved). This BIOS doesn't write there in the _PTS method like the 
ones in the report apparently do though.

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

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Linus Torvalds]

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. 

In general, I'm personally of the opinion that drivers should *not* 
actually go into D3 at all in the regular "->suspend()" phase. It should 
be done in ->suspend_late. The early suspend is for saving state and 
returning errors.

Sadly, we've made it a bit too inconvenient to actually do that. Almost 
all drivers only do the "->suspend" thing, and the default PCI behaviour 
doesn't help us in any way either.

Anyway, I wonder if a patch like this could make it easier for driver 
writers to handle things. It basically does:

 - if you don't have a regular "suspend()" function, we'll just save state 
   at suspend time.

 - if you don't have a "suspend_late()" function, we'll look at the 
   current state, and if it's still in PCI_D0, we'll suspend to PCI_D3hot 
   if it's a regular PCI device (ie not a bridge or something else odd).

 - then, at resume time, by default we don't do anything in the early 
   resume, but in the late resume we'll undo everything, of course.

Anyway, with this, most drivers could just remove the 
"pci_set_power_state()" call *entirely*, and let the default 
suspend_late action power the device down. But if you want to override 
that default action, you can either:

 - set the power state in your own ->suspend() routine (either by using 
   pci_set_power_state(), or by just explicitly setting the state to 
   unknown with "dev->current_state =  PCI_UNKNOWN"

 - have a "late_suspend()" action, which obviously will override the 
   default action entirely.

Hmm?

In the case of the NVidia issue, one thing to try migh be to remove the 
current call to "pci_set_power_state(pdev, 3);" in agp_nvidia_suspend() in 
drivers/char/agp/nvidia-agp.c. That sounds like the most likely culprit 
for something that ACPI might want to shut down.

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.

			Linus

---
 drivers/pci/pci-driver.c |   32 +++++++++++++++++++++++++-------
 1 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 6d1a216..6992f73 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -264,6 +264,28 @@ static int pci_device_remove(struct device * dev)
 	return 0;
 }
 
+static void pci_default_suspend(struct pci_dev *dev, pm_message_t state)
+{
+	pci_save_state(dev);
+}
+
+static void pci_default_suspend_late(struct pci_dev *dev, pm_message_t state)
+{
+	/* Something has already suspended it? Never mind then.. */
+	if (dev->current_state != PCI_D0)
+		return;
+
+	/* We avoid powering down bridges by default.. */
+	if (dev->hdr_type == PCI_HEADER_TYPE_NORMAL)
+		pci_set_power_state(dev, PCI_D3hot);
+
+	/*
+	 * mark its power state as "unknown", since we don't know if
+	 * e.g. the BIOS will change its device state when we suspend.
+	 */
+	dev->current_state = PCI_UNKNOWN;
+}
+
 static int pci_device_suspend(struct device * dev, pm_message_t state)
 {
 	struct pci_dev * pci_dev = to_pci_dev(dev);
@@ -274,13 +296,7 @@ static int pci_device_suspend(struct device * dev, pm_message_t state)
 		i = drv->suspend(pci_dev, state);
 		suspend_report_result(drv->suspend, i);
 	} else {
-		pci_save_state(pci_dev);
-		/*
-		 * mark its power state as "unknown", since we don't know if
-		 * e.g. the BIOS will change its device state when we suspend.
-		 */
-		if (pci_dev->current_state == PCI_D0)
-			pci_dev->current_state = PCI_UNKNOWN;
+		pci_default_suspend(pci_dev, state);
 	}
 	return i;
 }
@@ -294,6 +310,8 @@ static int pci_device_suspend_late(struct device * dev, pm_message_t state)
 	if (drv && drv->suspend_late) {
 		i = drv->suspend_late(pci_dev, state);
 		suspend_report_result(drv->suspend_late, i);
+	} else {
+		pci_default_suspend_late(pci_dev, state);
 	}
 	return i;
 }

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Carlos Corbacho]

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.

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

-Carlos

[1] And yes, I double checked the custom DSDT is not loaded this time.
-- 
E-Mail: carlos@strangeworlds.co.uk
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Robert Hancock]

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 hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Carlos Corbacho]

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: carlos@strangeworlds.co.uk
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

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: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Pavel Machek]

Hi!

> On Sunday, 23 of December 2007, Carlos Corbacho wrote:
> > Fix suspend-to-RAM on nForce 4 (CK804) boards by increasing
> > PCIBIOS_MIN_IO.
> > 
> > Fixes kernel bugzilla #9528
> > 
> > Problem:
> > 
> > Linus' patch (52ade9b3b97fd3bea42842a056fe0786c28d0555) to re-order
> > suspend (and fix fall out from Rafael's earlier suspend reordering work)
> > broke suspend-to-RAM on nForce 4 (CK804) boards.
> > 
> > Why:
> > 
> > After debugging _PTS() in the DSDT, it turns out these nVidia boards are
> > trying to write to an IO port > 0x1000 (0x142E) during suspend. Before the
> > re-ordering, we got away with this.
> > 
> > After the afore mentioned commit, we started hitting the PCIBIOS_MIN_IO
> > limit and suspend then broke on these machines (the machine simply hangs
> > when it reaches the 0x142E IO port write during suspend-to-RAM).
> > 
> > There was some previous work in the PCIBIOS_MIN_IO area over two years ago
> > (71db63acff69618b3d9d3114bd061938150e146b) which bumped this to 0x4000,
> > but this was reverted (2ba84684e8cf6f980e4e95a2300f53a505eb794e) after
> > causing new and entirely different problems on another nForce board.
> > 
> > 0x1500 has been picked here as a nice, round and more conservative value
> > than 0x4000, and which covers 0x142E.
> 
> The patch is fine by me, so if anyone has objections, please speak up.

Just.. I have been running with very similar patch for few years... it
fixes few prototype machines I have here.


> > diff --git a/include/asm-x86/pci.h b/include/asm-x86/pci.h
> > index e883619..03cb123 100644
> > --- a/include/asm-x86/pci.h
> > +++ b/include/asm-x86/pci.h
> > @@ -46,7 +46,7 @@ extern unsigned int pcibios_assign_all_busses(void);
> >  #define pcibios_scan_all_fns(a, b)	0
> >  
> >  extern unsigned long pci_mem_start;
> > -#define PCIBIOS_MIN_IO		0x1000
> > +#define PCIBIOS_MIN_IO		0x1500
> >  #define PCIBIOS_MIN_MEM		(pci_mem_start)
> >  
> >  #define PCIBIOS_MIN_CARDBUS_IO	0x4000


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

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Carlos Corbacho]

Pavel,

On Tuesday 25 December 2007 12:12:31 Pavel Machek wrote:
> > The patch is fine by me, so if anyone has objections, please speak up.
>
> Just.. I have been running with very similar patch for few years... it
> fixes few prototype machines I have here.

I've withdrawn the patch since it doesn't actually fix the issue (turns out 
it's actually a bug in Linux's handling of suspend-to-RAM for ACPI 1.0 and 
2.0 systems).

-Carlos
-- 
E-Mail: carlos@strangeworlds.co.uk
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Carlos Corbacho]

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: carlos@strangeworlds.co.uk
Web: strangeworlds.co.uk
GPG Key ID: 0x23EE722D

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:
> 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

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: 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:
> 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

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Robert Hancock]

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 hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Rafael J. Wysocki]

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

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Linus Torvalds]

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

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Robert Hancock]

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 hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM

[Avi Kivity]

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.

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