Hi, Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes: >>>> @@ -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. got it >>>> 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. d'oh! That's true > 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. 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. > 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. -- balbi
Attachment:
signature.asc
Description: PGP signature
