Re: [PATCH v3 2/3] usb: dwc3: gadget: Stall and restart EP0 if host is unresponsive

[Thinh Nguyen]

On Mon, Apr 10, 2023, Wesley Cheng wrote:
> It was observed that there are hosts that may complete pending SETUP
> transactions before the stop active transfers and controller halt occurs,
> leading to lingering endxfer commands on DEPs on subsequent pullup/gadget
> start iterations.
> 
>   dwc3_gadget_ep_disable   name=ep8in flags=0x3009  direction=1
>   dwc3_gadget_ep_disable   name=ep4in flags=1  direction=1
>   dwc3_gadget_ep_disable   name=ep3out flags=1  direction=0
>   usb_gadget_disconnect   deactivated=0  connected=0  ret=0
> 
> The sequence shows that the USB gadget disconnect (dwc3_gadget_pullup(0))
> routine completed successfully, allowing for the USB gadget to proceed with
> a USB gadget connect.  However, if this occurs the system runs into an
> issue where:
> 
>   BUG: spinlock already unlocked on CPU
>   spin_bug+0x0
>   dwc3_remove_requests+0x278
>   dwc3_ep0_out_start+0xb0
>   __dwc3_gadget_start+0x25c
> 
> This is due to the pending endxfers, leading to gadget start (w/o lock
> held) to execute the remove requests, which will unlock the dwc3
> spinlock as part of giveback.
> 
> To mitigate this, resolve the pending endxfers on the pullup disable
> path by re-locating the SETUP phase check after stop active transfers, since
> that is where the DWC3_EP_DELAY_STOP is potentially set.  This also allows
> for handling of a host that may be unresponsive by using the completion
> timeout to trigger the stall and restart for EP0.
> 
> Fixes: c96683798e27 ("usb: dwc3: ep0: Don't prepare beyond Setup stage")
> Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
> ---
>  drivers/usb/dwc3/gadget.c | 42 +++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 320e30476c88..91768f1bdbaf 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2546,29 +2546,17 @@ static int __dwc3_gadget_start(struct dwc3 *dwc);
>  static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
>  {
>  	unsigned long flags;
> +	int ret;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	dwc->connected = false;
>  
>  	/*
> -	 * Per databook, when we want to stop the gadget, if a control transfer
> -	 * is still in process, complete it and get the core into setup phase.
> +	 * Attempt to end pending SETUP status phase, and not wait for the
> +	 * function to do so.
>  	 */
> -	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);
> -		ret = wait_for_completion_timeout(&dwc->ep0_in_setup,
> -				msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT));
> -		spin_lock_irqsave(&dwc->lock, flags);
> -		if (ret == 0)
> -			dev_warn(dwc->dev, "timed out waiting for SETUP phase\n");
> -	}
> +	if (dwc->delayed_status)
> +		dwc3_ep0_send_delayed_status(dwc);
>  
>  	/*
>  	 * In the Synopsys DesignWare Cores USB3 Databook Rev. 3.30a
> @@ -2581,6 +2569,26 @@ static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc)
>  	__dwc3_gadget_stop(dwc);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +	/*
> +	 * Per databook, when we want to stop the gadget, if a control transfer
> +	 * is still in process, complete it and get the core into setup phase.
> +	 * In case the host is unresponsive to a SETUP transaction, forcefully
> +	 * stall the transfer, and move back to the SETUP phase, so that any
> +	 * pending endxfers can be executed.
> +	 */
> +	if (dwc->ep0state != EP0_SETUP_PHASE) {
> +		reinit_completion(&dwc->ep0_in_setup);
> +
> +		ret = wait_for_completion_timeout(&dwc->ep0_in_setup,
> +				msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT));
> +		if (ret == 0) {
> +			dev_warn(dwc->dev, "wait for SETUP phase timed out\n");
> +			spin_lock_irqsave(&dwc->lock, flags);
> +			dwc3_ep0_reset_state(dwc);
> +			spin_unlock_irqrestore(&dwc->lock, flags);
> +		}
> +	}
> +
>  	/*
>  	 * Note: if the GEVNTCOUNT indicates events in the event buffer, the
>  	 * driver needs to acknowledge them before the controller can halt.

Acked-by: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>

Thanks,
Thinh