net: dsa: Make switches VLAN aware when enslaved into a bridge
Subsystems (4)
RFC PATCH 1/1 net: dsa: Make switches VLAN aware when enslaved into a bridge
Commit 2ea7a679ca2a ("net: dsa: Don't add vlans when vlan filtering is disabled") changed the behavior of DSA switches when the switch ports are enslaved into the bridge and only pushed the VLAN configuration down to the switch if the bridge is configured with VLAN filtering enabled.
This is unfortunately wrong, because what vlan_filtering configures is a policy on the acceptance of VLAN tagged frames with an unknown VID.
vlan_filtering=0 means a frame with a VLAN tag that is not part of the VLAN table should be allowed to ingress the switch, and vlan_fltering=1 would reject that frame.
---
Andrew,
I checked with Jiri and he confirmed that our interpretention of vlan_filtering in DSA was incorrect and that it does denote whether the switch should be doing VID ingress policy checking.
You mentioned in the commit message some problems without being too specific about them which is why I am putting the same checks in mv88e6xxx in order not to break your use cases. Let me know if you want to drop that hunk entirely.
Thanks!
drivers/net/dsa/mv88e6xxx/chip.c | 7 ++++++- net/dsa/port.c | 10 ++-------- 2 files changed, 8 insertions(+), 9 deletions(-)
drivers/net/dsa/mv88e6xxx/chip.c +6 -1
@@ -1684,13 +1684,14 @@ static int _mv88e6xxx_port_vlan_add(struct mv88e6xxx_chip *chip, int port,static void mv88e6xxx_port_vlan_add (struct dsa_switch *ds,int port,const struct switchdev_obj_port_vlan *vlan) { +struct dsa_port *dp = &ds->ports[i];struct mv88e6xxx_chip *chip = ds->priv;bool untagged = vlan->flags &BRIDGE_VLAN_INFO_UNTAGGED ;bool pvid = vlan->flags &BRIDGE_VLAN_INFO_PVID ;u8 member;u16 vid; -if (!chip->info->max_vid) +if (!chip->info->max_vid ||br_vlan_enabled (dp->bridge_dev))return ;if (dsa_is_dsa_port (ds, port) ||dsa_is_cpu_port (ds, port))
@@ -1751,12 +1752,16 @@ static int mv88e6xxx_port_vlan_del(struct dsa_switch *ds, int port, const struct switchdev_obj_port_vlan *vlan) {struct mv88e6xxx_chip *chip = ds->priv; +struct dsa_port *dp = &ds->ports[i];u16 pvid, vid;int err =0 ;if (!chip->info->max_vid)return -EOPNOTSUPP ; +if (br_vlan_enabled (dp->bridge_dev)) +return 0 ; +mutex_lock (&chip->reg_lock); err =mv88e6xxx_port_get_pvid (chip, port, &pvid);
net/dsa/port.c +2 -8
@@ -255,10 +255,7 @@ int dsa_port_vlan_add(struct dsa_port *dp,if (netif_is_bridge_master (vlan->obj.orig_dev))return -EOPNOTSUPP ; -if (br_vlan_enabled (dp->bridge_dev)) -return dsa_port_notify (dp,DSA_NOTIFIER_VLAN_ADD , &info); - -return 0 ; +return dsa_port_notify (dp,DSA_NOTIFIER_VLAN_ADD , &info); } int dsa_port_vlan_del(struct dsa_port *dp,
@@ -273,10 +270,7 @@ int dsa_port_vlan_del(struct dsa_port *dp,if (netif_is_bridge_master (vlan->obj.orig_dev))return -EOPNOTSUPP ; -if (br_vlan_enabled (dp->bridge_dev)) -return dsa_port_notify (dp,DSA_NOTIFIER_VLAN_DEL , &info); - -return 0 ; +return dsa_port_notify (dp,DSA_NOTIFIER_VLAN_DEL , &info); }static struct phy_device *dsa_port_get_phy_device (struct dsa_port *dp)
Signature · 1 line
2.17.1
Quoted 37 lines · Florian Fainelli
Commit
2ea7a679ca2a("net: dsa: Don't add vlans when vlan filtering is disabled") changed the behavior of DSA switches when the switch ports are enslaved into the bridge and only pushed the VLAN configuration down to the switch if the bridge is configured with VLAN filtering enabled.This is unfortunately wrong, because what
vlan_filteringconfigures is a policy on the acceptance of VLAN tagged frames with an unknown VID.vlan_filtering=0 means a frame with a VLAN tag that is not part of the VLAN table should be allowed to ingress the switch, and vlan_fltering=1 would reject that frame.
Fixes 2ea7a679ca2a ("net: dsa: Don't add vlans when vlan filtering is disabled")Signed-off-by Florian Fainelli <[email protected]>---
Andrew,I checked with Jiri and he confirmed that our interpretention of
vlan_filteringin DSA was incorrect and that it does denote whether the switch should be doing VID ingress policy checking.You mentioned in the commit message some problems without being too specific about them which is why I am putting the same checks in mv88e6xxx in order not to break your use cases. Let me know if you want to drop that hunk entirely.
Thanks!
drivers/net/dsa/mv88e6xxx/chip.c | 7 ++++++- net/dsa/port.c | 10 ++-------- 2 files changed, 8 insertions(+), 9 deletions(-)drivers/net/dsa/mv88e6xxx/chip.c +0
-0
drivers/net/dsa/mv88e6xxx/chip.c static int _mv88e6xxx_port_vlan_add(struct mv88e6xxx_chip *chip, int port, · line 1687
Quoted 4 lines
drivers/net/dsa/mv88e6xxx/chip.c +1
-0@@ -1684,13 +1684,14 @@ static int _mv88e6xxx_port_vlan_add(struct mv88e6xxx_chip *chip, int port,static void mv88e6xxx_port_vlan_add (struct dsa_switch *ds,int port,const struct switchdev_obj_port_vlan *vlan) { +struct dsa_port *dp = &ds->ports[i];
This should obviously be port instead of i, but I would still appreciate a review, thanks!
Signature · 1 line
Florian
Quoted 4 lines · Florian Fainelli
Commit
2ea7a679ca2a("net: dsa: Don't add vlans when vlan filtering is disabled") changed the behavior of DSA switches when the switch ports are enslaved into the bridge and only pushed the VLAN configuration down to the switch if the bridge is configured with VLAN filtering enabled.
This is what mlxsw is doing.
Quoted 6 lines
This is unfortunately wrong, because what
vlan_filteringconfigures is a policy on the acceptance of VLAN tagged frames with an unknown VID.vlan_filtering=0 means a frame with a VLAN tag that is not part of the VLAN table should be allowed to ingress the switch, and vlan_fltering=1 would reject that frame.
While you correctly describe the logic, this is not how VLAN-unaware bridges are actually used. The expectation is that packets will be untagged when entering the bridge. Either because they are truly untagged or because they were untagged by a VLAN netdev.
For a long time we rejected the enslavement of physical ports to VLAN-unaware bridges and only allowed VLAN netdevs to be enslaved. In order to support the logic you described, we would need to map all 4K VLANs on each port to 4K different FIDs. In addition, each FDB entry would need to be programmed 4K times, each time with a different FID. This is because FDB lookup is performed using {MAC, FID} and not only MAC. I can go into more details about why we cannot map different VLANs on a port to the same FID, but I do not think it is pertinent to our discussion.
Eventually, users started complaining about this constraint and we relaxed it in commit 65b53bfd497b ("mlxsw: spectrum_switchdev: Allow port enslavement to a VLAN-unaware bridge").
P.S. Corrected Petr's mail address.
Quoted 30 lines · Ido Schimmel
Quoted 4 lines · Florian Fainelli
Commit
2ea7a679ca2a("net: dsa: Don't add vlans when vlan filtering is disabled") changed the behavior of DSA switches when the switch ports are enslaved into the bridge and only pushed the VLAN configuration down to the switch if the bridge is configured with VLAN filtering enabled.This is what mlxsw is doing.
Quoted 6 lines
This is unfortunately wrong, because what
vlan_filteringconfigures is a policy on the acceptance of VLAN tagged frames with an unknown VID.vlan_filtering=0 means a frame with a VLAN tag that is not part of the VLAN table should be allowed to ingress the switch, and vlan_fltering=1 would reject that frame.
While you correctly describe the logic, this is not how VLAN-unaware bridges are actually used. The expectation is that packets will be untagged when entering the bridge. Either because they are truly untagged or because they were untagged by a VLAN netdev.
For a long time we rejected the enslavement of physical ports to VLAN-unaware bridges and only allowed VLAN netdevs to be enslaved. In order to support the logic you described, we would need to map all 4K VLANs on each port to 4K different FIDs. In addition, each FDB entry would need to be programmed 4K times, each time with a different FID. This is because FDB lookup is performed using {MAC, FID} and not only MAC. I can go into more details about why we cannot map different VLANs on a port to the same FID, but I do not think it is pertinent to our discussion.
Quoted 3 lines
Eventually, users started complaining about this constraint and we relaxed it in commit
65b53bfd497b("mlxsw:spectrum_switchdev: Allow port enslavement to a VLAN-unaware bridge").
Thanks for providing more context, I suppose we will keep the current logic then, if nothing else it aligns us with mlxsw.
Signature · 1 line
Florian