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.