On Wed, Jan 23, 2013 at 4:08 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> Lei:
>
> It turns out that your patch setting the autosuspend delay for hubs to
> 0 causes problems for root hubs. They need a delay of at least 30 ms.
>
> When a child device sends a remote wakeup request, the root hub
> generates an interrupt. The HCD's interrupt handler sees what happened
> and requests a runtime resume for the root hub. However it can't tell
> the hub driver about the wakeup request until the port resume is
> finished, which takes about 25 ms. During that time, the hub driver
> won't know what has happened and so it will try to autosuspend. The
> autosuspend will fail because the port is resuming, but the hub driver
> will go right ahead and keep trying to autosuspend. This will continue
> until the port resume is complete.
>
> In order to avoid all these extra autosuspend attempts, the delay
> for root hubs should be set to something larger than 25 ms, such as 30
> ms. Do you agree?
IMO, that should be the simplest fix on the problem.
But, looks the delay of 25ms or 30ms is only required in the above
remote wakeup case, and user can override the delay too, so maybe
we need to figure out one better approach to deal with that.
Could we add a delay in hub_events() for handling the case? see below
draft patch for explaining the idea:
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 4225d5e..5f5a9ec 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2112,6 +2112,29 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
}
EXPORT_SYMBOL_GPL(usb_hcd_resume_root_hub);
+void usb_hcd_set_rh_ports_resuming(struct usb_device *rhdev, bool val)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave (&hcd_root_hub_lock, flags);
+ rhdev->rh_resuming_ports = val;
+ spin_unlock_irqrestore (&hcd_root_hub_lock, flags);
+}
+EXPORT_SYMBOL_GPL(usb_hcd_set_rh_ports_resuming);
+
+bool usb_hcd_get_rh_ports_resuming(struct usb_device *rhdev)
+{
+ unsigned long flags;
+ bool val;
+
+ spin_lock_irqsave (&hcd_root_hub_lock, flags);
+ val = rhdev->rh_resuming_ports;
+ spin_unlock_irqrestore (&hcd_root_hub_lock, flags);
+
+ return val;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_get_rh_ports_resuming);
+
#endif /* CONFIG_USB_SUSPEND */
/*-------------------------------------------------------------------------*/
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a815fd2..4b728e8 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4492,6 +4492,13 @@ static void hub_events(void)
hdev = hub->hdev;
hub_dev = hub->intfdev;
intf = to_usb_interface(hub_dev);
+
+ /* root hub is sending resume signal, so wait for its completion */
+ if (!hdev->parent && usb_hcd_get_rh_ports_resuming(hdev)) {
+ msleep(30);
+ usb_hcd_set_rh_ports_resuming(hdev, 0);
+ }
+
dev_dbg(hub_dev, "state %d ports %d chg %04x evt %04x\n",
hdev->state, hub->descriptor
? hub->descriptor->bNbrPorts
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index c97503b..3a90150 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -802,6 +802,7 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
set_bit(i, &ehci->resuming_ports);
ehci_dbg (ehci, "port %d remote wakeup\n", i + 1);
mod_timer(&hcd->rh_timer, ehci->reset_done[i]);
+ usb_hcd_set_rh_ports_resuming(hcd->self.root_hub, 1);
}
}
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 689b14b..ea16fda 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -562,6 +562,7 @@ struct usb_device {
unsigned do_remote_wakeup:1;
unsigned reset_resume:1;
unsigned port_is_suspended:1;
+ unsigned rh_resuming_ports:1;
#endif
struct wusb_dev *wusb_dev;
int slot_id;
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 608050b..451650d 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -590,11 +590,15 @@ extern int hcd_bus_resume(struct usb_device
*rhdev, pm_message_t msg);
#ifdef CONFIG_USB_SUSPEND
extern void usb_hcd_resume_root_hub(struct usb_hcd *hcd);
+extern void usb_hcd_set_rh_ports_resuming(struct usb_device *rhdev, bool val);
+extern bool usb_hcd_get_rh_ports_resuming(struct usb_device *rhdev);
#else
static inline void usb_hcd_resume_root_hub(struct usb_hcd *hcd)
{
return;
}
+static inline void usb_hcd_set_rh_ports_resuming(struct usb_device
*rhdev, bool val) {}
+static inline bool usb_hcd_get_rh_ports_resuming(struct usb_device
*rhdev) {return false;}
#endif /* CONFIG_USB_SUSPEND */
/*-------------------------------------------------------------------------*/
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
