netdev - RE: [PATCH] net: renesas: ravb: Fix a stuck issue when a lot of frames are received

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, 10 May 2021 10:29:59 +0000
From:   Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To:     Sergei Shtylyov <sergei.shtylyov@...il.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>
Subject: RE: [PATCH] net: renesas: ravb: Fix a stuck issue when a lot of
 frames are received

Hi Sergei,

> From: Sergei Shtylyov, Sent: Sunday, May 9, 2021 7:22 PM
> 
> On 08.05.2021 23:47, Sergei Shtylyov 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.

> >> 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".
In poll(), the interrupts are already disabled, and ravb_rx() will check the
descriptor's status. So, this patch keeps the "Interrupt mitigation" IIUC.

[1]
https://wiki.linuxfoundation.org/networking/napi

Best regards,
Yoshihiro Shimoda