[PATCH] usb: dwc3: gadget: Avoid canceling current request for queuing error

[PATCH] usb: dwc3: gadget: Avoid canceling current request for queuing error

[Wesley Cheng]

If an error is received when issuing a start or update transfer
command, the error handler will stop all active requests (including
the current USB request), and call dwc3_gadget_giveback() to notify
function drivers of the requests which have been stopped.  Avoid
having to cancel the current request which is trying to be queued, as
the function driver will handle the EP queue error accordingly.
Simply unmap the request as it was done before, and allow previously
started transfers to be cleaned up.

Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
---
 drivers/usb/dwc3/gadget.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index e1b04c97..4200775 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
 		if (ret == -EAGAIN)
 			return ret;
 
+		/* Avoid canceling current request, as it has not been started */
+		if (req->trb)
+			memset(req->trb, 0, sizeof(struct dwc3_trb));
+		dwc3_gadget_del_and_unmap_request(dep, req, ret);
+
 		dwc3_stop_active_transfer(dep, true, true);
 
 		list_for_each_entry_safe(req, tmp, &dep->started_list, list)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] usb: dwc3: gadget: Avoid canceling current request for queuing error

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]

Wesley Cheng <wcheng@codeaurora.org> writes:

> If an error is received when issuing a start or update transfer
> command, the error handler will stop all active requests (including
> the current USB request), and call dwc3_gadget_giveback() to notify
> function drivers of the requests which have been stopped.  Avoid
> having to cancel the current request which is trying to be queued, as
> the function driver will handle the EP queue error accordingly.
> Simply unmap the request as it was done before, and allow previously
> started transfers to be cleaned up.
>
> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
> ---
>  drivers/usb/dwc3/gadget.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index e1b04c97..4200775 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>  		if (ret == -EAGAIN)
>  			return ret;
>  
> +		/* Avoid canceling current request, as it has not been started */
> +		if (req->trb)
> +			memset(req->trb, 0, sizeof(struct dwc3_trb));

we don't need a full memset. I think ensuring HWO bit is zero is enough.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH] usb: dwc3: gadget: Avoid canceling current request for queuing error

[Wesley Cheng]

On 4/14/2021 11:26 PM, Felipe Balbi wrote:
> Wesley Cheng <wcheng@codeaurora.org> writes:
> 
>> If an error is received when issuing a start or update transfer
>> command, the error handler will stop all active requests (including
>> the current USB request), and call dwc3_gadget_giveback() to notify
>> function drivers of the requests which have been stopped.  Avoid
>> having to cancel the current request which is trying to be queued, as
>> the function driver will handle the EP queue error accordingly.
>> Simply unmap the request as it was done before, and allow previously
>> started transfers to be cleaned up.
>>
>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
>> ---
>>  drivers/usb/dwc3/gadget.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index e1b04c97..4200775 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>>  		if (ret == -EAGAIN)
>>  			return ret;
>>  
>> +		/* Avoid canceling current request, as it has not been started */
>> +		if (req->trb)
>> +			memset(req->trb, 0, sizeof(struct dwc3_trb));
> 
> we don't need a full memset. I think ensuring HWO bit is zero is enough.
> 
Hi Felipe,

Thanks for the input/review, will make this change to just clear the HWO.

Thanks
Wesley Cheng

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] usb: dwc3: gadget: Avoid canceling current request for queuing error

[Thinh Nguyen]

Wesley Cheng wrote:
> 
> 
> On 4/14/2021 11:26 PM, Felipe Balbi wrote:
>> Wesley Cheng <wcheng@codeaurora.org> writes:
>>
>>> If an error is received when issuing a start or update transfer
>>> command, the error handler will stop all active requests (including
>>> the current USB request), and call dwc3_gadget_giveback() to notify
>>> function drivers of the requests which have been stopped.  Avoid
>>> having to cancel the current request which is trying to be queued, as
>>> the function driver will handle the EP queue error accordingly.
>>> Simply unmap the request as it was done before, and allow previously
>>> started transfers to be cleaned up.
>>>
>>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
>>> ---
>>>  drivers/usb/dwc3/gadget.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index e1b04c97..4200775 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>>>  		if (ret == -EAGAIN)
>>>  			return ret;
>>>  
>>> +		/* Avoid canceling current request, as it has not been started */
>>> +		if (req->trb)
>>> +			memset(req->trb, 0, sizeof(struct dwc3_trb));
>>
>> we don't need a full memset. I think ensuring HWO bit is zero is enough.
>>
> Hi Felipe,
> 
> Thanks for the input/review, will make this change to just clear the HWO.
> 

Make sure to increment the dequeue pointer also. I think you can use
dwc3_gadget_ep_skip_trbs().

BR,
Thinh

Re: [PATCH] usb: dwc3: gadget: Avoid canceling current request for queuing error

[Thinh Nguyen]

Thinh Nguyen wrote:
> Wesley Cheng wrote:
>>
>>
>> On 4/14/2021 11:26 PM, Felipe Balbi wrote:
>>> Wesley Cheng <wcheng@codeaurora.org> writes:
>>>
>>>> If an error is received when issuing a start or update transfer
>>>> command, the error handler will stop all active requests (including
>>>> the current USB request), and call dwc3_gadget_giveback() to notify
>>>> function drivers of the requests which have been stopped.  Avoid
>>>> having to cancel the current request which is trying to be queued, as
>>>> the function driver will handle the EP queue error accordingly.
>>>> Simply unmap the request as it was done before, and allow previously
>>>> started transfers to be cleaned up.
>>>>
>>>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
>>>> ---
>>>>  drivers/usb/dwc3/gadget.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index e1b04c97..4200775 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>>>>  		if (ret == -EAGAIN)
>>>>  			return ret;
>>>>  
>>>> +		/* Avoid canceling current request, as it has not been started */
>>>> +		if (req->trb)
>>>> +			memset(req->trb, 0, sizeof(struct dwc3_trb));
>>>
>>> we don't need a full memset. I think ensuring HWO bit is zero is enough.
>>>
>> Hi Felipe,
>>
>> Thanks for the input/review, will make this change to just clear the HWO.
>>
> 
> Make sure to increment the dequeue pointer also. I think you can use
> dwc3_gadget_ep_skip_trbs().
> 

Nevermind. There maybe a problem with using dwc3_gadget_ep_skip_trbs().

Thinh

Re: [PATCH] usb: dwc3: gadget: Avoid canceling current request for queuing error

[Wesley Cheng]

On 4/15/2021 12:28 PM, Thinh Nguyen wrote:
> Thinh Nguyen wrote:
>> Wesley Cheng wrote:
>>>
>>>
>>> On 4/14/2021 11:26 PM, Felipe Balbi wrote:
>>>> Wesley Cheng <wcheng@codeaurora.org> writes:
>>>>
>>>>> If an error is received when issuing a start or update transfer
>>>>> command, the error handler will stop all active requests (including
>>>>> the current USB request), and call dwc3_gadget_giveback() to notify
>>>>> function drivers of the requests which have been stopped.  Avoid
>>>>> having to cancel the current request which is trying to be queued, as
>>>>> the function driver will handle the EP queue error accordingly.
>>>>> Simply unmap the request as it was done before, and allow previously
>>>>> started transfers to be cleaned up.
>>>>>
>>>>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
>>>>> ---
>>>>>  drivers/usb/dwc3/gadget.c | 5 +++++
>>>>>  1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>> index e1b04c97..4200775 100644
>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>> @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>>>>>  		if (ret == -EAGAIN)
>>>>>  			return ret;
>>>>>  
>>>>> +		/* Avoid canceling current request, as it has not been started */
>>>>> +		if (req->trb)
>>>>> +			memset(req->trb, 0, sizeof(struct dwc3_trb));
>>>>
>>>> we don't need a full memset. I think ensuring HWO bit is zero is enough.
>>>>
>>> Hi Felipe,
>>>
>>> Thanks for the input/review, will make this change to just clear the HWO.
>>>
>>
>> Make sure to increment the dequeue pointer also. I think you can use
>> dwc3_gadget_ep_skip_trbs().
>>
> 
> Nevermind. There maybe a problem with using dwc3_gadget_ep_skip_trbs().
> 
> Thinh
> 
Hi Thinh,

Thank you for your input.  In this case (if kick transfer fails w/ an
error), would we still need to mess with the enqueue/dequeue pointers?
Not sure if my assumption is correct, but the TRB wouldn't have been
started, so we can use the same (failed) TRB for future requests, right?

I think one thing I will need to update is to loop through num_trbs and
clear all HWO bits if the above is not needed.

Thanks
Wesley Cheng

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] usb: dwc3: gadget: Avoid canceling current request for queuing error

[Thinh Nguyen]

Wesley Cheng wrote:
> 
> 
> On 4/15/2021 12:28 PM, Thinh Nguyen wrote:
>> Thinh Nguyen wrote:
>>> Wesley Cheng wrote:
>>>>
>>>>
>>>> On 4/14/2021 11:26 PM, Felipe Balbi wrote:
>>>>> Wesley Cheng <wcheng@codeaurora.org> writes:
>>>>>
>>>>>> If an error is received when issuing a start or update transfer
>>>>>> command, the error handler will stop all active requests (including
>>>>>> the current USB request), and call dwc3_gadget_giveback() to notify
>>>>>> function drivers of the requests which have been stopped.  Avoid
>>>>>> having to cancel the current request which is trying to be queued, as
>>>>>> the function driver will handle the EP queue error accordingly.
>>>>>> Simply unmap the request as it was done before, and allow previously
>>>>>> started transfers to be cleaned up.
>>>>>>
>>>>>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
>>>>>> ---
>>>>>>  drivers/usb/dwc3/gadget.c | 5 +++++
>>>>>>  1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>>> index e1b04c97..4200775 100644
>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>> @@ -1399,6 +1399,11 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>>>>>>  		if (ret == -EAGAIN)
>>>>>>  			return ret;
>>>>>>  
>>>>>> +		/* Avoid canceling current request, as it has not been started */
>>>>>> +		if (req->trb)
>>>>>> +			memset(req->trb, 0, sizeof(struct dwc3_trb));
>>>>>
>>>>> we don't need a full memset. I think ensuring HWO bit is zero is enough.
>>>>>
>>>> Hi Felipe,
>>>>
>>>> Thanks for the input/review, will make this change to just clear the HWO.
>>>>
>>>
>>> Make sure to increment the dequeue pointer also. I think you can use
>>> dwc3_gadget_ep_skip_trbs().
>>>
>>
>> Nevermind. There maybe a problem with using dwc3_gadget_ep_skip_trbs().
>>
>> Thinh
>>
> Hi Thinh,
> 
> Thank you for your input.  In this case (if kick transfer fails w/ an
> error), would we still need to mess with the enqueue/dequeue pointers?
> Not sure if my assumption is correct, but the TRB wouldn't have been
> started, so we can use the same (failed) TRB for future requests, right?
> 
> I think one thing I will need to update is to loop through num_trbs and
> clear all HWO bits if the above is not needed.
> 

No. We track and increment the TRB enqueue counter whenever we prepared
one. What you need to do is the opposite as dwc3_gadget_ep_skip_trbs().
You'd need to decrement the enqueue counter of all the TRB prepared by
the request.

Not every request is the same, and we shouldn't rely on the function
driver behavior to reuse/resubmit the same request.

Also, we use __dwc3_gadget_kick_transfer() everywhere and not just when
queuing new request. Make sure that your change only applies when
queuing new request failed and not because of other scenarios.

Thanks,
Thinh