Re: [PATCH 1/3] usb: dwc3: gadget: Properly handle failed kick_transfer

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2752 bytes --]


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> 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@synopsys.com>
>>>> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]