Hi,
Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes:
>>>> The condition here is if (!request_complete()), then kick_transfer().
>>>> Let's take a look at what kick_transfer() do:
>>>>
>>>> kick_transfer() will prepare new TRBs and issue START_TRANSFER command
>>>> or UPDATE_TRANSFER command. The endpoint is already started, and nothing
>>>> is causing it to end at this point. So it should just be UPDATE_TRANSFER
>>>> command. UPDATE_TRANSFER command tells the controller to update its TRB
>>>> cache because there will be new TRBs prepared for the request.
>>>>
>>>> If this is non-SG/non-chained TRB request, then there's only 1 TRB per
>>>> request for IN endpoints. If that TRB is completed, that means that the
>>>> request is completed. There's no reason to issue kick_transfer() again.
>>> not entirely true for bulk. We never set LST bit; we will never complete
>>> a transfer, we continually add more TRBs. The reason for this is to
>>> amortize the cost of adding new transfers to the controller cache before
>>> it runs out of TRBs without HWO.
>> Right, I was referring to "request" rather than transfer (as in a
>> transfer may have 1 or more requests).
>>
>>> How about we change the test to say "if I have non-started TRBs and I'm
>>> bulk (non-stream) or interrupt endpoint, kick more transfers"?
>>>
>>>> When the function driver queues a new request, then there will be new
>>>> TRBs to prepare and then the driver can kick_transfer() again.
>>> We may already have more TRBs in the pending list which may not have
>>> been started before we didn't have free TRBs to use. We just completed a
>>> TRB, might as well try to use it for more requests.
>> Yes we can and we should, but we didn't check that. Also it shouldn't be
>> in the request_complete() check function as they are part of different
>> requests.
>>
>>>> So, this condition to check if request_complete() is only valid for a
>>>> request with multiple chained TRBs. Since we can only check for IN
>>>> direction, the chained TRB setup related to OUT direction to fit
>>>> MaxPacketSize does not apply here. What left is chained TRBs for SG. In
>>> this part is clear now and you're correct. Thanks
>>>
>>>> this case, we do want to kick_transfer again. This may happen when we
>>>> run out of TRBs and we have to wait for available TRBs. When there are
>>>> available TRBs and still pending SGs, then we want to prepare the rest
>>>> of the SG entries to finish the request. So kick_transfer() makes sense
>>>> here.
>>> Right but we can run out of TRBs even in non-chained case. I remember
>>> testing this years ago by giving g_mass_storage a list of 300
>>> requests. The reason for kicking the transfer is different, but it's
>>> beneficial anyhow.
>>>
>> In this case, the check should be for if the pending_list is not empty,
>> then kick again.
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 6a04c9afcab6..d8318de55000 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2975,14 +2975,7 @@ static int
>> dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep,
>>
>> static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
>> {
>> - /*
>> - * For OUT direction, host may send less than the setup
>> - * length. Return true for all OUT requests.
>> - */
>> - if (!req->direction)
>> - return true;
>> -
>> - return req->request.actual == req->request.length;
>> + return req->num_pending_sgs == 0;
>> }
>>
>> static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
>> @@ -3007,7 +3000,7 @@ static int
>> dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep,
>> req->request.actual = req->request.length - req->remaining;
>>
>> if (!dwc3_gadget_ep_request_completed(req) ||
>> - req->num_pending_sgs) {
>> + !list_empty(&dep->pending_list)) {
>> __dwc3_gadget_kick_transfer(dep);
>> goto out;
>> }
>>
>>
>> This is unlikely to happen, but it's necessary to be there.
>>
>> Let me know if you're ok with the change, I'll create a formal patch for it.
> Looks good, we may just rename the function to
> dwc3_gadget_ep_should_continue() or something similar and move the
> !list_empty() check in there too.
>
I forgot this condition skips the dwc3_gadget_giveback(). I have to
split it. Let me send out the revised patches and you can review.
Thanks,
Thinh
