Hi Thinh: >-----Original Message----- >From: Thinh Nguyen [mailto:thinh.nguyen@xxxxxxxxxxxx] >Sent: Saturday, December 15, 2018 5:43 AM >To: Felipe Balbi <balbi@xxxxxxxxxx>; Zengtao (B) ><prime.zeng@xxxxxxxxxxxxx> >Cc: liangshengjun <liangshengjun@xxxxxxxxxxxxx>; Greg Kroah-Hartman ><gregkh@xxxxxxxxxxxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; >linux-kernel@xxxxxxxxxxxxxxx; Thinh Nguyen ><thinh.nguyen@xxxxxxxxxxxx> >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@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. 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
