[U-Boot] [PATCH] fastboot: Fix OUT transaction length alignment

Steve Rae steve.rae at broadcom.com
Fri Apr 15 21:44:25 CEST 2016


On Thu, Apr 14, 2016 at 3:18 AM, Roger Quadros <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> wrote:
> >> Hi,
> >>
> >> On 13/04/16 15:01, Semen Protsenko wrote:
> >>> From: Sam Protsenko <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>
> >>> ---
> >>>  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

*** 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!!!




> 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