[U-Boot] [PATCH] fastboot: Fix OUT transaction length alignment
Roger Quadros
rogerq at ti.com
Mon Apr 18 09:56:39 CEST 2016
On 15/04/16 22:44, Steve Rae wrote:
>
>
> On Thu, Apr 14, 2016 at 3:18 AM, Roger Quadros <rogerq at ti.com <mailto:rogerq at 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 at ti.com <mailto:rogerq at ti.com>> wrote:
> >> Hi,
> >>
> >> On 13/04/16 15:01, Semen Protsenko wrote:
> >>> From: Sam Protsenko <semen.protsenko at linaro.org <mailto:semen.protsenko at 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 at linaro.org <mailto:semen.protsenko at 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
>
>
More information about the U-Boot
mailing list