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

From: Thinh Nguyen
Date: Tue Aug 16 2022 - 20:39:56 EST


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@xxxxxxxxxxxx>

Thanks for the patches!
Thinh