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.
BR,
Thinh
