RE: [PATCH] Workaround for a PCI restoring bug

RE: [PATCH] Workaround for a PCI restoring bug

[Brown, Len]

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

>> the problem is that saving device state didn't 
>> work, because the device was somehow turned off at suspend 
>> time before we even saved it, so we saved crap.
>> Or more specifically, we saved 
>> all-0xffffffff values from the config space, because the 
>> devices were effectively not there any more! ]
>> 
>> On Wed, 16 May 2007, Lukas Hejtmanek wrote:
>> > 
>> > ...
>> > The problematic code is in drivers/acpi/hardware/hwsleep.c method
>> > acpi_enter_sleep_state_prep.
>> > 
>> > In particular code:
>> > /* Run the _PTS and _GTS methods */
>> > 

>> 
>> Ok, so to fix this bug, we really should do 
>> acpi_enter_sleep_state_prep() 
>> _after_ we have done our own suspend/save of the device information, 
>> rather than before. Because clearly acpi_enter_sleep_state_prep() can

>> corrupt the state wee *want* to save!

Right.

>> Of course, it has to be between the "suspend()" and the 
>> "suspend_late()" 
>> calls, since it wants to run with interrupts enabled and the 
>> system in a 
>> working state, and the final suspend_late() really does end 
>> up shutting 
>> things down and does *not* want to have things in a "working" state.
>> 
>> But that's fine. It is, after all, how suspend/suspend_late 
>> was designed 
>> to work: the "suspend()" part should allocate memory and 
>> save state, the 
>> "suspend_late()" part should actually suspend the device. So 
>> this does fit in the model.

>> - call "device_suspend()" before the "pm_ops->prepare()" function,
and 
>>    then call the "suspend_late()" thing just before the actual
suspend. 
>> 
>>    This *should* work, and I think it's the right thing to do, but
the 
>>    "change order of ACPI calls" is dangerous. Much more dangerous
than I'd 
>>    like. There's no way I can do it before 2.6.22 - it would have to
go 
>>    into 2.6.23-rc1 (and the ugly patch might be a candidate for
2.6.22, 
>>    however much I hate it)

While I know it breaks your own rule for taking risks after rc1,
I don't see a huge down-side to pushing this patch now.
Suspend on Linux has numerous problems on numerous boxes, and this fix
appears to be obvious and important -- so the reward is potentially
large.
It is a small text change to code that is not otherwise changing --
so if it causes a regression before 2.6.22, simply revert it.
Consider that if Rafael had found this test-case box, I'm sure he would
have
shipped you this patch before the rc1 cutoff.

Acked-by: Len Brown <len.brown@intel.com>
If you want somebody to blame:-)

>>    However, the reason I think that's the correct thing to 
>>    do is that on *resume*, we actually already call "pm_finish()"
>>    before we call  "device_resume()", so _logically_ we should do
>>    the suspend in the reverse order too!

Agreed.

>> Len? What do you think? Does the ACPI spec say anything? Do 
>> you know what the other system (the one that BIOS vendors actually
test with) does?

>Well, I've raised exactly the same issue on linux-pm and 
>linux-acpi recently,
>but I haven't had a test case to support my observations.
>
>In fact, we're violating the spec by executing the _PTS and 
>_GTS control methods before suspending devices.

Agreed.  suspend_prepare() has this order wrong.
Linus' patch (attached) looks like it fixes it.

>The spec is clear in these points:
>_PTS should be executed *after* devices are placed in 
>low-power states and
>_GTS should be executed immediately before the sleep state is 
>*entered*.

Yep -- here's the quote from ACPI 3.0b:

OSPM will invoke _GTS, _PTS, _TTS, _WAK, and _BFS in the following
order:
1.OSPM decides (through a policy scheme) to place the system into a
sleeping state.
2._TTS(Sx) is run, where Sx is the desired sleep state to enter.
3. OSPM notifies all native device drivers of the sleep state transition
4._PTS is run
5.OSPM readies system for the sleep state transition
6._GTS is run
7.OSPM writes the sleep vector and the system enters the specified Sx
sleep state.
8.System Wakes up
9._BFS is run
10.OSPM readies system for the return from the sleep state transition
11._WAK is run
12. OSPM notifies all native device drivers of the return from the sleep
state transition
13._TTS(0) is run to indicate the return to the S0 state.

Technically we write the sleep vector too early as well.
And we don't evaluate _TTS at all -- though _TTS was
added only as of ACPI 3.0 and I've not seen it implemented on
any of the systems I've got.

>Thus I think the second patch is a better thing to do long 
>term.  Moreover, I'd
>also move the execution of _GTS to after disable_nonboot_cpus()

I'd agree to that on the grounds of symmetry.
However, I expect it will be a NOP -- since the BIOS
can't tell the difference between an enabled and a non-enabled CPU.
(The possible exception is if any BIOS hooks -- such as magic traps
 into SMM -- assume that they're run in cpu0).

In the resume case, the order is important.  Per our discussion
on this last November, _WAK must follow _INIT of the non boot cpus
so that it can patch up stuff in firmware that got cleared on reset.
This continues to be true in Linux.

>(and, symmetrically, the execution of _BFS to before 
>enable_nonboot_cpus(), which
>also would be more in line with the spec, but that's a separate issue).

True, _BFS is probably later than the spec wants.
But like _GTS, it was added with ACPI 2.0 and it is optional
for a BIOS to implement it.  Poking around, I don't see any systems
that actually implement _BFS or _GTS.  So that tweak probably isn't
a big deal like the problem at hand.

thanks,
-Len

[-- Attachment #2: diff-better --]
[-- Type: application/octet-stream, Size: 951 bytes --]

 kernel/power/main.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/kernel/power/main.c b/kernel/power/main.c
index 40d56a3..b98b80c 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -97,25 +97,26 @@ static int suspend_prepare(suspend_state_t state)
 		}
 	}
 
-	if (pm_ops->prepare) {
-		if ((error = pm_ops->prepare(state)))
-			goto Thaw;
-	}
-
 	suspend_console();
 	error = device_suspend(PMSG_SUSPEND);
 	if (error) {
 		printk(KERN_ERR "Some devices failed to suspend\n");
-		goto Resume_devices;
+		goto Resume_console;
 	}
+	if (pm_ops->prepare) {
+		if ((error = pm_ops->prepare(state)))
+			goto Resume_devices;
+	}
+
 	error = disable_nonboot_cpus();
 	if (!error)
 		return 0;
 
 	enable_nonboot_cpus();
- Resume_devices:
 	pm_finish(state);
+ Resume_devices:
 	device_resume();
+ Resume_console:
 	resume_console();
  Thaw:
 	thaw_processes();

Re: [PATCH] Workaround for a PCI restoring bug

[Lukas Hejtmanek]

> Yep -- here's the quote from ACPI 3.0b:
> 
> OSPM will invoke _GTS, _PTS, _TTS, _WAK, and _BFS in the following
> order:
> 1.OSPM decides (through a policy scheme) to place the system into a
> sleeping state.
> 2._TTS(Sx) is run, where Sx is the desired sleep state to enter.
> 3. OSPM notifies all native device drivers of the sleep state transition
> 4._PTS is run
> 5.OSPM readies system for the sleep state transition
> 6._GTS is run
> 7.OSPM writes the sleep vector and the system enters the specified Sx
> sleep state.
> 8.System Wakes up
> 9._BFS is run
> 10.OSPM readies system for the return from the sleep state transition
> 11._WAK is run
> 12. OSPM notifies all native device drivers of the return from the sleep
> state transition
> 13._TTS(0) is run to indicate the return to the S0 state.
> 
> Technically we write the sleep vector too early as well.
> And we don't evaluate _TTS at all -- though _TTS was
> added only as of ACPI 3.0 and I've not seen it implemented on
> any of the systems I've got.

However, we still do something wrong with ACPI and suspend-to-ram because my
system is unable to reboot after resume from ram. The only way to reboot is to
disable the ACPI in the machine_emergency_restart(). (in particular, keyboard
LEDs flash as the reset has been issued but the BIOS logo does not appear.)

Any ideas, Len?

Andrew dislikes the patch resulted from this bug report:
http://bugzilla.kernel.org/show_bug.cgi?id=6655

Anyway, it works for me pretty well.

-- 
Lukáš Hejtmánek
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Workaround for a PCI restoring bug

[Rafael J. Wysocki]

On Thursday, 17 May 2007 22:50, Lukas Hejtmanek wrote:
> > Yep -- here's the quote from ACPI 3.0b:
> > 
> > OSPM will invoke _GTS, _PTS, _TTS, _WAK, and _BFS in the following
> > order:
> > 1.OSPM decides (through a policy scheme) to place the system into a
> > sleeping state.
> > 2._TTS(Sx) is run, where Sx is the desired sleep state to enter.
> > 3. OSPM notifies all native device drivers of the sleep state transition
> > 4._PTS is run
> > 5.OSPM readies system for the sleep state transition
> > 6._GTS is run
> > 7.OSPM writes the sleep vector and the system enters the specified Sx
> > sleep state.
> > 8.System Wakes up
> > 9._BFS is run
> > 10.OSPM readies system for the return from the sleep state transition
> > 11._WAK is run
> > 12. OSPM notifies all native device drivers of the return from the sleep
> > state transition
> > 13._TTS(0) is run to indicate the return to the S0 state.
> > 
> > Technically we write the sleep vector too early as well.
> > And we don't evaluate _TTS at all -- though _TTS was
> > added only as of ACPI 3.0 and I've not seen it implemented on
> > any of the systems I've got.
> 
> However, we still do something wrong with ACPI and suspend-to-ram because my
> system is unable to reboot after resume from ram. The only way to reboot is to
> disable the ACPI in the machine_emergency_restart(). (in particular, keyboard
> LEDs flash as the reset has been issued but the BIOS logo does not appear.)

Can you please send me your DSDT?

Rafael

Re: [PATCH] Workaround for a PCI restoring bug

[Lukas Hejtmanek]

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

On Thu, May 17, 2007 at 11:25:05PM +0200, Rafael J. Wysocki wrote:
> > However, we still do something wrong with ACPI and suspend-to-ram because
> > my system is unable to reboot after resume from ram. The only way to
> > reboot is to disable the ACPI in the machine_emergency_restart(). (in
> > particular, keyboard LEDs flash as the reset has been issued but the BIOS
> > logo does not appear.)
> 
> Can you please send me your DSDT?

it's attached. 

-- 
Lukáš Hejtmánek

[-- Attachment #2: dsdt.gz --]
[-- Type: application/octet-stream, Size: 13773 bytes --]