Suspend code ordering (again) (was: Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM)

Suspend code ordering (again) (was: Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM)

[Rafael J. Wysocki]

On Monday, 24 of December 2007, 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. 
> 
> 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?

Well, as Carlos correctly noticed, the problem is related to the change in
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.

I'm going to prepare patches to implement this idea targeted for the 2.6.25
time frame.

Greetings,
Rafael

Re: Suspend code ordering (again) (was: 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:
>
> 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? And does Windows really do 
that?

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

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. That's when the system is really 
down, interrupts disabled etc, we don't want to call anything but the 
final ACPI "turn us off" stuff there!

			Linus

Re: Suspend code ordering (again) (was: Re: x86: Increase PCIBIOS_MIN_IO to 0x1500 to fix nForce 4 suspend-to-RAM)

[Rafael J. Wysocki]

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.

> That's when the system is really down, interrupts disabled etc, we don't want
> to call anything but the final ACPI "turn us off" stuff there!

OTOH, we ought to be able to put devices into low power states at any time, for
example when they are not used, without any problems and having to put them
back into D0 just in order to execute _PTS doesn't seem very logical to me. ;-)

Greetings,
Rafael

Re: Suspend code ordering (again)

[Alexey Starikovskiy]

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.
>   
Windows was compliant only with 1.x spec until Vista.
With Vista claims are 3.x compliance.

Re: Suspend code ordering (again)

[H. Peter Anvin]

Alexey Starikovskiy wrote:
>>
>> I don't know.
>>   
> Windows was compliant only with 1.x spec until Vista.
> With Vista claims are 3.x compliance.
> 

In other words, the 1.x spec is the only thing that matters, at least in 
the short term (*noone* is giving up XP compatibility at this point.)

	-hpa

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