Hi,
Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes:
>>> Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> 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.
Fair point.
>
> 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?
It's because there's no use for that function outside of checking for
number of pending SGs and resume.
>
> 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();
do_something() will always return true here.
> +
> return req->request.actual == req->request.length;
This should always be true if the request completes. By spec, bulk and
interrupt endpoints data delivery are guaranteed, and the retry/error
detection is done at the lower level. If by chance that the host fails
to request for data multiple times at the packet level, it will issue a
ClearFeature(halt_ep) request to the endpoint. This will trigger dwc3 to
stop the endpoint and cancel the transfer, and we still won't resume
this transfer.
> }
>
> @@ -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;
> }
>
BR,
Thinh
