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

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

[Wesley Cheng]

Changes in v3:
- Modified the msleep() duration to ~2s versus ~10s due to the minimum
mdelay() value.
- Removed patch to modify DEP flags during dwc3_stop_active_transfer().
This was not required after fixing the logic to allow EP xfercomplete
events to be handled on EP0.
- Added some changes to account for a cable disconnect scenario, where
dwc3_gadget_pullup() would not be executed to stop active transfers.
Needed to add some logic to the disconnect interrupt to ensure that we
cleanup/restart any pending SETUP transaction, so that we can clear the
EP0 delayed stop status. (if pending)
- Added patch to ensure that we don't proceed with umapping buffers
until the endxfer was actually sent.

Changes in v2:
- Moved msleep() to before reading status register for halted state
- Fixed kernel bot errors
- Clearing DEP flags in __dwc3_stop_active_transfers()
- Added Suggested-by tags and link references to previous discussions

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
Since EP0 events can occur during controller halt, it may increase the time
needed for the controller to fully stop.

#5
Account for cable disconnect scenarios where nothing may cause the endxfer
retry if DWC3_EP_DELAY_STOP is set.

#6
Avoid unmapping pending USB requests that were never stopped.  This would
lead to a potential SMMU fault.

Wesley Cheng (8):
  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: Synchronize IRQ between soft connect/disconnect
  usb: dwc3: gadget: Continue handling EP0 xfercomplete events
  usb: dwc3: Avoid unmapping USB requests if endxfer is not complete
  usb: dwc3: Increase DWC3 controller halt timeout
  usb: dwc3: gadget: Skip waiting for CMDACT cleared during endxfer
  usb: dwc3: gadget: Submit endxfer command if delayed during disconnect

 drivers/usb/dwc3/core.c   |  4 ----
 drivers/usb/dwc3/core.h   |  3 +++
 drivers/usb/dwc3/ep0.c    | 11 ++++++---
 drivers/usb/dwc3/gadget.c | 48 +++++++++++++++++++++++++++++++++------
 4 files changed, 52 insertions(+), 14 deletions(-)

[PATCH v3 1/8] 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 197af63f8d05..33cee0089609 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;
@@ -815,7 +815,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);
@@ -1118,6 +1118,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 aeeec751c53c..2818e3317064 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3865,6 +3865,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 v3 2/8] 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.

In addition, allow the DWC3_EP_DELAY_STOP to be set for if there is a
delayed status requested.  Ocasionally, a host may abort the current SETUP
transaction, by issuing a subsequent SETUP token.  In those situations, it
would result in an endxfer timeout as well.

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

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 2818e3317064..391e40c35747 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2501,6 +2501,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);
@@ -3693,7 +3696,7 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	 * timeout. Delay issuing the End Transfer command until the Setup TRB is
 	 * prepared.
 	 */
-	if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status) {
+	if (dwc->ep0state != EP0_SETUP_PHASE) {
 		dep->flags |= DWC3_EP_DELAY_STOP;
 		return;
 	}

[PATCH v3 3/8] usb: dwc3: gadget: Synchronize IRQ between soft connect/disconnect

[Wesley Cheng]

Ensure that there are no pending events being handled in between soft
connect/disconnect transitions.  As we are keeping interrupts enabled,
and EP0 events are still being serviced, this avoids any stale events from
being serviced.

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

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 391e40c35747..3b83e3c4b932 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2569,6 +2569,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 {

[PATCH v3 4/8] usb: dwc3: gadget: Continue handling EP0 xfercomplete events

[Wesley Cheng]

During soft disconnect, EP0 events are expected to be handled in order to
allow the controller to successfully move into the halted state.  Since
__dwc3_gadget_stop() is executed before polling, EP0 has been disabled, and
events are being blocked.  Allow xfercomplete events to be handled, so that
cached SETUP packets can be read out from the internal controller memory.

Without doing so, it will lead to endxfer timeouts, which results to
controller halt failures.

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

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 3b83e3c4b932..aff288b7baeb 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2721,6 +2721,7 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
 	dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);
 
 	dep = dwc->eps[0];
+	dep->flags = 0;
 	ret = __dwc3_gadget_ep_enable(dep, DWC3_DEPCFG_ACTION_INIT);
 	if (ret) {
 		dev_err(dwc->dev, "failed to enable %s\n", dep->name);
@@ -2728,6 +2729,7 @@ static int __dwc3_gadget_start(struct dwc3 *dwc)
 	}
 
 	dep = dwc->eps[1];
+	dep->flags = 0;
 	ret = __dwc3_gadget_ep_enable(dep, DWC3_DEPCFG_ACTION_INIT);
 	if (ret) {
 		dev_err(dwc->dev, "failed to enable %s\n", dep->name);
@@ -3599,11 +3601,12 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 	dep = dwc->eps[epnum];
 
 	if (!(dep->flags & DWC3_EP_ENABLED)) {
-		if (!(dep->flags & DWC3_EP_TRANSFER_STARTED))
+		if ((epnum > 1) && !(dep->flags & DWC3_EP_TRANSFER_STARTED))
 			return;
 
 		/* Handle only EPCMDCMPLT when EP disabled */
-		if (event->endpoint_event != DWC3_DEPEVT_EPCMDCMPLT)
+		if ((event->endpoint_event != DWC3_DEPEVT_EPCMDCMPLT) &&
+			!(epnum <= 1 && event->endpoint_event == DWC3_DEPEVT_XFERCOMPLETE))
 			return;
 	}
 

[PATCH v3 5/8] usb: dwc3: Avoid unmapping USB requests if endxfer is not complete

[Wesley Cheng]

If DWC3_EP_DELAYED_STOP is set during stop active transfers, then do not
continue attempting to unmap request buffers during dwc3_remove_requests().
This can lead to SMMU faults, as the controller has not stopped the
processing of the TRB.  Defer this sequence to the EP0 out start, which
ensures that there are no pending SETUP transactions before issuing the
endxfer.

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

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 4fe4287dc934..7327e5767df9 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1560,6 +1560,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
 int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
 		u32 param);
 void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc);
+void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep);
 #else
 static inline int dwc3_gadget_init(struct dwc3 *dwc)
 { return 0; }
@@ -1581,6 +1582,8 @@ static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc,
 { return 0; }
 static inline void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
 { }
+static inline void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep)
+{ }
 #endif
 
 #if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 33cee0089609..fbb154a5ee1f 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -293,7 +293,10 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
 			continue;
 
 		dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
-		dwc3_stop_active_transfer(dwc3_ep, true, true);
+		if (dwc->connected)
+			dwc3_stop_active_transfer(dwc3_ep, true, true);
+		else
+			dwc3_remove_requests(dwc, dwc3_ep);
 	}
 }
 
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index aff288b7baeb..7b66a54250a0 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -965,12 +965,16 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep, unsigned int action)
 	return 0;
 }
 
-static void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep)
+void dwc3_remove_requests(struct dwc3 *dwc, struct dwc3_ep *dep)
 {
 	struct dwc3_request		*req;
 
 	dwc3_stop_active_transfer(dep, true, false);
 
+	/* If endxfer is delayed, avoid unmapping requests */
+	if (dep->flags & DWC3_EP_DELAY_STOP)
+		return;
+
 	/* - giveback all requests to gadget driver */
 	while (!list_empty(&dep->started_list)) {
 		req = next_request(&dep->started_list);

[PATCH v3 6/8] usb: dwc3: 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.  Remove the need
for making dwc3_gadget_suspend() and dwc3_gadget_resume() to be called in
a spinlock.

Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 drivers/usb/dwc3/core.c   | 4 ----
 drivers/usb/dwc3/gadget.c | 8 +++++++-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index c5c238ab3083..23e123a1ab5f 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1976,9 +1976,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 	case DWC3_GCTL_PRTCAP_DEVICE:
 		if (pm_runtime_suspended(dwc->dev))
 			break;
-		spin_lock_irqsave(&dwc->lock, flags);
 		dwc3_gadget_suspend(dwc);
-		spin_unlock_irqrestore(&dwc->lock, flags);
 		synchronize_irq(dwc->irq_gadget);
 		dwc3_core_exit(dwc);
 		break;
@@ -2039,9 +2037,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 			return ret;
 
 		dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE);
-		spin_lock_irqsave(&dwc->lock, flags);
 		dwc3_gadget_resume(dwc);
-		spin_unlock_irqrestore(&dwc->lock, flags);
 		break;
 	case DWC3_GCTL_PRTCAP_HOST:
 		if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) {
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 7b66a54250a0..b2668a83cc29 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2444,7 +2444,7 @@ static void __dwc3_gadget_set_speed(struct dwc3 *dwc)
 static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
 {
 	u32			reg;
-	u32			timeout = 500;
+	u32			timeout = 100;
 
 	if (pm_runtime_suspended(dwc->dev))
 		return 0;
@@ -2477,6 +2477,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
 	dwc3_gadget_dctl_write_safe(dwc, reg);
 
 	do {
+		msleep(20);
 		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
 		reg &= DWC3_DSTS_DEVCTRLHLT;
 	} while (--timeout && !(!is_on ^ !reg));
@@ -4520,12 +4521,17 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
 
 int dwc3_gadget_suspend(struct dwc3 *dwc)
 {
+	unsigned long flags;
+
 	if (!dwc->gadget_driver)
 		return 0;
 
 	dwc3_gadget_run_stop(dwc, false, false);
+
+	spin_lock_irqsave(&dwc->lock, flags);
 	dwc3_disconnect_gadget(dwc);
 	__dwc3_gadget_stop(dwc);
+	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	return 0;
 }

[PATCH v3 7/8] usb: dwc3: gadget: Skip waiting for CMDACT cleared during endxfer

[Wesley Cheng]

For endxfer commands that do not require an endpoint complete interrupt,
avoid having to wait for the command active bit to clear.  This allows for
EP0 events to continue to be handled, which allows for the controller to
complete it.  Otherwise, it is known that the endxfer command will fail if
there is a pending SETUP token that needs to be read.

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

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index b2668a83cc29..5e8d3f02f99c 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -366,7 +366,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;
 	}

[PATCH v3 8/8] usb: dwc3: gadget: Submit endxfer command if delayed during disconnect

[Wesley Cheng]

During a cable disconnect sequence, if ep0state is not in the SETUP phase,
then nothing will trigger any pending end transfer commands.  Force
stopping of any pending SETUP transaction, and move back to the SETUP
phase.

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

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 5e8d3f02f99c..757ecb04b55a 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3776,13 +3776,24 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
 	reg &= ~DWC3_DCTL_INITU2ENA;
 	dwc3_gadget_dctl_write_safe(dwc, reg);
 
+	dwc->connected = false;
+
 	dwc3_disconnect_gadget(dwc);
 
 	dwc->gadget->speed = USB_SPEED_UNKNOWN;
 	dwc->setup_packet_pending = false;
 	usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
 
-	dwc->connected = false;
+	if (dwc->ep0state != EP0_SETUP_PHASE) {
+		unsigned int    dir;
+
+		dir = !!dwc->ep0_expect_in;
+		if (dwc->ep0state == EP0_DATA_PHASE)
+			dwc3_ep0_end_control_data(dwc, dwc->eps[dir]);
+		else
+			dwc3_ep0_end_control_data(dwc, dwc->eps[!dir]);
+		dwc3_ep0_stall_and_restart(dwc);
+	}
 }
 
 static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)

Re: [PATCH v3 6/8] usb: dwc3: Increase DWC3 controller halt timeout

[Thinh Nguyen]

On 8/15/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.  Remove the need
> for making dwc3_gadget_suspend() and dwc3_gadget_resume() to be called in
> a spinlock.

Sounds like the removal of the spin_lock and the increase of halt
timeout are 2 separate change. It would be nice to separate this patch
since the $subject only indicates 1 of them.

> 
> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>  drivers/usb/dwc3/core.c   | 4 ----
>  drivers/usb/dwc3/gadget.c | 8 +++++++-
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index c5c238ab3083..23e123a1ab5f 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1976,9 +1976,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  	case DWC3_GCTL_PRTCAP_DEVICE:
>  		if (pm_runtime_suspended(dwc->dev))
>  			break;
> -		spin_lock_irqsave(&dwc->lock, flags);
>  		dwc3_gadget_suspend(dwc);
> -		spin_unlock_irqrestore(&dwc->lock, flags);
>  		synchronize_irq(dwc->irq_gadget);
>  		dwc3_core_exit(dwc);
>  		break;
> @@ -2039,9 +2037,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>  			return ret;
>  
>  		dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> -		spin_lock_irqsave(&dwc->lock, flags);
>  		dwc3_gadget_resume(dwc);
> -		spin_unlock_irqrestore(&dwc->lock, flags);
>  		break;
>  	case DWC3_GCTL_PRTCAP_HOST:
>  		if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) {
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 7b66a54250a0..b2668a83cc29 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2444,7 +2444,7 @@ static void __dwc3_gadget_set_speed(struct dwc3 *dwc)
>  static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>  {
>  	u32			reg;
> -	u32			timeout = 500;
> +	u32			timeout = 100;
>  
>  	if (pm_runtime_suspended(dwc->dev))
>  		return 0;
> @@ -2477,6 +2477,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>  	dwc3_gadget_dctl_write_safe(dwc, reg);
>  
>  	do {
> +		msleep(20);

Polling interval of 20ms seems a bit much. Can we use usleep_range()
between 1-2ms?

Thanks,
Thinh

>  		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
>  		reg &= DWC3_DSTS_DEVCTRLHLT;
>  	} while (--timeout && !(!is_on ^ !reg));
> @@ -4520,12 +4521,17 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
>  
>  int dwc3_gadget_suspend(struct dwc3 *dwc)
>  {
> +	unsigned long flags;
> +
>  	if (!dwc->gadget_driver)
>  		return 0;
>  
>  	dwc3_gadget_run_stop(dwc, false, false);
> +
> +	spin_lock_irqsave(&dwc->lock, flags);
>  	dwc3_disconnect_gadget(dwc);
>  	__dwc3_gadget_stop(dwc);
> +	spin_unlock_irqrestore(&dwc->lock, flags);
>  
>  	return 0;
>  }

Re: [PATCH v3 0/8] Fix controller halt and endxfer timeout issues

[Thinh Nguyen]

Hi Wesley,

On 8/15/2022, Wesley Cheng wrote:
> Changes in v3:
> - Modified the msleep() duration to ~2s versus ~10s due to the minimum
> mdelay() value.
> - Removed patch to modify DEP flags during dwc3_stop_active_transfer().
> This was not required after fixing the logic to allow EP xfercomplete
> events to be handled on EP0.
> - Added some changes to account for a cable disconnect scenario, where
> dwc3_gadget_pullup() would not be executed to stop active transfers.
> Needed to add some logic to the disconnect interrupt to ensure that we
> cleanup/restart any pending SETUP transaction, so that we can clear the
> EP0 delayed stop status. (if pending)
> - Added patch to ensure that we don't proceed with umapping buffers
> until the endxfer was actually sent.
> 
> Changes in v2:
> - Moved msleep() to before reading status register for halted state
> - Fixed kernel bot errors
> - Clearing DEP flags in __dwc3_stop_active_transfers()
> - Added Suggested-by tags and link references to previous discussions
> 
> 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
> Since EP0 events can occur during controller halt, it may increase the time
> needed for the controller to fully stop.
> 
> #5
> Account for cable disconnect scenarios where nothing may cause the endxfer
> retry if DWC3_EP_DELAY_STOP is set.
> 
> #6
> Avoid unmapping pending USB requests that were never stopped.  This would
> lead to a potential SMMU fault.
> 
> Wesley Cheng (8):
>   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: Synchronize IRQ between soft connect/disconnect
>   usb: dwc3: gadget: Continue handling EP0 xfercomplete events
>   usb: dwc3: Avoid unmapping USB requests if endxfer is not complete
>   usb: dwc3: Increase DWC3 controller halt timeout
>   usb: dwc3: gadget: Skip waiting for CMDACT cleared during endxfer
>   usb: dwc3: gadget: Submit endxfer command if delayed during disconnect
> 
>  drivers/usb/dwc3/core.c   |  4 ----
>  drivers/usb/dwc3/core.h   |  3 +++
>  drivers/usb/dwc3/ep0.c    | 11 ++++++---
>  drivers/usb/dwc3/gadget.c | 48 +++++++++++++++++++++++++++++++++------
>  4 files changed, 52 insertions(+), 14 deletions(-)
> 

Beside the comment on [patch 6/8] increasing halt timeout, the rest
looks fine to me.

Reviewed-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks for the patches!
Thinh

Re: [PATCH v3 6/8] usb: dwc3: Increase DWC3 controller halt timeout

[Wesley Cheng]

Hi Thinh,

On 8/16/2022 5:34 PM, Thinh Nguyen wrote:
> On 8/15/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.  Remove the need
>> for making dwc3_gadget_suspend() and dwc3_gadget_resume() to be called in
>> a spinlock.
> 
> Sounds like the removal of the spin_lock and the increase of halt
> timeout are 2 separate change. It would be nice to separate this patch
> since the $subject only indicates 1 of them.
> 

Sure, I'll split these up.

>>
>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>   drivers/usb/dwc3/core.c   | 4 ----
>>   drivers/usb/dwc3/gadget.c | 8 +++++++-
>>   2 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index c5c238ab3083..23e123a1ab5f 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1976,9 +1976,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>>   	case DWC3_GCTL_PRTCAP_DEVICE:
>>   		if (pm_runtime_suspended(dwc->dev))
>>   			break;
>> -		spin_lock_irqsave(&dwc->lock, flags);
>>   		dwc3_gadget_suspend(dwc);
>> -		spin_unlock_irqrestore(&dwc->lock, flags);
>>   		synchronize_irq(dwc->irq_gadget);
>>   		dwc3_core_exit(dwc);
>>   		break;
>> @@ -2039,9 +2037,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>   			return ret;
>>   
>>   		dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>> -		spin_lock_irqsave(&dwc->lock, flags);
>>   		dwc3_gadget_resume(dwc);
>> -		spin_unlock_irqrestore(&dwc->lock, flags);
>>   		break;
>>   	case DWC3_GCTL_PRTCAP_HOST:
>>   		if (!PMSG_IS_AUTO(msg) && !device_can_wakeup(dwc->dev)) {
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 7b66a54250a0..b2668a83cc29 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2444,7 +2444,7 @@ static void __dwc3_gadget_set_speed(struct dwc3 *dwc)
>>   static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>>   {
>>   	u32			reg;
>> -	u32			timeout = 500;
>> +	u32			timeout = 100;
>>   
>>   	if (pm_runtime_suspended(dwc->dev))
>>   		return 0;
>> @@ -2477,6 +2477,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>>   	dwc3_gadget_dctl_write_safe(dwc, reg);
>>   
>>   	do {
>> +		msleep(20);
> 
> Polling interval of 20ms seems a bit much. Can we use usleep_range()
> between 1-2ms?
> 
> Thanks,
> Thinh

Sounds good. Let me reduce the interval gap and resubmit.

Thanks
Wesley Cheng

> 
>>   		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
>>   		reg &= DWC3_DSTS_DEVCTRLHLT;
>>   	} while (--timeout && !(!is_on ^ !reg));
>> @@ -4520,12 +4521,17 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
>>   
>>   int dwc3_gadget_suspend(struct dwc3 *dwc)
>>   {
>> +	unsigned long flags;
>> +
>>   	if (!dwc->gadget_driver)
>>   		return 0;
>>   
>>   	dwc3_gadget_run_stop(dwc, false, false);
>> +
>> +	spin_lock_irqsave(&dwc->lock, flags);
>>   	dwc3_disconnect_gadget(dwc);
>>   	__dwc3_gadget_stop(dwc);
>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>>   
>>   	return 0;
>>   }
>