Re: [PATCH v2] usb: gadget: fastboot: detach usb just before rebooting

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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-ef7657ce78b1@baylibre.com/

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