[PATCH v2] usb: gadget: fastboot: detach usb just before rebooting
The patch fixes the following error when updating a BSH SMM S2 board: 3:72>Start Cmd:FB[-t 8000]: ucmd nand write ${loadaddr} nanddtb ${filesize} 3:72>Okay (0.023s) 3:72>Start Cmd:FB: reboot 3:72>Fail Bulk(R):LIBUSB_ERROR_IO(0s)
The "fastboot reboot" command detaches the USB when it still needs to be used. So let's detach the USB just before the reset.
CC: Mattijs Korpershoek mkorpershoek@baylibre.com Fixes: 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands") Suggested-by: Michael Trimarchi michael@amarulasolutions.com Signed-off-by: Dario Binacchi dario.binacchi@amarulasolutions.com
---
Changes in v2: - Add 'Suggested-by' tag.
drivers/usb/gadget/f_fastboot.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 07b1681c8a9a..0b3fae564d0a 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -419,11 +419,6 @@ static int fastboot_tx_write_str(const char *buffer) return fastboot_tx_write(buffer, strlen(buffer)); }
-static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) -{ - do_reset(NULL, 0, 0, NULL); -} - static unsigned int rx_bytes_expected(struct usb_ep *ep) { int rx_remain = fastboot_data_remaining(); @@ -495,6 +490,12 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req) do_exit_on_complete(ep, req); }
+static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) +{ + do_exit_on_complete(ep, req); + do_reset(NULL, 0, 0, NULL); +} + #if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) static void do_acmd_complete(struct usb_ep *ep, struct usb_request *req) { @@ -544,7 +545,6 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) case FASTBOOT_COMMAND_REBOOT_FASTBOOTD: case FASTBOOT_COMMAND_REBOOT_RECOVERY: fastboot_func->in_req->complete = compl_do_reset; - g_dnl_trigger_detach(); break; #if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) case FASTBOOT_COMMAND_ACMD:
On Wed, Jan 04, 2023 at 17:46, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
The patch fixes the following error when updating a BSH SMM S2 board: 3:72>Start Cmd:FB[-t 8000]: ucmd nand write ${loadaddr} nanddtb ${filesize} 3:72>Okay (0.023s) 3:72>Start Cmd:FB: reboot 3:72>Fail Bulk(R):LIBUSB_ERROR_IO(0s)
The "fastboot reboot" command detaches the USB when it still needs to be used. So let's detach the USB just before the reset.
CC: Mattijs Korpershoek mkorpershoek@baylibre.com Fixes: 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands") Suggested-by: Michael Trimarchi michael@amarulasolutions.com Signed-off-by: Dario Binacchi dario.binacchi@amarulasolutions.com
Hi Dario,
Thank you for your patch and for reporting the problem.
Unfortunately, applying it seems to break the problem I tried to solve with 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands")
Does reverting 5f7e01e9d5d800 also solves your issue?
I'll spend some time on this.
Changes in v2:
- Add 'Suggested-by' tag.
drivers/usb/gadget/f_fastboot.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 07b1681c8a9a..0b3fae564d0a 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -419,11 +419,6 @@ static int fastboot_tx_write_str(const char *buffer) return fastboot_tx_write(buffer, strlen(buffer)); }
-static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) -{
- do_reset(NULL, 0, 0, NULL);
-}
static unsigned int rx_bytes_expected(struct usb_ep *ep) { int rx_remain = fastboot_data_remaining(); @@ -495,6 +490,12 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req) do_exit_on_complete(ep, req); }
+static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) +{
- do_exit_on_complete(ep, req);
- do_reset(NULL, 0, 0, NULL);
+}
#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) static void do_acmd_complete(struct usb_ep *ep, struct usb_request *req) { @@ -544,7 +545,6 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) case FASTBOOT_COMMAND_REBOOT_FASTBOOTD: case FASTBOOT_COMMAND_REBOOT_RECOVERY: fastboot_func->in_req->complete = compl_do_reset;
g_dnl_trigger_detach(); break;#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) case FASTBOOT_COMMAND_ACMD: -- 2.32.0
Hi Mattijs,
On Thu, Jan 5, 2023 at 10:06 AM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
On Wed, Jan 04, 2023 at 17:46, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
The patch fixes the following error when updating a BSH SMM S2 board: 3:72>Start Cmd:FB[-t 8000]: ucmd nand write ${loadaddr} nanddtb ${filesize} 3:72>Okay (0.023s) 3:72>Start Cmd:FB: reboot 3:72>Fail Bulk(R):LIBUSB_ERROR_IO(0s)
The "fastboot reboot" command detaches the USB when it still needs to be used. So let's detach the USB just before the reset.
CC: Mattijs Korpershoek mkorpershoek@baylibre.com Fixes: 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands") Suggested-by: Michael Trimarchi michael@amarulasolutions.com Signed-off-by: Dario Binacchi dario.binacchi@amarulasolutions.com
Hi Dario,
Thank you for your patch and for reporting the problem.
Unfortunately, applying it seems to break the problem I tried to solve with 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands")
Does reverting 5f7e01e9d5d800 also solves your issue?
Yes
I'll spend some time on this.
Ok, thanks. So we can test on our hardware. Regards Dario
Changes in v2:
- Add 'Suggested-by' tag.
drivers/usb/gadget/f_fastboot.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 07b1681c8a9a..0b3fae564d0a 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -419,11 +419,6 @@ static int fastboot_tx_write_str(const char *buffer) return fastboot_tx_write(buffer, strlen(buffer)); }
-static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) -{
do_reset(NULL, 0, 0, NULL);-}
static unsigned int rx_bytes_expected(struct usb_ep *ep) { int rx_remain = fastboot_data_remaining(); @@ -495,6 +490,12 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req) do_exit_on_complete(ep, req); }
+static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) +{
do_exit_on_complete(ep, req);do_reset(NULL, 0, 0, NULL);+}
#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) static void do_acmd_complete(struct usb_ep *ep, struct usb_request *req) { @@ -544,7 +545,6 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) case FASTBOOT_COMMAND_REBOOT_FASTBOOTD: case FASTBOOT_COMMAND_REBOOT_RECOVERY: fastboot_func->in_req->complete = compl_do_reset;
g_dnl_trigger_detach(); break;#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) case FASTBOOT_COMMAND_ACMD: -- 2.32.0
On Thu, Jan 05, 2023 at 10:10, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
Hi Mattijs,
On Thu, Jan 5, 2023 at 10:06 AM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
On Wed, Jan 04, 2023 at 17:46, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
The patch fixes the following error when updating a BSH SMM S2 board: 3:72>Start Cmd:FB[-t 8000]: ucmd nand write ${loadaddr} nanddtb ${filesize} 3:72>Okay (0.023s) 3:72>Start Cmd:FB: reboot 3:72>Fail Bulk(R):LIBUSB_ERROR_IO(0s)
The "fastboot reboot" command detaches the USB when it still needs to be used. So let's detach the USB just before the reset.
CC: Mattijs Korpershoek mkorpershoek@baylibre.com Fixes: 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands") Suggested-by: Michael Trimarchi michael@amarulasolutions.com Signed-off-by: Dario Binacchi dario.binacchi@amarulasolutions.com
Hi Dario,
Thank you for your patch and for reporting the problem.
Unfortunately, applying it seems to break the problem I tried to solve with 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands")
Does reverting 5f7e01e9d5d800 also solves your issue?
Yes
I'll spend some time on this.
Ok, thanks. So we can test on our hardware.
So historically, I wanted to make this a "arch specific fix, as discussed in https://lore.kernel.org/u-boot/20220728-reset-usb-controller-v2-0-ef7657ce78...
To me, the problem with calling g_dnl_trigger_detach() in the fastboot_func->in_req->complete() handler is that it's called too late.
In fact, the main fastboot loop is as following:
while (1) { if (g_dnl_detach()) break; if (ctrlc()) break; schedule(); usb_gadget_handle_interrupts(controller_index); } ret = CMD_RET_SUCCESS;
exit: usb_gadget_release(controller_index); g_dnl_unregister(); g_dnl_clear_detach();
return ret;
usb_gadget_handle_interrupts, will at some point, call complete() which will call compl_do_reset(). But in compl_do_reset(), we already reset the board (without going through the exit: section from cmd/fastboot.c)
For the BSH SMM S2 board, since it's being updated with uuu, I assume that the host PC also triggers a call of acmd_complete() afterwards?
Can you try the following diff as well, on top of your change? (for testing purposes)
--- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -492,7 +492,7 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req)
static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) { - do_exit_on_complete(ep, req); + usb_gadget_release(0); do_reset(NULL, 0, 0, NULL); }
I suspect this will also break your flashing.
Regards Dario
Changes in v2:
- Add 'Suggested-by' tag.
drivers/usb/gadget/f_fastboot.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 07b1681c8a9a..0b3fae564d0a 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -419,11 +419,6 @@ static int fastboot_tx_write_str(const char *buffer) return fastboot_tx_write(buffer, strlen(buffer)); }
-static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) -{
do_reset(NULL, 0, 0, NULL);-}
static unsigned int rx_bytes_expected(struct usb_ep *ep) { int rx_remain = fastboot_data_remaining(); @@ -495,6 +490,12 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req) do_exit_on_complete(ep, req); }
+static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) +{
do_exit_on_complete(ep, req);do_reset(NULL, 0, 0, NULL);+}
#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) static void do_acmd_complete(struct usb_ep *ep, struct usb_request *req) { @@ -544,7 +545,6 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) case FASTBOOT_COMMAND_REBOOT_FASTBOOTD: case FASTBOOT_COMMAND_REBOOT_RECOVERY: fastboot_func->in_req->complete = compl_do_reset;
g_dnl_trigger_detach(); break;#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) case FASTBOOT_COMMAND_ACMD: -- 2.32.0
--
Dario Binacchi
Senior Embedded Linux Developer
dario.binacchi@amarulasolutions.com
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310 info@amarulasolutions.com
www.amarulasolutions.com
Hi Mattijs,
On Thu, Jan 5, 2023 at 12:03 PM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
On Thu, Jan 05, 2023 at 10:10, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
Hi Mattijs,
On Thu, Jan 5, 2023 at 10:06 AM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
On Wed, Jan 04, 2023 at 17:46, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
The patch fixes the following error when updating a BSH SMM S2 board: 3:72>Start Cmd:FB[-t 8000]: ucmd nand write ${loadaddr} nanddtb ${filesize} 3:72>Okay (0.023s) 3:72>Start Cmd:FB: reboot 3:72>Fail Bulk(R):LIBUSB_ERROR_IO(0s)
The "fastboot reboot" command detaches the USB when it still needs to be used. So let's detach the USB just before the reset.
CC: Mattijs Korpershoek mkorpershoek@baylibre.com Fixes: 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands") Suggested-by: Michael Trimarchi michael@amarulasolutions.com Signed-off-by: Dario Binacchi dario.binacchi@amarulasolutions.com
Hi Dario,
Thank you for your patch and for reporting the problem.
Unfortunately, applying it seems to break the problem I tried to solve with 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands")
Does reverting 5f7e01e9d5d800 also solves your issue?
Yes
I'll spend some time on this.
Ok, thanks. So we can test on our hardware.
So historically, I wanted to make this a "arch specific fix, as discussed in https://lore.kernel.org/u-boot/20220728-reset-usb-controller-v2-0-ef7657ce78...
To me, the problem with calling g_dnl_trigger_detach() in the fastboot_func->in_req->complete() handler is that it's called too late.
In fact, the main fastboot loop is as following:
while (1) { if (g_dnl_detach()) break; if (ctrlc()) break; schedule(); usb_gadget_handle_interrupts(controller_index); } ret = CMD_RET_SUCCESS;exit: usb_gadget_release(controller_index); g_dnl_unregister(); g_dnl_clear_detach();
return ret;usb_gadget_handle_interrupts, will at some point, call complete() which will call compl_do_reset(). But in compl_do_reset(), we already reset the board (without going through the exit: section from cmd/fastboot.c)
For the BSH SMM S2 board, since it's being updated with uuu, I assume that the host PC also triggers a call of acmd_complete() afterwards?
Can you try the following diff as well, on top of your change? (for testing purposes)
--- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -492,7 +492,7 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req)
static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) {
do_exit_on_complete(ep, req);
usb_gadget_release(0); do_reset(NULL, 0, 0, NULL);}
I suspect this will also break your flashing.
No, I am able to flash and reset the board. This change works.
Regards Dario
Changes in v2:
- Add 'Suggested-by' tag.
drivers/usb/gadget/f_fastboot.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 07b1681c8a9a..0b3fae564d0a 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -419,11 +419,6 @@ static int fastboot_tx_write_str(const char *buffer) return fastboot_tx_write(buffer, strlen(buffer)); }
-static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) -{
do_reset(NULL, 0, 0, NULL);-}
static unsigned int rx_bytes_expected(struct usb_ep *ep) { int rx_remain = fastboot_data_remaining(); @@ -495,6 +490,12 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req) do_exit_on_complete(ep, req); }
+static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) +{
do_exit_on_complete(ep, req);do_reset(NULL, 0, 0, NULL);+}
#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) static void do_acmd_complete(struct usb_ep *ep, struct usb_request *req) { @@ -544,7 +545,6 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) case FASTBOOT_COMMAND_REBOOT_FASTBOOTD: case FASTBOOT_COMMAND_REBOOT_RECOVERY: fastboot_func->in_req->complete = compl_do_reset;
g_dnl_trigger_detach(); break;#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) case FASTBOOT_COMMAND_ACMD: -- 2.32.0
--
Dario Binacchi
Senior Embedded Linux Developer
dario.binacchi@amarulasolutions.com
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310 info@amarulasolutions.com
www.amarulasolutions.com
On Thu, Jan 05, 2023 at 14:25, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
Hi Mattijs,
On Thu, Jan 5, 2023 at 12:03 PM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
On Thu, Jan 05, 2023 at 10:10, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
Hi Mattijs,
On Thu, Jan 5, 2023 at 10:06 AM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
On Wed, Jan 04, 2023 at 17:46, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
The patch fixes the following error when updating a BSH SMM S2 board: 3:72>Start Cmd:FB[-t 8000]: ucmd nand write ${loadaddr} nanddtb ${filesize} 3:72>Okay (0.023s) 3:72>Start Cmd:FB: reboot 3:72>Fail Bulk(R):LIBUSB_ERROR_IO(0s)
The "fastboot reboot" command detaches the USB when it still needs to be used. So let's detach the USB just before the reset.
CC: Mattijs Korpershoek mkorpershoek@baylibre.com Fixes: 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands") Suggested-by: Michael Trimarchi michael@amarulasolutions.com Signed-off-by: Dario Binacchi dario.binacchi@amarulasolutions.com
Hi Dario,
Thank you for your patch and for reporting the problem.
Unfortunately, applying it seems to break the problem I tried to solve with 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands")
Does reverting 5f7e01e9d5d800 also solves your issue?
Yes
I'll spend some time on this.
Ok, thanks. So we can test on our hardware.
So historically, I wanted to make this a "arch specific fix, as discussed in https://lore.kernel.org/u-boot/20220728-reset-usb-controller-v2-0-ef7657ce78...
To me, the problem with calling g_dnl_trigger_detach() in the fastboot_func->in_req->complete() handler is that it's called too late.
In fact, the main fastboot loop is as following:
while (1) { if (g_dnl_detach()) break; if (ctrlc()) break; schedule(); usb_gadget_handle_interrupts(controller_index); } ret = CMD_RET_SUCCESS;exit: usb_gadget_release(controller_index); g_dnl_unregister(); g_dnl_clear_detach();
return ret;usb_gadget_handle_interrupts, will at some point, call complete() which will call compl_do_reset(). But in compl_do_reset(), we already reset the board (without going through the exit: section from cmd/fastboot.c)
For the BSH SMM S2 board, since it's being updated with uuu, I assume that the host PC also triggers a call of acmd_complete() afterwards?
Can you try the following diff as well, on top of your change? (for testing purposes)
--- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -492,7 +492,7 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req)
static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) {
do_exit_on_complete(ep, req);
usb_gadget_release(0); do_reset(NULL, 0, 0, NULL);}
I suspect this will also break your flashing.
No, I am able to flash and reset the board. This change works.
Huh. Ok. I will do some more debugging on my end. Thank you for your patience.
Regards Dario
Changes in v2:
- Add 'Suggested-by' tag.
drivers/usb/gadget/f_fastboot.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 07b1681c8a9a..0b3fae564d0a 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -419,11 +419,6 @@ static int fastboot_tx_write_str(const char *buffer) return fastboot_tx_write(buffer, strlen(buffer)); }
-static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) -{
do_reset(NULL, 0, 0, NULL);-}
static unsigned int rx_bytes_expected(struct usb_ep *ep) { int rx_remain = fastboot_data_remaining(); @@ -495,6 +490,12 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req) do_exit_on_complete(ep, req); }
+static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) +{
do_exit_on_complete(ep, req);do_reset(NULL, 0, 0, NULL);+}
#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) static void do_acmd_complete(struct usb_ep *ep, struct usb_request *req) { @@ -544,7 +545,6 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) case FASTBOOT_COMMAND_REBOOT_FASTBOOTD: case FASTBOOT_COMMAND_REBOOT_RECOVERY: fastboot_func->in_req->complete = compl_do_reset;
g_dnl_trigger_detach(); break;#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) case FASTBOOT_COMMAND_ACMD: -- 2.32.0
--
Dario Binacchi
Senior Embedded Linux Developer
dario.binacchi@amarulasolutions.com
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310 info@amarulasolutions.com
www.amarulasolutions.com
--
Dario Binacchi
Senior Embedded Linux Developer
dario.binacchi@amarulasolutions.com
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310 info@amarulasolutions.com
www.amarulasolutions.com
On Thu, Jan 05, 2023 at 14:25, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
Hi Mattijs,
On Thu, Jan 5, 2023 at 12:03 PM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
On Thu, Jan 05, 2023 at 10:10, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
Hi Mattijs,
On Thu, Jan 5, 2023 at 10:06 AM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
On Wed, Jan 04, 2023 at 17:46, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
The patch fixes the following error when updating a BSH SMM S2 board: 3:72>Start Cmd:FB[-t 8000]: ucmd nand write ${loadaddr} nanddtb ${filesize} 3:72>Okay (0.023s) 3:72>Start Cmd:FB: reboot 3:72>Fail Bulk(R):LIBUSB_ERROR_IO(0s)
The "fastboot reboot" command detaches the USB when it still needs to be used. So let's detach the USB just before the reset.
CC: Mattijs Korpershoek mkorpershoek@baylibre.com Fixes: 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands") Suggested-by: Michael Trimarchi michael@amarulasolutions.com Signed-off-by: Dario Binacchi dario.binacchi@amarulasolutions.com
Hi Dario,
Thank you for your patch and for reporting the problem.
Unfortunately, applying it seems to break the problem I tried to solve with 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands")
Does reverting 5f7e01e9d5d800 also solves your issue?
Yes
I'll spend some time on this.
Ok, thanks. So we can test on our hardware.
So historically, I wanted to make this a "arch specific fix, as discussed in https://lore.kernel.org/u-boot/20220728-reset-usb-controller-v2-0-ef7657ce78...
To me, the problem with calling g_dnl_trigger_detach() in the fastboot_func->in_req->complete() handler is that it's called too late.
In fact, the main fastboot loop is as following:
while (1) { if (g_dnl_detach()) break; if (ctrlc()) break; schedule(); usb_gadget_handle_interrupts(controller_index); } ret = CMD_RET_SUCCESS;exit: usb_gadget_release(controller_index); g_dnl_unregister(); g_dnl_clear_detach();
return ret;usb_gadget_handle_interrupts, will at some point, call complete() which will call compl_do_reset(). But in compl_do_reset(), we already reset the board (without going through the exit: section from cmd/fastboot.c)
For the BSH SMM S2 board, since it's being updated with uuu, I assume that the host PC also triggers a call of acmd_complete() afterwards?
Can you try the following diff as well, on top of your change? (for testing purposes)
--- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -492,7 +492,7 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req)
static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) {
do_exit_on_complete(ep, req);
usb_gadget_release(0); do_reset(NULL, 0, 0, NULL);}
I suspect this will also break your flashing.
No, I am able to flash and reset the board. This change works.
With some additional testing on my end, I can see that the final message (sending OKAY to the host) done on fastboot_func->in_ep is not done on my end. I think I am lucky that my controller driver (dwc2_udc_otg) delivers the message to my host anyways and calls done() even when messages are aborted.
I think I have something that might be better.
Dario, could you do another test for me, please? 1. Revert 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands")
2. Apply the following diff:
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index d0e92c7a071f..f4311328a2da 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -421,6 +421,7 @@ static int fastboot_tx_write_str(const char *buffer)
static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) { + g_dnl_unregister(); do_reset(NULL, 0, 0, NULL); }
The call to g_dnl_unregister() will end up calling g_dnl_unbind() which calls usb_gadget_disconnect() where each controller can implement a soft disconnect (which I can do for dwc2_udc_otg)
Regards Dario
Changes in v2:
- Add 'Suggested-by' tag.
drivers/usb/gadget/f_fastboot.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 07b1681c8a9a..0b3fae564d0a 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -419,11 +419,6 @@ static int fastboot_tx_write_str(const char *buffer) return fastboot_tx_write(buffer, strlen(buffer)); }
-static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) -{
do_reset(NULL, 0, 0, NULL);-}
static unsigned int rx_bytes_expected(struct usb_ep *ep) { int rx_remain = fastboot_data_remaining(); @@ -495,6 +490,12 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req) do_exit_on_complete(ep, req); }
+static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) +{
do_exit_on_complete(ep, req);do_reset(NULL, 0, 0, NULL);+}
#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) static void do_acmd_complete(struct usb_ep *ep, struct usb_request *req) { @@ -544,7 +545,6 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) case FASTBOOT_COMMAND_REBOOT_FASTBOOTD: case FASTBOOT_COMMAND_REBOOT_RECOVERY: fastboot_func->in_req->complete = compl_do_reset;
g_dnl_trigger_detach(); break;#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) case FASTBOOT_COMMAND_ACMD: -- 2.32.0
--
Dario Binacchi
Senior Embedded Linux Developer
dario.binacchi@amarulasolutions.com
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310 info@amarulasolutions.com
www.amarulasolutions.com
--
Dario Binacchi
Senior Embedded Linux Developer
dario.binacchi@amarulasolutions.com
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310 info@amarulasolutions.com
www.amarulasolutions.com
Hi Mattijs,
On Fri, Jan 6, 2023 at 5:42 PM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
On Thu, Jan 05, 2023 at 14:25, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
Hi Mattijs,
On Thu, Jan 5, 2023 at 12:03 PM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
On Thu, Jan 05, 2023 at 10:10, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
Hi Mattijs,
On Thu, Jan 5, 2023 at 10:06 AM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
On Wed, Jan 04, 2023 at 17:46, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
The patch fixes the following error when updating a BSH SMM S2 board: 3:72>Start Cmd:FB[-t 8000]: ucmd nand write ${loadaddr} nanddtb ${filesize} 3:72>Okay (0.023s) 3:72>Start Cmd:FB: reboot 3:72>Fail Bulk(R):LIBUSB_ERROR_IO(0s)
The "fastboot reboot" command detaches the USB when it still needs to be used. So let's detach the USB just before the reset.
CC: Mattijs Korpershoek mkorpershoek@baylibre.com Fixes: 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands") Suggested-by: Michael Trimarchi michael@amarulasolutions.com Signed-off-by: Dario Binacchi dario.binacchi@amarulasolutions.com
Hi Dario,
Thank you for your patch and for reporting the problem.
Unfortunately, applying it seems to break the problem I tried to solve with 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands")
Does reverting 5f7e01e9d5d800 also solves your issue?
Yes
I'll spend some time on this.
Ok, thanks. So we can test on our hardware.
So historically, I wanted to make this a "arch specific fix, as discussed in https://lore.kernel.org/u-boot/20220728-reset-usb-controller-v2-0-ef7657ce78...
To me, the problem with calling g_dnl_trigger_detach() in the fastboot_func->in_req->complete() handler is that it's called too late.
In fact, the main fastboot loop is as following:
while (1) { if (g_dnl_detach()) break; if (ctrlc()) break; schedule(); usb_gadget_handle_interrupts(controller_index); } ret = CMD_RET_SUCCESS;exit: usb_gadget_release(controller_index); g_dnl_unregister(); g_dnl_clear_detach();
return ret;usb_gadget_handle_interrupts, will at some point, call complete() which will call compl_do_reset(). But in compl_do_reset(), we already reset the board (without going through the exit: section from cmd/fastboot.c)
For the BSH SMM S2 board, since it's being updated with uuu, I assume that the host PC also triggers a call of acmd_complete() afterwards?
Can you try the following diff as well, on top of your change? (for testing purposes)
--- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -492,7 +492,7 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req)
static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) {
do_exit_on_complete(ep, req);
usb_gadget_release(0); do_reset(NULL, 0, 0, NULL);}
I suspect this will also break your flashing.
No, I am able to flash and reset the board. This change works.
With some additional testing on my end, I can see that the final message (sending OKAY to the host) done on fastboot_func->in_ep is not done on my end. I think I am lucky that my controller driver (dwc2_udc_otg) delivers the message to my host anyways and calls done() even when messages are aborted.
I think I have something that might be better.
Dario, could you do another test for me, please?
Revert 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands")
Apply the following diff:
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index d0e92c7a071f..f4311328a2da 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -421,6 +421,7 @@ static int fastboot_tx_write_str(const char *buffer)
static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) {
g_dnl_unregister(); do_reset(NULL, 0, 0, NULL);}
I applied your diff and it runs. I changed my patch with your suggestion. I'll submit v3. The patch can be applied without reverting 5f7e01e9d5d800. Can you re-test and apply your tested-by tag? I added the Co-developed-by tag too. Do you think it is correct to proceed like this?
Thanks and regards, Dario
The call to g_dnl_unregister() will end up calling g_dnl_unbind() which calls usb_gadget_disconnect() where each controller can implement a soft disconnect (which I can do for dwc2_udc_otg)
Regards Dario
Changes in v2:
- Add 'Suggested-by' tag.
drivers/usb/gadget/f_fastboot.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index 07b1681c8a9a..0b3fae564d0a 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -419,11 +419,6 @@ static int fastboot_tx_write_str(const char *buffer) return fastboot_tx_write(buffer, strlen(buffer)); }
-static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) -{
do_reset(NULL, 0, 0, NULL);-}
static unsigned int rx_bytes_expected(struct usb_ep *ep) { int rx_remain = fastboot_data_remaining(); @@ -495,6 +490,12 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req) do_exit_on_complete(ep, req); }
+static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) +{
do_exit_on_complete(ep, req);do_reset(NULL, 0, 0, NULL);+}
#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) static void do_acmd_complete(struct usb_ep *ep, struct usb_request *req) { @@ -544,7 +545,6 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) case FASTBOOT_COMMAND_REBOOT_FASTBOOTD: case FASTBOOT_COMMAND_REBOOT_RECOVERY: fastboot_func->in_req->complete = compl_do_reset;
g_dnl_trigger_detach(); break;#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) case FASTBOOT_COMMAND_ACMD: -- 2.32.0
--
Dario Binacchi
Senior Embedded Linux Developer
dario.binacchi@amarulasolutions.com
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310 info@amarulasolutions.com
www.amarulasolutions.com
--
Dario Binacchi
Senior Embedded Linux Developer
dario.binacchi@amarulasolutions.com
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310 info@amarulasolutions.com
www.amarulasolutions.com
Hi Dario,
On Sat, Jan 07, 2023 at 17:45, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
Hi Mattijs,
On Fri, Jan 6, 2023 at 5:42 PM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
On Thu, Jan 05, 2023 at 14:25, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
Hi Mattijs,
On Thu, Jan 5, 2023 at 12:03 PM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
On Thu, Jan 05, 2023 at 10:10, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
Hi Mattijs,
On Thu, Jan 5, 2023 at 10:06 AM Mattijs Korpershoek mkorpershoek@baylibre.com wrote:
On Wed, Jan 04, 2023 at 17:46, Dario Binacchi dario.binacchi@amarulasolutions.com wrote:
> The patch fixes the following error when updating a BSH SMM S2 board: > 3:72>Start Cmd:FB[-t 8000]: ucmd nand write ${loadaddr} nanddtb ${filesize} > 3:72>Okay (0.023s) > 3:72>Start Cmd:FB: reboot > 3:72>Fail Bulk(R):LIBUSB_ERROR_IO(0s) > > The "fastboot reboot" command detaches the USB when it still needs to be > used. So let's detach the USB just before the reset. > > CC: Mattijs Korpershoek mkorpershoek@baylibre.com > Fixes: 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands") > Suggested-by: Michael Trimarchi michael@amarulasolutions.com > Signed-off-by: Dario Binacchi dario.binacchi@amarulasolutions.com
Hi Dario,
Thank you for your patch and for reporting the problem.
Unfortunately, applying it seems to break the problem I tried to solve with 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands")
Does reverting 5f7e01e9d5d800 also solves your issue?
Yes
I'll spend some time on this.
Ok, thanks. So we can test on our hardware.
So historically, I wanted to make this a "arch specific fix, as discussed in https://lore.kernel.org/u-boot/20220728-reset-usb-controller-v2-0-ef7657ce78...
To me, the problem with calling g_dnl_trigger_detach() in the fastboot_func->in_req->complete() handler is that it's called too late.
In fact, the main fastboot loop is as following:
while (1) { if (g_dnl_detach()) break; if (ctrlc()) break; schedule(); usb_gadget_handle_interrupts(controller_index); } ret = CMD_RET_SUCCESS;exit: usb_gadget_release(controller_index); g_dnl_unregister(); g_dnl_clear_detach();
return ret;usb_gadget_handle_interrupts, will at some point, call complete() which will call compl_do_reset(). But in compl_do_reset(), we already reset the board (without going through the exit: section from cmd/fastboot.c)
For the BSH SMM S2 board, since it's being updated with uuu, I assume that the host PC also triggers a call of acmd_complete() afterwards?
Can you try the following diff as well, on top of your change? (for testing purposes)
--- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -492,7 +492,7 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req)
static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) {
do_exit_on_complete(ep, req);
usb_gadget_release(0); do_reset(NULL, 0, 0, NULL);}
I suspect this will also break your flashing.
No, I am able to flash and reset the board. This change works.
With some additional testing on my end, I can see that the final message (sending OKAY to the host) done on fastboot_func->in_ep is not done on my end. I think I am lucky that my controller driver (dwc2_udc_otg) delivers the message to my host anyways and calls done() even when messages are aborted.
I think I have something that might be better.
Dario, could you do another test for me, please?
Revert 5f7e01e9d5d800 ("usb: gadget: fastboot: detach usb on reboot commands")
Apply the following diff:
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c index d0e92c7a071f..f4311328a2da 100644 --- a/drivers/usb/gadget/f_fastboot.c +++ b/drivers/usb/gadget/f_fastboot.c @@ -421,6 +421,7 @@ static int fastboot_tx_write_str(const char *buffer)
static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) {
g_dnl_unregister(); do_reset(NULL, 0, 0, NULL);}
I applied your diff and it runs. I changed my patch with your suggestion. I'll submit v3. The patch can be applied without reverting 5f7e01e9d5d800. Can you re-test and apply your tested-by tag? I added the Co-developed-by tag too. Do you think it is correct to proceed like this?
Yes it's ok to proceed like this.
Thanks and regards, Dario
The call to g_dnl_unregister() will end up calling g_dnl_unbind() which calls usb_gadget_disconnect() where each controller can implement a soft disconnect (which I can do for dwc2_udc_otg)
Regards Dario
> > --- > > Changes in v2: > - Add 'Suggested-by' tag. > > drivers/usb/gadget/f_fastboot.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c > index 07b1681c8a9a..0b3fae564d0a 100644 > --- a/drivers/usb/gadget/f_fastboot.c > +++ b/drivers/usb/gadget/f_fastboot.c > @@ -419,11 +419,6 @@ static int fastboot_tx_write_str(const char *buffer) > return fastboot_tx_write(buffer, strlen(buffer)); > } > > -static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) > -{ > - do_reset(NULL, 0, 0, NULL); > -} > - > static unsigned int rx_bytes_expected(struct usb_ep *ep) > { > int rx_remain = fastboot_data_remaining(); > @@ -495,6 +490,12 @@ static void do_bootm_on_complete(struct usb_ep *ep, struct usb_request *req) > do_exit_on_complete(ep, req); > } > > +static void compl_do_reset(struct usb_ep *ep, struct usb_request *req) > +{ > + do_exit_on_complete(ep, req); > + do_reset(NULL, 0, 0, NULL); > +} > + > #if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) > static void do_acmd_complete(struct usb_ep *ep, struct usb_request *req) > { > @@ -544,7 +545,6 @@ static void rx_handler_command(struct usb_ep *ep, struct usb_request *req) > case FASTBOOT_COMMAND_REBOOT_FASTBOOTD: > case FASTBOOT_COMMAND_REBOOT_RECOVERY: > fastboot_func->in_req->complete = compl_do_reset; > - g_dnl_trigger_detach(); > break; > #if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT) > case FASTBOOT_COMMAND_ACMD: > -- > 2.32.0
--
Dario Binacchi
Senior Embedded Linux Developer
dario.binacchi@amarulasolutions.com
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310 info@amarulasolutions.com
www.amarulasolutions.com
--
Dario Binacchi
Senior Embedded Linux Developer
dario.binacchi@amarulasolutions.com
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310 info@amarulasolutions.com
www.amarulasolutions.com
--
Dario Binacchi
Senior Embedded Linux Developer
dario.binacchi@amarulasolutions.com
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310 info@amarulasolutions.com
www.amarulasolutions.com
participants (2)
-
Dario Binacchi -
Mattijs Korpershoek