lkml.org 
[lkml]   [2022]   [Jul]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 4/5] usb: dwc3: Allow end transfer commands to be sent during soft disconnect
From
Hi Thinh,

On 7/12/2022 6:42 PM, Thinh Nguyen wrote:
> On 7/12/2022, Wesley Cheng wrote:
>> Hi Thinh,
>>
>> On 7/8/2022 6:58 PM, Thinh Nguyen wrote:
>>> On 7/8/2022, Wesley Cheng wrote:
>>>> If soft disconnect is in progress, allow the endxfer command to be
>>>> sent,
>>>> without this, there is an issue where the stop active transfer call
>>>> (during pullup disable) wouldn't actually issue the endxfer command,
>>>> while clearing the DEP flag.
>>>>
>>>> In addition, if the DWC3_EP_DELAY_STOP flag was set before soft
>>>> disconnect
>>>> started (i.e. from the dequeue path), ensure that when the EP0
>>>> transaction
>>>> completes during soft disconnect, to issue the endxfer with the force
>>>> parameter set, as it does not expect a command complete event.
>>>>
>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>> ---
>>>>    drivers/usb/dwc3/ep0.c    | 3 +--
>>>>    drivers/usb/dwc3/gadget.c | 5 ++++-
>>>>    2 files changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>>>> index 506ef717fdc0..5851b0e9db0a 100644
>>>> --- a/drivers/usb/dwc3/ep0.c
>>>> +++ b/drivers/usb/dwc3/ep0.c
>>>> @@ -290,8 +290,7 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
>>>>            if (!(dwc3_ep->flags & DWC3_EP_DELAY_STOP))
>>>>                continue;
>>>>    -        dwc3_ep->flags &= ~DWC3_EP_DELAY_STOP;
>>>> -        dwc3_stop_active_transfer(dwc3_ep, true, true);
>>>> +        dwc3_stop_active_transfer(dwc3_ep, true, dwc->connected);
>>>>        }
>>>>    }
>>>>    diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index bd40608b19df..fba2797ad9ae 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -3696,8 +3696,10 @@ void dwc3_stop_active_transfer(struct dwc3_ep
>>>> *dep, bool force,
>>>>        if (dep->number <= 1 && dwc->ep0state != EP0_DATA_PHASE)
>>>>            return;
>>>>    +    if (interrupt && (dep->flags & DWC3_EP_DELAY_STOP))
>>>> +        return;
>>>> +
>>>>        if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>>> -        (dep->flags & DWC3_EP_DELAY_STOP) ||
>>>>            (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>>>            return;
>>>>    @@ -3744,6 +3746,7 @@ void dwc3_stop_active_transfer(struct
>>>> dwc3_ep *dep, bool force,
>>>>        __dwc3_stop_active_transfer(dep, force, interrupt);
>>>>        spin_lock(&dwc->lock);
>>>>    +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>
>>> Can we clear this flag in __dwc3_stop_active_transfer(). It should apply
>>> if End Transfer command was sent.
>>
>> I wanted to make sure that we weren't modifying the DEP flags outside
>> of a spin lock.  Patch#3 modifies it where we unlock before calling
>> __dwc3_stop_active_transfer(), so we can allow the dwc3 threaded IRQ
>> handle events while the cmd status polling happens.
>>
>> Maybe we can unlock/lock the dwc3->lock inside
>> __dwc3_stop_active_transfer() and that way we can ensure DEP flags are
>> modified properly?
>
> I didn't realize that you unlock/lock when calling
> __dwc3_stop_active_transfer(). We'd need to be careful if we want to
> unlock/lock it, and avoid it all together if possible. It can be easily
> overlooked just like dwc3_gadget_giveback().
>
> What issue did you see without doing this?

I saw endxfer timeout issues if I didn't do it. If we keep the lock
held, then the DWC3 event processing would be blocked across the entire
time we are waiting for the command act to clear. With unlocking before
polling, then at least we're still able to handle the EP0 events that
are pending.

It was definitely one of the harder scenarios to reproduce. The main
patch series which resolved a lot of the issues early on was patch#1.
After adding that the other issues are seen maybe after a day or so of
testing.

Thanks
Wesley Cheng

\
 
 \ /
  Last update: 2022-07-13 19:46    [W:0.089 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site