When the control TD doesn't have TRBs in the data stage, the URB actual length
is set equal to the transfer buffer length. E.g. with a 64 byte transfer buffer
length:
Transfer event len = 0, COMP_SHORT_SUCCESS (status)
|---------> URB actual length set to transfer buffer length = 64
When the control TD has TRBs in the data stage, the URB actual length is
computed based on the transfer event length reported in the data stage TRB. In
this case, the URB actual length won't be modified in the status stage. E.g.
again with a 64 byte transfer buffer length:
Transfer event len = 63, COMP_SHORT_TX (data)
|---------> URB actual length = 64 - 63 = 1
Transfer event len = 0, COMP_SHORT_SUCCESS (status)
|---------> URB actual length not modified = 1
The current logic, though, doesn't seem to contemplate the case where a TD has a
TDR in the data stage which actually reports 0 bytes (i.e. transfer event len
equal to transfer buffer len). The logic is currently handling this case in the
following way:
Transfer event len = 64, COMP_SHORT_TX (data)
|---------> URB actual length = 64 - 64 = 0
Transfer event len = 0, COMP_SHORT_SUCCESS (status)
|---------> URB actual length set to transfer buffer length = 64 <------
In this case, the logic shouldn't update the URB actual length during the status
stage; instead it should leave the URB actual length that was computed during
the data stage, even if it's 0:
Transfer event len = 63, COMP_SHORT_TX (data)
|---------> URB actual length = 64 - 64 = 0
Transfer event len = 0, COMP_SHORT_SUCCESS (status)
|---------> URB actual length not modified = 0 <------
The proposed fix re-uses the 'last_td_was_short' flag in the endpoint ring. The
setting of this flag is moved to after the actual TRB processing, so that the
flag value indicating whether the last TRB was short can be read during the
processing. The control TDR processing will use this flag to determine whether
it has to reset the URB actual length or instead leave it as it was already
precomputed by the previous short TRB.
The background for this change is that the HSO plugin will resubmit the control
RX URB until zero bytes get read. Without this fix, the reported URB actual
length never reached this situation as all reported URBs had actual length > 0,
and the driver ended up entering in an infinite loop of URB resubmissions
reading zeroes all the time, as in the following HSO debug logs:
--- Got muxed ctrl callback 0x00 ---
[ +0,000008] [1997:ctrl_callback]: Actual length of urb = 1
--- Got muxed ctrl callback 0x00 ---
[ +0,000006] [1997:ctrl_callback]: Actual length of urb = 1
--- Got muxed ctrl callback 0x00 ---
[ +0,000007] [1997:ctrl_callback]: Actual length of urb = 1
--- Got intr callback 0x00 ---
[ +0,000006] [1880:intr_callback]: port_req = 0x01
[ +0,000004] [1888:intr_callback]: Pending read interrupt on port 0
--- Got muxed ctrl callback 0x00 ---
[ +0,000007] [1997:ctrl_callback]: Actual length of urb = 6
[ +0,000004] [2035:put_rxbuf_data]: data to push to tty
--- Got muxed ctrl callback 0x00 ---
[ +0,000006] [1997:ctrl_callback]: Actual length of urb = 1024
[ +0,000004] [2035:put_rxbuf_data]: data to push to tty
--- Got muxed ctrl callback 0x00 ---
[ +0,000006] [1997:ctrl_callback]: Actual length of urb = 1024
[ +0,000003] [2035:put_rxbuf_data]: data to push to tty
--- Got muxed ctrl callback 0x00 ---
[ +0,000006] [1997:ctrl_callback]: Actual length of urb = 1024
[ +0,000004] [2035:put_rxbuf_data]: data to push to tty
.....
Signed-off-by: Aleksander Morgado <aleksan...@aleksander.es>
---
drivers/usb/host/xhci-ring.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 88da8d6..6b050f1 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1955,7 +1955,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct
xhci_td *td,
/* Did we already see a short data
* stage? */
*status = -EREMOTEIO;
- } else {
+ } else if (!ep_ring->last_td_was_short) {
td->urb->actual_length =
td->urb->transfer_buffer_length;
}
@@ -2447,10 +2447,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
ret = skip_isoc_td(xhci, td, event, ep, &status);
goto cleanup;
}
- if (trb_comp_code == COMP_SHORT_TX)
- ep_ring->last_td_was_short = true;
- else
- ep_ring->last_td_was_short = false;
if (ep->skip) {
xhci_dbg(xhci, "Found td. Clear skip flag.\n");
@@ -2484,6 +2480,14 @@ static int handle_tx_event(struct xhci_hcd *xhci,
ret = process_bulk_intr_td(xhci, td, event_trb, event,
ep, &status);
+ /* Flag whether the just processed TRB was short. Do it after
+ * processing, so that the processor methods can also use this
+ * flag. */
+ if (trb_comp_code == COMP_SHORT_TX)
+ ep_ring->last_td_was_short = true;
+ else
+ ep_ring->last_td_was_short = false;
+
cleanup:
/*
* Do not update event ring dequeue pointer if ep->skip is set.
--
2.3.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
