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

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

Hi Felipe,

Felipe Balbi wrote:
> 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

Yes, that's why we resurrected that flag and made the fix (the patch is 
upstream). It should not happen again.

>> 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.
>

Understood. We can add a WARN() to dwc3_send_gadget_ep_cmd() in a 
separate patch.

Is there any other concern regarding this series? Let me know if I need 
to resend it.

Thanks,
Thinh