Re: [PATCH] usb: dwc3: gadget: Remove incomplete check

[Felipe Balbi <balbi@kernel.org>]

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


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> 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.

-- 
balbi

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