Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes:
>>> 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.
> Sure, I think patch 1 can go in during -rc. Do we need a Cc stable on
> it, though?
>
> Patch 2 will have to wait until v5.8.
>
Sure. I'll resend with Cc stable tag.
Thanks,
Thinh
