On 26.02.2015 20:01, Alan Stern wrote: > On Thu, 26 Feb 2015, Mathias Nyman wrote: > >> On 26.02.2015 18:12, Mathias Nyman 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 sets 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. >>> >>> 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 patch changes the xhci driver to set the urb->actual_length in advance >>> to the expected value of a successful control transfer. >>> The urb->actual_length is then only adjusted in case of short transfers or >>> other special events, but not on COMP_SUCCESS events. >>> >>> 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: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx> >>> --- >>> drivers/usb/host/xhci-ring.c | 73 ++++++++++++++++++++++---------------------- >>> 1 file changed, 37 insertions(+), 36 deletions(-) >>> >>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c >>> index b46b5b9..0e02e79 100644 >>> --- a/drivers/usb/host/xhci-ring.c >>> +++ b/drivers/usb/host/xhci-ring.c >>> @@ -732,7 +732,11 @@ remove_finished_td: >>> /* Clean up the cancelled URB */ >>> /* Doesn't matter what we pass for status, since the core will >>> * just overwrite it (because the URB has been unlinked). >>> + * Control urbs have the urb->actual_length pre-set, clear it >>> + * as well >>> */ >>> + if (usb_endpoint_xfer_control(&cur_td->urb->ep->desc)) >>> + cur_td->urb->actual_length = 0; > > Now this would do the wrong thing if some data had been sent or > received before the URB was cancelled. Thats right, this would overwrite any length set in a transfer event for this TD before canceling the the TD > > Does the controller write back information about the number of bytes to > the data TRB? Maybe it would be easier to get the values from there > instead of trying to use the event structures. Unfortunately it looks like we need to rely on the lengths from the events. Streams do have a Event Data Transfer Length Accumulator (EDTLA) in the stream context that xhci writes to when the endpoint is stopped, but this requires that the host supports a "Stopped EDTLA capability", and is only for streams. I think that maybe Aleksanders suggestion of adding the "length_set" flag is the way to go, atleast that something that we can get to the next rc release, and it's not as intrusive. -Mathias -- 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
