On Friday 22 April 2005 11:52 am, Alan Stern wrote:
> David:
>
> Please take a look at this proposed patch and see what you think. It
> removes the recursion from the usbcore suspend/resume routines.
Well, as I said it resembled one I'd been working on. I merged what
I think are the best parts of the two patches, and limited it to
suspend side changes; here it is.
Since the only remaining point of that second usb_suspend_device()
parameter was to support the recursion, I removed it. By analogy
to pci_set_power_state(), it doesn't need the parameter ... USB
hardware only has one "suspend" state, and resume() is separate.
Also the software-only state (interface->dev.power.power_state)
stays as SUSPENDED whenever the driver is unbound, simplifying
the logic to test when a device can be suspended and getting
ready to kick in device-level autosuspend.
> There's
> just a couple of small exceptions; the big one is that resuming a device
> will cause all its interfaces to be resumed as well, and the small one is
> that before suspending a device all the interfaces are checked to make
> sure they are already suspended.
The former is basically "keep current behavior"; in this patch I took
it a step further and just didn't touch the resume path. (Later!)
The latter was something I'd done too.
I've tested this only lightly since merging things from your patch,
and there are various "should not matter" things I removed from the
version I'd been working on, but this version is similar to your patch,
or what's there now, that I don't think it'll make trouble.
- Dave
This is a net code shrink, mostly affecting CONFIG_USB_SUSPEND code:
* When suspending a device, insist all its interfaces already be
suspended rather than trying to suspend them directly. That is,
remove recursion during device-level suspend. (This is an issue
pmcore should handle; it only does so for whole-system suspend.)
* Generic USB suspend code now has the more featureful code to
suspend interfaces from usb_suspend_device() ... verifying that
any suspend method behaves, and issuing warnings for drivers that
don't yet provide suspend methods. (Eventually the warning should
be removed, once that pmcore self-deadlock is removed.)
* Generic hub suspend code now just insists that child devices be
already suspended. So do the PCI suspend paths for EHCI and OHCI.
* Get rid of the now-pointless extra parameter to usb_suspend_device().
Unlike pci_set_power_state(), there's no need for a parameter:
USB hardware only has one suspend state. If the hardware isn't
supposed to suspend, don't call this routine. Later we can define
some separate routine to power down a device's port.
* Unbound interfaces are always marked as suspended, since there's
no driver to handshake with about shutting it down and in any
case this never relates to hardware state.
Accordingly, these suspend paths must now return error codes in cases
where preconditions like "children are already suspended" fail. This
means suspend requests through sysfs won't "just work" any more; some
agent must manually do the recursion (hoping wakeups don't kick in).
These changes are not visible without CONFIG_USB_SUSPEND, except for
the warnings for drivers without suspend() methods.
(This incorporates pieces from a related patch from Alan Stern.)
Signed-off-by: David Brownell <[EMAIL PROTECTED]>
--- g26.orig/drivers/usb/core/hub.c 2005-05-09 18:24:35.000000000 -0700
+++ g26/drivers/usb/core/hub.c 2005-05-09 18:29:52.000000000 -0700
@@ -452,6 +452,7 @@
{
/* stop khubd and related activity */
hub->quiescing = 1;
+ hub->activating = 0;
usb_kill_urb(hub->urb);
if (hub->has_indicators)
cancel_delayed_work(&hub->leds);
@@ -1243,10 +1244,9 @@
*/
if (udev->bus->b_hnp_enable || udev->bus->is_b_host) {
static int __usb_suspend_device (struct usb_device *,
- int port1, pm_message_t state);
+ int port1);
err = __usb_suspend_device(udev,
- udev->bus->otg_port,
- PMSG_SUSPEND);
+ udev->bus->otg_port);
if (err < 0)
dev_dbg(&udev->dev, "HNP fail, %d\n", err);
}
@@ -1452,7 +1452,7 @@
/* FIXME let caller ask to power down the port:
* - some devices won't enumerate without a VBUS power cycle
* - SRP saves power that way
- * - usb_suspend_device(dev, PMSG_SUSPEND)
+ * - ... new call, TBD ...
* That's easy if this hub can switch power per-port, and
* khubd reactivates the port later (timer, SRP, etc).
* Powerdown must be optional, because of reset/DFU.
@@ -1539,8 +1539,7 @@
* Linux (2.6) currently has NO mechanisms to initiate that: no khubd
* timer, no SRP, no requests through sysfs.
*/
-static int __usb_suspend_device (struct usb_device *udev, int port1,
- pm_message_t state)
+static int __usb_suspend_device (struct usb_device *udev, int port1)
{
int status;
@@ -1553,67 +1552,23 @@
return 0;
}
- /* suspend interface drivers; if this is a hub, it
- * suspends the child devices
- */
+ /* insist all interfaces are already suspended */
if (udev->actconfig) {
int i;
for (i = 0; i < udev->actconfig->desc.bNumInterfaces; i++) {
struct usb_interface *intf;
- struct usb_driver *driver;
intf = udev->actconfig->interface[i];
- if (state <= intf->dev.power.power_state)
- continue;
if (!intf->dev.driver)
continue;
- driver = to_usb_driver(intf->dev.driver);
-
- if (driver->suspend) {
- status = driver->suspend(intf, state);
- if (intf->dev.power.power_state != state
- || status)
- dev_err(&intf->dev,
- "suspend %d fail, code %d\n",
- state, status);
- }
-
- /* only drivers with suspend() can ever resume();
- * and after power loss, even they won't.
- * bus_rescan_devices() can rebind drivers later.
- *
- * FIXME the PM core self-deadlocks when unbinding
- * drivers during suspend/resume ... everything grabs
- * dpm_sem (not a spinlock, ugh). we want to unbind,
- * since we know every driver's probe/disconnect works
- * even for drivers that can't suspend.
- */
- if (!driver->suspend || state > PM_SUSPEND_MEM) {
-#if 1
- dev_warn(&intf->dev, "resume is unsafe!\n");
-#else
- down_write(&usb_bus_type.rwsem);
- device_release_driver(&intf->dev);
- up_write(&usb_bus_type.rwsem);
-#endif
+ if (intf->dev.power.power_state == PMSG_ON) {
+ dev_dbg(&intf->dev, "nyet suspended\n");
+ return -EBUSY;
}
}
}
- /*
- * FIXME this needs port power off call paths too, to help force
- * USB into the "generic" PM model. At least for devices on
- * ports that aren't using ganged switching (usually root hubs).
- *
- * NOTE: SRP-capable links should adopt more aggressive poweroff
- * policies (when HNP doesn't apply) once we have mechanisms to
- * turn power back on! (Likely not before 2.7...)
- */
- if (state > PM_SUSPEND_MEM) {
- dev_warn(&udev->dev, "no poweroff yet, suspending instead\n");
- }
-
/* "global suspend" of the HC-to-USB interface (root hub), or
* "selective suspend" of just one hub-device link.
*/
@@ -1633,14 +1588,13 @@
udev);
if (status == 0)
- udev->dev.power.power_state = state;
+ udev->dev.power.power_state = PMSG_SUSPEND;
return status;
}
/**
* usb_suspend_device - suspend a usb device
* @udev: device that's no longer in active use
- * @state: PMSG_SUSPEND to suspend
* Context: must be able to sleep; device not locked
*
* Suspends a USB device that isn't in active use, conserving power.
@@ -1649,13 +1603,16 @@
* suspend by the host, using usb_resume_device(). It's also routine
* to disconnect devices while they are suspended.
*
+ * This only affects the USB hardware for a device; its interfaces
+ * (and, for hubs, child devices) must already have been suspended.
+ *
* Suspending OTG devices may trigger HNP, if that's been enabled
* between a pair of dual-role devices. That will change roles, such
* as from A-Host to A-Peripheral or from B-Host back to B-Peripheral.
*
* Returns 0 on success, else negative errno.
*/
-int usb_suspend_device(struct usb_device *udev, pm_message_t state)
+int usb_suspend_device(struct usb_device *udev)
{
int port1, status;
@@ -1663,7 +1620,7 @@
if (port1 < 0)
return port1;
- status = __usb_suspend_device(udev, port1, state);
+ status = __usb_suspend_device(udev, port1);
usb_unlock_device(udev);
return status;
}
@@ -1892,32 +1849,26 @@
return status;
}
-static int hub_suspend(struct usb_interface *intf, pm_message_t state)
+static int hub_suspend(struct usb_interface *intf, pm_message_t message)
{
struct usb_hub *hub = usb_get_intfdata (intf);
struct usb_device *hdev = hub->hdev;
unsigned port1;
- int status;
-
- /* stop khubd and related activity */
- hub_quiesce(hub);
- /* then suspend every port */
+ /* fail if children aren't already suspended */
for (port1 = 1; port1 <= hdev->maxchild; port1++) {
struct usb_device *udev;
udev = hdev->children [port1-1];
- if (!udev)
- continue;
- down(&udev->serialize);
- status = __usb_suspend_device(udev, port1, state);
- up(&udev->serialize);
- if (status < 0)
- dev_dbg(&intf->dev, "suspend port %d --> %d\n",
- port1, status);
+ if (udev && udev->state != USB_STATE_SUSPENDED) {
+ dev_dbg(&intf->dev, "port %d nyet suspended\n", port1);
+ return -EBUSY;
+ }
}
- intf->dev.power.power_state = state;
+ /* stop khubd and related activity */
+ hub_quiesce(hub);
+ intf->dev.power.power_state = PMSG_SUSPEND;
return 0;
}
@@ -1973,7 +1924,7 @@
#else /* !CONFIG_USB_SUSPEND */
-int usb_suspend_device(struct usb_device *udev, pm_message_t state)
+int usb_suspend_device(struct usb_device *udev)
{
return 0;
}
--- g26.orig/drivers/usb/core/message.c 2005-05-09 17:55:14.000000000 -0700
+++ g26/drivers/usb/core/message.c 2005-05-09 18:25:57.000000000 -0700
@@ -1391,6 +1391,7 @@
intf->dev.dma_mask = dev->dev.dma_mask;
intf->dev.release = release_interface;
device_initialize (&intf->dev);
+ intf->dev.power.power_state = PMSG_SUSPEND;
sprintf (&intf->dev.bus_id[0], "%d-%s:%d.%d",
dev->bus->busnum, dev->devpath,
configuration,
--- g26.orig/drivers/usb/core/usb.c 2005-05-09 17:55:14.000000000 -0700
+++ g26/drivers/usb/core/usb.c 2005-05-09 18:25:57.000000000 -0700
@@ -90,17 +90,29 @@
if (!driver->probe)
return error;
- /* FIXME we'd much prefer to just resume it ... */
- if (interface_to_usbdev(intf)->state == USB_STATE_SUSPENDED)
- return -EHOSTUNREACH;
id = usb_match_id (intf, driver->id_table);
if (id) {
dev_dbg (dev, "%s - got id\n", __FUNCTION__);
+
+ /* Interface "power state" doesn't correspond to any
+ * hardware state whatsoever. The software state is
+ * meaningful only when there's a driver to consult
+ * while suspending the hardware (usb_suspend_device).
+ *
+ * So unbound devices are always marked as suspended,
+ * no matter what state driver disconnect() left, and
+ * the rule for suspending hardware is simple: all
+ * children (interfaces) must be suspended beforehand.
+ */
+ intf->dev.power.power_state = PMSG_ON;
intf->condition = USB_INTERFACE_BINDING;
error = driver->probe (intf, id);
- intf->condition = error ? USB_INTERFACE_UNBOUND :
- USB_INTERFACE_BOUND;
+ if (error) {
+ intf->dev.power.power_state = PMSG_SUSPEND;
+ intf->condition = USB_INTERFACE_UNBOUND;
+ } else
+ intf->condition = USB_INTERFACE_BOUND;
}
return error;
@@ -126,6 +138,7 @@
0);
usb_set_intfdata(intf, NULL);
intf->condition = USB_INTERFACE_UNBOUND;
+ intf->dev.power.power_state = PMSG_SUSPEND;
return 0;
}
@@ -289,6 +302,7 @@
dev->driver = &driver->driver;
usb_set_intfdata(iface, priv);
iface->condition = USB_INTERFACE_BOUND;
+ iface->dev.power.power_state = PMSG_ON;
/* if interface was already added, bind now; else let
* the future device_add() bind it, bypassing probe()
@@ -329,6 +343,7 @@
dev->driver = NULL;
usb_set_intfdata(iface, NULL);
iface->condition = USB_INTERFACE_UNBOUND;
+ iface->dev.power.power_state = PMSG_SUSPEND;
}
/**
@@ -1375,26 +1390,51 @@
static int usb_generic_suspend(struct device *dev, pm_message_t message)
{
- struct usb_interface *intf;
- struct usb_driver *driver;
+ struct usb_interface *intf;
+ struct usb_driver *driver;
+ int status = 0;
if (dev->driver == &usb_generic_driver)
- return usb_suspend_device (to_usb_device(dev), message);
+ return usb_suspend_device (to_usb_device(dev));
- if ((dev->driver == NULL) ||
- (dev->driver_data == &usb_generic_driver_data))
- return 0;
+ if (dev->driver == NULL)
+ return status;
intf = to_usb_interface(dev);
driver = to_usb_driver(dev->driver);
/* there's only one USB suspend state */
if (intf->dev.power.power_state)
- return 0;
+ return status;
- if (driver->suspend)
- return driver->suspend(intf, message);
- return 0;
+ if (driver->suspend) {
+ status = driver->suspend(intf, message);
+ if (intf->dev.power.power_state != message && !status)
+ status = -EBADMSG;
+ if (status)
+ dev_err(&intf->dev, "suspend %d fail, code %d\n",
+ message, status);
+ } else {
+ /* only drivers with suspend() can ever resume();
+ * and after power loss, even they won't.
+ * bus_rescan_devices() can rebind drivers later.
+ */
+#if 1
+ /* FIXME the PM core self-deadlocks when unbinding
+ * drivers during suspend/resume ... everything grabs
+ * dpm_sem (not a spinlock, ugh). we want to unbind,
+ * since we know every driver's probe/disconnect works
+ * even for drivers that can't suspend.
+ */
+ dev_warn(&intf->dev, "resume is unsafe!\n");
+ intf->dev.power.power_state = PMSG_SUSPEND;
+#else
+ down_write(&usb_bus_type.rwsem);
+ device_release_driver(&intf->dev);
+ up_write(&usb_bus_type.rwsem);
+#endif
+ }
+ return status;
}
static int usb_generic_resume(struct device *dev)
--- g26.orig/drivers/usb/host/ehci-hcd.c 2005-05-09 17:55:14.000000000 -0700
+++ g26/drivers/usb/host/ehci-hcd.c 2005-05-09 18:25:57.000000000 -0700
@@ -758,22 +758,27 @@
static int ehci_suspend (struct usb_hcd *hcd, pm_message_t message)
{
struct ehci_hcd *ehci = hcd_to_ehci (hcd);
+ int status;
if (time_before (jiffies, ehci->next_statechange))
msleep (100);
#ifdef CONFIG_USB_SUSPEND
- (void) usb_suspend_device (hcd->self.root_hub, message);
+ if (hcd->self.root_hub->dev.power.power_state != PMSG_SUSPEND)
+ status = -EBUSY;
+ else
+ status = 0;
#else
usb_lock_device (hcd->self.root_hub);
- (void) ehci_hub_suspend (hcd);
+ status = ehci_hub_suspend (hcd);
usb_unlock_device (hcd->self.root_hub);
#endif
- // save (PCI) FLADJ in case of Vaux power loss
- // ... we'd only use it to handle clock skew
+ /* for PCI, we'd already have saved FLADJ (in case of Vaux
+ * power loss) since it'd be used only to handle clock skew
+ */
- return 0;
+ return status;
}
static int ehci_resume (struct usb_hcd *hcd)
--- g26.orig/drivers/usb/host/ohci-pci.c 2005-05-09 17:55:14.000000000 -0700
+++ g26/drivers/usb/host/ohci-pci.c 2005-05-09 18:25:57.000000000 -0700
@@ -116,18 +116,24 @@
static int ohci_pci_suspend (struct usb_hcd *hcd, pm_message_t message)
{
struct ohci_hcd *ohci = hcd_to_ohci (hcd);
+ int status;
/* suspend root hub, hoping it keeps power during suspend */
if (time_before (jiffies, ohci->next_statechange))
msleep (100);
#ifdef CONFIG_USB_SUSPEND
- (void) usb_suspend_device (hcd->self.root_hub, message);
+ if (hcd->self.root_hub->dev.power.power_state != PMSG_SUSPEND)
+ status = -EBUSY;
+ else
+ status = 0;
#else
usb_lock_device (hcd->self.root_hub);
- (void) ohci_hub_suspend (hcd);
+ status = ohci_hub_suspend (hcd);
usb_unlock_device (hcd->self.root_hub);
#endif
+ if (status < 0)
+ return status;
/* let things settle down a bit */
msleep (100);
--- g26.orig/include/linux/usb.h 2005-05-09 17:55:14.000000000 -0700
+++ g26/include/linux/usb.h 2005-05-09 18:25:57.000000000 -0700
@@ -977,7 +977,7 @@
int timeout);
/* selective suspend/resume */
-extern int usb_suspend_device(struct usb_device *dev, pm_message_t message);
+extern int usb_suspend_device(struct usb_device *dev);
extern int usb_resume_device(struct usb_device *dev);
