Hi Dileep,

If the string is copied without NULL termination using strncpy(), then strncat() on the next line, may concatenate the string after some stale (or random) data, if the response string was not zero-initialized.

Signed-off-by: Dileep Katta dileep.katta@linaro.org

common/fb_mmc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/fb_mmc.c b/common/fb_mmc.c index 3911989..73055cc 100644 --- a/common/fb_mmc.c +++ b/common/fb_mmc.c @@ -23,13 +23,13 @@ static char *response_str;

void fastboot_fail(const char *s) {

• strncpy(response_str, "FAIL", 4);

• strncpy(response_str, "FAIL\0", 5); strncat(response_str, s, RESPONSE_LEN - 4 - 1);

}

void fastboot_okay(const char *s) {

• strncpy(response_str, "OKAY", 4);

• strncpy(response_str, "OKAY\0", 5); strncat(response_str, s, RESPONSE_LEN - 4 - 1);

}

Applied to u-boot-dfu branch.

Thanks for the patch!

On 15-02-12 10:33 PM, Dileep Katta wrote:

Configure the serial number using the serial# environment variable during the fastboot bind.

This enables "fastboot devices" to return the serial number for the attached devices.

Signed-off-by: Dileep Katta dileep.katta@linaro.org

drivers/usb/gadget/f_fastboot.c | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 0d53a61..d114c07 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -136,6 +136,7 @@ static int fastboot_bind(struct usb_configuration *c, struct usb_function *f) int id; struct usb_gadget *gadget = c->cdev->gadget; struct f_fastboot *f_fb = func_to_fastboot(f);

• const char *s;

  /* DYNAMIC interface numbers assignments */ id = usb_interface_id(c, f);

@@ -161,6 +162,10 @@ static int fastboot_bind(struct usb_configuration *c, struct usb_function *f)

hs_ep_out.bEndpointAddress = fs_ep_out.bEndpointAddress;

• s = getenv("serial#");
• if (s)

• g_dnl_set_serialnumber((char *)s);
  

• return 0; }

Acked-by: Steve Rae srae@broadcom.com

OUT transactions must be aligned to wMaxPacketSize for each transfer, or else transfer will not complete successfully. This patch modifies rx_bytes_expected to return a transfer length that is aligned to wMaxPacketSize.

Note that the value of wMaxPacketSize and ep->maxpacket may not be the same value, and it is the value of wMaxPacketSize that should be used for alignment. wMaxPacketSize is passed depending on the speed of connection.

Signed-off-by: Dileep Katta dileep.katta@linaro.org --- Changes in v2: - Corrected source of wMaxPacketSize Changes in v3: - Corrected the logic to accomodate both HS and FS speeds

drivers/usb/gadget/f_fastboot.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index a8d8205..2793590 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -55,6 +55,7 @@ static inline struct f_fastboot *func_to_fastboot(struct usb_function *f) static struct f_fastboot *fastboot_func; 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, @@ -219,10 +220,13 @@ 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); - else + is_high_speed = true; + } 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; @@ -370,13 +374,20 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) fastboot_tx_write_str(response); }

-static unsigned int rx_bytes_expected(void) +static unsigned int rx_bytes_expected(unsigned int maxpacket) { 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; }

@@ -388,6 +399,7 @@ 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); @@ -425,7 +437,9 @@ static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request *req)

printf("\ndownloading of %d bytes finished\n", download_bytes); } else { - req->length = rx_bytes_expected(); + max = is_high_speed ? hs_ep_out.wMaxPacketSize : + fs_ep_out.wMaxPacketSize; + req->length = rx_bytes_expected(max); if (req->length < ep->maxpacket) req->length = ep->maxpacket; } @@ -438,6 +452,7 @@ static void cb_download(struct usb_ep *ep, struct usb_request *req) { char *cmd = req->buf; char response[RESPONSE_LEN]; + unsigned int max;

strsep(&cmd, ":"); download_size = simple_strtoul(cmd, NULL, 16); @@ -453,7 +468,9 @@ 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; - req->length = rx_bytes_expected(); + max = is_high_speed ? hs_ep_out.wMaxPacketSize : + fs_ep_out.wMaxPacketSize; + req->length = rx_bytes_expected(max); if (req->length < ep->maxpacket) req->length = ep->maxpacket; }

... updated the subject line, was: Re: [U-Boot][PATCH v3 1/3] fastboot: OUT transaction length must be aligned to wMaxPacketSize

On 15-02-24 02:28 AM, Lukasz Majewski wrote:

Hi Dileep,

...

OUT transactions must be aligned to wMaxPacketSize for each transfer, or else transfer will not complete successfully. This patch modifies rx_bytes_expected to return a transfer length that is aligned to wMaxPacketSize.

Note that the value of wMaxPacketSize and ep->maxpacket may not be the same value, and it is the value of wMaxPacketSize that should be used for alignment. wMaxPacketSize is passed depending on the speed of connection.

Signed-off-by: Dileep Katta dileep.katta@linaro.org

Changes in v2:

• Corrected source of wMaxPacketSize

Changes in v3:

• Corrected the logic to accomodate both HS and FS speeds

drivers/usb/gadget/f_fastboot.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index a8d8205..2793590 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -55,6 +55,7 @@ static inline struct f_fastboot *func_to_fastboot(struct usb_function *f) static struct f_fastboot *fastboot_func; 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, @@ -219,10 +220,13 @@ 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);

• else

• is_high_speed = true;
  

• } 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;

@@ -370,13 +374,20 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) fastboot_tx_write_str(response); }

-static unsigned int rx_bytes_expected(void) +static unsigned int rx_bytes_expected(unsigned int maxpacket) { 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);
  

• }

Is anyone else having problems with this code??? I need to remove these six (newly added) lines in order to get my boards to work -- otherwise, they just "hang" druing the download phase of "fastboot flash"....

Thanks in advance, Steve

...

return rx_remain; }

@@ -388,6 +399,7 @@ 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);

@@ -425,7 +437,9 @@ static void rx_handler_dl_image(struct usb_ep *ep, struct usb_request *req) printf("\ndownloading of %d bytes finished\n", download_bytes); } else {

• req->length = rx_bytes_expected();
  

• max = is_high_speed ? hs_ep_out.wMaxPacketSize :
  

• 		fs_ep_out.wMaxPacketSize;
  

• req->length = rx_bytes_expected(max);
  

  if (req->length < ep->maxpacket) req->length = ep->maxpacket; }

@@ -438,6 +452,7 @@ static void cb_download(struct usb_ep *ep, struct usb_request *req) { char *cmd = req->buf; char response[RESPONSE_LEN];

• unsigned int max;

  strsep(&cmd, ":"); download_size = simple_strtoul(cmd, NULL, 16);

@@ -453,7 +468,9 @@ 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;

• req->length = rx_bytes_expected();
  

• max = is_high_speed ? hs_ep_out.wMaxPacketSize :
  

• 	fs_ep_out.wMaxPacketSize;
  

• req->length = rx_bytes_expected(max);
  

  if (req->length < ep->maxpacket) req->length = ep->maxpacket; }

Applied to u-boot-dfu branch.

Thanks for the patch!

On 15-02-13 09:59 AM, Dileep Katta wrote:

OUT transactions must be aligned to wMaxPacketSize for each transfer, or else transfer will not complete successfully. This patch modifies rx_bytes_expected to return a transfer length that is aligned to wMaxPacketSize.

Note that the value of wMaxPacketSize and ep->maxpacket may not be the same value, and it is the value of wMaxPacketSize that should be used for alignment.

Signed-off-by: Dileep Katta dileep.katta@linaro.org

Changes from v1:

• Corrected source of wMaxPacketSize

drivers/usb/gadget/f_fastboot.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index a8d8205..b18452e 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -370,13 +370,20 @@ static void cb_getvar(struct usb_ep *ep, struct usb_request *req) fastboot_tx_write_str(response); }

-static unsigned int rx_bytes_expected(void) +static unsigned int rx_bytes_expected(unsigned maxpacket) { 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; }

@@ -425,7 +432,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 {

• req->length = rx_bytes_expected();
  

• req->length = rx_bytes_expected(fs_ep_out.wMaxPacketSize);
  

  if (req->length < ep->maxpacket) req->length = ep->maxpacket; }

@@ -453,7 +460,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;

• req->length = rx_bytes_expected();
  

• req->length = rx_bytes_expected(fs_ep_out.wMaxPacketSize);
  

  if (req->length < ep->maxpacket) req->length = ep->maxpacket; }

I don't understand... Why are we always limiting this to the "Full Speed" size? ( see fs_ep_out versus hs_ep_out defined previously in this file... )

Thanks, Steve