Hi Felipe, Felipe Balbi wrote: > Hi, > > Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes: >>> 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. > wait, huh? What about cases when user unplugs cable midtransfer? We have > versions of dwc3 HW which fail to produce disconnect interrupt, right? There won't even enter this code path if disconnect event is generated or not. > >>> @@ -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. > Will do "do_something", then return true or simply return true? I mean simply 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. > we can unplug the cable at any time, even mid-transfer. > That's fine if there's a disconnection mid-transfer. The transfer is cancelled in that case, why would we want to kick_transfer again? Also, the controller would not generate XferInProgress event to notify TRB completion for the driver to enter this code path. 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. When the function driver queues a new request, then there will be new TRBs to prepare and then the driver can kick_transfer() again. 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 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. Let me know if there's something not clear. Thanks, Thinh
