| Message ID | E1ij6pq-00084C-47@rmk-PC.armlinux.org.uk |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | VLANs, DSA switches and multiple bridges | expand |
+Ido, On 12/22/2019 11:24 AM, Russell King wrote: > DSA assumes that a bridge which has vlan filtering disabled is not > vlan aware, and ignores all vlan configuration. However, the kernel > software bridge code allows configuration in this state. > > This causes the kernel's idea of the bridge vlan state and the > hardware state to disagree, so "bridge vlan show" indicates a correct > configuration but the hardware lacks all configuration. Even worse, > enabling vlan filtering on a DSA bridge immediately blocks all traffic > which, given the output of "bridge vlan show", is very confusing. > > Provide an option that drivers can set to indicate they want to receive > vlan configuration even when vlan filtering is disabled. This is safe > for Marvell DSA bridges, which do not look up ingress traffic in the > VTU if the port is in 8021Q disabled state. Whether this change is > suitable for all DSA bridges is not known. s/DSA bridges/DSA switches/ ? this is also safe to do with b53 switches in fact this is even desirable because VLAN filtering is a global attribute so if you have at least one bridge that spans one of your switch ports and that bridge requests vlan_filtering=1, you *must* have a valid VID entry for the non-bridged ports, or the bridged ports with vlan_filtering=0 otherwise there is no default VID entry programmed to ingress the frame. Today this is achieved by making sure that the default untagged VID 0 for non bridged ports is always programmed at start-up and the switch is always configured with VLAN awareness. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> This ties in with this commit: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2ea7a679ca2abd251c1ec03f20508619707e1749 which I believe is still correct in the sense that with Linux a bridge with vlan_filtering=0 also means that the bridge is not VLAN aware. Ido, Jiri, do you disagree? This seems to be coming every now and then, so maybe it is time to revisit this documentation patch: https://github.com/ffainelli/linux/commit/3fe61b1722a3b79d2e317a812c54f3afc902e5b0
On Mon, Dec 23, 2019 at 10:02:12AM -0800, Florian Fainelli wrote: > +Ido, > > On 12/22/2019 11:24 AM, Russell King wrote: > > DSA assumes that a bridge which has vlan filtering disabled is not > > vlan aware, and ignores all vlan configuration. However, the kernel > > software bridge code allows configuration in this state. > > > > This causes the kernel's idea of the bridge vlan state and the > > hardware state to disagree, so "bridge vlan show" indicates a correct > > configuration but the hardware lacks all configuration. Even worse, > > enabling vlan filtering on a DSA bridge immediately blocks all traffic > > which, given the output of "bridge vlan show", is very confusing. > > > > Provide an option that drivers can set to indicate they want to receive > > vlan configuration even when vlan filtering is disabled. This is safe > > for Marvell DSA bridges, which do not look up ingress traffic in the > > VTU if the port is in 8021Q disabled state. Whether this change is > > suitable for all DSA bridges is not known. > > s/DSA bridges/DSA switches/ ? > > this is also safe to do with b53 switches in fact this is even desirable > because VLAN filtering is a global attribute so if you have at least one > bridge that spans one of your switch ports and that bridge requests > vlan_filtering=1, you *must* have a valid VID entry for the non-bridged > ports, or the bridged ports with vlan_filtering=0 otherwise there is no > default VID entry programmed to ingress the frame. Today this is > achieved by making sure that the default untagged VID 0 for non bridged > ports is always programmed at start-up and the switch is always > configured with VLAN awareness. > > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > This ties in with this commit: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2ea7a679ca2abd251c1ec03f20508619707e1749 > > which I believe is still correct in the sense that with Linux a bridge > with vlan_filtering=0 also means that the bridge is not VLAN aware. Ido, > Jiri, do you disagree? Hi Florian, Yes, vlan_filtering=0 means that the bridge is not VLAN aware. It's not only about filtering at ingress / egress. The man page says "When disabled, the bridge will not consider the VLAN tag when handling packets." It also affects the VLAN with which FDB entries are learned and the VLAN used for FDB lookup. It is really problematic for switch drivers to properly support the dynamic toggling of this option. This is why mlxsw forbids this toggling. Either you create the bridge with VLAN filtering disabled or enabled. Assuming I'm reading Cumulus documentation correctly, it seems they enforce the same behavior: https://docs.cumulusnetworks.com/cumulus-linux/Layer-2/Ethernet-Bridging-VLANs/VLAN-aware-Bridge-Mode/#convert-bridges-between-supported-modes > This seems to be coming every now and then, so maybe it is time to > revisit this documentation patch: > > https://github.com/ffainelli/linux/commit/3fe61b1722a3b79d2e317a812c54f3afc902e5b0 Yes, I remember reviewing it. Not sure why you didn't send another version :)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index cd79ee14ea5f..650b101caad7 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -2680,6 +2680,7 @@ static int mv88e6xxx_setup(struct dsa_switch *ds) chip->ds = ds; ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip); + ds->vlan_bridge_vtu = true; mv88e6xxx_reg_lock(chip); diff --git a/include/net/dsa.h b/include/net/dsa.h index 99f71be8cfdb..5e5c1a95af1a 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -269,6 +269,7 @@ struct dsa_switch { * settings on ports if not hardware-supported */ bool vlan_filtering_is_global; + bool vlan_bridge_vtu; /* In case vlan_filtering_is_global is set, the VLAN awareness state * should be retrieved from here and not from the per-port settings. diff --git a/net/dsa/slave.c b/net/dsa/slave.c index a7c1e7ded8ca..6d167713da83 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -323,7 +323,8 @@ static int dsa_slave_vlan_add(struct net_device *dev, if (obj->orig_dev != dev) return -EOPNOTSUPP; - if (dp->bridge_dev && !br_vlan_enabled(dp->bridge_dev)) + if (dp->bridge_dev && !dp->ds->vlan_bridge_vtu && + !br_vlan_enabled(dp->bridge_dev)) return 0; vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj); @@ -390,7 +391,8 @@ static int dsa_slave_vlan_del(struct net_device *dev, if (obj->orig_dev != dev) return -EOPNOTSUPP; - if (dp->bridge_dev && !br_vlan_enabled(dp->bridge_dev)) + if (dp->bridge_dev && !dp->ds->vlan_bridge_vtu && + !br_vlan_enabled(dp->bridge_dev)) return 0; /* Do not deprogram the CPU port as it may be shared with other user @@ -1122,7 +1124,8 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto, * need to emulate the switchdev prepare + commit phase. */ if (dp->bridge_dev) { - if (!br_vlan_enabled(dp->bridge_dev)) + if (!dp->ds->vlan_bridge_vtu && + !br_vlan_enabled(dp->bridge_dev)) return 0; /* br_vlan_get_info() returns -EINVAL or -ENOENT if the @@ -1156,7 +1159,8 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, * need to emulate the switchdev prepare + commit phase. */ if (dp->bridge_dev) { - if (!br_vlan_enabled(dp->bridge_dev)) + if (!dp->ds->vlan_bridge_vtu && + !br_vlan_enabled(dp->bridge_dev)) return 0; /* br_vlan_get_info() returns -EINVAL or -ENOENT if the
DSA assumes that a bridge which has vlan filtering disabled is not vlan aware, and ignores all vlan configuration. However, the kernel software bridge code allows configuration in this state. This causes the kernel's idea of the bridge vlan state and the hardware state to disagree, so "bridge vlan show" indicates a correct configuration but the hardware lacks all configuration. Even worse, enabling vlan filtering on a DSA bridge immediately blocks all traffic which, given the output of "bridge vlan show", is very confusing. Provide an option that drivers can set to indicate they want to receive vlan configuration even when vlan filtering is disabled. This is safe for Marvell DSA bridges, which do not look up ingress traffic in the VTU if the port is in 8021Q disabled state. Whether this change is suitable for all DSA bridges is not known. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- drivers/net/dsa/mv88e6xxx/chip.c | 1 + include/net/dsa.h | 1 + net/dsa/slave.c | 12 ++++++++---- 3 files changed, 10 insertions(+), 4 deletions(-)