On Sat, 21 Feb 2015, Aleksander Morgado wrote:
> Probably didn't explain well, sorry, likely mixing terms. What I mean
> is that when the data length received is equal to the transfer buffer
> length, we get a single IRQ event saying COMP_SUCCESS, with an event
> len = 0; the URB length in that case is set to be equal to the
> transfer buffer length.
> When the received data length is less that what it was requested in
> the transfer buffer length, we get 2 IRQ events: one with
> COMP_SHORT_TX status and the event length giving the 'unfilled' size
> of the buffer; and one with COMP_SUCCESS status and an event length
> set to 0. In this case, the URB length is set when the first event
> arrived, not when the second one arrived.
> The missing use case I'm trying to cover is when we do get 2 IRQ
> events for the same TD: one with COMP_SHORT_TX but where the
> "unfilled" length is equal to the transfer buffer length (hence, 0
> bytes transferred), and the second event with COMP_SUCCESS.
It sounds like this should be handled in the same way as the previous
case, but it isn't because of the peculiar way the driver is written.
> The current logic sets the final URB length in 2 different cases:
> * If the transferred length is equal to the transfer buffer length,
> the URB length is set when the event for the last TRB is received,
> notifying COMP_SUCESSS.
> * If the transferred length is less than the transfer buffer length
> but > 0, the URB length is set when the COMP_SHORT_TX event arrives;
> when the COMP_SUCCESS one arrives the URB length that was previously
> set is not modified.
>
> Now, there is this 3rd case, where the transferred length is 0; in
> this case we get both COMP_SHORT_TX and COMP_SUCCESS events; and the
> first one states that the "untransferred" length is equal to the
> transfer buffer length. With this patch in, which truth be told seems
> a bit like a hack (is there any cleaner way?), the case is covered,
> and we do get the 0-length URB notified.
I see the problem. The driver needs to distinguish between two cases:
COMP_SHORT_TX previously received and COMP_SHORT_TX not previously
received. The driver uses actual_length == 0 to make this distinction,
but that is not correct because it is possible to have actual_length ==
0 even after a COMP_SHORT_TX event. Using the last_td_was_short flag
instead seems like a reasonable solution.
> Currently the xhci driver doesn't report 0-sized URBs in the control
> endpoint, and that totally breaks the HSO driver usecase (which relies
> on the 0-sized URBs to stop re-submitting the RX URB), so Option HSO
> modems don't work properly on USB3 ports :/
Clearly this is a bug.
> Can also reword the commit message to try to make it clearer.
> Actually, I talk about COMP_SHORT_SUCCESS in the commit message when
> it really is COMP_SUCCESS... Just let me know how to move it forward
> :)
In general, shorter commit messages are easier to understand. Try to
avoid the tendency to report too much information. :-)
It wouldn't hurt simply to explain the situation as you did to me just
now (but perhaps in a more consise form). Something like this:
When a control transfer has a short data stage, the xHCI
controller generates two transfer events: a COMP_SHORT_TX event
that includes 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 actual_length was set previously. The driver
checks this by seeing whether actual_length == 0. But this is
the wrong test, because it is entirely possible for a short
transfer to have an actual_length of 0.
This patch changes the driver to use the last_td_was_short flag
instead of checking actual_length. This fixes a bug affecting
the HSO plugin ... etc.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
