On Fri, Nov 23, 2012 at 12:55:29PM -0500, Alan Stern wrote: > On Wed, 21 Nov 2012, Sarah Sharp wrote: > > > The USB core disables the USB 2.0 ports in several different error > > cases. It expects that all USB hubs will act like external hubs, and > > always return an interrupt event if a port change bit is set. External > > USB 2.0 and USB 3.0 hubs are level-triggered, so this works for them. > > > > However, xHCI roothubs are edge-triggered. If no port status change > > bits are set, and a new change bit is set, the xHCI host sends an > > interrupt and a Port Status Change Event. It will not generate another > > port event until all change bits are cleared and a new one is set. > > > > This opens up issues with the USB core port disabling code. Right now, > > it assumes that it can simply clear the port enable bit, and wait for > > the hub to send an interrupt when a change bit is set. If previous > > change bits weren't cleared, it's assumed to be fine because the hub > > will continue to remind the USB core about them through the interrupt > > endpoint. > > Is port-disabling the only place where this problem occurs? Probably not. > A more defensive approach would be to copy what ohci-hcd does. When a > port-change interrupt occurs, the driver switches over to polling for > root-hub status changes. It doesn't switch back to interrupt-driven > operation until the hub_status_data routine sees that none of the ports > have any change bits set. Yes, that does sound like a better idea. I'll take a look into it. But wouldn't I need to switch to polling in places in the roothub code other than hub_status_data? For example, the disable port code sets the port state to disabled without checking the status afterwards. > > diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c > > index a686cf4..7d9dcd6 100644 > > --- a/drivers/usb/host/xhci-hub.c > > +++ b/drivers/usb/host/xhci-hub.c > > @@ -340,6 +340,10 @@ static void xhci_disable_port(struct usb_hcd *hcd, struct xhci_hcd *xhci, > > return; > > } > > > > + /* Clear all change bits, so that we get a device connect event. */ > > + port_status |= PORT_CSC | PORT_PEC | PORT_WRC | > > + PORT_OCC | PORT_RC | PORT_PLC | > > + PORT_CEC; > > What if a connect-change occurred just before this? Would it get lost? I think it wouldn't get lost, because the device would have to re-connect after the port was disabled, and we're atomically clearing the change bits as we disable the port. But it's a moot point if this code isn't necessary because I switch the xHCI driver over to polling when a change bit is set. Sarah Sharp -- 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
