On Mon, Mar 2, 2015 at 12:57 PM, Mathias Nyman
<mathias.nyman@xxxxxxxxxxxxxxx> wrote:
> 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
>
I think my patch also lacks the -EREMOTEIO return for the case when 0
bytes are transferred. I'll try to update it today.
--
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
