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

[Steve Rae]

On Wed, Apr 13, 2016 at 9:56 AM, Sam Protsenko <semen.protsenko at linaro.org>
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;
> >>  }
> >>
> >>
>

yes -- anything that skips those 6 lines will work on my boards....


> >
> > 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.
>
> > 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.
>
> > Why is the code using wMaxPacketSize for the check. why can't it just
> use usb_ep->maxpacket?
> >
>
> I just reused already existed code. Sure we can use usb_ep->maxpacket
> instead, it's also 512 in my case (I believe it's assigned in DWC3
> code).
>
> [1]
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=219580e64f035bb9018dbb08d340f90b0ac50f8c
>
>
> > cheers,
> > -roger
>