* Aw: Re: Choose a default DSA CPU port
@ 2023-02-24 20:44 Frank Wunderlich
2023-02-24 21:08 ` Vladimir Oltean
0 siblings, 1 reply; 25+ messages in thread
From: Frank Wunderlich @ 2023-02-24 20:44 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Arınç ÜNAL, netdev, erkin.bozoglu, Andrew Lunn,
Florian Fainelli, Felix Fietkau, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang
> 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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Choose a default DSA CPU port
2023-02-24 20:44 Aw: Re: Choose a default DSA CPU port Frank Wunderlich
@ 2023-02-24 21:08 ` Vladimir Oltean
2023-02-25 11:14 ` Aw: " Frank Wunderlich
2023-02-25 13:50 ` Frank Wunderlich
0 siblings, 2 replies; 25+ messages in thread
From: Vladimir Oltean @ 2023-02-24 21:08 UTC (permalink / raw)
To: Frank Wunderlich
Cc: Arınç ÜNAL, netdev, erkin.bozoglu, Andrew Lunn,
Florian Fainelli, Felix Fietkau, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang
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?
^ permalink raw reply [flat|nested] 25+ messages in thread
* Aw: Re: Choose a default DSA CPU port
2023-02-24 21:08 ` Vladimir Oltean
@ 2023-02-25 11:14 ` Frank Wunderlich
2023-02-25 13:50 ` Frank Wunderlich
1 sibling, 0 replies; 25+ messages in thread
From: Frank Wunderlich @ 2023-02-25 11:14 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Arınç ÜNAL, netdev, erkin.bozoglu, Andrew Lunn,
Florian Fainelli, Felix Fietkau, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang
> 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()
^ permalink raw reply [flat|nested] 25+ messages in thread
* Aw: Re: Choose a default DSA CPU port
2023-02-24 21:08 ` Vladimir Oltean
2023-02-25 11:14 ` Aw: " Frank Wunderlich
@ 2023-02-25 13:50 ` Frank Wunderlich
2023-02-25 16:11 ` Arınç ÜNAL
1 sibling, 1 reply; 25+ messages in thread
From: Frank Wunderlich @ 2023-02-25 13:50 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Arınç ÜNAL, netdev, erkin.bozoglu, Andrew Lunn,
Florian Fainelli, Felix Fietkau, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Aw: Re: Choose a default DSA CPU port
2023-02-25 13:50 ` Frank Wunderlich
@ 2023-02-25 16:11 ` Arınç ÜNAL
2023-02-25 19:56 ` Arınç ÜNAL
0 siblings, 1 reply; 25+ messages in thread
From: Arınç ÜNAL @ 2023-02-25 16:11 UTC (permalink / raw)
To: Frank Wunderlich, Vladimir Oltean
Cc: netdev, erkin.bozoglu, Andrew Lunn, Florian Fainelli,
Felix Fietkau, John Crispin, Mark Lee, Lorenzo Bianconi,
Matthias Brugger, Landen Chao, Sean Wang, DENG Qingfang
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ç
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Aw: Re: Choose a default DSA CPU port
2023-02-25 16:11 ` Arınç ÜNAL
@ 2023-02-25 19:56 ` Arınç ÜNAL
2023-02-26 12:12 ` Aw: " Frank Wunderlich
0 siblings, 1 reply; 25+ messages in thread
From: Arınç ÜNAL @ 2023-02-25 19:56 UTC (permalink / raw)
To: Frank Wunderlich, Vladimir Oltean
Cc: netdev, erkin.bozoglu, Andrew Lunn, Florian Fainelli,
Felix Fietkau, John Crispin, Mark Lee, Lorenzo Bianconi,
Matthias Brugger, Landen Chao, Sean Wang, DENG Qingfang
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ç
^ permalink raw reply [flat|nested] 25+ messages in thread
* Aw: Re: Re: Choose a default DSA CPU port
2023-02-25 19:56 ` Arınç ÜNAL
@ 2023-02-26 12:12 ` Frank Wunderlich
2023-02-28 9:54 ` Arınç ÜNAL
2023-02-28 11:58 ` Vladimir Oltean
0 siblings, 2 replies; 25+ messages in thread
From: Frank Wunderlich @ 2023-02-26 12:12 UTC (permalink / raw)
To: Arınç ÜNAL, Felix Fietkau
Cc: Vladimir Oltean, netdev, erkin.bozoglu, Andrew Lunn,
Florian Fainelli, John Crispin, Mark Lee, Lorenzo Bianconi,
Matthias Brugger, Landen Chao, Sean Wang, DENG Qingfang
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Aw: Re: Re: Choose a default DSA CPU port
2023-02-26 12:12 ` Aw: " Frank Wunderlich
@ 2023-02-28 9:54 ` Arınç ÜNAL
2023-02-28 11:58 ` Vladimir Oltean
1 sibling, 0 replies; 25+ messages in thread
From: Arınç ÜNAL @ 2023-02-28 9:54 UTC (permalink / raw)
To: Frank Wunderlich, Felix Fietkau
Cc: Vladimir Oltean, netdev, erkin.bozoglu, Andrew Lunn,
Florian Fainelli, John Crispin, Mark Lee, Lorenzo Bianconi,
Matthias Brugger, Landen Chao, Sean Wang, DENG Qingfang
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ç
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Choose a default DSA CPU port
2023-02-26 12:12 ` Aw: " Frank Wunderlich
2023-02-28 9:54 ` Arınç ÜNAL
@ 2023-02-28 11:58 ` Vladimir Oltean
2023-02-28 13:48 ` Frank Wunderlich
2023-05-16 19:29 ` Arınç ÜNAL
1 sibling, 2 replies; 25+ messages in thread
From: Vladimir Oltean @ 2023-02-28 11:58 UTC (permalink / raw)
To: Frank Wunderlich
Cc: Arınç ÜNAL, Felix Fietkau, netdev, erkin.bozoglu,
Andrew Lunn, Florian Fainelli, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Choose a default DSA CPU port
2023-02-28 11:58 ` Vladimir Oltean
@ 2023-02-28 13:48 ` Frank Wunderlich
2023-02-28 22:56 ` Vladimir Oltean
2023-05-16 19:29 ` Arınç ÜNAL
1 sibling, 1 reply; 25+ messages in thread
From: Frank Wunderlich @ 2023-02-28 13:48 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Arınç ÜNAL, Felix Fietkau, netdev, erkin.bozoglu,
Andrew Lunn, Florian Fainelli, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Choose a default DSA CPU port
2023-02-28 13:48 ` Frank Wunderlich
@ 2023-02-28 22:56 ` Vladimir Oltean
2023-03-01 6:38 ` Frank Wunderlich
0 siblings, 1 reply; 25+ messages in thread
From: Vladimir Oltean @ 2023-02-28 22:56 UTC (permalink / raw)
To: Frank Wunderlich
Cc: Arınç ÜNAL, Felix Fietkau, netdev, erkin.bozoglu,
Andrew Lunn, Florian Fainelli, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang
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?
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Choose a default DSA CPU port
2023-02-28 22:56 ` Vladimir Oltean
@ 2023-03-01 6:38 ` Frank Wunderlich
2023-03-01 12:37 ` Vladimir Oltean
0 siblings, 1 reply; 25+ messages in thread
From: Frank Wunderlich @ 2023-03-01 6:38 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Arınç ÜNAL, Felix Fietkau, netdev, erkin.bozoglu,
Andrew Lunn, Florian Fainelli, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Choose a default DSA CPU port
2023-03-01 6:38 ` Frank Wunderlich
@ 2023-03-01 12:37 ` Vladimir Oltean
2023-03-06 18:20 ` Aw: " Frank Wunderlich
0 siblings, 1 reply; 25+ messages in thread
From: Vladimir Oltean @ 2023-03-01 12:37 UTC (permalink / raw)
To: Frank Wunderlich
Cc: Arınç ÜNAL, Felix Fietkau, netdev, erkin.bozoglu,
Andrew Lunn, Florian Fainelli, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Aw: Re: Choose a default DSA CPU port
2023-03-01 12:37 ` Vladimir Oltean
@ 2023-03-06 18:20 ` Frank Wunderlich
2023-03-07 17:43 ` Vladimir Oltean
0 siblings, 1 reply; 25+ messages in thread
From: Frank Wunderlich @ 2023-03-06 18:20 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Arınç ÜNAL, Felix Fietkau, netdev, erkin.bozoglu,
Andrew Lunn, Florian Fainelli, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Choose a default DSA CPU port
2023-03-06 18:20 ` Aw: " Frank Wunderlich
@ 2023-03-07 17:43 ` Vladimir Oltean
2023-04-13 18:09 ` Aw: " Frank Wunderlich
0 siblings, 1 reply; 25+ messages in thread
From: Vladimir Oltean @ 2023-03-07 17:43 UTC (permalink / raw)
To: Frank Wunderlich
Cc: Arınç ÜNAL, Felix Fietkau, netdev, erkin.bozoglu,
Andrew Lunn, Florian Fainelli, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Aw: Re: Choose a default DSA CPU port
2023-03-07 17:43 ` Vladimir Oltean
@ 2023-04-13 18:09 ` Frank Wunderlich
2023-04-13 21:30 ` Frank Wunderlich
0 siblings, 1 reply; 25+ messages in thread
From: Frank Wunderlich @ 2023-04-13 18:09 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Arınç ÜNAL, Felix Fietkau, netdev, erkin.bozoglu,
Andrew Lunn, Florian Fainelli, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Choose a default DSA CPU port
2023-04-13 18:09 ` Aw: " Frank Wunderlich
@ 2023-04-13 21:30 ` Frank Wunderlich
0 siblings, 0 replies; 25+ messages in thread
From: Frank Wunderlich @ 2023-04-13 21:30 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Arınç ÜNAL, Felix Fietkau, netdev, erkin.bozoglu,
Andrew Lunn, Florian Fainelli, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang, 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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Choose a default DSA CPU port
2023-02-28 11:58 ` Vladimir Oltean
2023-02-28 13:48 ` Frank Wunderlich
@ 2023-05-16 19:29 ` Arınç ÜNAL
2023-05-17 16:10 ` Vladimir Oltean
1 sibling, 1 reply; 25+ messages in thread
From: Arınç ÜNAL @ 2023-05-16 19:29 UTC (permalink / raw)
To: Vladimir Oltean, Frank Wunderlich
Cc: Felix Fietkau, netdev, erkin.bozoglu, Andrew Lunn,
Florian Fainelli, John Crispin, Mark Lee, Lorenzo Bianconi,
Matthias Brugger, Landen Chao, Sean Wang, DENG Qingfang
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ç
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Choose a default DSA CPU port
2023-05-16 19:29 ` Arınç ÜNAL
@ 2023-05-17 16:10 ` Vladimir Oltean
2023-05-17 16:14 ` Arınç ÜNAL
0 siblings, 1 reply; 25+ messages in thread
From: Vladimir Oltean @ 2023-05-17 16:10 UTC (permalink / raw)
To: Arınç ÜNAL
Cc: Frank Wunderlich, Felix Fietkau, netdev, erkin.bozoglu,
Andrew Lunn, Florian Fainelli, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Choose a default DSA CPU port
2023-05-17 16:10 ` Vladimir Oltean
@ 2023-05-17 16:14 ` Arınç ÜNAL
2023-05-17 16:16 ` Vladimir Oltean
0 siblings, 1 reply; 25+ messages in thread
From: Arınç ÜNAL @ 2023-05-17 16:14 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Frank Wunderlich, Felix Fietkau, netdev, erkin.bozoglu,
Andrew Lunn, Florian Fainelli, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang
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ç
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Choose a default DSA CPU port
2023-05-17 16:14 ` Arınç ÜNAL
@ 2023-05-17 16:16 ` Vladimir Oltean
2023-05-18 10:36 ` Arınç ÜNAL
0 siblings, 1 reply; 25+ messages in thread
From: Vladimir Oltean @ 2023-05-17 16:16 UTC (permalink / raw)
To: Arınç ÜNAL
Cc: Frank Wunderlich, Felix Fietkau, netdev, erkin.bozoglu,
Andrew Lunn, Florian Fainelli, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Choose a default DSA CPU port
2023-05-17 16:16 ` Vladimir Oltean
@ 2023-05-18 10:36 ` Arınç ÜNAL
2023-05-18 14:24 ` Vladimir Oltean
0 siblings, 1 reply; 25+ messages in thread
From: Arınç ÜNAL @ 2023-05-18 10:36 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Frank Wunderlich, Felix Fietkau, netdev, erkin.bozoglu,
Andrew Lunn, Florian Fainelli, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang, mithat.guner
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ç
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: Choose a default DSA CPU port
2023-05-18 10:36 ` Arınç ÜNAL
@ 2023-05-18 14:24 ` Vladimir Oltean
2023-05-19 9:00 ` Arınç ÜNAL
0 siblings, 1 reply; 25+ messages in thread
From: Vladimir Oltean @ 2023-05-18 14:24 UTC (permalink / raw)
To: Arınç ÜNAL
Cc: Frank Wunderlich, Felix Fietkau, netdev, erkin.bozoglu,
Andrew Lunn, Florian Fainelli, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang, mithat.guner
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ç
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Choose a default DSA CPU port
2023-05-18 14:24 ` Vladimir Oltean
@ 2023-05-19 9:00 ` Arınç ÜNAL
2023-05-19 23:54 ` Vladimir Oltean
0 siblings, 1 reply; 25+ messages in thread
From: Arınç ÜNAL @ 2023-05-19 9:00 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Frank Wunderlich, Felix Fietkau, netdev, erkin.bozoglu,
Andrew Lunn, Florian Fainelli, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang, mithat.guner
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ç
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Choose a default DSA CPU port
2023-05-19 9:00 ` Arınç ÜNAL
@ 2023-05-19 23:54 ` Vladimir Oltean
0 siblings, 0 replies; 25+ messages in thread
From: Vladimir Oltean @ 2023-05-19 23:54 UTC (permalink / raw)
To: Arınç ÜNAL
Cc: Frank Wunderlich, Felix Fietkau, netdev, erkin.bozoglu,
Andrew Lunn, Florian Fainelli, John Crispin, Mark Lee,
Lorenzo Bianconi, Matthias Brugger, Landen Chao, Sean Wang,
DENG Qingfang, mithat.guner
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2023-05-19 23:54 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-24 20:44 Aw: Re: Choose a default DSA CPU port Frank Wunderlich
2023-02-24 21:08 ` Vladimir Oltean
2023-02-25 11:14 ` Aw: " Frank Wunderlich
2023-02-25 13:50 ` Frank Wunderlich
2023-02-25 16:11 ` Arınç ÜNAL
2023-02-25 19:56 ` Arınç ÜNAL
2023-02-26 12:12 ` Aw: " Frank Wunderlich
2023-02-28 9:54 ` Arınç ÜNAL
2023-02-28 11:58 ` Vladimir Oltean
2023-02-28 13:48 ` Frank Wunderlich
2023-02-28 22:56 ` Vladimir Oltean
2023-03-01 6:38 ` Frank Wunderlich
2023-03-01 12:37 ` Vladimir Oltean
2023-03-06 18:20 ` Aw: " Frank Wunderlich
2023-03-07 17:43 ` Vladimir Oltean
2023-04-13 18:09 ` Aw: " Frank Wunderlich
2023-04-13 21:30 ` Frank Wunderlich
2023-05-16 19:29 ` Arınç ÜNAL
2023-05-17 16:10 ` Vladimir Oltean
2023-05-17 16:14 ` Arınç ÜNAL
2023-05-17 16:16 ` Vladimir Oltean
2023-05-18 10:36 ` Arınç ÜNAL
2023-05-18 14:24 ` Vladimir Oltean
2023-05-19 9:00 ` Arınç ÜNAL
2023-05-19 23:54 ` Vladimir Oltean
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).