Hi, Thinh Nguyen writes: >> Thinh Nguyen writes: >>>> Thinh Nguyen writes: >>>>> If dwc3 fails to issue START_TRANSFER/UPDATE_TRANSFER command, then we >>>>> should properly end an active transfer and give back all the started >>>>> requests. However if it's for an isoc endpoint, the failure maybe due to >>>>> bus-expiry status. In this case, don't give back the requests and wait >>>>> for the next retry. >>>>> >>>>> Fixes: 72246da40f37 ("usb: Introduce DesignWare USB3 DRD Driver") >>>>> Signed-off-by: Thinh Nguyen >>>> could you give some details regarding when does this happen? >>>> >>> So, here are the scenarios in which dwc3_send_gadget_ep_cmd() may return >>> a negative errno: >>> >>> * -EAGAIN: Isoc bus-expiry status >>>    As you already know, this occurs when we try to schedule isoc too >>> late. If we're going to retry the request, don't unmap it. >> right >> >>> * -EINVAL: No resource due to issuing START_TRANSFER to an already >>> started endpoint >>>    This happens generally because of SW programming error >> Sounds like this should be fixed separately and, probably, we should add >> a WARN() so we catch these situations. Have you reproduced this >> particular case? > > There are a couple of examples of no resource issue that I recall: > 1) When we removed the DWC3_EP_END_TRANSFER_PENDING flag, we wasn't able > to check if the endpoint had ended. So, if the function driver queues a > new request while END_TRANSFER command is in progress, it'd trigger > START_TRANSFER to an already started endpoint Okay, sounds like this deserves a patch of its own > 2) For this new method of retrying for isoc, when we issue END_TRANSFER > command, for some controller versions, the controller would generate > XferNotReady event while the END_TRANSFER command is in progress plus > another after it completed. That means we'd start on the same endpoint > twice and trigger a no-resource error. (We'd run into this scenario > because END_TRANSFER completion cleared the started flag). > > We added the checks to prevent this issue from happening, so this > scenario should not happen again. Cool, thanks > If we want to add a WARN(), I think we should add that inside of > dwc3_send_gadget_ep_cmd() function, as a separate patch. We can also > just look at the tracepoint for "no resource" status. The "no resource" status is important, sure. But users don't usually run with tracepoints enabled. They'll have a non-working USB port and forget about it. If there's a WARN() triggered, we are more likely to get bug reports. -- balbi