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