Re: [PATCH v2 net] net: dsa: fix switchdev objects on bridge master mistakenly being applied on ports

[Tobias Waldekranz <tobias@waldekranz.com>]

On Sun, Mar 07, 2021 at 17:48, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Sun, Mar 07, 2021 at 04:17:14PM +0100, Tobias Waldekranz wrote:
>> Please wait before applying.
>> 
>> I need to do some more testing later (possibly tomorrow). But I am
>> pretty sure that this patch does not work with the (admittedly somewhat
>> exotic) combination of:
>> 
>> - Non-offloaded LAG
>> - Bridge with VLAN filtering enabled.
>> 
>> When adding the LAG to the bridge, I get an error because mv88e6xxx
>> tries to add VLAN 1 to the ports (which it should not do as the LAG is
>> not offloaded).
>
> Weird, how are you testing, and why does it attempt to add VLAN 1? Is it
> the mv88e6xxx driver itself that does this? Where from?
>
> The following is my test procedure:
>
> cat ./test_bond_no_offload.sh
> #!/bin/bash
>
> ip link del bond0
> for eth in swp0 swp1 swp2; do ip link set $eth down; done
> ip link add bond0 type bond mode broadcast
> ip link add br0 type bridge vlan_filtering 1
> ip link set swp0 master bond0
> ip link set swp1 master bond0
> ip link set swp2 master br0
> ip link set bond0 master br0
> for eth in swp0 swp1 swp2 bond0 br0; do ip link set $eth up; done
>
> ./test_bond_no_offload.sh
> [   27.004206] bond0 (unregistering): Released all slaves
> [   27.068440] mscc_felix 0000:00:00.5 swp0: configuring for inband/qsgmii link mode
> [   27.077811] 8021q: adding VLAN 0 to HW filter on device swp0
> [   27.083728] bond0: (slave swp0): Enslaving as an active interface with an up link
> Warning: dsa_core: Offloading not supported.
> [   27.095035] mscc_felix 0000:00:00.5 swp1: configuring for inband/qsgmii link mode
> [   27.104073] 8021q: adding VLAN 0 to HW filter on device swp1
> [   27.109948] bond0: (slave swp1): Enslaving as an active interface with an up link
> Warning: dsa_core: Offloading not supported.
> [   27.120214] br0: port 1(swp2) entered blocking state
> [   27.125407] br0: port 1(swp2) entered disabled state
> [   27.131738] mscc_felix 0000:00:00.5: dsa_port_vlan_filtering: port 2 vlan_filtering 1
> [   27.139625] mscc_felix 0000:00:00.5 swp2: dsa_slave_vlan_add: vid 1
> [   27.149223] br0: port 2(bond0) entered blocking state
> [   27.154341] br0: port 2(bond0) entered disabled state
> [   27.159600] device bond0 entered promiscuous mode
> [   27.164340] device swp0 entered promiscuous mode
> [   27.169028] device swp1 entered promiscuous mode
> [   27.173718] device swp2 entered promiscuous mode
> [   27.187698] mscc_felix 0000:00:00.5 swp2: configuring for inband/qsgmii link mode
> [   27.196312] 8021q: adding VLAN 0 to HW filter on device swp2
> [   27.207605] 8021q: adding VLAN 0 to HW filter on device bond0
> [   28.060872] IPv6: ADDRCONF(NETDEV_CHANGE): bond0: link becomes ready
> [   28.067323] br0: port 2(bond0) entered blocking state
> [   28.072406] br0: port 2(bond0) entered forwarding state
> [   28.077751] IPv6: ADDRCONF(NETDEV_CHANGE): br0: link becomes ready
> # bridge link
> 8: swp2@eth1: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 master br0 state disabled priority 32 cost 100
> 10: bond0: <BROADCAST,MULTICAST,MASTER,UP,LOWER_UP> mtu 1500 master br0 state forwarding priority 32 cost 100
> # bridge vlan add dev bond0 vid 100
> # bridge vlan add dev swp2 vid 100
> [   48.669422] mscc_felix 0000:00:00.5 swp2: dsa_slave_vlan_add: vid 100
> # bridge vlan add dev br0 vid 100 self

I ran the same test on my box (s/swp/eth/g just because that is what the
ports are called on my board):

root@envoy:~# dmesg -c
root@envoy:~# ./test_bond_no_offload.sh
Warning: dsa_core: Offloading not supported.
Warning: dsa_core: Offloading not supported.
RTNETLINK answers: Operation not supported
root@envoy:~# dmesg -c
[   40.392113] device eth3 left promiscuous mode
[   40.392233] br0: port 1(eth3) entered disabled state
[   40.468035] bond0 (unregistering): (slave eth1): Releasing backup interface
[   40.480821] device eth1 left promiscuous mode
[   40.487626] bond0 (unregistering): (slave eth2): Releasing backup interface
[   40.508856] device eth2 left promiscuous mode
[   40.508870] device chan0 left promiscuous mode
[   40.515602] bond0 (unregistering): Released all slaves
[   40.571520] mv88e6085 30be0000.ethernet-1:04 eth1: configuring for inband/2500base-x link mode
[   40.574803] 8021q: adding VLAN 0 to HW filter on device eth1       
[   40.576595] bond0: (slave eth1): Enslaving as an active interface with an up link    
[   40.583908] mv88e6085 30be0000.ethernet-1:04 eth2: configuring for inband/sgmii link mode
[   40.587225] 8021q: adding VLAN 0 to HW filter on device eth2 
[   40.589014] bond0: (slave eth2): Enslaving as an active interface with an up link
[   40.591622] br0: port 1(eth3) entered blocking state
[   40.591642] br0: port 1(eth3) entered disabled state
[   40.602894] br0: port 2(bond0) entered blocking state
[   40.602931] br0: port 2(bond0) entered disabled state
[   40.603172] device bond0 entered promiscuous mode
[   40.603179] device eth1 entered promiscuous mode
[   40.603183] device chan0 entered promiscuous mode
[   40.603229] device eth2 entered promiscuous mode
[   40.603284] device eth3 entered promiscuous mode
[   40.605250] mv88e6085 30be0000.ethernet-1:04: p10: hw VLAN 1 already used by port 8 in br0
[   40.605268] CPU: 0 PID: 1734 Comm: ip Not tainted 5.11.0 #197
[   40.605276] Hardware name: lynx-2510 (DT)
[   40.605281] Call trace: 
[   40.605284]  dump_backtrace+0x0/0x1b0
[   40.605301]  show_stack+0x20/0x70
[   40.605310]  dump_stack+0xd0/0x12c
[   40.605320]  mv88e6xxx_port_vlan_add+0x79c/0x810
[   40.605333]  dsa_switch_event+0x600/0xc70
[   40.605343]  raw_notifier_call_chain+0x5c/0x80
[   40.605351]  dsa_tree_notify+0x1c/0x40
[   40.605358]  dsa_port_vlan_add+0x58/0x80
[   40.605365]  dsa_slave_vlan_rx_add_vid+0x80/0x130
[   40.605372]  vlan_add_rx_filter_info+0x60/0x90
[   40.605380]  vlan_vid_add+0xf4/0x1b0
[   40.605386]  bond_vlan_rx_add_vid+0x78/0x110
[   40.605394]  vlan_add_rx_filter_info+0x60/0x90
[   40.605400]  vlan_vid_add+0xf4/0x1b0
[   40.605406]  __vlan_add+0x6c8/0x840
[   40.605415]  nbp_vlan_add+0xfc/0x180
[   40.605423]  nbp_vlan_init+0x140/0x190
[   40.605433]  br_add_if+0x558/0x740
[   40.605440]  br_add_slave+0x1c/0x30

(I added the dump_stack() just for demonstration purposes)

So we are coming in from everyones favorite ndo: ndo_vlan_add_rx_vid!

mv88e6xxx complains (rightly IMHO) that the hardware cannot offload VLAN
1 to two different bridges. It sees that eth3 is connected to br0, and
the current port is trying to add the same VID to a different
bridge. The second bridge in this case is in fact NULL.

One could argue that mv88e6xxx could just skip config if dp->bridge_dev
is not set. OTOH, the DSA layer manages all the intricacies of that in
all other scenarios.

Should we return early from the ndo if dp->bridge_dev is NULL? But then
why do we implement those ndos at all?