On Fri, Dec 31, 2010 at 12:53:24PM -0500, Alan Stern wrote: > On Thu, 30 Dec 2010, Sarah Sharp wrote: > > > The hcd->flags are in a sorry state. Some of them are clearly specific to > > the particular roothub (HCD_POLL_RH, HCD_POLL_PENDING, and > > HCD_WAKEUP_PENDING), but some flags are related to PCI device state > > (HCD_HW_ACCESSIBLE and HCD_SAW_IRQ). This is an issue when one PCI device > > can have two roothubs that share the same IRQ line and hardware. > > > > Make sure to set HCD_FLAG_SAW_IRQ for both roothubs when an interrupt is > > serviced, or an URB is unlinked without an interrupt. (We can't tell if > > the host actually serviced an interrupt for a particular bus, but we can > > tell it serviced some interrupt.) > > You know, SAW_IRQ isn't used much any more. It was originally added to > help debug problems involving IRQ routing, but those have been pretty > much all fixed up by now. Instead of worrying about it, you can simply > get rid of this flag entirely. Yay! I'm always happy to get rid of dead code. > > HCD_HW_ACCESSIBLE is set once by usb_add_hcd(), which is set for both > > roothubs as they are added, so it doesn't need to be modified. > > HCD_POLL_RH and HCD_POLL_PENDING are only checked by the USB core, and > > they are never set by the xHCI driver, since the roothub never needs to be > > polled. > > > > The usb_hcd's state field is a similar mess. Sometimes the state applies > > to the underlying hardware: HC_STATE_HALT, HC_STATE_RUNNING, and > > HC_STATE_QUIESCING. But sometimes the state refers to the roothub state: > > HC_STATE_RESUMING and HC_STATE_SUSPENDED. > > I'm not sure this distinction makes any sense. But in any case, it > certainly is true that hcd->state is a terrible mess. I had thought that HC_STATE_SUSPENDED referred to when the usb_bus was suspended. Now that either bus under the xHCI host can be suspended, HC_STATE_RESUMING and HC_STATE_SUSPENDED doesn't make sense when applied to a usb_hcd, only to a specific usb_bus. > > This poses an issue with the xHCI split roothub, where two buses are > > registered for one PCI device. Each bus in the xHCI split roothub can be > > suspended separately, but both buses must be suspended before the PCI > > device can be suspended. Therefore, make sure that the USB core checks > > hcd->state equals HC_STATE_SUSPENDED for both roothubs before suspending > > the PCI host. > > This is tricky. Normally both buses will indeed be suspended before > the controller; the only situation where that wouldn't be true is if a > remote wakeup races with the controller suspend. I wonder if we can't > detect these races in a better way. Hmm, interesting. I don't have any ideas currently about that; I would have to look at the code. > > Make sure to kill off the shared roothub when the PCI resume fails. > > You can do this in the xhci resume routine instead of in the core. Ok, I'll look into doing that. > > The xHCI driver will need to ensure that HC_STATE_HALT, HC_STATE_RUNNING, > > and HC_STATE_QUIESCING will be set for both the roothubs. > > > > I'm not quite sure if the code in hcd_pci_suspend_noirq() is correct, > > please check. If one roothub is halted, then both roothubs should be > > halted (since they share the same hardware). But I suppose there could be > > a race condition where one usb_hcd->state is set to HC_STATE_HALT, but the > > other isn't yet? > > That business about not allowing remote wakeup if the controller is > HALTed can be handled elsewhere, such as in usb_hc_died(). Ok, sure. 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
