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 21:02, Tobias Waldekranz <tobias@waldekranz.com> wrote:
> 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?

If I understand Florian's original message (061f6a505ac3) correctly,
this was originally done to support HW that cannot control VLAN
filtering per port. I.e to support this setup:

  vlan1
    |
   br0     vlan2
  /   \      |
swp0  swp1  swp2

Where swp2 cannot be configured to ignore 1Q tags at the same time as
VLAN filtering is enabled on swp0 and swp1.

Florian, do I have that right?

If so, I think we can safely just `return 0` on these in mv88e6xxx (and
any other drivers where the HW can control this per port).

Adding a guard against configuring VLANs on unbridged user ports in
mv88e6xxx_port_vlan_add does seem to do the trick.