This patch series addresses some issues seen while testing with the latest
soft disconnect implementation where EP events are allowed to process while
the controller halt is occurring.
#1
Since routines can now interweave, we can see that the soft disconnect can
occur while conndone is being serviced. This leads to a controller halt
timeout, as the soft disconnect clears the DEP flags, for which conndone
interrupt handler will issue a __dwc3_ep_enable(ep0), that leads to
re-issuing the set ep config command for every endpoint.
#2
Function drivers can ask for a delayed_status phase, while it processes the
received SETUP packet. This can lead to large delays when handling the
soft disconnect routine. To improve the timing, forcefully send the status
phase, as we are going to disconnect from the host.
#3
Ensure that local interrupts are left enabled, so that EP0 events can be
processed while the soft disconnect/dequeue is happening.
#4
Modify the DWC3_EP_DELAY_STOP flag management so that if these flags were set
before soft disconnect, that the disconnect routine will be able to properly
issue the endxfer command.
#5
Since EP0 events can occur during controller halt, it may increase the time
needed for the controller to fully stop.
Wesley Cheng (5):
usb: dwc3: Do not service EP0 and conndone events if soft disconnected
usb: dwc3: gadget: Force sending delayed status during soft disconnect
usb: dwc3: gadget: Adjust IRQ management during soft
disconnect/connect
usb: dwc3: Allow end transfer commands to be sent during soft
disconnect
usb: dwc3: gadget: Increase DWC3 controller halt timeout
drivers/usb/dwc3/ep0.c | 9 +++++----
drivers/usb/dwc3/gadget.c | 31 ++++++++++++++++++++++---------
2 files changed, 27 insertions(+), 13 deletions(-)
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 <[email protected]>
---
drivers/usb/dwc3/ep0.c | 6 ++++--
drivers/usb/dwc3/gadget.c | 3 +++
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 2a510e84eef4..506ef717fdc0 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
int ret;
spin_lock_irqsave(&dwc->lock, flags);
- if (!dep->endpoint.desc || !dwc->pullups_connected) {
+ if (!dep->endpoint.desc || !dwc->pullups_connected || !dwc->connected) {
dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
dep->name);
ret = -ESHUTDOWN;
@@ -813,7 +813,7 @@ static void dwc3_ep0_inspect_setup(struct dwc3 *dwc,
int ret = -EINVAL;
u32 len;
- if (!dwc->gadget_driver || !dwc->connected)
+ if (!dwc->gadget_driver || !dwc->softconnect || !dwc->connected)
goto out;
trace_dwc3_ctrl_req(ctrl);
@@ -1116,6 +1116,8 @@ static void dwc3_ep0_xfernotready(struct dwc3 *dwc,
{
switch (event->status) {
case DEPEVT_STATUS_CONTROL_DATA:
+ if (!dwc->softconnect || !dwc->connected)
+ return;
/*
* We already have a DATA transfer in the controller's cache,
* if we receive a XferNotReady(DATA) we will ignore it, unless
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index a944c7a6c83a..298e842c384c 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3877,6 +3877,9 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
u8 lanes = 1;
u8 speed;
+ if (!dwc->softconnect)
+ return;
+
reg = dwc3_readl(dwc->regs, DWC3_DSTS);
speed = reg & DWC3_DSTS_CONNECTSPD;
dwc->speed = speed;
If any function drivers request for a delayed status phase, this leads to a
SETUP transfer timeout error, since the function may take longer to process
the DATA stage. This eventually results in end transfer timeouts, as there
is a pending SETUP transaction.
Signed-off-by: Wesley Cheng <[email protected]>
---
drivers/usb/dwc3/gadget.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 298e842c384c..75cbc3f185d0 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2513,6 +2513,9 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
if (dwc->ep0state != EP0_SETUP_PHASE) {
int ret;
+ if (dwc->delayed_status)
+ dwc3_ep0_send_delayed_status(dwc);
+
reinit_completion(&dwc->ep0_in_setup);
spin_unlock_irqrestore(&dwc->lock, flags);
Local interrupts are currently being disabled as part of aquiring the
spin lock before issuing the endxfer command. Leave interrupts enabled, so
that EP0 events can continue to be processed. Also, ensure that there are
no pending interrupts before attempting to handle any soft
connect/disconnect.
Signed-off-by: Wesley Cheng <[email protected]>
---
drivers/usb/dwc3/gadget.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 75cbc3f185d0..bd40608b19df 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2046,7 +2046,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
trace_dwc3_ep_dequeue(req);
- spin_lock_irqsave(&dwc->lock, flags);
+ spin_lock(&dwc->lock);
list_for_each_entry(r, &dep->cancelled_list, list) {
if (r == req)
@@ -2085,7 +2085,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
request, ep->name);
ret = -EINVAL;
out:
- spin_unlock_irqrestore(&dwc->lock, flags);
+ spin_unlock(&dwc->lock);
return ret;
}
@@ -2501,9 +2501,7 @@ static int __dwc3_gadget_start(struct dwc3 *dwc);
static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
{
- unsigned long flags;
-
- spin_lock_irqsave(&dwc->lock, flags);
+ spin_lock(&dwc->lock);
dwc->connected = false;
/*
@@ -2518,10 +2516,10 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
reinit_completion(&dwc->ep0_in_setup);
- spin_unlock_irqrestore(&dwc->lock, flags);
+ spin_unlock(&dwc->lock);
ret = wait_for_completion_timeout(&dwc->ep0_in_setup,
msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT));
- spin_lock_irqsave(&dwc->lock, flags);
+ spin_lock(&dwc->lock);
if (ret == 0)
dev_warn(dwc->dev, "timed out waiting for SETUP phase\n");
}
@@ -2535,7 +2533,7 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
*/
dwc3_stop_active_transfers(dwc);
__dwc3_gadget_stop(dwc);
- spin_unlock_irqrestore(&dwc->lock, flags);
+ spin_unlock(&dwc->lock);
/*
* Note: if the GEVNTCOUNT indicates events in the event buffer, the
@@ -2581,6 +2579,8 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
return 0;
}
+ synchronize_irq(dwc->irq_gadget);
+
if (!is_on) {
ret = dwc3_gadget_soft_disconnect(dwc);
} else {
@@ -3740,7 +3740,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
* This mode is NOT available on the DWC_usb31 IP.
*/
+ spin_unlock(&dwc->lock);
__dwc3_stop_active_transfer(dep, force, interrupt);
+ spin_lock(&dwc->lock);
+
}
static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
Since EP0 transactions need to be completed before the controller halt
sequence is finished, this may take some time depending on the host and the
enabled functions. Increase the controller halt timeout, so that we give
the controller sufficient time to handle EP0 transfers.
Signed-off-by: Wesley Cheng <[email protected]>
---
drivers/usb/dwc3/gadget.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index fba2797ad9ae..a5c0e39bd002 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2487,6 +2487,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
do {
reg = dwc3_readl(dwc->regs, DWC3_DSTS);
reg &= DWC3_DSTS_DEVCTRLHLT;
+ msleep(1);
} while (--timeout && !(!is_on ^ !reg));
if (!timeout)
If soft disconnect is in progress, allow the endxfer command to be sent,
without this, there is an issue where the stop active transfer call
(during pullup disable) wouldn't actually issue the endxfer command,
while clearing the DEP flag.
In addition, if the DWC3_EP_DELAY_STOP flag was set before soft disconnect
started (i.e. from the dequeue path), ensure that when the EP0 transaction
completes during soft disconnect, to issue the endxfer with the force
parameter set, as it does not expect a command complete event.
Signed-off-by: Wesley Cheng <[email protected]>
---
drivers/usb/dwc3/ep0.c | 3 +--
drivers/usb/dwc3/gadget.c | 5 ++++-
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 506ef717fdc0..5851b0e9db0a 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
continue;
- dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
- dwc3_stop_active_transfer(dwc3_ep, true, true);
+ dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
}
}
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index bd40608b19df..fba2797ad9ae 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
return;
+ if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
+ return;
+
if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
- (dep->flags & DWC3_EP_DELAY_STOP) ||
(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
return;
@@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
__dwc3_stop_active_transfer(dep, force, interrupt);
spin_lock(&dwc->lock);
+ dep->flags &= ~DWC3_EP_DELAY_STOP;
}
static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
On 7/8/2022, Wesley Cheng wrote:
> Since EP0 transactions need to be completed before the controller halt
> sequence is finished, this may take some time depending on the host and the
> enabled functions. Increase the controller halt timeout, so that we give
> the controller sufficient time to handle EP0 transfers.
>
> Signed-off-by: Wesley Cheng <[email protected]>
> ---
> drivers/usb/dwc3/gadget.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index fba2797ad9ae..a5c0e39bd002 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2487,6 +2487,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
> do {
> reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> reg &= DWC3_DSTS_DEVCTRLHLT;
> + msleep(1);
I think it makes more sense to msleep() first before reading the register.
Thanks,
Thinh
> } while (--timeout && !(!is_on ^ !reg));
>
> if (!timeout)
On 7/8/2022, Wesley Cheng wrote:
> If soft disconnect is in progress, allow the endxfer command to be sent,
> without this, there is an issue where the stop active transfer call
> (during pullup disable) wouldn't actually issue the endxfer command,
> while clearing the DEP flag.
>
> In addition, if the DWC3_EP_DELAY_STOP flag was set before soft disconnect
> started (i.e. from the dequeue path), ensure that when the EP0 transaction
> completes during soft disconnect, to issue the endxfer with the force
> parameter set, as it does not expect a command complete event.
>
> Signed-off-by: Wesley Cheng <[email protected]>
> ---
> drivers/usb/dwc3/ep0.c | 3 +--
> drivers/usb/dwc3/gadget.c | 5 ++++-
> 2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 506ef717fdc0..5851b0e9db0a 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
> if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
> continue;
>
> - dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
> - dwc3_stop_active_transfer(dwc3_ep, true, true);
> + dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
> }
> }
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index bd40608b19df..fba2797ad9ae 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
> if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
> return;
>
> + if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
> + return;
> +
> if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
> - (dep->flags & DWC3_EP_DELAY_STOP) ||
> (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
> return;
>
> @@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
> __dwc3_stop_active_transfer(dep, force, interrupt);
> spin_lock(&dwc->lock);
>
> + dep->flags &= ~DWC3_EP_DELAY_STOP;
Can we clear this flag in __dwc3_stop_active_transfer(). It should apply
if End Transfer command was sent.
Thanks,
Thinh
> }
>
> static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
Hi Wesley,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on linus/master v5.19-rc5 next-20220708]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Wesley-Cheng/Fix-controller-halt-and-endxfer-timeout-issues/20220709-025241
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-randconfig-a002 (https://download.01.org/0day-ci/archive/20220709/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/457fe4752b0f6dcc5c1b329f91003b7ffc518b44
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Wesley-Cheng/Fix-controller-halt-and-endxfer-timeout-issues/20220709-025241
git checkout 457fe4752b0f6dcc5c1b329f91003b7ffc518b44
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/usb/dwc3/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/usb/dwc3/gadget.c: In function 'dwc3_gadget_ep_dequeue':
>> drivers/usb/dwc3/gadget.c:2032:41: warning: unused variable 'flags' [-Wunused-variable]
2032 | unsigned long flags;
| ^~~~~
vim +/flags +2032 drivers/usb/dwc3/gadget.c
d4f1afe5e896c1 Felipe Balbi 2018-08-01 2022
72246da40f3719 Felipe Balbi 2011-08-19 2023 static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
72246da40f3719 Felipe Balbi 2011-08-19 2024 struct usb_request *request)
72246da40f3719 Felipe Balbi 2011-08-19 2025 {
72246da40f3719 Felipe Balbi 2011-08-19 2026 struct dwc3_request *req = to_dwc3_request(request);
72246da40f3719 Felipe Balbi 2011-08-19 2027 struct dwc3_request *r = NULL;
72246da40f3719 Felipe Balbi 2011-08-19 2028
72246da40f3719 Felipe Balbi 2011-08-19 2029 struct dwc3_ep *dep = to_dwc3_ep(ep);
72246da40f3719 Felipe Balbi 2011-08-19 2030 struct dwc3 *dwc = dep->dwc;
72246da40f3719 Felipe Balbi 2011-08-19 2031
72246da40f3719 Felipe Balbi 2011-08-19 @2032 unsigned long flags;
72246da40f3719 Felipe Balbi 2011-08-19 2033 int ret = 0;
72246da40f3719 Felipe Balbi 2011-08-19 2034
2c4cbe6e5a9c71 Felipe Balbi 2014-04-30 2035 trace_dwc3_ep_dequeue(req);
2c4cbe6e5a9c71 Felipe Balbi 2014-04-30 2036
457fe4752b0f6d Wesley Cheng 2022-07-08 2037 spin_lock(&dwc->lock);
72246da40f3719 Felipe Balbi 2011-08-19 2038
a7027ca69d82ae Thinh Nguyen 2020-03-05 2039 list_for_each_entry(r, &dep->cancelled_list, list) {
72246da40f3719 Felipe Balbi 2011-08-19 2040 if (r == req)
fcd2def6639293 Thinh Nguyen 2020-03-05 2041 goto out;
72246da40f3719 Felipe Balbi 2011-08-19 2042 }
72246da40f3719 Felipe Balbi 2011-08-19 2043
aa3342c8bb618a Felipe Balbi 2016-03-14 2044 list_for_each_entry(r, &dep->pending_list, list) {
fcd2def6639293 Thinh Nguyen 2020-03-05 2045 if (r == req) {
fcd2def6639293 Thinh Nguyen 2020-03-05 2046 dwc3_gadget_giveback(dep, req, -ECONNRESET);
fcd2def6639293 Thinh Nguyen 2020-03-05 2047 goto out;
fcd2def6639293 Thinh Nguyen 2020-03-05 2048 }
72246da40f3719 Felipe Balbi 2011-08-19 2049 }
72246da40f3719 Felipe Balbi 2011-08-19 2050
aa3342c8bb618a Felipe Balbi 2016-03-14 2051 list_for_each_entry(r, &dep->started_list, list) {
72246da40f3719 Felipe Balbi 2011-08-19 2052 if (r == req) {
a7027ca69d82ae Thinh Nguyen 2020-03-05 2053 struct dwc3_request *t;
a7027ca69d82ae Thinh Nguyen 2020-03-05 2054
72246da40f3719 Felipe Balbi 2011-08-19 2055 /* wait until it is processed */
c5353b225df9b2 Felipe Balbi 2019-02-13 2056 dwc3_stop_active_transfer(dep, true, true);
cf3113d893d442 Felipe Balbi 2017-02-17 2057
a7027ca69d82ae Thinh Nguyen 2020-03-05 2058 /*
a7027ca69d82ae Thinh Nguyen 2020-03-05 2059 * Remove any started request if the transfer is
a7027ca69d82ae Thinh Nguyen 2020-03-05 2060 * cancelled.
a7027ca69d82ae Thinh Nguyen 2020-03-05 2061 */
a7027ca69d82ae Thinh Nguyen 2020-03-05 2062 list_for_each_entry_safe(r, t, &dep->started_list, list)
04dd6e76b22889 Ray Chi 2021-03-28 2063 dwc3_gadget_move_cancelled_request(r,
04dd6e76b22889 Ray Chi 2021-03-28 2064 DWC3_REQUEST_STATUS_DEQUEUED);
cf3113d893d442 Felipe Balbi 2017-02-17 2065
a5c7682aaaa10e Thinh Nguyen 2021-01-04 2066 dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
a5c7682aaaa10e Thinh Nguyen 2021-01-04 2067
fcd2def6639293 Thinh Nguyen 2020-03-05 2068 goto out;
72246da40f3719 Felipe Balbi 2011-08-19 2069 }
72246da40f3719 Felipe Balbi 2011-08-19 2070 }
fcd2def6639293 Thinh Nguyen 2020-03-05 2071
04fb365c453e14 Felipe Balbi 2017-05-17 2072 dev_err(dwc->dev, "request %pK was not queued to %s\n",
72246da40f3719 Felipe Balbi 2011-08-19 2073 request, ep->name);
72246da40f3719 Felipe Balbi 2011-08-19 2074 ret = -EINVAL;
fcd2def6639293 Thinh Nguyen 2020-03-05 2075 out:
457fe4752b0f6d Wesley Cheng 2022-07-08 2076 spin_unlock(&dwc->lock);
72246da40f3719 Felipe Balbi 2011-08-19 2077
72246da40f3719 Felipe Balbi 2011-08-19 2078 return ret;
72246da40f3719 Felipe Balbi 2011-08-19 2079 }
72246da40f3719 Felipe Balbi 2011-08-19 2080
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Wesley,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on linus/master v5.19-rc5 next-20220708]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Wesley-Cheng/Fix-controller-halt-and-endxfer-timeout-issues/20220709-025241
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-randconfig-a012 (https://download.01.org/0day-ci/archive/20220709/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 562c3467a6738aa89203f72fc1d1343e5baadf3c)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/457fe4752b0f6dcc5c1b329f91003b7ffc518b44
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Wesley-Cheng/Fix-controller-halt-and-endxfer-timeout-issues/20220709-025241
git checkout 457fe4752b0f6dcc5c1b329f91003b7ffc518b44
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/usb/dwc3/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/usb/dwc3/gadget.c:2032:18: warning: unused variable 'flags' [-Wunused-variable]
unsigned long flags;
^
1 warning generated.
vim +/flags +2032 drivers/usb/dwc3/gadget.c
d4f1afe5e896c1 Felipe Balbi 2018-08-01 2022
72246da40f3719 Felipe Balbi 2011-08-19 2023 static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
72246da40f3719 Felipe Balbi 2011-08-19 2024 struct usb_request *request)
72246da40f3719 Felipe Balbi 2011-08-19 2025 {
72246da40f3719 Felipe Balbi 2011-08-19 2026 struct dwc3_request *req = to_dwc3_request(request);
72246da40f3719 Felipe Balbi 2011-08-19 2027 struct dwc3_request *r = NULL;
72246da40f3719 Felipe Balbi 2011-08-19 2028
72246da40f3719 Felipe Balbi 2011-08-19 2029 struct dwc3_ep *dep = to_dwc3_ep(ep);
72246da40f3719 Felipe Balbi 2011-08-19 2030 struct dwc3 *dwc = dep->dwc;
72246da40f3719 Felipe Balbi 2011-08-19 2031
72246da40f3719 Felipe Balbi 2011-08-19 @2032 unsigned long flags;
72246da40f3719 Felipe Balbi 2011-08-19 2033 int ret = 0;
72246da40f3719 Felipe Balbi 2011-08-19 2034
2c4cbe6e5a9c71 Felipe Balbi 2014-04-30 2035 trace_dwc3_ep_dequeue(req);
2c4cbe6e5a9c71 Felipe Balbi 2014-04-30 2036
457fe4752b0f6d Wesley Cheng 2022-07-08 2037 spin_lock(&dwc->lock);
72246da40f3719 Felipe Balbi 2011-08-19 2038
a7027ca69d82ae Thinh Nguyen 2020-03-05 2039 list_for_each_entry(r, &dep->cancelled_list, list) {
72246da40f3719 Felipe Balbi 2011-08-19 2040 if (r == req)
fcd2def6639293 Thinh Nguyen 2020-03-05 2041 goto out;
72246da40f3719 Felipe Balbi 2011-08-19 2042 }
72246da40f3719 Felipe Balbi 2011-08-19 2043
aa3342c8bb618a Felipe Balbi 2016-03-14 2044 list_for_each_entry(r, &dep->pending_list, list) {
fcd2def6639293 Thinh Nguyen 2020-03-05 2045 if (r == req) {
fcd2def6639293 Thinh Nguyen 2020-03-05 2046 dwc3_gadget_giveback(dep, req, -ECONNRESET);
fcd2def6639293 Thinh Nguyen 2020-03-05 2047 goto out;
fcd2def6639293 Thinh Nguyen 2020-03-05 2048 }
72246da40f3719 Felipe Balbi 2011-08-19 2049 }
72246da40f3719 Felipe Balbi 2011-08-19 2050
aa3342c8bb618a Felipe Balbi 2016-03-14 2051 list_for_each_entry(r, &dep->started_list, list) {
72246da40f3719 Felipe Balbi 2011-08-19 2052 if (r == req) {
a7027ca69d82ae Thinh Nguyen 2020-03-05 2053 struct dwc3_request *t;
a7027ca69d82ae Thinh Nguyen 2020-03-05 2054
72246da40f3719 Felipe Balbi 2011-08-19 2055 /* wait until it is processed */
c5353b225df9b2 Felipe Balbi 2019-02-13 2056 dwc3_stop_active_transfer(dep, true, true);
cf3113d893d442 Felipe Balbi 2017-02-17 2057
a7027ca69d82ae Thinh Nguyen 2020-03-05 2058 /*
a7027ca69d82ae Thinh Nguyen 2020-03-05 2059 * Remove any started request if the transfer is
a7027ca69d82ae Thinh Nguyen 2020-03-05 2060 * cancelled.
a7027ca69d82ae Thinh Nguyen 2020-03-05 2061 */
a7027ca69d82ae Thinh Nguyen 2020-03-05 2062 list_for_each_entry_safe(r, t, &dep->started_list, list)
04dd6e76b22889 Ray Chi 2021-03-28 2063 dwc3_gadget_move_cancelled_request(r,
04dd6e76b22889 Ray Chi 2021-03-28 2064 DWC3_REQUEST_STATUS_DEQUEUED);
cf3113d893d442 Felipe Balbi 2017-02-17 2065
a5c7682aaaa10e Thinh Nguyen 2021-01-04 2066 dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
a5c7682aaaa10e Thinh Nguyen 2021-01-04 2067
fcd2def6639293 Thinh Nguyen 2020-03-05 2068 goto out;
72246da40f3719 Felipe Balbi 2011-08-19 2069 }
72246da40f3719 Felipe Balbi 2011-08-19 2070 }
fcd2def6639293 Thinh Nguyen 2020-03-05 2071
04fb365c453e14 Felipe Balbi 2017-05-17 2072 dev_err(dwc->dev, "request %pK was not queued to %s\n",
72246da40f3719 Felipe Balbi 2011-08-19 2073 request, ep->name);
72246da40f3719 Felipe Balbi 2011-08-19 2074 ret = -EINVAL;
fcd2def6639293 Thinh Nguyen 2020-03-05 2075 out:
457fe4752b0f6d Wesley Cheng 2022-07-08 2076 spin_unlock(&dwc->lock);
72246da40f3719 Felipe Balbi 2011-08-19 2077
72246da40f3719 Felipe Balbi 2011-08-19 2078 return ret;
72246da40f3719 Felipe Balbi 2011-08-19 2079 }
72246da40f3719 Felipe Balbi 2011-08-19 2080
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Thinh,
On 7/8/2022 6:58 PM, Thinh Nguyen wrote:
> On 7/8/2022, Wesley Cheng wrote:
>> If soft disconnect is in progress, allow the endxfer command to be sent,
>> without this, there is an issue where the stop active transfer call
>> (during pullup disable) wouldn't actually issue the endxfer command,
>> while clearing the DEP flag.
>>
>> In addition, if the DWC3_EP_DELAY_STOP flag was set before soft disconnect
>> started (i.e. from the dequeue path), ensure that when the EP0 transaction
>> completes during soft disconnect, to issue the endxfer with the force
>> parameter set, as it does not expect a command complete event.
>>
>> Signed-off-by: Wesley Cheng <[email protected]>
>> ---
>> drivers/usb/dwc3/ep0.c | 3 +--
>> drivers/usb/dwc3/gadget.c | 5 ++++-
>> 2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 506ef717fdc0..5851b0e9db0a 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>> if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
>> continue;
>>
>> - dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
>> - dwc3_stop_active_transfer(dwc3_ep, true, true);
>> + dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
>> }
>> }
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index bd40608b19df..fba2797ad9ae 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>> if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
>> return;
>>
>> + if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
>> + return;
>> +
>> if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>> - (dep->flags & DWC3_EP_DELAY_STOP) ||
>> (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>> return;
>>
>> @@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>> __dwc3_stop_active_transfer(dep, force, interrupt);
>> spin_lock(&dwc->lock);
>>
>> + dep->flags &= ~DWC3_EP_DELAY_STOP;
>
> Can we clear this flag in __dwc3_stop_active_transfer(). It should apply
> if End Transfer command was sent.
I wanted to make sure that we weren't modifying the DEP flags outside of
a spin lock. Patch#3 modifies it where we unlock before calling
__dwc3_stop_active_transfer(), so we can allow the dwc3 threaded IRQ
handle events while the cmd status polling happens.
Maybe we can unlock/lock the dwc3->lock inside
__dwc3_stop_active_transfer() and that way we can ensure DEP flags are
modified properly?
Thanks
Wesley Cheng
On 7/12/2022, Wesley Cheng wrote:
> Hi Thinh,
>
> On 7/8/2022 6:58 PM, Thinh Nguyen wrote:
>> On 7/8/2022, Wesley Cheng wrote:
>>> If soft disconnect is in progress, allow the endxfer command to be
>>> sent,
>>> without this, there is an issue where the stop active transfer call
>>> (during pullup disable) wouldn't actually issue the endxfer command,
>>> while clearing the DEP flag.
>>>
>>> In addition, if the DWC3_EP_DELAY_STOP flag was set before soft
>>> disconnect
>>> started (i.e. from the dequeue path), ensure that when the EP0
>>> transaction
>>> completes during soft disconnect, to issue the endxfer with the force
>>> parameter set, as it does not expect a command complete event.
>>>
>>> Signed-off-by: Wesley Cheng <[email protected]>
>>> ---
>>> drivers/usb/dwc3/ep0.c | 3 +--
>>> drivers/usb/dwc3/gadget.c | 5 ++++-
>>> 2 files changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>> index 506ef717fdc0..5851b0e9db0a 100644
>>> --- a/drivers/usb/dwc3/ep0.c
>>> +++ b/drivers/usb/dwc3/ep0.c
>>> @@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>>> if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
>>> continue;
>>> - dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
>>> - dwc3_stop_active_transfer(dwc3_ep, true, true);
>>> + dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
>>> }
>>> }
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index bd40608b19df..fba2797ad9ae 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep
>>> *dep, bool force,
>>> if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
>>> return;
>>> + if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
>>> + return;
>>> +
>>> if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>> - (dep->flags & DWC3_EP_DELAY_STOP) ||
>>> (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>> return;
>>> @@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct
>>> dwc3_ep *dep, bool force,
>>> __dwc3_stop_active_transfer(dep, force, interrupt);
>>> spin_lock(&dwc->lock);
>>> + dep->flags &= ~DWC3_EP_DELAY_STOP;
>>
>> Can we clear this flag in __dwc3_stop_active_transfer(). It should apply
>> if End Transfer command was sent.
>
> I wanted to make sure that we weren't modifying the DEP flags outside
> of a spin lock. Patch#3 modifies it where we unlock before calling
> __dwc3_stop_active_transfer(), so we can allow the dwc3 threaded IRQ
> handle events while the cmd status polling happens.
>
> Maybe we can unlock/lock the dwc3->lock inside
> __dwc3_stop_active_transfer() and that way we can ensure DEP flags are
> modified properly?
I didn't realize that you unlock/lock when calling
__dwc3_stop_active_transfer(). We'd need to be careful if we want to
unlock/lock it, and avoid it all together if possible. It can be easily
overlooked just like dwc3_gadget_giveback().
What issue did you see without doing this?
Thanks,
Thinh
Hi Thinh,
On 7/12/2022 6:42 PM, Thinh Nguyen wrote:
> On 7/12/2022, Wesley Cheng wrote:
>> Hi Thinh,
>>
>> On 7/8/2022 6:58 PM, Thinh Nguyen wrote:
>>> On 7/8/2022, Wesley Cheng wrote:
>>>> If soft disconnect is in progress, allow the endxfer command to be
>>>> sent,
>>>> without this, there is an issue where the stop active transfer call
>>>> (during pullup disable) wouldn't actually issue the endxfer command,
>>>> while clearing the DEP flag.
>>>>
>>>> In addition, if the DWC3_EP_DELAY_STOP flag was set before soft
>>>> disconnect
>>>> started (i.e. from the dequeue path), ensure that when the EP0
>>>> transaction
>>>> completes during soft disconnect, to issue the endxfer with the force
>>>> parameter set, as it does not expect a command complete event.
>>>>
>>>> Signed-off-by: Wesley Cheng <[email protected]>
>>>> ---
>>>> drivers/usb/dwc3/ep0.c | 3 +--
>>>> drivers/usb/dwc3/gadget.c | 5 ++++-
>>>> 2 files changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>> index 506ef717fdc0..5851b0e9db0a 100644
>>>> --- a/drivers/usb/dwc3/ep0.c
>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>> @@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>>>> if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
>>>> continue;
>>>> - dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
>>>> - dwc3_stop_active_transfer(dwc3_ep, true, true);
>>>> + dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
>>>> }
>>>> }
>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index bd40608b19df..fba2797ad9ae 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep
>>>> *dep, bool force,
>>>> if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
>>>> return;
>>>> + if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
>>>> + return;
>>>> +
>>>> if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>>> - (dep->flags & DWC3_EP_DELAY_STOP) ||
>>>> (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>>> return;
>>>> @@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct
>>>> dwc3_ep *dep, bool force,
>>>> __dwc3_stop_active_transfer(dep, force, interrupt);
>>>> spin_lock(&dwc->lock);
>>>> + dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>
>>> Can we clear this flag in __dwc3_stop_active_transfer(). It should apply
>>> if End Transfer command was sent.
>>
>> I wanted to make sure that we weren't modifying the DEP flags outside
>> of a spin lock. Patch#3 modifies it where we unlock before calling
>> __dwc3_stop_active_transfer(), so we can allow the dwc3 threaded IRQ
>> handle events while the cmd status polling happens.
>>
>> Maybe we can unlock/lock the dwc3->lock inside
>> __dwc3_stop_active_transfer() and that way we can ensure DEP flags are
>> modified properly?
>
> I didn't realize that you unlock/lock when calling
> __dwc3_stop_active_transfer(). We'd need to be careful if we want to
> unlock/lock it, and avoid it all together if possible. It can be easily
> overlooked just like dwc3_gadget_giveback().
>
> What issue did you see without doing this?
I saw endxfer timeout issues if I didn't do it. If we keep the lock
held, then the DWC3 event processing would be blocked across the entire
time we are waiting for the command act to clear. With unlocking before
polling, then at least we're still able to handle the EP0 events that
are pending.
It was definitely one of the harder scenarios to reproduce. The main
patch series which resolved a lot of the issues early on was patch#1.
After adding that the other issues are seen maybe after a day or so of
testing.
Thanks
Wesley Cheng
On 7/13/2022, Wesley Cheng wrote:
> Hi Thinh,
>
> On 7/12/2022 6:42 PM, Thinh Nguyen wrote:
>> On 7/12/2022, Wesley Cheng wrote:
>>> Hi Thinh,
>>>
>>> On 7/8/2022 6:58 PM, Thinh Nguyen wrote:
>>>> On 7/8/2022, Wesley Cheng wrote:
>>>>> If soft disconnect is in progress, allow the endxfer command to be
>>>>> sent,
>>>>> without this, there is an issue where the stop active transfer call
>>>>> (during pullup disable) wouldn't actually issue the endxfer command,
>>>>> while clearing the DEP flag.
>>>>>
>>>>> In addition, if the DWC3_EP_DELAY_STOP flag was set before soft
>>>>> disconnect
>>>>> started (i.e. from the dequeue path), ensure that when the EP0
>>>>> transaction
>>>>> completes during soft disconnect, to issue the endxfer with the force
>>>>> parameter set, as it does not expect a command complete event.
>>>>>
>>>>> Signed-off-by: Wesley Cheng <[email protected]>
>>>>> ---
>>>>> drivers/usb/dwc3/ep0.c | 3 +--
>>>>> drivers/usb/dwc3/gadget.c | 5 ++++-
>>>>> 2 files changed, 5 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>>> index 506ef717fdc0..5851b0e9db0a 100644
>>>>> --- a/drivers/usb/dwc3/ep0.c
>>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>>> @@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>>>>> if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
>>>>> continue;
>>>>> - dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>> - dwc3_stop_active_transfer(dwc3_ep, true, true);
>>>>> + dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
>>>>> }
>>>>> }
>>>>> diff --git a/drivers/usb/dwc3/gadget.c
>>>>> b/drivers/usb/dwc3/gadget.c
>>>>> index bd40608b19df..fba2797ad9ae 100644
>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>> @@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep
>>>>> *dep, bool force,
>>>>> if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
>>>>> return;
>>>>> + if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
>>>>> + return;
>>>>> +
>>>>> if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>>>> - (dep->flags & DWC3_EP_DELAY_STOP) ||
>>>>> (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>>>> return;
>>>>> @@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct
>>>>> dwc3_ep *dep, bool force,
>>>>> __dwc3_stop_active_transfer(dep, force, interrupt);
>>>>> spin_lock(&dwc->lock);
>>>>> + dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>
>>>> Can we clear this flag in __dwc3_stop_active_transfer(). It should
>>>> apply
>>>> if End Transfer command was sent.
>>>
>>> I wanted to make sure that we weren't modifying the DEP flags outside
>>> of a spin lock. Patch#3 modifies it where we unlock before calling
>>> __dwc3_stop_active_transfer(), so we can allow the dwc3 threaded IRQ
>>> handle events while the cmd status polling happens.
>>>
>>> Maybe we can unlock/lock the dwc3->lock inside
>>> __dwc3_stop_active_transfer() and that way we can ensure DEP flags are
>>> modified properly?
>>
>> I didn't realize that you unlock/lock when calling
>> __dwc3_stop_active_transfer(). We'd need to be careful if we want to
>> unlock/lock it, and avoid it all together if possible. It can be easily
>> overlooked just like dwc3_gadget_giveback().
>>
>> What issue did you see without doing this?
>
> I saw endxfer timeout issues if I didn't do it. If we keep the lock
> held, then the DWC3 event processing would be blocked across the
> entire time we are waiting for the command act to clear. With
> unlocking before polling, then at least we're still able to handle the
> EP0 events that are pending.
>
> It was definitely one of the harder scenarios to reproduce. The main
> patch series which resolved a lot of the issues early on was patch#1.
> After adding that the other issues are seen maybe after a day or so of
> testing.
>
I see. The soft-disconnect still go through right?
Can you try this and see if this works for you?
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 13e4f1a03417..4f67a484c490 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -451,7 +451,9 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep,
unsigned int cmd,
dwc3_writel(dep->regs, DWC3_DEPCMD, cmd);
- if (!(cmd & DWC3_DEPCMD_CMDACT)) {
+ if (!(cmd & DWC3_DEPCMD_CMDACT) ||
+ (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_ENDTRANSFER &&
+ !(cmd & DWC3_DEPCMD_CMDIOC))) {
ret = 0;
goto skip_status;
}
We can set it and forget it when we're about to de-initialize the
controller. Just like what we've already done by not waiting for the End
Transfer endpoint command completion interrupt.
Thanks,
Thinh
Hi Thinh,
On 7/13/2022 5:19 PM, Thinh Nguyen wrote:
> On 7/13/2022, Wesley Cheng wrote:
>> Hi Thinh,
>>
>> On 7/12/2022 6:42 PM, Thinh Nguyen wrote:
>>> On 7/12/2022, Wesley Cheng wrote:
>>>> Hi Thinh,
>>>>
>>>> On 7/8/2022 6:58 PM, Thinh Nguyen wrote:
>>>>> On 7/8/2022, Wesley Cheng wrote:
>>>>>> If soft disconnect is in progress, allow the endxfer command to be
>>>>>> sent,
>>>>>> without this, there is an issue where the stop active transfer call
>>>>>> (during pullup disable) wouldn't actually issue the endxfer command,
>>>>>> while clearing the DEP flag.
>>>>>>
>>>>>> In addition, if the DWC3_EP_DELAY_STOP flag was set before soft
>>>>>> disconnect
>>>>>> started (i.e. from the dequeue path), ensure that when the EP0
>>>>>> transaction
>>>>>> completes during soft disconnect, to issue the endxfer with the force
>>>>>> parameter set, as it does not expect a command complete event.
>>>>>>
>>>>>> Signed-off-by: Wesley Cheng <[email protected]>
>>>>>> ---
>>>>>> drivers/usb/dwc3/ep0.c | 3 +--
>>>>>> drivers/usb/dwc3/gadget.c | 5 ++++-
>>>>>> 2 files changed, 5 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>>>> index 506ef717fdc0..5851b0e9db0a 100644
>>>>>> --- a/drivers/usb/dwc3/ep0.c
>>>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>>>> @@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>>>>>> if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
>>>>>> continue;
>>>>>> - dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>>> - dwc3_stop_active_transfer(dwc3_ep, true, true);
>>>>>> + dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
>>>>>> }
>>>>>> }
>>>>>> diff --git a/drivers/usb/dwc3/gadget.c
>>>>>> b/drivers/usb/dwc3/gadget.c
>>>>>> index bd40608b19df..fba2797ad9ae 100644
>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>> @@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep
>>>>>> *dep, bool force,
>>>>>> if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
>>>>>> return;
>>>>>> + if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
>>>>>> + return;
>>>>>> +
>>>>>> if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>>>>> - (dep->flags & DWC3_EP_DELAY_STOP) ||
>>>>>> (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>>>>> return;
>>>>>> @@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct
>>>>>> dwc3_ep *dep, bool force,
>>>>>> __dwc3_stop_active_transfer(dep, force, interrupt);
>>>>>> spin_lock(&dwc->lock);
>>>>>> + dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>>
>>>>> Can we clear this flag in __dwc3_stop_active_transfer(). It should
>>>>> apply
>>>>> if End Transfer command was sent.
>>>>
>>>> I wanted to make sure that we weren't modifying the DEP flags outside
>>>> of a spin lock. Patch#3 modifies it where we unlock before calling
>>>> __dwc3_stop_active_transfer(), so we can allow the dwc3 threaded IRQ
>>>> handle events while the cmd status polling happens.
>>>>
>>>> Maybe we can unlock/lock the dwc3->lock inside
>>>> __dwc3_stop_active_transfer() and that way we can ensure DEP flags are
>>>> modified properly?
>>>
>>> I didn't realize that you unlock/lock when calling
>>> __dwc3_stop_active_transfer(). We'd need to be careful if we want to
>>> unlock/lock it, and avoid it all together if possible. It can be easily
>>> overlooked just like dwc3_gadget_giveback().
>>>
>>> What issue did you see without doing this?
>>
>> I saw endxfer timeout issues if I didn't do it. If we keep the lock
>> held, then the DWC3 event processing would be blocked across the
>> entire time we are waiting for the command act to clear. With
>> unlocking before polling, then at least we're still able to handle the
>> EP0 events that are pending.
>>
>> It was definitely one of the harder scenarios to reproduce. The main
>> patch series which resolved a lot of the issues early on was patch#1.
>> After adding that the other issues are seen maybe after a day or so of
>> testing.
>>
>
> I see. The soft-disconnect still go through right?
> Can you try this and see if this works for you?
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 13e4f1a03417..4f67a484c490 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -451,7 +451,9 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep,
> unsigned int cmd,
>
> dwc3_writel(dep->regs, DWC3_DEPCMD, cmd);
>
> - if (!(cmd & DWC3_DEPCMD_CMDACT)) {
> + if (!(cmd & DWC3_DEPCMD_CMDACT) ||
> + (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_ENDTRANSFER &&
> + !(cmd & DWC3_DEPCMD_CMDIOC))) {
> ret = 0;
> goto skip_status;
> }
>
>
> We can set it and forget it when we're about to de-initialize the
> controller. Just like what we've already done by not waiting for the End
> Transfer endpoint command completion interrupt.
>
I can see this working for the soft disconnect case, but we'll need to
address it differently for the EP dequeue scenario where IOC/interrupt
is required to clean up the TRBs. This is one of the reasons why I went
with the spinlock approach, since it would apply to both situations.
Thanks
Wesley Cheng
On 7/13/2022, Wesley Cheng wrote:
> Hi Thinh,
>
> On 7/13/2022 5:19 PM, Thinh Nguyen wrote:
>> On 7/13/2022, Wesley Cheng wrote:
>>> Hi Thinh,
>>>
>>> On 7/12/2022 6:42 PM, Thinh Nguyen wrote:
>>>> On 7/12/2022, Wesley Cheng wrote:
>>>>> Hi Thinh,
>>>>>
>>>>> On 7/8/2022 6:58 PM, Thinh Nguyen wrote:
>>>>>> On 7/8/2022, Wesley Cheng wrote:
>>>>>>> If soft disconnect is in progress, allow the endxfer command to be
>>>>>>> sent,
>>>>>>> without this, there is an issue where the stop active transfer call
>>>>>>> (during pullup disable) wouldn't actually issue the endxfer
>>>>>>> command,
>>>>>>> while clearing the DEP flag.
>>>>>>>
>>>>>>> In addition, if the DWC3_EP_DELAY_STOP flag was set before soft
>>>>>>> disconnect
>>>>>>> started (i.e. from the dequeue path), ensure that when the EP0
>>>>>>> transaction
>>>>>>> completes during soft disconnect, to issue the endxfer with the
>>>>>>> force
>>>>>>> parameter set, as it does not expect a command complete event.
>>>>>>>
>>>>>>> Signed-off-by: Wesley Cheng <[email protected]>
>>>>>>> ---
>>>>>>> drivers/usb/dwc3/ep0.c | 3 +--
>>>>>>> drivers/usb/dwc3/gadget.c | 5 ++++-
>>>>>>> 2 files changed, 5 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>>>>> index 506ef717fdc0..5851b0e9db0a 100644
>>>>>>> --- a/drivers/usb/dwc3/ep0.c
>>>>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>>>>> @@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>>>>>>> if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
>>>>>>> continue;
>>>>>>> - dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>>>> - dwc3_stop_active_transfer(dwc3_ep, true, true);
>>>>>>> + dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
>>>>>>> }
>>>>>>> }
>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c
>>>>>>> b/drivers/usb/dwc3/gadget.c
>>>>>>> index bd40608b19df..fba2797ad9ae 100644
>>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>>> @@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct
>>>>>>> dwc3_ep
>>>>>>> *dep, bool force,
>>>>>>> if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
>>>>>>> return;
>>>>>>> + if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
>>>>>>> + return;
>>>>>>> +
>>>>>>> if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>>>>>> - (dep->flags & DWC3_EP_DELAY_STOP) ||
>>>>>>> (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>>>>>> return;
>>>>>>> @@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct
>>>>>>> dwc3_ep *dep, bool force,
>>>>>>> __dwc3_stop_active_transfer(dep, force, interrupt);
>>>>>>> spin_lock(&dwc->lock);
>>>>>>> + dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>>>
>>>>>> Can we clear this flag in __dwc3_stop_active_transfer(). It should
>>>>>> apply
>>>>>> if End Transfer command was sent.
>>>>>
>>>>> I wanted to make sure that we weren't modifying the DEP flags outside
>>>>> of a spin lock. Patch#3 modifies it where we unlock before calling
>>>>> __dwc3_stop_active_transfer(), so we can allow the dwc3 threaded IRQ
>>>>> handle events while the cmd status polling happens.
>>>>>
>>>>> Maybe we can unlock/lock the dwc3->lock inside
>>>>> __dwc3_stop_active_transfer() and that way we can ensure DEP flags
>>>>> are
>>>>> modified properly?
>>>>
>>>> I didn't realize that you unlock/lock when calling
>>>> __dwc3_stop_active_transfer(). We'd need to be careful if we want to
>>>> unlock/lock it, and avoid it all together if possible. It can be
>>>> easily
>>>> overlooked just like dwc3_gadget_giveback().
>>>>
>>>> What issue did you see without doing this?
>>>
>>> I saw endxfer timeout issues if I didn't do it. If we keep the lock
>>> held, then the DWC3 event processing would be blocked across the
>>> entire time we are waiting for the command act to clear. With
>>> unlocking before polling, then at least we're still able to handle the
>>> EP0 events that are pending.
>>>
>>> It was definitely one of the harder scenarios to reproduce. The main
>>> patch series which resolved a lot of the issues early on was patch#1.
>>> After adding that the other issues are seen maybe after a day or so of
>>> testing.
>>>
>>
>> I see. The soft-disconnect still go through right?
>> Can you try this and see if this works for you?
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 13e4f1a03417..4f67a484c490 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -451,7 +451,9 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep,
>> unsigned int cmd,
>>
>> dwc3_writel(dep->regs, DWC3_DEPCMD, cmd);
>>
>> - if (!(cmd & DWC3_DEPCMD_CMDACT)) {
>> + if (!(cmd & DWC3_DEPCMD_CMDACT) ||
>> + (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_ENDTRANSFER &&
>> + !(cmd & DWC3_DEPCMD_CMDIOC))) {
>> ret = 0;
>> goto skip_status;
>> }
>>
>>
>> We can set it and forget it when we're about to de-initialize the
>> controller. Just like what we've already done by not waiting for the End
>> Transfer endpoint command completion interrupt.
>>
>
> I can see this working for the soft disconnect case, but we'll need to
> address it differently for the EP dequeue scenario where IOC/interrupt
> is required to clean up the TRBs. This is one of the reasons why I
> went with the spinlock approach, since it would apply to both situations.
I thought we already handled the case for ep dequeue from the last fix.
Was that not addressed?
Thanks,
Thinh