Re: [PATCH v4 6/6] usb:cdns3 Fix for stuck packets in on-chip OUT buffer.

[Roger Quadros]

On 07/03/2019 09:06, Pawel Laszczak wrote:
> Hi,
> 
>> Hi,
>>
>> On 21/02/2019 09:14, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> (please break your emails at 80-columns)
>>>
>>> Pawel Laszczak <pawell@xxxxxxxxxxx> writes:
>>>>>> One more thing. Workaround has implemented algorithm that decide for which
>>>>>> endpoint it should be enabled.  e.g for composite device MSC+NCM+ACM it
>>>>>> should work only for ACM OUT endpoint.
>>>>>>
>>>>>
>>>>> If ACM driver didn't queue the request for ACM OUT endpoint, why does the
>>>>> controller accept the data at all?
>>>>>
>>>>> I didn't understand why we need a workaround for this. It should be standard
>>>>> behaviour to NAK data if function driver didn't request for all endpoints.
>>>>
>>>> Yes, I agree with you. Controller shouldnât accept such packet. As I know this
>>>> behavior will be fixed in RTL.
>>>>
>>>> But I assume that some older version of this controller are one the market,
>>>> and driver should work correct with them.
>>>>
>>>> In the feature this workaround can be limited only to selected controllers.
>>>>
>>>> Even now I assume that it can be enabled/disabled by module parameter.
>>>
>>> no module parameters, please. Use revision detection in runtime.
>>>
>>
>> This is about whether to enable or disable the workaround.
>> By default we don't want this workaround to be enabled.
>>
>> I'm debating whether we should have this workaround at all or not.
>>
>> It has the following problems.
>>
>> 1) It ACKs packets even when gadget end is not ready to accept the transfers.
>> 2) It stores these packets in a temporary buffer and then pushes them to the
>> gadget driver whenever the gadget driver is ready to process the data.
>> 3) Since the gadget driver can become ready at an indefinite time in the
>> future, it poses 2 problems:
>> a) It is sending stale data to the sink. (problematic at next protocol level?)
>> b) If this temporary buffer runs out we still hit the lock up issue.
>>
>> I think the right solution is to make sure that the gadget driver is always
>> reading all the enabled OUT endpoints *or* (keep the OUT endpoints disabled
>> if gadget driver is not ready to process OUT transfers).
> 
> If driver disable endpoint then it not answer for packets from host. 
> It will result that host reset the device, so I can't disable such endpoints. 
> 
> Other good solution is to change ACM driver in a way that it sends requests 
> to controller driver after enabling endpoint. The class driver could decide 

The ACM driver is doing exactly that. However, there is a large delay (depending
on when user opens the ttyACM) between endpoint enable and request queue and
that's the issue for this controller.

The endpoint is enabled whenever the host sends a SET_INTERFACE
[acm_set_alt()->gserial_connect()]
but the first request is queued only when user opens the ttyACM
[gs_open()->gs_start_io()->gs_start_rx()].

I'm don't think this sequence can be changed.
What happens if you defer gserial_connect() to be done at gs_open()?

> what should do with such not expected packets. It could delete all or e.g 
> keep only few last packets. 
> 
> This issue will be fixed in RTL but maybe driver should be compatible with previous 
> controllerâs version.
> 
> By default, this workaround will be disabled or will depend on controller version.  
>>
> 
> Cheers,
> Pawel
> 

-- 
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki