Aw: Re: Choose a default DSA CPU port

Aw: Re: Choose a default DSA CPU port

[Frank Wunderlich]

> mhm...tried again with port5 disabled (pause enabled again) and got same result, so this seems not to be the cause > as i thought (but have now other patches in like core-clock dropped and RX fix).

6.1.12 is clean and i get 940 Mbit/s over gmac0/port6

root@bpi-r2:~# iperf3 -c 192.168.0.21
Connecting to host 192.168.0.21, port 5201
[  5] local 192.168.0.11 port 44876 connected to 192.168.0.21 port 5201
[ ID] Interval           Transfer     Bitrate         Retr  Cwnd
[  5]   0.00-1.00   sec   113 MBytes   950 Mbits/sec    0    464 KBytes
[  5]   1.00-2.00   sec   112 MBytes   939 Mbits/sec    0    485 KBytes
[  5]   2.00-3.00   sec   112 MBytes   940 Mbits/sec    0    485 KBytes
[  5]   3.00-4.00   sec   112 MBytes   938 Mbits/sec    0    485 KBytes
[  5]   4.00-5.00   sec   112 MBytes   937 Mbits/sec    0    485 KBytes
[  5]   5.00-6.00   sec   112 MBytes   941 Mbits/sec    0    485 KBytes
[  5]   6.00-7.00   sec   112 MBytes   940 Mbits/sec    0    485 KBytes
[  5]   7.00-8.00   sec   112 MBytes   937 Mbits/sec    0    485 KBytes
[  5]   8.00-9.00   sec   112 MBytes   942 Mbits/sec    0    485 KBytes
[  5]   9.00-10.00  sec   112 MBytes   936 Mbits/sec    0    485 KBytes
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec  1.09 GBytes   940 Mbits/sec    0             sender
[  5]   0.00-10.05  sec  1.09 GBytes   935 Mbits/sec                  receiver

6.2.0 is not...so something else in 6.2 has caused the speed drop

regards Frank

Re: Choose a default DSA CPU port

[Vladimir Oltean]

On Fri, Feb 24, 2023 at 09:44:43PM +0100, Frank Wunderlich wrote:
> 6.1.12 is clean and i get 940 Mbit/s over gmac0/port6

Sounds like something which could be bisected?

Aw: Re: Choose a default DSA CPU port

[Frank Wunderlich]

> Gesendet: Freitag, 24. Februar 2023 um 22:08 Uhr
> Von: "Vladimir Oltean" <olteanv@gmail.com>
> An: "Frank Wunderlich" <frank-w@public-files.de>
> Cc: "Arınç ÜNAL" <arinc.unal@arinc9.com>, "netdev" <netdev@vger.kernel.org>, erkin.bozoglu@xeront.com, "Andrew Lunn" <andrew@lunn.ch>, "Florian Fainelli" <f.fainelli@gmail.com>, "Felix Fietkau" <nbd@nbd.name>, "John Crispin" <john@phrozen.org>, "Mark Lee" <Mark-MC.Lee@mediatek.com>, "Lorenzo Bianconi" <lorenzo@kernel.org>, "Matthias Brugger" <matthias.bgg@gmail.com>, "Landen Chao" <Landen.Chao@mediatek.com>, "Sean Wang" <sean.wang@mediatek.com>, "DENG Qingfang" <dqfext@gmail.com>
> Betreff: Re: Choose a default DSA CPU port
>
> On Fri, Feb 24, 2023 at 09:44:43PM +0100, Frank Wunderlich wrote:
> > 6.1.12 is clean and i get 940 Mbit/s over gmac0/port6
> 
> Sounds like something which could be bisected?

tried this, and got network completely broken on third step

git bisect start
# good: [830b3c68c1fb1e9176028d02ef86f3cf76aa2476] Linux 6.1
git bisect good 830b3c68c1fb1e9176028d02ef86f3cf76aa2476
# bad: [c9c3395d5e3dcc6daee66c6908354d47bf98cb0c] Linux 6.2
git bisect bad c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
# good: [1ca06f1c1acecbe02124f14a37cce347b8c1a90c] Merge tag 'xtensa-20221213' of https://github.com/jcmvbkbc/linux-xtensa
git bisect good 1ca06f1c1acecbe02124f14a37cce347b8c1a90c

$ git logone -1
b83a7080d300 2022-12-16 Merge tag 'staging-6.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging  (HEAD)

wan and eth0 are up, but no traffic :(

root@bpi-r2:~# ip link set eth0 up
[  259.865441] mtk_soc_eth 1b100000.ethernet eth0: configuring for fixed/trgmii 
link mode
[  259.873639] mtk_soc_eth 1b100000.ethernet eth0: Link is Up - 1Gbps/Full - flo
w control rx/tx
[  259.882175] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
root@bpi-r2:~# 
root@bpi-r2:~# ip link set wan up
[  269.651154] mt7530 mdio-bus:00 wan: configuring for phy/gmii link mode
root@bpi-r2:~# [  272.742227] mt7530 mdio-bus:00 wan: Link is Up - 1Gbps/Full - 
flow control rx/tx
[  272.749678] IPv6: ADDRCONF(NETDEV_CHANGE): wan: link becomes ready

root@bpi-r2:~# ip a
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group defaul
t qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host 
       valid_lft forever preferred_lft forever
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1504 qdisc mq state UP group defa
ult qlen 1000
    link/ether 3a:69:cb:48:04:40 brd ff:ff:ff:ff:ff:ff
    inet6 fe80::3869:cbff:fe48:440/64 scope link 
       valid_lft forever preferred_lft forever
3: sit0@NONE: <NOARP> mtu 1480 qdisc noop state DOWN group default qlen 1000
    link/sit 0.0.0.0 brd 0.0.0.0
4: wan@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP g
roup default qlen 1000
    link/ether 08:22:33:44:55:77 brd ff:ff:ff:ff:ff:ff
    inet 192.168.0.11/24 scope global wan
       valid_lft forever preferred_lft forever
    inet6 fe80::a22:33ff:fe44:5577/64 scope link 
       valid_lft forever preferred_lft forever
5: lan0@eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
 qlen 1000
    link/ether 3a:5d:98:f7:50:8b brd ff:ff:ff:ff:ff:ff
6: lan1@eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
 qlen 1000
    link/ether 3e:de:03:53:13:70 brd ff:ff:ff:ff:ff:ff
7: lan2@eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
 qlen 1000
    link/ether 66:8a:45:e7:49:14 brd ff:ff:ff:ff:ff:ff
8: lan3@eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default
 qlen 1000
    link/ether 0a:81:22:f8:21:57 brd ff:ff:ff:ff:ff:ff
root@bpi-r2:~# 
root@bpi-r2:~# 
root@bpi-r2:~# 
root@bpi-r2:~# ping 192.168.0.21
PING 192.168.0.21 (192.168.0.21) 56(84) bytes of data.
From 192.168.0.11 icmp_seq=1 Destination Host Unreachable
From 192.168.0.11 icmp_seq=2 Destination Host Unreachable
From 192.168.0.11 icmp_seq=3 Destination Host Unreachable
^C
--- 192.168.0.21 ping statistics ---
4 packets transmitted, 0 received, +3 errors, 100% packet loss, time 3111ms
pipe 4
root@bpi-r2:~# ethtool eth0
Settings for eth0:
        Supported ports: [ MII ]
        Supported link modes:   1000baseT/Full
        Supported pause frame use: Symmetric Receive-only
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  1000baseT/Full
        Advertised pause frame use: Symmetric Receive-only
        Advertised auto-negotiation: Yes
        Advertised FEC modes: Not reported
        Link partner advertised link modes:  1000baseT/Full
        Link partner advertised pause frame use: Symmetric
        Link partner advertised auto-negotiation: No
        Link partner advertised FEC modes: Not reported
        Speed: 1000Mb/s
        Duplex: Full
        Auto-negotiation: on
        Port: MII
        PHYAD: 0
        Transceiver: internal
        Current message level: 0x000000ff (255)
                               drv probe link timer ifdown ifup rx_err tx_err
        Link detected: yes
root@bpi-r2:~# ethtool -S eth0
NIC statistics:
     tx_bytes: 7342
     tx_packets: 90
     tx_skip: 0
     tx_collisions: 0
     rx_bytes: 9980
     rx_packets: 105
     rx_overflow: 0
     rx_fcs_errors: 0
     rx_short_errors: 0
     rx_long_errors: 0
     rx_checksum_errors: 0
     rx_flow_control_packets: 0
     rx_xdp_redirect: 0
     rx_xdp_pass: 0
     rx_xdp_drop: 0
     rx_xdp_tx: 0
     rx_xdp_tx_errors: 0
     tx_xdp_xmit: 0
     tx_xdp_xmit_errors: 0
     p06_TxDrop: 0
     p06_TxCrcErr: 0
     p06_TxUnicast: 21
     p06_TxMulticast: 80
     p06_TxBroadcast: 4
     p06_TxCollision: 0
     p06_TxSingleCollision: 0
     p06_TxMultipleCollision: 0
     p06_TxDeferred: 0
     p06_TxLateCollision: 0
     p06_TxExcessiveCollistion: 0
     p06_TxPause: 0
     p06_TxPktSz64: 0
     p06_TxPktSz65To127: 93
     p06_TxPktSz128To255: 4
     p06_TxPktSz256To511: 8
     p06_TxPktSz512To1023: 0
     p06_Tx1024ToMax: 0
     p06_TxBytes: 10400
     p06_RxDrop: 0
     p06_RxFiltering: 30
     p06_RxUnicast: 0
     p06_RxMulticast: 69
     p06_RxBroadcast: 21
     p06_RxAlignErr: 0
     p06_RxCrcErr: 0
     p06_RxUnderSizeErr: 0
     p06_RxFragErr: 0
     p06_RxOverSzErr: 0
     p06_RxJabberErr: 0
     p06_RxPause: 0
     p06_RxPktSz64: 25
     p06_RxPktSz65To127: 65
     p06_RxPktSz128To255: 0
     p06_RxPktSz256To511: 0
     p06_RxPktSz512To1023: 0
     p06_RxPktSz1024ToMax: 0
     p06_RxBytes: 7342
     p06_RxCtrlDrop: 0
     p06_RxIngressDrop: 0
     p06_RxArlDrop: 0
root@bpi-r2:~# ethtool -S wan
NIC statistics:
     tx_packets: 60
     tx_bytes: 3932
     rx_packets: 10
     rx_bytes: 1848
     TxDrop: 0
     TxCrcErr: 0
     TxUnicast: 0
     TxMulticast: 39
     TxBroadcast: 21
     TxCollision: 0
     TxSingleCollision: 0
     TxMultipleCollision: 0
     TxDeferred: 0
     TxLateCollision: 0
     TxExcessiveCollistion: 0
     TxPause: 0
     TxPktSz64: 25
     TxPktSz65To127: 35
     TxPktSz128To255: 0
     TxPktSz256To511: 0
     TxPktSz512To1023: 0
     Tx1024ToMax: 0
     TxBytes: 4574
     RxDrop: 0
     RxFiltering: 0
     RxUnicast: 21
     RxMulticast: 86
     RxBroadcast: 4
     RxAlignErr: 0
     RxCrcErr: 0
     RxUnderSizeErr: 0
     RxFragErr: 0
     RxOverSzErr: 0
     RxJabberErr: 0
     RxPause: 0
     RxPktSz64: 91
     RxPktSz65To127: 12
     RxPktSz128To255: 0
     RxPktSz256To511: 8
     RxPktSz512To1023: 0
     RxPktSz1024ToMax: 0
     RxBytes: 10364
     RxCtrlDrop: 0
     RxIngressDrop: 0
     RxArlDrop: 0
root@bpi-r2:~# 

checked commits at this point for mt7530 dsa driver and mtk-eth driver, first has no changes, but mediatek-driver has a bunch of commits which may break...most of them are wed-specific which is not available/enabled on mt7623.

$ git logone -20 -- drivers/net/ethernet/mediatek/
587585e1bbeb 2022-12-07 net: ethernet: mtk_wed: fix possible deadlock if mtk_wed_wo_init fails 
c79e0af5ae5e 2022-12-07 net: ethernet: mtk_wed: fix some possible NULL pointer dereferences 
e22dcbc9aa32 2022-12-05 net: ethernet: mtk_wed: Fix missing of_node_put() in mtk_wed_wo_hardware_init() 
ed883bec679b 2022-12-05 net: ethernet: mtk_wed: add reset to rx_ring_setup callback 
c9f8d73645b6 2022-12-03 net: mtk_eth_soc: enable flow offload support for MT7986 SoC 
65e6af6cebef 2022-12-01 net: ethernet: mtk_wed: fix sleep while atomic in mtk_wed_wo_queue_refill 
f2bb566f5c97 2022-11-29 Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net 
23dca7a90017 2022-11-24 net: ethernet: mtk_wed: add reset to tx_ring_setup callback 
b08134c6e109 2022-11-24 net: ethernet: mtk_wed: add mtk_wed_rx_reset routine 
f78cd9c783e0 2022-11-24 net: ethernet: mtk_wed: update mtk_wed_stop 
92b1169660eb 2022-11-24 net: ethernet: mtk_wed: move MTK_WDMA_RESET_IDX_TX configuration in mtk_wdma_tx_reset 
b0488c4598a5 2022-11-24 net: ethernet: mtk_wed: return status value in mtk_wdma_rx_reset 
a66d79ee0bd5 2022-11-24 net: ethernet: mtk_wed: add wcid overwritten support for wed v1 

603ea5e7ffa7 2022-11-20 net: ethernet: mtk_eth_soc: fix memory leak in error path <<<<<<< in 6.1 from here
8110437e5961 2022-11-20 net: ethernet: mtk_eth_soc: fix resource leak in error path 
3213f808ae21 2022-11-20 net: ethernet: mtk_eth_soc: fix potential memory leak in mtk_rx_alloc() 
ef8c373bd91d 2022-11-17 net: ethernet: mtk_eth_soc: fix RSTCTRL_PPE{0,1} definitions 
8bd8dcc5e47f 2022-11-16 net: ethernet: mediatek: ppe: assign per-port queues for offloaded traffic 
f63959c7eec3 2022-11-16 net: ethernet: mtk_eth_soc: implement multi-queue support for per-port queues 
71ba8e4891c7 2022-11-16 net: ethernet: mtk_eth_soc: avoid port_mg assignment on MT7622 and newer 
frank@frank-G5:/media/data_nvme/git/kernel/BPI-R2-4.14 (HEAD) [1M46U]
$ git logone -10 v6.1 -- drivers/net/ethernet/mediatek/
603ea5e7ffa7 2022-11-20 net: ethernet: mtk_eth_soc: fix memory leak in error path 
8110437e5961 2022-11-20 net: ethernet: mtk_eth_soc: fix resource leak in error path 
3213f808ae21 2022-11-20 net: ethernet: mtk_eth_soc: fix potential memory leak in mtk_rx_alloc() 
f70074140524 2022-11-17 net: ethernet: mtk_eth_soc: fix error handling in mtk_open() 
b0c09c7f08c2 2022-11-07 net: ethernet: mtk-star-emac: disable napi when connect and start PHY failed in mtk_star_enable() 
402fe7a57287 2022-10-17 net: ethernet: mediatek: ppe: Remove the unused function mtk_foe_entry_usable() 
e0bb4659e235 2022-10-17 net: ethernet: mtk_eth_wed: add missing of_node_put() 
9d4f20a476ca 2022-10-17 net: ethernet: mtk_eth_wed: add missing put_device() in mtk_wed_add_hw() 
b3d0d98179d6 2022-10-17 net: ethernet: mtk_eth_soc: fix possible memory leak in mtk_probe() 
4af609b216e8 2022-10-06 net: ethernet: mediatek: Remove -Warray-bounds exception


$ git logone -10 drivers/net/dsa/mt7530.c
accc3b4a572b 2022-09-29 Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net 
728c2af6ad8c 2022-09-17 net: mt7531: ensure all MACs are powered down before reset 
42bc4fafe359 2022-09-17 net: mt7531: only do PLL once after the reset 
e19de30d2080 2022-09-21 net: dsa: mt7530: add support for in-band link status 
ebe48922c0c4 2022-09-21 net: dsa: mt7530: remove unnecessary dev_set_drvdata() 
1f9a6abecf53 2022-06-10 net: dsa: mt7530: get cpu-port via dp->cpu_dp instead of constant 
6e19bc26cccd 2022-06-10 net: dsa: mt7530: rework mt753[01]_setup 
a9c317417c27 2022-06-10 net: dsa: mt7530: rework mt7530_hw_vlan_{add,del} 
c8227d568ddf 2022-05-05 Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net 
a9e9b091a1c1 2022-04-28 net: dsa: mt7530: add missing of_node_put() in mt7530_setup()

Aw: Re: Choose a default DSA CPU port

[Frank Wunderlich]

Hi

> Gesendet: Freitag, 24. Februar 2023 um 22:08 Uhr
> Von: "Vladimir Oltean" <olteanv@gmail.com>

> On Fri, Feb 24, 2023 at 09:44:43PM +0100, Frank Wunderlich wrote:
> > 6.1.12 is clean and i get 940 Mbit/s over gmac0/port6
>
> Sounds like something which could be bisected?

managed to do a full bisect...

most steps needed the fix from Vladimir (1a3245fe0cf84e630598da4ab110a5f8a2d6730d net: ethernet: mtk_eth_soc: fix DSA TX tag hwaccel for switch port 0)

here is the result:

f63959c7eec3151c30a2ee0d351827b62e742dcb is the first bad commit
commit f63959c7eec3151c30a2ee0d351827b62e742dcb
Author: Felix Fietkau <nbd@nbd.name>
Date:   Wed Nov 16 09:07:32 2022 +0100

    net: ethernet: mtk_eth_soc: implement multi-queue support for per-port queues

    When sending traffic to multiple ports with different link speeds, queued
    packets to one port can drown out tx to other ports.
    In order to better handle transmission to multiple ports, use the hardware
    shaper feature to implement weighted fair queueing between ports.
    Weight and maximum rate are automatically adjusted based on the link speed
    of the port.
    The first 3 queues are unrestricted and reserved for non-DSA direct tx on
    GMAC ports. The following queues are automatically assigned by the MTK DSA
    tag driver based on the target port number.
    The PPE offload code configures the queues for offloaded traffic in the same
    way.
    This feature is only supported on devices supporting QDMA. All queues still
    share the same DMA ring and descriptor pool.

    Signed-off-by: Felix Fietkau <nbd@nbd.name>
    Link: https://lore.kernel.org/r/20221116080734.44013-5-nbd@nbd.name
    Signed-off-by: Jakub Kicinski <kuba@kernel.org>

 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 281 +++++++++++++++++++++++-----
 drivers/net/ethernet/mediatek/mtk_eth_soc.h |  26 ++-
 2 files changed, 258 insertions(+), 49 deletions(-)
frank@frank-G5:/media/data_nvme/git/kernel/BPI-R2-4.14 (HEAD) [1M46U]
$ git bisect log
git bisect start
# good: [830b3c68c1fb1e9176028d02ef86f3cf76aa2476] Linux 6.1
git bisect good 830b3c68c1fb1e9176028d02ef86f3cf76aa2476
# good: [830b3c68c1fb1e9176028d02ef86f3cf76aa2476] Linux 6.1
git bisect good 830b3c68c1fb1e9176028d02ef86f3cf76aa2476
# bad: [c9c3395d5e3dcc6daee66c6908354d47bf98cb0c] Linux 6.2
git bisect bad c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
# good: [1ca06f1c1acecbe02124f14a37cce347b8c1a90c] Merge tag 'xtensa-20221213' of https://github.com/jcmvbkbc/linux-xtensa
git bisect good 1ca06f1c1acecbe02124f14a37cce347b8c1a90c
# bad: [b83a7080d30032cf70832bc2bb04cc342e203b88] Merge tag 'staging-6.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging
git bisect bad b83a7080d30032cf70832bc2bb04cc342e203b88
# bad: [7e68dd7d07a28faa2e6574dd6b9dbd90cdeaae91] Merge tag 'net-next-6.2' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next
git bisect bad 7e68dd7d07a28faa2e6574dd6b9dbd90cdeaae91
# bad: [7e68dd7d07a28faa2e6574dd6b9dbd90cdeaae91] Merge tag 'net-next-6.2' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next
git bisect bad 7e68dd7d07a28faa2e6574dd6b9dbd90cdeaae91
# good: [c609d739947894d7370eae4cf04eb2c49e910bcf] Merge tag 'wireless-next-2022-11-18' of git://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next
git bisect good c609d739947894d7370eae4cf04eb2c49e910bcf
# good: [c609d739947894d7370eae4cf04eb2c49e910bcf] Merge tag 'wireless-next-2022-11-18' of git://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next
git bisect good c609d739947894d7370eae4cf04eb2c49e910bcf
# bad: [32163491c0c205ffb1596baf9c308dee5338ae94] Merge branch 'r8169-irq-coalesce'
git bisect bad 32163491c0c205ffb1596baf9c308dee5338ae94
# bad: [32163491c0c205ffb1596baf9c308dee5338ae94] Merge branch 'r8169-irq-coalesce'
git bisect bad 32163491c0c205ffb1596baf9c308dee5338ae94
# good: [01f856ae6d0ca5ad0505b79bf2d22d7ca439b2a1] Merge tag 'net-6.1-rc8-2' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
git bisect good 01f856ae6d0ca5ad0505b79bf2d22d7ca439b2a1
# good: [662233731d66cf41e7494e532e702849c8ce18f3] i2c: core: Introduce i2c_client_get_device_id helper function
git bisect good 662233731d66cf41e7494e532e702849c8ce18f3
# good: [d0c006402e7941558e5283ae434e2847c7999378] jump_label: Use atomic_try_cmpxchg() in static_key_slow_inc_cpuslocked()
git bisect good d0c006402e7941558e5283ae434e2847c7999378
# bad: [a61474c41e8c530c54a26db4f5434f050ef7718d] nfp: ethtool: support reporting link modes
git bisect bad a61474c41e8c530c54a26db4f5434f050ef7718d
# bad: [c672e37279896f570cfa44926d57497e8d16033b] octeontx2-pf: Add support to filter packet based on IP fragment
git bisect bad c672e37279896f570cfa44926d57497e8d16033b
# bad: [94ef6fad3bf317b43cdc59ba171dff2486e59975] net: dsa: move headers exported by master.c to master.h
git bisect bad 94ef6fad3bf317b43cdc59ba171dff2486e59975
# bad: [418e0721d408e90564b22d4c74342557b7911d77] Merge branch 'gve-alternate-missed-completions'
git bisect bad 418e0721d408e90564b22d4c74342557b7911d77
# bad: [f63959c7eec3151c30a2ee0d351827b62e742dcb] net: ethernet: mtk_eth_soc: implement multi-queue support for per-port queues
git bisect bad f63959c7eec3151c30a2ee0d351827b62e742dcb
# good: [8cf4f8c7d99addb6c2c2273fac7c20ca7c50db45] Merge tag 'rxrpc-next-20221116' of git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs
git bisect good 8cf4f8c7d99addb6c2c2273fac7c20ca7c50db45
# good: [dbc4af768ba1670018c053b35a8613b811874f9c] net: fman: remove reference to non-existing config PCS
git bisect good dbc4af768ba1670018c053b35a8613b811874f9c
# good: [f4b2fa2c25e1ade78f766aa82e733a0b5198d484] net: ethernet: mtk_eth_soc: drop packets to WDMA if the ring is full
git bisect good f4b2fa2c25e1ade78f766aa82e733a0b5198d484
# good: [71ba8e4891c799f9f79ea219f155ac795750af41] net: ethernet: mtk_eth_soc: avoid port_mg assignment on MT7622 and newer
git bisect good 71ba8e4891c799f9f79ea219f155ac795750af41
# first bad commit: [f63959c7eec3151c30a2ee0d351827b62e742dcb] net: ethernet: mtk_eth_soc: implement multi-queue support for per-port queues

Re: Aw: Re: Choose a default DSA CPU port

[Arınç ÜNAL]

On 25.02.2023 16:50, Frank Wunderlich wrote:
> Hi
> 
>> Gesendet: Freitag, 24. Februar 2023 um 22:08 Uhr
>> Von: "Vladimir Oltean" <olteanv@gmail.com>
> 
>> On Fri, Feb 24, 2023 at 09:44:43PM +0100, Frank Wunderlich wrote:
>>> 6.1.12 is clean and i get 940 Mbit/s over gmac0/port6
>>
>> Sounds like something which could be bisected?
> 
> managed to do a full bisect...
> 
> most steps needed the fix from Vladimir (1a3245fe0cf84e630598da4ab110a5f8a2d6730d net: ethernet: mtk_eth_soc: fix DSA TX tag hwaccel for switch port 0)
> 
> here is the result:
> 
> f63959c7eec3151c30a2ee0d351827b62e742dcb is the first bad commit

Thanks a lot for finding this. I can confirm reverting this fixes the 
low throughput on my Bananapi BPI-R2 as well.

$ iperf3 -c 192.168.2.1 -R
Connecting to host 192.168.2.1, port 5201
Reverse mode, remote host 192.168.2.1 is sending
[  5] local 192.168.2.2 port 56840 connected to 192.168.2.1 port 5201
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-1.00   sec  74.6 MBytes   626 Mbits/sec
[  5]   1.00-2.00   sec  74.4 MBytes   624 Mbits/sec
[  5]   2.00-3.00   sec  74.4 MBytes   624 Mbits/sec
[  5]   3.00-4.00   sec  74.3 MBytes   624 Mbits/sec
[  5]   4.00-5.00   sec  74.4 MBytes   624 Mbits/sec
[  5]   5.00-6.00   sec  74.3 MBytes   623 Mbits/sec
[  5]   6.00-7.00   sec  74.4 MBytes   624 Mbits/sec
[  5]   7.00-8.00   sec  74.3 MBytes   623 Mbits/sec
[  5]   8.00-9.00   sec  74.4 MBytes   624 Mbits/sec
[  5]   9.00-10.00  sec  74.3 MBytes   624 Mbits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec   745 MBytes   625 Mbits/sec    0             sender
[  5]   0.00-10.00  sec   744 MBytes   624 Mbits/sec 
receiver

After reverting f63959c7eec3151c30a2ee0d351827b62e742dcb (and removing 
the lines that appeared on HEAD which caused a conflict):

$ iperf3 -c 192.168.2.1 -R
Connecting to host 192.168.2.1, port 5201
Reverse mode, remote host 192.168.2.1 is sending
[  5] local 192.168.2.2 port 36364 connected to 192.168.2.1 port 5201
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-1.00   sec   112 MBytes   938 Mbits/sec
[  5]   1.00-2.00   sec   112 MBytes   939 Mbits/sec
[  5]   2.00-3.00   sec   112 MBytes   939 Mbits/sec
[  5]   3.00-4.00   sec   112 MBytes   939 Mbits/sec
[  5]   4.00-5.00   sec   112 MBytes   939 Mbits/sec
[  5]   5.00-6.00   sec   112 MBytes   939 Mbits/sec
[  5]   6.00-7.00   sec   112 MBytes   939 Mbits/sec
[  5]   7.00-8.00   sec   112 MBytes   939 Mbits/sec
[  5]   8.00-9.00   sec   112 MBytes   939 Mbits/sec
[  5]   9.00-10.00  sec   112 MBytes   939 Mbits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec  1.09 GBytes   940 Mbits/sec    0             sender
[  5]   0.00-10.00  sec  1.09 GBytes   939 Mbits/sec 
receiver

Arınç

Re: Aw: Re: Choose a default DSA CPU port

[Arınç ÜNAL]

On 25.02.2023 19:11, Arınç ÜNAL wrote:
> On 25.02.2023 16:50, Frank Wunderlich wrote:
>> Hi
>>
>>> Gesendet: Freitag, 24. Februar 2023 um 22:08 Uhr
>>> Von: "Vladimir Oltean" <olteanv@gmail.com>
>>
>>> On Fri, Feb 24, 2023 at 09:44:43PM +0100, Frank Wunderlich wrote:
>>>> 6.1.12 is clean and i get 940 Mbit/s over gmac0/port6
>>>
>>> Sounds like something which could be bisected?
>>
>> managed to do a full bisect...
>>
>> most steps needed the fix from Vladimir 
>> (1a3245fe0cf84e630598da4ab110a5f8a2d6730d net: ethernet: mtk_eth_soc: 
>> fix DSA TX tag hwaccel for switch port 0)
>>
>> here is the result:
>>
>> f63959c7eec3151c30a2ee0d351827b62e742dcb is the first bad commit
> 
> Thanks a lot for finding this. I can confirm reverting this fixes the 
> low throughput on my Bananapi BPI-R2 as well.
> 
> $ iperf3 -c 192.168.2.1 -R
> Connecting to host 192.168.2.1, port 5201
> Reverse mode, remote host 192.168.2.1 is sending
> [  5] local 192.168.2.2 port 56840 connected to 192.168.2.1 port 5201
> [ ID] Interval           Transfer     Bitrate
> [  5]   0.00-1.00   sec  74.6 MBytes   626 Mbits/sec
> [  5]   1.00-2.00   sec  74.4 MBytes   624 Mbits/sec
> [  5]   2.00-3.00   sec  74.4 MBytes   624 Mbits/sec
> [  5]   3.00-4.00   sec  74.3 MBytes   624 Mbits/sec
> [  5]   4.00-5.00   sec  74.4 MBytes   624 Mbits/sec
> [  5]   5.00-6.00   sec  74.3 MBytes   623 Mbits/sec
> [  5]   6.00-7.00   sec  74.4 MBytes   624 Mbits/sec
> [  5]   7.00-8.00   sec  74.3 MBytes   623 Mbits/sec
> [  5]   8.00-9.00   sec  74.4 MBytes   624 Mbits/sec
> [  5]   9.00-10.00  sec  74.3 MBytes   624 Mbits/sec
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bitrate         Retr
> [  5]   0.00-10.00  sec   745 MBytes   625 Mbits/sec    0             
> sender
> [  5]   0.00-10.00  sec   744 MBytes   624 Mbits/sec receiver
> 
> After reverting f63959c7eec3151c30a2ee0d351827b62e742dcb (and removing 
> the lines that appeared on HEAD which caused a conflict):
> 
> $ iperf3 -c 192.168.2.1 -R
> Connecting to host 192.168.2.1, port 5201
> Reverse mode, remote host 192.168.2.1 is sending
> [  5] local 192.168.2.2 port 36364 connected to 192.168.2.1 port 5201
> [ ID] Interval           Transfer     Bitrate
> [  5]   0.00-1.00   sec   112 MBytes   938 Mbits/sec
> [  5]   1.00-2.00   sec   112 MBytes   939 Mbits/sec
> [  5]   2.00-3.00   sec   112 MBytes   939 Mbits/sec
> [  5]   3.00-4.00   sec   112 MBytes   939 Mbits/sec
> [  5]   4.00-5.00   sec   112 MBytes   939 Mbits/sec
> [  5]   5.00-6.00   sec   112 MBytes   939 Mbits/sec
> [  5]   6.00-7.00   sec   112 MBytes   939 Mbits/sec
> [  5]   7.00-8.00   sec   112 MBytes   939 Mbits/sec
> [  5]   8.00-9.00   sec   112 MBytes   939 Mbits/sec
> [  5]   9.00-10.00  sec   112 MBytes   939 Mbits/sec
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval           Transfer     Bitrate         Retr
> [  5]   0.00-10.00  sec  1.09 GBytes   940 Mbits/sec    0             
> sender
> [  5]   0.00-10.00  sec  1.09 GBytes   939 Mbits/sec receiver

Just tested on an MT7621 Unielec U7621-06 board. MT7621 is not affected.

$ iperf3 -c 192.168.2.1 -R
Connecting to host 192.168.2.1, port 5201
Reverse mode, remote host 192.168.2.1 is sending
[  5] local 192.168.2.2 port 38946 connected to 192.168.2.1 port 5201
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-1.00   sec  96.5 MBytes   809 Mbits/sec
[  5]   1.00-2.00   sec  97.6 MBytes   819 Mbits/sec
[  5]   2.00-3.00   sec  91.5 MBytes   767 Mbits/sec
[  5]   3.00-4.00   sec  91.7 MBytes   769 Mbits/sec
[  5]   4.00-5.00   sec  90.8 MBytes   762 Mbits/sec
[  5]   5.00-6.00   sec  91.3 MBytes   766 Mbits/sec
[  5]   6.00-7.00   sec  91.5 MBytes   768 Mbits/sec
[  5]   7.00-8.00   sec  93.9 MBytes   788 Mbits/sec
[  5]   8.00-9.00   sec  93.8 MBytes   787 Mbits/sec
[  5]   9.00-10.00  sec  93.3 MBytes   783 Mbits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.01  sec   932 MBytes   782 Mbits/sec    0             sender
[  5]   0.00-10.00  sec   932 MBytes   782 Mbits/sec 
receiver

After reverting:

$ iperf3 -c 192.168.2.1 -R
Connecting to host 192.168.2.1, port 5201
Reverse mode, remote host 192.168.2.1 is sending
[  5] local 192.168.2.2 port 57204 connected to 192.168.2.1 port 5201
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-1.00   sec  92.7 MBytes   778 Mbits/sec
[  5]   1.00-2.00   sec  94.0 MBytes   788 Mbits/sec
[  5]   2.00-3.00   sec  93.4 MBytes   783 Mbits/sec
[  5]   3.00-4.00   sec  92.0 MBytes   772 Mbits/sec
[  5]   4.00-5.00   sec  92.0 MBytes   772 Mbits/sec
[  5]   5.00-6.00   sec  92.8 MBytes   779 Mbits/sec
[  5]   6.00-7.00   sec  92.4 MBytes   775 Mbits/sec
[  5]   7.00-8.00   sec  92.1 MBytes   773 Mbits/sec
[  5]   8.00-9.00   sec  92.7 MBytes   777 Mbits/sec
[  5]   9.00-10.00  sec  94.3 MBytes   791 Mbits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec   929 MBytes   779 Mbits/sec    0             sender
[  5]   0.00-10.00  sec   928 MBytes   779 Mbits/sec 
receiver

Arınç

Aw: Re: Re: Choose a default DSA CPU port

[Frank Wunderlich]

Hi,
> Gesendet: Samstag, 25. Februar 2023 um 20:56 Uhr
> Von: "Arınç ÜNAL" <arinc.unal@arinc9.com>

> On 25.02.2023 19:11, Arınç ÜNAL wrote:
> > On 25.02.2023 16:50, Frank Wunderlich wrote:

> >> f63959c7eec3151c30a2ee0d351827b62e742dcb is the first bad commit
> > 
> > Thanks a lot for finding this. I can confirm reverting this fixes the 
> > low throughput on my Bananapi BPI-R2 as well.

> Just tested on an MT7621 Unielec U7621-06 board. MT7621 is not affected.

do you have full 1G (940 Mbit/s) on mt7621 device in 6.1??

if you look at the commit you see a special handling for mt7621

if (IS_ENABLED(CONFIG_SOC_MT7621)) {
...
}else{
//all others go there including mt7623, out (t)rgmii should be here (internally SPEED_100 afair, but higher clock for trgmii):
               case SPEED_1000:
                       val |= MTK_QTX_SCH_MAX_RATE_EN |
                              FIELD_PREP(MTK_QTX_SCH_MAX_RATE_MAN, 10) |
                              FIELD_PREP(MTK_QTX_SCH_MAX_RATE_EXP, 5) |
                              FIELD_PREP(MTK_QTX_SCH_MAX_RATE_WEIGHT, 10);
                       break;
}

but i do not understand the full code as it looks like it changes the full packet-handling ;)

imho reverting is good for test, but dropping the full change is not the right way...we should wait for felix here

but back to topic...we have a patch from vladuimir which allows setting the preferred cpu-port...how do we handle mt7531 here correctly (which still sets port5 if defined and then break)?

https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c#n2383

	/* BPDU to CPU port */
	dsa_switch_for_each_cpu_port(cpu_dp, ds) {
		mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK,
			   BIT(cpu_dp->index));
		break; //<<< should we drop this break only to set all "cpu-bits"? what happens then (flooding both ports with packets?)
	}

as dsa only handles only 1 cpu-port we want the real cpu-port (preferred | first). is this bit set also if the master is changed with your follow-up patch?

regards Frank

Re: Aw: Re: Re: Choose a default DSA CPU port

[Arınç ÜNAL]

On 26.02.2023 15:12, Frank Wunderlich wrote:
> Hi,
>> Gesendet: Samstag, 25. Februar 2023 um 20:56 Uhr
>> Von: "Arınç ÜNAL" <arinc.unal@arinc9.com>
> 
>> On 25.02.2023 19:11, Arınç ÜNAL wrote:
>>> On 25.02.2023 16:50, Frank Wunderlich wrote:
> 
>>>> f63959c7eec3151c30a2ee0d351827b62e742dcb is the first bad commit
>>>
>>> Thanks a lot for finding this. I can confirm reverting this fixes the
>>> low throughput on my Bananapi BPI-R2 as well.
> 
>> Just tested on an MT7621 Unielec U7621-06 board. MT7621 is not affected.
> 
> do you have full 1G (940 Mbit/s) on mt7621 device in 6.1??

Just tried 6.1 on MT7621. The result is similar. This SoC isn't capable 
of delivering 1 Gbps throughput anyway, unless hardware flow offloading 
is used, which I don't here.

$ iperf3 -c 192.168.2.1 -R
Connecting to host 192.168.2.1, port 5201
Reverse mode, remote host 192.168.2.1 is sending
[  5] local 192.168.2.2 port 42310 connected to 192.168.2.1 port 5201
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-1.00   sec  88.6 MBytes   743 Mbits/sec
[  5]   1.00-2.00   sec  88.9 MBytes   745 Mbits/sec
[  5]   2.00-3.00   sec  90.2 MBytes   757 Mbits/sec
[  5]   3.00-4.00   sec  91.9 MBytes   771 Mbits/sec
[  5]   4.00-5.00   sec  92.0 MBytes   772 Mbits/sec
[  5]   5.00-6.00   sec  91.6 MBytes   768 Mbits/sec
[  5]   6.00-7.00   sec  91.9 MBytes   771 Mbits/sec
[  5]   7.00-8.00   sec  91.9 MBytes   771 Mbits/sec
[  5]   8.00-9.00   sec  91.8 MBytes   770 Mbits/sec
[  5]   9.00-10.00  sec  91.4 MBytes   767 Mbits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.01  sec   911 MBytes   764 Mbits/sec    0             sender
[  5]   0.00-10.00  sec   910 MBytes   764 Mbits/sec 
receiver

> 
> if you look at the commit you see a special handling for mt7621
> 
> if (IS_ENABLED(CONFIG_SOC_MT7621)) {
> ...
> }else{
> //all others go there including mt7623, out (t)rgmii should be here (internally SPEED_100 afair, but higher clock for trgmii):
>                 case SPEED_1000:
>                         val |= MTK_QTX_SCH_MAX_RATE_EN |
>                                FIELD_PREP(MTK_QTX_SCH_MAX_RATE_MAN, 10) |
>                                FIELD_PREP(MTK_QTX_SCH_MAX_RATE_EXP, 5) |
>                                FIELD_PREP(MTK_QTX_SCH_MAX_RATE_WEIGHT, 10);
>                         break;
> }
> 
> but i do not understand the full code as it looks like it changes the full packet-handling ;)

Well, whatever it's doing, it doesn't hinder the performance on MT7621. ;P

> 
> imho reverting is good for test, but dropping the full change is not the right way...we should wait for felix here

Agreed, I did that to make sure nothing else on current linux-next 
affects the performance.

> 
> but back to topic...we have a patch from vladuimir which allows setting the preferred cpu-port...how do we handle mt7531 here correctly (which still sets port5 if defined and then break)?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c#n2383
> 
> 	/* BPDU to CPU port */
> 	dsa_switch_for_each_cpu_port(cpu_dp, ds) {
> 		mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK,
> 			   BIT(cpu_dp->index));
> 		break; //<<< should we drop this break only to set all "cpu-bits"? what happens then (flooding both ports with packets?)
> 	}
> 
> as dsa only handles only 1 cpu-port we want the real cpu-port (preferred | first). is this bit set also if the master is changed with your follow-up patch?

Honestly, I don't know. I'd like to leave this to you to figure out. You 
should be able to get to the bottom of this with some testing on an 
MT7531 switch. I haven't got access to one.

Arınç

Re: Choose a default DSA CPU port

[Vladimir Oltean]

On Sun, Feb 26, 2023 at 01:12:04PM +0100, Frank Wunderlich wrote:
> but back to topic...we have a patch from vladuimir which allows
> setting the preferred cpu-port...how do we handle mt7531 here
> correctly (which still sets port5 if defined and then break)?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c#n2383
> 
> 	/* BPDU to CPU port */
> 	dsa_switch_for_each_cpu_port(cpu_dp, ds) {
> 		mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK,
> 			   BIT(cpu_dp->index));
> 		break; //<<< should we drop this break only to set all "cpu-bits"? what happens then (flooding both ports with packets?)
> 	}
> 
> as dsa only handles only 1 cpu-port we want the real cpu-port
> (preferred | first). is this bit set also if the master is changed
> with your follow-up patch?

Could you please make a best-effort attempt at describing what does the
MT7531_CFC[MT7531_CPU_PMAP_MASK] register affect? From the comment, if
affects the trapping of control packets. Does the MT7530 not have this
register? Do they behave differently? Does the register affect anything
else? If that logic is commented out, does DSA-tagged traffic still work
on MT7531? How about a bridge created with stp_state 1? I don't
understand at the moment why the hardware allows specifying a port mask
rather than a single port. Intuitively I'd say that if this field
contains more than one bit set, then control packets would be delivered
to all CPU ports that are up, effectively resulting in double processing
in Linux. So that doesn't seem to be useful. But I don't have enough data.

Re: Choose a default DSA CPU port

[Frank Wunderlich]

Am 28. Februar 2023 12:58:46 MEZ schrieb Vladimir Oltean <olteanv@gmail.com>:
>On Sun, Feb 26, 2023 at 01:12:04PM +0100, Frank Wunderlich wrote:
>> but back to topic...we have a patch from vladuimir which allows
>> setting the preferred cpu-port...how do we handle mt7531 here
>> correctly (which still sets port5 if defined and then break)?
>> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c#n2383
>> 
>> 	/* BPDU to CPU port */
>> 	dsa_switch_for_each_cpu_port(cpu_dp, ds) {
>> 		mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK,
>> 			   BIT(cpu_dp->index));
>> 		break; //<<< should we drop this break only to set all "cpu-bits"? what happens then (flooding both ports with packets?)
>> 	}
>> 
>> as dsa only handles only 1 cpu-port we want the real cpu-port
>> (preferred | first). is this bit set also if the master is changed
>> with your follow-up patch?
>
>Could you please make a best-effort attempt at describing what does the
>MT7531_CFC[MT7531_CPU_PMAP_MASK] register affect? From the comment, if
>affects the trapping of control packets. Does the MT7530 not have this
>register? Do they behave differently? Does the register affect anything
>else? If that logic is commented out, does DSA-tagged traffic still work
>on MT7531? How about a bridge created with stp_state 1? I don't
>understand at the moment why the hardware allows specifying a port mask
>rather than a single port. Intuitively I'd say that if this field
>contains more than one bit set, then control packets would be delivered
>to all CPU ports that are up, effectively resulting in double processing
>in Linux. So that doesn't seem to be useful. But I don't have enough data.

I have only this datasheet from bpi for mt7531

https://drive.google.com/file/d/1aVdQz3rbKWjkvdga8-LQ-VFXjmHR8yf9/view

On page 23 the register is defined but without additional information about setting multiple bits in this range. CFC IS CPU_FORWARD_CONTROL register and CPU_PMAP is a 8bit part of it which have a bit for selecting each port as cpu-port (0-7). I found no information about packets sent over both cpu-ports, round-robin or something else.

For mt7530 i have no such document.

The way i got from mtk some time ago was using a vlan_aware bridge for selecting a "cpu-port" for a specific user-port. At this point port5 was no cpu-port and traffic is directly routed to this port bypassing dsa and the cpu-port define in driver...afaik this way port5 was handled as userport too.
regards Frank

Re: Choose a default DSA CPU port

[Vladimir Oltean]

On Tue, Feb 28, 2023 at 02:48:13PM +0100, Frank Wunderlich wrote:
> I have only this datasheet from bpi for mt7531
> 
> https://drive.google.com/file/d/1aVdQz3rbKWjkvdga8-LQ-VFXjmHR8yf9/view
> 
> On page 23 the register is defined but without additional information
> about setting multiple bits in this range. CFC IS CPU_FORWARD_CONTROL
> register and CPU_PMAP is a 8bit part of it which have a bit for
> selecting each port as cpu-port (0-7). I found no information about
> packets sent over both cpu-ports, round-robin or something else.
> 
> For mt7530 i have no such document.

I did have the document you shared and did read that description.
I was asking for the results of some experiments because the description
isn't clear, and characterizing the impact it has seems like a more
practical way to find out.

> The way i got from mtk some time ago was using a vlan_aware bridge for
> selecting a "cpu-port" for a specific user-port. At this point port5
> was no cpu-port and traffic is directly routed to this port bypassing
> dsa and the cpu-port define in driver...afaik this way port5 was
> handled as userport too.

Sorry, I understood nothing from this. Can you rephrase?

Re: Choose a default DSA CPU port

[Frank Wunderlich]

Am 28. Februar 2023 23:56:22 MEZ schrieb Vladimir Oltean <olteanv@gmail.com>:
>On Tue, Feb 28, 2023 at 02:48:13PM +0100, Frank Wunderlich wrote:
>> I have only this datasheet from bpi for mt7531
>> 
>> https://drive.google.com/file/d/1aVdQz3rbKWjkvdga8-LQ-VFXjmHR8yf9/view
>> 
>> On page 23 the register is defined but without additional information
>> about setting multiple bits in this range. CFC IS CPU_FORWARD_CONTROL
>> register and CPU_PMAP is a 8bit part of it which have a bit for
>> selecting each port as cpu-port (0-7). I found no information about
>> packets sent over both cpu-ports, round-robin or something else.
>> 
>> For mt7530 i have no such document.
>
>I did have the document you shared and did read that description.
>I was asking for the results of some experiments because the description
>isn't clear, and characterizing the impact it has seems like a more
>practical way to find out.
>
>> The way i got from mtk some time ago was using a vlan_aware bridge for
>> selecting a "cpu-port" for a specific user-port. At this point port5
>> was no cpu-port and traffic is directly routed to this port bypassing
>> dsa and the cpu-port define in driver...afaik this way port5 was
>> handled as userport too.
>
>Sorry, I understood nothing from this. Can you rephrase?

It was a userspace way to use the second ethernet lane p5-mac1 without defining p5 as cpu-port (and so avoiding this cpu-port handling). I know it is completely different,but maybe using multiple cpu-ports require some vlan assignment inside the switch to not always flood both cpu-ports with same packets. So p5 could only accept tagged packets which has to been tagged by userport.

How can i check if same packets processed by linux  on gmacs (in case i drop the break for testing)? Looking if rx increases the same way for both macs looks not like a reliable test.

regards Frank

Re: Choose a default DSA CPU port

[Vladimir Oltean]

On Wed, Mar 01, 2023 at 07:38:10AM +0100, Frank Wunderlich wrote:
> It was a userspace way to use the second ethernet lane p5-mac1 without
> defining p5 as cpu-port (and so avoiding this cpu-port handling).
> I know it is completely different,but maybe using multiple cpu-ports
> require some vlan assignment inside the switch to not always flood
> both cpu-ports with same packets. So p5 could only accept tagged
> packets which has to been tagged by userport.
> 
> How can i check if same packets processed by linux  on gmacs (in case
> i drop the break for testing)? Looking if rx increases the same way
> for both macs looks not like a reliable test.

I'd say that using a protocol with sequence numbers would be a good way
to do that. Most obvious would be ping (ICMP), but if the code comment
is right and MT7531_CFC[MT7531_CPU_PMAP_MASK] only affects link-local
multicast packet trapping (the 01:80:c2:00:00:xx MAC DA range), then
this won't do anything, because ping is unicast.

The next most obvious thing would be L2 PTP (ptp4l -2), but since mt7530
doesn't support hw timestamping, you'd need to try software timestamping
instead ("ptp4l -i swpX -2 -P -S -m", plus the equivalent command on a
link partner).

When testing, make sure that both CPU ports are active and their DSA
masters are up! Otherwise, the switch may send duplicate link-local
packets to both CPU ports, but DSA would only process one of them,
leading us to believe that there isn't any duplication.

Putting a tcpdump -i eth0 -w eth0.pcap and a tcpdump -i eth1 -w eth1.pcap
in parallel would also be a good way to double-check.

Aw: Re: Choose a default DSA CPU port

[Frank Wunderlich]

Hi,

sorry for the delay, but have not yet found time to test :(

> Gesendet: Mittwoch, 01. März 2023 um 13:37 Uhr
> Von: "Vladimir Oltean" <olteanv@gmail.com>
> Betreff: Re: Choose a default DSA CPU port
>
> On Wed, Mar 01, 2023 at 07:38:10AM +0100, Frank Wunderlich wrote:
> > It was a userspace way to use the second ethernet lane p5-mac1 without
> > defining p5 as cpu-port (and so avoiding this cpu-port handling).
> > I know it is completely different,but maybe using multiple cpu-ports
> > require some vlan assignment inside the switch to not always flood
> > both cpu-ports with same packets. So p5 could only accept tagged
> > packets which has to been tagged by userport.
> > 
> > How can i check if same packets processed by linux  on gmacs (in case
> > i drop the break for testing)? Looking if rx increases the same way
> > for both macs looks not like a reliable test.
> 
> I'd say that using a protocol with sequence numbers would be a good way
> to do that. Most obvious would be ping (ICMP), but if the code comment
> is right and MT7531_CFC[MT7531_CPU_PMAP_MASK] only affects link-local
> multicast packet trapping (the 01:80:c2:00:00:xx MAC DA range), then
> this won't do anything, because ping is unicast.

is it possible to map this function only to mt7530, not mt7531?

as one way i would add a check for the chip

if (priv->id != ID_MT7530) { return NULL; }
//existing content for mt7531

where did you find the comment about multicast?

https://elixir.bootlin.com/linux/v6.3-rc1/source/drivers/net/dsa/mt7530.c has
"multicast" only in the packet-counters (mib_desc)

> The next most obvious thing would be L2 PTP (ptp4l -2), but since mt7530
> doesn't support hw timestamping, you'd need to try software timestamping
> instead ("ptp4l -i swpX -2 -P -S -m", plus the equivalent command on a
> link partner).

have not done anything with l2 p2p yet, and no server running...i'm not sure
i can check this the right way.

> When testing, make sure that both CPU ports are active and their DSA
> masters are up! Otherwise, the switch may send duplicate link-local
> packets to both CPU ports, but DSA would only process one of them,
> leading us to believe that there isn't any duplication.

> Putting a tcpdump -i eth0 -w eth0.pcap and a tcpdump -i eth1 -w eth1.pcap
> in parallel would also be a good way to double-check.

regards Frank

Re: Choose a default DSA CPU port

[Vladimir Oltean]

On Mon, Mar 06, 2023 at 07:20:25PM +0100, Frank Wunderlich wrote:
> is it possible to map this function only to mt7530, not mt7531?
> 
> as one way i would add a check for the chip
> 
> if (priv->id != ID_MT7530) { return NULL; }
> //existing content for mt7531

yeah, returning "NULL" to ds->ops->preferred_default_local_cpu_port()
would mean "don't know, don't care" and DSA would choose by itself.

although I feel we're not at the stage where we should discuss about
that just yet.

> where did you find the comment about multicast?

well, I didn't find "link-local multicast", but "BPDU to CPU port" and
may have ran a little bit too far with that info.

If you search for the "Bridge Group Address" keyword in IEEE 802.1Q or
IEEE 802.1D (older) documents, you'll see that STP BPDUs are sent to a
reserved multicast MAC DA of 01-80-C2-00-00-00, which is link-local,
meaning that switches don't forward it but trap it. Since I knew that,
I just assumed that "BPDU to CPU port" means "trapping of any frames
with that MAC DA to the CPU port", since if I were a hardware designer,
that's what I would do. It's possible to identify STP BPDUs (to trap
just those) by examining the LLC header, but I wouldn't bother since the
MAC DA is reserved for this kind of stuff and I'd be locking myself out
of being compatible with possible protocol changes in the future.

> https://elixir.bootlin.com/linux/v6.3-rc1/source/drivers/net/dsa/mt7530.c has
> "multicast" only in the packet-counters (mib_desc)
> 
> > The next most obvious thing would be L2 PTP (ptp4l -2), but since mt7530
> > doesn't support hw timestamping, you'd need to try software timestamping
> > instead ("ptp4l -i swpX -2 -P -S -m", plus the equivalent command on a
> > link partner).
> 
> have not done anything with l2 p2p yet, and no server running...i'm not sure
> i can check this the right way.

Anyway, it doesn't have to be PTP, it can be literally any application
using a PF_PACKET socket to send sequence-numbered packets towards a
mt7530 port with the 01:80:c2:00:00:00 MAC DA, and using 2 tcpdump
instances on the 2 GMACs to check whether packets are received once or
twice.

If this is still too complicated, just send 5 actual BPDUs and see if
you receive them on both CPU ports:

mausezahn eth0 -b 01:80:c2:00:00:00 -c 5 -t bpdu

Aw: Re: Choose a default DSA CPU port

[Frank Wunderlich]

Hi
> Gesendet: Dienstag, 07. März 2023 um 19:43 Uhr
> Von: "Vladimir Oltean" <olteanv@gmail.com>
> Betreff: Re: Choose a default DSA CPU port
>
> On Mon, Mar 06, 2023 at 07:20:25PM +0100, Frank Wunderlich wrote:
> > is it possible to map this function only to mt7530, not mt7531?
> > 
> > as one way i would add a check for the chip
> > 
> > if (priv->id != ID_MT7530) { return NULL; }
> > //existing content for mt7531
> 
> yeah, returning "NULL" to ds->ops->preferred_default_local_cpu_port()
> would mean "don't know, don't care" and DSA would choose by itself.
> 
> although I feel we're not at the stage where we should discuss about
> that just yet.
> 
> > where did you find the comment about multicast?
> 
> well, I didn't find "link-local multicast", but "BPDU to CPU port" and
> may have ran a little bit too far with that info.
> 
> If you search for the "Bridge Group Address" keyword in IEEE 802.1Q or
> IEEE 802.1D (older) documents, you'll see that STP BPDUs are sent to a
> reserved multicast MAC DA of 01-80-C2-00-00-00, which is link-local,
> meaning that switches don't forward it but trap it. Since I knew that,
> I just assumed that "BPDU to CPU port" means "trapping of any frames
> with that MAC DA to the CPU port", since if I were a hardware designer,
> that's what I would do. It's possible to identify STP BPDUs (to trap
> just those) by examining the LLC header, but I wouldn't bother since the
> MAC DA is reserved for this kind of stuff and I'd be locking myself out
> of being compatible with possible protocol changes in the future.
> 
> > https://elixir.bootlin.com/linux/v6.3-rc1/source/drivers/net/dsa/mt7530.c has
> > "multicast" only in the packet-counters (mib_desc)
> > 
> > > The next most obvious thing would be L2 PTP (ptp4l -2), but since mt7530
> > > doesn't support hw timestamping, you'd need to try software timestamping
> > > instead ("ptp4l -i swpX -2 -P -S -m", plus the equivalent command on a
> > > link partner).
> > 
> > have not done anything with l2 p2p yet, and no server running...i'm not sure
> > i can check this the right way.
> 
> Anyway, it doesn't have to be PTP, it can be literally any application
> using a PF_PACKET socket to send sequence-numbered packets towards a
> mt7530 port with the 01:80:c2:00:00:00 MAC DA, and using 2 tcpdump
> instances on the 2 GMACs to check whether packets are received once or
> twice.
> 
> If this is still too complicated, just send 5 actual BPDUs and see if
> you receive them on both CPU ports:
> 
> mausezahn eth0 -b 01:80:c2:00:00:00 -c 5 -t bpdu


hi i tried last approach on bananapi r64 as it is the one and only board i have with mt7531 which has both gmacs connected to the switch.

base is my 6.3-rc tree (modified r64 dts for having second gmac):

https://github.com/frank-w/BPI-Router-Linux/commits/6.3-rc

root@bpi-r64:~# ip a s eth0                                                                                          
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1504 qdisc mq state UP group default qlen 1000                        
    link/ether 66:ea:04:18:30:6f brd ff:ff:ff:ff:ff:ff                                                               
root@bpi-r64:~# ip a s eth1                                                                                          
3: eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000                        
    link/ether 86:97:05:8f:80:63 brd ff:ff:ff:ff:ff:ff                                                               
    inet6 fe80::8497:5ff:fe8f:8063/64 scope link                                                                     
       valid_lft forever preferred_lft forever 

root@bpi-r64:~# tcpdump -i eth0 > eth0.log &                                                                         
[1] 3774                                                                                                             
                                                                                                                    e
listening on eth0, link-type NULL (BSD loopback), snapshot length 262144 bytes                                       
root@bpi-r64:~# tcpdump -i eth1 > eth1.log &                                                                         
[2] 3779                                                                                                             
                                                                                                                    e
tcpdump: verbose output suppressed, use -v[v]... for full protocol decode                                            
listening on eth1, link-type EN10MB (Ethernet), snapshot length 262144 bytes                                         
root@bpi-r64:~# mausezahn eth0 -b 01:80:c2:00:00:00 -c 5 -t bpdu                                                     
0.00 seconds (30864 packets per second)                                                                              
root@bpi-r64:~# killall tcpdump                                                                                      
5 packets captur[ 2981.315951] mtk_soc_eth 1b100000.ethernet eth1: left promiscuous mode                             
ed                                                                                                                   
5 packets received by filter                                                                                         
0 packets dropped by kernel                                                                                          
0 packets captured                                                                                                   
0 packets received by filter                                                                                         
0 packets dropped by kernel                                                                                          
[1]-  Done                    tcpdump -i eth0 > eth0.log                                                             
[2]+  Done                    tcpdump -i eth1 > eth1.log

root@bpi-r64:~# cat eth0.log                                                                                         
20:04:47.124519 AF Unknown (25215488), length 60:                                                                    
        0x0000:  0000 66ea 0418 306f 0026 4242 0300 0000  ..f...0o.&BB....                                           
        0x0010:  0000 0000 66ea 0418 306f 0000 0000 0000  ....f...0o......                                           
        0x0020:  66ea 0418 306f 0000 0000 1400 0200 0f00  f...0o..........                                           
        0x0030:  0000 0000 0000 0000                      ........                                                   
20:04:47.124555 AF Unknown (25215488), length 60:                                                                    
        0x0000:  0000 66ea 0418 306f 0026 4242 0300 0000  ..f...0o.&BB....                                           
        0x0010:  0000 0000 66ea 0418 306f 0000 0000 0000  ....f...0o......                                           
        0x0020:  66ea 0418 306f 0000 0000 1400 0200 0f00  f...0o..........                                           
        0x0030:  0000 0000 0000 0000                      ........                                                   
20:04:47.124568 AF Unknown (25215488), length 60:                                                                    
        0x0000:  0000 66ea 0418 306f 0026 4242 0300 0000  ..f...0o.&BB....                                           
        0x0010:  0000 0000 66ea 0418 306f 0000 0000 0000  ....f...0o......                                           
        0x0020:  66ea 0418 306f 0000 0000 1400 0200 0f00  f...0o..........                                           
        0x0030:  0000 0000 0000 0000                      ........                                                   
20:04:47.124580 AF Unknown (25215488), length 60:                                                                    
        0x0000:  0000 66ea 0418 306f 0026 4242 0300 0000  ..f...0o.&BB....                                           
        0x0010:  0000 0000 66ea 0418 306f 0000 0000 0000  ....f...0o......                                           
        0x0020:  66ea 0418 306f 0000 0000 1400 0200 0f00  f...0o..........                                           
        0x0030:  0000 0000 0000 0000                      ........                                                   
20:04:47.124592 AF Unknown (25215488), length 60:                                                                    
        0x0000:  0000 66ea 0418 306f 0026 4242 0300 0000  ..f...0o.&BB....                                           
        0x0010:  0000 0000 66ea 0418 306f 0000 0000 0000  ....f...0o......                                           
        0x0020:  66ea 0418 306f 0000 0000 1400 0200 0f00  f...0o..........                                           
        0x0030:  0000 0000 0000 0000                      ........                                                   
                                                                                                                     
root@bpi-r64:~# cat eth1.log                                                                                         
                                                                                                                     
root@bpi-r64:~# 

so it looks like packets are not duplicated

regards Frank

Re: Choose a default DSA CPU port

[Frank Wunderlich]

Am 13. April 2023 20:09:52 MESZ schrieb Frank Wunderlich <frank-w@public-files.de>:
>Hi
>> Gesendet: Dienstag, 07. März 2023 um 19:43 Uhr
>> Von: "Vladimir Oltean" <olteanv@gmail.com>
>> Betreff: Re: Choose a default DSA CPU port
>>
>> On Mon, Mar 06, 2023 at 07:20:25PM +0100, Frank Wunderlich wrote:
>> > is it possible to map this function only to mt7530, not mt7531?
>> > 
>> > as one way i would add a check for the chip
>> > 
>> > if (priv->id != ID_MT7530) { return NULL; }
>> > //existing content for mt7531
>> 
>> yeah, returning "NULL" to ds->ops->preferred_default_local_cpu_port()
>> would mean "don't know, don't care" and DSA would choose by itself.
>> 
>> although I feel we're not at the stage where we should discuss about
>> that just yet.
>> 
>> > where did you find the comment about multicast?
>> 
>> well, I didn't find "link-local multicast", but "BPDU to CPU port" and
>> may have ran a little bit too far with that info.
>> 
>> If you search for the "Bridge Group Address" keyword in IEEE 802.1Q or
>> IEEE 802.1D (older) documents, you'll see that STP BPDUs are sent to a
>> reserved multicast MAC DA of 01-80-C2-00-00-00, which is link-local,
>> meaning that switches don't forward it but trap it. Since I knew that,
>> I just assumed that "BPDU to CPU port" means "trapping of any frames
>> with that MAC DA to the CPU port", since if I were a hardware designer,
>> that's what I would do. It's possible to identify STP BPDUs (to trap
>> just those) by examining the LLC header, but I wouldn't bother since the
>> MAC DA is reserved for this kind of stuff and I'd be locking myself out
>> of being compatible with possible protocol changes in the future.
>> 
>> > https://elixir.bootlin.com/linux/v6.3-rc1/source/drivers/net/dsa/mt7530.c has
>> > "multicast" only in the packet-counters (mib_desc)
>> > 
>> > > The next most obvious thing would be L2 PTP (ptp4l -2), but since mt7530
>> > > doesn't support hw timestamping, you'd need to try software timestamping
>> > > instead ("ptp4l -i swpX -2 -P -S -m", plus the equivalent command on a
>> > > link partner).
>> > 
>> > have not done anything with l2 p2p yet, and no server running...i'm not sure
>> > i can check this the right way.
>> 
>> Anyway, it doesn't have to be PTP, it can be literally any application
>> using a PF_PACKET socket to send sequence-numbered packets towards a
>> mt7530 port with the 01:80:c2:00:00:00 MAC DA, and using 2 tcpdump
>> instances on the 2 GMACs to check whether packets are received once or
>> twice.
>> 
>> If this is still too complicated, just send 5 actual BPDUs and see if
>> you receive them on both CPU ports:
>> 
>> mausezahn eth0 -b 01:80:c2:00:00:00 -c 5 -t bpdu
>
>
>hi i tried last approach on bananapi r64 as it is the one and only board i have with mt7531 which has both gmacs connected to the switch.
>
>base is my 6.3-rc tree (modified r64 dts for having second gmac):
>
>https://github.com/frank-w/BPI-Router-Linux/commits/6.3-rc
>
>root@bpi-r64:~# ip a s eth0                                                                                          
>2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1504 qdisc mq state UP group default qlen 1000                        
>    link/ether 66:ea:04:18:30:6f brd ff:ff:ff:ff:ff:ff                                                               
>root@bpi-r64:~# ip a s eth1                                                                                          
>3: eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group default qlen 1000                        
>    link/ether 86:97:05:8f:80:63 brd ff:ff:ff:ff:ff:ff                                                               
>    inet6 fe80::8497:5ff:fe8f:8063/64 scope link                                                                     
>       valid_lft forever preferred_lft forever 
>
>root@bpi-r64:~# tcpdump -i eth0 > eth0.log &                                                                         
>[1] 3774                                                                                                             
>                                                                                                                    e
>listening on eth0, link-type NULL (BSD loopback), snapshot length 262144 bytes                                       
>root@bpi-r64:~# tcpdump -i eth1 > eth1.log &                                                                         
>[2] 3779                                                                                                             
>                                                                                                                    e
>tcpdump: verbose output suppressed, use -v[v]... for full protocol decode                                            
>listening on eth1, link-type EN10MB (Ethernet), snapshot length 262144 bytes                                         
>root@bpi-r64:~# mausezahn eth0 -b 01:80:c2:00:00:00 -c 5 -t bpdu                                                     
>0.00 seconds (30864 packets per second)                                                                              
>root@bpi-r64:~# killall tcpdump                                                                                      
>5 packets captur[ 2981.315951] mtk_soc_eth 1b100000.ethernet eth1: left promiscuous mode                             
>ed                                                                                                                   
>5 packets received by filter                                                                                         
>0 packets dropped by kernel                                                                                          
>0 packets captured                                                                                                   
>0 packets received by filter                                                                                         
>0 packets dropped by kernel                                                                                          
>[1]-  Done                    tcpdump -i eth0 > eth0.log                                                             
>[2]+  Done                    tcpdump -i eth1 > eth1.log
>
>root@bpi-r64:~# cat eth0.log                                                                                         
>20:04:47.124519 AF Unknown (25215488), length 60:                                                                    
>        0x0000:  0000 66ea 0418 306f 0026 4242 0300 0000  ..f...0o.&BB....                                           
>        0x0010:  0000 0000 66ea 0418 306f 0000 0000 0000  ....f...0o......                                           
>        0x0020:  66ea 0418 306f 0000 0000 1400 0200 0f00  f...0o..........                                           
>        0x0030:  0000 0000 0000 0000                      ........                                                   
>20:04:47.124555 AF Unknown (25215488), length 60:                                                                    
>        0x0000:  0000 66ea 0418 306f 0026 4242 0300 0000  ..f...0o.&BB....                                           
>        0x0010:  0000 0000 66ea 0418 306f 0000 0000 0000  ....f...0o......                                           
>        0x0020:  66ea 0418 306f 0000 0000 1400 0200 0f00  f...0o..........                                           
>        0x0030:  0000 0000 0000 0000                      ........                                                   
>20:04:47.124568 AF Unknown (25215488), length 60:                                                                    
>        0x0000:  0000 66ea 0418 306f 0026 4242 0300 0000  ..f...0o.&BB....                                           
>        0x0010:  0000 0000 66ea 0418 306f 0000 0000 0000  ....f...0o......                                           
>        0x0020:  66ea 0418 306f 0000 0000 1400 0200 0f00  f...0o..........                                           
>        0x0030:  0000 0000 0000 0000                      ........                                                   
>20:04:47.124580 AF Unknown (25215488), length 60:                                                                    
>        0x0000:  0000 66ea 0418 306f 0026 4242 0300 0000  ..f...0o.&BB....                                           
>        0x0010:  0000 0000 66ea 0418 306f 0000 0000 0000  ....f...0o......                                           
>        0x0020:  66ea 0418 306f 0000 0000 1400 0200 0f00  f...0o..........                                           
>        0x0030:  0000 0000 0000 0000                      ........                                                   
>20:04:47.124592 AF Unknown (25215488), length 60:                                                                    
>        0x0000:  0000 66ea 0418 306f 0026 4242 0300 0000  ..f...0o.&BB....                                           
>        0x0010:  0000 0000 66ea 0418 306f 0000 0000 0000  ....f...0o......                                           
>        0x0020:  66ea 0418 306f 0000 0000 1400 0200 0f00  f...0o..........                                           
>        0x0030:  0000 0000 0000 0000                      ........                                                   
>                                                                                                                     
>root@bpi-r64:~# cat eth1.log                                                                                         
>                                                                                                                     
>root@bpi-r64:~# 
>
>so it looks like packets are not duplicated
>
>regards Frank

tried from my laptop wirh additional usb2eth adapter and see no bpdu

$ sudo mausezahn enx00e04c6c1dd3 -b 01:80:c2:00:00:00 -c 5 -t bpdu
0.00 seconds (22727 packets per second)

connected lan0 (eth0) to my laptop and wan to my switch (changed this to eth1)

eth0 is empty, eth1 contains some packets (but this is not where i sent the bpdu)

seems dsa-user-port does not accept packets to this mac...do not see them on lan0 interface too

regards Frank

Re: Choose a default DSA CPU port

[Arınç ÜNAL]

On 28.02.2023 14:58, Vladimir Oltean wrote:
> On Sun, Feb 26, 2023 at 01:12:04PM +0100, Frank Wunderlich wrote:
>> but back to topic...we have a patch from vladuimir which allows
>> setting the preferred cpu-port...how do we handle mt7531 here
>> correctly (which still sets port5 if defined and then break)?
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/dsa/mt7530.c#n2383
>>
>> 	/* BPDU to CPU port */
>> 	dsa_switch_for_each_cpu_port(cpu_dp, ds) {
>> 		mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK,
>> 			   BIT(cpu_dp->index));
>> 		break; //<<< should we drop this break only to set all "cpu-bits"? what happens then (flooding both ports with packets?)
>> 	}
>>
>> as dsa only handles only 1 cpu-port we want the real cpu-port
>> (preferred | first). is this bit set also if the master is changed
>> with your follow-up patch?
> 
> Could you please make a best-effort attempt at describing what does the
> MT7531_CFC[MT7531_CPU_PMAP_MASK] register affect? From the comment, if
> affects the trapping of control packets. Does the MT7530 not have this
> register? Do they behave differently? Does the register affect anything
> else? If that logic is commented out, does DSA-tagged traffic still work
> on MT7531? How about a bridge created with stp_state 1? I don't
> understand at the moment why the hardware allows specifying a port mask
> rather than a single port. Intuitively I'd say that if this field
> contains more than one bit set, then control packets would be delivered
> to all CPU ports that are up, effectively resulting in double processing
> in Linux. So that doesn't seem to be useful. But I don't have enough data.

I have thoroughly tested BPDU trapping using mausezahn on MT7530, and 
now MT7531.

First of all, the MT7530_MFC register exists on MT7530 and MT7531 but 
they're not the same. CPU_EN and CPU_PORT bits are specific to MT7530. 
The MT7531_CFC register and therefore the CPU_PMAP bits are specific to 
MT7531.

All these bits are used only for trapping frames. They don't affect 
anything else. DSA tagged traffic still works without setting anything 
on CPU_EN and CPU_PORT on MT7530. DSA tagged traffic still works without 
setting anything on CPU_PMAP on MT7531.

For MT7530, the port to trap the frames to is fixed since CPU_PORT is 
only of 3 bits so only one CPU port can be defined. This means that, in 
case of multiple ports used as CPU ports, any frames set for trapping to 
CPU port will be trapped to the numerically greatest CPU port.

For MT7531, after properly setting the CPU port bitmap [0], the switch 
traps the frame to the CPU port the user port is connected to. So 
there's no double processing!

I confirmed this by adding support for changing DSA conduit [1], then 
doing a BPDU test using mausezahn.

Here's the test from my computer. One port is connected to an MT7531 
port connected to eth0, the other is connected to an MT7531 port 
connected to eth1.

BPDUs only appear on eth0:
sudo mausezahn enp9s0 -c 0 -d 1s -t bpdu

BPDUs only appear on eth1:
sudo mausezahn eno1 -c 0 -d 1s -t bpdu

[0] 
https://github.com/arinc9/linux/commit/6dfa74e6383249bd017092eada0c41f178fa3d25
[1] 
https://github.com/arinc9/linux/commit/5ed8f08bd750ab5db521bf97584ddc1d43d23b2c

Arınç

Re: Choose a default DSA CPU port

[Vladimir Oltean]

On Tue, May 16, 2023 at 10:29:27PM +0300, Arınç ÜNAL wrote:
> For MT7530, the port to trap the frames to is fixed since CPU_PORT is only
> of 3 bits so only one CPU port can be defined. This means that, in case of
> multiple ports used as CPU ports, any frames set for trapping to CPU port
> will be trapped to the numerically greatest CPU port.

*that is up

Re: Choose a default DSA CPU port

[Arınç ÜNAL]

On 17.05.2023 19:10, Vladimir Oltean wrote:
> On Tue, May 16, 2023 at 10:29:27PM +0300, Arınç ÜNAL wrote:
>> For MT7530, the port to trap the frames to is fixed since CPU_PORT is only
>> of 3 bits so only one CPU port can be defined. This means that, in case of
>> multiple ports used as CPU ports, any frames set for trapping to CPU port
>> will be trapped to the numerically greatest CPU port.
> 
> *that is up

Yes, the DSA conduit interface of the CPU port must be up. Otherwise, 
these frames won't appear anywhere. I should mention this on my patch, 
thanks.

Arınç

Re: Choose a default DSA CPU port

[Vladimir Oltean]

On Wed, May 17, 2023 at 07:14:01PM +0300, Arınç ÜNAL wrote:
> On 17.05.2023 19:10, Vladimir Oltean wrote:
> > On Tue, May 16, 2023 at 10:29:27PM +0300, Arınç ÜNAL wrote:
> > > For MT7530, the port to trap the frames to is fixed since CPU_PORT is only
> > > of 3 bits so only one CPU port can be defined. This means that, in case of
> > > multiple ports used as CPU ports, any frames set for trapping to CPU port
> > > will be trapped to the numerically greatest CPU port.
> > 
> > *that is up
> 
> Yes, the DSA conduit interface of the CPU port must be up. Otherwise, these
> frames won't appear anywhere. I should mention this on my patch, thanks.

Well, mentioning it in the patch or in a comment is likely not going to
be enough. You likely have to implement ds->ops->master_state_change()
and update the MT7530_MFC register to a value that is always valid when
the conduit interfaces come and go.

Re: Choose a default DSA CPU port

[Arınç ÜNAL]

On 17.05.2023 19:16, Vladimir Oltean wrote:
> On Wed, May 17, 2023 at 07:14:01PM +0300, Arınç ÜNAL wrote:
>> On 17.05.2023 19:10, Vladimir Oltean wrote:
>>> On Tue, May 16, 2023 at 10:29:27PM +0300, Arınç ÜNAL wrote:
>>>> For MT7530, the port to trap the frames to is fixed since CPU_PORT is only
>>>> of 3 bits so only one CPU port can be defined. This means that, in case of
>>>> multiple ports used as CPU ports, any frames set for trapping to CPU port
>>>> will be trapped to the numerically greatest CPU port.
>>>
>>> *that is up
>>
>> Yes, the DSA conduit interface of the CPU port must be up. Otherwise, these
>> frames won't appear anywhere. I should mention this on my patch, thanks.
> 
> Well, mentioning it in the patch or in a comment is likely not going to
> be enough. You likely have to implement ds->ops->master_state_change()
> and update the MT7530_MFC register to a value that is always valid when
> the conduit interfaces come and go.

Thanks for pointing this out. I have done this below and confirm frames are
trapped as expected.

The frames won't necessarily be trapped to the CPU port the user port is
connected to. This operation is only there to make sure the trapped frames
always reach the CPU.

I don't (know how to) check for other conduits being up when changing the
trap port. So if a conduit is set down which results in both conduits being
down, the trap port will still be changed to the other port which is
unnecessary but it doesn't break anything.

Looking forward to your comments.

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index b5c8fdd381e5..55c11633f96f 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -961,11 +961,6 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
  	mt7530_set(priv, MT753X_MFC, MT753X_BC_FFP(BIT(port)) |
  		   MT753X_UNM_FFP(BIT(port)) | MT753X_UNU_FFP(BIT(port)));
  
-	/* Set CPU port number */
-	if (priv->id == ID_MT7621)
-		mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_MASK, MT7530_CPU_EN |
-			   MT7530_CPU_PORT(port));
-
  	/* Add the CPU port to the CPU port bitmap for MT7531 and switch on
  	 * MT7988 SoC. Any frames set for trapping to CPU port will be trapped
  	 * to the CPU port the user port is connected to.
@@ -2258,6 +2253,10 @@ mt7530_setup(struct dsa_switch *ds)
  			   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
  	}
  
+	/* Trap BPDUs to the CPU port */
+	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
+		   MT753X_BPDU_CPU_ONLY);
+
  	/* Setup VLAN ID 0 for VLAN-unaware bridges */
  	ret = mt7530_setup_vlan0(priv);
  	if (ret)
@@ -2886,6 +2885,50 @@ static const struct phylink_pcs_ops mt7530_pcs_ops = {
  	.pcs_an_restart = mt7530_pcs_an_restart,
  };
  
+static void
+mt753x_master_state_change(struct dsa_switch *ds,
+			   const struct net_device *master,
+			   bool operational)
+{
+	struct mt7530_priv *priv = ds->priv;
+	struct dsa_port *cpu_dp = master->dsa_ptr;
+	unsigned int trap_port;
+
+	/* Set the CPU port to trap frames to for MT7530. There can be only one
+	 * CPU port due to MT7530_CPU_PORT having only 3 bits. Any frames set
+	 * for trapping to CPU port will be trapped to the CPU port connected to
+	 * the most recently set up DSA conduit. If the most recently set up DSA
+	 * conduit is set down, frames will be trapped to the CPU port connected
+	 * to the other DSA conduit.
+	 */
+	if (priv->id == ID_MT7530 || priv->id == ID_MT7621) {
+		trap_port = (mt7530_read(priv, MT753X_MFC) & MT7530_CPU_PORT_MASK) >> 4;
+		dev_info(priv->dev, "trap_port is %d\n", trap_port);
+		if (operational) {
+			dev_info(priv->dev, "the conduit for cpu port %d is up\n", cpu_dp->index);
+
+			/* This check will be unnecessary if we find a way to
+			 * not change the trap port to the other port when a
+			 * conduit is set down which results in both conduits
+			 * being down.
+			 */
+			if (!(cpu_dp->index == trap_port)) {
+				dev_info(priv->dev, "trap to cpu port %d\n", cpu_dp->index);
+				mt7530_set(priv, MT753X_MFC, MT7530_CPU_EN);
+				mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_PORT_MASK, MT7530_CPU_PORT(cpu_dp->index));
+			}
+		} else {
+			if (cpu_dp->index == 5 && trap_port == 5) {
+				dev_info(priv->dev, "the conduit for cpu port 5 is down, trap frames to port 6\n");
+				mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_PORT_MASK, MT7530_CPU_PORT(6));
+			} else if (cpu_dp->index == 6 && trap_port == 6) {
+				dev_info(priv->dev, "the conduit for cpu port 6 is down, trap frames to port 5\n");
+				mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_PORT_MASK, MT7530_CPU_PORT(5));
+			}
+		}
+	}
+}
+
  static int
  mt753x_setup(struct dsa_switch *ds)
  {
@@ -2999,6 +3042,7 @@ const struct dsa_switch_ops mt7530_switch_ops = {
  	.phylink_mac_link_up	= mt753x_phylink_mac_link_up,
  	.get_mac_eee		= mt753x_get_mac_eee,
  	.set_mac_eee		= mt753x_set_mac_eee,
+	.master_state_change	= mt753x_master_state_change,
  };
  EXPORT_SYMBOL_GPL(mt7530_switch_ops);
  
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index fd2a2f726b8a..2abd3c5ce05a 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -41,8 +41,8 @@ enum mt753x_id {
  #define  MT753X_UNU_FFP(x)		(((x) & 0xff) << 8)
  #define  MT753X_UNU_FFP_MASK		MT753X_UNU_FFP(~0)
  #define  MT7530_CPU_EN			BIT(7)
-#define  MT7530_CPU_PORT(x)		((x) << 4)
-#define  MT7530_CPU_MASK		(0xf << 4)
+#define  MT7530_CPU_PORT(x)		(((x) & 0x7) << 4)
+#define  MT7530_CPU_PORT_MASK		MT7530_CPU_PORT(~0)
  #define  MT7530_MIRROR_EN		BIT(3)
  #define  MT7530_MIRROR_PORT(x)		((x) & 0x7)
  #define  MT7530_MIRROR_MASK		0x7

Arınç

Re: Choose a default DSA CPU port

[Vladimir Oltean]

On Thu, May 18, 2023 at 01:36:42PM +0300, Arınç ÜNAL wrote:
> The frames won't necessarily be trapped to the CPU port the user port is
> connected to. This operation is only there to make sure the trapped frames
> always reach the CPU.

That's kind of understated and I don't regard that as that big of a deal.
Since control packets cannot be guaranteed to be processed by the
conduit interface affine to the user port, I would go one step further
and say: don't even attempt to keep an affinity, just use for that purpose
the numerically first conduit interface that is up.

> I don't (know how to) check for other conduits being up when changing the
> trap port. So if a conduit is set down which results in both conduits being
> down, the trap port will still be changed to the other port which is
> unnecessary but it doesn't break anything.
> 
> Looking forward to your comments.
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index b5c8fdd381e5..55c11633f96f 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -961,11 +961,6 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
>  	mt7530_set(priv, MT753X_MFC, MT753X_BC_FFP(BIT(port)) |
>  		   MT753X_UNM_FFP(BIT(port)) | MT753X_UNU_FFP(BIT(port)));
> -	/* Set CPU port number */
> -	if (priv->id == ID_MT7621)
> -		mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_MASK, MT7530_CPU_EN |
> -			   MT7530_CPU_PORT(port));
> -
>  	/* Add the CPU port to the CPU port bitmap for MT7531 and switch on
>  	 * MT7988 SoC. Any frames set for trapping to CPU port will be trapped
>  	 * to the CPU port the user port is connected to.
> @@ -2258,6 +2253,10 @@ mt7530_setup(struct dsa_switch *ds)
>  			   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
>  	}
> +	/* Trap BPDUs to the CPU port */
> +	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
> +		   MT753X_BPDU_CPU_ONLY);
> +

This part will need its own patch + explanation
.
>  	/* Setup VLAN ID 0 for VLAN-unaware bridges */
>  	ret = mt7530_setup_vlan0(priv);
>  	if (ret)
> @@ -2886,6 +2885,50 @@ static const struct phylink_pcs_ops mt7530_pcs_ops = {
>  	.pcs_an_restart = mt7530_pcs_an_restart,
>  };
> +static void
> +mt753x_master_state_change(struct dsa_switch *ds,
> +			   const struct net_device *master,
> +			   bool operational)
> +{
> +	struct mt7530_priv *priv = ds->priv;
> +	struct dsa_port *cpu_dp = master->dsa_ptr;
> +	unsigned int trap_port;
> +
> +	/* Set the CPU port to trap frames to for MT7530. There can be only one
> +	 * CPU port due to MT7530_CPU_PORT having only 3 bits. Any frames set
> +	 * for trapping to CPU port will be trapped to the CPU port connected to
> +	 * the most recently set up DSA conduit. If the most recently set up DSA
> +	 * conduit is set down, frames will be trapped to the CPU port connected
> +	 * to the other DSA conduit.
> +	 */
> +	if (priv->id == ID_MT7530 || priv->id == ID_MT7621) {

Just return early which saves one level of indentation.

	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
		return;

> +		trap_port = (mt7530_read(priv, MT753X_MFC) & MT7530_CPU_PORT_MASK) >> 4;
> +		dev_info(priv->dev, "trap_port is %d\n", trap_port);
> +		if (operational) {
> +			dev_info(priv->dev, "the conduit for cpu port %d is up\n", cpu_dp->index);
> +
> +			/* This check will be unnecessary if we find a way to
> +			 * not change the trap port to the other port when a
> +			 * conduit is set down which results in both conduits
> +			 * being down.
> +			 */
> +			if (!(cpu_dp->index == trap_port)) {
> +				dev_info(priv->dev, "trap to cpu port %d\n", cpu_dp->index);
> +				mt7530_set(priv, MT753X_MFC, MT7530_CPU_EN);
> +				mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_PORT_MASK, MT7530_CPU_PORT(cpu_dp->index));
> +			}
> +		} else {
> +			if (cpu_dp->index == 5 && trap_port == 5) {
> +				dev_info(priv->dev, "the conduit for cpu port 5 is down, trap frames to port 6\n");
> +				mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_PORT_MASK, MT7530_CPU_PORT(6));
> +			} else if (cpu_dp->index == 6 && trap_port == 6) {
> +				dev_info(priv->dev, "the conduit for cpu port 6 is down, trap frames to port 5\n");
> +				mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_PORT_MASK, MT7530_CPU_PORT(5));
> +			}
> +		}

I believe that the implementation where you cache the "operational"
value of previous calls will be cleaner. Something like this (written in
an email client, so take it with a grain of salt):

struct mt7530_priv {
	unsigned long active_cpu_ports;
	...
};

	if (operational)
		priv->active_cpu_ports |= BIT(cpu_dp->index);
	else
		priv->active_cpu_ports &= ~BIT(cpu_dp->index);

	if (priv->active_cpu_ports) {
		mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_EN | MT7530_CPU_PORT_MASK,
			   MT7530_CPU_EN |
			   MT7530_CPU_PORT(__ffs(priv->active_cpu_ports)));
	} else {
		mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_EN | MT7530_CPU_PORT_MASK,
			   MT7530_CPU_PORT(0));
	}

> +	}
> +}
> +
>  static int
>  mt753x_setup(struct dsa_switch *ds)
>  {
> @@ -2999,6 +3042,7 @@ const struct dsa_switch_ops mt7530_switch_ops = {
>  	.phylink_mac_link_up	= mt753x_phylink_mac_link_up,
>  	.get_mac_eee		= mt753x_get_mac_eee,
>  	.set_mac_eee		= mt753x_set_mac_eee,
> +	.master_state_change	= mt753x_master_state_change,
>  };
>  EXPORT_SYMBOL_GPL(mt7530_switch_ops);
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index fd2a2f726b8a..2abd3c5ce05a 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -41,8 +41,8 @@ enum mt753x_id {
>  #define  MT753X_UNU_FFP(x)		(((x) & 0xff) << 8)
>  #define  MT753X_UNU_FFP_MASK		MT753X_UNU_FFP(~0)
>  #define  MT7530_CPU_EN			BIT(7)
> -#define  MT7530_CPU_PORT(x)		((x) << 4)
> -#define  MT7530_CPU_MASK		(0xf << 4)
> +#define  MT7530_CPU_PORT(x)		(((x) & 0x7) << 4)
> +#define  MT7530_CPU_PORT_MASK		MT7530_CPU_PORT(~0)
>  #define  MT7530_MIRROR_EN		BIT(3)
>  #define  MT7530_MIRROR_PORT(x)		((x) & 0x7)
>  #define  MT7530_MIRROR_MASK		0x7
> 
> Arınç

Re: Choose a default DSA CPU port

[Arınç ÜNAL]

On 18.05.2023 17:24, Vladimir Oltean wrote:
> On Thu, May 18, 2023 at 01:36:42PM +0300, Arınç ÜNAL wrote:
>> The frames won't necessarily be trapped to the CPU port the user port is
>> connected to. This operation is only there to make sure the trapped frames
>> always reach the CPU.
> 
> That's kind of understated and I don't regard that as that big of a deal.
> Since control packets cannot be guaranteed to be processed by the
> conduit interface affine to the user port, I would go one step further
> and say: don't even attempt to keep an affinity, just use for that purpose
> the numerically first conduit interface that is up.

Makes sense. Good thing the MT7531 switch is capable of having the control
packets processed by the DSA conduit interface affine to the user port so
it's only the MT7530 switch that we need to implement "don't even attempt
to keep an affinity, just use the numerically first conduit interface that
is up" for.

> 
>> I don't (know how to) check for other conduits being up when changing the
>> trap port. So if a conduit is set down which results in both conduits being
>> down, the trap port will still be changed to the other port which is
>> unnecessary but it doesn't break anything.
>>
>> Looking forward to your comments.
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index b5c8fdd381e5..55c11633f96f 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -961,11 +961,6 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
>>   	mt7530_set(priv, MT753X_MFC, MT753X_BC_FFP(BIT(port)) |
>>   		   MT753X_UNM_FFP(BIT(port)) | MT753X_UNU_FFP(BIT(port)));
>> -	/* Set CPU port number */
>> -	if (priv->id == ID_MT7621)
>> -		mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_MASK, MT7530_CPU_EN |
>> -			   MT7530_CPU_PORT(port));
>> -
>>   	/* Add the CPU port to the CPU port bitmap for MT7531 and switch on
>>   	 * MT7988 SoC. Any frames set for trapping to CPU port will be trapped
>>   	 * to the CPU port the user port is connected to.
>> @@ -2258,6 +2253,10 @@ mt7530_setup(struct dsa_switch *ds)
>>   			   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
>>   	}
>> +	/* Trap BPDUs to the CPU port */
>> +	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
>> +		   MT753X_BPDU_CPU_ONLY);
>> +
> 
> This part will need its own patch + explanation

Will split.

> .
>>   	/* Setup VLAN ID 0 for VLAN-unaware bridges */
>>   	ret = mt7530_setup_vlan0(priv);
>>   	if (ret)
>> @@ -2886,6 +2885,50 @@ static const struct phylink_pcs_ops mt7530_pcs_ops = {
>>   	.pcs_an_restart = mt7530_pcs_an_restart,
>>   };
>> +static void
>> +mt753x_master_state_change(struct dsa_switch *ds,
>> +			   const struct net_device *master,
>> +			   bool operational)
>> +{
>> +	struct mt7530_priv *priv = ds->priv;
>> +	struct dsa_port *cpu_dp = master->dsa_ptr;
>> +	unsigned int trap_port;
>> +
>> +	/* Set the CPU port to trap frames to for MT7530. There can be only one
>> +	 * CPU port due to MT7530_CPU_PORT having only 3 bits. Any frames set
>> +	 * for trapping to CPU port will be trapped to the CPU port connected to
>> +	 * the most recently set up DSA conduit. If the most recently set up DSA
>> +	 * conduit is set down, frames will be trapped to the CPU port connected
>> +	 * to the other DSA conduit.
>> +	 */
>> +	if (priv->id == ID_MT7530 || priv->id == ID_MT7621) {
> 
> Just return early which saves one level of indentation.
> 
> 	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
> 		return;

Will do.

> 
>> +		trap_port = (mt7530_read(priv, MT753X_MFC) & MT7530_CPU_PORT_MASK) >> 4;
>> +		dev_info(priv->dev, "trap_port is %d\n", trap_port);
>> +		if (operational) {
>> +			dev_info(priv->dev, "the conduit for cpu port %d is up\n", cpu_dp->index);
>> +
>> +			/* This check will be unnecessary if we find a way to
>> +			 * not change the trap port to the other port when a
>> +			 * conduit is set down which results in both conduits
>> +			 * being down.
>> +			 */
>> +			if (!(cpu_dp->index == trap_port)) {
>> +				dev_info(priv->dev, "trap to cpu port %d\n", cpu_dp->index);
>> +				mt7530_set(priv, MT753X_MFC, MT7530_CPU_EN);
>> +				mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_PORT_MASK, MT7530_CPU_PORT(cpu_dp->index));
>> +			}
>> +		} else {
>> +			if (cpu_dp->index == 5 && trap_port == 5) {
>> +				dev_info(priv->dev, "the conduit for cpu port 5 is down, trap frames to port 6\n");
>> +				mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_PORT_MASK, MT7530_CPU_PORT(6));
>> +			} else if (cpu_dp->index == 6 && trap_port == 6) {
>> +				dev_info(priv->dev, "the conduit for cpu port 6 is down, trap frames to port 5\n");
>> +				mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_PORT_MASK, MT7530_CPU_PORT(5));
>> +			}
>> +		}
> 
> I believe that the implementation where you cache the "operational"
> value of previous calls will be cleaner. Something like this (written in
> an email client, so take it with a grain of salt):
> 
> struct mt7530_priv {
> 	unsigned long active_cpu_ports;
> 	...
> };
> 
> 	if (operational)
> 		priv->active_cpu_ports |= BIT(cpu_dp->index);
> 	else
> 		priv->active_cpu_ports &= ~BIT(cpu_dp->index);
> 
> 	if (priv->active_cpu_ports) {
> 		mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_EN | MT7530_CPU_PORT_MASK,
> 			   MT7530_CPU_EN |
> 			   MT7530_CPU_PORT(__ffs(priv->active_cpu_ports)));

This is nice and simple, thank you.

> 	} else {
> 		mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_EN | MT7530_CPU_PORT_MASK,
> 			   MT7530_CPU_PORT(0));

If I understand correctly, the MT7530_CPU_EN bit here wouldn't be modified
since it's not on the set parameter. On top of this, I believe we can
completely get rid of the else case. The MT7530_CPU_PORT bits will be
overwritten when there's an active CPU port so there's no need to clear
them when there's no active CPU ports. MT7530_CPU_EN might as well stay
set.

Arınç

Re: Choose a default DSA CPU port

[Vladimir Oltean]

On Fri, May 19, 2023 at 12:00:17PM +0300, Arınç ÜNAL wrote:
> > 		mt7530_rmw(priv, MT753X_MFC, MT7530_CPU_EN | MT7530_CPU_PORT_MASK,
> > 			   MT7530_CPU_PORT(0));
> 
> If I understand correctly, the MT7530_CPU_EN bit here wouldn't be modified
> since it's not on the set parameter.

"mask": which bits are affected by the read-modify-write (rmw)
"set": which subset of "mask" remains set (the other subset, mask & ~set,
remains unset)

So no, MT7530_CPU_EN is not unmodified; it is quite cleared.

> On top of this, I believe we can completely get rid of the else case.
> The MT7530_CPU_PORT bits will be overwritten when there's an active
> CPU port so there's no need to clear them when there's no active CPU
> ports. MT7530_CPU_EN might as well stay set.

Sounds plausible.

Aw: Re: Choose a default DSA CPU port

[Frank Wunderlich]

fixed the broken network by applying this patch on top of the current position

1a3245fe0cf8 net: ethernet: mtk_eth_soc: fix DSA TX tag hwaccel for switch port 0

now ping works again but throughput is now 620Mbit/s

i try to continue the bisect (have to apply the change on each step i guess)

Choose a default DSA CPU port

[Arınç ÜNAL]

Hey there folks,

The problem is this. Frank and I have got a Bananapi BPI-R2 with MT7623 
SoC. The port5 of MT7530 switch is wired to gmac1 of the SoC. Port6 is 
wired to gmac0. Since DSA sets the first CPU port it finds on the 
devicetree, port5 becomes the CPU port for all DSA slaves.

But we'd prefer port6 since it uses trgmii while port5 uses rgmii. There 
are also some performance issues with the port5 - gmac1 link.

Now we could change it manually on userspace if the DSA subdriver 
supported changing the DSA master.

I'd like to find a solution which would work for the cases of; the 
driver not supporting changing the DSA master, or saving the effort of 
manually changing it on userspace.

The solution that came to my mind:

Introduce a DT property to designate a CPU port as the default CPU port.
If this property exists on a CPU port, that port becomes the CPU port 
for all DSA slaves.
If it doesn't exist, fallback to the first-found-cpu-port method.

Frank doesn't like this idea:

 > maybe define the default cpu in driver which gets picked up by core 
(define port6 as default if available).
 > Imho additional dts-propperty is wrong approch...it should be handled 
by driver. But cpu-port-selection is currently done in dsa-core which 
makes it a bit difficult.

What are your thoughts?

Arınç

Re: Choose a default DSA CPU port

[Vladimir Oltean]

Hi Arınç,

On Sat, Feb 18, 2023 at 08:07:53PM +0300, Arınç ÜNAL wrote:
> Hey there folks,
> 
> The problem is this. Frank and I have got a Bananapi BPI-R2 with MT7623 SoC.
> The port5 of MT7530 switch is wired to gmac1 of the SoC. Port6 is wired to
> gmac0. Since DSA sets the first CPU port it finds on the devicetree, port5
> becomes the CPU port for all DSA slaves.
> 
> But we'd prefer port6 since it uses trgmii while port5 uses rgmii. There are
> also some performance issues with the port5 - gmac1 link.
> 
> Now we could change it manually on userspace if the DSA subdriver supported
> changing the DSA master.
> 
> I'd like to find a solution which would work for the cases of; the driver
> not supporting changing the DSA master, or saving the effort of manually
> changing it on userspace.
> 
> The solution that came to my mind:
> 
> Introduce a DT property to designate a CPU port as the default CPU port.
> If this property exists on a CPU port, that port becomes the CPU port for
> all DSA slaves.
> If it doesn't exist, fallback to the first-found-cpu-port method.
> 
> Frank doesn't like this idea:
> 
> > maybe define the default cpu in driver which gets picked up by core
> (define port6 as default if available).
> > Imho additional dts-propperty is wrong approch...it should be handled by
> driver. But cpu-port-selection is currently done in dsa-core which makes it
> a bit difficult.
> 
> What are your thoughts?
> 
> Arınç

Before making any change, I believe that the first step is to be in agreement
as to what the problem is.

The current DSA device tree binding has always chosen the numerically first
CPU port as the default CPU port. This was also the case at the time when the
mt7530 driver was accepted upstream. Clearly there are cases where this choice
is not the optimal choice (from the perspective of an outside observer).
For example when another CPU port has a higher link speed. But it's not
exactly obvious that higher link speed is always better. If you have a CPU
port at a higher link speed than the user ports, but it doesn't have flow
control, then the choice is practically worse than the CPU port which operates
at the same link speed as user ports.

So the choice between RGMII and TRGMII is not immediately obvious to some code
which should take this decision automatically. And this complicates things
a lot. If there is no downside to having the kernel take a decision automatically,
generally I don't have a problem taking it. But here, I would like to hear
some strong arguments why port 6 is preferable over port 5.

If there are strong reasons to consider port 6 a primary CPU port and
port 5 a secondary one, then there is also a very valid concern of forward
compatibility between the mt7530 driver and device trees from the future
(with multiple CPU ports defined). The authors of the mt7530 driver must have
been aware of the DSA binding implementation's choice of selecting the
first CPU port as the default, but they decided to hide their head in
the sand and pretend like this issue will never crop up. The driver has
not been coded up to accept port 5 as a valid CPU port until very recently.
What should have been done (*assuming strong arguments*) is that
dsa_tree_setup_default_cpu() should have been modified from day one of
mt7530 driver introduction, such that the driver has a way of specifying
a preferred default CPU port.

In other words, the fact that the CPU port becomes port 5 when booting
on a new device tree is equally a problem for current kernels as it is
for past LTS kernels. I would like this to be treated seriously, and
current + stable kernels should behave in a reasonable manner with
device trees from the future, before support for multiple CPU ports is
added to mt7530. Forcing users to change device trees in lockstep with
the kernel is not something that I want to maintain and support, if user
questions and issues do crop up.

Since this wasn't done, the only thing we're left with is to retroactively
add this functionality to pick a preferred default CPU port, as patches
to "net" which get backported to stable kernels. Given enough forethought
in the mt7530 driver development, this should not have been necessary,
but here we are.

Now that I expressed this point of view, let me comment on why your
proposal, Arınç, solves exactly nothing.

If you add a device tree property for the preferred CPU port, you
haven't solved any of the compatibility problems:

- old kernels + new device trees will not have the logic to interpret
  that device tree property
- old device trees + new kernels will not have that device tree property

so... yeah.

Re: Choose a default DSA CPU port

[Arınç ÜNAL]

On 18.02.2023 23:52, Vladimir Oltean wrote:
> Hi Arınç,
> 
> On Sat, Feb 18, 2023 at 08:07:53PM +0300, Arınç ÜNAL wrote:
>> Hey there folks,
>>
>> The problem is this. Frank and I have got a Bananapi BPI-R2 with MT7623 SoC.
>> The port5 of MT7530 switch is wired to gmac1 of the SoC. Port6 is wired to
>> gmac0. Since DSA sets the first CPU port it finds on the devicetree, port5
>> becomes the CPU port for all DSA slaves.
>>
>> But we'd prefer port6 since it uses trgmii while port5 uses rgmii. There are
>> also some performance issues with the port5 - gmac1 link.
>>
>> Now we could change it manually on userspace if the DSA subdriver supported
>> changing the DSA master.
>>
>> I'd like to find a solution which would work for the cases of; the driver
>> not supporting changing the DSA master, or saving the effort of manually
>> changing it on userspace.
>>
>> The solution that came to my mind:
>>
>> Introduce a DT property to designate a CPU port as the default CPU port.
>> If this property exists on a CPU port, that port becomes the CPU port for
>> all DSA slaves.
>> If it doesn't exist, fallback to the first-found-cpu-port method.
>>
>> Frank doesn't like this idea:
>>
>>> maybe define the default cpu in driver which gets picked up by core
>> (define port6 as default if available).
>>> Imho additional dts-propperty is wrong approch...it should be handled by
>> driver. But cpu-port-selection is currently done in dsa-core which makes it
>> a bit difficult.
>>
>> What are your thoughts?
>>
>> Arınç
> 
> Before making any change, I believe that the first step is to be in agreement
> as to what the problem is.
> 
> The current DSA device tree binding has always chosen the numerically first
> CPU port as the default CPU port. This was also the case at the time when the
> mt7530 driver was accepted upstream. Clearly there are cases where this choice
> is not the optimal choice (from the perspective of an outside observer).
> For example when another CPU port has a higher link speed. But it's not
> exactly obvious that higher link speed is always better. If you have a CPU
> port at a higher link speed than the user ports, but it doesn't have flow
> control, then the choice is practically worse than the CPU port which operates
> at the same link speed as user ports.
> 
> So the choice between RGMII and TRGMII is not immediately obvious to some code
> which should take this decision automatically. And this complicates things
> a lot. If there is no downside to having the kernel take a decision automatically,
> generally I don't have a problem taking it. But here, I would like to hear
> some strong arguments why port 6 is preferable over port 5.

I'm leaving this to Frank to explain.

> 
> If there are strong reasons to consider port 6 a primary CPU port and
> port 5 a secondary one, then there is also a very valid concern of forward
> compatibility between the mt7530 driver and device trees from the future
> (with multiple CPU ports defined). The authors of the mt7530 driver must have
> been aware of the DSA binding implementation's choice of selecting the
> first CPU port as the default, but they decided to hide their head in
> the sand and pretend like this issue will never crop up. The driver has
> not been coded up to accept port 5 as a valid CPU port until very recently.
> What should have been done (*assuming strong arguments*) is that
> dsa_tree_setup_default_cpu() should have been modified from day one of
> mt7530 driver introduction, such that the driver has a way of specifying
> a preferred default CPU port.
> 
> In other words, the fact that the CPU port becomes port 5 when booting
> on a new device tree is equally a problem for current kernels as it is
> for past LTS kernels. I would like this to be treated seriously, and
> current + stable kernels should behave in a reasonable manner with
> device trees from the future, before support for multiple CPU ports is
> added to mt7530. Forcing users to change device trees in lockstep with
> the kernel is not something that I want to maintain and support, if user
> questions and issues do crop up.
> 
> Since this wasn't done, the only thing we're left with is to retroactively
> add this functionality to pick a preferred default CPU port, as patches
> to "net" which get backported to stable kernels. Given enough forethought
> in the mt7530 driver development, this should not have been necessary,
> but here we are.
> 
> Now that I expressed this point of view, let me comment on why your
> proposal, Arınç, solves exactly nothing.
> 
> If you add a device tree property for the preferred CPU port, you
> haven't solved any of the compatibility problems:
> 
> - old kernels + new device trees will not have the logic to interpret
>    that device tree property
> - old device trees + new kernels will not have that device tree property
> 
> so... yeah.

Makes perfect sense. I always make the assumption that once the DTs on 
the kernel source code is updated, it will be used everywhere, which is 
just not the case.

Arınç

Aw: Re: Choose a default DSA CPU port

[Frank Wunderlich]

Hi,

> Gesendet: Sonntag, 19. Februar 2023 um 08:35 Uhr
> Von: "Arınç ÜNAL" <arinc.unal@arinc9.com>
> On 18.02.2023 23:52, Vladimir Oltean wrote:

> > Before making any change, I believe that the first step is to be in agreement
> > as to what the problem is.

the problem is that the first cpu-port is not always ideal. in our case driver supports only port6 over
years till we have changed this to get r2pro working which uses only port 5 (mt7531).

so port6 is tested over long time and port 5 now comes into the view as spare cpu-port for
getting additional bandwidth.

> > The current DSA device tree binding has always chosen the numerically first
> > CPU port as the default CPU port. This was also the case at the time when the
> > mt7530 driver was accepted upstream. Clearly there are cases where this choice
> > is not the optimal choice (from the perspective of an outside observer).
> > For example when another CPU port has a higher link speed. But it's not
> > exactly obvious that higher link speed is always better. If you have a CPU
> > port at a higher link speed than the user ports, but it doesn't have flow
> > control, then the choice is practically worse than the CPU port which operates
> > at the same link speed as user ports.
> > 
> > So the choice between RGMII and TRGMII is not immediately obvious to some code
> > which should take this decision automatically. And this complicates things
> > a lot. If there is no downside to having the kernel take a decision automatically,
> > generally I don't have a problem taking it. But here, I would like to hear
> > some strong arguments why port 6 is preferable over port 5.
> 
> I'm leaving this to Frank to explain.

yes only looking at phy speed may not be the right way, but one way...i would like the hw driver
choose the right port which is used as default.

currently i try to figure out why dsa_tree_setup_cpu_ports does not use dsa_tree_find_first_cpu.
the loops looks same, first returns the first one, second skips all further which should be same and at the end it calls dsa_tree_find_first_cpu for all ports not yet having a cpu-port assigned...looks starnge to me, but maybe i 
oversee a detail.

======================================================
static struct dsa_port *dsa_tree_find_first_cpu(struct dsa_switch_tree *dst)
{
	list_for_each_entry(dp, &dst->ports, list)
		if (dsa_port_is_cpu(dp))
			return dp;
...

static int dsa_tree_setup_default_cpu(struct dsa_switch_tree *dst)
{
	cpu_dp = dsa_tree_find_first_cpu(dst);
...

static int dsa_tree_setup_cpu_ports(struct dsa_switch_tree *dst)
{
...
	list_for_each_entry(cpu_dp, &dst->ports, list) {
		if (!dsa_port_is_cpu(cpu_dp))
			continue;
...
return dsa_tree_setup_default_cpu(dst);
}

===============================================================================
so i would change dsa_tree_setup_cpu_ports to something like this (not ready):

struct dsa_switch_ops {
...
	struct dsa_port	*(*get_default_cpu_port)(struct dsa_switch *ds);


static struct dsa_port *dsa_tree_get_default_cpu(struct dsa_switch *ds)
{
	struct dsa_port *cpu_dp;
	struct dsa_switch_tree *dst = ds->dst;

	//first let driver choose a cpu_port
	if (ds->ops->get_default_cpu_port)
		cpu_dp = ds->ops->get_default_cpu_port(ds);

	//if driver callback not implemented or no result fall back to dsa core default
	if (!cpu_dp)
		cpu_dp = dsa_tree_find_first_cpu(dst);

	return cpu_dp;
}


static int dsa_tree_setup_cpu_ports(struct dsa_switch_tree *dst)
{
	struct dsa_port *cpu_dp, *dp;

	cpu_dp = dsa_tree_get_default_cpu(ds); //how to get ds from dst??
	
	//list_for_each_entry(cpu_dp, &dst->ports, list) {
	//	if (!dsa_port_is_cpu(cpu_dp))
	//		continue;

		/* Prefer a local CPU port */
		dsa_switch_for_each_port(dp, cpu_dp->ds) {
			/* Prefer the first local CPU port found */
			if (dp->cpu_dp)
				continue;

			if (dsa_port_is_user(dp) || dsa_port_is_dsa(dp))
			{
				dp->cpu_dp = cpu_dp;
				printk(KERN_ALERT "DEBUG: Passed %s %d cpu:%d set to port %s (%d)\n",__FUNCTION__,__LINE__,cpu_dp->index,dp->name,dp->index);
			}
		}
	//}

	return dsa_tree_setup_default_cpu(dst);
}

and in mt7530.c (can use other values for selecting the better one)

static struct dsa_port *mt753x_get_default_cpu(struct dsa_switch *ds)
{
	struct dsa_port *cpu_dp;
	unsigned int port = 0;

	if (dsa_is_cpu_port(ds, 6))
		port=6;
	else if (dsa_is_cpu_port(ds, 5))
		port =5;
	if (port)
		cpu_dp = dsa_to_port(ds, port);
	
	return cpu_dp;
}

static const struct dsa_switch_ops mt7530_switch_ops = {
...
	.get_default_cpu_port	= mt753x_get_default_cpu,

does not yet compile because i do not know how to get dsa_switch from dsa_switch_tree,
but basicly shows how i would do it (select the right cpu at driver level without dts properties).

> > 
> > If there are strong reasons to consider port 6 a primary CPU port and
> > port 5 a secondary one, then there is also a very valid concern of forward
> > compatibility between the mt7530 driver and device trees from the future
> > (with multiple CPU ports defined). The authors of the mt7530 driver must have
> > been aware of the DSA binding implementation's choice of selecting the
> > first CPU port as the default, but they decided to hide their head in
> > the sand and pretend like this issue will never crop up. The driver has
> > not been coded up to accept port 5 as a valid CPU port until very recently.
> > What should have been done (*assuming strong arguments*) is that
> > dsa_tree_setup_default_cpu() should have been modified from day one of
> > mt7530 driver introduction, such that the driver has a way of specifying
> > a preferred default CPU port.
> > 
> > In other words, the fact that the CPU port becomes port 5 when booting
> > on a new device tree is equally a problem for current kernels as it is
> > for past LTS kernels. I would like this to be treated seriously, and
> > current + stable kernels should behave in a reasonable manner with
> > device trees from the future, before support for multiple CPU ports is
> > added to mt7530. Forcing users to change device trees in lockstep with
> > the kernel is not something that I want to maintain and support, if user
> > questions and issues do crop up.
> > 
> > Since this wasn't done, the only thing we're left with is to retroactively
> > add this functionality to pick a preferred default CPU port, as patches
> > to "net" which get backported to stable kernels. Given enough forethought
> > in the mt7530 driver development, this should not have been necessary,
> > but here we are.
> > 
> > Now that I expressed this point of view, let me comment on why your
> > proposal, Arınç, solves exactly nothing.
> > 
> > If you add a device tree property for the preferred CPU port, you
> > haven't solved any of the compatibility problems:
> > 
> > - old kernels + new device trees will not have the logic to interpret
> >    that device tree property
> > - old device trees + new kernels will not have that device tree property
> > 
> > so... yeah.
> 
> Makes perfect sense. I always make the assumption that once the DTs on 
> the kernel source code is updated, it will be used everywhere, which is 
> just not the case.

imho there is no way to ensure both ways backwards compatible..in the moment you add port5 it
will be the default cpu-port which is what we try to "fix" here. either driver should select the better one (drivercode not backported if no real fix) or it needs a setting in dts (which is not read in older driver/core).

regards Frank

Re: Choose a default DSA CPU port

[Vladimir Oltean]

On Sun, Feb 19, 2023 at 10:49:24AM +0100, Frank Wunderlich wrote:
> > I'm leaving this to Frank to explain.
> 
> yes only looking at phy speed may not be the right way, but one way...i would like the hw driver
> choose the right port which is used as default.
> 
> currently i try to figure out why dsa_tree_setup_cpu_ports does not use dsa_tree_find_first_cpu.
> the loops looks same, first returns the first one, second skips all further which should be same
> and at the end it calls dsa_tree_find_first_cpu for all ports not yet having a cpu-port assigned...

Because by the time dsa_tree_find_first_cpu() has been called, all user
and cascade ports got their dp->cpu_dp pointers resolved by the earlier
logic, which prefers a CPU port local to that switch before assigning
CPU ports which are potentially on remote switches.

> looks starnge to me, but maybe i oversee a detail.

Yeah, at least one.

> does not yet compile because i do not know how to get dsa_switch from dsa_switch_tree,
> but basicly shows how i would do it (select the right cpu at driver level without dts properties).

Basically what you want is something like this:

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3a15015bc409..c4771a848319 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -393,6 +393,20 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
 		mt7530_write(priv, MT7530_ATA1 + (i * 4), reg[i]);
 }
 
+/* If port 6 is available as a CPU port, always prefer that as the default,
+ * otherwise don't care.
+ */
+static struct dsa_port *
+mt7530_preferred_default_local_cpu_port(struct dsa_switch *ds)
+{
+	struct dsa_port *cpu_dp = dsa_to_port(ds, 6);
+
+	if (dsa_port_is_cpu(cpu_dp))
+		return cpu_dp;
+
+	return NULL;
+}
+
 /* Setup TX circuit including relevant PAD and driving */
 static int
 mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
@@ -3152,6 +3166,7 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
 static const struct dsa_switch_ops mt7530_switch_ops = {
 	.get_tag_protocol	= mtk_get_tag_protocol,
 	.setup			= mt753x_setup,
+	.preferred_default_local_cpu_port = mt7530_preferred_default_local_cpu_port,
 	.get_strings		= mt7530_get_strings,
 	.get_ethtool_stats	= mt7530_get_ethtool_stats,
 	.get_sset_count		= mt7530_get_sset_count,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index a15f17a38eca..5db43be2a464 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -973,6 +973,14 @@ struct dsa_switch_ops {
 			       struct phy_device *phy);
 	void	(*port_disable)(struct dsa_switch *ds, int port);
 
+	/*
+	 * Compatibility between device trees defining multiple CPU ports and
+	 * drivers which are not ok to use by default the numerically first CPU
+	 * port of a switch for its local ports. This can return NULL, meaning
+	 * "don't know/don't care".
+	 */
+	struct dsa_port *(*preferred_default_local_cpu_port)(struct dsa_switch *ds);
+
 	/*
 	 * Port's MAC EEE settings
 	 */
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index e5f156940c67..6cd8607a3928 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -402,6 +402,24 @@ static int dsa_tree_setup_default_cpu(struct dsa_switch_tree *dst)
 	return 0;
 }
 
+static struct dsa_port *
+dsa_switch_preferred_default_local_cpu_port(struct dsa_switch *ds)
+{
+	struct dsa_port *cpu_dp;
+
+	if (!ds->ops->preferred_default_local_cpu_port)
+		return NULL;
+
+	cpu_dp = ds->ops->preferred_default_local_cpu_port(ds);
+	if (!cpu_dp)
+		return NULL;
+
+	if (WARN_ON(!dsa_port_is_cpu(cpu_dp) || cpu_dp->ds != ds))
+		return NULL;
+
+	return cpu_dp;
+}
+
 /* Perform initial assignment of CPU ports to user ports and DSA links in the
  * fabric, giving preference to CPU ports local to each switch. Default to
  * using the first CPU port in the switch tree if the port does not have a CPU
@@ -409,12 +427,16 @@ static int dsa_tree_setup_default_cpu(struct dsa_switch_tree *dst)
  */
 static int dsa_tree_setup_cpu_ports(struct dsa_switch_tree *dst)
 {
-	struct dsa_port *cpu_dp, *dp;
+	struct dsa_port *preferred_cpu_dp, *cpu_dp, *dp;
 
 	list_for_each_entry(cpu_dp, &dst->ports, list) {
 		if (!dsa_port_is_cpu(cpu_dp))
 			continue;
 
+		preferred_cpu_dp = dsa_switch_preferred_default_local_cpu_port(cpu_dp->ds);
+		if (preferred_cpu_dp && preferred_cpu_dp != cpu_dp)
+			continue;
+
 		/* Prefer a local CPU port */
 		dsa_switch_for_each_port(dp, cpu_dp->ds) {
 			/* Prefer the first local CPU port found */

> imho there is no way to ensure both ways backwards compatible..

if you say so... can't argue with that

> in the moment you add port5 it will be the default cpu-port which is
> what we try to "fix" here. either driver should select the better one
> (drivercode not backported if no real fix) or it needs a setting in
> dts (which is not read in older driver/core).

If changes to driver code to resolve a device tree compatibility issue
are not treated as stable-worthy bug fixes, then the implication is that
the whole thing with device tree as stable ABI is just a moronic and
pointless effort.

Have you ever tried fixing some kind of issue similar to this and gotten
adverse feedback?

There are 2 ways of addressing the compatibility issue for stable
kernels. Either port 6 always gets preferred over port 5, or the patches
which make port 5 work as a CPU port are resubmitted as stable material.
I was hoping you'd be able to bring an argument why preferring port 6
would unconditionally be a better choice than working with the first
port that's given: $insert_reason_here.

Aw: Re: Choose a default DSA CPU port

[Frank Wunderlich]

Hi

thanks vladimir for the Patch, seems to work so far...
system now says dsa-ports are routed over eth0 and ethtool stats say it too.

wonder why i get only 620Mbit on wan-port like over eth1.

root@bpi-r2:~# ip a s wan
5: wan@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
    link/ether 08:22:33:44:55:77 brd ff:ff:ff:ff:ff:ff
    inet 192.168.0.11/24 scope global wan
       valid_lft forever preferred_lft forever
    inet6 fe80::a22:33ff:fe44:5577/64 scope link
       valid_lft forever preferred_lft forever
root@bpi-r2:~# dmesg | grep eth0
[    3.309829] mtk_soc_eth 1b100000.ethernet eth0: mediatek frame engine at 0xe09e0000, irq 213
[    4.883250] mtk_soc_eth 1b100000.ethernet eth0: entered promiscuous mode
[  394.459951] mtk_soc_eth 1b100000.ethernet eth0: configuring for fixed/trgmii link mode
[  394.468881] mtk_soc_eth 1b100000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
[  394.477858] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
root@bpi-r2:~# ethtool -S eth0 | grep bytes
     tx_bytes: 819990532
     rx_bytes: 18465803
root@bpi-r2:~#
root@bpi-r2:~# ethtool -S eth1 | grep bytes
     tx_bytes: 0
     rx_bytes: 0
root@bpi-r2:~# iperf3 -c 192.168.0.21
Connecting to host 192.168.0.21, port 5201
[  5] local 192.168.0.11 port 60280 connected to 192.168.0.21 port 5201
[ ID] Interval           Transfer     Bitrate         Retr  Cwnd
[  5]   0.00-1.00   sec  75.3 MBytes   631 Mbits/sec    0    358 KBytes
[  5]   1.00-2.00   sec  74.3 MBytes   623 Mbits/sec    0    376 KBytes
[  5]   2.00-3.00   sec  74.6 MBytes   626 Mbits/sec    0    376 KBytes
[  5]   3.00-4.00   sec  74.1 MBytes   622 Mbits/sec    0    376 KBytes
[  5]   4.00-5.00   sec  74.6 MBytes   626 Mbits/sec    0    376 KBytes
[  5]   5.00-6.00   sec  74.5 MBytes   625 Mbits/sec    0    376 KBytes
[  5]   6.00-7.00   sec  74.4 MBytes   625 Mbits/sec    0    411 KBytes
[  5]   7.00-8.00   sec  74.1 MBytes   622 Mbits/sec    0    411 KBytes
[  5]   8.00-9.00   sec  74.6 MBytes   626 Mbits/sec    0    411 KBytes
[  5]   9.00-10.00  sec  74.0 MBytes   621 Mbits/sec    0    411 KBytes
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec   744 MBytes   624 Mbits/sec    0             sender
[  5]   0.00-10.04  sec   744 MBytes   621 Mbits/sec                  receiver

iperf Done.
root@bpi-r2:~# iperf3 -c 192.168.0.21 -R
Connecting to host 192.168.0.21, port 5201
Reverse mode, remote host 192.168.0.21 is sending
[  5] local 192.168.0.11 port 45834 connected to 192.168.0.21 port 5201
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-1.00   sec   112 MBytes   939 Mbits/sec
[  5]   1.00-2.00   sec   112 MBytes   939 Mbits/sec
[  5]   2.00-3.00   sec   105 MBytes   881 Mbits/sec
[  5]   3.00-4.00   sec   105 MBytes   883 Mbits/sec
[  5]   4.00-5.00   sec   105 MBytes   883 Mbits/sec
[  5]   5.00-6.00   sec   105 MBytes   884 Mbits/sec
[  5]   6.00-7.00   sec   105 MBytes   884 Mbits/sec
[  5]   7.00-8.00   sec   105 MBytes   883 Mbits/sec
[  5]   8.00-9.00   sec   105 MBytes   878 Mbits/sec
[  5]   9.00-10.00  sec   105 MBytes   880 Mbits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.04  sec  1.04 GBytes   893 Mbits/sec    0             sender
[  5]   0.00-10.00  sec  1.04 GBytes   893 Mbits/sec                  receiver

iperf Done.

without Arincs Patch i got 940Mbit on gmac0, so something seems to affect the gmac when port5 is enabled.

I guess here we need mtk for more information, so i extended cc based on get_maintainers-script (except generic maintainers and mainlinglists...can be added later if needed)

regards Frank

Re: Choose a default DSA CPU port

[Vladimir Oltean]

On Wed, Feb 22, 2023 at 06:17:42PM +0100, Frank Wunderlich wrote:
> without Arincs Patch i got 940Mbit on gmac0, so something seems to affect the gmac when port5 is enabled.

which patch?

Re: Choose a default DSA CPU port

[Arınç ÜNAL]

On 22.02.2023 21:06, Vladimir Oltean wrote:
> On Wed, Feb 22, 2023 at 06:17:42PM +0100, Frank Wunderlich wrote:
>> without Arincs Patch i got 940Mbit on gmac0, so something seems to affect the gmac when port5 is enabled.
> 
> which patch?

I believe Frank is referring to the patch series I submitted which adds 
port@5 to Bananapi BPI-R2. Without the patch, gmac0 is the default DSA 
master.

https://lore.kernel.org/linux-mediatek/20230210182505.24597-1-arinc.unal@arinc9.com/

Arınç

Re: Choose a default DSA CPU port

[Vladimir Oltean]

On Wed, Feb 22, 2023 at 09:08:03PM +0300, Arınç ÜNAL wrote:
> On 22.02.2023 21:06, Vladimir Oltean wrote:
> > On Wed, Feb 22, 2023 at 06:17:42PM +0100, Frank Wunderlich wrote:
> > > without Arincs Patch i got 940Mbit on gmac0, so something seems to affect the gmac when port5 is enabled.
> > 
> > which patch?
> 
> I believe Frank is referring to the patch series I submitted which adds
> port@5 to Bananapi BPI-R2. Without the patch, gmac0 is the default DSA
> master.
> 
> https://lore.kernel.org/linux-mediatek/20230210182505.24597-1-arinc.unal@arinc9.com/
> 
> Arınç

And with your patch + my patch, gmac0 is still the default DSA master,
but gmac1/port5 are also enabled. The claim is that switch port 6/gmac0
has lower bandwidth when switch port 5/gmac1 is enabled, than when it isn't?

Frank's testing is done on the MT7623 SoC (with the MT7530 switch),
an SoC which you have access to, since you've submitted those device
tree changes, correct? Do you confirm his result?

The posted ethtool stats are not sufficient to determine the cause of
the issue. It would be necessary to see all non-zero Ethernet counters
on both CPU port pairs:

ethtool -S eth0 | grep -v ': 0'
ethtool -S eth1 | grep -v ': 0'

to determine whether the cause of the performance degradation is packet
loss or just a lossless slowdown of some sorts. For example, the
degradation might be caused by the added flow control + uncalibrated
watermarks, not by the activation of the other GMAC.

Re: Choose a default DSA CPU port

[Arınç ÜNAL]

On 22.02.2023 22:34, Vladimir Oltean wrote:
> On Wed, Feb 22, 2023 at 09:08:03PM +0300, Arınç ÜNAL wrote:
>> On 22.02.2023 21:06, Vladimir Oltean wrote:
>>> On Wed, Feb 22, 2023 at 06:17:42PM +0100, Frank Wunderlich wrote:
>>>> without Arincs Patch i got 940Mbit on gmac0, so something seems to affect the gmac when port5 is enabled.
>>>
>>> which patch?
>>
>> I believe Frank is referring to the patch series I submitted which adds
>> port@5 to Bananapi BPI-R2. Without the patch, gmac0 is the default DSA
>> master.
>>
>> https://lore.kernel.org/linux-mediatek/20230210182505.24597-1-arinc.unal@arinc9.com/
>>
>> Arınç
> 
> And with your patch + my patch, gmac0 is still the default DSA master,
> but gmac1/port5 are also enabled. The claim is that switch port 6/gmac0
> has lower bandwidth when switch port 5/gmac1 is enabled, than when it isn't?
> 
> Frank's testing is done on the MT7623 SoC (with the MT7530 switch),
> an SoC which you have access to, since you've submitted those device
> tree changes, correct? Do you confirm his result?

I just sent a big patch series, doing some tests on this issue is next 
on my task list. So I can't confirm Frank's result for now.

> 
> The posted ethtool stats are not sufficient to determine the cause of
> the issue. It would be necessary to see all non-zero Ethernet counters
> on both CPU port pairs:
> 
> ethtool -S eth0 | grep -v ': 0'
> ethtool -S eth1 | grep -v ': 0'
> 
> to determine whether the cause of the performance degradation is packet
> loss or just a lossless slowdown of some sorts. For example, the
> degradation might be caused by the added flow control + uncalibrated
> watermarks, not by the activation of the other GMAC.

I'll keep this in mind thanks.

Frank, here's my task page for this issue, for your information.

https://arinc9.notion.site/MT7530-port5-performance-issue-98ac5fa19dc248e0b12fab08dcb2e387

Arınç

Aw: Re: Choose a default DSA CPU port

[Frank Wunderlich]

Hi

> Gesendet: Mittwoch, 22. Februar 2023 um 20:42 Uhr
> Von: "Arınç ÜNAL" <arinc.unal@arinc9.com>
> On 22.02.2023 22:34, Vladimir Oltean wrote:
> > The posted ethtool stats are not sufficient to determine the cause of
> > the issue. It would be necessary to see all non-zero Ethernet counters
> > on both CPU port pairs:
> > 
> > ethtool -S eth0 | grep -v ': 0'
> > ethtool -S eth1 | grep -v ': 0'
> > 
> > to determine whether the cause of the performance degradation is packet
> > loss or just a lossless slowdown of some sorts. For example, the
> > degradation might be caused by the added flow control + uncalibrated
> > watermarks, not by the activation of the other GMAC.
> 
> I'll keep this in mind thanks.
> 
> Frank, here's my task page for this issue, for your information.
> 
> https://arinc9.notion.site/MT7530-port5-performance-issue-98ac5fa19dc248e0b12fab08dcb2e387

tried after adding the fix from daniel, but same issue, here the iperf and ethtool-results (does not look wrong except tx-speed)


root@bpi-r2:~# iperf3 -c 192.168.0.21                                           
Connecting to host 192.168.0.21, port 5201                                      
[  5] local 192.168.0.11 port 36832 connected to 192.168.0.21 port 5201         
[ ID] Interval           Transfer     Bitrate         Retr  Cwnd                
[  5]   0.00-1.00   sec  75.4 MBytes   633 Mbits/sec    0    361 KBytes         
[  5]   1.00-2.00   sec  74.4 MBytes   624 Mbits/sec    0    361 KBytes         
[  5]   2.00-3.00   sec  74.1 MBytes   622 Mbits/sec    0    361 KBytes         
[  5]   3.00-4.00   sec  74.2 MBytes   622 Mbits/sec    0    361 KBytes         
[  5]   4.00-5.00   sec  74.6 MBytes   626 Mbits/sec    0    361 KBytes         
[  5]   5.00-6.00   sec  74.4 MBytes   624 Mbits/sec    0    379 KBytes         
[  5]   6.00-7.00   sec  74.1 MBytes   621 Mbits/sec    0    379 KBytes         
[  5]   7.00-8.00   sec  74.7 MBytes   627 Mbits/sec    0    379 KBytes         
[  5]   8.00-9.00   sec  74.5 MBytes   625 Mbits/sec    0    564 KBytes         
[  5]   9.00-10.00  sec  74.3 MBytes   623 Mbits/sec    0    564 KBytes         
- - - - - - - - - - - - - - - - - - - - - - - - -                               
[ ID] Interval           Transfer     Bitrate         Retr                      
[  5]   0.00-10.00  sec   745 MBytes   625 Mbits/sec    0             sender    
[  5]   0.00-10.04  sec   744 MBytes   621 Mbits/sec                  receiver  
                                                                                
iperf Done.                                                                     
root@bpi-r2:~# iperf3 -c 192.168.0.21 -R                                        
Connecting to host 192.168.0.21, port 5201                                      
Reverse mode, remote host 192.168.0.21 is sending                               
[  5] local 192.168.0.11 port 44428 connected to 192.168.0.21 port 5201         
[ ID] Interval           Transfer     Bitrate                                   
[  5]   0.00-1.00   sec   112 MBytes   939 Mbits/sec                            
[  5]   1.00-2.00   sec   112 MBytes   940 Mbits/sec                            
[  5]   2.00-3.00   sec   112 MBytes   939 Mbits/sec                            
[  5]   3.00-4.00   sec   112 MBytes   940 Mbits/sec                            
[  5]   4.00-5.00   sec   112 MBytes   940 Mbits/sec                            
[  5]   5.00-6.00   sec   112 MBytes   940 Mbits/sec                            
[  5]   6.00-7.00   sec   112 MBytes   940 Mbits/sec                            
[  5]   7.00-8.00   sec   112 MBytes   940 Mbits/sec                            
[  5]   8.00-9.00   sec   112 MBytes   940 Mbits/sec                            
[  5]   9.00-10.00  sec   112 MBytes   940 Mbits/sec                            
- - - - - - - - - - - - - - - - - - - - - - - - -                               
[ ID] Interval           Transfer     Bitrate         Retr                      
[  5]   0.00-10.04  sec  1.10 GBytes   938 Mbits/sec    0             sender    
[  5]   0.00-10.00  sec  1.09 GBytes   940 Mbits/sec                  receiver  
                                                                                
iperf Done.                                                                     
root@bpi-r2:~# ethtool -S eth0 | grep -v ': 0'                                  
NIC statistics:                                                                 
     tx_bytes: 1643364546                                                       
     tx_packets: 1121377                                                        
     rx_bytes: 1270088499                                                       
     rx_packets: 1338400                                                        
     p06_TxUnicast: 1338274                                                     
     p06_TxMulticast: 120                                                       
     p06_TxBroadcast: 6                                                         
     p06_TxPktSz65To127: 525948                                                 
     p06_TxPktSz128To255: 5                                                     
     p06_TxPktSz256To511: 16                                                    
     p06_TxPktSz512To1023: 4                                                    
     p06_Tx1024ToMax: 812427                                                    
     p06_TxBytes: 1275442099                                                    
     p06_RxFiltering: 16                                                        
     p06_RxUnicast: 1121339                                                     
     p06_RxMulticast: 37                                                        
     p06_RxBroadcast: 1                                                         
     p06_RxPktSz64: 3                                                           
     p06_RxPktSz65To127: 43757                                                  
     p06_RxPktSz128To255: 3                                                     
     p06_RxPktSz256To511: 3                                                     
     p06_RxPktSz1024ToMax: 1077611                                              
     p06_RxBytes: 1643364546                                                    
root@bpi-r2:~# ethtool -S eth1 | grep -v ': 0'                                  
NIC statistics:                                                                 
root@bpi-r2:~# 

breaking mt7531 as stated by vladimir is bad...currently afaik no other board uses second cpu-port (r2pro uses only port5, r3 uses port 5 as user-port for SFP).

if interested this is my tree:

https://github.com/frank-w/BPI-Router-Linux/tree/6.2-rc8-r2-dsa

regards Frank

Re: Choose a default DSA CPU port

[Vladimir Oltean]

On Fri, Feb 24, 2023 at 07:07:23PM +0100, Frank Wunderlich wrote:
> root@bpi-r2:~# ethtool -S eth0 | grep -v ': 0'
> NIC statistics:
>      tx_bytes: 1643364546
>      tx_packets: 1121377
>      rx_bytes: 1270088499
>      rx_packets: 1338400
>      p06_TxUnicast: 1338274
>      p06_TxMulticast: 120
>      p06_TxBroadcast: 6
>      p06_TxPktSz65To127: 525948
>      p06_TxPktSz128To255: 5
>      p06_TxPktSz256To511: 16
>      p06_TxPktSz512To1023: 4
>      p06_Tx1024ToMax: 812427
>      p06_TxBytes: 1275442099
>      p06_RxFiltering: 16
>      p06_RxUnicast: 1121339
>      p06_RxMulticast: 37
>      p06_RxBroadcast: 1
>      p06_RxPktSz64: 3
>      p06_RxPktSz65To127: 43757
>      p06_RxPktSz128To255: 3
>      p06_RxPktSz256To511: 3
>      p06_RxPktSz1024ToMax: 1077611
>      p06_RxBytes: 1643364546

Looking at the drivers, I see pause frames aren't counted in ethtool -S,
so we wouldn't know this. However the slowdown *is* lossless, so the
hypothesis is still not disproved.

Could you please test after removing the "pause" property from the
switch's port@6 device tree node?

Aw: Re: Choose a default DSA CPU port

[Frank Wunderlich]

result is same

root@bpi-r2:~# iperf3 -c 192.168.0.21                                           
Connecting to host 192.168.0.21, port 5201                                      
[  5] local 192.168.0.11 port 48882 connected to 192.168.0.21 port 5201         
[ ID] Interval           Transfer     Bitrate         Retr  Cwnd                
[  5]   0.00-1.00   sec  75.3 MBytes   632 Mbits/sec    0    396 KBytes         
[  5]   1.00-2.00   sec  74.3 MBytes   623 Mbits/sec    0    396 KBytes         
[  5]   2.00-3.00   sec  74.6 MBytes   625 Mbits/sec    0    396 KBytes         
[  5]   3.00-4.00   sec  73.9 MBytes   620 Mbits/sec    0    396 KBytes         
[  5]   4.00-5.00   sec  74.6 MBytes   626 Mbits/sec    0    396 KBytes         
[  5]   5.00-6.00   sec  74.4 MBytes   624 Mbits/sec    0    396 KBytes         
[  5]   6.00-7.00   sec  74.4 MBytes   624 Mbits/sec    0    396 KBytes         
[  5]   7.00-8.00   sec  74.4 MBytes   624 Mbits/sec    0    396 KBytes         
[  5]   8.00-9.00   sec  73.9 MBytes   620 Mbits/sec    0    396 KBytes         
[  5]   9.00-10.00  sec  74.6 MBytes   626 Mbits/sec    0    396 KBytes         
- - - - - - - - - - - - - - - - - - - - - - - - -                               
[ ID] Interval           Transfer     Bitrate         Retr                      
[  5]   0.00-10.00  sec   745 MBytes   625 Mbits/sec    0             sender    
[  5]   0.00-10.05  sec   744 MBytes   621 Mbits/sec                  receiver  
                                                                                
iperf Done.                                                                     
root@bpi-r2:~# ethtool -S eth0 | grep -v ': 0'                                  
NIC statistics:                                                                 
     tx_bytes: 819999267                                                        
     tx_packets: 538815                                                         
     rx_bytes: 18338089                                                         
     rx_packets: 261984                                                         
     p06_TxUnicast: 261974                                                      
     p06_TxMulticast: 10                                                        
     p06_TxPktSz65To127: 261983                                                 
     p06_TxPktSz256To511: 1                                                     
     p06_TxBytes: 19386025                                                      
     p06_RxFiltering: 13                                                        
     p06_RxUnicast: 538783                                                      
     p06_RxMulticast: 31                                                        
     p06_RxBroadcast: 1                                                         
     p06_RxPktSz64: 3                                                           
     p06_RxPktSz65To127: 47                                                     
     p06_RxPktSz128To255: 2                                                     
     p06_RxPktSz256To511: 1                                                     
     p06_RxPktSz512To1023: 2                                                    
     p06_RxPktSz1024ToMax: 538760                                               
     p06_RxBytes: 819999267

just to verify no pause is set (which is working without p5 enabled)

root@bpi-r2:~# ls /sys/firmware/devicetree/base/ethernet\@1b100000/mdio-bus/switch\@1f/ports/port\@6/fixed-link/
full-duplex  name  speed
root@bpi-r2:~# ls /sys/firmware/devicetree/base/ethernet\@1b100000/mac\@0/fixed-link/
full-duplex  name  speed

as speed is lower than gmac speed i guess there is no need for pause frames, but yes they can slow down... 

mhm...tried again with port5 disabled (pause enabled again) and got same result, so this seems not to be the cause as i thought (but have now other patches in like core-clock dropped and RX fix).

regards Frank


> Gesendet: Freitag, 24. Februar 2023 um 19:13 Uhr
> Von: "Vladimir Oltean" <olteanv@gmail.com>
> An: "Frank Wunderlich" <frank-w@public-files.de>
> Cc: "Arınç ÜNAL" <arinc.unal@arinc9.com>, "netdev" <netdev@vger.kernel.org>, erkin.bozoglu@xeront.com, "Andrew Lunn" <andrew@lunn.ch>, "Florian Fainelli" <f.fainelli@gmail.com>, "Felix Fietkau" <nbd@nbd.name>, "John Crispin" <john@phrozen.org>, "Mark Lee" <Mark-MC.Lee@mediatek.com>, "Lorenzo Bianconi" <lorenzo@kernel.org>, "Matthias Brugger" <matthias.bgg@gmail.com>, "Landen Chao" <Landen.Chao@mediatek.com>, "Sean Wang" <sean.wang@mediatek.com>, "DENG Qingfang" <dqfext@gmail.com>
> Betreff: Re: Choose a default DSA CPU port
>
> On Fri, Feb 24, 2023 at 07:07:23PM +0100, Frank Wunderlich wrote:
> > root@bpi-r2:~# ethtool -S eth0 | grep -v ': 0'
> > NIC statistics:
> >      tx_bytes: 1643364546
> >      tx_packets: 1121377
> >      rx_bytes: 1270088499
> >      rx_packets: 1338400
> >      p06_TxUnicast: 1338274
> >      p06_TxMulticast: 120
> >      p06_TxBroadcast: 6
> >      p06_TxPktSz65To127: 525948
> >      p06_TxPktSz128To255: 5
> >      p06_TxPktSz256To511: 16
> >      p06_TxPktSz512To1023: 4
> >      p06_Tx1024ToMax: 812427
> >      p06_TxBytes: 1275442099
> >      p06_RxFiltering: 16
> >      p06_RxUnicast: 1121339
> >      p06_RxMulticast: 37
> >      p06_RxBroadcast: 1
> >      p06_RxPktSz64: 3
> >      p06_RxPktSz65To127: 43757
> >      p06_RxPktSz128To255: 3
> >      p06_RxPktSz256To511: 3
> >      p06_RxPktSz1024ToMax: 1077611
> >      p06_RxBytes: 1643364546
> 
> Looking at the drivers, I see pause frames aren't counted in ethtool -S,
> so we wouldn't know this. However the slowdown *is* lossless, so the
> hypothesis is still not disproved.
> 
> Could you please test after removing the "pause" property from the
> switch's port@6 device tree node?
>