| Message ID | E1j41KW-0002v5-2S@rmk-PC.armlinux.org.uk |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | VLANs, DSA switches and multiple bridges | expand |
Hi Russell, On Tue, 18 Feb 2020 11:46:20 +0000, Russell King <rmk+kernel@armlinux.org.uk> 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. > > 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(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index 629eb7bbbb23..e656e571ef7d 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -2934,6 +2934,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 63495e3443ac..d3a826646e8e 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -273,6 +273,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 088c886e609e..534d511b349e 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -318,7 +318,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); > @@ -385,7 +386,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 > @@ -1106,7 +1108,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 > @@ -1140,7 +1143,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 It is confusing to add a Marvell specific term (VTU) in the generic dsa_switch structure to bypass the fact that VLAN configuration in hardware was already bypassed for some reasons until vlan filtering is turned on. As you said, simply offloading the VLAN configuration in hardware and only turning the ports' 802.1Q mode to Secure once vlan_filtering is flipped to 1 should work in theory for both VLAN filtering aware and unaware scenarios, but this was causing problems if I'm not mistaken, I'll try to dig this out. In the meantime, does the issue you're trying to solve here happens if you create a vlan-filtering aware bridge in the first place, before any VLAN configuration? i.e.: # ip link add name br0 type bridge vlan_filtering 1 # ip link set master br0 dev lan1 up # bridge vlan add ... Thank you, Vivien
On 2/18/20 11:09 AM, Vivien Didelot wrote: > Hi Russell, > > On Tue, 18 Feb 2020 11:46:20 +0000, Russell King <rmk+kernel@armlinux.org.uk> 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. >> >> 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(-) >> >> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c >> index 629eb7bbbb23..e656e571ef7d 100644 >> --- a/drivers/net/dsa/mv88e6xxx/chip.c >> +++ b/drivers/net/dsa/mv88e6xxx/chip.c >> @@ -2934,6 +2934,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 63495e3443ac..d3a826646e8e 100644 >> --- a/include/net/dsa.h >> +++ b/include/net/dsa.h >> @@ -273,6 +273,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 088c886e609e..534d511b349e 100644 >> --- a/net/dsa/slave.c >> +++ b/net/dsa/slave.c >> @@ -318,7 +318,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); >> @@ -385,7 +386,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 >> @@ -1106,7 +1108,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 >> @@ -1140,7 +1143,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 > > It is confusing to add a Marvell specific term (VTU) in the generic dsa_switch > structure to bypass the fact that VLAN configuration in hardware was already > bypassed for some reasons until vlan filtering is turned on. As you said, > simply offloading the VLAN configuration in hardware and only turning the > ports' 802.1Q mode to Secure once vlan_filtering is flipped to 1 should work > in theory for both VLAN filtering aware and unaware scenarios, but this was > causing problems if I'm not mistaken, I'll try to dig this out. > > In the meantime, does the issue you're trying to solve here happens if you > create a vlan-filtering aware bridge in the first place, before any VLAN > configuration? i.e.: > > # ip link add name br0 type bridge vlan_filtering 1 > # ip link set master br0 dev lan1 up > # bridge vlan add ... That will work okay because then you do get around the restrictions added by 2ea7a679ca2abd251c1ec03f20508619707e1749 ("net: dsa: Don't add vlans when vlan filtering is disabled") and you will get VLAN objects programming request to flow down your DSA driver. AFAICT, mlxsw specifically prevents to toggle vlan_filtering at runtime for that reason, because the VLAN objects notification are not "sent again" while you toggle vlan_filtering. I really wonder if we should not do the same in DSA as runtime toggling is of questionable use beyond just testing. Russell, in your tests/examples, did the tagged traffic of $VN continue to work after you toggled vlan_filtering on? If so, that must be because on a bridge with VLAN filtering disabled, we still ended up calling down to the lan1..6 ndo_vlan_rx_add_vid() and so we do have VLAN entries programmed for $VN.
On Tue, Feb 18, 2020 at 11:34:38AM -0800, Florian Fainelli wrote: > Russell, in your tests/examples, did the tagged traffic of $VN continue > to work after you toggled vlan_filtering on? If so, that must be because > on a bridge with VLAN filtering disabled, we still ended up calling down > to the lan1..6 ndo_vlan_rx_add_vid() and so we do have VLAN entries > programmed for $VN. From what I remember, _all_ traffic was blocked because the VTU was completely empty when vlan filtering is turned on.
Hi Russell, On Tue, 18 Feb 2020 23:48:06 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > On Tue, Feb 18, 2020 at 11:34:38AM -0800, Florian Fainelli wrote: > > Russell, in your tests/examples, did the tagged traffic of $VN continue > > to work after you toggled vlan_filtering on? If so, that must be because > > on a bridge with VLAN filtering disabled, we still ended up calling down > > to the lan1..6 ndo_vlan_rx_add_vid() and so we do have VLAN entries > > programmed for $VN. > > From what I remember, _all_ traffic was blocked because the VTU > was completely empty when vlan filtering is turned on. Not sure if the default PVID is 0 or 1, but one of them is ignored somehow. Setting up a default PVID in the first place is a way to ensure the programmation of the VTU, like in the following example: # ip link add name br0 type bridge vlan_filtering 1 vlan_default_pvid 42 # ip link set master br0 dev lan2 up # cat /sys/kernel/debug/mv88e6xxx/sw0/vtu vid 42 fid 1 sid 0 dpv 0 unmodified 2 untagged 10 unmodified Russell can you test this? If things work out for you when setting up a VLAN filtering aware bridge with your own default PVID before enslaving DSA ports, then the problem is narrowed to propagating the default bridge configuration (default PVID and configuration after enabling VLAN filtering). Thanks, Vivien
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 629eb7bbbb23..e656e571ef7d 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -2934,6 +2934,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 63495e3443ac..d3a826646e8e 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -273,6 +273,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 088c886e609e..534d511b349e 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -318,7 +318,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); @@ -385,7 +386,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 @@ -1106,7 +1108,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 @@ -1140,7 +1143,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(-)