On 26.02.2015 23:59, Aleksander Morgado wrote:
> 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?
>
Thanks, thats right.
The -EREMOTEIO return is missing in case short transfers are not supported.
This patch was written a bit too quickly and touches too many areas to include
in the next rc release, so I think we'll go with your patch to get the issue solved
in 4.0 final
-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
