| Message ID | 20210308135509.3040286-1-olteanv@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net] net: dsa: only unset VLAN filtering when last port leaves last VLAN-aware bridge | expand |
Hi Vladimir, On Mon Mar 08 2021, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > DSA is aware of switches with global VLAN filtering since the blamed > commit, but it makes a bad decision when multiple bridges are spanning > the same switch: > > ip link add br0 type bridge vlan_filtering 1 > ip link add br1 type bridge vlan_filtering 1 > ip link set swp2 master br0 > ip link set swp3 master br0 > ip link set swp4 master br1 > ip link set swp5 master br1 > ip link set swp5 nomaster > ip link set swp4 nomaster > [138665.939930] sja1105 spi0.1: port 3: dsa_core: VLAN filtering is a global setting > [138665.947514] DSA: failed to notify DSA_NOTIFIER_BRIDGE_LEAVE > > When all ports leave br1, DSA blindly attempts to disable VLAN filtering > on the switch, ignoring the fact that br0 still exists and is VLAN-aware > too. It fails while doing that. > > This patch checks whether any port exists at all and is under a > VLAN-aware bridge. > > Fixes: d371b7c92d19 ("net: dsa: Unset vlan_filtering when ports leave the bridge") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Code looks correct. One comment below. > --- > net/dsa/switch.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/net/dsa/switch.c b/net/dsa/switch.c > index 4b5da89dc27a..56ed31b0e636 100644 > --- a/net/dsa/switch.c > +++ b/net/dsa/switch.c > @@ -107,7 +107,7 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds, > bool unset_vlan_filtering = br_vlan_enabled(info->br); > struct dsa_switch_tree *dst = ds->dst; > struct netlink_ext_ack extack = {0}; > - int err, i; > + int err, port; > > if (dst->index == info->tree_index && ds->index == info->sw_index && > ds->ops->port_bridge_join) > @@ -124,13 +124,16 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds, > * it. That is a good thing, because that lets us handle it and also > * handle the case where the switch's vlan_filtering setting is global > * (not per port). When that happens, the correct moment to trigger the > - * vlan_filtering callback is only when the last port left this bridge. > + * vlan_filtering callback is only when the last port the last Somehow "left" got missing. Shouldn't that line be: "vlan_filtering callback is only when the last port left the last" ? > + * VLAN-aware bridge. > */ Thanks, Kurt
On Tue, Mar 09, 2021 at 07:31:25AM +0100, Kurt Kanzenbach wrote: > > @@ -124,13 +124,16 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds, > > * it. That is a good thing, because that lets us handle it and also > > * handle the case where the switch's vlan_filtering setting is global > > * (not per port). When that happens, the correct moment to trigger the > > - * vlan_filtering callback is only when the last port left this bridge. > > + * vlan_filtering callback is only when the last port the last > > Somehow "left" got missing. Shouldn't that line be: > > "vlan_filtering callback is only when the last port left the last" ? > Thanks, you're right, I don't know how it happened, I'll resend.
On 3/8/21 5:55 AM, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > DSA is aware of switches with global VLAN filtering since the blamed > commit, but it makes a bad decision when multiple bridges are spanning > the same switch: > > ip link add br0 type bridge vlan_filtering 1 > ip link add br1 type bridge vlan_filtering 1 > ip link set swp2 master br0 > ip link set swp3 master br0 > ip link set swp4 master br1 > ip link set swp5 master br1 > ip link set swp5 nomaster > ip link set swp4 nomaster > [138665.939930] sja1105 spi0.1: port 3: dsa_core: VLAN filtering is a global setting > [138665.947514] DSA: failed to notify DSA_NOTIFIER_BRIDGE_LEAVE > > When all ports leave br1, DSA blindly attempts to disable VLAN filtering > on the switch, ignoring the fact that br0 still exists and is VLAN-aware > too. It fails while doing that. > > This patch checks whether any port exists at all and is under a > VLAN-aware bridge. > > Fixes: d371b7c92d19 ("net: dsa: Unset vlan_filtering when ports leave the bridge") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> This uncovered a bug in the bcm_sf2 driver which failed to set ds->vlan_filtering_is_global so with that fixed, I was able to verify that the scenario above would produce the above message and failure, and with the patch applied this works correctly as expected without disabling VLAN filtering on br0. When the last port on br0 leaves the bridge however, VLAN filtering is turned off since there is no more user requiring it. Tested-by: Florian Fainelli <f.fainelli@gmail.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Thanks! -- Florian
diff --git a/net/dsa/switch.c b/net/dsa/switch.c index 4b5da89dc27a..56ed31b0e636 100644 --- a/net/dsa/switch.c +++ b/net/dsa/switch.c @@ -107,7 +107,7 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds, bool unset_vlan_filtering = br_vlan_enabled(info->br); struct dsa_switch_tree *dst = ds->dst; struct netlink_ext_ack extack = {0}; - int err, i; + int err, port; if (dst->index == info->tree_index && ds->index == info->sw_index && ds->ops->port_bridge_join) @@ -124,13 +124,16 @@ static int dsa_switch_bridge_leave(struct dsa_switch *ds, * it. That is a good thing, because that lets us handle it and also * handle the case where the switch's vlan_filtering setting is global * (not per port). When that happens, the correct moment to trigger the - * vlan_filtering callback is only when the last port left this bridge. + * vlan_filtering callback is only when the last port the last + * VLAN-aware bridge. */ if (unset_vlan_filtering && ds->vlan_filtering_is_global) { - for (i = 0; i < ds->num_ports; i++) { - if (i == info->port) - continue; - if (dsa_to_port(ds, i)->bridge_dev == info->br) { + for (port = 0; port < ds->num_ports; port++) { + struct net_device *bridge_dev; + + bridge_dev = dsa_to_port(ds, port)->bridge_dev; + + if (bridge_dev && br_vlan_enabled(bridge_dev)) { unset_vlan_filtering = false; break; }