Hi Wesley,
Wesley Cheng wrote:
> 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@xxxxxxxxxxxx>
>> ---
>> 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?
It should be fine even the End Transfer completes after the run_stop bit
is clear. Clearing the run_stop would stop activity because the host is
disconnected. But the controller can still assert interrupt if there are
pending events even after the run_stop bit is cleared.
>
> 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)
No, it shouldn't advance. The patch "[PATCH 4/6] usb: dwc3: ep0: Don't
prepare beyond Setup stage" will cause the controller to respond a STALL
to any new control transfer and put it back and prepare a new TRB for a
new SETUP packet.
>
> /*
> * 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.
Because we make sure ep0state won't advance beyond EP0_SETUP_PHASE, we
don't have to worry about DWC3_EP_DELAY_STOP.
>
> __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 for testing!
BR,
Thinh
