Date:   Wed, 17 Aug 2022 00:39:31 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Wesley Cheng <quic_wcheng@...cinc.com>,
        "balbi@...nel.org" <balbi@...nel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "quic_jackp@...cinc.com" <quic_jackp@...cinc.com>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Subject: Re: [PATCH v3 0/8] Fix controller halt and endxfer timeout issues

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@...opsys.com>

Thanks for the patches!
Thinh

Powered by blists - more mailing lists