On Mon, Jun 04, 2012 at 03:08:06PM -0400, Alan Stern wrote: > On Mon, 4 Jun 2012, Sarah Sharp wrote: > > > This patch changes the xHCI ring handling code to allow the URB > > completion handler to run before ringing the doorbell on an endpoint > > ring. This means adding a new flag to prevent the doorbell ring > > (EP_STAY_HALTED), to avoid a race condition with the URB completion > > handler and a completing Set TR Dequeue Pointer command (which will > > restart the ring). The new flag temporarily stops the ring until the > > URB completion function has a chance to cancel any pending URBs. That > > will set a different cancellation pending flag, which will also halt the > > endpoint ring. > > > > Make sure to set the new EP_STAY_HALTED flag before calling > > xhci_cleanup_halted_endpoint(). That function will queue a Set TR > > Dequeue command, which will ring the doorbell and restart the ring after > > it completes. We need to make sure that the URB completion handlers > > have a chance to run before the ring is restarted. > > I really don't understand the structure of your driver, but this sounds > like it's more complicated than necessary. It's not really that complex. But yes, I agree that the xHCI ring handling code is pretty hairy, so this patch probably looks complex. > The general approach for > handling a stopped endpoint ring should be: > > Give back all the URBs that are complete; > > Clean up the ring; > > Restart it if any URBs remain queued. I can't give back URBs before cleaning up the ring because finish_td() sets several variables in the xhci_virt_endpoint that the cancellation code uses to move the dequeue pointer. That code perhaps somewhat easy to push upwards and duplicate, but... The real reason I want to keep this patch as coded is that I really don't want to change the behavior of the order of the URB completion WRT the ring cleanup and cancellation code. The cleanup and cancellation code is really complicated, and I would have to spend a month working out all the code paths that a behavioral change would touch. > As far as I can see, the only state you need to keep track of is > whether the ring is active, stopping, or stopped. There shouldn't be > any need for an EP_STAY_HALTED flag. There are several other "stopping" endpoint states, so it's really not as simple as you make it out to be: #define SET_DEQ_PENDING (1 << 0) #define EP_HALTED (1 << 1) /* For stall handling */ #define EP_HALT_PENDING (1 << 2) /* For URB cancellation */ /* Transitioning the endpoint to using streams, don't enqueue URBs */ #define EP_GETTING_STREAMS (1 << 3) #define EP_HAS_STREAMS (1 << 4) /* Transitioning the endpoint to not using streams, don't enqueue URBs */ #define EP_GETTING_NO_STREAMS (1 << 5) EP_HALTED means the endpoint is halted due to an error, and it's a condition that can only be cleared by the USB core, a class driver, or the xHCI driver if the USB core *should* be clearing the halt but it doesn't, like for control stalls. The other four states indicate that the ring is stopped, and the doorbell cannot be rung for various reasons. These flags may also indicate that other resources, such as the endpoint or stream ring pointers, should not be manipulated. We can't (and don't) ring the endpoint doorbell until all of the conditions have been cleared: - SET_DEQ_PENDING: A Set TR Dequeue command is pending. - EP_HALT_PENDING: A Stop Endpoint command is pending. - EP_GETTING_STREAMS: A Configure Endpoint command is pending to add streams. - EP_GETTING_NO_STREAMS: A Configure Endpoint command is pending to remove streams. So at this point, we're just adding a fifth "stopping" condition that prevents the endpoint ring from being rung, EP_STAY_HALTED. I think adding a new flag will cause less breakage than changing the order of the URB completion and cancellation/ring clean up. 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
