Hey Alan, On Sat, Feb 21, 2015 at 3:47 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > On Sat, 21 Feb 2015, Aleksander Morgado wrote: > >> When the control TD doesn't have TRBs in the data stage, the URB actual length >> is set equal to the transfer buffer length. E.g. with a 64 byte transfer buffer >> length: >> >> Transfer event len = 0, COMP_SHORT_SUCCESS (status) >> |---------> URB actual length set to transfer buffer length = 64 > > How can a control TD have a transfer buffer length that is > 0 and also > have no TRBs in the data stage? That doesn't make sense. > 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 control TD has TRBs in the data stage, the URB actual length is >> computed based on the transfer event length reported in the data stage TRB. In >> this case, the URB actual length won't be modified in the status stage. E.g. >> again with a 64 byte transfer buffer length: >> >> Transfer event len = 63, COMP_SHORT_TX (data) >> |---------> URB actual length = 64 - 63 = 1 >> Transfer event len = 0, COMP_SHORT_SUCCESS (status) >> |---------> URB actual length not modified = 1 >> 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 current logic, though, doesn't seem to contemplate the case where a TD has a >> TDR in the data stage which actually reports 0 bytes (i.e. transfer event len >> equal to transfer buffer len). The logic is currently handling this case in the >> following way: >> >> Transfer event len = 64, COMP_SHORT_TX (data) >> |---------> URB actual length = 64 - 64 = 0 >> Transfer event len = 0, COMP_SHORT_SUCCESS (status) >> |---------> URB actual length set to transfer buffer length = 64 <------ >> >> In this case, the logic shouldn't update the URB actual length during the status >> stage; instead it should leave the URB actual length that was computed during >> the data stage, even if it's 0: >> >> Transfer event len = 63, COMP_SHORT_TX (data) >> |---------> URB actual length = 64 - 64 = 0 >> Transfer event len = 0, COMP_SHORT_SUCCESS (status) >> |---------> URB actual length not modified = 0 <------ > > In fact, is there _ever_ any reason for changing the actual length > during the status stage? It seems to me that by the end of the data > stage, we already know the actual length. > 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. >> The proposed fix re-uses the 'last_td_was_short' flag in the endpoint ring. The >> setting of this flag is moved to after the actual TRB processing, so that the >> flag value indicating whether the last TRB was short can be read during the >> processing. The control TDR processing will use this flag to determine whether >> it has to reset the URB actual length or instead leave it as it was already >> precomputed by the previous short TRB. > > Why not always leave the actual length as is? > 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. 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 :/ 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 :) -- 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
