Re: [PATCH] usb: dwc3: gadget: fix miss isoc issue introduced by IRQ latency

[Thinh Nguyen]

Hi Zengtao,

On 12/14/2018 3:24 AM, Felipe Balbi wrote:
> Hi,
>
> "Zengtao (B)" <prime.zeng@xxxxxxxxxxxxx> writes:
>>> -----Original Message-----
>>> From: Felipe Balbi [mailto:balbi@xxxxxxxxxx]
>>> Sent: Friday, December 14, 2018 4:52 PM
>>> To: Zengtao (B) <prime.zeng@xxxxxxxxxxxxx>
>>> Cc: liangshengjun <liangshengjun@xxxxxxxxxxxxx>; Zengtao (B)
>>> <prime.zeng@xxxxxxxxxxxxx>; Greg Kroah-Hartman
>>> <gregkh@xxxxxxxxxxxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx;
>>> linux-kernel@xxxxxxxxxxxxxxx
>>> Subject: Re: [PATCH] usb: dwc3: gadget: fix miss isoc issue introduced by
>>> IRQ latency
>>>
>>> * PGP Signed by an unknown key
>>>
>>> Zeng Tao <prime.zeng@xxxxxxxxxxxxx> writes:
>>>
>>>> If it's a busy system, some times when we start an isoc transfer, the
>>>> framenumber get from the event buffer may be already elasped, in this
>>>> case, we will get all the packets dropped due to miss isoc. And we
>>>> turn into transfer nothing, to fix this issue, we need to fix the
>>>> framenumber to make sure that it's not out of date.
>>>>
>>>> Signed-off-by: Liang Shengjun <liangshengjun@xxxxxxxxxxxxx>
>>>> Signed-off-by: Zeng Tao <prime.zeng@xxxxxxxxxxxxx>
>>> There's a patch going upstream already doing this:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git/commit/?h
>>> =next&id=d53701067f048b8b11635e964b6d3bd9a248c622
>>>
>> Sorry, I think I missed to update to the latest version. But I think my 
>> patch is more efficient. Because I just sync the frame from the HW, it
>>  won't fail and there is no need to extra tries.
>>
>> What do you think about it?
> the 14 bits you use for your check is not representative of the actual
> micro-frame you're currently in. Thinh explained that in the discussion
> we had until we came to the patch I pointed you to above. Please look at
> the mailing list archives for details.
>

There are several issues with this patch.
1) Your frame elapsed time check is not based on interval but statically
DWC3_EVENT_PRAM_SOFFN / 2. That's about 1 second. So it doesn't account
for isoc transfers with large service interval of 1 sec or more.
2) This function __dwc3_gadget_target_frame_elapsed() should have
comments for what it does. The name implies that this function checks
for eframe > cframe, and not eframe > cframe + 1s.
3) If this check fails, then it will do DWC3_ALIGN_FRAME() at least
twice. The isoc transfer will start 1 more interval into the future than
it needs to.

Also, I think the role of this check should be from the controller as it
has more information and its own logic to decide if the selected future
uframe has elapsed.

BR,
Thinh