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@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;
>> + }
>> +
>
> 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
