* RE: [PATCH] Workaround for a PCI restoring bug
[not found] <200705161925.53410.rjw@sisk.pl>
@ 2007-05-17 6:14 ` Brown, Len
2007-05-17 20:50 ` Lukas Hejtmanek
0 siblings, 1 reply; 4+ messages in thread
From: Brown, Len @ 2007-05-17 6:14 UTC (permalink / raw)
To: Rafael J. Wysocki, Linus Torvalds
Cc: Lukas Hejtmanek, Pavel Machek, Andrew Morton, Greg KH, linux-acpi
[-- 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();
^ permalink raw reply related [flat|nested] 4+ messages in thread