* [U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize" @ 2016-03-23 0:50 Steve Rae 2016-03-23 2:04 ` Marek Vasut 2016-03-24 17:15 ` Tom Rini 0 siblings, 2 replies; 13+ messages in thread From: Steve Rae @ 2016-03-23 0:50 UTC (permalink / raw) To: u-boot This reverts commit 9e4b510d40310bf46e09f4edd0a0b6356213df47. Signed-off-by: Steve Rae <srae@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). Does anyone know how to contact: Dileep Katta <dileep.katta@linaro.org> (this email bounces) ? 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; } -- 1.8.5 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize" 2016-03-23 0:50 [U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize" Steve Rae @ 2016-03-23 2:04 ` Marek Vasut 2016-03-23 9:49 ` Lukasz Majewski 2016-03-24 17:15 ` Tom Rini 1 sibling, 1 reply; 13+ messages in thread From: Marek Vasut @ 2016-03-23 2:04 UTC (permalink / raw) To: u-boot On 03/23/2016 01:50 AM, Steve Rae wrote: > This reverts commit 9e4b510d40310bf46e09f4edd0a0b6356213df47. > > Signed-off-by: Steve Rae <srae@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@linaro.org> > (this email bounces) ? No clue, sorry. Applied for now as it fixes real bug. > 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, Marek Vasut ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize" 2016-03-23 2:04 ` Marek Vasut @ 2016-03-23 9:49 ` Lukasz Majewski 2016-03-23 15:54 ` Sam Protsenko 0 siblings, 1 reply; 13+ messages in thread From: Lukasz Majewski @ 2016-03-23 9:49 UTC (permalink / raw) To: u-boot Hi Marek, > On 03/23/2016 01:50 AM, Steve Rae wrote: > > This reverts commit 9e4b510d40310bf46e09f4edd0a0b6356213df47. > > > > Signed-off-by: Steve Rae <srae@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@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. What Steve is describing is that HOST is waiting for ZLP ACK. 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize" 2016-03-23 9:49 ` Lukasz Majewski @ 2016-03-23 15:54 ` Sam Protsenko 2016-03-23 17:40 ` Steve Rae 0 siblings, 1 reply; 13+ messages in thread From: Sam Protsenko @ 2016-03-23 15:54 UTC (permalink / raw) To: u-boot On Wed, Mar 23, 2016 at 11:49 AM, Lukasz Majewski <l.majewski@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@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@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. > > What Steve is describing is that HOST is waiting for ZLP ACK. > > 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). [1] http://omapzoom.org/?p=repo/u-boot.git;a=commit;h=92471c8af13493491a4118473279077fa6b765cf ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize" 2016-03-23 15:54 ` Sam Protsenko @ 2016-03-23 17:40 ` Steve Rae 2016-03-23 19:40 ` Sam Protsenko 0 siblings, 1 reply; 13+ messages in thread From: Steve Rae @ 2016-03-23 17:40 UTC (permalink / raw) To: u-boot Lukasz & Sam: On Wed, Mar 23, 2016 at 8:54 AM, Sam Protsenko <semen.protsenko@linaro.org> wrote: > > On Wed, Mar 23, 2016 at 11:49 AM, Lukasz Majewski > <l.majewski@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@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@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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize" 2016-03-23 17:40 ` Steve Rae @ 2016-03-23 19:40 ` Sam Protsenko 2016-03-23 19:49 ` Steve Rae 0 siblings, 1 reply; 13+ messages in thread From: Sam Protsenko @ 2016-03-23 19:40 UTC (permalink / raw) To: u-boot On Wed, Mar 23, 2016 at 7:40 PM, Steve Rae <steve.rae@broadcom.com> wrote: > Lukasz & Sam: > > On Wed, Mar 23, 2016 at 8:54 AM, Sam Protsenko > <semen.protsenko@linaro.org> wrote: >> >> On Wed, Mar 23, 2016 at 11:49 AM, Lukasz Majewski >> <l.majewski@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@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@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. > > >> >> [1] http://omapzoom.org/?p=repo/u-boot.git;a=commit;h=92471c8af13493491a4118473279077fa6b765cf ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize" 2016-03-23 19:40 ` Sam Protsenko @ 2016-03-23 19:49 ` Steve Rae 2016-04-03 14:07 ` Sam Protsenko 0 siblings, 1 reply; 13+ messages in thread From: Steve Rae @ 2016-03-23 19:49 UTC (permalink / raw) To: u-boot On Wed, Mar 23, 2016 at 12:40 PM, Sam Protsenko <semen.protsenko@linaro.org> wrote: > On Wed, Mar 23, 2016 at 7:40 PM, Steve Rae <steve.rae@broadcom.com> wrote: >> Lukasz & Sam: >> >> On Wed, Mar 23, 2016 at 8:54 AM, Sam Protsenko >> <semen.protsenko@linaro.org> wrote: >>> >>> On Wed, Mar 23, 2016 at 11:49 AM, Lukasz Majewski >>> <l.majewski@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@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@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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize" 2016-03-23 19:49 ` Steve Rae @ 2016-04-03 14:07 ` Sam Protsenko 0 siblings, 0 replies; 13+ messages in thread From: Sam Protsenko @ 2016-04-03 14:07 UTC (permalink / raw) To: u-boot On Wed, Mar 23, 2016 at 9:49 PM, Steve Rae <steve.rae@broadcom.com> wrote: > On Wed, Mar 23, 2016 at 12:40 PM, Sam Protsenko > <semen.protsenko@linaro.org> wrote: >> On Wed, Mar 23, 2016 at 7:40 PM, Steve Rae <steve.rae@broadcom.com> wrote: >>> Lukasz & Sam: >>> >>> On Wed, Mar 23, 2016 at 8:54 AM, Sam Protsenko >>> <semen.protsenko@linaro.org> wrote: >>>> >>>> On Wed, Mar 23, 2016 at 11:49 AM, Lukasz Majewski >>>> <l.majewski@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@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@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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize" 2016-03-23 0:50 [U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize" Steve Rae 2016-03-23 2:04 ` Marek Vasut @ 2016-03-24 17:15 ` Tom Rini 2016-04-03 14:19 ` Sam Protsenko 1 sibling, 1 reply; 13+ messages in thread From: Tom Rini @ 2016-03-24 17:15 UTC (permalink / raw) To: u-boot On Tue, Mar 22, 2016 at 05:50:41PM -0700, Steve Rae wrote: > This reverts commit 9e4b510d40310bf46e09f4edd0a0b6356213df47. > > Signed-off-by: Steve Rae <srae@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). > > Does anyone know how to contact: > Dileep Katta <dileep.katta@linaro.org> > (this email bounces) ? Sam, can you comment / ack this? Dileep isn't with Linaro anymore and I assume isn't overly interested in this area anymore. Thanks! > > 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; > } > -- > 1.8.5 > -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160324/4d8214e1/attachment.sig> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize" 2016-03-24 17:15 ` Tom Rini @ 2016-04-03 14:19 ` Sam Protsenko 2016-04-04 17:16 ` Steve Rae 0 siblings, 1 reply; 13+ messages in thread From: Sam Protsenko @ 2016-04-03 14:19 UTC (permalink / raw) To: u-boot On Thu, Mar 24, 2016 at 7:15 PM, Tom Rini <trini@konsulko.com> wrote: > On Tue, Mar 22, 2016 at 05:50:41PM -0700, Steve Rae wrote: > >> This reverts commit 9e4b510d40310bf46e09f4edd0a0b6356213df47. >> >> Signed-off-by: Steve Rae <srae@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). >> >> Does anyone know how to contact: >> Dileep Katta <dileep.katta@linaro.org> >> (this email bounces) ? > > Sam, can you comment / ack this? Dileep isn't with Linaro anymore and I > assume isn't overly interested in this area anymore. Thanks! > Yes, Dileep is not with Linaro anymore. But I don't really know how to contact him, if it's what you are asking. As for this revert -- I already mentioned in this thread that it breaks TI platforms, causing them to hang forever on "fastboot flash" command. So I guess we need to root cause the problem itself instead of reverting useful code. Steve, Can you please try fastboot tool from "android-tools-fastboot" package (available in Debian and Ubuntu)? It works for me as well as fastboot tool from Android Lollipop build, but this way you don't have to build the whole AFS just to test fastboot tool. Thanks. >> >> 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; >> } >> -- >> 1.8.5 >> > > -- > Tom ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize" 2016-04-03 14:19 ` Sam Protsenko @ 2016-04-04 17:16 ` Steve Rae 2016-04-05 12:09 ` Sam Protsenko 0 siblings, 1 reply; 13+ messages in thread From: Steve Rae @ 2016-04-04 17:16 UTC (permalink / raw) To: u-boot That one fails for me too.... So I suspect that a conditional is required - something like: diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index a54b4ee..41ec211 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -422,17 +422,19 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) static unsigned int rx_bytes_expected(unsigned int maxpacket) { int rx_remain = download_size - download_bytes; - int rem = 0; + int __maybe_unused rem = 0; if (rx_remain < 0) return 0; if (rx_remain > EP_BUFFER_SIZE) return EP_BUFFER_SIZE; +#if XXX_alignment_required if (rx_remain < maxpacket) { rx_remain = maxpacket; } else if (rx_remain % maxpacket != 0) { rem = rx_remain % maxpacket; rx_remain = rx_remain + (maxpacket - rem); } +#endif return rx_remain; } If you submit a PATCH like this, then I will abort the request to "revert" this PATCH.... Thanks, Steve On Sun, Apr 3, 2016 at 7:19 AM, Sam Protsenko <semen.protsenko@linaro.org> wrote: > On Thu, Mar 24, 2016 at 7:15 PM, Tom Rini <trini@konsulko.com> wrote: > > On Tue, Mar 22, 2016 at 05:50:41PM -0700, Steve Rae wrote: > > > >> This reverts commit 9e4b510d40310bf46e09f4edd0a0b6356213df47. > >> > >> Signed-off-by: Steve Rae <srae@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). > >> > >> Does anyone know how to contact: > >> Dileep Katta <dileep.katta@linaro.org> > >> (this email bounces) ? > > > > Sam, can you comment / ack this? Dileep isn't with Linaro anymore and I > > assume isn't overly interested in this area anymore. Thanks! > > > > Yes, Dileep is not with Linaro anymore. But I don't really know how to > contact him, if it's what you are asking. As for this revert -- I > already mentioned in this thread that it breaks TI platforms, causing > them to hang forever on "fastboot flash" command. So I guess we need > to root cause the problem itself instead of reverting useful code. > > Steve, > > Can you please try fastboot tool from "android-tools-fastboot" package > (available in Debian and Ubuntu)? It works for me as well as fastboot > tool from Android Lollipop build, but this way you don't have to build > the whole AFS just to test fastboot tool. > > Thanks. > > >> > >> 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; > >> } > >> -- > >> 1.8.5 > >> > > > > -- > > Tom > ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize" 2016-04-04 17:16 ` Steve Rae @ 2016-04-05 12:09 ` Sam Protsenko 2016-04-05 12:59 ` Marek Vasut 0 siblings, 1 reply; 13+ messages in thread From: Sam Protsenko @ 2016-04-05 12:09 UTC (permalink / raw) To: u-boot On Mon, Apr 4, 2016 at 8:16 PM, Steve Rae <steve.rae@broadcom.com> wrote: > That one fails for me too.... > > So I suspect that a conditional is required - something like: > > diff --git a/drivers/usb/gadget/f_fastboot.c > b/drivers/usb/gadget/f_fastboot.c > index a54b4ee..41ec211 100644 > --- a/drivers/usb/gadget/f_fastboot.c > +++ b/drivers/usb/gadget/f_fastboot.c > @@ -422,17 +422,19 @@ static void cb_getvar(struct usb_ep *ep, struct > usb_request *req) > static unsigned int rx_bytes_expected(unsigned int maxpacket) > { > int rx_remain = download_size - download_bytes; > - int rem = 0; > + int __maybe_unused rem = 0; > if (rx_remain < 0) > return 0; > if (rx_remain > EP_BUFFER_SIZE) > return EP_BUFFER_SIZE; > +#if XXX_alignment_required > if (rx_remain < maxpacket) { > rx_remain = maxpacket; > } else if (rx_remain % maxpacket != 0) { > rem = rx_remain % maxpacket; > rx_remain = rx_remain + (maxpacket - rem); > } > +#endif > return rx_remain; > } > > If you submit a PATCH like this, then I will abort the request to "revert" > this PATCH.... > Thanks, Steve Well, I'm pretty busy doing another work right now, and obviously revert shouldn't be merged anyway (as it breaks another platforms). But you can go ahead and send v2 of this patch, adding corresponding options (perhaps to Kconfig) and testing them via #ifdef, and I will verify if it works fine for TI platforms. Also, it would be good to know why that stuck is happening for you in the first place, so we can come up with real solution to this problem instead of fixing it with hack. > > > > On Sun, Apr 3, 2016 at 7:19 AM, Sam Protsenko <semen.protsenko@linaro.org> > wrote: >> >> On Thu, Mar 24, 2016 at 7:15 PM, Tom Rini <trini@konsulko.com> wrote: >> > On Tue, Mar 22, 2016 at 05:50:41PM -0700, Steve Rae wrote: >> > >> >> This reverts commit 9e4b510d40310bf46e09f4edd0a0b6356213df47. >> >> >> >> Signed-off-by: Steve Rae <srae@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). >> >> >> >> Does anyone know how to contact: >> >> Dileep Katta <dileep.katta@linaro.org> >> >> (this email bounces) ? >> > >> > Sam, can you comment / ack this? Dileep isn't with Linaro anymore and I >> > assume isn't overly interested in this area anymore. Thanks! >> > >> >> Yes, Dileep is not with Linaro anymore. But I don't really know how to >> contact him, if it's what you are asking. As for this revert -- I >> already mentioned in this thread that it breaks TI platforms, causing >> them to hang forever on "fastboot flash" command. So I guess we need >> to root cause the problem itself instead of reverting useful code. >> >> Steve, >> >> Can you please try fastboot tool from "android-tools-fastboot" package >> (available in Debian and Ubuntu)? It works for me as well as fastboot >> tool from Android Lollipop build, but this way you don't have to build >> the whole AFS just to test fastboot tool. >> >> Thanks. >> >> >> >> >> 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; >> >> } >> >> -- >> >> 1.8.5 >> >> >> > >> > -- >> > Tom > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize" 2016-04-05 12:09 ` Sam Protsenko @ 2016-04-05 12:59 ` Marek Vasut 0 siblings, 0 replies; 13+ messages in thread From: Marek Vasut @ 2016-04-05 12:59 UTC (permalink / raw) To: u-boot On 04/05/2016 02:09 PM, Sam Protsenko wrote: > On Mon, Apr 4, 2016 at 8:16 PM, Steve Rae <steve.rae@broadcom.com> wrote: >> That one fails for me too.... >> >> So I suspect that a conditional is required - something like: >> >> diff --git a/drivers/usb/gadget/f_fastboot.c >> b/drivers/usb/gadget/f_fastboot.c >> index a54b4ee..41ec211 100644 >> --- a/drivers/usb/gadget/f_fastboot.c >> +++ b/drivers/usb/gadget/f_fastboot.c >> @@ -422,17 +422,19 @@ static void cb_getvar(struct usb_ep *ep, struct >> usb_request *req) >> static unsigned int rx_bytes_expected(unsigned int maxpacket) >> { >> int rx_remain = download_size - download_bytes; >> - int rem = 0; >> + int __maybe_unused rem = 0; >> if (rx_remain < 0) >> return 0; >> if (rx_remain > EP_BUFFER_SIZE) >> return EP_BUFFER_SIZE; >> +#if XXX_alignment_required >> if (rx_remain < maxpacket) { >> rx_remain = maxpacket; >> } else if (rx_remain % maxpacket != 0) { >> rem = rx_remain % maxpacket; >> rx_remain = rx_remain + (maxpacket - rem); >> } >> +#endif >> return rx_remain; >> } >> >> If you submit a PATCH like this, then I will abort the request to "revert" >> this PATCH.... >> Thanks, Steve > > Well, I'm pretty busy doing another work right now, and obviously > revert shouldn't be merged anyway (as it breaks another platforms). The revert is not applied. > But you can go ahead and send v2 of this patch, adding corresponding > options (perhaps to Kconfig) and testing them via #ifdef, and I will > verify if it works fine for TI platforms. > > Also, it would be good to know why that stuck is happening for you in > the first place, so we can come up with real solution to this problem > instead of fixing it with hack. Right Best regards, Marek Vasut ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-04-05 12:59 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-03-23 0:50 [U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize" Steve Rae 2016-03-23 2:04 ` Marek Vasut 2016-03-23 9:49 ` Lukasz Majewski 2016-03-23 15:54 ` Sam Protsenko 2016-03-23 17:40 ` Steve Rae 2016-03-23 19:40 ` Sam Protsenko 2016-03-23 19:49 ` Steve Rae 2016-04-03 14:07 ` Sam Protsenko 2016-03-24 17:15 ` Tom Rini 2016-04-03 14:19 ` Sam Protsenko 2016-04-04 17:16 ` Steve Rae 2016-04-05 12:09 ` Sam Protsenko 2016-04-05 12:59 ` Marek Vasut
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox