On Mon, Jan 20, 2014 at 02:45:11PM -0600, Felipe Balbi wrote: > If the HW reports the endpoint as Halted, there's > no need to start any URB for that endpoint, we can > simply return -EPIPE and this will tell upper layers > that the endpoint is, indeed, halted. > > This patch fixes usbtest's set/clear halt test #13 > on xHCI-based hosts. It looks like that test assumes that the xHCI driver should not allow URBs to be enqueued to the endpoint when the driver needs to call usb_clear_halt(). Or is it expecting that the host will attempt the transfer, and the device will stall the transfer? > Cc: <stable@xxxxxxxxxxxxxxx> > Signed-off-by: Felipe Balbi <balbi@xxxxxx> > --- > > Hi Sarah, > > seems like this has been broken for quite a long > time. > > I tested the patch on an AM437x which has XHCI Host and > the same IP configured as device as well (two dwc3 instances, > basically). > > It has been running for the past hour or so without any failures > when running testusb -t 13 -c 5000 -s 2048 -a in a loop, but > if you think there's a better way to fix this, let me know. I just > didn't understand why xhci-hcd still queues the URB even though > HW already told us the endpoint is halted. Any comments ? The reason the driver still queues URBs is because the xHC halts the endpoint for events that are outside of stalls. For instance, it will halt the endpoint on a babble error, a transfer error, or a split transaction error. The upper layers don't call usb_clear_halt() for those cases, so the xHCI driver handles the halted endpoint internally. Look for calls to xhci_requires_manual_halt_cleanup(). The xHC also halts the endpoint on a control transfer stall. The upper layers don't ever call usb_clear_halt on the control endpoint, because the bus spec says the next control transfer will clear the halted condition. Therefore the xHCI driver has to handle the halt manually. The way the driver does that is to set a flag for the endpoint (EP_HALTED), issue a Reset Endpoint command, and then a Set TR Dequeue command to move the dequeue pointer past the transfer that failed. The driver still accepts URBs submitted to the endpoint (because from the upper layer's perspective, we should in the cases where we manually handle the halt), but it does not ring the doorbell for that endpoint. Once the command to set the dequeue pointer completes, the EP_HALTED flag is cleared, the doorbell is rung for the endpoint, and the queued URBs are processed. The end result is that the xHCI driver *needs* to allow URBs to be queued when the endpoint is halted and it needs to manual cleanup the ring. Otherwise, the driver might queue an URB while the two commands are still being processed, the driver will reject the URBs, and the behavior in these corner cases will be different than other host controllers. So, no, I don't think this is quite the right solution. You could try to differentiate between an endpoint halt that requires a manual cleanup, and ones that the upper layer should handle, perhaps by adding a new EP_HALTED_MANUAL flag. The driver could accept URBs for the manual cleanup case, and reject URBs with -EPIPE for the ones the upper layers should handle. Code that refuses to ring the endpoint doorbell would have to look for both EP_HALTED and EP_HALTED_MANUAL. There might be other implications as well; grep for EP_HALTED and see what code checks it. Sarah Sharp > drivers/usb/host/xhci-ring.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index 53c2e29..e9df61a 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -2959,7 +2959,8 @@ static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring, > /* XXX not sure if this should be -ENOENT or not */ > return -EINVAL; > case EP_STATE_HALTED: > - xhci_dbg(xhci, "WARN halted endpoint, queueing URB anyway.\n"); > + xhci_dbg(xhci, "WARN halted endpoint.\n"); > + return -EPIPE; > case EP_STATE_STOPPED: > case EP_STATE_RUNNING: > break; > -- > 1.8.4.GIT > -- 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
