[U-Boot] [PATCH v1] Revert "fastboot: OUT transaction length must be aligned to wMaxPacketSize"
Sam Protsenko
semen.protsenko at linaro.org
Tue Apr 5 14:09:03 CEST 2016
On Mon, Apr 4, 2016 at 8:16 PM, Steve Rae <steve.rae at 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 at linaro.org>
> wrote:
>>
>> On Thu, Mar 24, 2016 at 7:15 PM, Tom Rini <trini at 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 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).
>> >>
>> >> Does anyone know how to contact:
>> >> Dileep Katta <dileep.katta at 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
>
>
More information about the U-Boot
mailing list