diff mbox series

[4/5] usb: dwc3: Allow end transfer commands to be sent during soft disconnect

Message ID 20220708185007.21743-5-quic_wcheng@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Fix controller halt and endxfer timeout issues | expand

Commit Message

Wesley Cheng July 8, 2022, 6:50 p.m. UTC
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(-)

Comments

Thinh Nguyen July 9, 2022, 1:58 a.m. UTC | #1
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.

Thanks,
Thinh

>   }
>   
>   static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
Wesley Cheng July 12, 2022, 10:08 p.m. UTC | #2
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?

Thanks
Wesley Cheng
Thinh Nguyen July 13, 2022, 1:42 a.m. UTC | #3
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?

Thanks,
Thinh
Wesley Cheng July 13, 2022, 5:45 p.m. UTC | #4
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
Thinh Nguyen July 14, 2022, 12:19 a.m. UTC | #5
On 7/13/2022, Wesley Cheng wrote:
> 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.
>

I see. The soft-disconnect still go through right?
Can you try this and see if this works for you?

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 13e4f1a03417..4f67a484c490 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -451,7 +451,9 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, 
unsigned int cmd,

         dwc3_writel(dep->regs, DWC3_DEPCMD, cmd);

-       if (!(cmd & DWC3_DEPCMD_CMDACT)) {
+       if (!(cmd & DWC3_DEPCMD_CMDACT) ||
+           (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_ENDTRANSFER &&
+            !(cmd & DWC3_DEPCMD_CMDIOC))) {
                 ret = 0;
                 goto skip_status;
         }


We can set it and forget it when we're about to de-initialize the 
controller. Just like what we've already done by not waiting for the End 
Transfer endpoint command completion interrupt.

Thanks,
Thinh
Wesley Cheng July 14, 2022, 12:33 a.m. UTC | #6
Hi Thinh,

On 7/13/2022 5:19 PM, Thinh Nguyen wrote:
> On 7/13/2022, Wesley Cheng wrote:
>> 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.
>>
> 
> I see. The soft-disconnect still go through right?
> Can you try this and see if this works for you?
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 13e4f1a03417..4f67a484c490 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -451,7 +451,9 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep,
> unsigned int cmd,
> 
>           dwc3_writel(dep->regs, DWC3_DEPCMD, cmd);
> 
> -       if (!(cmd & DWC3_DEPCMD_CMDACT)) {
> +       if (!(cmd & DWC3_DEPCMD_CMDACT) ||
> +           (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_ENDTRANSFER &&
> +            !(cmd & DWC3_DEPCMD_CMDIOC))) {
>                   ret = 0;
>                   goto skip_status;
>           }
> 
> 
> We can set it and forget it when we're about to de-initialize the
> controller. Just like what we've already done by not waiting for the End
> Transfer endpoint command completion interrupt.
> 

I can see this working for the soft disconnect case, but we'll need to 
address it differently for the EP dequeue scenario where IOC/interrupt 
is required to clean up the TRBs.  This is one of the reasons why I went 
with the spinlock approach, since it would apply to both situations.

Thanks
Wesley Cheng
Thinh Nguyen July 14, 2022, 12:41 a.m. UTC | #7
On 7/13/2022, Wesley Cheng wrote:
> Hi Thinh,
>
> On 7/13/2022 5:19 PM, Thinh Nguyen wrote:
>> On 7/13/2022, Wesley Cheng wrote:
>>> 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.
>>>
>>
>> I see. The soft-disconnect still go through right?
>> Can you try this and see if this works for you?
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 13e4f1a03417..4f67a484c490 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -451,7 +451,9 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep,
>> unsigned int cmd,
>>
>>           dwc3_writel(dep->regs, DWC3_DEPCMD, cmd);
>>
>> -       if (!(cmd & DWC3_DEPCMD_CMDACT)) {
>> +       if (!(cmd & DWC3_DEPCMD_CMDACT) ||
>> +           (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_ENDTRANSFER &&
>> +            !(cmd & DWC3_DEPCMD_CMDIOC))) {
>>                   ret = 0;
>>                   goto skip_status;
>>           }
>>
>>
>> We can set it and forget it when we're about to de-initialize the
>> controller. Just like what we've already done by not waiting for the End
>> Transfer endpoint command completion interrupt.
>>
>
> I can see this working for the soft disconnect case, but we'll need to 
> address it differently for the EP dequeue scenario where IOC/interrupt 
> is required to clean up the TRBs.  This is one of the reasons why I 
> went with the spinlock approach, since it would apply to both situations.

I thought we already handled the case for ep dequeue from the last fix. 
Was that not addressed?

Thanks,
Thinh
diff mbox series

Patch

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;
 }
 
 static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)