> > > The patch should have said something more like this:
> > >
> > > + udev = to_usb_device(dev);
> > > + if (message.event == PM_EVENT_FREEZE && udev->parent) {
> > > + dev->power.power_state = message;
> > > + return 0;
> > > + }
> > > + return usb_suspend_device (udev);
> >
> > The operative change being testing udev->parent. I think I prefer
> > the solution of having the hub suspend code call the root hub suspend
> > code, to make layering a bit more clear.
>
> Do you mean having the HCD suspend code call the root hub suspend code?
No, I mean having the hub_suspend() code call bus->hub_suspend(),
as in the attached compiles-but-untested patch.
It could be followed with patches trimming code out of HCDs, although
depending on how defensively those paths are coded, that may not be
needed right away.
> Either way could work. I prefer to do it this way, though.
It's certainly a good quick-fix for that issue. But it leaves an
inconsistency (actually, two of them) I'd rather do away with.
With this patch, the device suspend logic _only_ deals with a device's
upstream USB link, when it has one (which root hubs don't). Which gives
a clean definition of USB_SUSPEND to use as a good stabilization target:
it enables (a) the USB suspend state, and (b) remote wakeup paths.
Or said differently: for HCDs, there'd be one set of suspend/resume
paths to test and debug. Not two.
The second inconsistency is that the hub driver should be like other
USB drivers, and have suspend/resume methods if just PM is defined.
We decided on that policy a while back, and "hub.c" is the only
driver I know about which still uses USB_SUSPEND for that not PM.
> I looked at the ohci_hub_suspend
> and ohci_hub_resume routines; the problem seems to be that they set
> hcd->state in the wrong place. For instance, the resume routine doesn't
> set it back to HC_STATE_RUNNING until just before returning, but
> port-change interrupts (which will trigger the completion and resubmission
> of the status URB) can start up many milliseconds earlier.
>
> In fact, it seems a little odd that the root-hub routines should affect
> hcd->state. In the UHCI driver, only the HC routines change it. But of
> course, that's a matter of personal programming style.
It's more a matter of history. Remember that two years ago there WERE no
separate routines for root hub suspend/resume. OHCI was the first driver
to get any ... so that non-PCI hardware could suspend root hubs, thereby
saving power. It was only later that there was any hookup between usbcore
and root hub suspend. (Which for OHCI meant publishing those root hub
suspend/resume routines.) Or remote wakeup support. Or even suspend
and resume support for individual USB device drivers! And only recently
has that root hub hookup started to behave well for root hub timers.
Plus there've been many code changes affecting those layers, including Pavel's
changes (including "throw away information OHCI PM support relies on"),
yours, the PCI and ACPI folk, PPC-specific ones, and even some from me. :)
So the bottom line is that we need even more code changing there ... but
are now "finally" in a position to have the changes structured in a way
that may not imply significant changes in every kernel release. (Crosses
his fingers...)
- Dave
This patch associates hub suspend and resume logic (including for root hubs)
with CONFIG_PM not CONFIG_USB_SUSPEND, unifying two branches of suspend logic.
- HCD glue now always includes "usb_bus" code for root hub suspend/resume
- Hub driver always provides the suspend/resume methods
- Root hub suspend/resume is now called from hub suspend/resume, not
device suspend/resume
Depending on how defensively their suspend and resume paths are coded, some
HCDs may also need patches to remove non-USB_SUSPEND logic.
EXPERIMENTAL, UNTESTED ... but it compiles.
Depends on the other recent USB PM patches.
--- g26.orig/drivers/usb/core/hcd.c 2005-09-14 00:34:31.000000000 -0700
+++ g26/drivers/usb/core/hcd.c 2005-09-16 09:12:13.000000000 -0700
@@ -1430,9 +1430,7 @@ rescan:
/*-------------------------------------------------------------------------*/
-/* FIXME make this #ifdef CONFIG_PM ... update root hubs, retest */
-
-#ifdef CONFIG_USB_SUSPEND
+#ifdef CONFIG_PM
static int hcd_hub_suspend (struct usb_bus *bus)
{
@@ -1472,13 +1470,9 @@ void usb_hcd_resume_root_hub (struct usb
usb_resume_root_hub (hcd->self.root_hub);
spin_unlock_irqrestore (&hcd_root_hub_lock, flags);
}
+EXPORT_SYMBOL_GPL(usb_hcd_resume_root_hub);
-#else
-void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
-{
-}
#endif
-EXPORT_SYMBOL_GPL(usb_hcd_resume_root_hub);
/*-------------------------------------------------------------------------*/
@@ -1531,7 +1525,7 @@ static struct usb_operations usb_hcd_ope
.buffer_alloc = hcd_buffer_alloc,
.buffer_free = hcd_buffer_free,
.disable = hcd_endpoint_disable,
-#ifdef CONFIG_USB_SUSPEND
+#ifdef CONFIG_PM
.hub_suspend = hcd_hub_suspend,
.hub_resume = hcd_hub_resume,
#endif
--- g26.orig/drivers/usb/core/hub.c 2005-09-14 00:34:31.000000000 -0700
+++ g26/drivers/usb/core/hub.c 2005-09-16 09:36:55.000000000 -0700
@@ -1604,7 +1604,7 @@ static int hub_port_suspend(struct usb_h
*/
static int __usb_suspend_device (struct usb_device *udev, int port1)
{
- int status;
+ int status = 0;
/* caller owns the udev device lock */
if (port1 < 0)
@@ -1630,21 +1630,10 @@ static int __usb_suspend_device (struct
}
}
- /* "global suspend" of the HC-to-USB interface (root hub), or
- * "selective suspend" of just one hub-device link.
+ /* we only change a device's upstream USB link.
+ * root hubs have no upstream USB link.
*/
- if (!udev->parent) {
- struct usb_bus *bus = udev->bus;
- if (bus && bus->op->hub_suspend) {
- status = bus->op->hub_suspend (bus);
- if (status == 0) {
- dev_dbg(&udev->dev, "usb suspend\n");
- usb_set_device_state(udev,
- USB_STATE_SUSPENDED);
- }
- } else
- status = -EOPNOTSUPP;
- } else
+ if (udev->parent)
status = hub_port_suspend(hdev_to_hub(udev->parent), port1,
udev);
@@ -1833,25 +1822,9 @@ int usb_resume_device(struct usb_device
if (port1 < 0)
return port1;
- /* "global resume" of the HC-to-USB interface (root hub), or
- * selective resume of one hub-to-device port
- */
- if (!udev->parent) {
- struct usb_bus *bus = udev->bus;
- if (bus && bus->op->hub_resume) {
- status = bus->op->hub_resume (bus);
- } else
- status = -EOPNOTSUPP;
- if (status == 0) {
- dev_dbg(&udev->dev, "usb resume\n");
- /* TRSMRCY = 10 msec */
- msleep(10);
- usb_set_device_state (udev, USB_STATE_CONFIGURED);
- udev->dev.power.power_state = PMSG_ON;
- status = hub_resume (udev
- ->actconfig->interface[0]);
- }
- } else if (udev->state == USB_STATE_SUSPENDED) {
+ /* selective resume of one hub-to-device port */
+ if (udev->parent && udev->state == USB_STATE_SUSPENDED) {
+ // NOTW swsusp may bork us, device state being wrong...
// NOTE this fails if parent is also suspended...
status = hub_port_resume(hdev_to_hub(udev->parent),
port1, udev);
@@ -1892,6 +1865,28 @@ static int remote_wakeup(struct usb_devi
return status;
}
+#else /* !CONFIG_USB_SUSPEND */
+
+int usb_suspend_device(struct usb_device *udev)
+{
+ /* state does NOT lie by saying it's USB_STATE_SUSPENDED! */
+ return 0;
+}
+
+int usb_resume_device(struct usb_device *udev)
+{
+ udev->dev.power_state.event = PM_EVENT_ON;
+ return 0;
+}
+
+#define remote_wakeup(x) 0
+
+#endif /* CONFIG_USB_SUSPEND */
+
+EXPORT_SYMBOL(usb_suspend_device);
+EXPORT_SYMBOL(usb_resume_device);
+
+
static int hub_suspend(struct usb_interface *intf, pm_message_t msg)
{
struct usb_hub *hub = usb_get_intfdata (intf);
@@ -1909,6 +1904,21 @@ static int hub_suspend(struct usb_interf
}
}
+ /* "global suspend" of the HC-to-USB interface (root hub) */
+ if (!hdev->parent) {
+ struct usb_bus *bus = hdev->bus;
+ if (bus && bus->op->hub_suspend) {
+ int status = bus->op->hub_suspend (bus);
+
+ if (status != 0) {
+ dev_dbg(&hdev->dev, "root hub suspend %d\n",
+ status);
+ return status;
+ }
+ } else
+ return -EOPNOTSUPP;
+ }
+
/* stop khubd and related activity */
hub_quiesce(hub);
return 0;
@@ -1921,6 +1931,30 @@ static int hub_resume(struct usb_interfa
unsigned port1;
int status;
+ /* "global resume" of the HC-to-USB interface (root hub) */
+ if (!hdev->parent) {
+ struct usb_bus *bus = hdev->bus;
+ if (bus && bus->op->hub_resume) {
+ status = bus->op->hub_resume (bus);
+ if (status) {
+ dev_dbg(&hdev->dev, "root hub resume %d\n",
+ status);
+ return status;
+ }
+ } else
+ return -EOPNOTSUPP;
+ if (status == 0) {
+ dev_dbg(&hdev->dev, "usb resume\n");
+ /* TRSMRCY = 10 msec */
+ msleep(10);
+ }
+ }
+
+ /* REVISIT: this recursion probably shouldn't exist. But
+ * removing it is a separate patch ... that needs retesting
+ * for both remote wakeup and normal resume paths.
+ */
+#ifdef CONFIG_USB_SUSPEND
for (port1 = 1; port1 <= hdev->maxchild; port1++) {
struct usb_device *udev;
u16 portstat, portchange;
@@ -1954,6 +1988,7 @@ static int hub_resume(struct usb_interfa
}
up(&udev->serialize);
}
+#endif
hub->resume_root_hub = 0;
hub_activate(hub);
return 0;
@@ -1967,30 +2002,6 @@ void usb_resume_root_hub(struct usb_devi
kick_khubd(hub);
}
-#else /* !CONFIG_USB_SUSPEND */
-
-int usb_suspend_device(struct usb_device *udev)
-{
- /* state does NOT lie by saying it's USB_STATE_SUSPENDED! */
- return 0;
-}
-
-int usb_resume_device(struct usb_device *udev)
-{
- udev->dev.power_state.event = PM_EVENT_ON;
- return 0;
-}
-
-#define hub_suspend NULL
-#define hub_resume NULL
-#define remote_wakeup(x) 0
-
-#endif /* CONFIG_USB_SUSPEND */
-
-EXPORT_SYMBOL(usb_suspend_device);
-EXPORT_SYMBOL(usb_resume_device);
-
-
/* USB 2.0 spec, 7.1.7.3 / fig 7-29:
*
