Re: [RFC PATCH v2 1/3] usb: dwc3: Flush pending SETUP data during stop active xfers

[Thinh Nguyen]

Hi Wesley,

Wesley Cheng wrote:
> Hi Thinh,
> 
> On 3/22/2022 6:31 PM, Thinh Nguyen wrote:
>> Hi Wesley,
>>
>> Wesley Cheng wrote:
>>> Hi Thinh,
>>>
>>> On 2/15/2022 4:08 PM, Wesley Cheng wrote:
>>>> While running the pullup disable sequence, if there are pending SETUP
>>>> transfers stored in the controller, then the ENDTRANSFER commands on non
>>>> control eps will fail w/ ETIMEDOUT.  As a suggestion from SNPS, in order
>>>> to drain potentially cached SETUP packets, SW needs to issue a
>>>> STARTTRANSFER command.  After issuing the STARTTRANSFER, and retrying the
>>>> ENDTRANSFER, the command should succeed.  Else, if the endpoints are not
>>>> properly stopped, the controller halt sequence will fail as well.
>>>>
>>>> One limitation is that the current logic will drop the SETUP data
>>>> being received (ie dropping the SETUP packet), however, it should be
>>>> acceptable in the pullup disable case, as the device is eventually
>>>> going to disconnect from the host.
>>>>
>>>> Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
>>>> ---
>>> Just wondering if you had any inputs for this particular change?  I
>>> think on the dequeue path discussion you had some concerns with parts of
>>> this change?  I think the difficult part for the pullup disable path is
>>> that we have this API running w/ interrupts disabled, so the EP0 state
>>> won't be able to advance compared to the dequeue case.
>>
>> This doesn't sound right. The pullup disable (or device initiated
>> disconnect) should wait for the EP0 state to be EP0_SETUP_PHASE before
>> proceeding, which it does.
>>
> That is correct, but even though that check is passed, it doesn't
> prevent the host from sending another SETUP token between the pending
> setup packet check and run/stop clear.
> 

That should be fine, because we would have the TRB ready for that SETUP
packet.

>>>
>>> Ideally, if there is a way to ensure that we avoid reading further setup
>>> packets, that would be nice, but from our discussions with Synopsys,
>>> this was not possible. (controller is always armed and ready to ACK
>>> setup tokens)
>>>
>>> I did evaluate keeping IRQs enabled and periodically releasing/attaining
>>> the lock between the stop active xfer calls, but that opened another can
>>> of worms.  If you think this is the approach we should take, I can take
>>> a look at this implementation further.
>>>
>>
>> This patch doesn't look right to me. The change I suggested before
>> should address this (I believe Greg already picked it up). What other
>> problem do you see, I'm not clear what's the problem here. One potential
>> problem that I can see is that currently dwc3 driver doesn't wait for
>> active endpoints to complete/end before clearing the run_stop bit on
>> device initiated disconnect, but I'm not sure if that's what you're seeing.
>>
>> Please help clarify further. If there's trace points log, that'd help.
>>
> Main difference between the issue Greg recently pulled in and this one
> is that this is on the pullup disable patch (no dequeue involved).  In
> this situation we will also stop active transfers, so that the
> controller can halt properly.
> 
> I attached a few files, and will give a summary of them below:
> 1.  pullup_disable_timeout.txt - ftrace of an instance of when we see
> several endxfer timeouts.  Refer to line#2016.
> 
> 2.  lecroy+ftrace_snip.png - picture showing a matching bus sniffer log
> and a ftrace collected (difference instance to #1)
> 
> #2 will show that we completed a SETUP transfer before entering into
> dwc3_gadget_stop_active_transfers().  In here, we then see DWC ACK
> another SETUP token, which leads to endxfer timeouts on all subsequent
> endpoints.

Thanks for the captures. I think the problem here is because the dwc3
driver disables the control endpoint. It shouldn't do that.

Can you try this:

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index ab725d2262d6..f0b9ea353620 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1010,12 +1010,12 @@ static int __dwc3_gadget_ep_disable(struct
dwc3_ep *dep)
        if (dep->flags & DWC3_EP_STALL)
                __dwc3_gadget_ep_set_halt(dep, 0, false);

-       reg = dwc3_readl(dwc->regs, DWC3_DALEPENA);
-       reg &= ~DWC3_DALEPENA_EP(dep->number);
-       dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
-
-       /* Clear out the ep descriptors for non-ep0 */
        if (dep->number > 1) {
+               reg = dwc3_readl(dwc->regs, DWC3_DALEPENA);
+               reg &= ~DWC3_DALEPENA_EP(dep->number);
+               dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
+
+               /* Clear out the ep descriptors for non-ep0 */
                dep->endpoint.comp_desc = NULL;
                dep->endpoint.desc = NULL;
        }


Thanks,
Thinh