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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>>>>>> ---
>>>>>>  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