Hi,
Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes:
>> Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes:
>>>> Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> 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 <thinhn@xxxxxxxxxxxx>
>>>> 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
Attachment:
signature.asc
Description: PGP signature
