diff mbox series

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

Message ID 20220713003523.29309-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 13, 2022, 12:35 a.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.

Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to complete during dequeue")
Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
Link:
  https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/

 drivers/usb/dwc3/ep0.c    | 3 +--
 drivers/usb/dwc3/gadget.c | 5 ++++-
 2 files changed, 5 insertions(+), 3 deletions(-)

Comments

Thinh Nguyen July 21, 2022, 10:08 p.m. UTC | #1
Hi Wesley,

On 7/12/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.
>
> Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to complete during dequeue")
> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
> Link:
>    https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/__;!!A4F2R9G_pg!cXW2TlALYWnXNPg-NoMFUrQ8K1egEZiQizZ5CA1DOM1Gcw4tfOULy-_2eMGvL8pQPte5dScFON-0wxH2eXu8ijEQUbs$
>
>   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;

If we don't clear this flag here,

> -		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 ee85b773e3fe..41b7007358de 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1693,6 +1693,7 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
>   		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>   	else if (!ret)
>   		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
> +	dep->flags &= ~DWC3_EP_DELAY_STOP;
>   
>   	return ret;
>   }
> @@ -3686,8 +3687,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;
> +

then it may not go through here. I think I made this same mistake in my 
previous suggestion.

>   	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
> -	    (dep->flags & DWC3_EP_DELAY_STOP) ||
>   	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>   		return;
>   

Thanks,
Thinh
Wesley Cheng July 21, 2022, 10:13 p.m. UTC | #2
Hi Thinh,

On 7/21/2022 3:08 PM, Thinh Nguyen wrote:
> Hi Wesley,
> 
> On 7/12/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.
>>
>> Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to complete during dequeue")
>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>> Link:
>>     https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/__;!!A4F2R9G_pg!cXW2TlALYWnXNPg-NoMFUrQ8K1egEZiQizZ5CA1DOM1Gcw4tfOULy-_2eMGvL8pQPte5dScFON-0wxH2eXu8ijEQUbs$
>>
>>    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;
> 
> If we don't clear this flag here,
> 

This is why I added the dwc->connected argument to control the 
"interrupt" argument.

>> -		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 ee85b773e3fe..41b7007358de 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1693,6 +1693,7 @@ static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
>>    		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>>    	else if (!ret)
>>    		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
>> +	dep->flags &= ~DWC3_EP_DELAY_STOP;
>>    
>>    	return ret;
>>    }
>> @@ -3686,8 +3687,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;
>> +
> 
> then it may not go through here. I think I made this same mistake in my
> previous suggestion.
> 

Since dwc->connected is set to FALSE before we call stop active 
transfers, if we ever run into a situation where delayed stop is set 
already, then it should go through.

Thanks
Wesley Cheng
Thinh Nguyen July 21, 2022, 10:20 p.m. UTC | #3
On 7/21/2022, Wesley Cheng wrote:
> Hi Thinh,
>
> On 7/21/2022 3:08 PM, Thinh Nguyen wrote:
>> Hi Wesley,
>>
>> On 7/12/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.
>>>
>>> Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to 
>>> complete during dequeue")
>>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>> ---
>>> Link:
>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/__;!!A4F2R9G_pg!cXW2TlALYWnXNPg-NoMFUrQ8K1egEZiQizZ5CA1DOM1Gcw4tfOULy-_2eMGvL8pQPte5dScFON-0wxH2eXu8ijEQUbs$
>>>
>>>    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;
>>
>> If we don't clear this flag here,
>>
>
> This is why I added the dwc->connected argument to control the 
> "interrupt" argument.
>
>>> - 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 ee85b773e3fe..41b7007358de 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -1693,6 +1693,7 @@ static int __dwc3_stop_active_transfer(struct 
>>> dwc3_ep *dep, bool force, bool int
>>>            dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>>>        else if (!ret)
>>>            dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
>>> +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>           return ret;
>>>    }
>>> @@ -3686,8 +3687,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;
>>> +
>>
>> then it may not go through here. I think I made this same mistake in my
>> previous suggestion.
>>
>
> Since dwc->connected is set to FALSE before we call stop active 
> transfers, if we ever run into a situation where delayed stop is set 
> already, then it should go through.
>

Then the check for request dequeue that we did previously will not work 
anymore.

BR,
Thinh
Wesley Cheng July 21, 2022, 10:40 p.m. UTC | #4
Hi Thinh,

On 7/21/2022 3:20 PM, Thinh Nguyen wrote:
> On 7/21/2022, Wesley Cheng wrote:
>> Hi Thinh,
>>
>> On 7/21/2022 3:08 PM, Thinh Nguyen wrote:
>>> Hi Wesley,
>>>
>>> On 7/12/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.
>>>>
>>>> Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to
>>>> complete during dequeue")
>>>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>> ---
>>>> Link:
>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/__;!!A4F2R9G_pg!cXW2TlALYWnXNPg-NoMFUrQ8K1egEZiQizZ5CA1DOM1Gcw4tfOULy-_2eMGvL8pQPte5dScFON-0wxH2eXu8ijEQUbs$
>>>>
>>>>     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;
>>>
>>> If we don't clear this flag here,
>>>
>>
>> This is why I added the dwc->connected argument to control the
>> "interrupt" argument.
>>
>>>> - 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 ee85b773e3fe..41b7007358de 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -1693,6 +1693,7 @@ static int __dwc3_stop_active_transfer(struct
>>>> dwc3_ep *dep, bool force, bool int
>>>>             dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>>>>         else if (!ret)
>>>>             dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
>>>> +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>            return ret;
>>>>     }
>>>> @@ -3686,8 +3687,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;
>>>> +
>>>
>>> then it may not go through here. I think I made this same mistake in my
>>> previous suggestion.
>>>
>>
>> Since dwc->connected is set to FALSE before we call stop active
>> transfers, if we ever run into a situation where delayed stop is set
>> already, then it should go through.
>>
> 
> Then the check for request dequeue that we did previously will not work
> anymore.
> 

Could you help clarify?  The pullup refactor kind of shifted some of the 
previous checks we had in the dequeue path, and combined with with the 
stop active transfer checks.

I think there was an issue w/ the patch I submitted though.  the above 
snippet should be replacing what is there:

void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
	bool interrupt)
{
...
	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
	    /* Following should be removed --- (dep->flags & 
DWC3_EP_DELAY_STOP) || */
	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
		return;

Thanks
Wesley Cheng
Thinh Nguyen July 22, 2022, midnight UTC | #5
On 7/21/2022, Wesley Cheng wrote:
> Hi Thinh,
>
> On 7/21/2022 3:20 PM, Thinh Nguyen wrote:
>> On 7/21/2022, Wesley Cheng wrote:
>>> Hi Thinh,
>>>
>>> On 7/21/2022 3:08 PM, Thinh Nguyen wrote:
>>>> Hi Wesley,
>>>>
>>>> On 7/12/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.
>>>>>
>>>>> Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to
>>>>> complete during dequeue")
>>>>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>>> ---
>>>>> Link:
>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/__;!!A4F2R9G_pg!cXW2TlALYWnXNPg-NoMFUrQ8K1egEZiQizZ5CA1DOM1Gcw4tfOULy-_2eMGvL8pQPte5dScFON-0wxH2eXu8ijEQUbs$ 
>>>>>
>>>>>
>>>>>     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;
>>>>
>>>> If we don't clear this flag here,
>>>>
>>>
>>> This is why I added the dwc->connected argument to control the
>>> "interrupt" argument.
>>>
>>>>> - 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 ee85b773e3fe..41b7007358de 100644
>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>> @@ -1693,6 +1693,7 @@ static int __dwc3_stop_active_transfer(struct
>>>>> dwc3_ep *dep, bool force, bool int
>>>>>             dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>>>>>         else if (!ret)
>>>>>             dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
>>>>> +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>>            return ret;
>>>>>     }
>>>>> @@ -3686,8 +3687,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;
>>>>> +
>>>>
>>>> then it may not go through here. I think I made this same mistake 
>>>> in my
>>>> previous suggestion.
>>>>
>>>
>>> Since dwc->connected is set to FALSE before we call stop active
>>> transfers, if we ever run into a situation where delayed stop is set
>>> already, then it should go through.
>>>
>>
>> Then the check for request dequeue that we did previously will not work
>> anymore.
>>
>
> Could you help clarify?  The pullup refactor kind of shifted some of 
> the previous checks we had in the dequeue path, and combined with with 
> the stop active transfer checks.
>
> I think there was an issue w/ the patch I submitted though.  the above 
> snippet should be replacing what is there:
>
> void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>     bool interrupt)
> {
> ...
>     if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>         /* Following should be removed --- (dep->flags & 
> DWC3_EP_DELAY_STOP) || */
>         (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>         return;
>

Request dequeue can occur while the device is connected. The 
DWC3_EP_DELAY_STOP intention is to delay sending the End Transfer 
command until the SETUP stage is prepared. If we don't clear the flag 
before the flag check, then the End Transfer command will not go through.

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

On 7/21/2022 5:00 PM, Thinh Nguyen wrote:
> On 7/21/2022, Wesley Cheng wrote:
>> Hi Thinh,
>>
>> On 7/21/2022 3:20 PM, Thinh Nguyen wrote:
>>> On 7/21/2022, Wesley Cheng wrote:
>>>> Hi Thinh,
>>>>
>>>> On 7/21/2022 3:08 PM, Thinh Nguyen wrote:
>>>>> Hi Wesley,
>>>>>
>>>>> On 7/12/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.
>>>>>>
>>>>>> Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to
>>>>>> complete during dequeue")
>>>>>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>>>> ---
>>>>>> Link:
>>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/__;!!A4F2R9G_pg!cXW2TlALYWnXNPg-NoMFUrQ8K1egEZiQizZ5CA1DOM1Gcw4tfOULy-_2eMGvL8pQPte5dScFON-0wxH2eXu8ijEQUbs$
>>>>>>
>>>>>>
>>>>>>      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;
>>>>>
>>>>> If we don't clear this flag here,
>>>>>
>>>>
>>>> This is why I added the dwc->connected argument to control the
>>>> "interrupt" argument.
>>>>
>>>>>> - 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 ee85b773e3fe..41b7007358de 100644
>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>> @@ -1693,6 +1693,7 @@ static int __dwc3_stop_active_transfer(struct
>>>>>> dwc3_ep *dep, bool force, bool int
>>>>>>              dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>>>>>>          else if (!ret)
>>>>>>              dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
>>>>>> +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>>>             return ret;
>>>>>>      }
>>>>>> @@ -3686,8 +3687,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;
>>>>>> +
>>>>>
>>>>> then it may not go through here. I think I made this same mistake
>>>>> in my
>>>>> previous suggestion.
>>>>>
>>>>
>>>> Since dwc->connected is set to FALSE before we call stop active
>>>> transfers, if we ever run into a situation where delayed stop is set
>>>> already, then it should go through.
>>>>
>>>
>>> Then the check for request dequeue that we did previously will not work
>>> anymore.
>>>
>>
>> Could you help clarify?  The pullup refactor kind of shifted some of
>> the previous checks we had in the dequeue path, and combined with with
>> the stop active transfer checks.
>>
>> I think there was an issue w/ the patch I submitted though.  the above
>> snippet should be replacing what is there:
>>
>> void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>>      bool interrupt)
>> {
>> ...
>>      if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>          /* Following should be removed --- (dep->flags &
>> DWC3_EP_DELAY_STOP) || */
>>          (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>          return;
>>
> 
> Request dequeue can occur while the device is connected. The
> DWC3_EP_DELAY_STOP intention is to delay sending the End Transfer
> command until the SETUP stage is prepared. If we don't clear the flag
> before the flag check, then the End Transfer command will not go through.
> 

Thanks, got it.  Understand what you mean now.  Let me think about how 
to go about it.  I probably don't see any issues as of now, because my 
test does the soft disconnect, which will do the stop active transfers 
forcefully.

Thanks
Wesley Cheng
Wesley Cheng July 22, 2022, 2:27 a.m. UTC | #7
Hi Thinh,

On 7/21/2022 5:04 PM, Wesley Cheng wrote:
> Hi Thinh,
> 
> On 7/21/2022 5:00 PM, Thinh Nguyen wrote:
>> On 7/21/2022, Wesley Cheng wrote:
>>> Hi Thinh,
>>>
>>> On 7/21/2022 3:20 PM, Thinh Nguyen wrote:
>>>> On 7/21/2022, Wesley Cheng wrote:
>>>>> Hi Thinh,
>>>>>
>>>>> On 7/21/2022 3:08 PM, Thinh Nguyen wrote:
>>>>>> Hi Wesley,
>>>>>>
>>>>>> On 7/12/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.
>>>>>>>
>>>>>>> Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to
>>>>>>> complete during dequeue")
>>>>>>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>>>>> ---
>>>>>>> Link:
>>>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/__;!!A4F2R9G_pg!cXW2TlALYWnXNPg-NoMFUrQ8K1egEZiQizZ5CA1DOM1Gcw4tfOULy-_2eMGvL8pQPte5dScFON-0wxH2eXu8ijEQUbs$ 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>      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;
>>>>>>
>>>>>> If we don't clear this flag here,
>>>>>>
>>>>>
>>>>> This is why I added the dwc->connected argument to control the
>>>>> "interrupt" argument.
>>>>>
>>>>>>> - 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 ee85b773e3fe..41b7007358de 100644
>>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>>> @@ -1693,6 +1693,7 @@ static int __dwc3_stop_active_transfer(struct
>>>>>>> dwc3_ep *dep, bool force, bool int
>>>>>>>              dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>>>>>>>          else if (!ret)
>>>>>>>              dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
>>>>>>> +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>>>>             return ret;
>>>>>>>      }
>>>>>>> @@ -3686,8 +3687,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;
>>>>>>> +
>>>>>>
>>>>>> then it may not go through here. I think I made this same mistake
>>>>>> in my
>>>>>> previous suggestion.
>>>>>>
>>>>>
>>>>> Since dwc->connected is set to FALSE before we call stop active
>>>>> transfers, if we ever run into a situation where delayed stop is set
>>>>> already, then it should go through.
>>>>>
>>>>
>>>> Then the check for request dequeue that we did previously will not work
>>>> anymore.
>>>>
>>>
>>> Could you help clarify?  The pullup refactor kind of shifted some of
>>> the previous checks we had in the dequeue path, and combined with with
>>> the stop active transfer checks.
>>>
>>> I think there was an issue w/ the patch I submitted though.  the above
>>> snippet should be replacing what is there:
>>>
>>> void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>>>      bool interrupt)
>>> {
>>> ...
>>>      if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>>          /* Following should be removed --- (dep->flags &
>>> DWC3_EP_DELAY_STOP) || */
>>>          (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>>          return;
>>>
>>
>> Request dequeue can occur while the device is connected. The
>> DWC3_EP_DELAY_STOP intention is to delay sending the End Transfer
>> command until the SETUP stage is prepared. If we don't clear the flag
>> before the flag check, then the End Transfer command will not go through.
>>
> 
> Thanks, got it.  Understand what you mean now.  Let me think about how 
> to go about it.  I probably don't see any issues as of now, because my 
> test does the soft disconnect, which will do the stop active transfers 
> forcefully.
> 
What do you think about just removing the

(dep->flags & DWC3_EP_DELAY_STOP)

check?  In the end, as long as the conditions are satisfied (ie we 
aren't in the middle of a SETUP transaction) then we don't care too much 
about who called dwc3_stop_active_transfer().  We would still be able to 
avoid issuing the endxfer here:

	if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status) {
		dep->flags |= DWC3_EP_DELAY_STOP;
		return 0;
	}


Thanks
Wesley Cheng
Thinh Nguyen July 22, 2022, 7:59 p.m. UTC | #8
On 7/21/2022, Wesley Cheng wrote:
> Hi Thinh,
>
> On 7/21/2022 5:04 PM, Wesley Cheng wrote:
>> Hi Thinh,
>>
>> On 7/21/2022 5:00 PM, Thinh Nguyen wrote:
>>> On 7/21/2022, Wesley Cheng wrote:
>>>> Hi Thinh,
>>>>
>>>> On 7/21/2022 3:20 PM, Thinh Nguyen wrote:
>>>>> On 7/21/2022, Wesley Cheng wrote:
>>>>>> Hi Thinh,
>>>>>>
>>>>>> On 7/21/2022 3:08 PM, Thinh Nguyen wrote:
>>>>>>> Hi Wesley,
>>>>>>>
>>>>>>> On 7/12/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.
>>>>>>>>
>>>>>>>> Fixes: e4cf6580ac740 ("usb: dwc3: gadget: Wait for ep0 xfers to
>>>>>>>> complete during dequeue")
>>>>>>>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>>>>>> ---
>>>>>>>> Link:
>>>>>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1a1a5485-790e-79ce-f5a6-1e96d9b49a47@synopsys.com/__;!!A4F2R9G_pg!cXW2TlALYWnXNPg-NoMFUrQ8K1egEZiQizZ5CA1DOM1Gcw4tfOULy-_2eMGvL8pQPte5dScFON-0wxH2eXu8ijEQUbs$ 
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>      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;
>>>>>>>
>>>>>>> If we don't clear this flag here,
>>>>>>>
>>>>>>
>>>>>> This is why I added the dwc->connected argument to control the
>>>>>> "interrupt" argument.
>>>>>>
>>>>>>>> - 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 ee85b773e3fe..41b7007358de 100644
>>>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>>>> @@ -1693,6 +1693,7 @@ static int 
>>>>>>>> __dwc3_stop_active_transfer(struct
>>>>>>>> dwc3_ep *dep, bool force, bool int
>>>>>>>>              dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
>>>>>>>>          else if (!ret)
>>>>>>>>              dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
>>>>>>>> +    dep->flags &= ~DWC3_EP_DELAY_STOP;
>>>>>>>>             return ret;
>>>>>>>>      }
>>>>>>>> @@ -3686,8 +3687,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;
>>>>>>>> +
>>>>>>>
>>>>>>> then it may not go through here. I think I made this same mistake
>>>>>>> in my
>>>>>>> previous suggestion.
>>>>>>>
>>>>>>
>>>>>> Since dwc->connected is set to FALSE before we call stop active
>>>>>> transfers, if we ever run into a situation where delayed stop is set
>>>>>> already, then it should go through.
>>>>>>
>>>>>
>>>>> Then the check for request dequeue that we did previously will not 
>>>>> work
>>>>> anymore.
>>>>>
>>>>
>>>> Could you help clarify?  The pullup refactor kind of shifted some of
>>>> the previous checks we had in the dequeue path, and combined with with
>>>> the stop active transfer checks.
>>>>
>>>> I think there was an issue w/ the patch I submitted though. the above
>>>> snippet should be replacing what is there:
>>>>
>>>> void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>>>>      bool interrupt)
>>>> {
>>>> ...
>>>>      if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>>>>          /* Following should be removed --- (dep->flags &
>>>> DWC3_EP_DELAY_STOP) || */
>>>>          (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>>>>          return;
>>>>
>>>
>>> Request dequeue can occur while the device is connected. The
>>> DWC3_EP_DELAY_STOP intention is to delay sending the End Transfer
>>> command until the SETUP stage is prepared. If we don't clear the flag
>>> before the flag check, then the End Transfer command will not go 
>>> through.
>>>
>>
>> Thanks, got it.  Understand what you mean now.  Let me think about 
>> how to go about it.  I probably don't see any issues as of now, 
>> because my test does the soft disconnect, which will do the stop 
>> active transfers forcefully.
>>
> What do you think about just removing the
>
> (dep->flags & DWC3_EP_DELAY_STOP)
>
> check?  In the end, as long as the conditions are satisfied (ie we 
> aren't in the middle of a SETUP transaction) then we don't care too 
> much about who called dwc3_stop_active_transfer().  We would still be 
> able to avoid issuing the endxfer here:
>
>     if (dwc->ep0state != EP0_SETUP_PHASE && !dwc->delayed_status) {
>         dep->flags |= DWC3_EP_DELAY_STOP;
>         return 0;
>     }

That's good. Then check for interrupt-on-completion there instead.

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 ee85b773e3fe..41b7007358de 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1693,6 +1693,7 @@  static int __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool int
 		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
 	else if (!ret)
 		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
+	dep->flags &= ~DWC3_EP_DELAY_STOP;
 
 	return ret;
 }
@@ -3686,8 +3687,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;