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

[Felipe Balbi <balbi@kernel.org>]

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


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>> We only care to resume transfer for SG because the request maybe
>>> partially completed. dwc3_gadget_ep_request_completed() doesn't check
>>> that of a request, at least not fully.
>>>
>>> 1) It doesn't account for OUT direction.
>>> 2) It doesn't account for isoc. For isoc, a request maybe completed with
>>> partial data.
>> I would rather fix the function for these cases instead of removing it
>> completely. While at that, also move the req->num_pending_sgs check
>> inside dwc3_gadget_ep_request_completed()
>>
>
> If we want to keep this function, the only thing this function does is 
> to check req->num_pending_sgs. We'd only resume the request because 
> there are pending TRBs from SG not completed yet. If all the TRBs of a 
> request are completed, regardless if all the data are received/sent, we 
> don't queue them again. Do you still want to have this function?

The function gives a name to a very specific "concept", that of a
completed request. You can see that even today, the function is super
simple: OUT direction is always completed, IN direction is completed
when actual == length, we're just missing the pending_sgs check. So
something like the hunks below.

One thing I don't get from your patch is why you're completely removing
the function and why isn't req->direction and actual == length not
needed anymore. Could you explain?

hunks:

@@ -2482,7 +2482,8 @@ static int dwc3_gadget_ep_reclaim_trb_linear(struct dwc3_ep *dep,
 			event, status, false);
 }
 
-static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
+static bool dwc3_gadget_ep_request_completed(struct dwc3_ep *dep,
+		struct dwc3_request *req)
 {
 	/*
 	 * For OUT direction, host may send less than the setup
@@ -2491,6 +2492,16 @@ static bool dwc3_gadget_ep_request_completed(struct dwc3_request *req)
 	if (!req->direction)
 		return true;
 
+	/*
+	 * If there are pending scatterlist entries, we should
+	 * continue processing them.
+	 */
+	if (req->num_pending_sgs)
+		return false;
+
+	if (usb_endpoint_xfer_isoc(dep->endpoint.desc))
+		do_something();
+
 	return req->request.actual == req->request.length;
 }
 
@@ -2515,8 +2526,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) {
+	if (!dwc3_gadget_ep_request_completed(dep, req))
 		__dwc3_gadget_kick_transfer(dep);
 		goto out;
 	}

-- 
balbi

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