| Message ID | 1436021176-15701-2-git-send-email-contact@paulk.fr |
|---|---|
| State | Deferred |
| Delegated to: | Łukasz Majewski |
| Headers | show |
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]
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.
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.
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. >
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.
-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.
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);
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(+)