[PATCH v2] usb: dwc3: gadget: Clear wait flag on dequeue

[PATCH v2] usb: dwc3: gadget: Clear wait flag on dequeue

[Thinh Nguyen]

If an active transfer is dequeued, then the endpoint is freed to start a
new transfer. Make sure to clear the endpoint's transfer wait flag for
this case.

Cc: stable@vger.kernel.org
Fixes: e0d19563eb6c ("usb: dwc3: gadget: Wait for transfer completion")
Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 Changes in v2:
 - Only clear the wait flag if the selected request is of an active transfer.
   Otherwise, any dequeue will change the endpoint's state even if it's for
   some random request.

 drivers/usb/dwc3/gadget.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 78cb4db8a6e4..9a00dcaca010 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1763,6 +1763,8 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 			list_for_each_entry_safe(r, t, &dep->started_list, list)
 				dwc3_gadget_move_cancelled_request(r);
 
+			dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
+
 			goto out;
 		}
 	}

base-commit: 2edc7af892d0913bf06f5b35e49ec463f03d5ed8
-- 
2.28.0

Re: [PATCH v2] usb: dwc3: gadget: Clear wait flag on dequeue

[Felipe Balbi]

Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> If an active transfer is dequeued, then the endpoint is freed to start a
> new transfer. Make sure to clear the endpoint's transfer wait flag for
> this case.
>
> Cc: stable@vger.kernel.org
> Fixes: e0d19563eb6c ("usb: dwc3: gadget: Wait for transfer completion")
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  Changes in v2:
>  - Only clear the wait flag if the selected request is of an active transfer.
>    Otherwise, any dequeue will change the endpoint's state even if it's for
>    some random request.
>
>  drivers/usb/dwc3/gadget.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 78cb4db8a6e4..9a00dcaca010 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1763,6 +1763,8 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>  			list_for_each_entry_safe(r, t, &dep->started_list, list)
>  				dwc3_gadget_move_cancelled_request(r);
>  
> +			dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;

I'm not sure this is correct. This could create a race condition between
clearing this bit and getting the transfer complete interrupt. It also
seems to break the assumptions made by
dwc3_gadget_endpoint_trbs_complete() (actually its users), specially
regarding ISOC endpoints.

Have you verified all transfer types with this commit?

-- 
balbi

Re: [PATCH v2] usb: dwc3: gadget: Clear wait flag on dequeue

[Thinh Nguyen]

Hi Felipe,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> If an active transfer is dequeued, then the endpoint is freed to start a
>> new transfer. Make sure to clear the endpoint's transfer wait flag for
>> this case.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: e0d19563eb6c ("usb: dwc3: gadget: Wait for transfer completion")
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> ---
>>  Changes in v2:
>>  - Only clear the wait flag if the selected request is of an active transfer.
>>    Otherwise, any dequeue will change the endpoint's state even if it's for
>>    some random request.
>>
>>  drivers/usb/dwc3/gadget.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 78cb4db8a6e4..9a00dcaca010 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1763,6 +1763,8 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>>  			list_for_each_entry_safe(r, t, &dep->started_list, list)
>>  				dwc3_gadget_move_cancelled_request(r);
>>  
>> +			dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
> I'm not sure this is correct. This could create a race condition between
> clearing this bit and getting the transfer complete interrupt. It also
> seems to break the assumptions made by
> dwc3_gadget_endpoint_trbs_complete() (actually its users), specially
> regarding ISOC endpoints.
>
> Have you verified all transfer types with this commit?
>

It shouldn't race. It's protected by the spinlock irq and it doesn't
matter whether dwc3_gadget_endpoint_trbs_complete() or this dequeue
function clears it first. The flag DWC3_EP_WAIT_TRANSFER_COMPLETE is
only applicable to stream transfer as the driver needs to wait for 1
stream to finish before starting another.

This is verified with our test environment (which includes UASP CV and
others).

BR,
Thinh

Re: [PATCH v2] usb: dwc3: gadget: Clear wait flag on dequeue

[Felipe Balbi]

Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> If an active transfer is dequeued, then the endpoint is freed to start a
>>> new transfer. Make sure to clear the endpoint's transfer wait flag for
>>> this case.
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: e0d19563eb6c ("usb: dwc3: gadget: Wait for transfer completion")
>>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>> ---
>>>  Changes in v2:
>>>  - Only clear the wait flag if the selected request is of an active transfer.
>>>    Otherwise, any dequeue will change the endpoint's state even if it's for
>>>    some random request.
>>>
>>>  drivers/usb/dwc3/gadget.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 78cb4db8a6e4..9a00dcaca010 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -1763,6 +1763,8 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
>>>  			list_for_each_entry_safe(r, t, &dep->started_list, list)
>>>  				dwc3_gadget_move_cancelled_request(r);
>>>  
>>> +			dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE;
>> I'm not sure this is correct. This could create a race condition between
>> clearing this bit and getting the transfer complete interrupt. It also
>> seems to break the assumptions made by
>> dwc3_gadget_endpoint_trbs_complete() (actually its users), specially
>> regarding ISOC endpoints.
>>
>> Have you verified all transfer types with this commit?
>>
>
> It shouldn't race. It's protected by the spinlock irq and it doesn't
> matter whether dwc3_gadget_endpoint_trbs_complete() or this dequeue
> function clears it first. The flag DWC3_EP_WAIT_TRANSFER_COMPLETE is
> only applicable to stream transfer as the driver needs to wait for 1
> stream to finish before starting another.
>
> This is verified with our test environment (which includes UASP CV and
> others).

Fair enough, I'll take your word for it :-)

Acked-by: Felipe Balbi <balbi@kernel.org>

-- 
balbi