On Thu, Feb 26, 2015 at 5:12 PM, Mathias Nyman
<mathias.ny...@linux.intel.com> 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.ny...@linux.intel.com>
Tested this patch (with the extra "else if" suggested in the follow up
commit) and it seems to work correctly with the HSO plugin. Not sure
if it'll end up being the last version or not, but anyway:
Tested-by: Aleksander Morgado <aleksan...@aleksander.es>
Let me know if you want me to test anything else.
> ---
> 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;
> }
> }
> @@ -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);
> --
> 1.8.3.2
>
> --
> 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
--
Aleksander
https://aleksander.es
--
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
