Re: usbcore and root-hub suspend/wakeup races

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jan 30, 2012 at 05:04:55PM -0500, Alan Stern wrote:
> Oliver and Sarah:
> 
> Thinking about the xHCI remote-wakeup issues made me realize that we
> still have a race in usbcore.  What happens if a wakeup event occurs on
> a root hub at about the same time as the root hub is suspended?

>From an xHCI driver perspective, there's a race condition you didn't
mention that was introduced by my hub suspend patchset.  When a port
wakes up, we first get a notification of the resume signaling through an
xHCI host IRQ and a port status change event.  Then the xHCI driver sets
the state to U0 and waits for a second port status change event to
appear, without calling usb_hcd_poll_rh_status().  If we were in the
middle of suspending the roothub when the first port status change event
occurred, the host controller may be suspended before the second port
status change event appears.

Should it just call usb_hcd_poll_rh_status() when the resume is first
detected, even though we will report no port status changes when khubd
polls the hub events (since the xHCI roothub is trying to act like an
external hub, which won't set PLC on device-initiated resume)?

I was actually trying to avoid having the USB core poll the port status
in between the xHCI driver noticing the resume and setting the port
state to U0, and the second port link change bit.  In that brief period
of time, some hardware will show bogus port status link states.  I
suppose I can make the xhci-hub code look at the new
bus_state->port_remote_wakeup bits and say the port is in U0 if that
port's bit is set.

> 
> 	(A) If the event happens after the controller device is 
> 	suspended, the controller will signal a wakeup request (PCI 
> 	PME# or GPIO interrupt or something similar).  The system will 
> 	wake up (if it has already gone to sleep) or the system sleep 
> 	transition will be aborted or the controller will autoresume.  
> 	This case is okay.
> 
> 	(B) If the event happens before the controller device is 
> 	suspended but after the root hub is suspended, there will be a 
> 	root-hub wakeup interrupt.  The HCD's handler will call 
> 	usb_hcd_resume_root_hub(), and again the sleep transition will
> 	be aborted or the root hub will autoresume.  Again we're okay.
> 
> 	(C) If the event happens before the root hub is suspended but 
> 	after the hub driver's suspend routine has run, the root-hub 
> 	interrupt URB will have been unlinked.  While processing the
> 	status-change interrupt, the HCD will clear the hardware's 
> 	interrupt request bit and call usb_hcd_poll_rh_status(), which 
> 	will set the HCD_FLAG_POLL_PENDING bit but not do anything 
> 	else.  The event won't be processed until the hub driver is
> 	resumed -- it won't abort anything or cause an autoresume.  
> 	This case is not okay.
> 
> 	(D) If the event happens before the hub driver's suspend 
> 	routine runs but after khubd is frozen, the root-hub interrupt
> 	URB will complete and hub_irq() will call kick_khubd().  But 
> 	since khubd is frozen, nothing more will happen.  Again we're
> 	not okay.
> 
> 	(E) If the event happens before khubd is frozen then 
> 	kick_khubd() will wake khubd.  The khubd thread checks for
> 	freeze requests only when its event list is empty, so the event 
> 	will be processed before khubd is frozen, and this case is 
> 	okay.
> 
> The patch below is intended to handle cases (C) and (D).  I haven't
> tested it.  Does it seem all right to you?  It uses an arbitrary 100-ms
> delay for case (D), on the basis that suspending and resuming hubs is a
> fairly slow process.

Are you trying to make sure the host controller is resumed within 100ms?
I'm not sure how long the xHCI resume process could take.  We have to
wait on the hardware to be ready to accept doorbell rings, and there's
no upper bound on the time it takes for a host controller to do that.

Otherwise, yes, this looks fine.

Sarah Sharp

> Index: usb-3.3/drivers/usb/core/hcd.c
> ===================================================================
> --- usb-3.3.orig/drivers/usb/core/hcd.c
> +++ usb-3.3/drivers/usb/core/hcd.c
> @@ -1978,6 +1978,11 @@ int hcd_bus_suspend(struct usb_device *r
>  	if (status == 0) {
>  		usb_set_device_state(rhdev, USB_STATE_SUSPENDED);
>  		hcd->state = HC_STATE_SUSPENDED;
> +
> +		/* Did we race with a root-hub wakeup event? */
> +		if (unlikely(HCD_POLL_PENDING(hcd) &&
> +				rhdev->do_remote_wakeup))
> +			usb_hcd_resume_root_hub(hcd);
>  	} else {
>  		spin_lock_irq(&hcd_root_hub_lock);
>  		if (!HCD_DEAD(hcd)) {
> Index: usb-3.3/drivers/usb/core/hub.c
> ===================================================================
> --- usb-3.3.orig/drivers/usb/core/hub.c
> +++ usb-3.3/drivers/usb/core/hub.c
> @@ -399,6 +399,11 @@ static void kick_khubd(struct usb_hub *h
>  		usb_autopm_get_interface_no_resume(
>  				to_usb_interface(hub->intfdev));
>  		wake_up(&khubd_wait);
> +
> +		/* Did a wakeup event arrive while khubd was frozen? */
> +		if (unlikely(frozen(khubd_task) &&
> +				device_may_wakeup(&hub->hdev->dev)))
> +			pm_wakeup_event(&hub->hdev->dev, 100);
>  	}
>  	spin_unlock_irqrestore(&hub_event_lock, flags);
>  }
> 
--
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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux