>>>> 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)
>>>
>>
>> Well, I can only speak for this usecase I have here with the Option
>> HSO modem; in this case the COMP_SHORT_TX+COMP_SUCCESS pair happens
>> always, as the hso driver submits a URB with a 1024 byte buffer, and
>> the modem usually replies with AT responses character by character;
>> for each character I end up getting both events: first with
>> COMP_SHORT_TX and the event length set to 1023, second with
>> COMP_SUCESS and event length set to 0.
>>
>
> Looking at the code it seems that xhci controllers after 0.96 generate a
> spurious COMP_SUCCESS after short packet, code says:
>
> /* In xhci controllers which follow xhci 1.0 spec gives a spurious
> * success event after a short transfer. This quirk will ignore such
> * spurious event.
> */
> if (xhci->hci_version > 0x96)
> xhci->quirks |= XHCI_SPURIOUS_SUCCESS;
>
>> I read the xhci specs as well and I also got the impression that there
>> wasn't anything explicitly stating that a COMP_SUCCESS event always
>> follows a COMP_SHORT_TX. But, the code already assumes that if you get
>> a COMP_SHORT_TX event for a TRB which isn't the last one, you should
>> still get an event for the last TRB (i.e. finish_td() isn't called for
>> a COMP_SHORT_TX event if the TRB isn't the first one (event_trb !=
>> ep_ring->dequeue) and if it isn't the last one (event_trb !=
>> td->last_trb).
>
> Thats right, we always set the IOC (interrupt on completion) in the last TRB of
> a control transfer, so it will always interrupt, generating a transfer event.
>
> I read through your previous mails where you were investigating this and also looked a bit
> in more detail at the xhci code. I now understand better the issue. Its clearly a bug in xhci driver.
>
> Can you still add some debugging and check the if the second event after the COMP_SHORT_TX really
> is a COMP_SUCCESS ? and also what it says about the transfer length.
>
> printing out these values for the second event should do it:
> GET_COMP_CODE(le32_to_cpu(event->transfer_len))
> EVENT_TRB_LEN(le32_to_cpu(event->transfer_len))
>
The second event is always COMP_SUCCESS and the event->transfer_len is
always set to 0 in that one. The 3 cases I've seen are:
case 1: 1 event on last TRB
COMP_SUCCESS, event->len=0
case 2: short event but with data
COMP_SHORT_TX, event->len < urb->transfer_buffer_len
COMP_SUCCESS, event->len=0
case 3: short event with no data
COMP_SHORT_TX, event->len = urb->transfer_buffer_len
COMP_SUCCESS, event->len=0
> Right now process_ctrl_td() sets the urb->actual_length based on TRB position in TD (first, last, neither) and
> what value urb->actual_length contains. I think that by checking the actual event completion code, and the event
> reported remaining length we could get this correct without adding any additional fields to struct xhci_td.
>
The problem with that logic is that just by checking the last event
completion code and event->length we cannot know whether there was a
previous COMP_SHORT_TX event. i.e. when processing that last event you
wouldn't know whether it was case 1, case 2 or case 3 from above.
> If I find the time I'll rewrite this part, if not then I'll add your patch, (v4)
>
If you want to suggest any other approach, let me know and I'll spend
time with it.
>
>>
>>>
>>>> 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?
>>>
>>
>> If that is something that can happen, i.e. if COMP_SHORT_TX isn't
>> always followed by a COMP_SUCCESS, then we should definitely not use
>> the flag in the ep_ring. Maybe some flag in the URB itself?
>
> I think this is possible for xhci host v0.96 and older, we probably get a
> second COMP_SHORT_TX for the status stage TRB
>
>>
>> The other thing I thought of was to somehow always initialize the URB
>> actual length to the transfer buffer length from the very beginning,
>> and only update it if a COMP_SHORT_TX event is received. Not sure if
>> that would be much more complex to handle, though.
>>
>
> This could be an option, need to look into it.
--
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
