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

[Florian Fainelli <f.fainelli@gmail.com>]

On 3/7/2021 2:49 PM, Tobias Waldekranz wrote:
> 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?

Yes, this change was intended to support switches that have global VLAN
filtering attributes (like b53, bcm_sf2) and where standalone ports that
get a VLAN upper require us to program an appropriate VLAN table entry
for these uppers to keep working.

> 
> 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.
> 

OK.
-- 
Florian