LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] usb: dwc3: gadget: Stop EP0 transfers during pullup disable
@ 2021-08-23  9:23 Wesley Cheng
  2021-08-23  9:34 ` Felipe Balbi
  0 siblings, 1 reply; 3+ messages in thread
From: Wesley Cheng @ 2021-08-23  9:23 UTC (permalink / raw)
  To: balbi, gregkh, Thinh.Nguyen; +Cc: linux-usb, linux-kernel, jackp, Wesley Cheng

During a USB cable disconnect, or soft disconnect scenario, a pending
SETUP transaction may not be completed, leading to the following
error:

    dwc3 a600000.dwc3: timed out waiting for SETUP phase

If this occurs, then the entire pullup disable routine is skipped and
proper cleanup and halting of the controller does not complete.
Instead of returning an error (which is ignored from the UDC
perspective), allow the pullup disable to routine to continue, which
will also handle disabling of EP0/1.  This will end any active
transfers as well.  Ensure to clear any delayed_status as well, as the
timeout could happen within the STATUS stage.

Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
---
Changes in v2:
 - Removed calls to dwc3_ep0_end_control_data() and just allow the ep disables
   on EP0 handle the proper ending of transfers.
 - Ensure that delayed_status is cleared, as ran into enumeration issues if the
   SETUP transaction fails on a STATUS stage.  Saw delayed_status == TRUE on the
   next connect, which blocked further SETUP transactions to be handled.

 drivers/usb/dwc3/gadget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5d084542718d..8b6a95c35741 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2430,7 +2430,6 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 				msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT));
 		if (ret == 0) {
 			dev_err(dwc->dev, "timed out waiting for SETUP phase\n");
-			return -ETIMEDOUT;
 		}
 	}
 
@@ -2643,6 +2642,7 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
 	/* begin to receive SETUP packets */
 	dwc->ep0state = EP0_SETUP_PHASE;
 	dwc->link_state = DWC3_LINK_STATE_SS_DIS;
+	dwc->delayed_status = false;
 	dwc3_ep0_out_start(dwc);
 
 	dwc3_gadget_enable_irq(dwc);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] usb: dwc3: gadget: Stop EP0 transfers during pullup disable
  2021-08-23  9:23 [PATCH v2] usb: dwc3: gadget: Stop EP0 transfers during pullup disable Wesley Cheng
@ 2021-08-23  9:34 ` Felipe Balbi
  2021-08-23 10:59   ` Wesley Cheng
  0 siblings, 1 reply; 3+ messages in thread
From: Felipe Balbi @ 2021-08-23  9:34 UTC (permalink / raw)
  To: Wesley Cheng; +Cc: gregkh, Thinh.Nguyen, linux-usb, linux-kernel, jackp


Wesley Cheng <wcheng@codeaurora.org> writes:

> During a USB cable disconnect, or soft disconnect scenario, a pending
> SETUP transaction may not be completed, leading to the following
> error:
>
>     dwc3 a600000.dwc3: timed out waiting for SETUP phase
>
> If this occurs, then the entire pullup disable routine is skipped and
> proper cleanup and halting of the controller does not complete.

nit: might want to add a blank line between paragraphs to aid
readability

> Instead of returning an error (which is ignored from the UDC
> perspective), allow the pullup disable to routine to continue, which
                                         ^^
                                         remove this?

> will also handle disabling of EP0/1.  This will end any active
> transfers as well.  Ensure to clear any delayed_status as well, as the
> timeout could happen within the STATUS stage.
>
> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
> ---
> Changes in v2:
>  - Removed calls to dwc3_ep0_end_control_data() and just allow the ep disables
>    on EP0 handle the proper ending of transfers.
>  - Ensure that delayed_status is cleared, as ran into enumeration issues if the
>    SETUP transaction fails on a STATUS stage.  Saw delayed_status == TRUE on the
>    next connect, which blocked further SETUP transactions to be handled.
>
>  drivers/usb/dwc3/gadget.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 5d084542718d..8b6a95c35741 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2430,7 +2430,6 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  				msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT));
>  		if (ret == 0) {
>  			dev_err(dwc->dev, "timed out waiting for SETUP phase\n");
> -			return -ETIMEDOUT;
>  		}

Since the `if' now has a single statement, you should remove the curly braces.

-- 
balbi

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] usb: dwc3: gadget: Stop EP0 transfers during pullup disable
  2021-08-23  9:34 ` Felipe Balbi
@ 2021-08-23 10:59   ` Wesley Cheng
  0 siblings, 0 replies; 3+ messages in thread
From: Wesley Cheng @ 2021-08-23 10:59 UTC (permalink / raw)
  To: Felipe Balbi; +Cc: gregkh, Thinh.Nguyen, linux-usb, linux-kernel, jackp

Hi Felipe,

On 8/23/2021 2:34 AM, Felipe Balbi wrote:
> 
> Wesley Cheng <wcheng@codeaurora.org> writes:
> 
>> During a USB cable disconnect, or soft disconnect scenario, a pending
>> SETUP transaction may not be completed, leading to the following
>> error:
>>
>>     dwc3 a600000.dwc3: timed out waiting for SETUP phase
>>
>> If this occurs, then the entire pullup disable routine is skipped and
>> proper cleanup and halting of the controller does not complete.
> 
> nit: might want to add a blank line between paragraphs to aid
> readability
> 
>> Instead of returning an error (which is ignored from the UDC
>> perspective), allow the pullup disable to routine to continue, which
>                                          ^^
>                                          remove this?
> 
>> will also handle disabling of EP0/1.  This will end any active
>> transfers as well.  Ensure to clear any delayed_status as well, as the
>> timeout could happen within the STATUS stage.
>>
>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
>> ---
>> Changes in v2:
>>  - Removed calls to dwc3_ep0_end_control_data() and just allow the ep disables
>>    on EP0 handle the proper ending of transfers.
>>  - Ensure that delayed_status is cleared, as ran into enumeration issues if the
>>    SETUP transaction fails on a STATUS stage.  Saw delayed_status == TRUE on the
>>    next connect, which blocked further SETUP transactions to be handled.
>>
>>  drivers/usb/dwc3/gadget.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 5d084542718d..8b6a95c35741 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2430,7 +2430,6 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>  				msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT));
>>  		if (ret == 0) {
>>  			dev_err(dwc->dev, "timed out waiting for SETUP phase\n");
>> -			return -ETIMEDOUT;
>>  		}
> 
> Since the `if' now has a single statement, you should remove the curly braces.
> 
Thanks for the reviews!  Will fix them up and resend.

Thanks
Wesley Cheng

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-08-23 10:59 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-23  9:23 [PATCH v2] usb: dwc3: gadget: Stop EP0 transfers during pullup disable Wesley Cheng
2021-08-23  9:34 ` Felipe Balbi
2021-08-23 10:59   ` Wesley Cheng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).