On 23.02.2015 19:02, Aleksander Morgado wrote:
> On Mon, Feb 23, 2015 at 4:23 PM, Mathias Nyman
> <mathias.nyman@xxxxxxxxxxxxxxx> wrote:
>> Hi
>>
>> On 23.02.2015 13:52, Aleksander Morgado 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 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))
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.
If I find the time I'll rewrite this part, if not then I'll add your patch, (v4)
>
>>
>>> 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.
-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
