On Mon, 30 Jan 2012, Sarah Sharp wrote: > On Mon, Jan 30, 2012 at 05:04:55PM -0500, Alan Stern wrote: > > Oliver and Sarah: > > > > Thinking about the xHCI remote-wakeup issues made me realize that we > > still have a race in usbcore. What happens if a wakeup event occurs on > > a root hub at about the same time as the root hub is suspended? > > From an xHCI driver perspective, there's a race condition you didn't > mention that was introduced by my hub suspend patchset. When a port > wakes up, we first get a notification of the resume signaling through an > xHCI host IRQ and a port status change event. Then the xHCI driver sets > the state to U0 and waits for a second port status change event to > appear, without calling usb_hcd_poll_rh_status(). If we were in the > middle of suspending the roothub when the first port status change event > occurred, the host controller may be suspended before the second port > status change event appears. Right. I was considering usbcore only. What you described has to be handled in the xhci_bus_suspend() routine. > Should it just call usb_hcd_poll_rh_status() when the resume is first > detected, even though we will report no port status changes when khubd > polls the hub events (since the xHCI roothub is trying to act like an > external hub, which won't set PLC on device-initiated resume)? In fact, all HCDs need to address this sort of problem. It looks like the best answer is to disable IRQ generation at the start of bus_suspend and call synchronize_irq(). As a bonus, the remainder of bus_suspend can run without holding the private lock, since no other threads will enter the driver until the suspend finishes and IRQ generation is re-enabled. > I was actually trying to avoid having the USB core poll the port status > in between the xHCI driver noticing the resume and setting the port > state to U0, and the second port link change bit. There's no good reason for polling the port status during that interval. > In that brief period > of time, some hardware will show bogus port status link states. I > suppose I can make the xhci-hub code look at the new > bus_state->port_remote_wakeup bits and say the port is in U0 if that > port's bit is set. > > > > > (A) If the event happens after the controller device is > > suspended, the controller will signal a wakeup request (PCI > > PME# or GPIO interrupt or something similar). The system will > > wake up (if it has already gone to sleep) or the system sleep > > transition will be aborted or the controller will autoresume. > > This case is okay. > > > > (B) If the event happens before the controller device is > > suspended but after the root hub is suspended, there will be a > > root-hub wakeup interrupt. The HCD's handler will call > > usb_hcd_resume_root_hub(), and again the sleep transition will > > be aborted or the root hub will autoresume. Again we're okay. > > > > (C) If the event happens before the root hub is suspended but > > after the hub driver's suspend routine has run, the root-hub > > interrupt URB will have been unlinked. While processing the > > status-change interrupt, the HCD will clear the hardware's > > interrupt request bit and call usb_hcd_poll_rh_status(), which > > will set the HCD_FLAG_POLL_PENDING bit but not do anything > > else. The event won't be processed until the hub driver is > > resumed -- it won't abort anything or cause an autoresume. > > This case is not okay. > > > > (D) If the event happens before the hub driver's suspend > > routine runs but after khubd is frozen, the root-hub interrupt > > URB will complete and hub_irq() will call kick_khubd(). But > > since khubd is frozen, nothing more will happen. Again we're > > not okay. > > > > (E) If the event happens before khubd is frozen then > > kick_khubd() will wake khubd. The khubd thread checks for > > freeze requests only when its event list is empty, so the event > > will be processed before khubd is frozen, and this case is > > okay. > > > > The patch below is intended to handle cases (C) and (D). I haven't > > tested it. Does it seem all right to you? It uses an arbitrary 100-ms > > delay for case (D), on the basis that suspending and resuming hubs is a > > fairly slow process. > > Are you trying to make sure the host controller is resumed within 100ms? No, all that's really needed is for khubd to wake up before another suspend is attempted. But that won't require any delay; khubd will wake up as soon as it leaves the freezer. The idea behind the 100-ms delay is that it shouldn't be too long -- we don't want to prevent the system from suspending again after the wakeup event is processed. > I'm not sure how long the xHCI resume process could take. We have to > wait on the hardware to be ready to accept doorbell rings, and there's > no upper bound on the time it takes for a host controller to do that. > > Otherwise, yes, this looks fine. Good. The basic idea is that usbcore guarantees it won't lose wakeup events, provided that when one occurs: If the root hub is running, the HCD should call usb_hcd_poll_rh_status() or anything that does kick_khubd() (such as usb_wakeup_notification()). If the root hub is suspended, the HCD should call usb_hcd_resume_root_hub() or anything that does kick_khubd(). While the root hub is suspending, calling any of those three routines would be good enough -- although with IRQs generation disabled, there shouldn't be any wakeup events. 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
