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

[Felipe Balbi <balbi@kernel.org>]

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


Hi,

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.

-- 
balbi

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