On Monday 20 September 2004 12:28 pm, Alan Stern wrote:
> > And there are others. Some of those fixes are in my patchset.
>
> Would you care to post your fixes?
Here you go! I think you mentioned a couple issues I didn't address
here, by the way -- haven't had a chance to look yet.
> > .... I'd
> > rather have USB behaving better -- notably, integrated so that it works
> > OK with system-wide suspend not just USB suspend/wake/resume -- before
> > any of that starts.
>
> I think that should be very doable, and it shouldn't even need very much
> work -- apart from the root hub implementations (subject of a different
> thread).
I think I'll disagree about "much work" given the testing requirements,
but if you measure in lines of code, this patch isn't very large and it
gets a bunch of the problems solved. OHCI was mostly behaving...
- Dave
This patch has various updates that seem to help make USB_SUSPEND
integrate better with suspend-to-disk (swsusp). The whole thing
doesn't work right yet; khubd's ignoring the root hub after resuming
from power-off.
PMCORE:
- fix the state numbering problem
- fail attempts to suspend/resume devices with a suspended parent
- filter some superfluous suspend calls
SWSUSP:
- use symbolic names for device/system/suspend/??? state
USBCORE:
- refuse some suspend paths.
- "legacy" PCI devices will fake pci_set_power_state() effects
- handle "deeper" suspends in case anyone tries
Notice how many states are associated with root hubs: the hub interface
state, the hub state, the PCI device state, the HCD state, and the driver
model power_state. Did I miss one?
Probably all the HCDs need changes here. I've tested with OHCI, and
have some EHCI patches too.
--- 1.14/drivers/base/power/resume.c Wed Jun 9 23:34:24 2004
+++ edited/drivers/base/power/resume.c Mon Sep 20 20:10:59 2004
@@ -22,6 +22,14 @@
int resume_device(struct device * dev)
{
+ if (dev->power.pm_parent
+ && dev->power.pm_parent->power.power_state) {
+ dev_err(dev, "PM: resume from %d, parent %s still %d\n",
+ dev->power.power_state,
+ dev->power.pm_parent->bus_id,
+ dev->power.pm_parent->power.power_state);
+ return -EIO;
+ }
if (dev->bus && dev->bus->resume)
return dev->bus->resume(dev);
return 0;
--- 1.16/drivers/base/power/suspend.c Wed Jun 9 23:34:24 2004
+++ edited/drivers/base/power/suspend.c Mon Sep 20 20:10:59 2004
@@ -39,6 +39,20 @@
{
int error = 0;
+ if (dev->power.power_state >= state) {
+ dev_dbg(dev, "PM: suspend %d-->%d\n",
+ dev->power.power_state, state);
+ return 0;
+ }
+ if (dev->power.pm_parent
+ && dev->power.pm_parent->power.power_state) {
+ dev_err(dev,
+ "PM: suspend %d->%d, parent %s already %d\n",
+ dev->power.power_state, state,
+ dev->power.pm_parent->bus_id,
+ dev->power.pm_parent->power.power_state);
+ return -EDEADLK;
+ }
dev_dbg(dev, "suspending\n");
dev->power.prev_state = dev->power.power_state;
--- 1.36/drivers/usb/core/hcd-pci.c Wed Sep 15 11:51:43 2004
+++ edited/drivers/usb/core/hcd-pci.c Mon Sep 20 20:19:19 2004
@@ -268,6 +268,18 @@
#ifdef CONFIG_PM
+static char __attribute_used__ *pci_state(u32 state)
+{
+ switch (state) {
+ case 0: return "D0";
+ case 1: return "D1";
+ case 2: return "D2";
+ case 3: return "D3hot";
+ case 4: return "D3cold";
+ }
+ return NULL;
+}
+
/**
* usb_hcd_pci_suspend - power management suspend of a PCI-based HCD
* @dev: USB Host Controller being suspended
@@ -288,16 +300,34 @@
* PM-sensitive HCDs may already have done this.
*/
has_pci_pm = pci_find_capability(dev, PCI_CAP_ID_PM);
- if (has_pci_pm)
- dev_dbg(hcd->self.controller, "suspend D%d --> D%d\n",
- dev->current_state, state);
+ if (state > 4)
+ state = 4;
switch (hcd->state) {
case USB_STATE_HALT:
dev_dbg (hcd->self.controller, "halted; hcd not suspended\n");
break;
case HCD_STATE_SUSPENDED:
- dev_dbg (hcd->self.controller, "hcd already suspended\n");
+ dev_dbg (hcd->self.controller, "PCI %s --> %s\n",
+ pci_state(dev->current_state),
+ pci_state(state));
+ if (state > 3)
+ state = 3;
+
+ if (state == dev->current_state)
+ break;
+ else if (state < dev->current_state)
+ retval = -EIO;
+ else if (has_pci_pm)
+ retval = pci_set_power_state (dev, state);
+ else
+ dev->current_state = state;
+
+ if (retval == 0)
+ dev->dev.power.power_state = state;
+ else
+ dev_dbg (hcd->self.controller,
+ "re-suspend fail, %d\n", retval);
break;
default:
retval = hcd->driver->suspend (hcd, state);
@@ -308,22 +338,48 @@
else {
hcd->state = HCD_STATE_SUSPENDED;
pci_save_state (dev, hcd->pci_state);
-#ifdef CONFIG_USB_SUSPEND
- pci_enable_wake (dev, state, hcd->remote_wakeup);
- pci_enable_wake (dev, 4, hcd->remote_wakeup);
-#endif
+
/* no DMA or IRQs except in D0 */
pci_disable_device (dev);
free_irq (hcd->irq, hcd);
- if (has_pci_pm)
+ if (has_pci_pm) {
retval = pci_set_power_state (dev, state);
- dev->dev.power.power_state = state;
+
+ /* POLICY: ignore D1/D2/D3hot differences;
+ * we know D3hot will always work.
+ */
+ if (retval < 0 && state < 3) {
+ retval = pci_set_power_state (dev, 3);
+ if (retval == 0)
+ state = 3;
+ }
+ if (retval == 0) {
+ dev->dev.power.power_state = state;
+#ifdef CONFIG_USB_SUSPEND
+ pci_enable_wake (dev, state,
+ hcd->remote_wakeup);
+ pci_enable_wake (dev, 4,
+ hcd->remote_wakeup);
+#endif
+ }
+ } else {
+ if (state > 3)
+ state = 3;
+ dev->current_state = state;
+ dev->dev.power.power_state = state;
+ }
if (retval < 0) {
dev_dbg (&dev->dev,
- "PCI suspend fail, %d\n",
+ "PCI %s suspend fail, %d\n",
+ pci_state(state),
retval);
(void) usb_hcd_pci_resume (dev);
+ } else {
+ dev_dbg(hcd->self.controller,
+ "suspended to PCI %s%s\n",
+ pci_state(state),
+ has_pci_pm ? " (PM)" : "");
}
}
}
@@ -345,9 +401,10 @@
hcd = pci_get_drvdata(dev);
has_pci_pm = pci_find_capability(dev, PCI_CAP_ID_PM);
- if (has_pci_pm)
- dev_dbg(hcd->self.controller, "resume from state D%d\n",
- dev->current_state);
+
+ /* D3cold resume isn't usually reported this way... */
+ dev_dbg(hcd->self.controller, "resume from PCI %s\n",
+ pci_state(dev->current_state));
if (hcd->state != HCD_STATE_SUSPENDED) {
dev_dbg (hcd->self.controller,
@@ -358,6 +415,8 @@
if (has_pci_pm)
pci_set_power_state (dev, 0);
+ else
+ dev->current_state = 0;
dev->dev.power.power_state = 0;
retval = request_irq (dev->irq, usb_hcd_irq, SA_SHIRQ,
hcd->description, hcd);
--- 1.97/drivers/usb/core/hcd.c Mon Sep 13 04:47:16 2004
+++ edited/drivers/usb/core/hcd.c Mon Sep 20 20:10:32 2004
@@ -540,7 +540,7 @@
urb->actual_length = length;
urb->status = 0;
urb->hcpriv = NULL;
- } else
+ } else if (!urb->dev->dev.power.power_state)
mod_timer (&hcd->rh_timer, jiffies + HZ/4);
spin_unlock (&hcd_data_lock);
spin_unlock (&urb->lock);
--- 1.138/drivers/usb/core/hub.c Mon Sep 13 05:12:00 2004
+++ edited/drivers/usb/core/hub.c Mon Sep 20 20:10:32 2004
@@ -1487,14 +1487,13 @@
{
int status;
- /* caller owns the udev device lock */
if (port < 0)
return port;
- if (state <= udev->dev.power.power_state
- || state < PM_SUSPEND_MEM
- || udev->state == USB_STATE_SUSPENDED
+ /* NOTE: udev->serialize released on all real returns! */
+ if (udev->dev.power.power_state
|| udev->state == USB_STATE_NOTATTACHED) {
+ up(&udev->serialize);
return 0;
}
@@ -1557,7 +1556,6 @@
*/
if (state > PM_SUSPEND_MEM) {
dev_warn(&udev->dev, "no poweroff yet, suspending instead\n");
- state = PM_SUSPEND_MEM;
}
/* "global suspend" of the HC-to-USB interface (root hub), or
@@ -1861,6 +1859,9 @@
struct usb_hub *hub = usb_get_intfdata (intf);
unsigned port;
int status;
+
+ if (!intf->dev.power_state)
+ return 0;
for (port = 0; port < hdev->maxchild; port++) {
struct usb_device *udev;
--- 1.176/drivers/usb/core/usb.c Mon Sep 13 05:11:34 2004
+++ edited/drivers/usb/core/usb.c Mon Sep 20 20:10:32 2004
@@ -1402,6 +1402,10 @@
struct usb_interface *intf;
struct usb_driver *driver;
+ /* there's only one USB suspend state */
+ if (dev->power.power_state)
+ return 0;
+
if (dev->driver == &usb_generic_driver)
return usb_suspend_device (to_usb_device(dev), state);
--- 1.16/include/linux/pm.h Thu Jul 1 22:23:53 2004
+++ edited/include/linux/pm.h Mon Sep 20 20:10:59 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 Mon Sep 20 20:10:59 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 Mon Sep 20 20:10:59 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");
