On Tue, Jan 31, 2012 at 10:49:48AM -0500, Alan Stern wrote: > 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. Ok, sure. > > 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. What you described (disabling IRQs, etc) needs to be done in xhci_bus_suspend() as well, correct? Or were you thinking it could be done in the USB core, since other HCDs also have this problem? > > 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. Yep, makes sense to me. I had assumed calling usb_hcd_poll_rh_status() would mean the event was always taken care of, so I'm glad you're closing these race conditions. 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
