[U-Boot] [PATCH 1/2] usb: gadget: fastboot: Request status and length check in rx handler

[U-Boot] [PATCH 1/2] usb: gadget: fastboot: Request status and length check in rx handler

[Paul Kocialkowski]

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);
 }
-- 
1.9.1

[U-Boot] [PATCH 2/2] usb: gadget: fastboot: Dequeue the previous IN request for the current request

[Paul Kocialkowski]

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);
-- 
1.9.1

[U-Boot] [PATCH 2/2] usb: gadget: fastboot: Dequeue the previous IN request for the current request

[Lukasz Majewski]

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]

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] Subject

[Eugeniu Rosca]

Hi Marek, Paul, cc: Alex

jFYI/FWIW, reverting [1-2] allows getting rid of below warnings on
R-Car3, when running basic fastboot commands (e.g. fastboot getvar):

[    8.035764] status: -104 ep 'ep1' trans: 0
[   18.744354] status: -104 ep 'ep1' trans: 28
[   18.748950] status: -104 ep 'ep1' trans: 9
[   18.753370] status: -104 ep 'ep1' trans: 28
[   26.064761] status: -104 ep 'ep1' trans: 28

This has been pointed out by Dmytro (To:) in one of his patches which
reached us via official Renesas release. Since R-Car USB gadget driver
is not yet present in mainline [3], the behavior cannot be reproduced
with vanilla U-Boot. In case, at some point in time, you hit the same
problem and come to the same or an alternative solution, please share.
TIA!

[1] https://patchwork.ozlabs.org/patch/491249/ ("[U-Boot,2/2] usb: gadget: fastboot: Dequeue the previous IN request for the current request")
[2] http://git.denx.de/?p=u-boot.git;a=commitdiff;h=bc9071c9f318 ("usb: gadget: fastboot: Dequeue the previous IN request for the current request")
[3] https://patchwork.ozlabs.org/patch/978026/#2107803

Best regards,
Eugeniu.

[U-Boot] Subject

[Paul Kocialkowski]

Hi,

Le lundi 18 mars 2019 à 10:56 +0100, Eugeniu Rosca a écrit :
> Hi Marek, Paul, cc: Alex
> 
> jFYI/FWIW, reverting [1-2] allows getting rid of below warnings on
> R-Car3, when running basic fastboot commands (e.g. fastboot getvar):
> 
> [    8.035764] status: -104 ep 'ep1' trans: 0
> [   18.744354] status: -104 ep 'ep1' trans: 28
> [   18.748950] status: -104 ep 'ep1' trans: 9
> [   18.753370] status: -104 ep 'ep1' trans: 28
> [   26.064761] status: -104 ep 'ep1' trans: 28

I don't think reverting this patch in mainline U-Boot would be a good
idea, as it is required to get fastboot to work at all on sunxi
platforms with the MUSB controller.

Did you investigate the issue to see what is happening here precisely?

Cheers,

Paul

> This has been pointed out by Dmytro (To:) in one of his patches which
> reached us via official Renesas release. Since R-Car USB gadget driver
> is not yet present in mainline [3], the behavior cannot be reproduced
> with vanilla U-Boot. In case, at some point in time, you hit the same
> problem and come to the same or an alternative solution, please share.
> TIA!
> 
> [1] https://patchwork.ozlabs.org/patch/491249/ ("[U-Boot,2/2] usb: gadget: fastboot: Dequeue the previous IN request for the current request")
> [2] http://git.denx.de/?p=u-boot.git;a=commitdiff;h=bc9071c9f318 ("usb: gadget: fastboot: Dequeue the previous IN request for the current request")
> [3] https://patchwork.ozlabs.org/patch/978026/#2107803
> 
> Best regards,
> Eugeniu.

[U-Boot] Subject

[Eugeniu Rosca]

Hi Paul, hello Marek,

On Mon, Mar 18, 2019 at 11:12:02AM +0100, Paul Kocialkowski wrote:
> Hi,
> 
> Le lundi 18 mars 2019 à 10:56 +0100, Eugeniu Rosca a écrit :
> > Hi Marek, Paul, cc: Alex
> > 
> > jFYI/FWIW, reverting [1-2] allows getting rid of below warnings on
> > R-Car3, when running basic fastboot commands (e.g. fastboot getvar):
> > 
> > [    8.035764] status: -104 ep 'ep1' trans: 0
> > [   18.744354] status: -104 ep 'ep1' trans: 28
> > [   18.748950] status: -104 ep 'ep1' trans: 9
> > [   18.753370] status: -104 ep 'ep1' trans: 28
> > [   26.064761] status: -104 ep 'ep1' trans: 28
> 
> I don't think reverting this patch in mainline U-Boot would be a good
> idea, as it is required to get fastboot to work at all on sunxi
> platforms with the MUSB controller.
> 
> Did you investigate the issue to see what is happening here precisely?

I attempted to record some traces to visualize the functions called
during my use-case, but it seems like lib/trace.c and cmd/trace.c are
not usable on arm64 (no U-Boot console output on my H3-ULCB-KF). Anyone
with a hint in this direction?

Anyway, given that I use an out-of-tree USB gadget driver, any logs I
can share will only be of limited/little help, I assume. I expect the
discussion to make more sense during upstreaming of USBHS R-Car Linux
driver. Now, it is more of a heads up.

Best regards,
Eugeniu.

[U-Boot] [U-Boot, 2/2] usb: gadget: fastboot: Dequeue the previous IN request for the current request

[Eugeniu Rosca]

-bouncing e-mails
cc: Łukasz

On Mon, Mar 18, 2019 at 06:15:48PM +0100, Eugeniu Rosca wrote:
> Hi Paul, hello Marek,
> 
> On Mon, Mar 18, 2019 at 11:12:02AM +0100, Paul Kocialkowski wrote:
> > Hi,
> > 
> > Le lundi 18 mars 2019 à 10:56 +0100, Eugeniu Rosca a écrit :
> > > Hi Marek, Paul, cc: Alex
> > > 
> > > jFYI/FWIW, reverting [1-2] allows getting rid of below warnings on
> > > R-Car3, when running basic fastboot commands (e.g. fastboot getvar):
> > > 
> > > [    8.035764] status: -104 ep 'ep1' trans: 0
> > > [   18.744354] status: -104 ep 'ep1' trans: 28
> > > [   18.748950] status: -104 ep 'ep1' trans: 9
> > > [   18.753370] status: -104 ep 'ep1' trans: 28
> > > [   26.064761] status: -104 ep 'ep1' trans: 28
> > 
> > I don't think reverting this patch in mainline U-Boot would be a good
> > idea, as it is required to get fastboot to work at all on sunxi
> > platforms with the MUSB controller.
> > 
> > Did you investigate the issue to see what is happening here precisely?
> 
> I attempted to record some traces to visualize the functions called
> during my use-case, but it seems like lib/trace.c and cmd/trace.c are
> not usable on arm64 (no U-Boot console output on my H3-ULCB-KF). Anyone
> with a hint in this direction?
> 
> Anyway, given that I use an out-of-tree USB gadget driver, any logs I
> can share will only be of limited/little help, I assume. I expect the
> discussion to make more sense during upstreaming of USBHS R-Car Linux
> driver. Now, it is more of a heads up.
> 
> Best regards,
> Eugeniu.

[U-Boot] Subject

[Marek Vasut]

On 3/18/19 10:56 AM, Eugeniu Rosca wrote:
> Hi Marek, Paul, cc: Alex

Hi,

> jFYI/FWIW, reverting [1-2] allows getting rid of below warnings on
> R-Car3, when running basic fastboot commands (e.g. fastboot getvar):

Maybe a more constructive approach would be to send a patch fixing the
issue instead of reverting patches :)

> [    8.035764] status: -104 ep 'ep1' trans: 0
> [   18.744354] status: -104 ep 'ep1' trans: 28
> [   18.748950] status: -104 ep 'ep1' trans: 9
> [   18.753370] status: -104 ep 'ep1' trans: 28
> [   26.064761] status: -104 ep 'ep1' trans: 28
> 
> This has been pointed out by Dmytro (To:) in one of his patches which
> reached us via official Renesas release. Since R-Car USB gadget driver
> is not yet present in mainline [3], the behavior cannot be reproduced
> with vanilla U-Boot. In case, at some point in time, you hit the same
> problem and come to the same or an alternative solution, please share.
> TIA!

Since this is gadget code, CCing U-Boot USB gadget maintainer.

> [1] https://patchwork.ozlabs.org/patch/491249/ ("[U-Boot,2/2] usb: gadget: fastboot: Dequeue the previous IN request for the current request")
> [2] http://git.denx.de/?p=u-boot.git;a=commitdiff;h=bc9071c9f318 ("usb: gadget: fastboot: Dequeue the previous IN request for the current request")
> [3] https://patchwork.ozlabs.org/patch/978026/#2107803
> 
> Best regards,
> Eugeniu.
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 1/2] usb: gadget: fastboot: Request status and length check in rx handler

[Lukasz Majewski]

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.

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 1/2] usb: gadget: fastboot: Request status and length check in rx handler

[Paul Kocialkowski]

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>

Thanks!

> 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.

That would be nice, but it should only affect people using a recent
version of fastboot. I have also tested this with other (older) versions
(for which everything worked fine already).

However, I certainly haven't covered all use cases, so more testing is
indeed welcome!

-- 
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: http://www.replicant.us/
Blog: http://blog.replicant.us/
Wiki/tracker/forums: http://redmine.replicant.us/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150706/1fe9b008/attachment.sig>

[U-Boot] [PATCH 1/2] usb: gadget: fastboot: Request status and length check in rx handler

[Paul Kocialkowski]

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".

-- 
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: http://www.replicant.us/
Blog: http://blog.replicant.us/
Wiki/tracker/forums: http://redmine.replicant.us/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150706/2f9b296d/attachment.sig>

[U-Boot] [PATCH 1/2] usb: gadget: fastboot: Request status and length check in rx handler

[Lukasz Majewski]

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.

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 1/2] usb: gadget: fastboot: Request status and length check in rx handler

[Paul Kocialkowski]

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.

-- 
Paul Kocialkowski, Replicant developer

Replicant is a fully free Android distribution running on several
devices, a free software mobile operating system putting the emphasis on
freedom and privacy/security.

Website: http://www.replicant.us/
Blog: http://blog.replicant.us/
Wiki/tracker/forums: http://redmine.replicant.us/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150714/35efec3d/attachment.sig>

[U-Boot] [PATCH 1/2] usb: gadget: fastboot: Request status and length check in rx handler

[Lukasz Majewski]

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.

Then I will pick those patches to -dfu tree.

Thanks !

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH 1/2] usb: gadget: fastboot: Request status and length check in rx handler

[Paul Kocialkowski]

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!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150715/8f437a22/attachment.sig>

[U-Boot] [PATCH 1/2] usb: gadget: fastboot: Request status and length check in rx handler

[Lukasz Majewski]

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

Applied to u-boot-dfu.

Thanks for the patch!

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group