On Saturday 25 September 2004 9:23 am, Alan Stern wrote:
>
> On Fri, 24 Sep 2004, Pavel Machek wrote:
>
[ in email that didn't make it to the list, or at least to me ... ]
> > > The meaning of the power states is unclear, particularly for
> > > buses that don't support all the PCI states.
> >
> > Well, in suspend I'm currently only using 0/3, where 0 is on, and 3 is
> > suspended.
You should be using PM_SUSPEND_ON and PM_SUSPEND_DISK,
not 0/3 ... PM_SUSPEND_MEM is used in other parts of suspend.
See the attached patch, resolving a pmcore abuse of the
PCI selective suspend routines ...
> > What should be done is convert that interface to enums or
> > some kind of structure, probably solving per-device suspend at the
> > same time. I'd prefer drivers to ignore that parameter for now.
Too late for that. PCI drivers have been using it since 2.4 ... though
not all of them. Just enough to make lots of trouble. :)
> Although the picture isn't really clear, I gather the device states one
> really needs to consider could be described something like this (using an
> ad hoc notation):
These states don't correspond to PCI or ACPI states, so it'd
be clearer to talk using names that can't be confused with
either of them (like ACPI S1) ...
In general I think the PM core has to firmly accept the notion
of driver-specified device power states ... as things that are
distinct from system power states, and that drivers sometimes
use to coordinate with each other.
> S0' is a device's initial unconfigured state.
That'd be hardware-specific. Some initialize in your S2', and there can
be multiple analogues of PME# and power (like autogated clocks).
Which means lots more states in some devices than this S0/S1/S2/...
> S0 is the normal
> operational state. Selective suspend would use state S1. Suspend-to-RAM
> and suspend-to-disk would use S2 for devices that support PME#, otherwise
> S2'. Perhaps there's no need to distinguish between the two; I don't know
> how closely people want to tie remote wakeup enable/disable with power
> states, although they are related concepts.
They're coupled in that wakeup makes no sense without some kind of
suspend ... and a usable "selective suspend" has to account for devices
waking up out of low-power states (just like "system suspend", except
of course only waking up part of the system).
PME# is wierd though, since at least in current Linux it's only relevant
for _system_ wakeup.
Which makes it useless for systems that want to leave devices in
selective suspend almost all the time, since only devices in PCI D0
can issue IRQs, yet deeply suspended devices can only issue PME#.
- Dave
Changes the PM_SUSPEND_MEM (and PM_SUSPEND_DISK) enum values so that
they make sense as PCI device power states.
(a) Fixes bugs whereby PCI drivers are being given bogus values.
The should resolve OSDL bugid 2886 without changing the PCI
API (its PM calls still act as on 2.4 kernels).
(b) Doesn't change the awkward assumption in the 2.6 PMcore that
the /sys/bus/*/devices/power/state, /proc/acpi/sleep,
dev->power.power_state, and dev->detach_state files share
the same numeric codes ... even for busses very unlike PCI,
or systems with several "on" policies as well as STD and STR.
Really we need to move away from "u32" codes that are easily confused
with each other, towards typed values (probably struct pointers), but
this is the simplest comprehensive fix for the PCI problem.
Signed-off-by: David Brownell <[EMAIL PROTECTED]>
--- 1.16/include/linux/pm.h Thu Jul 1 22:23:53 2004
+++ edited/include/linux/pm.h Wed Sep 8 07:54:20 2004
@@ -194,11 +194,12 @@
extern void (*pm_power_off)(void);
enum {
- PM_SUSPEND_ON,
- PM_SUSPEND_STANDBY,
- PM_SUSPEND_MEM,
- PM_SUSPEND_DISK,
- PM_SUSPEND_MAX,
+ PM_SUSPEND_ON = 0,
+ PM_SUSPEND_STANDBY = 1,
+ /* NOTE: PM_SUSPEND_MEM == PCI_D3hot */
+ PM_SUSPEND_MEM = 3,
+ PM_SUSPEND_DISK = 4,
+ PM_SUSPEND_MAX = 5,
};
enum {
--- 1.17/kernel/power/main.c Mon Apr 12 10:54:21 2004
+++ edited/kernel/power/main.c Wed Sep 8 07:54:20 2004
@@ -225,8 +225,8 @@
p = memchr(buf, '\n', n);
len = p ? p - buf : n;
- for (s = &pm_states[state]; *s; s++, state++) {
- if (!strncmp(buf, *s, len))
+ for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
+ if (*s && !strncmp(buf, *s, len))
break;
}
if (*s)
--- 1.86/kernel/power/swsusp.c Thu Jul 1 22:23:48 2004
+++ edited/kernel/power/swsusp.c Wed Sep 8 07:54:20 2004
@@ -699,7 +699,7 @@
else
#endif
{
- device_suspend(3);
+ device_suspend(PM_SUSPEND_DISK);
device_shutdown();
machine_power_off();
}
@@ -720,7 +720,7 @@
mb();
spin_lock_irq(&suspend_pagedir_lock); /* Done to disable interrupts */
- device_power_down(3);
+ device_power_down(PM_SUSPEND_DISK);
PRINTK( "Waiting for DMAs to settle down...\n");
mdelay(1000); /* We do not want some readahead with DMA to corrupt our memory, right?
Do it with disabled interrupts for best effect. That way, if some
@@ -789,7 +789,7 @@
{
int is_problem;
read_swapfiles();
- device_power_down(3);
+ device_power_down(PM_SUSPEND_DISK);
is_problem = suspend_prepare_image();
device_power_up();
spin_unlock_irq(&suspend_pagedir_lock);
@@ -844,8 +844,8 @@
free_some_memory();
disable_nonboot_cpus();
/* Save state of all device drivers, and stop them. */
- printk("Suspending devices... ");
- if ((res = device_suspend(3))==0) {
+ printk("Suspending devices...\n");
+ if ((res = device_suspend(PM_SUSPEND_DISK))==0) {
/* If stopping device drivers worked, we proceed basically into
* suspend_save_image.
*
@@ -1200,7 +1200,7 @@
goto read_failure;
/* FIXME: Should we stop processes here, just to be safer? */
disable_nonboot_cpus();
- device_suspend(3);
+ device_suspend(PM_SUSPEND_DISK);
do_magic(1);
panic("This never returns");
