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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
