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

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(-)

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

On Thu, Apr 21, 2022 at 07:23:03PM -0700, 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;
> +	}
> +

dwc3_remove_requests() calls dwc3_stop_active_transfer() but does not check
any flags before retiring all the requests. should we add some assert/WARN_ON
to make sure that we are not retiring the requests before stopping the active
transfers?

Thanks,
Pavan

Pavan Kondeti wrote:
> On Thu, Apr 21, 2022 at 07:23:03PM -0700, 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;
>> +	}
>> +
> 
> dwc3_remove_requests() calls dwc3_stop_active_transfer() but does not check
> any flags before retiring all the requests. should we add some assert/WARN_ON
> to make sure that we are not retiring the requests before stopping the active
> transfers?
> 

Why? Do you see a problem with it?

Ideally we should wait for the controller to release the TRBs before
returning the requests to the upperlayer, and only force retire the
requests on timeout. However, in the case of dwc3_remove_requests(), it
should only be called when there's a disconnect or de-initialization of
the controller. The dwc3 driver reports -ESHUTDOWN error in the requests
to the function driver. The function driver shouldn't be using/reusing
the request buffer as it should be doing driver tear down/cleanup at
this point.

I haven't seen a problem handling it this way yet.

From what I recall, previously, the reason we had it this way is because
the events generated from End Transfer completion prevents the
controller from halting. However, with this rework of pullup(), it
should be able to handle that case now. So we can create a patch to make
sure that we handle all End Transfer completion cases.

Thanks,
Thinh