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