On Sat, Feb 21, 2015 at 4:34 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > 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. > I think that part of the mess in the long explanation was because I was trying to explain to myself the issue in the first place. But yes, as soon as the issue is understood, a shorter explanation is clearly more efficient. I'll resubmit the patch with the shorter explanation. Thanks! -- 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
