| Message ID | 570F6E61.10206@ti.com |
|---|---|
| State | New |
| Headers | show |
On 15/04/16 22:44, Steve Rae wrote: > > > On Thu, Apr 14, 2016 at 3:18 AM, Roger Quadros <rogerq@ti.com <mailto:rogerq@ti.com>> wrote: > > Hi, > > On 13/04/16 19:56, Sam Protsenko wrote: > > On Wed, Apr 13, 2016 at 3:32 PM, Roger Quadros <rogerq@ti.com <mailto:rogerq@ti.com>> wrote: > >> Hi, > >> > >> On 13/04/16 15:01, Semen Protsenko wrote: > >>> From: Sam Protsenko <semen.protsenko@linaro.org <mailto:semen.protsenko@linaro.org>> > >>> > >>> Some UDC controllers may require buffer size to be aligned to > >>> wMaxPacketSize. It's indicated by gadget->quirk_ep_out_aligned_size > >>> field being set to "true" (in UDC driver code). In that case > >>> rx_bytes_expected must be aligned to wMaxPacket size, otherwise stuck on > >>> transaction will happen. For example, it's required by DWC3 controller > >>> data manual: > >>> > >>> section 8.2.3.3 Buffer Size Rules and Zero-Length Packets: > >>> > >>> For OUT endpoints, the following rules apply: > >>> - The BUFSIZ field must be ≥ 1 byte. > >>> - The total size of a Buffer Descriptor must be a multiple of > >>> MaxPacketSize > >>> - A received zero-length packet still requires a MaxPacketSize buffer. > >>> Therefore, if the expected amount of data to be received is a multiple > >>> of MaxPacketSize, software should add MaxPacketSize bytes to the buffer > >>> to sink a possible zero-length packet at the end of the transfer. > >>> > >>> But other UDC controllers don't need such alignment, so mentioned field > >>> is set to "false". If buffer size is aligned to wMaxPacketSize, those > >>> controllers may stuck on transaction. The example is DWC2. > >>> > >>> This patch checks gadget->quirk_ep_out_aligned_size field and aligns > >>> rx_bytes_expected to wMaxPacketSize only when it's needed. > >>> > >>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org <mailto:semen.protsenko@linaro.org>> > >>> --- > >>> drivers/usb/gadget/f_fastboot.c | 9 +++++++++ > >>> 1 file changed, 9 insertions(+) > >>> > >>> diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c > >>> index 2e87fee..54dcce0 100644 > >>> --- a/drivers/usb/gadget/f_fastboot.c > >>> +++ b/drivers/usb/gadget/f_fastboot.c > >>> @@ -58,6 +58,7 @@ static unsigned int fastboot_flash_session_id; > >>> static unsigned int download_size; > >>> static unsigned int download_bytes; > >>> static bool is_high_speed; > >>> +static bool quirk_ep_out_aligned_size; > >>> > >>> static struct usb_endpoint_descriptor fs_ep_in = { > >>> .bLength = USB_DT_ENDPOINT_SIZE, > >>> @@ -240,6 +241,8 @@ static int fastboot_set_alt(struct usb_function *f, > >>> debug("%s: func: %s intf: %d alt: %d\n", > >>> __func__, f->name, interface, alt); > >>> > >>> + quirk_ep_out_aligned_size = gadget->quirk_ep_out_aligned_size; > >>> + > >>> /* make sure we don't enable the ep twice */ > >>> if (gadget->speed == USB_SPEED_HIGH) { > >>> ret = usb_ep_enable(f_fb->out_ep, &hs_ep_out); > >>> @@ -435,12 +438,18 @@ static unsigned int rx_bytes_expected(unsigned int maxpacket) > >>> return 0; > >>> if (rx_remain > EP_BUFFER_SIZE) > >>> return EP_BUFFER_SIZE; > >>> + > >>> + if (!quirk_ep_out_aligned_size) > >>> + goto out; > >>> + > >>> if (rx_remain < maxpacket) { > >>> rx_remain = maxpacket; > >>> } else if (rx_remain % maxpacket != 0) { > >>> rem = rx_remain % maxpacket; > >>> rx_remain = rx_remain + (maxpacket - rem); > >>> } > >>> + > >>> +out: > >>> return rx_remain; > >>> } > >>> > >>> > >> > >> Why do we need a special flag for this driver if other drivers e.g. mass storage > >> are doing perfectly fine without it? > >> > > > > I don't know how it works in other gadgets, but please see this patch > > in kernel: [1]. That patch is doing just the same as I did (and also > > in gadget code), using usb_ep_align_maybe() function for alignment. > > NOTE: there haven't been such quirks in the kernel drivers except for that one > driver that has a user mode interface and needs more moral policing for user > provided buffers. So that example is not something we should be using as reference. > Our buffers are alreay aligned to maxpacket size. The only thing we need to worry > about is the length of the last transaction that is not integral multiple of > maxpacket size. > > If my understanding is right all USB controllers should work fine with > bulk OUT requests that are integral multiple of maxpacket size. > So we shouldn't be needing any quirk flags. > > > > >> This patch is just covering up the real problem, by bypassing the faulty code > >> with a flag. > >> > >> The buffer size is EP_BUFFER_SIZE and is already aligned to wMaxPacketSize so > >> the problem shouldn't have happened in the first place. But it is happening > >> indicating something else is wrong. > >> > > > > There is what I'm observing on platform with DWC3 controller: > > - when doing "fastboot flash xloader MLO": > > - the whole size of data to send is 70964 bytes > > - the size of all packets (except of last packet) is 4096 bytes > > - so those are being sent just fine (in req->complete, which is > > rx_handler_dl_image() function) > > - but last packet has size of 1332 bytes (because 70964 % 4096 = 1332) > > - when its req->length is aligned to wMaxPacketSize (so it's 1536 > > bytes): after we send it using usb_ep_queue(), the req->complete > > callback is called one last time and we see that transmission is > > finished (download_bytes >= download_size) > > - but when its req->length is not aligned to wMaxPacketSize (so it's > > 1332 bytes): req->complete callback is not called last time, so > > transaction is not finished and we are stuck in "fastboot flash" > > > > So I guess the issue is real and related to DWC3 quirk. If you have > > any thoughts regarding other possible causes of this problem -- please > > share. I can't predict all possible causes as I'm not USB expert. > > I've tried to clean up the bulk out handling code in the below patch. > Note you will need to apply this on top of the 2 patches I sent earlier. > https://patchwork.ozlabs.org/patch/609417/ > https://patchwork.ozlabs.org/patch/609896/ > > Steve, do let me know if it works for you. If it doesn't then we need to figure out why. > Can you please share details about the USB controller on your board? > Does it not like OUT requests that are aligned to maxpacket size? > What does ep->maxpacket show for your board? > > > This (still) does not work.... > - using the standard U-Boot DWC2 driver, > - do not know if it doesn't like "OUT requests that are aligned to maxpacket size" -- perhaps @Lukasz could answer this.... > - ep->maxpacket is 512 > > with logging in "drivers/usb/gadget/dwc2_udc_otg.c" enabled, output is (for the last transactions...): > > *** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 > *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 > EP2-OUT : DOEPINT = 0x2011 > complete_rx: RX DMA done : ep = 2, rx bytes = 4096/4096, is_short = 0, DOEPTSIZ = 0x0, remained bytes = 4096 > complete_rx: Next Rx request start... > setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb84f80,DOEPTSIZ = 0x401000, DOEPCTL = 0x80098200 > buf = 0xffb84f80, pktcnt = 8, xfersize = 4096 OK so we asked for 4096 bytes and looks like we received 3968 bytes, which is probably the end of transfer. > > *** dwc2_udc_irq : GINTSTS=0x14088028(on state WAIT_FOR_SETUP), GINTMSK : 0x800c3800,DAINT : 0x40000, DAINTMSK : 0x50003 > *** process_ep_out_intr: EP OUT interrupt : DAINT = 0x40000 > EP2-OUT : DOEPINT = 0x2011 > complete_rx: RX DMA done : ep = 2, rx bytes = 3968/4096, is_short = 0, DOEPTSIZ = 0x80, remained bytes = 3968 > setdma_rx: EP2 RX DMA start : DOEPDMA = 0xffb85f00,DOEPTSIZ = 0x80080, DOEPCTL = 0x80098200 > buf = 0xffb85f00, pktcnt = 1, xfersize = 128 > >>>>> and it hangs here!!! The dwc2 driver should have returned then and not queued another 128 bytes. IMO there is a bug in the dwc2 driver. 3968 = 7 x 512 + 384 This means the last packet (384 bytes) was a short packet and it signals end of transfer so the dwc2 driver shouldn't be queuing another transfer. It should end the usb ep_queue request. So, question to dwc2 mantainers. Can we modify dwc2 driver to not automatically queue a pending transfer if the transfer ended in a short packet? cheers, -roger > > > > > diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c > index 7961231..28b244a 100644 > --- a/drivers/usb/gadget/f_fastboot.c > +++ b/drivers/usb/gadget/f_fastboot.c > @@ -39,6 +39,11 @@ > #define TX_ENDPOINT_MAXIMUM_PACKET_SIZE (0x0040) > > #define EP_BUFFER_SIZE 4096 > +/* > + * EP_BUFFER_SIZE must always be an integral multiple of maxpacket size > + * (64 or 512 or 1024), else we break on certain controllers like DWC3 > + * that expect bulk OUT requests to be divisible by maxpacket size. > + */ > > struct f_fastboot { > struct usb_function usb_function; > @@ -57,7 +62,6 @@ static struct f_fastboot *fastboot_func; > static unsigned int fastboot_flash_session_id; > static unsigned int download_size; > static unsigned int download_bytes; > -static bool is_high_speed; > > static struct usb_endpoint_descriptor fs_ep_in = { > .bLength = USB_DT_ENDPOINT_SIZE, > @@ -269,11 +273,6 @@ static int fastboot_set_alt(struct usb_function *f, > debug("%s: func: %s intf: %d alt: %d\n", > __func__, f->name, interface, alt); > > - if (gadget->speed == USB_SPEED_HIGH) > - is_high_speed = true; > - else > - is_high_speed = false; > - > d = fb_ep_desc(gadget, &fs_ep_out, &hs_ep_out); > ret = usb_ep_enable(f_fb->out_ep, d); > if (ret) { > @@ -455,20 +454,27 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) > fastboot_tx_write_str(response); > } > > -static unsigned int rx_bytes_expected(unsigned int maxpacket) > +static unsigned int rx_bytes_expected(struct usb_ep *ep) > { > int rx_remain = download_size - download_bytes; > - int rem = 0; > - if (rx_remain < 0) > + unsigned int rem; > + unsigned int maxpacket = ep->maxpacket; > + > + if (rx_remain <= 0) > return 0; > - if (rx_remain > EP_BUFFER_SIZE) > + else if (rx_remain > EP_BUFFER_SIZE) > return EP_BUFFER_SIZE; > - if (rx_remain < maxpacket) { > - rx_remain = maxpacket; > - } else if (rx_remain % maxpacket != 0) { > - rem = rx_remain % maxpacket; > + > + /* > + * Some controllers e.g. DWC3 don't like OUT transfers to be > + * not ending in maxpacket boundary. So just make them happy by > + * always requesting for integral multiple of maxpackets. > + * This shouldn't bother controllers that don't care about it. > + */ > + rem = rx_remain % maxpacket; > + if (rem > 0) > rx_remain = rx_remain + (maxpacket - rem); > - } > + > return rx_remain; > } > > @@ -480,7 +486,6 @@ static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request *req) > const unsigned char *buffer = req->buf; > unsigned int buffer_size = req->actual; > unsigned int pre_dot_num, now_dot_num; > - unsigned int max; > > if (req->status != 0) { > printf("Bad status: %d\n", req->status); > @@ -518,11 +523,7 @@ static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request *req) > > printf("\ndownloading of %d bytes finished\n", download_bytes); > } else { > - max = is_high_speed ? hs_ep_out.wMaxPacketSize : > - fs_ep_out.wMaxPacketSize; > - req->length = rx_bytes_expected(max); > - if (req->length < ep->maxpacket) > - req->length = ep->maxpacket; > + req->length = rx_bytes_expected(ep); > } > > req->actual = 0; > @@ -533,7 +534,6 @@ static void cb_download(struct usb_ep *ep, struct usb_request *req) > { > char *cmd = req->buf; > char response[FASTBOOT_RESPONSE_LEN]; > - unsigned int max; > > strsep(&cmd, ":"); > download_size = simple_strtoul(cmd, NULL, 16); > @@ -549,11 +549,7 @@ static void cb_download(struct usb_ep *ep, struct usb_request *req) > } else { > sprintf(response, "DATA%08x", download_size); > req->complete = rx_handler_dl_image; > - max = is_high_speed ? hs_ep_out.wMaxPacketSize : > - fs_ep_out.wMaxPacketSize; > - req->length = rx_bytes_expected(max); > - if (req->length < ep->maxpacket) > - req->length = ep->maxpacket; > + req->length = rx_bytes_expected(ep); > } > fastboot_tx_write_str(response); > } > -- > 2.5.0 > >
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 7961231..28b244a 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -39,6 +39,11 @@ #define TX_ENDPOINT_MAXIMUM_PACKET_SIZE (0x0040) #define EP_BUFFER_SIZE 4096 +/* + * EP_BUFFER_SIZE must always be an integral multiple of maxpacket size + * (64 or 512 or 1024), else we break on certain controllers like DWC3 + * that expect bulk OUT requests to be divisible by maxpacket size. + */ struct f_fastboot { struct usb_function usb_function; @@ -57,7 +62,6 @@ static struct f_fastboot *fastboot_func; static unsigned int fastboot_flash_session_id; static unsigned int download_size; static unsigned int download_bytes; -static bool is_high_speed; static struct usb_endpoint_descriptor fs_ep_in = { .bLength = USB_DT_ENDPOINT_SIZE, @@ -269,11 +273,6 @@ static int fastboot_set_alt(struct usb_function *f, debug("%s: func: %s intf: %d alt: %d\n", __func__, f->name, interface, alt); - if (gadget->speed == USB_SPEED_HIGH) - is_high_speed = true; - else - is_high_speed = false; - d = fb_ep_desc(gadget, &fs_ep_out, &hs_ep_out); ret = usb_ep_enable(f_fb->out_ep, d); if (ret) { @@ -455,20 +454,27 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) fastboot_tx_write_str(response); } -static unsigned int rx_bytes_expected(unsigned int maxpacket) +static unsigned int rx_bytes_expected(struct usb_ep *ep) { int rx_remain = download_size - download_bytes; - int rem = 0; - if (rx_remain < 0) + unsigned int rem; + unsigned int maxpacket = ep->maxpacket; + + if (rx_remain <= 0) return 0; - if (rx_remain > EP_BUFFER_SIZE) + else if (rx_remain > EP_BUFFER_SIZE) return EP_BUFFER_SIZE; - if (rx_remain < maxpacket) { - rx_remain = maxpacket; - } else if (rx_remain % maxpacket != 0) { - rem = rx_remain % maxpacket; + + /* + * Some controllers e.g. DWC3 don't like OUT transfers to be + * not ending in maxpacket boundary. So just make them happy by + * always requesting for integral multiple of maxpackets. + * This shouldn't bother controllers that don't care about it. + */ + rem = rx_remain % maxpacket; + if (rem > 0) rx_remain = rx_remain + (maxpacket - rem); - } + return rx_remain; } @@ -480,7 +486,6 @@ static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request *req) const unsigned char *buffer = req->buf; unsigned int buffer_size = req->actual; unsigned int pre_dot_num, now_dot_num; - unsigned int max; if (req->status != 0) { printf("Bad status: %d\n", req->status); @@ -518,11 +523,7 @@ static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request *req) printf("\ndownloading of %d bytes finished\n", download_bytes); } else { - max = is_high_speed ? hs_ep_out.wMaxPacketSize : - fs_ep_out.wMaxPacketSize; - req->length = rx_bytes_expected(max); - if (req->length < ep->maxpacket) - req->length = ep->maxpacket; + req->length = rx_bytes_expected(ep); } req->actual = 0; @@ -533,7 +534,6 @@ static void cb_download(struct usb_ep *ep, struct usb_request *req) { char *cmd = req->buf; char response[FASTBOOT_RESPONSE_LEN]; - unsigned int max; strsep(&cmd, ":"); download_size = simple_strtoul(cmd, NULL, 16); @@ -549,11 +549,7 @@ static void cb_download(struct usb_ep *ep, struct usb_request *req) } else { sprintf(response, "DATA%08x", download_size); req->complete = rx_handler_dl_image; - max = is_high_speed ? hs_ep_out.wMaxPacketSize : - fs_ep_out.wMaxPacketSize; - req->length = rx_bytes_expected(max); - if (req->length < ep->maxpacket) - req->length = ep->maxpacket; + req->length = rx_bytes_expected(ep); } fastboot_tx_write_str(response); }