[PATCH 0/5] Fix controller halt and endxfer timeout issues

[PATCH 0/5] Fix controller halt and endxfer timeout issues

[Wesley Cheng]

This patch series addresses some issues seen while testing with the latest
soft disconnect implementation where EP events are allowed to process while
the controller halt is occurring.

#1
Since routines can now interweave, we can see that the soft disconnect can
occur while conndone is being serviced.  This leads to a controller halt
timeout, as the soft disconnect clears the DEP flags, for which conndone
interrupt handler will issue a __dwc3_ep_enable(ep0), that leads to
re-issuing the set ep config command for every endpoint.

#2
Function drivers can ask for a delayed_status phase, while it processes the
received SETUP packet.  This can lead to large delays when handling the
soft disconnect routine.  To improve the timing, forcefully send the status
phase, as we are going to disconnect from the host.

#3
Ensure that local interrupts are left enabled, so that EP0 events can be
processed while the soft disconnect/dequeue is happening.

#4
Modify the DWC3_EP_DELAY_STOP flag management so that if these flags were set
before soft disconnect, that the disconnect routine will be able to properly
issue the endxfer command.

#5
Since EP0 events can occur during controller halt, it may increase the time
needed for the controller to fully stop.

Wesley Cheng (5):
  usb: dwc3: Do not service EP0 and conndone events if soft disconnected
  usb: dwc3: gadget: Force sending delayed status during soft disconnect
  usb: dwc3: gadget: Adjust IRQ management during soft
    disconnect/connect
  usb: dwc3: Allow end transfer commands to be sent during soft
    disconnect
  usb: dwc3: gadget: Increase DWC3 controller halt timeout

 drivers/usb/dwc3/ep0.c    |  9 +++++----
 drivers/usb/dwc3/gadget.c | 31 ++++++++++++++++++++++---------
 2 files changed, 27 insertions(+), 13 deletions(-)

[PATCH 1/5] usb: dwc3: Do not service EP0 and conndone events if soft disconnected

[Wesley Cheng]

There are some operations that need to be ignored if there is a soft
disconnect in progress.  This is to avoid having a pending EP0 transfer in
progress while attempting to stop active transfers and halting the
controller.

There were several instances seen where a soft disconnect was able to occur
during early link negotiation, i.e. bus reset/conndone, which leads to the
conndone handler re-configuring EPs while attempting to halt the
controller, as DEP flags are cleared as part of the soft disconnect path.

ep0out: cmd 'Start New Configuration'
ep0out: cmd 'Set Endpoint Transfer Resource'
ep0in: cmd 'Set Endpoint Transfer Resource'
ep1out: cmd 'Set Endpoint Transfer Resource'
...
event (00030601): Suspend [U3]
event (00000101): Reset [U0]
ep0out: req ffffff87e5c9e100 length 0/0 zsI ==> 0
event (00000201): Connection Done [U0]
ep0out: cmd 'Start New Configuration'
ep0out: cmd 'Set Endpoint Transfer Resource'

In addition, if a soft disconnect occurs, EP0 events are still allowed to
process, however, it will stall/restart during the SETUP phase.  The
host is still able to query for the DATA phase, leading to a
xfernotready(DATA) event.  Since none of the SETUP transfer parameters are
populated, the xfernotready is treated as a "wrong direction" error,
leading to a duplicate stall/restart routine.

Add the proper softconnect/connected checks in sequences that are
potentially involved during soft disconnect processing.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 drivers/usb/dwc3/ep0.c    | 6 ++++--
 drivers/usb/dwc3/gadget.c | 3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 2a510e84eef4..506ef717fdc0 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
 	int				ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
-	if (!dep->endpoint.desc || !dwc->pullups_connected) {
+	if (!dep->endpoint.desc || !dwc->pullups_connected || !dwc->connected) {
 		dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
 				dep->name);
 		ret = -ESHUTDOWN;
@@ -813,7 +813,7 @@ static void dwc3_ep0_inspect_setup(struct dwc3 *dwc,
 	int ret = -EINVAL;
 	u32 len;
 
-	if (!dwc->gadget_driver || !dwc->connected)
+	if (!dwc->gadget_driver || !dwc->softconnect || !dwc->connected)
 		goto out;
 
 	trace_dwc3_ctrl_req(ctrl);
@@ -1116,6 +1116,8 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
 {
 	switch (event->status) {
 	case DEPEVT_STATUS_CONTROL_DATA:
+		if (!dwc->softconnect || !dwc->connected)
+			return;
 		/*
 		 * We already have a DATA transfer in the controller's cache,
 		 * if we receive a XferNotReady(DATA) we will ignore it, unless
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index a944c7a6c83a..298e842c384c 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3877,6 +3877,9 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
 	u8			lanes = 1;
 	u8			speed;
 
+	if (!dwc->softconnect)
+		return;
+
 	reg = dwc3_readl(dwc->regs, DWC3_DSTS);
 	speed = reg & DWC3_DSTS_CONNECTSPD;
 	dwc->speed = speed;

[PATCH 2/5] usb: dwc3: gadget: Force sending delayed status during soft disconnect

[Wesley Cheng]

If any function drivers request for a delayed status phase, this leads to a
SETUP transfer timeout error, since the function may take longer to process
the DATA stage.  This eventually results in end transfer timeouts, as there
is a pending SETUP transaction.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 drivers/usb/dwc3/gadget.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 298e842c384c..75cbc3f185d0 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2513,6 +2513,9 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
 	if (dwc->ep0state != EP0_SETUP_PHASE) {
 		int ret;
 
+		if (dwc->delayed_status)
+			dwc3_ep0_send_delayed_status(dwc);
+
 		reinit_completion(&dwc->ep0_in_setup);
 
 		spin_unlock_irqrestore(&dwc->lock, flags);

[PATCH 3/5] usb: dwc3: gadget: Adjust IRQ management during soft disconnect/connect

[Wesley Cheng]

Local interrupts are currently being disabled as part of aquiring the
spin lock before issuing the endxfer command.  Leave interrupts enabled, so
that EP0 events can continue to be processed.  Also, ensure that there are
no pending interrupts before attempting to handle any soft
connect/disconnect.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 drivers/usb/dwc3/gadget.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 75cbc3f185d0..bd40608b19df 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2046,7 +2046,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 
 	trace_dwc3_ep_dequeue(req);
 
-	spin_lock_irqsave(&dwc->lock, flags);
+	spin_lock(&dwc->lock);
 
 	list_for_each_entry(r, &dep->cancelled_list, list) {
 		if (r == req)
@@ -2085,7 +2085,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 		request, ep->name);
 	ret = -EINVAL;
 out:
-	spin_unlock_irqrestore(&dwc->lock, flags);
+	spin_unlock(&dwc->lock);
 
 	return ret;
 }
@@ -2501,9 +2501,7 @@ static int __dwc3_gadget_start(struct dwc3 *dwc);
 
 static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&dwc->lock, flags);
+	spin_lock(&dwc->lock);
 	dwc->connected = false;
 
 	/*
@@ -2518,10 +2516,10 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
 
 		reinit_completion(&dwc->ep0_in_setup);
 
-		spin_unlock_irqrestore(&dwc->lock, flags);
+		spin_unlock(&dwc->lock);
 		ret = wait_for_completion_timeout(&dwc->ep0_in_setup,
 				msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT));
-		spin_lock_irqsave(&dwc->lock, flags);
+		spin_lock(&dwc->lock);
 		if (ret == 0)
 			dev_warn(dwc->dev, "timed out waiting for SETUP phase\n");
 	}
@@ -2535,7 +2533,7 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
 	 */
 	dwc3_stop_active_transfers(dwc);
 	__dwc3_gadget_stop(dwc);
-	spin_unlock_irqrestore(&dwc->lock, flags);
+	spin_unlock(&dwc->lock);
 
 	/*
 	 * Note: if the GEVNTCOUNT indicates events in the event buffer, the
@@ -2581,6 +2579,8 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 		return 0;
 	}
 
+	synchronize_irq(dwc->irq_gadget);
+
 	if (!is_on) {
 		ret = dwc3_gadget_soft_disconnect(dwc);
 	} else {
@@ -3740,7 +3740,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	 * This mode is NOT available on the DWC_usb31 IP.
 	 */
 
+	spin_unlock(&dwc->lock);
 	__dwc3_stop_active_transfer(dep, force, interrupt);
+	spin_lock(&dwc->lock);
+
 }
 
 static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)

[PATCH 4/5] usb: dwc3: Allow end transfer commands to be sent during soft disconnect

[Wesley Cheng]

If soft disconnect is in progress, allow the endxfer command to be sent,
without this, there is an issue where the stop active transfer call
(during pullup disable) wouldn't actually issue the endxfer command,
while clearing the DEP flag.

In addition, if the DWC3_EP_DELAY_STOP flag was set before soft disconnect
started (i.e. from the dequeue path), ensure that when the EP0 transaction
completes during soft disconnect, to issue the endxfer with the force
parameter set, as it does not expect a command complete event.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 drivers/usb/dwc3/ep0.c    | 3 +--
 drivers/usb/dwc3/gadget.c | 5 ++++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 506ef717fdc0..5851b0e9db0a 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
 		if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
 			continue;
 
-		dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
-		dwc3_stop_active_transfer(dwc3_ep, true, true);
+		dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
 	}
 }
 
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index bd40608b19df..fba2797ad9ae 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
 		return;
 
+	if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
+		return;
+
 	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
-	    (dep->flags & DWC3_EP_DELAY_STOP) ||
 	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
 		return;
 
@@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	__dwc3_stop_active_transfer(dep, force, interrupt);
 	spin_lock(&dwc->lock);
 
+	dep->flags &= ~DWC3_EP_DELAY_STOP;
 }
 
 static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)

[PATCH 5/5] usb: dwc3: gadget: Increase DWC3 controller halt timeout

[Wesley Cheng]

Since EP0 transactions need to be completed before the controller halt
sequence is finished, this may take some time depending on the host and the
enabled functions.  Increase the controller halt timeout, so that we give
the controller sufficient time to handle EP0 transfers.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 drivers/usb/dwc3/gadget.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index fba2797ad9ae..a5c0e39bd002 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2487,6 +2487,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
 	do {
 		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
 		reg &= DWC3_DSTS_DEVCTRLHLT;
+		msleep(1);
 	} while (--timeout && !(!is_on ^ !reg));
 
 	if (!timeout)

Re: [PATCH 5/5] usb: dwc3: gadget: Increase DWC3 controller halt timeout

[Thinh Nguyen]

On 7/8/2022, Wesley Cheng wrote:
> Since EP0 transactions need to be completed before the controller halt
> sequence is finished, this may take some time depending on the host and the
> enabled functions.  Increase the controller halt timeout, so that we give
> the controller sufficient time to handle EP0 transfers.
>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>   drivers/usb/dwc3/gadget.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index fba2797ad9ae..a5c0e39bd002 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2487,6 +2487,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>   	do {
>   		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
>   		reg &= DWC3_DSTS_DEVCTRLHLT;
> +		msleep(1);

I think it makes more sense to msleep() first before reading the register.

Thanks,
Thinh

>   	} while (--timeout && !(!is_on ^ !reg));
>   
>   	if (!timeout)

Re: [PATCH 4/5] usb: dwc3: Allow end transfer commands to be sent during soft disconnect

[Thinh Nguyen]

On 7/8/2022, Wesley Cheng wrote:
> If soft disconnect is in progress, allow the endxfer command to be sent,
> without this, there is an issue where the stop active transfer call
> (during pullup disable) wouldn't actually issue the endxfer command,
> while clearing the DEP flag.
>
> In addition, if the DWC3_EP_DELAY_STOP flag was set before soft disconnect
> started (i.e. from the dequeue path), ensure that when the EP0 transaction
> completes during soft disconnect, to issue the endxfer with the force
> parameter set, as it does not expect a command complete event.
>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>   drivers/usb/dwc3/ep0.c    | 3 +--
>   drivers/usb/dwc3/gadget.c | 5 ++++-
>   2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 506ef717fdc0..5851b0e9db0a 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>   		if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
>   			continue;
>   
> -		dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
> -		dwc3_stop_active_transfer(dwc3_ep, true, true);
> +		dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
>   	}
>   }
>   
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index bd40608b19df..fba2797ad9ae 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>   	if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
>   		return;
>   
> +	if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
> +		return;
> +
>   	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
> -	    (dep->flags & DWC3_EP_DELAY_STOP) ||
>   	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>   		return;
>   
> @@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>   	__dwc3_stop_active_transfer(dep, force, interrupt);
>   	spin_lock(&dwc->lock);
>   
> +	dep->flags &= ~DWC3_EP_DELAY_STOP;

Can we clear this flag in __dwc3_stop_active_transfer(). It should apply 
if End Transfer command was sent.

Thanks,
Thinh

>   }
>   
>   static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)

Re: [PATCH 3/5] usb: dwc3: gadget: Adjust IRQ management during soft disconnect/connect

[kernel test robot]

Hi Wesley,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on linus/master v5.19-rc5 next-20220708]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Wesley-Cheng/Fix-controller-halt-and-endxfer-timeout-issues/20220709-025241
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-randconfig-a002 (https://download.01.org/0day-ci/archive/20220709/202207091054.eGEUvBXn-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/457fe4752b0f6dcc5c1b329f91003b7ffc518b44
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Wesley-Cheng/Fix-controller-halt-and-endxfer-timeout-issues/20220709-025241
        git checkout 457fe4752b0f6dcc5c1b329f91003b7ffc518b44
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/usb/dwc3/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/usb/dwc3/gadget.c: In function 'dwc3_gadget_ep_dequeue':
>> drivers/usb/dwc3/gadget.c:2032:41: warning: unused variable 'flags' [-Wunused-variable]
    2032 |         unsigned long                   flags;
         |                                         ^~~~~


vim +/flags +2032 drivers/usb/dwc3/gadget.c

d4f1afe5e896c1 Felipe Balbi 2018-08-01  2022  
72246da40f3719 Felipe Balbi 2011-08-19  2023  static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
72246da40f3719 Felipe Balbi 2011-08-19  2024  		struct usb_request *request)
72246da40f3719 Felipe Balbi 2011-08-19  2025  {
72246da40f3719 Felipe Balbi 2011-08-19  2026  	struct dwc3_request		*req = to_dwc3_request(request);
72246da40f3719 Felipe Balbi 2011-08-19  2027  	struct dwc3_request		*r = NULL;
72246da40f3719 Felipe Balbi 2011-08-19  2028  
72246da40f3719 Felipe Balbi 2011-08-19  2029  	struct dwc3_ep			*dep = to_dwc3_ep(ep);
72246da40f3719 Felipe Balbi 2011-08-19  2030  	struct dwc3			*dwc = dep->dwc;
72246da40f3719 Felipe Balbi 2011-08-19  2031  
72246da40f3719 Felipe Balbi 2011-08-19 @2032  	unsigned long			flags;
72246da40f3719 Felipe Balbi 2011-08-19  2033  	int				ret = 0;
72246da40f3719 Felipe Balbi 2011-08-19  2034  
2c4cbe6e5a9c71 Felipe Balbi 2014-04-30  2035  	trace_dwc3_ep_dequeue(req);
2c4cbe6e5a9c71 Felipe Balbi 2014-04-30  2036  
457fe4752b0f6d Wesley Cheng 2022-07-08  2037  	spin_lock(&dwc->lock);
72246da40f3719 Felipe Balbi 2011-08-19  2038  
a7027ca69d82ae Thinh Nguyen 2020-03-05  2039  	list_for_each_entry(r, &dep->cancelled_list, list) {
72246da40f3719 Felipe Balbi 2011-08-19  2040  		if (r == req)
fcd2def6639293 Thinh Nguyen 2020-03-05  2041  			goto out;
72246da40f3719 Felipe Balbi 2011-08-19  2042  	}
72246da40f3719 Felipe Balbi 2011-08-19  2043  
aa3342c8bb618a Felipe Balbi 2016-03-14  2044  	list_for_each_entry(r, &dep->pending_list, list) {
fcd2def6639293 Thinh Nguyen 2020-03-05  2045  		if (r == req) {
fcd2def6639293 Thinh Nguyen 2020-03-05  2046  			dwc3_gadget_giveback(dep, req, -ECONNRESET);
fcd2def6639293 Thinh Nguyen 2020-03-05  2047  			goto out;
fcd2def6639293 Thinh Nguyen 2020-03-05  2048  		}
72246da40f3719 Felipe Balbi 2011-08-19  2049  	}
72246da40f3719 Felipe Balbi 2011-08-19  2050  
aa3342c8bb618a Felipe Balbi 2016-03-14  2051  	list_for_each_entry(r, &dep->started_list, list) {
72246da40f3719 Felipe Balbi 2011-08-19  2052  		if (r == req) {
a7027ca69d82ae Thinh Nguyen 2020-03-05  2053  			struct dwc3_request *t;
a7027ca69d82ae Thinh Nguyen 2020-03-05  2054  
72246da40f3719 Felipe Balbi 2011-08-19  2055  			/* wait until it is processed */
c5353b225df9b2 Felipe Balbi 2019-02-13  2056  			dwc3_stop_active_transfer(dep, true, true);
cf3113d893d442 Felipe Balbi 2017-02-17  2057  
a7027ca69d82ae Thinh Nguyen 2020-03-05  2058  			/*
a7027ca69d82ae Thinh Nguyen 2020-03-05  2059  			 * Remove any started request if the transfer is
a7027ca69d82ae Thinh Nguyen 2020-03-05  2060  			 * cancelled.
a7027ca69d82ae Thinh Nguyen 2020-03-05  2061  			 */
a7027ca69d82ae Thinh Nguyen 2020-03-05  2062  			list_for_each_entry_safe(r, t, &dep->started_list, list)
04dd6e76b22889 Ray Chi      2021-03-28  2063  				dwc3_gadget_move_cancelled_request(r,
04dd6e76b22889 Ray Chi      2021-03-28  2064  						DWC3_REQUEST_STATUS_DEQUEUED);
cf3113d893d442 Felipe Balbi 2017-02-17  2065  
a5c7682aaaa10e Thinh Nguyen 2021-01-04  2066  			dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
a5c7682aaaa10e Thinh Nguyen 2021-01-04  2067  
fcd2def6639293 Thinh Nguyen 2020-03-05  2068  			goto out;
72246da40f3719 Felipe Balbi 2011-08-19  2069  		}
72246da40f3719 Felipe Balbi 2011-08-19  2070  	}
fcd2def6639293 Thinh Nguyen 2020-03-05  2071  
04fb365c453e14 Felipe Balbi 2017-05-17  2072  	dev_err(dwc->dev, "request %pK was not queued to %s\n",
72246da40f3719 Felipe Balbi 2011-08-19  2073  		request, ep->name);
72246da40f3719 Felipe Balbi 2011-08-19  2074  	ret = -EINVAL;
fcd2def6639293 Thinh Nguyen 2020-03-05  2075  out:
457fe4752b0f6d Wesley Cheng 2022-07-08  2076  	spin_unlock(&dwc->lock);
72246da40f3719 Felipe Balbi 2011-08-19  2077  
72246da40f3719 Felipe Balbi 2011-08-19  2078  	return ret;
72246da40f3719 Felipe Balbi 2011-08-19  2079  }
72246da40f3719 Felipe Balbi 2011-08-19  2080  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 3/5] usb: dwc3: gadget: Adjust IRQ management during soft disconnect/connect

[kernel test robot]

Hi Wesley,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on linus/master v5.19-rc5 next-20220708]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Wesley-Cheng/Fix-controller-halt-and-endxfer-timeout-issues/20220709-025241
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-randconfig-a012 (https://download.01.org/0day-ci/archive/20220709/202207091200.uLedVxl7-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 562c3467a6738aa89203f72fc1d1343e5baadf3c)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/457fe4752b0f6dcc5c1b329f91003b7ffc518b44
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Wesley-Cheng/Fix-controller-halt-and-endxfer-timeout-issues/20220709-025241
        git checkout 457fe4752b0f6dcc5c1b329f91003b7ffc518b44
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/usb/dwc3/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/usb/dwc3/gadget.c:2032:18: warning: unused variable 'flags' [-Wunused-variable]
           unsigned long                   flags;
                                           ^
   1 warning generated.


vim +/flags +2032 drivers/usb/dwc3/gadget.c

d4f1afe5e896c1 Felipe Balbi 2018-08-01  2022  
72246da40f3719 Felipe Balbi 2011-08-19  2023  static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
72246da40f3719 Felipe Balbi 2011-08-19  2024  		struct usb_request *request)
72246da40f3719 Felipe Balbi 2011-08-19  2025  {
72246da40f3719 Felipe Balbi 2011-08-19  2026  	struct dwc3_request		*req = to_dwc3_request(request);
72246da40f3719 Felipe Balbi 2011-08-19  2027  	struct dwc3_request		*r = NULL;
72246da40f3719 Felipe Balbi 2011-08-19  2028  
72246da40f3719 Felipe Balbi 2011-08-19  2029  	struct dwc3_ep			*dep = to_dwc3_ep(ep);
72246da40f3719 Felipe Balbi 2011-08-19  2030  	struct dwc3			*dwc = dep->dwc;
72246da40f3719 Felipe Balbi 2011-08-19  2031  
72246da40f3719 Felipe Balbi 2011-08-19 @2032  	unsigned long			flags;
72246da40f3719 Felipe Balbi 2011-08-19  2033  	int				ret = 0;
72246da40f3719 Felipe Balbi 2011-08-19  2034  
2c4cbe6e5a9c71 Felipe Balbi 2014-04-30  2035  	trace_dwc3_ep_dequeue(req);
2c4cbe6e5a9c71 Felipe Balbi 2014-04-30  2036  
457fe4752b0f6d Wesley Cheng 2022-07-08  2037  	spin_lock(&dwc->lock);
72246da40f3719 Felipe Balbi 2011-08-19  2038  
a7027ca69d82ae Thinh Nguyen 2020-03-05  2039  	list_for_each_entry(r, &dep->cancelled_list, list) {
72246da40f3719 Felipe Balbi 2011-08-19  2040  		if (r == req)
fcd2def6639293 Thinh Nguyen 2020-03-05  2041  			goto out;
72246da40f3719 Felipe Balbi 2011-08-19  2042  	}
72246da40f3719 Felipe Balbi 2011-08-19  2043  
aa3342c8bb618a Felipe Balbi 2016-03-14  2044  	list_for_each_entry(r, &dep->pending_list, list) {
fcd2def6639293 Thinh Nguyen 2020-03-05  2045  		if (r == req) {
fcd2def6639293 Thinh Nguyen 2020-03-05  2046  			dwc3_gadget_giveback(dep, req, -ECONNRESET);
fcd2def6639293 Thinh Nguyen 2020-03-05  2047  			goto out;
fcd2def6639293 Thinh Nguyen 2020-03-05  2048  		}
72246da40f3719 Felipe Balbi 2011-08-19  2049  	}
72246da40f3719 Felipe Balbi 2011-08-19  2050  
aa3342c8bb618a Felipe Balbi 2016-03-14  2051  	list_for_each_entry(r, &dep->started_list, list) {
72246da40f3719 Felipe Balbi 2011-08-19  2052  		if (r == req) {
a7027ca69d82ae Thinh Nguyen 2020-03-05  2053  			struct dwc3_request *t;
a7027ca69d82ae Thinh Nguyen 2020-03-05  2054  
72246da40f3719 Felipe Balbi 2011-08-19  2055  			/* wait until it is processed */
c5353b225df9b2 Felipe Balbi 2019-02-13  2056  			dwc3_stop_active_transfer(dep, true, true);
cf3113d893d442 Felipe Balbi 2017-02-17  2057  
a7027ca69d82ae Thinh Nguyen 2020-03-05  2058  			/*
a7027ca69d82ae Thinh Nguyen 2020-03-05  2059  			 * Remove any started request if the transfer is
a7027ca69d82ae Thinh Nguyen 2020-03-05  2060  			 * cancelled.
a7027ca69d82ae Thinh Nguyen 2020-03-05  2061  			 */
a7027ca69d82ae Thinh Nguyen 2020-03-05  2062  			list_for_each_entry_safe(r, t, &dep->started_list, list)
04dd6e76b22889 Ray Chi      2021-03-28  2063  				dwc3_gadget_move_cancelled_request(r,
04dd6e76b22889 Ray Chi      2021-03-28  2064  						DWC3_REQUEST_STATUS_DEQUEUED);
cf3113d893d442 Felipe Balbi 2017-02-17  2065  
a5c7682aaaa10e Thinh Nguyen 2021-01-04  2066  			dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
a5c7682aaaa10e Thinh Nguyen 2021-01-04  2067  
fcd2def6639293 Thinh Nguyen 2020-03-05  2068  			goto out;
72246da40f3719 Felipe Balbi 2011-08-19  2069  		}
72246da40f3719 Felipe Balbi 2011-08-19  2070  	}
fcd2def6639293 Thinh Nguyen 2020-03-05  2071  
04fb365c453e14 Felipe Balbi 2017-05-17  2072  	dev_err(dwc->dev, "request %pK was not queued to %s\n",
72246da40f3719 Felipe Balbi 2011-08-19  2073  		request, ep->name);
72246da40f3719 Felipe Balbi 2011-08-19  2074  	ret = -EINVAL;
fcd2def6639293 Thinh Nguyen 2020-03-05  2075  out:
457fe4752b0f6d Wesley Cheng 2022-07-08  2076  	spin_unlock(&dwc->lock);
72246da40f3719 Felipe Balbi 2011-08-19  2077  
72246da40f3719 Felipe Balbi 2011-08-19  2078  	return ret;
72246da40f3719 Felipe Balbi 2011-08-19  2079  }
72246da40f3719 Felipe Balbi 2011-08-19  2080  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 4/5] usb: dwc3: Allow end transfer commands to be sent during soft disconnect

[Wesley Cheng]

Hi Thinh,

On 7/8/2022 6:58 PM, Thinh Nguyen wrote:
> On 7/8/2022, Wesley Cheng wrote:
>> If soft disconnect is in progress, allow the endxfer command to be sent,
>> without this, there is an issue where the stop active transfer call
>> (during pullup disable) wouldn't actually issue the endxfer command,
>> while clearing the DEP flag.
>>
>> In addition, if the DWC3_EP_DELAY_STOP flag was set before soft disconnect
>> started (i.e. from the dequeue path), ensure that when the EP0 transaction
>> completes during soft disconnect, to issue the endxfer with the force
>> parameter set, as it does not expect a command complete event.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>    drivers/usb/dwc3/ep0.c    | 3 +--
>>    drivers/usb/dwc3/gadget.c | 5 ++++-
>>    2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 506ef717fdc0..5851b0e9db0a 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>>    		if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
>>    			continue;
>>    
>> -		dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
>> -		dwc3_stop_active_transfer(dwc3_ep, true, true);
>> +		dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
>>    	}
>>    }
>>    
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index bd40608b19df..fba2797ad9ae 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>>    	if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
>>    		return;
>>    
>> +	if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
>> +		return;
>> +
>>    	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>> -	    (dep->flags & DWC3_EP_DELAY_STOP) ||
>>    	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>    		return;
>>    
>> @@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>>    	__dwc3_stop_active_transfer(dep, force, interrupt);
>>    	spin_lock(&dwc->lock);
>>    
>> +	dep->flags &= ~DWC3_EP_DELAY_STOP;
> 
> Can we clear this flag in __dwc3_stop_active_transfer(). It should apply
> if End Transfer command was sent.

I wanted to make sure that we weren't modifying the DEP flags outside of 
a spin lock.  Patch#3 modifies it where we unlock before calling 
__dwc3_stop_active_transfer(), so we can allow the dwc3 threaded IRQ 
handle events while the cmd status polling happens.

Maybe we can unlock/lock the dwc3->lock inside 
__dwc3_stop_active_transfer() and that way we can ensure DEP flags are 
modified properly?

Thanks
Wesley Cheng

Re: [PATCH 4/5] usb: dwc3: Allow end transfer commands to be sent during soft disconnect

[Thinh Nguyen]

On 7/12/2022, Wesley Cheng wrote:
> Hi Thinh,
>
> On 7/8/2022 6:58 PM, Thinh Nguyen wrote:
>> On 7/8/2022, Wesley Cheng wrote:
>>> If soft disconnect is in progress, allow the endxfer command to be 
>>> sent,
>>> without this, there is an issue where the stop active transfer call
>>> (during pullup disable) wouldn't actually issue the endxfer command,
>>> while clearing the DEP flag.
>>>
>>> In addition, if the DWC3_EP_DELAY_STOP flag was set before soft 
>>> disconnect
>>> started (i.e. from the dequeue path), ensure that when the EP0 
>>> transaction
>>> completes during soft disconnect, to issue the endxfer with the force
>>> parameter set, as it does not expect a command complete event.
>>>
>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>> ---
>>>    drivers/usb/dwc3/ep0.c    | 3 +--
>>>    drivers/usb/dwc3/gadget.c | 5 ++++-
>>>    2 files changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>> index 506ef717fdc0..5851b0e9db0a 100644
>>> --- a/drivers/usb/dwc3/ep0.c
>>> +++ b/drivers/usb/dwc3/ep0.c
>>> @@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>>>            if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
>>>                continue;
>>>    -        dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
>>> -        dwc3_stop_active_transfer(dwc3_ep, true, true);
>>> +        dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
>>>        }
>>>    }
>>>    diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index bd40608b19df..fba2797ad9ae 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep 
>>> *dep, bool force,
>>>        if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
>>>            return;
>>>    +    if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
>>> +        return;
>>> +
>>>        if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>> -        (dep->flags & DWC3_EP_DELAY_STOP) ||
>>>            (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>>            return;
>>>    @@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct 
>>> dwc3_ep *dep, bool force,
>>>        __dwc3_stop_active_transfer(dep, force, interrupt);
>>>        spin_lock(&dwc->lock);
>>>    +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>
>> Can we clear this flag in __dwc3_stop_active_transfer(). It should apply
>> if End Transfer command was sent.
>
> I wanted to make sure that we weren't modifying the DEP flags outside 
> of a spin lock.  Patch#3 modifies it where we unlock before calling 
> __dwc3_stop_active_transfer(), so we can allow the dwc3 threaded IRQ 
> handle events while the cmd status polling happens.
>
> Maybe we can unlock/lock the dwc3->lock inside 
> __dwc3_stop_active_transfer() and that way we can ensure DEP flags are 
> modified properly?

I didn't realize that you unlock/lock when calling 
__dwc3_stop_active_transfer(). We'd need to be careful if we want to 
unlock/lock it, and avoid it all together if possible. It can be easily 
overlooked just like dwc3_gadget_giveback().

What issue did you see without doing this?

Thanks,
Thinh

Re: [PATCH 4/5] usb: dwc3: Allow end transfer commands to be sent during soft disconnect

[Wesley Cheng]

Hi Thinh,

On 7/12/2022 6:42 PM, Thinh Nguyen wrote:
> On 7/12/2022, Wesley Cheng wrote:
>> Hi Thinh,
>>
>> On 7/8/2022 6:58 PM, Thinh Nguyen wrote:
>>> On 7/8/2022, Wesley Cheng wrote:
>>>> If soft disconnect is in progress, allow the endxfer command to be
>>>> sent,
>>>> without this, there is an issue where the stop active transfer call
>>>> (during pullup disable) wouldn't actually issue the endxfer command,
>>>> while clearing the DEP flag.
>>>>
>>>> In addition, if the DWC3_EP_DELAY_STOP flag was set before soft
>>>> disconnect
>>>> started (i.e. from the dequeue path), ensure that when the EP0
>>>> transaction
>>>> completes during soft disconnect, to issue the endxfer with the force
>>>> parameter set, as it does not expect a command complete event.
>>>>
>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>> ---
>>>>     drivers/usb/dwc3/ep0.c    | 3 +--
>>>>     drivers/usb/dwc3/gadget.c | 5 ++++-
>>>>     2 files changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>> index 506ef717fdc0..5851b0e9db0a 100644
>>>> --- a/drivers/usb/dwc3/ep0.c
>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>> @@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>>>>             if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
>>>>                 continue;
>>>>     -        dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
>>>> -        dwc3_stop_active_transfer(dwc3_ep, true, true);
>>>> +        dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
>>>>         }
>>>>     }
>>>>     diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index bd40608b19df..fba2797ad9ae 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep
>>>> *dep, bool force,
>>>>         if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
>>>>             return;
>>>>     +    if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
>>>> +        return;
>>>> +
>>>>         if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>>> -        (dep->flags & DWC3_EP_DELAY_STOP) ||
>>>>             (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>>>             return;
>>>>     @@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct
>>>> dwc3_ep *dep, bool force,
>>>>         __dwc3_stop_active_transfer(dep, force, interrupt);
>>>>         spin_lock(&dwc->lock);
>>>>     +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>
>>> Can we clear this flag in __dwc3_stop_active_transfer(). It should apply
>>> if End Transfer command was sent.
>>
>> I wanted to make sure that we weren't modifying the DEP flags outside
>> of a spin lock.  Patch#3 modifies it where we unlock before calling
>> __dwc3_stop_active_transfer(), so we can allow the dwc3 threaded IRQ
>> handle events while the cmd status polling happens.
>>
>> Maybe we can unlock/lock the dwc3->lock inside
>> __dwc3_stop_active_transfer() and that way we can ensure DEP flags are
>> modified properly?
> 
> I didn't realize that you unlock/lock when calling
> __dwc3_stop_active_transfer(). We'd need to be careful if we want to
> unlock/lock it, and avoid it all together if possible. It can be easily
> overlooked just like dwc3_gadget_giveback().
> 
> What issue did you see without doing this?

I saw endxfer timeout issues if I didn't do it.  If we keep the lock 
held, then the DWC3 event processing would be blocked across the entire 
time we are waiting for the command act to clear.  With unlocking before 
polling, then at least we're still able to handle the EP0 events that 
are pending.

It was definitely one of the harder scenarios to reproduce.  The main 
patch series which resolved a lot of the issues early on was patch#1. 
After adding that the other issues are seen maybe after a day or so of 
testing.

Thanks
Wesley Cheng

Re: [PATCH 4/5] usb: dwc3: Allow end transfer commands to be sent during soft disconnect

[Thinh Nguyen]

On 7/13/2022, Wesley Cheng wrote:
> Hi Thinh,
>
> On 7/12/2022 6:42 PM, Thinh Nguyen wrote:
>> On 7/12/2022, Wesley Cheng wrote:
>>> Hi Thinh,
>>>
>>> On 7/8/2022 6:58 PM, Thinh Nguyen wrote:
>>>> On 7/8/2022, Wesley Cheng wrote:
>>>>> If soft disconnect is in progress, allow the endxfer command to be
>>>>> sent,
>>>>> without this, there is an issue where the stop active transfer call
>>>>> (during pullup disable) wouldn't actually issue the endxfer command,
>>>>> while clearing the DEP flag.
>>>>>
>>>>> In addition, if the DWC3_EP_DELAY_STOP flag was set before soft
>>>>> disconnect
>>>>> started (i.e. from the dequeue path), ensure that when the EP0
>>>>> transaction
>>>>> completes during soft disconnect, to issue the endxfer with the force
>>>>> parameter set, as it does not expect a command complete event.
>>>>>
>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>>> ---
>>>>>     drivers/usb/dwc3/ep0.c    | 3 +--
>>>>>     drivers/usb/dwc3/gadget.c | 5 ++++-
>>>>>     2 files changed, 5 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>>> index 506ef717fdc0..5851b0e9db0a 100644
>>>>> --- a/drivers/usb/dwc3/ep0.c
>>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>>> @@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>>>>>             if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
>>>>>                 continue;
>>>>>     -        dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>> -        dwc3_stop_active_transfer(dwc3_ep, true, true);
>>>>> +        dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
>>>>>         }
>>>>>     }
>>>>>     diff --git a/drivers/usb/dwc3/gadget.c 
>>>>> b/drivers/usb/dwc3/gadget.c
>>>>> index bd40608b19df..fba2797ad9ae 100644
>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>> @@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep
>>>>> *dep, bool force,
>>>>>         if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
>>>>>             return;
>>>>>     +    if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
>>>>> +        return;
>>>>> +
>>>>>         if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>>>> -        (dep->flags & DWC3_EP_DELAY_STOP) ||
>>>>>             (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>>>>             return;
>>>>>     @@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct
>>>>> dwc3_ep *dep, bool force,
>>>>>         __dwc3_stop_active_transfer(dep, force, interrupt);
>>>>>         spin_lock(&dwc->lock);
>>>>>     +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>
>>>> Can we clear this flag in __dwc3_stop_active_transfer(). It should 
>>>> apply
>>>> if End Transfer command was sent.
>>>
>>> I wanted to make sure that we weren't modifying the DEP flags outside
>>> of a spin lock.  Patch#3 modifies it where we unlock before calling
>>> __dwc3_stop_active_transfer(), so we can allow the dwc3 threaded IRQ
>>> handle events while the cmd status polling happens.
>>>
>>> Maybe we can unlock/lock the dwc3->lock inside
>>> __dwc3_stop_active_transfer() and that way we can ensure DEP flags are
>>> modified properly?
>>
>> I didn't realize that you unlock/lock when calling
>> __dwc3_stop_active_transfer(). We'd need to be careful if we want to
>> unlock/lock it, and avoid it all together if possible. It can be easily
>> overlooked just like dwc3_gadget_giveback().
>>
>> What issue did you see without doing this?
>
> I saw endxfer timeout issues if I didn't do it.  If we keep the lock 
> held, then the DWC3 event processing would be blocked across the 
> entire time we are waiting for the command act to clear.  With 
> unlocking before polling, then at least we're still able to handle the 
> EP0 events that are pending.
>
> It was definitely one of the harder scenarios to reproduce.  The main 
> patch series which resolved a lot of the issues early on was patch#1. 
> After adding that the other issues are seen maybe after a day or so of 
> testing.
>

I see. The soft-disconnect still go through right?
Can you try this and see if this works for you?

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 13e4f1a03417..4f67a484c490 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -451,7 +451,9 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, 
unsigned int cmd,

         dwc3_writel(dep->regs, DWC3_DEPCMD, cmd);

-       if (!(cmd & DWC3_DEPCMD_CMDACT)) {
+       if (!(cmd & DWC3_DEPCMD_CMDACT) ||
+           (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_ENDTRANSFER &&
+            !(cmd & DWC3_DEPCMD_CMDIOC))) {
                 ret = 0;
                 goto skip_status;
         }


We can set it and forget it when we're about to de-initialize the 
controller. Just like what we've already done by not waiting for the End 
Transfer endpoint command completion interrupt.

Thanks,
Thinh

Re: [PATCH 4/5] usb: dwc3: Allow end transfer commands to be sent during soft disconnect

[Wesley Cheng]

Hi Thinh,

On 7/13/2022 5:19 PM, Thinh Nguyen wrote:
> On 7/13/2022, Wesley Cheng wrote:
>> Hi Thinh,
>>
>> On 7/12/2022 6:42 PM, Thinh Nguyen wrote:
>>> On 7/12/2022, Wesley Cheng wrote:
>>>> Hi Thinh,
>>>>
>>>> On 7/8/2022 6:58 PM, Thinh Nguyen wrote:
>>>>> On 7/8/2022, Wesley Cheng wrote:
>>>>>> If soft disconnect is in progress, allow the endxfer command to be
>>>>>> sent,
>>>>>> without this, there is an issue where the stop active transfer call
>>>>>> (during pullup disable) wouldn't actually issue the endxfer command,
>>>>>> while clearing the DEP flag.
>>>>>>
>>>>>> In addition, if the DWC3_EP_DELAY_STOP flag was set before soft
>>>>>> disconnect
>>>>>> started (i.e. from the dequeue path), ensure that when the EP0
>>>>>> transaction
>>>>>> completes during soft disconnect, to issue the endxfer with the force
>>>>>> parameter set, as it does not expect a command complete event.
>>>>>>
>>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>>>> ---
>>>>>>      drivers/usb/dwc3/ep0.c    | 3 +--
>>>>>>      drivers/usb/dwc3/gadget.c | 5 ++++-
>>>>>>      2 files changed, 5 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>>>> index 506ef717fdc0..5851b0e9db0a 100644
>>>>>> --- a/drivers/usb/dwc3/ep0.c
>>>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>>>> @@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>>>>>>              if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
>>>>>>                  continue;
>>>>>>      -        dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>>> -        dwc3_stop_active_transfer(dwc3_ep, true, true);
>>>>>> +        dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
>>>>>>          }
>>>>>>      }
>>>>>>      diff --git a/drivers/usb/dwc3/gadget.c
>>>>>> b/drivers/usb/dwc3/gadget.c
>>>>>> index bd40608b19df..fba2797ad9ae 100644
>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>> @@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep
>>>>>> *dep, bool force,
>>>>>>          if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
>>>>>>              return;
>>>>>>      +    if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
>>>>>> +        return;
>>>>>> +
>>>>>>          if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>>>>> -        (dep->flags & DWC3_EP_DELAY_STOP) ||
>>>>>>              (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>>>>>              return;
>>>>>>      @@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct
>>>>>> dwc3_ep *dep, bool force,
>>>>>>          __dwc3_stop_active_transfer(dep, force, interrupt);
>>>>>>          spin_lock(&dwc->lock);
>>>>>>      +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>>
>>>>> Can we clear this flag in __dwc3_stop_active_transfer(). It should
>>>>> apply
>>>>> if End Transfer command was sent.
>>>>
>>>> I wanted to make sure that we weren't modifying the DEP flags outside
>>>> of a spin lock.  Patch#3 modifies it where we unlock before calling
>>>> __dwc3_stop_active_transfer(), so we can allow the dwc3 threaded IRQ
>>>> handle events while the cmd status polling happens.
>>>>
>>>> Maybe we can unlock/lock the dwc3->lock inside
>>>> __dwc3_stop_active_transfer() and that way we can ensure DEP flags are
>>>> modified properly?
>>>
>>> I didn't realize that you unlock/lock when calling
>>> __dwc3_stop_active_transfer(). We'd need to be careful if we want to
>>> unlock/lock it, and avoid it all together if possible. It can be easily
>>> overlooked just like dwc3_gadget_giveback().
>>>
>>> What issue did you see without doing this?
>>
>> I saw endxfer timeout issues if I didn't do it.  If we keep the lock
>> held, then the DWC3 event processing would be blocked across the
>> entire time we are waiting for the command act to clear.  With
>> unlocking before polling, then at least we're still able to handle the
>> EP0 events that are pending.
>>
>> It was definitely one of the harder scenarios to reproduce.  The main
>> patch series which resolved a lot of the issues early on was patch#1.
>> After adding that the other issues are seen maybe after a day or so of
>> testing.
>>
> 
> I see. The soft-disconnect still go through right?
> Can you try this and see if this works for you?
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 13e4f1a03417..4f67a484c490 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -451,7 +451,9 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep,
> unsigned int cmd,
> 
>           dwc3_writel(dep->regs, DWC3_DEPCMD, cmd);
> 
> -       if (!(cmd & DWC3_DEPCMD_CMDACT)) {
> +       if (!(cmd & DWC3_DEPCMD_CMDACT) ||
> +           (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_ENDTRANSFER &&
> +            !(cmd & DWC3_DEPCMD_CMDIOC))) {
>                   ret = 0;
>                   goto skip_status;
>           }
> 
> 
> We can set it and forget it when we're about to de-initialize the
> controller. Just like what we've already done by not waiting for the End
> Transfer endpoint command completion interrupt.
> 

I can see this working for the soft disconnect case, but we'll need to 
address it differently for the EP dequeue scenario where IOC/interrupt 
is required to clean up the TRBs.  This is one of the reasons why I went 
with the spinlock approach, since it would apply to both situations.

Thanks
Wesley Cheng

Re: [PATCH 4/5] usb: dwc3: Allow end transfer commands to be sent during soft disconnect

[Thinh Nguyen]

On 7/13/2022, Wesley Cheng wrote:
> Hi Thinh,
>
> On 7/13/2022 5:19 PM, Thinh Nguyen wrote:
>> On 7/13/2022, Wesley Cheng wrote:
>>> Hi Thinh,
>>>
>>> On 7/12/2022 6:42 PM, Thinh Nguyen wrote:
>>>> On 7/12/2022, Wesley Cheng wrote:
>>>>> Hi Thinh,
>>>>>
>>>>> On 7/8/2022 6:58 PM, Thinh Nguyen wrote:
>>>>>> On 7/8/2022, Wesley Cheng wrote:
>>>>>>> If soft disconnect is in progress, allow the endxfer command to be
>>>>>>> sent,
>>>>>>> without this, there is an issue where the stop active transfer call
>>>>>>> (during pullup disable) wouldn't actually issue the endxfer 
>>>>>>> command,
>>>>>>> while clearing the DEP flag.
>>>>>>>
>>>>>>> In addition, if the DWC3_EP_DELAY_STOP flag was set before soft
>>>>>>> disconnect
>>>>>>> started (i.e. from the dequeue path), ensure that when the EP0
>>>>>>> transaction
>>>>>>> completes during soft disconnect, to issue the endxfer with the 
>>>>>>> force
>>>>>>> parameter set, as it does not expect a command complete event.
>>>>>>>
>>>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>>>>> ---
>>>>>>>      drivers/usb/dwc3/ep0.c    | 3 +--
>>>>>>>      drivers/usb/dwc3/gadget.c | 5 ++++-
>>>>>>>      2 files changed, 5 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>>>>> index 506ef717fdc0..5851b0e9db0a 100644
>>>>>>> --- a/drivers/usb/dwc3/ep0.c
>>>>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>>>>> @@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>>>>>>>              if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
>>>>>>>                  continue;
>>>>>>>      -        dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>>>> -        dwc3_stop_active_transfer(dwc3_ep, true, true);
>>>>>>> +        dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
>>>>>>>          }
>>>>>>>      }
>>>>>>>      diff --git a/drivers/usb/dwc3/gadget.c
>>>>>>> b/drivers/usb/dwc3/gadget.c
>>>>>>> index bd40608b19df..fba2797ad9ae 100644
>>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>>> @@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct 
>>>>>>> dwc3_ep
>>>>>>> *dep, bool force,
>>>>>>>          if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
>>>>>>>              return;
>>>>>>>      +    if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
>>>>>>> +        return;
>>>>>>> +
>>>>>>>          if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>>>>>> -        (dep->flags & DWC3_EP_DELAY_STOP) ||
>>>>>>>              (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>>>>>>              return;
>>>>>>>      @@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct
>>>>>>> dwc3_ep *dep, bool force,
>>>>>>>          __dwc3_stop_active_transfer(dep, force, interrupt);
>>>>>>>          spin_lock(&dwc->lock);
>>>>>>>      +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>>>
>>>>>> Can we clear this flag in __dwc3_stop_active_transfer(). It should
>>>>>> apply
>>>>>> if End Transfer command was sent.
>>>>>
>>>>> I wanted to make sure that we weren't modifying the DEP flags outside
>>>>> of a spin lock.  Patch#3 modifies it where we unlock before calling
>>>>> __dwc3_stop_active_transfer(), so we can allow the dwc3 threaded IRQ
>>>>> handle events while the cmd status polling happens.
>>>>>
>>>>> Maybe we can unlock/lock the dwc3->lock inside
>>>>> __dwc3_stop_active_transfer() and that way we can ensure DEP flags 
>>>>> are
>>>>> modified properly?
>>>>
>>>> I didn't realize that you unlock/lock when calling
>>>> __dwc3_stop_active_transfer(). We'd need to be careful if we want to
>>>> unlock/lock it, and avoid it all together if possible. It can be 
>>>> easily
>>>> overlooked just like dwc3_gadget_giveback().
>>>>
>>>> What issue did you see without doing this?
>>>
>>> I saw endxfer timeout issues if I didn't do it.  If we keep the lock
>>> held, then the DWC3 event processing would be blocked across the
>>> entire time we are waiting for the command act to clear.  With
>>> unlocking before polling, then at least we're still able to handle the
>>> EP0 events that are pending.
>>>
>>> It was definitely one of the harder scenarios to reproduce. The main
>>> patch series which resolved a lot of the issues early on was patch#1.
>>> After adding that the other issues are seen maybe after a day or so of
>>> testing.
>>>
>>
>> I see. The soft-disconnect still go through right?
>> Can you try this and see if this works for you?
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 13e4f1a03417..4f67a484c490 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -451,7 +451,9 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep,
>> unsigned int cmd,
>>
>>           dwc3_writel(dep->regs, DWC3_DEPCMD, cmd);
>>
>> -       if (!(cmd & DWC3_DEPCMD_CMDACT)) {
>> +       if (!(cmd & DWC3_DEPCMD_CMDACT) ||
>> +           (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_ENDTRANSFER &&
>> +            !(cmd & DWC3_DEPCMD_CMDIOC))) {
>>                   ret = 0;
>>                   goto skip_status;
>>           }
>>
>>
>> We can set it and forget it when we're about to de-initialize the
>> controller. Just like what we've already done by not waiting for the End
>> Transfer endpoint command completion interrupt.
>>
>
> I can see this working for the soft disconnect case, but we'll need to 
> address it differently for the EP dequeue scenario where IOC/interrupt 
> is required to clean up the TRBs.  This is one of the reasons why I 
> went with the spinlock approach, since it would apply to both situations.

I thought we already handled the case for ep dequeue from the last fix. 
Was that not addressed?

Thanks,
Thinh