[PATCH net] net: mscc: ocelot: don't let phylink re-enable TX PAUSE on the NPI port

[PATCH net] net: mscc: ocelot: don't let phylink re-enable TX PAUSE on the NPI port

[Vladimir Oltean]

Since commit b39648079db4 ("net: mscc: ocelot: disable flow control on
NPI interface"), flow control should be disabled on the DSA CPU port
when used in NPI mode.

However, the commit blamed in the Fixes: tag below broke this, because
it allowed felix_phylink_mac_link_up() to overwrite SYS_PAUSE_CFG_PAUSE_ENA
for the DSA CPU port.

This issue became noticeable since the device tree update from commit
8fcea7be5736 ("arm64: dts: ls1028a: mark internal links between Felix
and ENETC as capable of flow control").

The solution is to check whether this is the currently configured NPI
port from ocelot_phylink_mac_link_up(), and to not modify the statically
disabled PAUSE frame transmission if it is.

When the port is configured for lossless mode as opposed to tail drop
mode, but the link partner (DSA master) doesn't observe the transmitted
PAUSE frames, the switch termination throughput is much worse, as can be
seen below.

Before:

root@debian:~# iperf3 -c 192.168.100.2
Connecting to host 192.168.100.2, port 5201
[  5] local 192.168.100.1 port 37504 connected to 192.168.100.2 port 5201
[ ID] Interval           Transfer     Bitrate         Retr  Cwnd
[  5]   0.00-1.00   sec  28.4 MBytes   238 Mbits/sec  357   22.6 KBytes
[  5]   1.00-2.00   sec  33.6 MBytes   282 Mbits/sec  426   19.8 KBytes
[  5]   2.00-3.00   sec  34.0 MBytes   285 Mbits/sec  343   21.2 KBytes
[  5]   3.00-4.00   sec  32.9 MBytes   276 Mbits/sec  354   22.6 KBytes
[  5]   4.00-5.00   sec  32.3 MBytes   271 Mbits/sec  297   18.4 KBytes
^C[  5]   5.00-5.06   sec  2.05 MBytes   270 Mbits/sec   45   19.8 KBytes
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-5.06   sec   163 MBytes   271 Mbits/sec  1822             sender
[  5]   0.00-5.06   sec  0.00 Bytes  0.00 bits/sec                  receiver

After:

root@debian:~# iperf3 -c 192.168.100.2
Connecting to host 192.168.100.2, port 5201
[  5] local 192.168.100.1 port 49470 connected to 192.168.100.2 port 5201
[ ID] Interval           Transfer     Bitrate         Retr  Cwnd
[  5]   0.00-1.00   sec   112 MBytes   941 Mbits/sec  259    143 KBytes
[  5]   1.00-2.00   sec   110 MBytes   920 Mbits/sec  329    144 KBytes
[  5]   2.00-3.00   sec   112 MBytes   936 Mbits/sec  255    144 KBytes
[  5]   3.00-4.00   sec   110 MBytes   927 Mbits/sec  355    105 KBytes
[  5]   4.00-5.00   sec   110 MBytes   926 Mbits/sec  350    156 KBytes
[  5]   5.00-6.00   sec   110 MBytes   925 Mbits/sec  305    148 KBytes
[  5]   6.00-7.00   sec   110 MBytes   924 Mbits/sec  320    143 KBytes
[  5]   7.00-8.00   sec   110 MBytes   925 Mbits/sec  273   97.6 KBytes
[  5]   8.00-9.00   sec   109 MBytes   913 Mbits/sec  299    141 KBytes
[  5]   9.00-10.00  sec   110 MBytes   922 Mbits/sec  287    146 KBytes
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec  1.08 GBytes   926 Mbits/sec  3032             sender
[  5]   0.00-10.00  sec  1.08 GBytes   925 Mbits/sec                  receiver

Fixes: de274be32cb2 ("net: dsa: felix: set TX flow control according to the phylink_mac_link_up resolution")
Reported-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index b1311b656e17..455293aa6343 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -771,7 +771,10 @@ void ocelot_phylink_mac_link_up(struct ocelot *ocelot, int port,
 
 	ocelot_write_rix(ocelot, 0, ANA_POL_FLOWC, port);
 
-	ocelot_fields_write(ocelot, port, SYS_PAUSE_CFG_PAUSE_ENA, tx_pause);
+	/* Don't attempt to send PAUSE frames on the NPI port, it's broken */
+	if (port != ocelot->npi)
+		ocelot_fields_write(ocelot, port, SYS_PAUSE_CFG_PAUSE_ENA,
+				    tx_pause);
 
 	/* Undo the effects of ocelot_phylink_mac_link_down:
 	 * enable MAC module
-- 
2.25.1

Re: [PATCH net] net: mscc: ocelot: don't let phylink re-enable TX PAUSE on the NPI port

[Florian Fainelli]

On 1/12/22 12:21 PM, Vladimir Oltean wrote:
> Since commit b39648079db4 ("net: mscc: ocelot: disable flow control on
> NPI interface"), flow control should be disabled on the DSA CPU port
> when used in NPI mode.
> 
> However, the commit blamed in the Fixes: tag below broke this, because
> it allowed felix_phylink_mac_link_up() to overwrite SYS_PAUSE_CFG_PAUSE_ENA
> for the DSA CPU port.
> 
> This issue became noticeable since the device tree update from commit
> 8fcea7be5736 ("arm64: dts: ls1028a: mark internal links between Felix
> and ENETC as capable of flow control").
> 
> The solution is to check whether this is the currently configured NPI
> port from ocelot_phylink_mac_link_up(), and to not modify the statically
> disabled PAUSE frame transmission if it is.
> 
> When the port is configured for lossless mode as opposed to tail drop
> mode, but the link partner (DSA master) doesn't observe the transmitted
> PAUSE frames, the switch termination throughput is much worse, as can be
> seen below.
> 
> Before:
> 
> root@debian:~# iperf3 -c 192.168.100.2
> Connecting to host 192.168.100.2, port 5201
> [  5] local 192.168.100.1 port 37504 connected to 192.168.100.2 port 5201
> [ ID] Interval           Transfer     Bitrate         Retr  Cwnd
> [  5]   0.00-1.00   sec  28.4 MBytes   238 Mbits/sec  357   22.6 KBytes
> [  5]   1.00-2.00   sec  33.6 MBytes   282 Mbits/sec  426   19.8 KBytes
> [  5]   2.00-3.00   sec  34.0 MBytes   285 Mbits/sec  343   21.2 KBytes
> [  5]   3.00-4.00   sec  32.9 MBytes   276 Mbits/sec  354   22.6 KBytes
> [  5]   4.00-5.00   sec  32.3 MBytes   271 Mbits/sec  297   18.4 KBytes
> ^C[  5]   5.00-5.06   sec  2.05 MBytes   270 Mbits/sec   45   19.8 KBytes
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bitrate         Retr
> [  5]   0.00-5.06   sec   163 MBytes   271 Mbits/sec  1822             sender
> [  5]   0.00-5.06   sec  0.00 Bytes  0.00 bits/sec                  receiver
> 
> After:
> 
> root@debian:~# iperf3 -c 192.168.100.2
> Connecting to host 192.168.100.2, port 5201
> [  5] local 192.168.100.1 port 49470 connected to 192.168.100.2 port 5201
> [ ID] Interval           Transfer     Bitrate         Retr  Cwnd
> [  5]   0.00-1.00   sec   112 MBytes   941 Mbits/sec  259    143 KBytes
> [  5]   1.00-2.00   sec   110 MBytes   920 Mbits/sec  329    144 KBytes
> [  5]   2.00-3.00   sec   112 MBytes   936 Mbits/sec  255    144 KBytes
> [  5]   3.00-4.00   sec   110 MBytes   927 Mbits/sec  355    105 KBytes
> [  5]   4.00-5.00   sec   110 MBytes   926 Mbits/sec  350    156 KBytes
> [  5]   5.00-6.00   sec   110 MBytes   925 Mbits/sec  305    148 KBytes
> [  5]   6.00-7.00   sec   110 MBytes   924 Mbits/sec  320    143 KBytes
> [  5]   7.00-8.00   sec   110 MBytes   925 Mbits/sec  273   97.6 KBytes
> [  5]   8.00-9.00   sec   109 MBytes   913 Mbits/sec  299    141 KBytes
> [  5]   9.00-10.00  sec   110 MBytes   922 Mbits/sec  287    146 KBytes
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bitrate         Retr
> [  5]   0.00-10.00  sec  1.08 GBytes   926 Mbits/sec  3032             sender
> [  5]   0.00-10.00  sec  1.08 GBytes   925 Mbits/sec                  receiver

Still quite a lot of retries, do you know where they are coming from?

> 
> Fixes: de274be32cb2 ("net: dsa: felix: set TX flow control according to the phylink_mac_link_up resolution")
> Reported-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net] net: mscc: ocelot: don't let phylink re-enable TX PAUSE on the NPI port

[Vladimir Oltean]

On Wed, Jan 12, 2022 at 12:31:14PM -0800, Florian Fainelli wrote:
> Still quite a lot of retries, do you know where they are coming from?

Yes, they are coming from the fact that the CPU port runs at 2.5G and
the front port at 1G, and as mentioned, there isn't any flow control
when the CPU port is used in NPI mode.

Flow control on that port pair _is_ available when the CPU port is used
with the ocelot-8021q tagging protocol, which is the reason why I
updated the device tree to declare "pause" for those fixed-links.
We can't enable or disable flow control on a fixed-link at runtime in
Linux.

Re: [PATCH net] net: mscc: ocelot: don't let phylink re-enable TX PAUSE on the NPI port

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Wed, 12 Jan 2022 22:21:27 +0200 you wrote:
> Since commit b39648079db4 ("net: mscc: ocelot: disable flow control on
> NPI interface"), flow control should be disabled on the DSA CPU port
> when used in NPI mode.
> 
> However, the commit blamed in the Fixes: tag below broke this, because
> it allowed felix_phylink_mac_link_up() to overwrite SYS_PAUSE_CFG_PAUSE_ENA
> for the DSA CPU port.
> 
> [...]

Here is the summary with links:
  - [net] net: mscc: ocelot: don't let phylink re-enable TX PAUSE on the NPI port
    https://git.kernel.org/netdev/net/c/33cb0ff30cff

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html