When a control transfer has a short data stage, the xHCI controller generates
two transfer events: a COMP_SHORT_TX event that specifies the untransferred
amount, and a COMP_SUCCESS event. But when the data stage is not short, only the
COMP_SUCCESS event occurs. Therefore, xhci-hcd must set urb->actual_length to
urb->transfer_buffer_length while processing the COMP_SUCCESS event, unless
urb->actual_length was set already by a previous COMP_SHORT_TX event.
The driver checks this by seeing whether urb->actual_length == 0, but this alone
is the wrong test, as it is entirely possible for a short transfer to have an
urb->actual_length = 0.
This patch changes the xhci driver to rely not only on the urb->actual_length,
but also on a new td->urb_length_set flag, which is set to true when a
COMP_SHORT_TX event is received and the URB length updated at that stage.
This fixes a bug which affected the HSO plugin, which relies on URBs with
urb->actual_length == 0 to halt re-submitting the RX URB in the control
endpoint.
Signed-off-by: Aleksander Morgado <aleksander@xxxxxxxxxxxxx>
---
Hey Mathias,
I have now updated the patch to avoid re-using the 'last_td_was_short' flag, and
instead use a new 'urb_length_set' flag in the xhci_td struct. This new flag
will be unique for each URB, and therefore the flag shouldn't affect other TDs
that may come afterwards, as was the case when reusing the 'last_td_was_short'
flag in 'ep_ring'.
I was wondering whether the xhci_td struct is the best place to add this logic;
maybe it isn't, given that this is control-transfer specific?
Let me know what you think.
Cheers!
---
drivers/usb/host/xhci-ring.c | 9 +++++++--
drivers/usb/host/xhci.h | 3 +++
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 88da8d6..be9c560 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1955,12 +1955,17 @@ 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 (!td->urb_length_set) {
td->urb->actual_length =
td->urb->transfer_buffer_length;
}
} else {
- /* Maybe the event was for the data stage? */
+ /*
+ * Maybe the event was for the data stage? If so, update already the
+ * actual_length of the URB and flag it as set, so that it is not
+ * overwritten in the event for the last TRB.
+ */
+ td->urb_length_set = true;
td->urb->actual_length =
td->urb->transfer_buffer_length -
EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 9745147..bd868aa 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1,3 +1,4 @@
+
/*
* xHCI host controller driver
*
@@ -1288,6 +1289,8 @@ struct xhci_td {
struct xhci_segment *start_seg;
union xhci_trb *first_trb;
union xhci_trb *last_trb;
+ /* actual_length of the URB has already been set */
+ bool urb_length_set;
};
/* xHCI command default timeout value */
--
2.3.0
--
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
