Re: [PATCH 6/6] usb: dwc3: gadget: Delay issuing End Transfer

[Wesley Cheng <quic_wcheng@quicinc.com>]

Hi Thinh,

On 4/21/2022 7:23 PM, Thinh Nguyen wrote:
> If the controller hasn't DMA'ed the Setup data from its fifo, it won't
> process the End Transfer command. Polling for the command completion may
> block the driver from servicing the Setup phase and cause a timeout.
> Previously we only check and delay issuing End Transfer in the case of
> endpoint dequeue. Let's do that for all End Transfer scenarios.
> 
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>   drivers/usb/dwc3/gadget.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 7c4d5f671687..f0fd7c32828b 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2056,16 +2056,6 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>   		if (r == req) {
>   			struct dwc3_request *t;
>   
> -			/*
> -			 * If a Setup packet is received but yet to DMA out, the controller will
> -			 * not process the End Transfer command of any endpoint. Polling of its
> -			 * DEPCMD.CmdAct may block setting up TRB for Setup packet, causing a
> -			 * timeout. Delay issuing the End Transfer command until the Setup TRB is
> -			 * prepared.
> -			 */
> -			if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status)
> -				dep->flags |= DWC3_EP_DELAY_STOP;
> -
>   			/* wait until it is processed */
>   			dwc3_stop_active_transfer(dep, true, true);
>   
> @@ -3657,6 +3647,18 @@ void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>   	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>   		return;
>   
> +	/*
> +	 * If a Setup packet is received but yet to DMA out, the controller will
> +	 * not process the End Transfer command of any endpoint. Polling of its
> +	 * DEPCMD.CmdAct may block setting up TRB for Setup packet, causing a
> +	 * timeout. Delay issuing the End Transfer command until the Setup TRB is
> +	 * prepared.
> +	 */
> +	if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status) {
> +		dep->flags |= DWC3_EP_DELAY_STOP;
> +		return;
> +	}
> +

Since we could be calling dwc3_stop_active_transfer() as part of the 
dwc3_gadget_pullup(0) case (due to dwc3_stop_active_transfers()), how do 
we ensure that all active EPs are stopped before calling run/stop clear?

static int dwc3_gadget_soft_disconnect(struct dwc3 *dwc) {
...
        if (dwc->ep0state != EP0_SETUP_PHASE) {
                int ret;

                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");
        }

---> We know that ep0state will be in SETUP phase now, but host can send 
the SETUP data shortly after here.  This may cause some endpoints in the 
below dwc3_stop_active_transfers() call to mark DWC3_EP_DELAY_STOP. 
(ep0state could advance as we call gadget giveback in between servicing 
each dep, and lock is released briefly)
	
        /*
         * In the Synopsys DesignWare Cores USB3 Databook Rev. 3.30a
         * Section 4.1.8 Table 4-7, it states that for a device-initiated
         * disconnect, the SW needs to ensure that it sends "a DEPENDXFER
         * command for any active transfers" before clearing the RunStop
         * bit.
         */
	dwc3_stop_active_transfers(dwc);

---> Do we need to add some synchronization here to make sure that all 
EPs that had the DWC3_EP_DELAY_STOP had been serviced by the status 
phase complete handler?  Otherwise, we will continue to try to halt the 
controller, which will fail since there could still be EPs which are active.

	__dwc3_gadget_stop(dwc);
	spin_unlock_irqrestore(&dwc->lock, flags);

	return dwc3_gadget_run_stop(dwc, false, false);
}

I haven't run into this particular scenario yet, but thought I'd ask to 
see if there was some flow that I missed.

Thanks
Wesley Cheng