[PATCH] fastboot: release usb_gadget on reboot commands

[PATCH] fastboot: release usb_gadget on reboot commands

[Mattijs Korpershoek]

When host issues "fastboot reboot fastboot", it's expected that the
board drops the USB connection before resetting.

On some boards, such as Khadas VIM3L and SEI610, this is not the case.

We observe the following error:
$ fastboot reboot fastboot
Rebooting into fastboot                            OKAY [  0.004s]
fastboot: error: Failed to boot into userspace fastboot; one or more components might be unbootable.

This does not happen when we use the RST button on the board, nor
in linux. We might hit a undefined hardware behavior, where D+ and D-
are in an unknown state.

Change the logic in cmd/fastboot.c to do a "reset request" instead of
actually calling reset. This allows us to get out of the infinite loop
in fastboot.c, de-initialize the usb stack and do a clean reset.

Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
---
 cmd/fastboot.c                  |  3 +++
 drivers/usb/gadget/f_fastboot.c |  3 ++-
 drivers/usb/gadget/g_dnl.c      | 17 +++++++++++++++++
 include/g_dnl.h                 |  5 +++++
 4 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/cmd/fastboot.c b/cmd/fastboot.c
index 033a2c95e8f0..97ce311e6d9b 100644
--- a/cmd/fastboot.c
+++ b/cmd/fastboot.c
@@ -87,6 +87,9 @@ exit:
 	g_dnl_clear_detach();
 	usb_gadget_release(controller_index);
 
+	if (g_dnl_should_reset())
+		do_reset(NULL, 0, 0, NULL);
+
 	return ret;
 #else
 	pr_err("Fastboot USB not enabled\n");
diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
index 8ba55aab9f8f..a00d1ca571d1 100644
--- a/drivers/usb/gadget/f_fastboot.c
+++ b/drivers/usb/gadget/f_fastboot.c
@@ -420,7 +420,8 @@ static int fastboot_tx_write_str(const char *buffer)
 
 static void compl_do_reset(struct usb_ep *ep, struct usb_request *req)
 {
-	do_reset(NULL, 0, 0, NULL);
+	g_dnl_trigger_detach();
+	g_dnl_trigger_reset_request();
 }
 
 static unsigned int rx_bytes_expected(struct usb_ep *ep)
diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
index afb7b74f3057..d083a214290d 100644
--- a/drivers/usb/gadget/g_dnl.c
+++ b/drivers/usb/gadget/g_dnl.c
@@ -200,6 +200,23 @@ void g_dnl_clear_detach(void)
 	g_dnl_detach_request = false;
 }
 
+static bool g_dnl_reset_request;
+
+bool g_dnl_should_reset(void)
+{
+	return g_dnl_reset_request;
+}
+
+void g_dnl_trigger_reset_request(void)
+{
+	g_dnl_reset_request = true;
+}
+
+void g_dnl_clear_reset_request(void)
+{
+	g_dnl_reset_request = false;
+}
+
 static int g_dnl_get_bcd_device_number(struct usb_composite_dev *cdev)
 {
 	struct usb_gadget *gadget = cdev->gadget;
diff --git a/include/g_dnl.h b/include/g_dnl.h
index 836ee602c8da..95c772385fa6 100644
--- a/include/g_dnl.h
+++ b/include/g_dnl.h
@@ -43,6 +43,11 @@ void g_dnl_set_product(const char *s);
 bool g_dnl_detach(void);
 void g_dnl_trigger_detach(void);
 void g_dnl_clear_detach(void);
+
+bool g_dnl_should_reset(void);
+void g_dnl_trigger_reset_request(void);
+void g_dnl_clear_reset_request(void);
+
 int run_usb_dnl_gadget(int usbctrl_index, char *usb_dnl_gadget);
 
 #endif /* __G_DOWNLOAD_H_ */
-- 
2.36.1

Re: [PATCH] fastboot: release usb_gadget on reboot commands

[Marek Vasut]

On 7/21/22 15:59, Mattijs Korpershoek wrote:

[...]

> diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
> index 8ba55aab9f8f..a00d1ca571d1 100644
> --- a/drivers/usb/gadget/f_fastboot.c
> +++ b/drivers/usb/gadget/f_fastboot.c
> @@ -420,7 +420,8 @@ static int fastboot_tx_write_str(const char *buffer)
>   
>   static void compl_do_reset(struct usb_ep *ep, struct usb_request *req)
>   {
> -	do_reset(NULL, 0, 0, NULL);
> +	g_dnl_trigger_detach();
> +	g_dnl_trigger_reset_request();

Wouldn't it be enough to call usb_gadget_release() before do_reset() 
here ? Or actually fix up the hardware state in your platform reset 
implementation, which would cover all such odd states for every other 
USB UDC mode of operation, not just fastboot ?

Re: [PATCH] fastboot: release usb_gadget on reboot commands

[Mattijs Korpershoek]

On ven., juil. 22, 2022 at 23:48, Marek Vasut <marex@denx.de> wrote:

> On 7/21/22 15:59, Mattijs Korpershoek wrote:
>
> [...]
>
>> diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
>> index 8ba55aab9f8f..a00d1ca571d1 100644
>> --- a/drivers/usb/gadget/f_fastboot.c
>> +++ b/drivers/usb/gadget/f_fastboot.c
>> @@ -420,7 +420,8 @@ static int fastboot_tx_write_str(const char *buffer)
>>   
>>   static void compl_do_reset(struct usb_ep *ep, struct usb_request *req)
>>   {
>> -	do_reset(NULL, 0, 0, NULL);
>> +	g_dnl_trigger_detach();
>> +	g_dnl_trigger_reset_request();

Hi Marek,

Thank you for your review and your suggestions.

>
> Wouldn't it be enough to call usb_gadget_release() before do_reset() 
> here ? Or actually fix up the hardware state in your platform reset 

Calling usb_gadget_release() just before do_reset() is sufficient as well.
However usb_gadget_release(int index) takes a controller index as
argument.
This controller index is retrieved from the "=> fastboot" cmd and I
could not come up with something elegant to retrieve it from
f_fastboot.c.
I can have another look if you think that's a better solution.

> implementation, which would cover all such odd states for every other 
> USB UDC mode of operation, not just fastboot ?

Implementing a platform_reset to reset the usb controller also works.
I discussed this with Neil at [1] and he suggested to send the f_fastboot.c fix
(this one) because it's generic and might fix issues for other boards.

So, should I abandon this version and go with the platform specific fix instead ?

[1] https://gitlab.com/baylibre/amlogic/atv/u-boot/-/commit/94c79b8226875babc20311cac7dac30e79238c9d#note_1020214136

Re: [PATCH] fastboot: release usb_gadget on reboot commands

[Marek Vasut]

On 7/25/22 15:19, Mattijs Korpershoek wrote:
> On ven., juil. 22, 2022 at 23:48, Marek Vasut <marex@denx.de> wrote:
> 
>> On 7/21/22 15:59, Mattijs Korpershoek wrote:
>>
>> [...]
>>
>>> diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
>>> index 8ba55aab9f8f..a00d1ca571d1 100644
>>> --- a/drivers/usb/gadget/f_fastboot.c
>>> +++ b/drivers/usb/gadget/f_fastboot.c
>>> @@ -420,7 +420,8 @@ static int fastboot_tx_write_str(const char *buffer)
>>>    
>>>    static void compl_do_reset(struct usb_ep *ep, struct usb_request *req)
>>>    {
>>> -	do_reset(NULL, 0, 0, NULL);
>>> +	g_dnl_trigger_detach();
>>> +	g_dnl_trigger_reset_request();
> 
> Hi Marek,
> 
> Thank you for your review and your suggestions.
> 
>>
>> Wouldn't it be enough to call usb_gadget_release() before do_reset()
>> here ? Or actually fix up the hardware state in your platform reset
> 
> Calling usb_gadget_release() just before do_reset() is sufficient as well.
> However usb_gadget_release(int index) takes a controller index as
> argument.
> This controller index is retrieved from the "=> fastboot" cmd and I
> could not come up with something elegant to retrieve it from
> f_fastboot.c.
> I can have another look if you think that's a better solution.

I think it would be nice to have this kind of generic fix in addition to 
one specific to your platform , as discussed below.

>> implementation, which would cover all such odd states for every other
>> USB UDC mode of operation, not just fastboot ?
> 
> Implementing a platform_reset to reset the usb controller also works.
> I discussed this with Neil at [1] and he suggested to send the f_fastboot.c fix
> (this one) because it's generic and might fix issues for other boards.
> 
> So, should I abandon this version and go with the platform specific fix instead ?
> 
> [1] https://gitlab.com/baylibre/amlogic/atv/u-boot/-/commit/94c79b8226875babc20311cac7dac30e79238c9d#note_1020214136

If your controller is responsible for causing the platform to fail to 
reboot, then I think it makes sense to do a fix either in the reset 
implementation for that platform, or the controller driver.

Re: [PATCH] fastboot: release usb_gadget on reboot commands

[Mattijs Korpershoek]

On Mon, Jul 25, 2022 at 17:30, Marek Vasut <marex@denx.de> wrote:

> On 7/25/22 15:19, Mattijs Korpershoek wrote:
>> On ven., juil. 22, 2022 at 23:48, Marek Vasut <marex@denx.de> wrote:
>> 
>>> On 7/21/22 15:59, Mattijs Korpershoek wrote:
>>>
>>> [...]
>>>
>>>> diff --git a/drivers/usb/gadget/f_fastboot.c b/drivers/usb/gadget/f_fastboot.c
>>>> index 8ba55aab9f8f..a00d1ca571d1 100644
>>>> --- a/drivers/usb/gadget/f_fastboot.c
>>>> +++ b/drivers/usb/gadget/f_fastboot.c
>>>> @@ -420,7 +420,8 @@ static int fastboot_tx_write_str(const char *buffer)
>>>>    
>>>>    static void compl_do_reset(struct usb_ep *ep, struct usb_request *req)
>>>>    {
>>>> -	do_reset(NULL, 0, 0, NULL);
>>>> +	g_dnl_trigger_detach();
>>>> +	g_dnl_trigger_reset_request();
>> 
>> Hi Marek,
>> 
>> Thank you for your review and your suggestions.
>> 
>>>
>>> Wouldn't it be enough to call usb_gadget_release() before do_reset()
>>> here ? Or actually fix up the hardware state in your platform reset
>> 
>> Calling usb_gadget_release() just before do_reset() is sufficient as well.
>> However usb_gadget_release(int index) takes a controller index as
>> argument.
>> This controller index is retrieved from the "=> fastboot" cmd and I
>> could not come up with something elegant to retrieve it from
>> f_fastboot.c.
>> I can have another look if you think that's a better solution.
>
> I think it would be nice to have this kind of generic fix in addition to 
> one specific to your platform , as discussed below.

ACK

>
>>> implementation, which would cover all such odd states for every other
>>> USB UDC mode of operation, not just fastboot ?
>> 
>> Implementing a platform_reset to reset the usb controller also works.
>> I discussed this with Neil at [1] and he suggested to send the f_fastboot.c fix
>> (this one) because it's generic and might fix issues for other boards.
>> 
>> So, should I abandon this version and go with the platform specific fix instead ?
>> 
>> [1] https://gitlab.com/baylibre/amlogic/atv/u-boot/-/commit/94c79b8226875babc20311cac7dac30e79238c9d#note_1020214136
>
> If your controller is responsible for causing the platform to fail to 
> reboot, then I think it makes sense to do a fix either in the reset 
> implementation for that platform, or the controller driver.

To be clear, the usb controller is *not* causing the whole platform to
reboot.
On do_reset(), the platform resets fine. However, the usb controller is
*not* put in reset. Because of that, the host does not detect a USB
disconnection. Which is what I'm trying to solve here.

If we do the generic fix, there is not really a point into implementing
a platform specific reset.
Note that on linux, when we run "reboot" the driver's remove() function
takes care of usb reset.

Re: [PATCH] fastboot: release usb_gadget on reboot commands

[Marek Vasut]

On 7/26/22 10:25, Mattijs Korpershoek wrote:
[...]
>>>> implementation, which would cover all such odd states for every other
>>>> USB UDC mode of operation, not just fastboot ?
>>>
>>> Implementing a platform_reset to reset the usb controller also works.
>>> I discussed this with Neil at [1] and he suggested to send the f_fastboot.c fix
>>> (this one) because it's generic and might fix issues for other boards.
>>>
>>> So, should I abandon this version and go with the platform specific fix instead ?
>>>
>>> [1] https://gitlab.com/baylibre/amlogic/atv/u-boot/-/commit/94c79b8226875babc20311cac7dac30e79238c9d#note_1020214136
>>
>> If your controller is responsible for causing the platform to fail to
>> reboot, then I think it makes sense to do a fix either in the reset
>> implementation for that platform, or the controller driver.
> 
> To be clear, the usb controller is *not* causing the whole platform to
> reboot.
> On do_reset(), the platform resets fine. However, the usb controller is
> *not* put in reset. Because of that, the host does not detect a USB
> disconnection. Which is what I'm trying to solve here.

Yes, this is how I understand the problem too.

Assume someone would run e.g. ums code, which would bring up the 
controller too, I suspect it would also cause the same reset issues. 
Which is why I wonder whether we shouldn't add this kind of fix into the 
reset handler for the platform itself.

> If we do the generic fix, there is not really a point into implementing
> a platform specific reset.
> Note that on linux, when we run "reboot" the driver's remove() function
> takes care of usb reset.

Does that also work with sysrq-B or reboot -nf (one of the fast reboots) ?

Re: [PATCH] fastboot: release usb_gadget on reboot commands

[Mattijs Korpershoek]

Hi Marek,

On Tue, Jul 26, 2022 at 15:36, Marek Vasut <marex@denx.de> wrote:

> On 7/26/22 10:25, Mattijs Korpershoek wrote:
> [...]
>>>>> implementation, which would cover all such odd states for every other
>>>>> USB UDC mode of operation, not just fastboot ?
>>>>
>>>> Implementing a platform_reset to reset the usb controller also works.
>>>> I discussed this with Neil at [1] and he suggested to send the f_fastboot.c fix
>>>> (this one) because it's generic and might fix issues for other boards.
>>>>
>>>> So, should I abandon this version and go with the platform specific fix instead ?
>>>>
>>>> [1] https://gitlab.com/baylibre/amlogic/atv/u-boot/-/commit/94c79b8226875babc20311cac7dac30e79238c9d#note_1020214136
>>>
>>> If your controller is responsible for causing the platform to fail to
>>> reboot, then I think it makes sense to do a fix either in the reset
>>> implementation for that platform, or the controller driver.
>> 
>> To be clear, the usb controller is *not* causing the whole platform to
>> reboot.
>> On do_reset(), the platform resets fine. However, the usb controller is
>> *not* put in reset. Because of that, the host does not detect a USB
>> disconnection. Which is what I'm trying to solve here.
>
> Yes, this is how I understand the problem too.
>
> Assume someone would run e.g. ums code, which would bring up the 
> controller too, I suspect it would also cause the same reset issues. 
> Which is why I wonder whether we shouldn't add this kind of fix into the 
> reset handler for the platform itself.

Oh. Got it. I was solely focusing on my use case (fastboot) and not
considering ums or other things.
Thank you for the explanation.
It probably makes sense to add it in the platform reset in that case.

>
>> If we do the generic fix, there is not really a point into implementing
>> a platform specific reset.
>> Note that on linux, when we run "reboot" the driver's remove() function
>> takes care of usb reset.
>
> Does that also work with sysrq-B or reboot -nf (one of the fast reboots) ?

# echo b > /proc/sysrq-trigger 
[ 3713.306318] sysrq: Resetting
[ 3713.306462] SMP: stopping secondary CPUs

Also triggers the same bug as the one I'm attempting to solve here.
We observe a very long delay before the host reports:
[31124.104342] usb 1-1: USB disconnect, device number 20

Thank you for bringing that up!