RE: [net,1/1] net: stmmac: Set OWN bit last in dwmac4_set_rx_owner()

From: EnDe Tan
Date: Fri Aug 09 2024 - 23:33:20 EST


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.

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

Regards,
Tan En De