linux-kernel - RE: [PATCH] usb: dwc3: gadget: fix miss isoc issue introduced by IRQ latency

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 17 Dec 2018 01:45:33 +0000
From:   "Zengtao (B)" <prime.zeng@...ilicon.com>
To:     Thinh Nguyen <thinh.nguyen@...opsys.com>,
        Felipe Balbi <balbi@...nel.org>
CC:     liangshengjun <liangshengjun@...ilicon.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] usb: dwc3: gadget: fix miss isoc issue introduced by
 IRQ latency

Hi Thinh:

>-----Original Message-----
>From: Thinh Nguyen [mailto:thinh.nguyen@...opsys.com]
>Sent: Saturday, December 15, 2018 5:43 AM
>To: Felipe Balbi <balbi@...nel.org>; Zengtao (B)
><prime.zeng@...ilicon.com>
>Cc: liangshengjun <liangshengjun@...ilicon.com>; Greg Kroah-Hartman
><gregkh@...uxfoundation.org>; linux-usb@...r.kernel.org;
>linux-kernel@...r.kernel.org; Thinh Nguyen
><thinh.nguyen@...opsys.com>
>Subject: Re: [PATCH] usb: dwc3: gadget: fix miss isoc issue introduced by
>IRQ latency
>
>Hi Zengtao,
>
>On 12/14/2018 3:24 AM, Felipe Balbi wrote:
>> Hi,
>>
>> "Zengtao (B)" <prime.zeng@...ilicon.com> writes:
>>>> -----Original Message-----
>>>> From: Felipe Balbi [mailto:balbi@...nel.org]
>>>> Sent: Friday, December 14, 2018 4:52 PM
>>>> To: Zengtao (B) <prime.zeng@...ilicon.com>
>>>> Cc: liangshengjun <liangshengjun@...ilicon.com>; Zengtao (B)
>>>> <prime.zeng@...ilicon.com>; Greg Kroah-Hartman
>>>> <gregkh@...uxfoundation.org>; linux-usb@...r.kernel.org;
>>>> linux-kernel@...r.kernel.org
>>>> Subject: Re: [PATCH] usb: dwc3: gadget: fix miss isoc issue
>>>> introduced by IRQ latency
>>>>
>>>> * PGP Signed by an unknown key
>>>>
>>>> Zeng Tao <prime.zeng@...ilicon.com> 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@...ilicon.com>
>>>>> Signed-off-by: Zeng Tao <prime.zeng@...ilicon.com>
>>>> 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.


This is just for checking whether the Frame number is overflow or not. So
1 second should a reason value.

>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.
eframe > cframe + 1s is used to deal with the Frame number overflow.

>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.
>
If the interval is one, 1 more interval should be more reasonable because the
core always fetch the trb one frame ahead.

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

Yes, agree, but I think if the sw can be used to do the same thing and more
effective, Why not? 

BR
Zengtao