Hi,
On 06/04/2012 08:27 PM, Sarah Sharp wrote:
Hans, can you please test this patch?
Tested, and I'm afraid I've bad news, not only does the patch not
work, it seems that it is impossible to make an xHCI controller stop
the ring on a short packet :|
First why the patch does not work:
<snip>
@@ -1594,12 +1594,20 @@ static int finish_td(struct xhci_hcd *xhci, struct xhci_td *td,
* pointer past the TD. We can't do that here because
* the halt condition must be cleared first. Let the
* USB class driver clear the stall later.
+ *
+ * The endpoint will remain halted until the stall is
+ * cleared, so completion handlers will run before the
+ * ring is restarted.
*/
ep->stopped_td = td;
ep->stopped_trb = event_trb;
ep->stopped_stream = ep_ring->stream_id;
} else if (xhci_requires_manual_halt_cleanup(xhci,
ep_ctx, trb_comp_code)) {
+ /* Let the URB completion handlers run before restarting
+ * the endpoint ring.
+ */
+ ep->ep_state |= EP_STAY_HALTED;
/* Other types of errors halt the endpoint, but the
* class driver doesn't call usb_reset_endpoint() unless
* the error is -EPIPE. Clear the halted status in the
At a first review, before testing, this looked very good, but
xhci_requires_manual_halt_cleanup returns false on a trb_comp_code of
COMP_SHORT_TX.
This does mean that the patch is still valuable / good to have to enforce the
usb core / drivers expected behavior for other types of errors, but that
it does not work for short packet "errors".
Which has resulted in me doing a good 2 hours of reading the XHCI spec, and
xhci_requires_manual_halt_cleanup is 100% correct here, not only does a
short packet currently not stop the ep ring, there is no way to make a short
packet stop the ep ring. A (normal) trb does have the Interrupt-on Short Packet
(ISP) flag, but that only causes the controller to generate a Transfer Event,
not to stop the ep ring.
So it looks like, if we want to preserve the usb-core / drivers expected behavior
of a short packet in an urb with the URB_SHORT_NOT_OK flag set causing the ep
to halt until the completion handler for that ep has run, that the xHCI code
would need to not submit later urbs for the same ep to the controller until the
urb with the URB_SHORT_NOT_OK flag set has completed.
An alternative which I've quickly explored is auditing all users of the
URB_SHORT_NOT_OK flag and see if they do any pipe-lining, iow submit more then
one urb at a time. And if they do fix them to somehow not do that (and then
document that URB_SHORT_NOT_OK does not guarantee to stop the ep).
A quick grep shows that that seems to be quite doable, only the following .c
files contain URB_SHORT_NOT_OK:
./drivers/usb/host/imx21-hcd.c
./drivers/usb/host/ohci-q.c
./drivers/usb/host/whci/qset.c
./drivers/usb/host/isp1362-hcd.c
./drivers/usb/host/xhci-ring.c
./drivers/usb/host/isp116x-hcd.c
./drivers/usb/host/oxu210hp-hcd.c
./drivers/usb/host/fhci-q.c
./drivers/usb/host/u132-hcd.c
./drivers/usb/host/ehci-q.c
./drivers/usb/host/isp1760-hcd.c
./drivers/usb/host/uhci-q.c
./drivers/usb/misc/usbtest.c
./drivers/usb/musb/cppi_dma.c
./drivers/usb/musb/musb_host.c
./drivers/usb/core/urb.c
./drivers/usb/core/devio.c
./drivers/usb/core/message.c
./drivers/usb/core/hcd.c
./drivers/staging/rts5139/rts51x_transport.c
./drivers/staging/usbip/stub_rx.c
But an important user of the flag is usb_sg_init and friends, which gets used in:
./drivers/usb/storage/transport.c
./drivers/usb/misc/usbtest.c
./drivers/usb/core/message.c
./drivers/staging/rts5139/rts51x_transport.c
./drivers/staging/usbip/vhci_hcd.c
./drivers/staging/keucr/transport.c
./drivers/mmc/host/vub300.c
Which also means that the issues of XHCI ep rings not
stopping on a short transfer is bigger then we thought, as
under the right circumstances it can impact the linux
usb-storage driver too .. :|
And this means that simply serializing urbs with the
URB_SHORT_NOT_OK flag set is not really an option either as this
will seriously hurt performance.
All in all it looks like
1) We need a way to submit sg lists (or some such) directly to
the hcd driver, so the the xHCI driver can put all the parts into 1 td.
2) Change usbdevfs to use an sg like interface for the BULK_CONTINUATION
flag, which also means changing the userspace API since userspace now
also needs a way to signal that the first urb is part of a multi urb
transfer
3) audit all the other users of URB_SHORT_NOT_OK
So a lot of work I'm afraid, but I see no other way to really fix this :(
Regards,
Hans
--
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
