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

[Sam Protsenko]

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.

> 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