Hello! > From: Sergei Shtylyov, Sent: Tuesday, May 18, 2021 4:36 AM > > On 5/10/21 1:29 PM, Yoshihiro Shimoda wrote: > > >>> Posting a review of the already commited (over my head) patch. It would have > >>> been appropriate if the patch looked OK but it's not. :-/ > >>> > >>>> When a lot of frames were received in the short term, the driver > >>>> caused a stuck of receiving until a new frame was received. For example, > >>>> the following command from other device could cause this issue. > >>>> > >>>> $ sudo ping -f -l 1000 -c 1000 <this driver's ipaddress> > >>> > >>> -l is essential here, right? > > > > Yes. > > > >>> Have you tried testing sh_eth sriver like that, BTW? > >> > >> It's driver! :-) > > > > I have not tried testing sh_eth driver yet. I'll test it after I got an actual board. > > Now you've got it, let's not rush forth with the fix this time. I sent a report yesterday: https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210421045246.215779-1-yoshihiro.shimoda.uh@xxxxxxxxxxx/#24181167 > >>>> The previous code always cleared the interrupt flag of RX but checks > >>>> the interrupt flags in ravb_poll(). So, ravb_poll() could not call > >>>> ravb_rx() in the next time until a new RX frame was received if > >>>> ravb_rx() returned true. To fix the issue, always calls ravb_rx() > >>>> regardless the interrupt flags condition. > >>> > >>> That bacially defeats the purpose of IIUC... > >> ^ NAPI, > >> > >> I was sure I typed NAPI here, yet it got lost in the edits. :-) > > > > I could not understand "that" (calling ravb_rx() regardless the interrupt > > flags condition) defeats the purpose of NAPI. According to an article on > > the Linux Foundation wiki [1], one of the purpose of NAPI is "Interrupt mitigation". > > Thank you for the pointer, BTW! Would have helped me with enabling NAPI in sh_eth > (and ravb) drivers... > > > In poll(), the interrupts are already disabled, and ravb_rx() will check the > > descriptor's status. So, this patch keeps the "Interrupt mitigation" IIUC. > > I think we'll still have the short race window, described in section 5.1 > of this doc. So perhaps what we should do is changing the order of the code in > the poll() method, not eliminating the loops totally. Thoughts? The ravb hardware acts as "non-level sensitive IRQs". However, fortunately, the hardware can set an interrupt flag even if the interrupt is masked. So, I don't think this patch have any race window. Best regards, Yoshihiro Shimoda > > [1] > > > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwiki.linuxfoundation.org%2Fnetworking%2Fnapi&d > ata=04%7C01%7Cyoshihiro.shimoda.uh%40renesas.com%7C0102c1f2995947bcca1608d9196af978%7C53d82571da1947e49cb4625a166a4a > 2a%7C0%7C0%7C637568769530134169%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6 > Mn0%3D%7C1000&sdata=47kgAmI3d%2Fz%2BHunT0a8bzHRRQk1VdnxRETSExLkTrdI%3D&reserved=0 > > > > Best regards, > > Yoshihiro Shimoda > > MBR, Sergei
