Workaround for a PCI restoring bug

PATCH Brown, Len 3 participants 4 messages May 17, 2007 1 message not in the archive ACPI

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Brown, Len

author Acked-by

2007-05-17 06:14 UTC #1 lore raw

Quoted 18 lines

Quoted 18 lines
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! ]
Quoted 8 lines · Lukas Hejtmanek
...
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 */

Quoted 6 lines

Quoted 6 lines
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.

Quoted 8 lines

Quoted 8 lines
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.

Quoted 8 lines

Quoted 8 lines
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

Quoted 1 line

Quoted 1 line
then call the "suspend_late()" thing just before the actual

suspend.

Quoted 2 lines

Quoted 2 lines
This *should* work, and I think it's the right thing to do, but

the

Quoted 1 line

Quoted 1 line
"change order of ACPI calls" is dangerous. Much more dangerous

than I'd

Quoted 1 line

Quoted 1 line
like. There's no way I can do it before 2.6.22 - it would have to

Quoted 1 line

Quoted 1 line
into 2.6.23-rc1 (and the ugly patch might be a candidate for

2.6.22,

Quoted 1 line

Quoted 1 line
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 <[email protected]>

If you want somebody to blame:-)

Quoted 4 lines

Quoted 4 lines
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.

Quoted 2 lines

Quoted 2 lines
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?

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

(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

Lukas Hejtmanek

2007-05-17 20:52 UTC #2 lore raw

Quoted 21 lines

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.

Quoted 4 lines

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.

Signature · 5 lines

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

Rafael J. Wysocki

2007-05-17 21:20 UTC #3 lore raw

Quoted 26 lines · Lukas Hejtmanek

Quoted 21 lines
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.
Quoted 4 lines
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.

Quoted 4 lines

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

Lukas Hejtmanek

2007-05-17 22:26 UTC #4 lore raw

Quoted 7 lines · Rafael J. Wysocki

Quoted 5 lines
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.

Signature · 1 line

Lukáš Hejtmánek