linux-kernel - Re: [net,1/1] net: stmmac: Set OWN bit last in dwmac4_set_rx

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] [day] [month] [year] [list]

Message-ID: <a3b52da3-a084-4fc5-a60f-90e18d9f8132@lunn.ch>
Date: Sat, 10 Aug 2024 17:36:34 +0200
From: Andrew Lunn <andrew@...n.ch>
To: EnDe Tan <ende.tan@...rfivetech.com>
Cc: Tan En De <endeneer@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"alexandre.torgue@...s.st.com" <alexandre.torgue@...s.st.com>,
	"joabreu@...opsys.com" <joabreu@...opsys.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"mcoquelin.stm32@...il.com" <mcoquelin.stm32@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Leyfoon Tan <leyfoon.tan@...rfivetech.com>
Subject: Re: [net,1/1] net: stmmac: Set OWN bit last in dwmac4_set_rx_owner()

On Sat, Aug 10, 2024 at 02:58:50AM +0000, EnDe Tan wrote:
>  Hi Andrew, thanks for taking a look at my patch.
> 
> > Are you seeing things going wrong with real hardware, or is this just code
> > review? If this is a real problem, please add a description of what the user
> > would see.
> >
> > Does this need to be backported in stable?
> 
> On my FPGA, after running iperf3 for a while, the GMAC somehow got stuck,
> as shown by 0.00 bits/sec, for example:
> ```
> iperf3 -t 6000 -c 192.168.xxx.xxx -i 10 -P 2 -l 128
> ...
> [  5] 220.00-230.00 sec  2.04 MBytes  1.71 Mbits/sec    3   1.41 KBytes
> [  7] 220.00-230.00 sec  2.04 MBytes  1.71 Mbits/sec    3   1.41 KBytes
> [SUM] 220.00-230.00 sec  4.07 MBytes  3.42 Mbits/sec    6
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [  5] 230.00-240.01 sec  0.00 Bytes  0.00 bits/sec    2   1.41 KBytes
> [  7] 230.00-240.01 sec  0.00 Bytes  0.00 bits/sec    2   1.41 KBytes
> [SUM] 230.00-240.01 sec  0.00 Bytes  0.00 bits/sec    4
> ```
> 
> Used devmem to check registers:
> 0x780 (Rx_Packets_Count_Good_Bad)
> - The count was incrementing, so packets were still received.
> 0x114C (DMA_CH0_Current_App_RxDesc)
> - Value was changing, so DMA did update the RX descriptor address pointer.
> 0x1160 (DMA_CH0_Status).
> - Receive Buffer Unavailable RBU = 0.
> - Receive Interrupt RI = 1 (stuck at 1).
> 
> which led me to suspect that there was missed RX interrupt.
> I then came across dwmac4_set_rx_owner() function, and saw the possibility of 
> missed interrupt (when DMA sees OWN bit before INT_ON_COMPLETION bit).
> 
> However, even with this patch, the problem persists on my FPGA.
> Therefore, I'd treat this patch as a code review, as I can't provide a concrete proof
> that this patch fixes any real hardware.

O.K. Please target this patch to net-next. We can always get it
backported to stable later.

> > Is the problem here that RDES3_INT_ON_COMPLETION_EN is added after the
> > RDES3_OWN above has hit the hardware, so it gets ignored?
> > 
> > It seems like it would be better to calculate the value in a local variable, and
> > then assign to p->des3 once.
> 
> I didn't use local variable because I worry about CPU out-of-order execution. 
> For example,
> ```
> local_var = (INT_ON_COMPLETION | OWN)
> des3 |= local_var
> ```
> CPU optimization might result in this
> ```
> des3 |= INT_ON_COMPLETION
> des3 |= OWN
> ```
> or worst, out of order like this
> ```
> des3 |= OWN
> des3 |= INT_ON_COMPLETION
> ```
> which could cause missing interrupt.

I'm assuming the des3 is mapped as non-cached. So each access is
expensive. If you can convince the compiler to assemble the value in a
register and then perform one write, it will be cheaper than two
writes.

	Andrew