mbox series

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

Message ID 20220815213134.23783-1-quic_wcheng@quicinc.com (mailing list archive)
Headers show
Series Fix controller halt and endxfer timeout issues | expand

Message

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

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

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

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

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

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

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

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

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

Wesley Cheng (8):
  usb: dwc3: Do not service EP0 and conndone events if soft disconnected
  usb: dwc3: gadget: Force sending delayed status during soft disconnect
  usb: dwc3: gadget: Synchronize IRQ between soft connect/disconnect
  usb: dwc3: gadget: Continue handling EP0 xfercomplete events
  usb: dwc3: Avoid unmapping USB requests if endxfer is not complete
  usb: dwc3: Increase DWC3 controller halt timeout
  usb: dwc3: gadget: Skip waiting for CMDACT cleared during endxfer
  usb: dwc3: gadget: Submit endxfer command if delayed during disconnect

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

Comments

Thinh Nguyen Aug. 17, 2022, 12:39 a.m. UTC | #1
Hi Wesley,

On 8/15/2022, Wesley Cheng wrote:
> Changes in v3:
> - Modified the msleep() duration to ~2s versus ~10s due to the minimum
> mdelay() value.
> - Removed patch to modify DEP flags during dwc3_stop_active_transfer().
> This was not required after fixing the logic to allow EP xfercomplete
> events to be handled on EP0.
> - Added some changes to account for a cable disconnect scenario, where
> dwc3_gadget_pullup() would not be executed to stop active transfers.
> Needed to add some logic to the disconnect interrupt to ensure that we
> cleanup/restart any pending SETUP transaction, so that we can clear the
> EP0 delayed stop status. (if pending)
> - Added patch to ensure that we don't proceed with umapping buffers
> until the endxfer was actually sent.
> 
> Changes in v2:
> - Moved msleep() to before reading status register for halted state
> - Fixed kernel bot errors
> - Clearing DEP flags in __dwc3_stop_active_transfers()
> - Added Suggested-by tags and link references to previous discussions
> 
> This patch series addresses some issues seen while testing with the latest
> soft disconnect implementation where EP events are allowed to process while
> the controller halt is occurring.
> 
> #1
> Since routines can now interweave, we can see that the soft disconnect can
> occur while conndone is being serviced.  This leads to a controller halt
> timeout, as the soft disconnect clears the DEP flags, for which conndone
> interrupt handler will issue a __dwc3_ep_enable(ep0), that leads to
> re-issuing the set ep config command for every endpoint.
> 
> #2
> Function drivers can ask for a delayed_status phase, while it processes the
> received SETUP packet.  This can lead to large delays when handling the
> soft disconnect routine.  To improve the timing, forcefully send the status
> phase, as we are going to disconnect from the host.
> 
> #3
> Ensure that local interrupts are left enabled, so that EP0 events can be
> processed while the soft disconnect/dequeue is happening.
> 
> #4
> Since EP0 events can occur during controller halt, it may increase the time
> needed for the controller to fully stop.
> 
> #5
> Account for cable disconnect scenarios where nothing may cause the endxfer
> retry if DWC3_EP_DELAY_STOP is set.
> 
> #6
> Avoid unmapping pending USB requests that were never stopped.  This would
> lead to a potential SMMU fault.
> 
> Wesley Cheng (8):
>   usb: dwc3: Do not service EP0 and conndone events if soft disconnected
>   usb: dwc3: gadget: Force sending delayed status during soft disconnect
>   usb: dwc3: gadget: Synchronize IRQ between soft connect/disconnect
>   usb: dwc3: gadget: Continue handling EP0 xfercomplete events
>   usb: dwc3: Avoid unmapping USB requests if endxfer is not complete
>   usb: dwc3: Increase DWC3 controller halt timeout
>   usb: dwc3: gadget: Skip waiting for CMDACT cleared during endxfer
>   usb: dwc3: gadget: Submit endxfer command if delayed during disconnect
> 
>  drivers/usb/dwc3/core.c   |  4 ----
>  drivers/usb/dwc3/core.h   |  3 +++
>  drivers/usb/dwc3/ep0.c    | 11 ++++++---
>  drivers/usb/dwc3/gadget.c | 48 +++++++++++++++++++++++++++++++++------
>  4 files changed, 52 insertions(+), 14 deletions(-)
> 

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

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

Thanks for the patches!
Thinh