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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
