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

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

[Wesley Cheng]

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
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 | 33 +++++++++++++++++++++++----------
 2 files changed, 28 insertions(+), 14 deletions(-)

[PATCH v2 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.

Fixes: 861c010a2ee1 ("usb: dwc3: gadget: Refactor pullup()")
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 4366c45c28cf..d1d7a5e5bd7c 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 v2 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.

Fixes: 861c010a2ee1 ("usb: dwc3: gadget: Refactor pullup()")
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 d1d7a5e5bd7c..a455f8d4631d 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);

[PATCH v2 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.

Fixes: 861c010a2ee1 ("usb: dwc3: gadget: Refactor pullup()")
Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 drivers/usb/dwc3/gadget.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index a455f8d4631d..ee85b773e3fe 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1674,6 +1674,7 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
 static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt)
 {
 	struct dwc3_gadget_ep_cmd_params params;
+	struct dwc3 *dwc = dep->dwc;
 	u32 cmd;
 	int ret;
 
@@ -1682,7 +1683,9 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
 	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
 	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
 	memset(&params, 0, sizeof(params));
+	spin_unlock(&dwc->lock);
 	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
+	spin_lock(&dwc->lock);
 	WARN_ON_ONCE(ret);
 	dep->resource_index = 0;
 
@@ -2029,12 +2032,11 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 	struct dwc3_ep			*dep = to_dwc3_ep(ep);
 	struct dwc3			*dwc = dep->dwc;
 
-	unsigned long			flags;
 	int				ret = 0;
 
 	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)
@@ -2073,7 +2075,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;
 }
@@ -2489,9 +2491,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;
 
 	/*
@@ -2506,10 +2506,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");
 	}
@@ -2523,7 +2523,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
@@ -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 {
@@ -3729,6 +3731,7 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
 	 */
 
 	__dwc3_stop_active_transfer(dep, force, interrupt);
+
 }
 
 static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)

[PATCH v2 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.

Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to complete during dequeue")
Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
Link:
  https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.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 ee85b773e3fe..41b7007358de 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1693,6 +1693,7 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
 		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
 	else if (!ret)
 		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
+	dep->flags &= ~DWC3_EP_DELAY_STOP;
 
 	return ret;
 }
@@ -3686,8 +3687,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;
 

[PATCH v2 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.

Fixes: 861c010a2ee1 ("usb: dwc3: gadget: Refactor pullup()")
Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
Link:
  https://lore.kernel.org/linux-usb/4988ed34-04a4-060a-ccef-f57790f76a2b@synopsys.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 41b7007358de..e32d7293c447 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2476,6 +2476,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
 	dwc3_gadget_dctl_write_safe(dwc, reg);
 
 	do {
+		msleep(1);
 		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
 		reg &= DWC3_DSTS_DEVCTRLHLT;
 	} while (--timeout && !(!is_on ^ !reg));

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

[Jack Pham]

Hi Wesley,

On Tue, Jul 12, 2022 at 05:35:23PM -0700, 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.
> 
> Fixes: 861c010a2ee1 ("usb: dwc3: gadget: Refactor pullup()")
> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
> Link:
>   https://lore.kernel.org/linux-usb/4988ed34-04a4-060a-ccef-f57790f76a2b@synopsys.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 41b7007358de..e32d7293c447 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2476,6 +2476,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>  	dwc3_gadget_dctl_write_safe(dwc, reg);
>  
>  	do {
> +		msleep(1);

Be aware that this probably won't sleep for *just* 1ms.  From
Documentation/timers/timers-howto.rst:

	msleep(1~20) may not do what the caller intends, and
	will often sleep longer (~20 ms actual sleep for any
	value given in the 1~20ms range). In many cases this
	is not the desired behavior.

So with timeout==500 this loop could very well end up iterating for up
to 10 seconds.  Granted this shouldn't be called from any atomic context
but just wanted to make sure that the effective increase in timeout as
$SUBJECT intends is made clear here and that it's not overly generous.

>  		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
>  		reg &= DWC3_DSTS_DEVCTRLHLT;
>  	} while (--timeout && !(!is_on ^ !reg));

Jack

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

[John Keeping]

On Tue, Jul 12, 2022 at 07:56:43PM -0700, Jack Pham wrote:
> Hi Wesley,
> 
> On Tue, Jul 12, 2022 at 05:35:23PM -0700, 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.
> > 
> > Fixes: 861c010a2ee1 ("usb: dwc3: gadget: Refactor pullup()")
> > Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> > ---
> > Link:
> >   https://lore.kernel.org/linux-usb/4988ed34-04a4-060a-ccef-f57790f76a2b@synopsys.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 41b7007358de..e32d7293c447 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -2476,6 +2476,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
> >  	dwc3_gadget_dctl_write_safe(dwc, reg);
> >  
> >  	do {
> > +		msleep(1);
> 
> Be aware that this probably won't sleep for *just* 1ms.  From
> Documentation/timers/timers-howto.rst:
> 
> 	msleep(1~20) may not do what the caller intends, and
> 	will often sleep longer (~20 ms actual sleep for any
> 	value given in the 1~20ms range). In many cases this
> 	is not the desired behavior.
> 
> So with timeout==500 this loop could very well end up iterating for up
> to 10 seconds.  Granted this shouldn't be called from any atomic context
> but just wanted to make sure that the effective increase in timeout as
> $SUBJECT intends is made clear here and that it's not overly generous.
> 
> >  		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> >  		reg &= DWC3_DSTS_DEVCTRLHLT;
> >  	} while (--timeout && !(!is_on ^ !reg));

Does it make sense to convert this loop to use read_poll_timeout() and
make the timeout explicit, something like:

	ret = read_poll_timeout(dwc3_readl, reg, !(!is_on ^ !(reg & DWC3_DSTS_DEVCTRLHLT)),
				100, timeout * USEC_PER_MSEC, true, dwc->regs, DWC3_DSTS);

?

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

[Jack Pham]

On Wed, Jul 13, 2022 at 12:40:53PM +0100, John Keeping wrote:
> On Tue, Jul 12, 2022 at 07:56:43PM -0700, Jack Pham wrote:
> > Hi Wesley,
> > 
> > On Tue, Jul 12, 2022 at 05:35:23PM -0700, 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.
> > > 
> > > Fixes: 861c010a2ee1 ("usb: dwc3: gadget: Refactor pullup()")
> > > Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> > > ---
> > > Link:
> > >   https://lore.kernel.org/linux-usb/4988ed34-04a4-060a-ccef-f57790f76a2b@synopsys.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 41b7007358de..e32d7293c447 100644
> > > --- a/drivers/usb/dwc3/gadget.c
> > > +++ b/drivers/usb/dwc3/gadget.c
> > > @@ -2476,6 +2476,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
> > >  	dwc3_gadget_dctl_write_safe(dwc, reg);
> > >  
> > >  	do {
> > > +		msleep(1);
> > 
> > Be aware that this probably won't sleep for *just* 1ms.  From
> > Documentation/timers/timers-howto.rst:
> > 
> > 	msleep(1~20) may not do what the caller intends, and
> > 	will often sleep longer (~20 ms actual sleep for any
> > 	value given in the 1~20ms range). In many cases this
> > 	is not the desired behavior.
> > 
> > So with timeout==500 this loop could very well end up iterating for up
> > to 10 seconds.  Granted this shouldn't be called from any atomic context
> > but just wanted to make sure that the effective increase in timeout as
> > $SUBJECT intends is made clear here and that it's not overly generous.
> > 
> > >  		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> > >  		reg &= DWC3_DSTS_DEVCTRLHLT;
> > >  	} while (--timeout && !(!is_on ^ !reg));
> 
> Does it make sense to convert this loop to use read_poll_timeout() and
> make the timeout explicit, something like:
> 
> 	ret = read_poll_timeout(dwc3_readl, reg, !(!is_on ^ !(reg & DWC3_DSTS_DEVCTRLHLT)),
> 				100, timeout * USEC_PER_MSEC, true, dwc->regs, DWC3_DSTS);
> 
> ?

Yeah I think it would make sense.  Might even be worthwhile to revisit
similar loops being performed in dwc3_send_gadget_generic_command() and
dwc3_send_gadget_ep_cmd() which are currently spinning delay-lessly for a
fixed number of iterations.

Jack

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

[Thinh Nguyen]

On 7/13/2022, Jack Pham wrote:
> On Wed, Jul 13, 2022 at 12:40:53PM +0100, John Keeping wrote:
>> On Tue, Jul 12, 2022 at 07:56:43PM -0700, Jack Pham wrote:
>>> Hi Wesley,
>>>
>>> On Tue, Jul 12, 2022 at 05:35:23PM -0700, 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.
>>>>
>>>> Fixes: 861c010a2ee1 ("usb: dwc3: gadget: Refactor pullup()")
>>>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>> ---
>>>> Link:
>>>>    https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/4988ed34-04a4-060a-ccef-f57790f76a2b@synopsys.com/__;!!A4F2R9G_pg!eidgBYKrTCOm9XSLhpRDscGcM5pkmRIG-XDwBbOYmdcEWUM2MhWJLeeJHhTm8TPNNs9hOgaK1yT8W-0zeZ51Pip-VA$
>>>>
>>>>   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 41b7007358de..e32d7293c447 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -2476,6 +2476,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>>>>   	dwc3_gadget_dctl_write_safe(dwc, reg);
>>>>   
>>>>   	do {
>>>> +		msleep(1);
>>> Be aware that this probably won't sleep for *just* 1ms.  From
>>> Documentation/timers/timers-howto.rst:
>>>
>>> 	msleep(1~20) may not do what the caller intends, and
>>> 	will often sleep longer (~20 ms actual sleep for any
>>> 	value given in the 1~20ms range). In many cases this
>>> 	is not the desired behavior.
>>>
>>> So with timeout==500 this loop could very well end up iterating for up
>>> to 10 seconds.  Granted this shouldn't be called from any atomic context
>>> but just wanted to make sure that the effective increase in timeout as
>>> $SUBJECT intends is made clear here and that it's not overly generous.
>>>
>>>>   		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
>>>>   		reg &= DWC3_DSTS_DEVCTRLHLT;
>>>>   	} while (--timeout && !(!is_on ^ !reg));
>> Does it make sense to convert this loop to use read_poll_timeout() and
>> make the timeout explicit, something like:
>>
>> 	ret = read_poll_timeout(dwc3_readl, reg, !(!is_on ^ !(reg & DWC3_DSTS_DEVCTRLHLT)),
>> 				100, timeout * USEC_PER_MSEC, true, dwc->regs, DWC3_DSTS);
>>
>> ?
> Yeah I think it would make sense.  Might even be worthwhile to revisit
> similar loops being performed in dwc3_send_gadget_generic_command() and
> dwc3_send_gadget_ep_cmd() which are currently spinning delay-lessly for a
> fixed number of iterations.
>

++ Jun

BTW, Jun started on this awhile ago. You can review his patch for 
reference if anyone wants to take on this:

https://patchwork.kernel.org/project/linux-usb/patch/1588928985-1585-1-git-send-email-jun.li@nxp.com/

Thanks,
Thinh

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

[Thinh Nguyen]

On 7/12/2022, Wesley Cheng wrote:
> 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.
>
> Fixes: 861c010a2ee1 ("usb: dwc3: gadget: Refactor pullup()")
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>   drivers/usb/dwc3/gadget.c | 21 ++++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index a455f8d4631d..ee85b773e3fe 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1674,6 +1674,7 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>   static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt)
>   {
>   	struct dwc3_gadget_ep_cmd_params params;
> +	struct dwc3 *dwc = dep->dwc;
>   	u32 cmd;
>   	int ret;
>   
> @@ -1682,7 +1683,9 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
>   	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
>   	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
>   	memset(&params, 0, sizeof(params));
> +	spin_unlock(&dwc->lock);
>   	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
> +	spin_lock(&dwc->lock);
>   	WARN_ON_ONCE(ret);
>   	dep->resource_index = 0;
>   
> @@ -2029,12 +2032,11 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>   	struct dwc3_ep			*dep = to_dwc3_ep(ep);
>   	struct dwc3			*dwc = dep->dwc;
>   
> -	unsigned long			flags;
>   	int				ret = 0;
>   
>   	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)
> @@ -2073,7 +2075,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;
>   }
> @@ -2489,9 +2491,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;
>   
>   	/*
> @@ -2506,10 +2506,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");
>   	}
> @@ -2523,7 +2523,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
> @@ -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 {
> @@ -3729,6 +3731,7 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>   	 */
>   
>   	__dwc3_stop_active_transfer(dep, force, interrupt);
> +
>   }
>   
>   static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)

Hi Greg,

Please don't pick up this patch yet. We're still in discussion with 
this. I have some concern with unlocking/locking when sending End 
Transfer command. For example, this patch may cause issues with 
DWC3_EP_END_TRANSFER_PENDING checks.

Hi Wesley,

Did you try out my suggestion yet?

Thanks,
Thinh

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

[Wesley Cheng]

Hi Thinh,

On 7/14/2022 10:38 AM, Thinh Nguyen wrote:
> On 7/12/2022, Wesley Cheng wrote:
>> 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.
>>
>> Fixes: 861c010a2ee1 ("usb: dwc3: gadget: Refactor pullup()")
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>    drivers/usb/dwc3/gadget.c | 21 ++++++++++++---------
>>    1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index a455f8d4631d..ee85b773e3fe 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1674,6 +1674,7 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>>    static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt)
>>    {
>>    	struct dwc3_gadget_ep_cmd_params params;
>> +	struct dwc3 *dwc = dep->dwc;
>>    	u32 cmd;
>>    	int ret;
>>    
>> @@ -1682,7 +1683,9 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
>>    	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
>>    	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
>>    	memset(&params, 0, sizeof(params));
>> +	spin_unlock(&dwc->lock);
>>    	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
>> +	spin_lock(&dwc->lock);
>>    	WARN_ON_ONCE(ret);
>>    	dep->resource_index = 0;
>>    
>> @@ -2029,12 +2032,11 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>>    	struct dwc3_ep			*dep = to_dwc3_ep(ep);
>>    	struct dwc3			*dwc = dep->dwc;
>>    
>> -	unsigned long			flags;
>>    	int				ret = 0;
>>    
>>    	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)
>> @@ -2073,7 +2075,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;
>>    }
>> @@ -2489,9 +2491,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;
>>    
>>    	/*
>> @@ -2506,10 +2506,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");
>>    	}
>> @@ -2523,7 +2523,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
>> @@ -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 {
>> @@ -3729,6 +3731,7 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>>    	 */
>>    
>>    	__dwc3_stop_active_transfer(dep, force, interrupt);
>> +
>>    }
>>    
>>    static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
> 
> Hi Greg,
> 
> Please don't pick up this patch yet. We're still in discussion with
> this. I have some concern with unlocking/locking when sending End
> Transfer command. For example, this patch may cause issues with
> DWC3_EP_END_TRANSFER_PENDING checks.
> 

Agreed.

> Hi Wesley,
> 
> Did you try out my suggestion yet?
> 

In process of testing.  Will update you in a few days, since it might 
take a day or so to reproduce.

Thanks
Wesley Cheng

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

[gregkh]

On Thu, Jul 14, 2022 at 10:41:01AM -0700, Wesley Cheng wrote:
> Hi Thinh,
> 
> On 7/14/2022 10:38 AM, Thinh Nguyen wrote:
> > On 7/12/2022, Wesley Cheng wrote:
> > > 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.
> > > 
> > > Fixes: 861c010a2ee1 ("usb: dwc3: gadget: Refactor pullup()")
> > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> > > ---
> > >    drivers/usb/dwc3/gadget.c | 21 ++++++++++++---------
> > >    1 file changed, 12 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > > index a455f8d4631d..ee85b773e3fe 100644
> > > --- a/drivers/usb/dwc3/gadget.c
> > > +++ b/drivers/usb/dwc3/gadget.c
> > > @@ -1674,6 +1674,7 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
> > >    static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt)
> > >    {
> > >    	struct dwc3_gadget_ep_cmd_params params;
> > > +	struct dwc3 *dwc = dep->dwc;
> > >    	u32 cmd;
> > >    	int ret;
> > > @@ -1682,7 +1683,9 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
> > >    	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
> > >    	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
> > >    	memset(&params, 0, sizeof(params));
> > > +	spin_unlock(&dwc->lock);
> > >    	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
> > > +	spin_lock(&dwc->lock);
> > >    	WARN_ON_ONCE(ret);
> > >    	dep->resource_index = 0;
> > > @@ -2029,12 +2032,11 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
> > >    	struct dwc3_ep			*dep = to_dwc3_ep(ep);
> > >    	struct dwc3			*dwc = dep->dwc;
> > > -	unsigned long			flags;
> > >    	int				ret = 0;
> > >    	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)
> > > @@ -2073,7 +2075,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;
> > >    }
> > > @@ -2489,9 +2491,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;
> > >    	/*
> > > @@ -2506,10 +2506,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");
> > >    	}
> > > @@ -2523,7 +2523,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
> > > @@ -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 {
> > > @@ -3729,6 +3731,7 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
> > >    	 */
> > >    	__dwc3_stop_active_transfer(dep, force, interrupt);
> > > +
> > >    }
> > >    static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
> > 
> > Hi Greg,
> > 
> > Please don't pick up this patch yet. We're still in discussion with
> > this. I have some concern with unlocking/locking when sending End
> > Transfer command. For example, this patch may cause issues with
> > DWC3_EP_END_TRANSFER_PENDING checks.
> > 
> 
> Agreed.
> 
> > Hi Wesley,
> > 
> > Did you try out my suggestion yet?
> > 
> 
> In process of testing.  Will update you in a few days, since it might take a
> day or so to reproduce.

Ok, I'll drop this whole series from my tree for now.  Please resend
when you have it working.

greg k-h

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

[Wesley Cheng]

Hi Thinh,

On 7/14/2022 10:38 AM, Thinh Nguyen wrote:
> On 7/12/2022, Wesley Cheng wrote:
>> 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.
>>
>> Fixes: 861c010a2ee1 ("usb: dwc3: gadget: Refactor pullup()")
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>    drivers/usb/dwc3/gadget.c | 21 ++++++++++++---------
>>    1 file changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index a455f8d4631d..ee85b773e3fe 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1674,6 +1674,7 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>>    static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt)
>>    {
>>    	struct dwc3_gadget_ep_cmd_params params;
>> +	struct dwc3 *dwc = dep->dwc;
>>    	u32 cmd;
>>    	int ret;
>>    
>> @@ -1682,7 +1683,9 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
>>    	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
>>    	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
>>    	memset(&params, 0, sizeof(params));
>> +	spin_unlock(&dwc->lock);
>>    	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
>> +	spin_lock(&dwc->lock);
>>    	WARN_ON_ONCE(ret);
>>    	dep->resource_index = 0;
>>    
>> @@ -2029,12 +2032,11 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>>    	struct dwc3_ep			*dep = to_dwc3_ep(ep);
>>    	struct dwc3			*dwc = dep->dwc;
>>    
>> -	unsigned long			flags;
>>    	int				ret = 0;
>>    
>>    	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)
>> @@ -2073,7 +2075,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;
>>    }
>> @@ -2489,9 +2491,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;
>>    
>>    	/*
>> @@ -2506,10 +2506,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");
>>    	}
>> @@ -2523,7 +2523,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
>> @@ -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 {
>> @@ -3729,6 +3731,7 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>>    	 */
>>    
>>    	__dwc3_stop_active_transfer(dep, force, interrupt);
>> +
>>    }
>>    
>>    static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
> 
> Hi Greg,
> 
> Please don't pick up this patch yet. We're still in discussion with
> this. I have some concern with unlocking/locking when sending End
> Transfer command. For example, this patch may cause issues with
> DWC3_EP_END_TRANSFER_PENDING checks.
> 
> Hi Wesley,
> 
> Did you try out my suggestion yet?
> 

Just providing a quick update.

So with your suggestion, I was able to consistently reproduce the 
controller halt issue after a day or so of testing.  However, when I 
took a further look, I believe the problem is due to the DWC3 event handler:

static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
		const struct dwc3_event_depevt *event)
{
...
	if (!(dep->flags & DWC3_EP_ENABLED)) {
		if (!(dep->flags & DWC3_EP_TRANSFER_STARTED))
			return;

		/* Handle only EPCMDCMPLT when EP disabled */
		if (event->endpoint_event != DWC3_DEPEVT_EPCMDCMPLT)
			return;
	}

The soft disconnect routine reached to the run/stop polling point, and I 
could see that DWC3_EP_DELAYED_STOP was set, and we got a xfercomplete 
event for the STATUS phase.  However, since we exit early in the event 
handler (due to __dwc3_gadget_stop() being called and disabling EP0), 
the STATUS complete is never handled, and we do not issue the endxfer 
command.

I don't think I saw this issue with my change, as we allowed the STATUS 
phase handling to happen BEFORE gadget stop was called (since I released 
the lock in the stop active transfers API).

However, I think even with my approach, we'd eventually run into a 
possibility of this issue, as we aren't truly handling EP0 events while 
polling for the halted status due to the above.  It was just reducing 
the chances.  The scenario of this issue is coming because the host took 
a long time to complete the STATUS phase, so we ran into a "timed out 
waiting for SETUP phase," which allowed us to call the run/stop routine 
while we were not yet in the SETUP phase.

I'm currently running a change to add a EP num check to this IF condition:

	if ((epnum > 1) && !(dep->flags & DWC3_EP_ENABLED)) {
		if (!(dep->flags & DWC3_EP_TRANSFER_STARTED))
			return;

		/* Handle only EPCMDCMPLT when EP disabled */
		if (event->endpoint_event != DWC3_DEPEVT_EPCMDCMPLT)
			return;
	}

Thanks
Wesley Cheng

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

[Thinh Nguyen]

Hi Wesley,

On 7/20/2022, Wesley Cheng wrote:
> Hi Thinh,
>
> On 7/14/2022 10:38 AM, Thinh Nguyen wrote:
>> On 7/12/2022, Wesley Cheng wrote:
>>> 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.
>>>
>>> Fixes: 861c010a2ee1 ("usb: dwc3: gadget: Refactor pullup()")
>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>> ---
>>>    drivers/usb/dwc3/gadget.c | 21 ++++++++++++---------
>>>    1 file changed, 12 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index a455f8d4631d..ee85b773e3fe 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -1674,6 +1674,7 @@ static int __dwc3_gadget_get_frame(struct dwc3 
>>> *dwc)
>>>    static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool 
>>> force, bool interrupt)
>>>    {
>>>        struct dwc3_gadget_ep_cmd_params params;
>>> +    struct dwc3 *dwc = dep->dwc;
>>>        u32 cmd;
>>>        int ret;
>>>    @@ -1682,7 +1683,9 @@ static int 
>>> __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
>>>        cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
>>>        cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
>>>        memset(&params, 0, sizeof(params));
>>> +    spin_unlock(&dwc->lock);
>>>        ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
>>> +    spin_lock(&dwc->lock);
>>>        WARN_ON_ONCE(ret);
>>>        dep->resource_index = 0;
>>>    @@ -2029,12 +2032,11 @@ static int dwc3_gadget_ep_dequeue(struct 
>>> usb_ep *ep,
>>>        struct dwc3_ep            *dep = to_dwc3_ep(ep);
>>>        struct dwc3            *dwc = dep->dwc;
>>>    -    unsigned long            flags;
>>>        int                ret = 0;
>>>           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)
>>> @@ -2073,7 +2075,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;
>>>    }
>>> @@ -2489,9 +2491,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;
>>>           /*
>>> @@ -2506,10 +2506,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");
>>>        }
>>> @@ -2523,7 +2523,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
>>> @@ -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 {
>>> @@ -3729,6 +3731,7 @@ void dwc3_stop_active_transfer(struct dwc3_ep 
>>> *dep, bool force,
>>>         */
>>>           __dwc3_stop_active_transfer(dep, force, interrupt);
>>> +
>>>    }
>>>       static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
>>
>> Hi Greg,
>>
>> Please don't pick up this patch yet. We're still in discussion with
>> this. I have some concern with unlocking/locking when sending End
>> Transfer command. For example, this patch may cause issues with
>> DWC3_EP_END_TRANSFER_PENDING checks.
>>
>> Hi Wesley,
>>
>> Did you try out my suggestion yet?
>>
>
> Just providing a quick update.
>
> So with your suggestion, I was able to consistently reproduce the 
> controller halt issue after a day or so of testing.  However, when I 
> took a further look, I believe the problem is due to the DWC3 event 
> handler:
>
> static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
>         const struct dwc3_event_depevt *event)
> {
> ...
>     if (!(dep->flags & DWC3_EP_ENABLED)) {
>         if (!(dep->flags & DWC3_EP_TRANSFER_STARTED))
>             return;
>
>         /* Handle only EPCMDCMPLT when EP disabled */
>         if (event->endpoint_event != DWC3_DEPEVT_EPCMDCMPLT)
>             return;
>     }
>
> The soft disconnect routine reached to the run/stop polling point, and 
> I could see that DWC3_EP_DELAYED_STOP was set, and we got a 
> xfercomplete event for the STATUS phase.  However, since we exit early 
> in the event handler (due to __dwc3_gadget_stop() being called and 
> disabling EP0), the STATUS complete is never handled, and we do not 
> issue the endxfer command.
>
> I don't think I saw this issue with my change, as we allowed the 
> STATUS phase handling to happen BEFORE gadget stop was called (since I 
> released the lock in the stop active transfers API).
>
> However, I think even with my approach, we'd eventually run into a 
> possibility of this issue, as we aren't truly handling EP0 events 
> while polling for the halted status due to the above.  It was just 
> reducing the chances.  The scenario of this issue is coming because 
> the host took a long time to complete the STATUS phase, so we ran into 
> a "timed out waiting for SETUP phase," which allowed us to call the 
> run/stop routine while we were not yet in the SETUP phase.
>
> I'm currently running a change to add a EP num check to this IF 
> condition:
>
>     if ((epnum > 1) && !(dep->flags & DWC3_EP_ENABLED)) {
>         if (!(dep->flags & DWC3_EP_TRANSFER_STARTED))
>             return;
>
>         /* Handle only EPCMDCMPLT when EP disabled */
>         if (event->endpoint_event != DWC3_DEPEVT_EPCMDCMPLT)
>             return;
>     }
>

Ah... good catch! Thanks for all the testings.

BR,
Thinh

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

[Thinh Nguyen]

Hi Wesley,

On 7/12/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.
>
> Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to complete during dequeue")
> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
> Link:
>    https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/__;!!A4F2R9G_pg!cXW2TlALYWnXNPg-NoMFUrQ8K1egEZiQizZ5CA1DOM1Gcw4tfOULy-_2eMGvL8pQPte5dScFON-0wxH2eXu8ijEQUbs$
>
>   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;

If we don't clear this flag here,

> -		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 ee85b773e3fe..41b7007358de 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1693,6 +1693,7 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
>   		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>   	else if (!ret)
>   		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
> +	dep->flags &= ~DWC3_EP_DELAY_STOP;
>   
>   	return ret;
>   }
> @@ -3686,8 +3687,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;
> +

then it may not go through here. I think I made this same mistake in my 
previous suggestion.

>   	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
> -	    (dep->flags & DWC3_EP_DELAY_STOP) ||
>   	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>   		return;
>   

Thanks,
Thinh

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

[Wesley Cheng]

Hi Thinh,

On 7/21/2022 3:08 PM, Thinh Nguyen wrote:
> Hi Wesley,
> 
> On 7/12/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.
>>
>> Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to complete during dequeue")
>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>> Link:
>>     https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/__;!!A4F2R9G_pg!cXW2TlALYWnXNPg-NoMFUrQ8K1egEZiQizZ5CA1DOM1Gcw4tfOULy-_2eMGvL8pQPte5dScFON-0wxH2eXu8ijEQUbs$
>>
>>    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;
> 
> If we don't clear this flag here,
> 

This is why I added the dwc->connected argument to control the 
"interrupt" argument.

>> -		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 ee85b773e3fe..41b7007358de 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1693,6 +1693,7 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
>>    		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>>    	else if (!ret)
>>    		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
>> +	dep->flags &= ~DWC3_EP_DELAY_STOP;
>>    
>>    	return ret;
>>    }
>> @@ -3686,8 +3687,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;
>> +
> 
> then it may not go through here. I think I made this same mistake in my
> previous suggestion.
> 

Since dwc->connected is set to FALSE before we call stop active 
transfers, if we ever run into a situation where delayed stop is set 
already, then it should go through.

Thanks
Wesley Cheng

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

[Thinh Nguyen]

On 7/21/2022, Wesley Cheng wrote:
> Hi Thinh,
>
> On 7/21/2022 3:08 PM, Thinh Nguyen wrote:
>> Hi Wesley,
>>
>> On 7/12/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.
>>>
>>> Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to 
>>> complete during dequeue")
>>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>> ---
>>> Link:
>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/__;!!A4F2R9G_pg!cXW2TlALYWnXNPg-NoMFUrQ8K1egEZiQizZ5CA1DOM1Gcw4tfOULy-_2eMGvL8pQPte5dScFON-0wxH2eXu8ijEQUbs$
>>>
>>>    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;
>>
>> If we don't clear this flag here,
>>
>
> This is why I added the dwc->connected argument to control the 
> "interrupt" argument.
>
>>> - 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 ee85b773e3fe..41b7007358de 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -1693,6 +1693,7 @@ static int __dwc3_stop_active_transfer(struct 
>>> dwc3_ep *dep, bool force, bool int
>>>            dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>>>        else if (!ret)
>>>            dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
>>> +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>           return ret;
>>>    }
>>> @@ -3686,8 +3687,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;
>>> +
>>
>> then it may not go through here. I think I made this same mistake in my
>> previous suggestion.
>>
>
> Since dwc->connected is set to FALSE before we call stop active 
> transfers, if we ever run into a situation where delayed stop is set 
> already, then it should go through.
>

Then the check for request dequeue that we did previously will not work 
anymore.

BR,
Thinh

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

[Wesley Cheng]

Hi Thinh,

On 7/21/2022 3:20 PM, Thinh Nguyen wrote:
> On 7/21/2022, Wesley Cheng wrote:
>> Hi Thinh,
>>
>> On 7/21/2022 3:08 PM, Thinh Nguyen wrote:
>>> Hi Wesley,
>>>
>>> On 7/12/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.
>>>>
>>>> Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to
>>>> complete during dequeue")
>>>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>> ---
>>>> Link:
>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/__;!!A4F2R9G_pg!cXW2TlALYWnXNPg-NoMFUrQ8K1egEZiQizZ5CA1DOM1Gcw4tfOULy-_2eMGvL8pQPte5dScFON-0wxH2eXu8ijEQUbs$
>>>>
>>>>     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;
>>>
>>> If we don't clear this flag here,
>>>
>>
>> This is why I added the dwc->connected argument to control the
>> "interrupt" argument.
>>
>>>> - 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 ee85b773e3fe..41b7007358de 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -1693,6 +1693,7 @@ static int __dwc3_stop_active_transfer(struct
>>>> dwc3_ep *dep, bool force, bool int
>>>>             dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>>>>         else if (!ret)
>>>>             dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
>>>> +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>            return ret;
>>>>     }
>>>> @@ -3686,8 +3687,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;
>>>> +
>>>
>>> then it may not go through here. I think I made this same mistake in my
>>> previous suggestion.
>>>
>>
>> Since dwc->connected is set to FALSE before we call stop active
>> transfers, if we ever run into a situation where delayed stop is set
>> already, then it should go through.
>>
> 
> Then the check for request dequeue that we did previously will not work
> anymore.
> 

Could you help clarify?  The pullup refactor kind of shifted some of the 
previous checks we had in the dequeue path, and combined with with the 
stop active transfer checks.

I think there was an issue w/ the patch I submitted though.  the above 
snippet should be replacing what is there:

void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
	bool interrupt)
{
...
	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
	    /* Following should be removed --- (dep->flags & 
DWC3_EP_DELAY_STOP) || */
	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
		return;

Thanks
Wesley Cheng

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

[Thinh Nguyen]

On 7/21/2022, Wesley Cheng wrote:
> Hi Thinh,
>
> On 7/21/2022 3:20 PM, Thinh Nguyen wrote:
>> On 7/21/2022, Wesley Cheng wrote:
>>> Hi Thinh,
>>>
>>> On 7/21/2022 3:08 PM, Thinh Nguyen wrote:
>>>> Hi Wesley,
>>>>
>>>> On 7/12/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.
>>>>>
>>>>> Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to
>>>>> complete during dequeue")
>>>>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>>> ---
>>>>> Link:
>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/__;!!A4F2R9G_pg!cXW2TlALYWnXNPg-NoMFUrQ8K1egEZiQizZ5CA1DOM1Gcw4tfOULy-_2eMGvL8pQPte5dScFON-0wxH2eXu8ijEQUbs$ 
>>>>>
>>>>>
>>>>>     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;
>>>>
>>>> If we don't clear this flag here,
>>>>
>>>
>>> This is why I added the dwc->connected argument to control the
>>> "interrupt" argument.
>>>
>>>>> - 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 ee85b773e3fe..41b7007358de 100644
>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>> @@ -1693,6 +1693,7 @@ static int __dwc3_stop_active_transfer(struct
>>>>> dwc3_ep *dep, bool force, bool int
>>>>>             dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>>>>>         else if (!ret)
>>>>>             dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
>>>>> +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>>            return ret;
>>>>>     }
>>>>> @@ -3686,8 +3687,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;
>>>>> +
>>>>
>>>> then it may not go through here. I think I made this same mistake 
>>>> in my
>>>> previous suggestion.
>>>>
>>>
>>> Since dwc->connected is set to FALSE before we call stop active
>>> transfers, if we ever run into a situation where delayed stop is set
>>> already, then it should go through.
>>>
>>
>> Then the check for request dequeue that we did previously will not work
>> anymore.
>>
>
> Could you help clarify?  The pullup refactor kind of shifted some of 
> the previous checks we had in the dequeue path, and combined with with 
> the stop active transfer checks.
>
> I think there was an issue w/ the patch I submitted though.  the above 
> snippet should be replacing what is there:
>
> void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>     bool interrupt)
> {
> ...
>     if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>         /* Following should be removed --- (dep->flags & 
> DWC3_EP_DELAY_STOP) || */
>         (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>         return;
>

Request dequeue can occur while the device is connected. The 
DWC3_EP_DELAY_STOP intention is to delay sending the End Transfer 
command until the SETUP stage is prepared. If we don't clear the flag 
before the flag check, then the End Transfer command will not go through.

Thanks,
Thinh

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

[Wesley Cheng]

Hi Thinh,

On 7/21/2022 5:00 PM, Thinh Nguyen wrote:
> On 7/21/2022, Wesley Cheng wrote:
>> Hi Thinh,
>>
>> On 7/21/2022 3:20 PM, Thinh Nguyen wrote:
>>> On 7/21/2022, Wesley Cheng wrote:
>>>> Hi Thinh,
>>>>
>>>> On 7/21/2022 3:08 PM, Thinh Nguyen wrote:
>>>>> Hi Wesley,
>>>>>
>>>>> On 7/12/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.
>>>>>>
>>>>>> Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to
>>>>>> complete during dequeue")
>>>>>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>>>> ---
>>>>>> Link:
>>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/__;!!A4F2R9G_pg!cXW2TlALYWnXNPg-NoMFUrQ8K1egEZiQizZ5CA1DOM1Gcw4tfOULy-_2eMGvL8pQPte5dScFON-0wxH2eXu8ijEQUbs$
>>>>>>
>>>>>>
>>>>>>      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;
>>>>>
>>>>> If we don't clear this flag here,
>>>>>
>>>>
>>>> This is why I added the dwc->connected argument to control the
>>>> "interrupt" argument.
>>>>
>>>>>> - 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 ee85b773e3fe..41b7007358de 100644
>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>> @@ -1693,6 +1693,7 @@ static int __dwc3_stop_active_transfer(struct
>>>>>> dwc3_ep *dep, bool force, bool int
>>>>>>              dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>>>>>>          else if (!ret)
>>>>>>              dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
>>>>>> +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>>>             return ret;
>>>>>>      }
>>>>>> @@ -3686,8 +3687,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;
>>>>>> +
>>>>>
>>>>> then it may not go through here. I think I made this same mistake
>>>>> in my
>>>>> previous suggestion.
>>>>>
>>>>
>>>> Since dwc->connected is set to FALSE before we call stop active
>>>> transfers, if we ever run into a situation where delayed stop is set
>>>> already, then it should go through.
>>>>
>>>
>>> Then the check for request dequeue that we did previously will not work
>>> anymore.
>>>
>>
>> Could you help clarify?  The pullup refactor kind of shifted some of
>> the previous checks we had in the dequeue path, and combined with with
>> the stop active transfer checks.
>>
>> I think there was an issue w/ the patch I submitted though.  the above
>> snippet should be replacing what is there:
>>
>> void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>>      bool interrupt)
>> {
>> ...
>>      if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>          /* Following should be removed --- (dep->flags &
>> DWC3_EP_DELAY_STOP) || */
>>          (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>          return;
>>
> 
> Request dequeue can occur while the device is connected. The
> DWC3_EP_DELAY_STOP intention is to delay sending the End Transfer
> command until the SETUP stage is prepared. If we don't clear the flag
> before the flag check, then the End Transfer command will not go through.
> 

Thanks, got it.  Understand what you mean now.  Let me think about how 
to go about it.  I probably don't see any issues as of now, because my 
test does the soft disconnect, which will do the stop active transfers 
forcefully.

Thanks
Wesley Cheng

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

[Wesley Cheng]

Hi Thinh,

On 7/21/2022 5:04 PM, Wesley Cheng wrote:
> Hi Thinh,
> 
> On 7/21/2022 5:00 PM, Thinh Nguyen wrote:
>> On 7/21/2022, Wesley Cheng wrote:
>>> Hi Thinh,
>>>
>>> On 7/21/2022 3:20 PM, Thinh Nguyen wrote:
>>>> On 7/21/2022, Wesley Cheng wrote:
>>>>> Hi Thinh,
>>>>>
>>>>> On 7/21/2022 3:08 PM, Thinh Nguyen wrote:
>>>>>> Hi Wesley,
>>>>>>
>>>>>> On 7/12/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.
>>>>>>>
>>>>>>> Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to
>>>>>>> complete during dequeue")
>>>>>>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>>>>> ---
>>>>>>> Link:
>>>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/__;!!A4F2R9G_pg!cXW2TlALYWnXNPg-NoMFUrQ8K1egEZiQizZ5CA1DOM1Gcw4tfOULy-_2eMGvL8pQPte5dScFON-0wxH2eXu8ijEQUbs$ 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>      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;
>>>>>>
>>>>>> If we don't clear this flag here,
>>>>>>
>>>>>
>>>>> This is why I added the dwc->connected argument to control the
>>>>> "interrupt" argument.
>>>>>
>>>>>>> - 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 ee85b773e3fe..41b7007358de 100644
>>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>>> @@ -1693,6 +1693,7 @@ static int __dwc3_stop_active_transfer(struct
>>>>>>> dwc3_ep *dep, bool force, bool int
>>>>>>>              dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>>>>>>>          else if (!ret)
>>>>>>>              dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
>>>>>>> +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>>>>             return ret;
>>>>>>>      }
>>>>>>> @@ -3686,8 +3687,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;
>>>>>>> +
>>>>>>
>>>>>> then it may not go through here. I think I made this same mistake
>>>>>> in my
>>>>>> previous suggestion.
>>>>>>
>>>>>
>>>>> Since dwc->connected is set to FALSE before we call stop active
>>>>> transfers, if we ever run into a situation where delayed stop is set
>>>>> already, then it should go through.
>>>>>
>>>>
>>>> Then the check for request dequeue that we did previously will not work
>>>> anymore.
>>>>
>>>
>>> Could you help clarify?  The pullup refactor kind of shifted some of
>>> the previous checks we had in the dequeue path, and combined with with
>>> the stop active transfer checks.
>>>
>>> I think there was an issue w/ the patch I submitted though.  the above
>>> snippet should be replacing what is there:
>>>
>>> void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>>>      bool interrupt)
>>> {
>>> ...
>>>      if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>>          /* Following should be removed --- (dep->flags &
>>> DWC3_EP_DELAY_STOP) || */
>>>          (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>>          return;
>>>
>>
>> Request dequeue can occur while the device is connected. The
>> DWC3_EP_DELAY_STOP intention is to delay sending the End Transfer
>> command until the SETUP stage is prepared. If we don't clear the flag
>> before the flag check, then the End Transfer command will not go through.
>>
> 
> Thanks, got it.  Understand what you mean now.  Let me think about how 
> to go about it.  I probably don't see any issues as of now, because my 
> test does the soft disconnect, which will do the stop active transfers 
> forcefully.
> 
What do you think about just removing the

(dep->flags & DWC3_EP_DELAY_STOP)

check?  In the end, as long as the conditions are satisfied (ie we 
aren't in the middle of a SETUP transaction) then we don't care too much 
about who called dwc3_stop_active_transfer().  We would still be able to 
avoid issuing the endxfer here:

	if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status) {
		dep->flags |= DWC3_EP_DELAY_STOP;
		return 0;
	}


Thanks
Wesley Cheng

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

[Thinh Nguyen]

On 7/21/2022, Wesley Cheng wrote:
> Hi Thinh,
>
> On 7/21/2022 5:04 PM, Wesley Cheng wrote:
>> Hi Thinh,
>>
>> On 7/21/2022 5:00 PM, Thinh Nguyen wrote:
>>> On 7/21/2022, Wesley Cheng wrote:
>>>> Hi Thinh,
>>>>
>>>> On 7/21/2022 3:20 PM, Thinh Nguyen wrote:
>>>>> On 7/21/2022, Wesley Cheng wrote:
>>>>>> Hi Thinh,
>>>>>>
>>>>>> On 7/21/2022 3:08 PM, Thinh Nguyen wrote:
>>>>>>> Hi Wesley,
>>>>>>>
>>>>>>> On 7/12/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.
>>>>>>>>
>>>>>>>> Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to
>>>>>>>> complete during dequeue")
>>>>>>>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>>>>>> ---
>>>>>>>> Link:
>>>>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/__;!!A4F2R9G_pg!cXW2TlALYWnXNPg-NoMFUrQ8K1egEZiQizZ5CA1DOM1Gcw4tfOULy-_2eMGvL8pQPte5dScFON-0wxH2eXu8ijEQUbs$ 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>      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;
>>>>>>>
>>>>>>> If we don't clear this flag here,
>>>>>>>
>>>>>>
>>>>>> This is why I added the dwc->connected argument to control the
>>>>>> "interrupt" argument.
>>>>>>
>>>>>>>> - 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 ee85b773e3fe..41b7007358de 100644
>>>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>>>> @@ -1693,6 +1693,7 @@ static int 
>>>>>>>> __dwc3_stop_active_transfer(struct
>>>>>>>> dwc3_ep *dep, bool force, bool int
>>>>>>>>              dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>>>>>>>>          else if (!ret)
>>>>>>>>              dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
>>>>>>>> +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>>>>>             return ret;
>>>>>>>>      }
>>>>>>>> @@ -3686,8 +3687,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;
>>>>>>>> +
>>>>>>>
>>>>>>> then it may not go through here. I think I made this same mistake
>>>>>>> in my
>>>>>>> previous suggestion.
>>>>>>>
>>>>>>
>>>>>> Since dwc->connected is set to FALSE before we call stop active
>>>>>> transfers, if we ever run into a situation where delayed stop is set
>>>>>> already, then it should go through.
>>>>>>
>>>>>
>>>>> Then the check for request dequeue that we did previously will not 
>>>>> work
>>>>> anymore.
>>>>>
>>>>
>>>> Could you help clarify?  The pullup refactor kind of shifted some of
>>>> the previous checks we had in the dequeue path, and combined with with
>>>> the stop active transfer checks.
>>>>
>>>> I think there was an issue w/ the patch I submitted though. the above
>>>> snippet should be replacing what is there:
>>>>
>>>> void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>>>>      bool interrupt)
>>>> {
>>>> ...
>>>>      if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>>>          /* Following should be removed --- (dep->flags &
>>>> DWC3_EP_DELAY_STOP) || */
>>>>          (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>>>          return;
>>>>
>>>
>>> Request dequeue can occur while the device is connected. The
>>> DWC3_EP_DELAY_STOP intention is to delay sending the End Transfer
>>> command until the SETUP stage is prepared. If we don't clear the flag
>>> before the flag check, then the End Transfer command will not go 
>>> through.
>>>
>>
>> Thanks, got it.  Understand what you mean now.  Let me think about 
>> how to go about it.  I probably don't see any issues as of now, 
>> because my test does the soft disconnect, which will do the stop 
>> active transfers forcefully.
>>
> What do you think about just removing the
>
> (dep->flags & DWC3_EP_DELAY_STOP)
>
> check?  In the end, as long as the conditions are satisfied (ie we 
> aren't in the middle of a SETUP transaction) then we don't care too 
> much about who called dwc3_stop_active_transfer().  We would still be 
> able to avoid issuing the endxfer here:
>
>     if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status) {
>         dep->flags |= DWC3_EP_DELAY_STOP;
>         return 0;
>     }

That's good. Then check for interrupt-on-completion there instead.

Thanks,
Thinh