[PATCH v2] usb: dwc2: resume root hub when device detect with suspend state

[Alan Stern]

On Tue, 18 Nov 2014, Julius Werner wrote:

> >> You should be aware that it's not safe to use hcd->state for anything
> >> in a host controller driver.  That field is owned by usbcore, not by
> >> the HCD, and it is not protected by any locks.
> >>
> >> Thus, for example, hcd->state does not get set to HC_STATE_SUSPENDED
> >> until some time after the bus_suspend routine has returned.  A
> >> port-change interrupt might occur during that time interval.
> 
> Looks like there is explicit code in hcd_bus_suspend() to check for
> that race condition right after setting hcd->state, or do I
> misinterpret that (the "Did we race with a root-hub wakeup event?"
> part)?

That code doesn't quite do what you think.  For example:

	CPU 1				CPU 2
	-----				-----
	hcd_bus_suspend():
	call hcd->bus_suspend():
	    root hub gets suspended

					Wakeup IRQ arrives and is
					  ignored because hcd->state
					  is still HC_STATE_QUIESCING

	set hcd->state to HC_STATE_SUSPENDED
	Did we race with a wakeup event?
	  No because usb_hcd_resume_root_hub()
	  wasn't called.

Result: the wakeup event is lost.

> Also, it seems xhci_bus_suspend() explicitly sets 'hcd->state =
> HC_STATE_SUSPENDED' before giving up the spinlock for some
> undocumented reason, maybe to avoid exactly this problem. We could
> just copy that trick if the hcd.c solution isn't enough (although the
> DWC2 bus_suspend/bus_resume in the other patch don't grab that
> spinlock right now, where I'm also not so sure if that's a good
> idea...).

It's better for HCDs to avoid testing hcd->state at all.  They should 
set it to appropriate values at the right times, because usbcore checks 
it, but they should not test it.  This is why ehci-hcd, ohci-hcd, and 
uhci-hcd all have a private rh_state variable, and they use it while 
holding their respective private spinlocks.

Alan Stern