Hi
On 23.02.2015 13:52, Aleksander Morgado wrote:
> 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.
>
I think that the only case you should see a COMP_SUCCESS after a COMP_SHORT_TX
is if xhci hits a link TRB while automatically moving to the next TRB after the
short packet.
If Event Data TRBs are used, or a later TRB in that TD has the IOC flag set then xhci should just
generate a second transfer event with the same COMP_SHORT_TX completion code.
(xhci specs section 4.10.1.1)
> 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 should be fixed, handling short packets look a bit messy in general right now
>
> This patch changes the xhci driver to rely not only on the urb->actual_length,
> but also on the ep_ring->last_td_was_short flag, which is set to true when a
> COMP_SHORT_TX event is received.
>
> 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,
>
> This is the third update of the patch:
>
> * v2 modified the commit message to make it shorter and clearer.
>
> * v3 updated the format of the commented lines in the patch.
>
> Cheers!
>
> ---
> drivers/usb/host/xhci-ring.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 88da8d6..eda3276 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,16 @@ 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;
> +
How about the case where we only get one COMP_SHORT_TX event for that control transfer,
xhci then advances to the next TD, which completes successfully? That successful TD won't get
its td->urb->actual length set because the last_td_was_short flag it still set?
-Mathias
--
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
