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.
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).
>
>> 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?
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.
--
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
