Hi Paul,

Recent versions of the fastboot tool will query the partition type before doing an operation on a partition (such as erase, flash, etc). It will then submit the operation as soon as the response for the partition type is received.

Usually, the MUSB controller will see that the partition type request return status was read by the host at the very same time as the actual operation request is submitted by the host. However, the operation will be read first (int_rx is handled first in musb_interrupt) and after it is completed, the fastboot USB gadget driver will send another return status. Hence, this happens before the musb gadget framework has had a chance to handle the previous acknowledgement that the host read the return status and dequeue the request.

The host will then usually empty the FIFO by the time musb_interrupt gets around handling the return status acknowledgement (for the previous request, this is still on the same musb_interrupt call), so no other interrupt is generated and the most recent return status acknowledgement remains unaccounted for.

It will then be used as a response for the next command, and the proper response for it will be delayed to the next command, and so on.

Dequeuing the previous IN request in the fastboot code ensures that no previous return status remains. It is acceptable to do it since there is no callback to it anyways.

Signed-off-by: Paul Kocialkowski contact@paulk.fr

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

diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index b9a9099..60c846d 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -311,6 +311,9 @@ static int fastboot_tx_write(const char *buffer, unsigned int buffer_size) memcpy(in_req->buf, buffer, buffer_size); in_req->length = buffer_size;

• usb_ep_dequeue(fastboot_func->in_ep, in_req);
• ret = usb_ep_queue(fastboot_func->in_ep, in_req, 0); if (ret) printf("Error %d on queue\n", ret);

Reviewed-by: Lukasz Majewski l.majewski@samsung.com

Comment the same as for [PATCH 1/2]

Le lundi 06 juillet 2015 à 12:28 +0200, Lukasz Majewski a écrit :

Hi Paul,

...

This avoids handling requests that have an error status or no data. In particular, this avoids showing unnecessary error messages when the USB gadget gets disconnected (e.g. with fastboot continue) and the fastboot USB gadget driver sends an error back to the host (that has disconnected already).

Signed-off-by: Paul Kocialkowski contact@paulk.fr

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

diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 206b6d1..b9a9099 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -635,6 +635,9 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) void (*func_cb)(struct usb_ep *ep, struct usb_request *req) = NULL; int i;

• if (req->status != 0 || req->length == 0)

• return;
  

• for (i = 0; i < ARRAY_SIZE(cmd_dispatch_info); i++) { if (!strcmp_l1(cmd_dispatch_info[i].cmd, cmdbuf)) { func_cb = cmd_dispatch_info[i].cb;

@@ -656,9 +659,7 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) } }

• if (req->status == 0) {

• *cmdbuf = '\0';
  

• req->actual = 0;
  

• usb_ep_queue(ep, req, 0);
  

• }

• *cmdbuf = '\0';
• req->actual = 0;
• usb_ep_queue(ep, req, 0);

}

Reviewed-by: Lukasz Majewski l.majewski@samsung.com

Let's wait for some more time before pulling this patch to u-boot-dfu tree. I hope that other fastboot users will also review this patch.

Oh and by the way, in the fastboot protocol description (doc/README.android-fastboot-protocol), it clearly says: "zero-length packets are ignored".

Le mardi 14 juillet 2015 à 12:22 +0200, Lukasz Majewski a écrit :

Hi Paul,

...

This avoids handling requests that have an error status or no data. In particular, this avoids showing unnecessary error messages when the USB gadget gets disconnected (e.g. with fastboot continue) and the fastboot USB gadget driver sends an error back to the host (that has disconnected already).

Signed-off-by: Paul Kocialkowski contact@paulk.fr

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

diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 206b6d1..b9a9099 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -635,6 +635,9 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) void (*func_cb)(struct usb_ep *ep, struct usb_request *req) = NULL; int i;

• if (req->status != 0 || req->length == 0)

• return;
  

• for (i = 0; i < ARRAY_SIZE(cmd_dispatch_info); i++) { if (!strcmp_l1(cmd_dispatch_info[i].cmd, cmdbuf)) { func_cb = cmd_dispatch_info[i].cb;

@@ -656,9 +659,7 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) } }

• if (req->status == 0) {

• *cmdbuf = '\0';
  

• req->actual = 0;
  

• usb_ep_queue(ep, req, 0);
  

• }

• *cmdbuf = '\0';
• req->actual = 0;
• usb_ep_queue(ep, req, 0);

}

Any more comments to those patches? I mean 1/2 and 2/2.

I could test them on OMAP3 and OMAP5 if you'd like. Together with sunxi (that I developped this on), I think a fair share of platforms using musb-new would be covered. Testing on am33xx and omap4 would be nice, too.

Le mardi 14 juillet 2015 à 13:32 +0200, Lukasz Majewski a écrit :

Hi Paul,

...

Le mardi 14 juillet 2015 à 12:22 +0200, Lukasz Majewski a écrit :

...

Hi Paul,

...

This avoids handling requests that have an error status or no data. In particular, this avoids showing unnecessary error messages when the USB gadget gets disconnected (e.g. with fastboot continue) and the fastboot USB gadget driver sends an error back to the host (that has disconnected already).

Signed-off-by: Paul Kocialkowski contact@paulk.fr

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

diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 206b6d1..b9a9099 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -635,6 +635,9 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) void (*func_cb)(struct usb_ep *ep, struct usb_request *req) = NULL; int i;

• if (req->status != 0 || req->length == 0)

• return;
  

• for (i = 0; i < ARRAY_SIZE(cmd_dispatch_info); i++) { if (!strcmp_l1(cmd_dispatch_info[i].cmd,

cmdbuf)) { func_cb = cmd_dispatch_info[i].cb; @@ -656,9 +659,7 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) } }

• if (req->status == 0) {

• *cmdbuf = '\0';
  

• req->actual = 0;
  

• usb_ep_queue(ep, req, 0);
  

• }

• *cmdbuf = '\0';
• req->actual = 0;
• usb_ep_queue(ep, req, 0);

}

Any more comments to those patches? I mean 1/2 and 2/2.

I could test them on OMAP3 and OMAP5 if you'd like. Together with sunxi (that I developped this on), I think a fair share of platforms using musb-new would be covered. Testing on am33xx and omap4 would be nice, too.

More testing is always welcome. Please write proper Tested-by with target SoC/board name as a reply to this e-mail.

In the end, the OMAP5 uses another OTG controller, so it doesn't apply. However, I could test on the LG Optimus Black (P970), which is not yet upstream (I'll submit patches soon).

Tested-By: Paul Kocialkowski contact@paulk.fr on the Cubieboard2 (sunxi) and the LG Optimus Black (P970) (omap3)

Then I will pick those patches to -dfu tree.

Thanks, I'm looking forward to it, and to see it land it the main tree as well, now that the merge window is open!