On Fri, 4 Nov 2011, Sarah Sharp wrote: > On Fri, Nov 04, 2011 at 10:43:58AM -0400, Alan Stern wrote: > > On Thu, 3 Nov 2011, Sarah Sharp wrote: > > > > > I also think there's a potential race condition because the PLC bit can > > > race with another port status change bit to cause a port status event, > > > and it will take some time for the USB core to react to it. I can > > > envision the following scenario (although I think it's pretty unlikely > > > to be hit by anyone): > > > > > > 1. USB 2.0 device on port 1 has hardware LPM enabled, and it's currently > > > in U2. > > > > > > 2. A second device on port 2 shares a ganged power switch with port 1, > > > and it starts to draw too much current. > > > > > > 3. The xHC sets the overcurrent change bit for ports 1 and 2, and > > > generates a port status change event. > > > > > > 4. The event is handled and khubd is kicked. However, before it can > > > clear the OCC bit, the device on port 1 comes out of U2, and the PLC > > > bit is set. > > > > > > Since OCC is set, the xHC won't generate a port status change event for > > > the PLC bit, > > > > That's not right. Regardless of whether other port-status-change bits > > are set, if PLC wasn't set originally then setting it should generate a > > port-status-change event. > > > > If the controller doesn't behave this way then it is indeed subject to > > races. > > That's how it was designed in the xHCI spec, unfortunately. See section > 4.19.2: > > "The change bits (CSC. PEC, etc.) of each port are ORed together and > gated by the HCHalted (HCH) flag to form the Port Status Change Event > Generation signal. A port shall generate a Port Status Change Event when > there is ‘0’ to ‘1’ transition of the PSCEG signal." > > Apparently some host controllers are pretty strict about this rule, and > others aren't. This means you need to fix up the xhci_hub_status_data() routine. It must turn root-hub polling on whenever it returns a nonzero status, and turn polling off whenever the status is 0. Without this extra polling, status-change events can be lost if they occur while other port-status-change bits are set -- then they'll never cause an IRQ. Something very similar had to be added to ohci_hub_status_data(), because OHCI status-change interrupts are level-driven. Take a look at the end of that function to get an idea of what I mean. > > > and it will never have the chance to be cleared in the port > > > status change event handler. I think what we need to do is check if the > > > PLC bit is set for a USB 2.0 device in the function that clears any > > > other port status change bit, and make sure to clear PLC there. > > > > You're talking about USB_PORT_STAT_C_PORT_LINK_STATE, right? This > > happens in two places: hub_activate() and hub_events(). > > But USB_PORT_STAT_C_PORT_LINK_STATE is never exposed to the USB core for > USB 2.0 roothubs, since external USB 2.0 hubs don't have that bit. So > it needs to be cleared internally in the xHCI driver. Hmmm. It looks like include/linux/usb/ch11.h also needs to be fixed up. Table 11-17 in the USB-2.0 spec defines port feature numbers only up to 22; the remaining entries (23 - 30) must be introduced elsewhere. In addition, some of the port feature #define's in that file are duplicates. Anyway, once that's done we could add code to handle USB_PORT_STAT_C_PORT_LINK_STATE to the hub driver. Alan Stern -- 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
