[U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize"
Steve Rae
steve.rae at broadcom.com
Wed Mar 23 20:49:16 CET 2016
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?
>
>>
>>
>>>
>>> [1] http://omapzoom.org/?p=repo/u-boot.git;a=commit;h=92471c8af13493491a4118473279077fa6b765cf
More information about the U-Boot
mailing list