[U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize"
Sam Protsenko
semen.protsenko at linaro.org
Sun Apr 3 16:07:00 CEST 2016
On Wed, Mar 23, 2016 at 9:49 PM, Steve Rae <steve.rae at broadcom.com> wrote:
> On Wed, Mar 23, 2016 at 12:40 PM, Sam Protsenko
> <semen.protsenko at linaro.org> wrote:
>> On Wed, Mar 23, 2016 at 7:40 PM, Steve Rae <steve.rae at broadcom.com> wrote:
>>> Lukasz & Sam:
>>>
>>> On Wed, Mar 23, 2016 at 8:54 AM, Sam Protsenko
>>> <semen.protsenko at linaro.org> wrote:
>>>>
>>>> On Wed, Mar 23, 2016 at 11:49 AM, Lukasz Majewski
>>>> <l.majewski at samsung.com> wrote:
>>>> > Hi Marek,
>>>> >
>>>> >> On 03/23/2016 01:50 AM, Steve Rae wrote:
>>>> >> > This reverts commit 9e4b510d40310bf46e09f4edd0a0b6356213df47.
>>>> >> >
>>>> >> > Signed-off-by: Steve Rae <srae at broadcom.com>
>>>> >> > ---
>>>> >> > As discussed on the mailing list, this change breaks the download
>>>> >> > portion of fastboot by causing the server (the device running
>>>> >> > U-Boot) to wait forever for bytes which are never sent by the
>>>> >> > client (the host connected via USB).
>>>> >>
>>>> >> It'd be real cool if you could reference the ML discussion next time.
>>>> >>
>>>> >> > Does anyone know how to contact:
>>>> >> > Dileep Katta <dileep.katta at linaro.org>
>>>> >> > (this email bounces) ?
>>>> >>
>>>> >> No clue, sorry.
>>>> >
>>>> > Probably he is no longer employed by Linaro.
>>>> >
>>>> >>
>>>> >> Applied for now as it fixes real bug.
>>>> >
>>>> > I've a gut feeling that there are different versions of "fastboot"
>>>> > protocol.
>>>> >
>>>
>>> Yes - I suspect that also, which is why I test with two (2) fastboot clients:
>>> (1) from 2013 (and I don't know its origin), and
>>> (2) from the Adroid SDK (r24.4.1) for Linux - from
>>> developer.android.com; which requires
>>> the "Android SDK Platform-tools (rev 23.1)" to be installed...
>>> [ this is the current version - checked yesterday! ]
>>>
>>>> > What Steve is describing is that HOST is waiting for ZLP ACK.
>>>
>>> Lukasz:
>>> Is there any debugging I can enable to verify this?
>>>
>>>> >
>>>> > Anyway, I'm not able to test fastboot gadget since our (I mean
>>>> > Samsung's) boards aren't supporting fastboot - we use UMS, DFU, LTHOR.
>>>> >
>>>> > Hence, I can only rely on somebody's testing and review the code without
>>>> > the ability to know about implementation "quirks".
>>>> >
>>>> >>
>>>> >> > drivers/usb/gadget/f_fastboot.c | 27 +++++----------------------
>>>> >> > 1 file changed, 5 insertions(+), 22 deletions(-)
>>>> >> >
>>>> >> > diff --git a/drivers/usb/gadget/f_fastboot.c
>>>> >> > b/drivers/usb/gadget/f_fastboot.c index a54b4ee..887cf4f 100644
>>>> >> > --- a/drivers/usb/gadget/f_fastboot.c
>>>> >> > +++ b/drivers/usb/gadget/f_fastboot.c
>>>> >> > @@ -57,7 +57,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,
>>>> >> > @@ -241,13 +240,10 @@ static int fastboot_set_alt(struct
>>>> >> > usb_function *f, __func__, f->name, interface, alt);
>>>> >> >
>>>> >> > /* make sure we don't enable the ep twice */
>>>> >> > - if (gadget->speed == USB_SPEED_HIGH) {
>>>> >> > + if (gadget->speed == USB_SPEED_HIGH)
>>>> >> > ret = usb_ep_enable(f_fb->out_ep, &hs_ep_out);
>>>> >> > - is_high_speed = true;
>>>> >> > - } else {
>>>> >> > + else
>>>> >> > ret = usb_ep_enable(f_fb->out_ep, &fs_ep_out);
>>>> >> > - is_high_speed = false;
>>>> >> > - }
>>>> >> > if (ret) {
>>>> >> > puts("failed to enable out ep\n");
>>>> >> > return ret;
>>>> >> > @@ -419,20 +415,13 @@ 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(void)
>>>> >> > {
>>>> >> > int rx_remain = download_size - download_bytes;
>>>> >> > - int rem = 0;
>>>> >> > if (rx_remain < 0)
>>>> >> > return 0;
>>>> >> > 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;
>>>> >> > - rx_remain = rx_remain + (maxpacket - rem);
>>>> >> > - }
>>>> >> > return rx_remain;
>>>> >> > }
>>>> >> >
>>>> >> > @@ -444,7 +433,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);
>>>> >> > @@ -482,9 +470,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);
>>>> >> > + req->length = rx_bytes_expected();
>>>> >> > if (req->length < ep->maxpacket)
>>>> >> > req->length = ep->maxpacket;
>>>> >> > }
>>>> >> > @@ -497,7 +483,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);
>>>> >> > @@ -513,9 +498,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);
>>>> >> > + req->length = rx_bytes_expected();
>>>> >> > if (req->length < ep->maxpacket)
>>>> >> > req->length = ep->maxpacket;
>>>> >> > }
>>>> >> >
>>>> >>
>>>> >>
>>>> >
>>>> >
>>>> >
>>>> > --
>>>> > Best regards,
>>>> >
>>>> > Lukasz Majewski
>>>> >
>>>> > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
>>>> > _______________________________________________
>>>> > U-Boot mailing list
>>>> > U-Boot at lists.denx.de
>>>> > http://lists.denx.de/mailman/listinfo/u-boot
>>>>
>>>> + Angela Stegmaier
>>>>
>>>> Steve,
>>>>
>>>> Actually this code was originally added by Angela Stegmaier (see [1]).
>>>> Before reverting it, we should ask her opinion on that.
>>>>
>>>> I'm gonna verify this change on DRA7 EVM board today and see if
>>>> "fastboot flash" works fine (and it certainly was, before this
>>>> revert).
>>>
>>> Sam:
>>> Thanks!
>>>
>>> Could you please add the following printf lines, and let me know the results?
>>>
>>> diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
>>> index a54b4ee..2e3aa5f 100644
>>> --- a/drivers/usb/gadget/f_fastboot.c
>>> +++ b/drivers/usb/gadget/f_fastboot.c
>>> @@ -427,12 +427,14 @@ static unsigned int rx_bytes_expected(unsigned
>>> int maxpacket)
>>> return 0;
>>> if (rx_remain > EP_BUFFER_SIZE)
>>> return EP_BUFFER_SIZE;
>>> + printf("\nFIXME: rx_remain (before) = %d\n", rx_remain);
>>> if (rx_remain < maxpacket) {
>>> rx_remain = maxpacket;
>>> } else if (rx_remain % maxpacket != 0) {
>>> rem = rx_remain % maxpacket;
>>> rx_remain = rx_remain + (maxpacket - rem);
>>> }
>>> + printf("\nFIXME: rx_remain (after) = %d\n", rx_remain);
>>> return rx_remain;
>>> }
>>>
>>> mine show:
>>> FIXME: rx_remain (before) = 4072
>>> FIXME: rx_remain (after) = 4096
>>> and then it just hangs forever....
>>
>> First of all, with this revert my fastboot hangs forever (on "flash"
>> command). So we probably shouldn't merge it as is (although it may fix
>> fastboot on your platform, it also breaks fastboot on TI platforms).
>>
>> Next, I collected debug output that you asked me for.
>>
>> 1. Without your patch (my fastboot works):
>>
>> 1.1 fastboot flash xloader MLO
>>
>> Starting download of 69316 bytes
>> FIXME: rx_remain (before) = 3780
>> FIXME: rx_remain (after) = 4096
>> downloading of 69316 bytes finished
>> Flashing Raw Image
>> ........ wrote 69632 bytes to 'xloader'
>>
>> 1.2 fastboot flash bootloader u-boot.img
>>
>> Starting download of 350280 bytes
>> FIXME: rx_remain (before) = 2120
>> FIXME: rx_remain (after) = 2560
>> downloading of 350280 bytes finished
>> Flashing Raw Image
>> ........ wrote 350720 bytes to 'bootloader'
>>
>> 2. With your patch (my fastboot hangs on "flash" command):
>>
>> 2.1 fastboot flash xloader MLO
>>
>> Starting download of 69316 bytes
>> FIXME: rx_remain (after) = 3780
>>
>> 2.2 fastboot flash bootloader u-boot
>>
>> Starting download of 350072 bytes
>> FIXME: rx_remain (after) = 1912
>>
>> Probably you use some different host-side fastboot tool than me.
>>
>
> Appreciate these tests -- now I'm totally confused....
> ( as documented above, I'm using: )
> from the Adroid SDK (r24.4.1) for Linux - from
> developer.android.com; which requires
> the "Android SDK Platform-tools (rev 23.1)" to be installed...
> [ this is the current version - checked yesterday! ]
>
> what is your fastboot?
>
I'm using fastboot tool from Android Lollipop build (using [1]
instructions). And looking into git log for my
system/core/fastboot/fastboot.c, it seems like it doesn't have any
vendor-specific commits on top of it, which means it's mainline Google
fastboot.
So basically you can build mainline Android Lollipop AFS and get the
same fastboot tool as I have.
[1] http://omappedia.org/wiki/6AL.1.2_Release_Notes
>>
>>>
>>>
>>>>
>>>> [1] http://omapzoom.org/?p=repo/u-boot.git;a=commit;h=92471c8af13493491a4118473279077fa6b765cf
More information about the U-Boot
mailing list