On Thu, Feb 26, 2015 at 5:12 PM, Mathias Nyman
<mathias.nyman@xxxxxxxxxxxxxxx> 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 sets 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 set the urb->actual_length in advance
> to the expected value of a successful control transfer.
> The urb->actual_length is then only adjusted in case of short transfers or
> other special events, but not on COMP_SUCCESS events.
>
> 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: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> ---
> drivers/usb/host/xhci-ring.c | 73 ++++++++++++++++++++++----------------------
> 1 file changed, 37 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index b46b5b9..0e02e79 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -732,7 +732,11 @@ remove_finished_td:
> /* Clean up the cancelled URB */
> /* Doesn't matter what we pass for status, since the core will
> * just overwrite it (because the URB has been unlinked).
> + * Control urbs have the urb->actual_length pre-set, clear it
> + * as well
> */
> + if (usb_endpoint_xfer_control(&cur_td->urb->ep->desc))
> + cur_td->urb->actual_length = 0;
> xhci_giveback_urb_in_irq(xhci, cur_td, 0);
>
> /* Stop processing the cancelled list if the watchdog timer is
> @@ -755,6 +759,7 @@ static void xhci_kill_ring_urbs(struct xhci_hcd *xhci, struct xhci_ring *ring)
> list_del_init(&cur_td->td_list);
> if (!list_empty(&cur_td->cancelled_td_list))
> list_del_init(&cur_td->cancelled_td_list);
> + cur_td->urb->actual_length = 0;
> xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
> }
> }
> @@ -792,6 +797,7 @@ static void xhci_kill_endpoint_urbs(struct xhci_hcd *xhci,
> cur_td = list_first_entry(&ep->cancelled_td_list,
> struct xhci_td, cancelled_td_list);
> list_del_init(&cur_td->cancelled_td_list);
> + cur_td->urb->actual_length = 0;
> xhci_giveback_urb_in_irq(xhci, cur_td, -ESHUTDOWN);
> }
> }
> @@ -1888,6 +1894,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
> int ep_index;
> struct xhci_ep_ctx *ep_ctx;
> u32 trb_comp_code;
> + bool force_finish_td = false;
>
> slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags));
> xdev = xhci->devs[slot_id];
> @@ -1906,7 +1913,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
> xhci_warn(xhci, "WARN: Success on ctrl data TRB "
> "without IOC set??\n");
> *status = -ESHUTDOWN;
> - } else {
> + } else if (*status == -EINPROGRESS) {
> + /* only set to 0 if no previous event set it earlier */
> *status = 0;
> }
> break;
> @@ -1918,6 +1926,8 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
> break;
> case COMP_STOP_INVAL:
> case COMP_STOP:
> + /* we don't continue stopped TDs, so length can be set to 0 */
> + td->urb->actual_length = 0;
> return finish_td(xhci, td, event_trb, event, ep, status, false);
> default:
> if (!xhci_requires_manual_halt_cleanup(xhci,
> @@ -1928,44 +1938,26 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct xhci_td *td,
> trb_comp_code, ep_index);
> /* else fall through */
> case COMP_STALL:
> - /* Did we transfer part of the data (middle) phase? */
> - if (event_trb != ep_ring->dequeue &&
> - event_trb != td->last_trb)
> - td->urb->actual_length =
> - td->urb->transfer_buffer_length -
> - EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
> - else
> - td->urb->actual_length = 0;
> -
> - return finish_td(xhci, td, event_trb, event, ep, status, false);
> + /* length will be set later below if we stall on data stage */
> + td->urb->actual_length = 0;
> + force_finish_td = true;
> + break;
> }
> +
> + /* If in setup stage, nothing transferred */
> + if (event_trb == ep_ring->dequeue)
> + td->urb->actual_length = 0;
> +
> /*
> - * Did we transfer any data, despite the errors that might have
> - * happened? I.e. did we get past the setup stage?
> + * In data stage, check if we transferred any data despite the possible
> + * errors that might have happened. Give back the urb if stalled,
> + * otherwise wait for the status stage event.
> */
> - if (event_trb != ep_ring->dequeue) {
> - /* The event was for the status stage */
> - if (event_trb == td->last_trb) {
> - if (td->urb->actual_length != 0) {
> - /* Don't overwrite a previously set error code
> - */
> - if ((*status == -EINPROGRESS || *status == 0) &&
> - (td->urb->transfer_flags
> - & URB_SHORT_NOT_OK))
> - /* Did we already see a short data
> - * stage? */
> - *status = -EREMOTEIO;
> - } else {
> - td->urb->actual_length =
> - td->urb->transfer_buffer_length;
> - }
> - } else {
> - /* Maybe the event was for the data stage? */
> - td->urb->actual_length =
> - td->urb->transfer_buffer_length -
> - EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
> - xhci_dbg(xhci, "Waiting for status "
> - "stage event\n");
> + if (event_trb != td->last_trb) {
> + td->urb->actual_length = td->urb->transfer_buffer_length -
> + EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
> + if (!force_finish_td) {
> + xhci_dbg(xhci, "Waiting for status stage event\n");
> return 0;
> }
> }
Now that I see it... before finishing the td and if the
urb->actual_length < urb->transfer_buffer_length (i.e. short tx),
shouldn't we also check if (td->urb->transfer_flags &
URB_SHORT_NOT_OK) in order to return a status of -EREMOTEIO?
> @@ -3346,6 +3338,15 @@ int xhci_queue_ctrl_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
> */
> if (urb->transfer_buffer_length > 0)
> num_trbs++;
> +
> + /*
> + * We want to set the urb->actual_length in advance and change it in
> + * case of error or short transfer. A SHORT_TX event may be followed
> + * by a SUCCESS event for that same TD, messing up the transfer length.
> + * So we can't touch urb->actual_length later on successful transfers
> + */
> + urb->actual_length = urb->transfer_buffer_length;
> +
> ret = prepare_transfer(xhci, xhci->devs[slot_id],
> ep_index, urb->stream_id,
> num_trbs, urb, 0, mem_flags);
--
Aleksander
https://aleksander.es
--
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
