On Wed, Jan 23, 2013 at 01:33:21PM +0800, Ming Lei wrote:
> 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 */
>
> /*-------------------------------------------------------------------------*/
Hi Ming, I also find this problem at my platform.
(At chipidea controller, the resume signal will be ended about 21ms
automatically)
Neither delay 30ms at hub_events, nor revert your patch
(596d789a211d134dc5f94d1e5957248c204ef850) can work.
This problem may exist after my patch (6273f1810f95f4deeb2f0d6810f301726ad32308)
The first log is for this problem, the second one without problem
due to console output occupy much times and resume bit is cleared.
-------------------------------First Log------------------------
mx_usb 2184000.usb: Wakeup interrupt occurs imx_controller_resume
mxs_phy 20c9000.usbphy: Connect line between phy and controller
ci_hdrc ci_hdrc.0: port 1 remote wakeup
imx_usb 2184000.usb: at the end of imx_controller_resume, ret = 0
usb usb1: usb wakeup-resume
usb usb1: usb auto-resume
ci_hdrc ci_hdrc.0: resume root hub
hub 1-0:1.0: hub_resume
ci_hdrc ci_hdrc.0: GetStatus port:1 status 100014c5 8 ACK POWER sig=k SUSPEND RESUME PE CONNECT
hub 1-0:1.0: port 1: status 0107 change 0000
hub 1-0:1.0: state 7 ports 1 chg 0000 evt 0000
hub 1-0:1.0: state 7 ports 1 chg 0000 evt 0000
hub 1-0:1.0: hub_suspend
usb usb1: bus auto-suspend, wakeup 1
ci_hdrc ci_hdrc.0: suspend root hub
ci_hdrc ci_hdrc.0: suspend failed because a port is resuming
usb usb1: bus suspend fail, err -16
hub 1-0:1.0: hub_resume
ci_hdrc ci_hdrc.0: GetStatus port:1 status 10001805 8 ACK POWER sig=j PE CONNECT
hub 1-0:1.0: port 1: status 0103 change 0004
hub 1-0:1.0: state 7 ports 1 chg 0002 evt 0000
ci_hdrc ci_hdrc.0: GetStatus port:1 status 10001805 8 ACK POWER sig=j PE CONNECT
usb 1-1: usb wakeup-resume
ci_hdrc ci_hdrc.0: GetStatus port:1 status 10001805 8 ACK POWER sig=j PE CONNECT
usb 1-1: finish resume
hub 1-1:1.0: hub_resume
hub 1-1:1.0: port 1: status 0103 change 0004
ci_hdrc ci_hdrc.0: reused qh bfbb1740 schedule
usb 1-1: link qh16-0e01/bfbb1740 start 1 [1/2 us]
hub 1-0:1.0: resume on port 1, status 0
hub 1-0:1.0: port 1, status 0103, change 0004, 12 Mb/s
hub 1-1:1.0: state 7 ports 3 chg 0002 evt 0000
usb 1-1.1: usb wakeup-resume
usb 1-1.1: finish resume
------------------------------Second Log----------------------------
root@freescale ~$ imx_usb 2184000.usb: at ci13xxx_imx_runtime_resume
imx_usb 2184000.usb: at the beginning of imx_controller_resume
imx_usb 2184000.usb: Wakeup interrupt occurs imx_controller_resume
mxs_phy 20c9000.usbphy: Connect line between phy and controller
ci_hdrc ci_hdrc.0: port 1 remote wakeup
imx_usb 2184000.usb: at the end of imx_controller_resume, ret = 0
usb usb1: usb wakeup-resume
usb usb1: usb auto-resume
ci_hdrc ci_hdrc.0: resume root hub
ci_hdrc ci_hdrc.0: Port status(0x10601405) is wrong
hub 1-0:1.0: hub_resume
ci_hdrc ci_hdrc.0: GetStatus port:1 status 10001805 8 ACK POWER sig=j PE CONNECT
hub 1-0:1.0: port 1: status 0103 change 0004
hub 1-0:1.0: state 7 ports 1 chg 0002 evt 0002
ci_hdrc ci_hdrc.0: GetStatus port:1 status 10001805 8 ACK POWER sig=j PE CONNECT
usb 1-1: usb wakeup-resume
ci_hdrc ci_hdrc.0: GetStatus port:1 status 10001805 8 ACK POWER sig=j PE CONNECT
usb 1-1: finish resume
hub 1-1:1.0: hub_resume
hub 1-1:1.0: port 1: status 0103 change 0004
ci_hdrc ci_hdrc.0: reused qh bfbb1740 schedule
--
Best Regards,
Peter Chen
--
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
