| Message ID | 20200907182910.1285496-5-olteanv@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Some VLAN handling cleanup in DSA | expand |
On 9/7/2020 11:29 AM, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > As explained in commit 54a0ed0df496 ("net: dsa: provide an option for > drivers to always receive bridge VLANs"), DSA has historically been > skipping VLAN switchdev operations when the bridge wasn't in > vlan_filtering mode, but the reason why it was doing that has never been > clear. So the configure_vlan_while_not_filtering option is there merely > to preserve functionality for existing drivers. It isn't some behavior > that drivers should opt into. Ideally, when all drivers leave this flag > set, we can delete the dsa_port_skip_vlan_configuration() function. > > New drivers always seem to omit setting this flag, for some reason. So > let's reverse the logic: the DSA core sets it by default to true before > the .setup() callback, and legacy drivers can turn it off. This way, new > drivers get the new behavior by default, unless they explicitly set the > flag to false, which is more obvious during review. > > Remove the assignment from drivers which were setting it to true, and > add the assignment to false for the drivers that didn't previously have > it. This way, it should be easier to see how many we have left. > > The following drivers: lan9303, mv88e6060 were skipped from setting this > flag to false, because they didn't have any VLAN offload ops in the > first place. > > Also, print a message to the console every time a VLAN has been skipped. > This is mildly annoying on purpose, so that (a) it is at least clear > that VLANs are being skipped - the legacy behavior in itself is > confusing - and (b) people have one more incentive to convert to the new > behavior. > > No behavior change except for the added prints is intended at this time. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> You should be able to make b53 and bcm_sf2 also use configure_vlan_while_not_filtering to true (or rather not specify it), give me until tomorrow to run various tests if you don't mind.
On Mon Sep 07 2020, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > As explained in commit 54a0ed0df496 ("net: dsa: provide an option for > drivers to always receive bridge VLANs"), DSA has historically been > skipping VLAN switchdev operations when the bridge wasn't in > vlan_filtering mode, but the reason why it was doing that has never been > clear. So the configure_vlan_while_not_filtering option is there merely > to preserve functionality for existing drivers. It isn't some behavior > that drivers should opt into. Ideally, when all drivers leave this flag > set, we can delete the dsa_port_skip_vlan_configuration() function. > > New drivers always seem to omit setting this flag, for some reason. Yes, because it's not well documented, or is it? Before writing the hellcreek DSA driver, I've read dsa.rst documentation to find out what callback function should to what. Did I miss something? > So let's reverse the logic: the DSA core sets it by default to true > before the .setup() callback, and legacy drivers can turn it off. This > way, new drivers get the new behavior by default, unless they > explicitly set the flag to false, which is more obvious during review. Yeah, that behavior makes more sense to me. Thank you. Thanks, Kurt
On Tue, Sep 08, 2020 at 12:14:12PM +0200, Kurt Kanzenbach wrote: > On Mon Sep 07 2020, Vladimir Oltean wrote: > > New drivers always seem to omit setting this flag, for some reason. > > Yes, because it's not well documented, or is it? Before writing the > hellcreek DSA driver, I've read dsa.rst documentation to find out what > callback function should to what. Did I miss something? Honestly, Documentation/networking/dsa/dsa.rst is out of date by quite a bit. And this trend of having boolean flags in struct dsa_switch started after the documentation stopped being updated. But I didn't say it's your fault for not setting the flag, it is easy to miss, and that's what this patch is trying to improve. > > So let's reverse the logic: the DSA core sets it by default to true > > before the .setup() callback, and legacy drivers can turn it off. This > > way, new drivers get the new behavior by default, unless they > > explicitly set the flag to false, which is more obvious during review. > > Yeah, that behavior makes more sense to me. Thank you. Ok, thanks. -Vladimir
On Mon, Sep 07, 2020 at 09:07:34PM -0700, Florian Fainelli wrote: > You should be able to make b53 and bcm_sf2 also use > configure_vlan_while_not_filtering to true (or rather not specify it), give > me until tomorrow to run various tests if you don't mind. Ok, but I would prefer not doing that in this patch. Note that, given a choice, I try to avoid introducing functional changes in drivers where I don't have the hardware to test, or somebody to confirm that it works, at the very least. For that reason, mv88e6xxx doesn't even have this flag set, even though Russell had sent a patch for it with the old name: https://lore.kernel.org/netdev/E1ij6pq-00084C-47@rmk-PC.armlinux.org.uk/ Somebody with a Marvell switch should probably pick that up and resend. Thanks, -Vladimir
On 9/7/2020 9:07 PM, Florian Fainelli wrote: > > > On 9/7/2020 11:29 AM, Vladimir Oltean wrote: >> From: Vladimir Oltean <vladimir.oltean@nxp.com> >> >> As explained in commit 54a0ed0df496 ("net: dsa: provide an option for >> drivers to always receive bridge VLANs"), DSA has historically been >> skipping VLAN switchdev operations when the bridge wasn't in >> vlan_filtering mode, but the reason why it was doing that has never been >> clear. So the configure_vlan_while_not_filtering option is there merely >> to preserve functionality for existing drivers. It isn't some behavior >> that drivers should opt into. Ideally, when all drivers leave this flag >> set, we can delete the dsa_port_skip_vlan_configuration() function. >> >> New drivers always seem to omit setting this flag, for some reason. So >> let's reverse the logic: the DSA core sets it by default to true before >> the .setup() callback, and legacy drivers can turn it off. This way, new >> drivers get the new behavior by default, unless they explicitly set the >> flag to false, which is more obvious during review. >> >> Remove the assignment from drivers which were setting it to true, and >> add the assignment to false for the drivers that didn't previously have >> it. This way, it should be easier to see how many we have left. >> >> The following drivers: lan9303, mv88e6060 were skipped from setting this >> flag to false, because they didn't have any VLAN offload ops in the >> first place. >> >> Also, print a message to the console every time a VLAN has been skipped. >> This is mildly annoying on purpose, so that (a) it is at least clear >> that VLANs are being skipped - the legacy behavior in itself is >> confusing - and (b) people have one more incentive to convert to the new >> behavior. >> >> No behavior change except for the added prints is intended at this time. >> >> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > You should be able to make b53 and bcm_sf2 also use > configure_vlan_while_not_filtering to true (or rather not specify it), > give me until tomorrow to run various tests if you don't mind. For a reason I do not understand, we should be able to set configure_vlan_while_not_filtering to true, and get things to work, however this does not work for bridged ports unless the bridge also happens to have VLAN filtering enabled. There is a valid VLAN entry configured for the desired port, but nothing ingress or egresses, I will keep debugging but you can send this patch as-is I believe and I will amend b53 later once I understand what is going on.
On 9/8/2020 3:28 PM, Florian Fainelli wrote: > > > On 9/7/2020 9:07 PM, Florian Fainelli wrote: >> >> >> On 9/7/2020 11:29 AM, Vladimir Oltean wrote: >>> From: Vladimir Oltean <vladimir.oltean@nxp.com> >>> >>> As explained in commit 54a0ed0df496 ("net: dsa: provide an option for >>> drivers to always receive bridge VLANs"), DSA has historically been >>> skipping VLAN switchdev operations when the bridge wasn't in >>> vlan_filtering mode, but the reason why it was doing that has never been >>> clear. So the configure_vlan_while_not_filtering option is there merely >>> to preserve functionality for existing drivers. It isn't some behavior >>> that drivers should opt into. Ideally, when all drivers leave this flag >>> set, we can delete the dsa_port_skip_vlan_configuration() function. >>> >>> New drivers always seem to omit setting this flag, for some reason. So >>> let's reverse the logic: the DSA core sets it by default to true before >>> the .setup() callback, and legacy drivers can turn it off. This way, new >>> drivers get the new behavior by default, unless they explicitly set the >>> flag to false, which is more obvious during review. >>> >>> Remove the assignment from drivers which were setting it to true, and >>> add the assignment to false for the drivers that didn't previously have >>> it. This way, it should be easier to see how many we have left. >>> >>> The following drivers: lan9303, mv88e6060 were skipped from setting this >>> flag to false, because they didn't have any VLAN offload ops in the >>> first place. >>> >>> Also, print a message to the console every time a VLAN has been skipped. >>> This is mildly annoying on purpose, so that (a) it is at least clear >>> that VLANs are being skipped - the legacy behavior in itself is >>> confusing - and (b) people have one more incentive to convert to the new >>> behavior. >>> >>> No behavior change except for the added prints is intended at this time. >>> >>> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> >> >> You should be able to make b53 and bcm_sf2 also use >> configure_vlan_while_not_filtering to true (or rather not specify it), >> give me until tomorrow to run various tests if you don't mind. > > For a reason I do not understand, we should be able to set > configure_vlan_while_not_filtering to true, and get things to work, > however this does not work for bridged ports unless the bridge also > happens to have VLAN filtering enabled. There is a valid VLAN entry > configured for the desired port, but nothing ingress or egresses, I will > keep debugging but you can send this patch as-is I believe and I will > amend b53 later once I understand what is going on. Found the problem, we do not allow the CPU port to be configured as untagged, and when we toggle vlan_filtering we actually incorrectly "move" the PVID from 1 to 0, which is incorrect, but since the CPU is also untagged in VID 0 this is why it "works" or rather two mistakes canceling it each other. I still need to confirm this, but the bridge in VLAN filtering mode seems to support receiving frames with the default_pvid as tagged, and it will untag it for the bridge master device transparently. The reason for not allowing the CPU port to be untagged (ca8931948344c485569b04821d1f6bcebccd376b) was because the CPU port could be added as untagged in several VLANs, e.g.: when port0-3 are PVID 1 untagged, and port 4 is PVID 2 untagged. Back then there was no support for Broadcom tags, so the only way to differentiate traffic properly was to also add a pair of tagged VIDs to the DSA master. I am still trying to remember whether there were other concerns that prompted me to make that change and would appreciate some thoughts on that. Tangentially, maybe we should finally add support for programming the CPU port's VLAN membership independently from the other ports. The following appears to work nicely now and allows us to get rid of the b53_vlan_filtering() logic, which would no longer work now because it assumed that toggling vlan_filtering implied that there would be no VLAN configuration when filtering was off. diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c index 26fcff85d881..fac033730f4a 100644 --- a/drivers/net/dsa/b53/b53_common.c +++ b/drivers/net/dsa/b53/b53_common.c @@ -1322,23 +1322,6 @@ EXPORT_SYMBOL(b53_phylink_mac_link_up); int b53_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering) { struct b53_device *dev = ds->priv; - u16 pvid, new_pvid; - - b53_read16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port), &pvid); - if (!vlan_filtering) { - /* Filtering is currently enabled, use the default PVID since - * the bridge does not expect tagging anymore - */ - dev->ports[port].pvid = pvid; - new_pvid = b53_default_pvid(dev); - } else { - /* Filtering is currently disabled, restore the previous PVID */ - new_pvid = dev->ports[port].pvid; - } - - if (pvid != new_pvid) - b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port), - new_pvid); b53_enable_vlan(dev, dev->vlan_enabled, vlan_filtering); @@ -1389,7 +1372,7 @@ void b53_vlan_add(struct dsa_switch *ds, int port, untagged = true; vl->members |= BIT(port); - if (untagged && !dsa_is_cpu_port(ds, port)) + if (untagged) vl->untag |= BIT(port); else vl->untag &= ~BIT(port); @@ -1427,7 +1410,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port, if (pvid == vid) pvid = b53_default_pvid(dev); - if (untagged && !dsa_is_cpu_port(ds, port)) + if (untagged) vl->untag &= ~(BIT(port)); b53_set_vlan_entry(dev, vid, vl); @@ -2563,6 +2546,8 @@ struct b53_device *b53_switch_alloc(struct device *base, dev->priv = priv; dev->ops = ops; ds->ops = &b53_switch_ops; + ds->configure_vlan_while_not_filtering = true; + dev->vlan_enabled = ds->configure_vlan_while_not_filtering; mutex_init(&dev->reg_mutex); mutex_init(&dev->stats_mutex);
On Tue, Sep 08, 2020 at 05:02:06PM -0700, Florian Fainelli wrote: > Found the problem, we do not allow the CPU port to be configured as > untagged, and when we toggle vlan_filtering we actually incorrectly "move" > the PVID from 1 to 0, pvid 1 must be coming from the default_pvid of the bridge, I assume. Where is pvid 0 (aka dev->ports[port].pvid) coming from? Is it simply the cached value from B53_VLAN_PORT_DEF_TAG, from a previous b53_vlan_filtering() call? Strange. > which is incorrect, but since the CPU is also untagged in VID 0 this > is why it "works" or rather two mistakes canceling it each other. How does the CPU end up untagged in VLAN 0? > I still need to confirm this, but the bridge in VLAN filtering mode seems to > support receiving frames with the default_pvid as tagged, and it will untag > it for the bridge master device transparently. So it seems. > The reason for not allowing the CPU port to be untagged > (ca8931948344c485569b04821d1f6bcebccd376b) was because the CPU port could be > added as untagged in several VLANs, e.g.: when port0-3 are PVID 1 untagged, > and port 4 is PVID 2 untagged. Back then there was no support for Broadcom > tags, so the only way to differentiate traffic properly was to also add a > pair of tagged VIDs to the DSA master. > I am still trying to remember whether there were other concerns that > prompted me to make that change and would appreciate some thoughts on that. I think it makes some sense to always configure the VLANs on the CPU port as tagged either way. I did the same in Felix and it's ok. But that was due to a hardware limitation. On sja1105 I'm keeping the same flags as on the user port, and that is ok too. > Tangentially, maybe we should finally add support for programming the CPU > port's VLAN membership independently from the other ports. How? > The following appears to work nicely now and allows us to get rid of the > b53_vlan_filtering() logic, which would no longer work now because it > assumed that toggling vlan_filtering implied that there would be no VLAN > configuration when filtering was off. > > diff --git a/drivers/net/dsa/b53/b53_common.c > b/drivers/net/dsa/b53/b53_common.c > index 26fcff85d881..fac033730f4a 100644 > --- a/drivers/net/dsa/b53/b53_common.c > +++ b/drivers/net/dsa/b53/b53_common.c > @@ -1322,23 +1322,6 @@ EXPORT_SYMBOL(b53_phylink_mac_link_up); > int b53_vlan_filtering(struct dsa_switch *ds, int port, bool > vlan_filtering) > { > struct b53_device *dev = ds->priv; > - u16 pvid, new_pvid; > - > - b53_read16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port), &pvid); > - if (!vlan_filtering) { > - /* Filtering is currently enabled, use the default PVID > since > - * the bridge does not expect tagging anymore > - */ > - dev->ports[port].pvid = pvid; > - new_pvid = b53_default_pvid(dev); > - } else { > - /* Filtering is currently disabled, restore the previous > PVID */ > - new_pvid = dev->ports[port].pvid; > - } > - > - if (pvid != new_pvid) > - b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port), > - new_pvid); Yes, much simpler. > > b53_enable_vlan(dev, dev->vlan_enabled, vlan_filtering); > > @@ -1389,7 +1372,7 @@ void b53_vlan_add(struct dsa_switch *ds, int port, > untagged = true; > > vl->members |= BIT(port); > - if (untagged && !dsa_is_cpu_port(ds, port)) > + if (untagged) > vl->untag |= BIT(port); > else > vl->untag &= ~BIT(port); > @@ -1427,7 +1410,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port, > if (pvid == vid) > pvid = b53_default_pvid(dev); > > - if (untagged && !dsa_is_cpu_port(ds, port)) > + if (untagged) Ok, so you're removing this workaround now. A welcome simplification. > vl->untag &= ~(BIT(port)); > > b53_set_vlan_entry(dev, vid, vl); > @@ -2563,6 +2546,8 @@ struct b53_device *b53_switch_alloc(struct device > *base, > dev->priv = priv; > dev->ops = ops; > ds->ops = &b53_switch_ops; > + ds->configure_vlan_while_not_filtering = true; > + dev->vlan_enabled = ds->configure_vlan_while_not_filtering; > mutex_init(&dev->reg_mutex); > mutex_init(&dev->stats_mutex); > > > -- > Florian Looks good! I'm going to hold off with my configure_vlan_while_not_filtering patch. You can send this one before me. Thanks, -Vladimir
On 9/9/2020 9:31 AM, Vladimir Oltean wrote: > On Tue, Sep 08, 2020 at 05:02:06PM -0700, Florian Fainelli wrote: >> Found the problem, we do not allow the CPU port to be configured as >> untagged, and when we toggle vlan_filtering we actually incorrectly "move" >> the PVID from 1 to 0, > > pvid 1 must be coming from the default_pvid of the bridge, I assume. > Where is pvid 0 (aka dev->ports[port].pvid) coming from? Is it simply > the cached value from B53_VLAN_PORT_DEF_TAG, from a previous > b53_vlan_filtering() call? Strange. The logic that writes to B53_VLAN_PORT_DEF_TAG does not update the shadow copy in dev->ports[port].pvid which is how they are out of sync. > >> which is incorrect, but since the CPU is also untagged in VID 0 this >> is why it "works" or rather two mistakes canceling it each other. > > How does the CPU end up untagged in VLAN 0? The CPU port gets also programmed with 0 in B53_VLAN_PORT_DEF_TAG. > >> I still need to confirm this, but the bridge in VLAN filtering mode seems to >> support receiving frames with the default_pvid as tagged, and it will untag >> it for the bridge master device transparently. > > So it seems. > >> The reason for not allowing the CPU port to be untagged >> (ca8931948344c485569b04821d1f6bcebccd376b) was because the CPU port could be >> added as untagged in several VLANs, e.g.: when port0-3 are PVID 1 untagged, >> and port 4 is PVID 2 untagged. Back then there was no support for Broadcom >> tags, so the only way to differentiate traffic properly was to also add a >> pair of tagged VIDs to the DSA master. >> I am still trying to remember whether there were other concerns that >> prompted me to make that change and would appreciate some thoughts on that. > > I think it makes some sense to always configure the VLANs on the CPU > port as tagged either way. I did the same in Felix and it's ok. But that > was due to a hardware limitation. On sja1105 I'm keeping the same flags > as on the user port, and that is ok too. How do you make sure that the CPU port sees the frame untagged which would be necessary for a VLAN-unaware bridge? Do you have a special remapping rule? Initially the concern I had was with the use case described above which was a 802.1Q separation, but in hindsight MAC address learning would result in the frames going to the appropriate ports/VLANs anyway. > >> Tangentially, maybe we should finally add support for programming the CPU >> port's VLAN membership independently from the other ports. > > How? Something like this: https://lore.kernel.org/lkml/20180625091713.GA13442@apalos/T/ > >> The following appears to work nicely now and allows us to get rid of the >> b53_vlan_filtering() logic, which would no longer work now because it >> assumed that toggling vlan_filtering implied that there would be no VLAN >> configuration when filtering was off. >> >> diff --git a/drivers/net/dsa/b53/b53_common.c >> b/drivers/net/dsa/b53/b53_common.c >> index 26fcff85d881..fac033730f4a 100644 >> --- a/drivers/net/dsa/b53/b53_common.c >> +++ b/drivers/net/dsa/b53/b53_common.c >> @@ -1322,23 +1322,6 @@ EXPORT_SYMBOL(b53_phylink_mac_link_up); >> int b53_vlan_filtering(struct dsa_switch *ds, int port, bool >> vlan_filtering) >> { >> struct b53_device *dev = ds->priv; >> - u16 pvid, new_pvid; >> - >> - b53_read16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port), &pvid); >> - if (!vlan_filtering) { >> - /* Filtering is currently enabled, use the default PVID >> since >> - * the bridge does not expect tagging anymore >> - */ >> - dev->ports[port].pvid = pvid; >> - new_pvid = b53_default_pvid(dev); >> - } else { >> - /* Filtering is currently disabled, restore the previous >> PVID */ >> - new_pvid = dev->ports[port].pvid; >> - } >> - >> - if (pvid != new_pvid) >> - b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port), >> - new_pvid); > > Yes, much simpler. > >> >> b53_enable_vlan(dev, dev->vlan_enabled, vlan_filtering); >> >> @@ -1389,7 +1372,7 @@ void b53_vlan_add(struct dsa_switch *ds, int port, >> untagged = true; >> >> vl->members |= BIT(port); >> - if (untagged && !dsa_is_cpu_port(ds, port)) >> + if (untagged) >> vl->untag |= BIT(port); >> else >> vl->untag &= ~BIT(port); >> @@ -1427,7 +1410,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port, >> if (pvid == vid) >> pvid = b53_default_pvid(dev); >> >> - if (untagged && !dsa_is_cpu_port(ds, port)) >> + if (untagged) > > Ok, so you're removing this workaround now. A welcome simplification. > >> vl->untag &= ~(BIT(port)); >> >> b53_set_vlan_entry(dev, vid, vl); >> @@ -2563,6 +2546,8 @@ struct b53_device *b53_switch_alloc(struct device >> *base, >> dev->priv = priv; >> dev->ops = ops; >> ds->ops = &b53_switch_ops; >> + ds->configure_vlan_while_not_filtering = true; >> + dev->vlan_enabled = ds->configure_vlan_while_not_filtering; >> mutex_init(&dev->reg_mutex); >> mutex_init(&dev->stats_mutex); >> >> >> -- >> Florian > > Looks good! > > I'm going to hold off with my configure_vlan_while_not_filtering patch. > You can send this one before me. That's the plan, thanks!
On Wed, Sep 09, 2020 at 10:22:42AM -0700, Florian Fainelli wrote: > How do you make sure that the CPU port sees the frame untagged which would > be necessary for a VLAN-unaware bridge? Do you have a special remapping > rule? No, I don't have any remapping rules that would be relevant here. Why would the frames need to be necessarily untagged for a VLAN-unaware bridge, why is it a problem if they aren't? bool br_allowed_ingress(const struct net_bridge *br, struct net_bridge_vlan_group *vg, struct sk_buff *skb, u16 *vid, u8 *state) { /* If VLAN filtering is disabled on the bridge, all packets are * permitted. */ if (!br_opt_get(br, BROPT_VLAN_ENABLED)) { BR_INPUT_SKB_CB(skb)->vlan_filtered = false; return true; } return __allowed_ingress(br, vg, skb, vid, state); } If I have a VLAN on a bridged switch port where the bridge is not filtering, I have an 8021q upper of the bridge with that VLAN ID. > Initially the concern I had was with the use case described above which was > a 802.1Q separation, but in hindsight MAC address learning would result in > the frames going to the appropriate ports/VLANs anyway. If by "separation" you mean "limiting the forwarding domain", the switch keeps the same VLAN associated with the frame internally, regardless of whether it's egress-tagged or not. > > > > > Tangentially, maybe we should finally add support for programming the CPU > > > port's VLAN membership independently from the other ports. > > > > How? > > Something like this: > > https://lore.kernel.org/lkml/20180625091713.GA13442@apalos/T/ I need to take some time to understand what's going on there.
On 9/9/2020 10:53 AM, Vladimir Oltean wrote: > On Wed, Sep 09, 2020 at 10:22:42AM -0700, Florian Fainelli wrote: >> How do you make sure that the CPU port sees the frame untagged which would >> be necessary for a VLAN-unaware bridge? Do you have a special remapping >> rule? > > No, I don't have any remapping rules that would be relevant here. > Why would the frames need to be necessarily untagged for a VLAN-unaware > bridge, why is it a problem if they aren't? > > bool br_allowed_ingress(const struct net_bridge *br, > struct net_bridge_vlan_group *vg, struct sk_buff *skb, > u16 *vid, u8 *state) > { > /* If VLAN filtering is disabled on the bridge, all packets are > * permitted. > */ > if (!br_opt_get(br, BROPT_VLAN_ENABLED)) { > BR_INPUT_SKB_CB(skb)->vlan_filtered = false; > return true; > } > > return __allowed_ingress(br, vg, skb, vid, state); > } > > If I have a VLAN on a bridged switch port where the bridge is not > filtering, I have an 8021q upper of the bridge with that VLAN ID. Yes that is the key right there, you need an 8021q upper to pop the VLAN ID or push it, that is another thing that users need to be aware of which is a bit awkward, most expect things to just work. Maybe we should just refuse to have bridge devices that are not VLAN-aware, because this is just too cumbersome to deal with. > >> Initially the concern I had was with the use case described above which was >> a 802.1Q separation, but in hindsight MAC address learning would result in >> the frames going to the appropriate ports/VLANs anyway. > > If by "separation" you mean "limiting the forwarding domain", the switch > keeps the same VLAN associated with the frame internally, regardless of > whether it's egress-tagged or not. True, so I am not sure what I was thinking back then. > >>> >>>> Tangentially, maybe we should finally add support for programming the CPU >>>> port's VLAN membership independently from the other ports. >>> >>> How? >> >> Something like this: >> >> https://lore.kernel.org/lkml/20180625091713.GA13442@apalos/T/ > > I need to take some time to understand what's going on there. >
On 9/9/2020 11:34 AM, Florian Fainelli wrote: > > > On 9/9/2020 10:53 AM, Vladimir Oltean wrote: >> On Wed, Sep 09, 2020 at 10:22:42AM -0700, Florian Fainelli wrote: >>> How do you make sure that the CPU port sees the frame untagged which >>> would >>> be necessary for a VLAN-unaware bridge? Do you have a special remapping >>> rule? >> >> No, I don't have any remapping rules that would be relevant here. >> Why would the frames need to be necessarily untagged for a VLAN-unaware >> bridge, why is it a problem if they aren't? >> >> bool br_allowed_ingress(const struct net_bridge *br, >> struct net_bridge_vlan_group *vg, struct sk_buff *skb, >> u16 *vid, u8 *state) >> { >> /* If VLAN filtering is disabled on the bridge, all packets are >> * permitted. >> */ >> if (!br_opt_get(br, BROPT_VLAN_ENABLED)) { >> BR_INPUT_SKB_CB(skb)->vlan_filtered = false; >> return true; >> } >> >> return __allowed_ingress(br, vg, skb, vid, state); >> } >> >> If I have a VLAN on a bridged switch port where the bridge is not >> filtering, I have an 8021q upper of the bridge with that VLAN ID. > > Yes that is the key right there, you need an 8021q upper to pop the VLAN > ID or push it, that is another thing that users need to be aware of > which is a bit awkward, most expect things to just work. Maybe we should > just refuse to have bridge devices that are not VLAN-aware, because this > is just too cumbersome to deal with. With the drivers that you currently maintain and with the CPU port being always tagged in the VLANs added to the user-facing ports, when you are using a non-VLAN aware bridge, do you systematically add an br0.1 upper 802.1Q device to pop/push the VLAN tag? I am about ready to submit the changes we discussed to b53, but I am still a bit uncomfortable with this part of the change because it will make the CPU port follow the untagged attribute of an user-facing port. @@ -1444,7 +1427,7 @@ void b53_vlan_add(struct dsa_switch *ds, int port, untagged = true; vl->members |= BIT(port); - if (untagged && !dsa_is_cpu_port(ds, port)) + if (untagged) vl->untag |= BIT(port); else vl->untag &= ~BIT(port); @@ -1482,7 +1465,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port, if (pvid == vid) pvid = b53_default_pvid(dev); - if (untagged && !dsa_is_cpu_port(ds, port)) + if (untagged) vl->untag &= ~(BIT(port));
On Thu, Sep 10, 2020 at 02:58:04PM -0700, Florian Fainelli wrote: > On 9/9/2020 11:34 AM, Florian Fainelli wrote: > > On 9/9/2020 10:53 AM, Vladimir Oltean wrote: > > > On Wed, Sep 09, 2020 at 10:22:42AM -0700, Florian Fainelli wrote: > > > > How do you make sure that the CPU port sees the frame untagged > > > > which would > > > > be necessary for a VLAN-unaware bridge? Do you have a special remapping > > > > rule? > > > > > > No, I don't have any remapping rules that would be relevant here. > > > Why would the frames need to be necessarily untagged for a VLAN-unaware > > > bridge, why is it a problem if they aren't? > > > > > > bool br_allowed_ingress(const struct net_bridge *br, > > > struct net_bridge_vlan_group *vg, struct sk_buff *skb, > > > u16 *vid, u8 *state) > > > { > > > /* If VLAN filtering is disabled on the bridge, all packets are > > > * permitted. > > > */ > > > if (!br_opt_get(br, BROPT_VLAN_ENABLED)) { > > > BR_INPUT_SKB_CB(skb)->vlan_filtered = false; > > > return true; > > > } > > > > > > return __allowed_ingress(br, vg, skb, vid, state); > > > } > > > > > > If I have a VLAN on a bridged switch port where the bridge is not > > > filtering, I have an 8021q upper of the bridge with that VLAN ID. > > > > Yes that is the key right there, you need an 8021q upper to pop the VLAN > > ID or push it, that is another thing that users need to be aware of > > which is a bit awkward, most expect things to just work. Maybe we should > > just refuse to have bridge devices that are not VLAN-aware, because this > > is just too cumbersome to deal with. > > With the drivers that you currently maintain and with the CPU port being > always tagged in the VLANs added to the user-facing ports, when you are > using a non-VLAN aware bridge, do you systematically add an br0.1 upper > 802.1Q device to pop/push the VLAN tag? Talking to you, I realized that I confused you uselessly. But in doing that, I actually cleared up a couple of things for myself. So thanks, I guess? This is actually a great question, and it gave me the opportunity to reflect. So, only 1 driver that I maintain has the logic of always marking the CPU port as egress-tagged. And that would be ocelot/felix. I need to give you a bit of background. The DSA mode of Ocelot switches is more of an afterthought, and I am saying this because there is a distinction I need to make between the "CPU port module" (which is a set of queues that the CPU can inject and extract frames from), and the "NPI port" (which is an operating mode, where a regular front-panel Ethernet port is connected internally to the CPU port module and injection/extraction I/O can therefore be done via Ethernet, and that's your DSA). Basically, when the NPI mode is in use, then it behaves less like an Ethernet port, and more like a set of CPU queues that connect over Ethernet, if that makes sense. The port settings for VLAN are bypassed, and the packet is copied as-is from ingress to the NPI port. The egress-tagged port VLAN configuration does not actually result in a VLAN header being pushed into the frame, if that egress port is the NPI port. Instead, the classified VLAN ID (i.e. derived from the packet, or from the port-based VLAN, or from custom VLAN classification TCAM rules) is always kept in a 12-bit field of the Extraction Frame Header. Currently I am ignoring the classified VLAN from the Extraction Frame Header, and simply passing the skb as-is to the stack. As-is, meaning as the switch ingress port had received it. So, in retrospect, my patch 183be6f967fe ("net: dsa: felix: send VLANs on CPU port as egress-tagged") is nothing more than a formality to make this piece of code shut up and not error out: static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port, u16 vid) { struct ocelot_port *ocelot_port = ocelot->ports[port]; u32 val = 0; if (ocelot_port->vid != vid) { /* Always permit deleting the native VLAN (vid = 0) */ if (ocelot_port->vid && vid) { dev_err(ocelot->dev, "Port already has a native VLAN: %d\n", ocelot_port->vid); return -EBUSY; } ocelot_port->vid = vid; } It's just now that I connected the dots and realized that. So, looks like I don't really know what it's like to always have a tagged skb on ingress, even for egress-tagged VLANs. It must suck, I guess? I think if I were in that situation, and the source port would be under a vlan_filtering=0 bridge, then I would simply pop the tag from the skb in the DSA rcv function, for all VLANs that I don't have an 8021q upper for. Explaining this, it makes a lot of sense to do what Vitesse / Microsemi / Microchip is doing, which is to copy the frame as-is to the CPU, and to also tell you, separately, what the classified VLAN is. For example, in vlan_filtering=0 mode, the classified VLAN will always be 1, regardless of how the frame is tagged, because VLAN awareness is truly turned off for the ingress port, and the port-based VLAN is always used. In this way, you have the most flexibility: you can either ignore the classified VLAN and proceed with just what was in the ingress skb (this way, you'll have a switch that is not VLAN-unaware, just "checking" as opposed to "secure". It has passed the ingress VLAN filter, but you still remember what the VLAN ID was. Or you can have a completely VLAN-unaware switch, if you pop all VLANs that you can find in the skb, and add a hwaccel tag based on the classified VLAN, if it isn't equal to the pvid of the port. This is great for things like compatibility with a vlan_filtering=0 upper bridge which is what we're talking about. Basically, this is what, I think, DSA tries to emulate with the rule of copying the flags of a user port VLAN to the CPU port too. If we had the API to request an "unmodified" VLAN (not egress-tagged, not egress-untagged, just the same as on ingress), I'm sure we'd be using that by default (useful when vlan_filtering is 1). Knowing what the classified VLAN was also can be very useful at times (like when vlan_filtering is 0), so if there was an API for that, I'm sure DSA would have used that as well. With no such APIs, we can only use approximations. > I am about ready to submit the changes we discussed to b53, but I am still a > bit uncomfortable with this part of the change because it will make the CPU > port follow the untagged attribute of an user-facing port. > > @@ -1444,7 +1427,7 @@ void b53_vlan_add(struct dsa_switch *ds, int port, > untagged = true; > > vl->members |= BIT(port); > - if (untagged && !dsa_is_cpu_port(ds, port)) > + if (untagged) > vl->untag |= BIT(port); > else > vl->untag &= ~BIT(port); > @@ -1482,7 +1465,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port, > if (pvid == vid) > pvid = b53_default_pvid(dev); > > - if (untagged && !dsa_is_cpu_port(ds, port)) > + if (untagged) > vl->untag &= ~(BIT(port)); > Which is ok, I believe? I mean, that's the default DSA logic. If you think that isn't ok, we should change it at a more fundamental level. What we've been discussing so far is akin to your current setup, not to the one you're planning to change to, isn't it? Are there any problems with the new setup? Cheers, -Vladimir
On 9/10/2020 5:03 PM, Vladimir Oltean wrote: > On Thu, Sep 10, 2020 at 02:58:04PM -0700, Florian Fainelli wrote: >> On 9/9/2020 11:34 AM, Florian Fainelli wrote: >>> On 9/9/2020 10:53 AM, Vladimir Oltean wrote: >>>> On Wed, Sep 09, 2020 at 10:22:42AM -0700, Florian Fainelli wrote: >>>>> How do you make sure that the CPU port sees the frame untagged >>>>> which would >>>>> be necessary for a VLAN-unaware bridge? Do you have a special remapping >>>>> rule? >>>> >>>> No, I don't have any remapping rules that would be relevant here. >>>> Why would the frames need to be necessarily untagged for a VLAN-unaware >>>> bridge, why is it a problem if they aren't? >>>> >>>> bool br_allowed_ingress(const struct net_bridge *br, >>>> struct net_bridge_vlan_group *vg, struct sk_buff *skb, >>>> u16 *vid, u8 *state) >>>> { >>>> /* If VLAN filtering is disabled on the bridge, all packets are >>>> * permitted. >>>> */ >>>> if (!br_opt_get(br, BROPT_VLAN_ENABLED)) { >>>> BR_INPUT_SKB_CB(skb)->vlan_filtered = false; >>>> return true; >>>> } >>>> >>>> return __allowed_ingress(br, vg, skb, vid, state); >>>> } >>>> >>>> If I have a VLAN on a bridged switch port where the bridge is not >>>> filtering, I have an 8021q upper of the bridge with that VLAN ID. >>> >>> Yes that is the key right there, you need an 8021q upper to pop the VLAN >>> ID or push it, that is another thing that users need to be aware of >>> which is a bit awkward, most expect things to just work. Maybe we should >>> just refuse to have bridge devices that are not VLAN-aware, because this >>> is just too cumbersome to deal with. >> >> With the drivers that you currently maintain and with the CPU port being >> always tagged in the VLANs added to the user-facing ports, when you are >> using a non-VLAN aware bridge, do you systematically add an br0.1 upper >> 802.1Q device to pop/push the VLAN tag? > > Talking to you, I realized that I confused you uselessly. But in doing > that, I actually cleared up a couple of things for myself. So thanks, I > guess? > > This is actually a great question, and it gave me the opportunity to > reflect. So, only 1 driver that I maintain has the logic of always > marking the CPU port as egress-tagged. And that would be ocelot/felix. > > I need to give you a bit of background. > The DSA mode of Ocelot switches is more of an afterthought, and I am > saying this because there is a distinction I need to make between the > "CPU port module" (which is a set of queues that the CPU can inject and > extract frames from), and the "NPI port" (which is an operating mode, > where a regular front-panel Ethernet port is connected internally to the > CPU port module and injection/extraction I/O can therefore be done via > Ethernet, and that's your DSA). > Basically, when the NPI mode is in use, then it behaves less like an > Ethernet port, and more like a set of CPU queues that connect over > Ethernet, if that makes sense. SYSTEMPORT + bcm_sf2 act a lot like that, too, except the CPU port still obeys VLAN, buffering, classification and other switch internal rules, but essentially we want to map queues from the user-facing port to DMAs used by the host processor. > The port settings for VLAN are bypassed, and the packet is copied as-is > from ingress to the NPI port. The egress-tagged port VLAN configuration > does not actually result in a VLAN header being pushed into the frame, > if that egress port is the NPI port. Instead, the classified VLAN ID > (i.e. derived from the packet, or from the port-based VLAN, or from > custom VLAN classification TCAM rules) is always kept in a 12-bit field > of the Extraction Frame Header. > > Currently I am ignoring the classified VLAN from the Extraction Frame > Header, and simply passing the skb as-is to the stack. As-is, meaning as > the switch ingress port had received it. So, in retrospect, my patch > 183be6f967fe ("net: dsa: felix: send VLANs on CPU port as > egress-tagged") is nothing more than a formality to make this piece of > code shut up and not error out: > > static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port, > u16 vid) > { > struct ocelot_port *ocelot_port = ocelot->ports[port]; > u32 val = 0; > > if (ocelot_port->vid != vid) { > /* Always permit deleting the native VLAN (vid = 0) */ > if (ocelot_port->vid && vid) { > dev_err(ocelot->dev, > "Port already has a native VLAN: %d\n", > ocelot_port->vid); > return -EBUSY; > } > ocelot_port->vid = vid; > } > > It's just now that I connected the dots and realized that. > > So, looks like I don't really know what it's like to always have a > tagged skb on ingress, even for egress-tagged VLANs. It must suck, I > guess? > > I think if I were in that situation, and the source port would be under > a vlan_filtering=0 bridge, then I would simply pop the tag from the skb > in the DSA rcv function, for all VLANs that I don't have an 8021q upper > for. > > Explaining this, it makes a lot of sense to do what Vitesse / Microsemi > / Microchip is doing, which is to copy the frame as-is to the CPU, and > to also tell you, separately, what the classified VLAN is. For example, > in vlan_filtering=0 mode, the classified VLAN will always be 1, > regardless of how the frame is tagged, because VLAN awareness is truly > turned off for the ingress port, and the port-based VLAN is always used. > In this way, you have the most flexibility: you can either ignore the > classified VLAN and proceed with just what was in the ingress skb (this > way, you'll have a switch that is not VLAN-unaware, just "checking" as > opposed to "secure". It has passed the ingress VLAN filter, but you > still remember what the VLAN ID was. > Or you can have a completely VLAN-unaware switch, if you pop all VLANs > that you can find in the skb, and add a hwaccel tag based on the > classified VLAN, if it isn't equal to the pvid of the port. This is > great for things like compatibility with a vlan_filtering=0 upper bridge > which is what we're talking about. > > Basically, this is what, I think, DSA tries to emulate with the rule of > copying the flags of a user port VLAN to the CPU port too. If we had the > API to request an "unmodified" VLAN (not egress-tagged, not > egress-untagged, just the same as on ingress), I'm sure we'd be using > that by default (useful when vlan_filtering is 1). Knowing what the > classified VLAN was also can be very useful at times (like when > vlan_filtering is 0), so if there was an API for that, I'm sure DSA > would have used that as well. With no such APIs, we can only use > approximations. egress unmodified is what mv88e6xxx uses which is why I do not believe they have had the same issues that I had with vlan_filtering=0. For Broadcom switches there is not any option to have an umodified mode, the CPU port must have a valid VLAN membership (with vlan_filtering=1) and the egress untagged from the CPU port to the Ethernet MAC must match the expectations of the software data path behind. > >> I am about ready to submit the changes we discussed to b53, but I am still a >> bit uncomfortable with this part of the change because it will make the CPU >> port follow the untagged attribute of an user-facing port. >> >> @@ -1444,7 +1427,7 @@ void b53_vlan_add(struct dsa_switch *ds, int port, >> untagged = true; >> >> vl->members |= BIT(port); >> - if (untagged && !dsa_is_cpu_port(ds, port)) >> + if (untagged) >> vl->untag |= BIT(port); >> else >> vl->untag &= ~BIT(port); >> @@ -1482,7 +1465,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port, >> if (pvid == vid) >> pvid = b53_default_pvid(dev); >> >> - if (untagged && !dsa_is_cpu_port(ds, port)) >> + if (untagged) >> vl->untag &= ~(BIT(port)); >> > > Which is ok, I believe? I mean, that's the default DSA logic. If you > think that isn't ok, we should change it at a more fundamental level. > What we've been discussing so far is akin to your current setup, not > to the one you're planning to change to, isn't it? Are there any > problems with the new setup? The change above is functional because the CPU port ends up being egress untagged in all of the bridge's default_pvid, whether vlan_filtering is 0 or 1 and that works. The slightly confusing part is that a vlan_filtering=1 bridge accepts the default_pvid either tagged or untagged whereas a vlan_filtering=0 bridge does not, except for DHCP for instance. I would have to add a br0.1 802.1Q upper to take care of the default_pvid being egress tagged on the CPU port. We could solve this in the DSA receive path, or the Broadcom tag receive path as you say since that is dependent on the tagging format and switch properties. With Broadcom tags enabled now, all is well since we can differentiate traffic from different source ports using that 4 bytes tag. Where this broke was when using a 802.1Q separation because all frames that egressed the CPU were egress tagged and it was no longer possible to differentiate whether they came from the LAN group in VID 1 or the WAN group in VID 2. But all of this should be a thing of the past now, ok, all is clear again now. Thanks!
On Thu, Sep 10, 2020 at 08:09:19PM -0700, Florian Fainelli wrote: > On 9/10/2020 5:03 PM, Vladimir Oltean wrote: > > On Thu, Sep 10, 2020 at 02:58:04PM -0700, Florian Fainelli wrote: > > > On 9/9/2020 11:34 AM, Florian Fainelli wrote: > > > > On 9/9/2020 10:53 AM, Vladimir Oltean wrote: > > > > > On Wed, Sep 09, 2020 at 10:22:42AM -0700, Florian Fainelli wrote: > > > > > > How do you make sure that the CPU port sees the frame untagged > > > > > > which would > > > > > > be necessary for a VLAN-unaware bridge? Do you have a special remapping > > > > > > rule? > > > > > > > > > > No, I don't have any remapping rules that would be relevant here. > > > > > Why would the frames need to be necessarily untagged for a VLAN-unaware > > > > > bridge, why is it a problem if they aren't? > > > > > > > > > > bool br_allowed_ingress(const struct net_bridge *br, > > > > > struct net_bridge_vlan_group *vg, struct sk_buff *skb, > > > > > u16 *vid, u8 *state) > > > > > { > > > > > /* If VLAN filtering is disabled on the bridge, all packets are > > > > > * permitted. > > > > > */ > > > > > if (!br_opt_get(br, BROPT_VLAN_ENABLED)) { > > > > > BR_INPUT_SKB_CB(skb)->vlan_filtered = false; > > > > > return true; > > > > > } > > > > > > > > > > return __allowed_ingress(br, vg, skb, vid, state); > > > > > } > > > > > > > > > > If I have a VLAN on a bridged switch port where the bridge is not > > > > > filtering, I have an 8021q upper of the bridge with that VLAN ID. > > > > > > > > Yes that is the key right there, you need an 8021q upper to pop the VLAN > > > > ID or push it, that is another thing that users need to be aware of > > > > which is a bit awkward, most expect things to just work. Maybe we should > > > > just refuse to have bridge devices that are not VLAN-aware, because this > > > > is just too cumbersome to deal with. > > > > > > With the drivers that you currently maintain and with the CPU port being > > > always tagged in the VLANs added to the user-facing ports, when you are > > > using a non-VLAN aware bridge, do you systematically add an br0.1 upper > > > 802.1Q device to pop/push the VLAN tag? > > > > Talking to you, I realized that I confused you uselessly. But in doing > > that, I actually cleared up a couple of things for myself. So thanks, I > > guess? > > > > This is actually a great question, and it gave me the opportunity to > > reflect. So, only 1 driver that I maintain has the logic of always > > marking the CPU port as egress-tagged. And that would be ocelot/felix. > > > > I need to give you a bit of background. > > The DSA mode of Ocelot switches is more of an afterthought, and I am > > saying this because there is a distinction I need to make between the > > "CPU port module" (which is a set of queues that the CPU can inject and > > extract frames from), and the "NPI port" (which is an operating mode, > > where a regular front-panel Ethernet port is connected internally to the > > CPU port module and injection/extraction I/O can therefore be done via > > Ethernet, and that's your DSA). > > Basically, when the NPI mode is in use, then it behaves less like an > > Ethernet port, and more like a set of CPU queues that connect over > > Ethernet, if that makes sense. > > SYSTEMPORT + bcm_sf2 act a lot like that, too, except the CPU port still > obeys VLAN, buffering, classification and other switch internal rules, but > essentially we want to map queues from the user-facing port to DMAs used by > the host processor. Digressing a lot here, but the NPI port of Ocelot switches really isn't like that. For example, the NPI port doesn't even need to be in the reachability domain for a frame to reach it. Other example, a TCAM rule to drop a frame won't prevent it from reaching the NPI port, if that was previously selected as a destination for that frame. Other example: there is no source address learning for traffic injected by the network stack over the NPI port. So, on RX, every frame that should reach the CPU is actually _flooded_, due to the destination being unknown. Other example: the NPI port is so hardcoded to wrap everything in an Extraction Frame Header, that it even wraps PAUSE frames in it. That one especially is so bad that I have a patch series in the works to simply disable the NPI port and use tag_8021q instead. I just hate it. > > > The port settings for VLAN are bypassed, and the packet is copied as-is > > from ingress to the NPI port. The egress-tagged port VLAN configuration > > does not actually result in a VLAN header being pushed into the frame, > > if that egress port is the NPI port. Instead, the classified VLAN ID > > (i.e. derived from the packet, or from the port-based VLAN, or from > > custom VLAN classification TCAM rules) is always kept in a 12-bit field > > of the Extraction Frame Header. > > > > Currently I am ignoring the classified VLAN from the Extraction Frame > > Header, and simply passing the skb as-is to the stack. As-is, meaning as > > the switch ingress port had received it. So, in retrospect, my patch > > 183be6f967fe ("net: dsa: felix: send VLANs on CPU port as > > egress-tagged") is nothing more than a formality to make this piece of > > code shut up and not error out: > > > > static int ocelot_port_set_native_vlan(struct ocelot *ocelot, int port, > > u16 vid) > > { > > struct ocelot_port *ocelot_port = ocelot->ports[port]; > > u32 val = 0; > > > > if (ocelot_port->vid != vid) { > > /* Always permit deleting the native VLAN (vid = 0) */ > > if (ocelot_port->vid && vid) { > > dev_err(ocelot->dev, > > "Port already has a native VLAN: %d\n", > > ocelot_port->vid); > > return -EBUSY; > > } > > ocelot_port->vid = vid; > > } > > > > It's just now that I connected the dots and realized that. > > > > So, looks like I don't really know what it's like to always have a > > tagged skb on ingress, even for egress-tagged VLANs. It must suck, I > > guess? > > > > I think if I were in that situation, and the source port would be under > > a vlan_filtering=0 bridge, then I would simply pop the tag from the skb > > in the DSA rcv function, for all VLANs that I don't have an 8021q upper > > for. > > > > Explaining this, it makes a lot of sense to do what Vitesse / Microsemi > > / Microchip is doing, which is to copy the frame as-is to the CPU, and > > to also tell you, separately, what the classified VLAN is. For example, > > in vlan_filtering=0 mode, the classified VLAN will always be 1, > > regardless of how the frame is tagged, because VLAN awareness is truly > > turned off for the ingress port, and the port-based VLAN is always used. > > In this way, you have the most flexibility: you can either ignore the > > classified VLAN and proceed with just what was in the ingress skb (this > > way, you'll have a switch that is not VLAN-unaware, just "checking" as > > opposed to "secure". It has passed the ingress VLAN filter, but you > > still remember what the VLAN ID was. > > Or you can have a completely VLAN-unaware switch, if you pop all VLANs > > that you can find in the skb, and add a hwaccel tag based on the > > classified VLAN, if it isn't equal to the pvid of the port. This is > > great for things like compatibility with a vlan_filtering=0 upper bridge > > which is what we're talking about. > > > > Basically, this is what, I think, DSA tries to emulate with the rule of > > copying the flags of a user port VLAN to the CPU port too. If we had the > > API to request an "unmodified" VLAN (not egress-tagged, not > > egress-untagged, just the same as on ingress), I'm sure we'd be using > > that by default (useful when vlan_filtering is 1). Knowing what the > > classified VLAN was also can be very useful at times (like when > > vlan_filtering is 0), so if there was an API for that, I'm sure DSA > > would have used that as well. With no such APIs, we can only use > > approximations. > > egress unmodified is what mv88e6xxx uses which is why I do not believe they > have had the same issues that I had with vlan_filtering=0. For Broadcom > switches there is not any option to have an umodified mode, the CPU port > must have a valid VLAN membership (with vlan_filtering=1) and the egress > untagged from the CPU port to the Ethernet MAC must match the expectations > of the software data path behind. > So excepting mv88e6xxx and ocelot/felix, you are really in the same situation now with b53 and starfighter as everybody else is, am I not right? The "pvid and not untagged" VLAN is problematic for everybody in vlan_filtering=0 mode. > > > > > I am about ready to submit the changes we discussed to b53, but I am still a > > > bit uncomfortable with this part of the change because it will make the CPU > > > port follow the untagged attribute of an user-facing port. > > > > > > @@ -1444,7 +1427,7 @@ void b53_vlan_add(struct dsa_switch *ds, int port, > > > untagged = true; > > > > > > vl->members |= BIT(port); > > > - if (untagged && !dsa_is_cpu_port(ds, port)) > > > + if (untagged) > > > vl->untag |= BIT(port); > > > else > > > vl->untag &= ~BIT(port); > > > @@ -1482,7 +1465,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port, > > > if (pvid == vid) > > > pvid = b53_default_pvid(dev); > > > > > > - if (untagged && !dsa_is_cpu_port(ds, port)) > > > + if (untagged) > > > vl->untag &= ~(BIT(port)); > > > > > > > Which is ok, I believe? I mean, that's the default DSA logic. If you > > think that isn't ok, we should change it at a more fundamental level. > > What we've been discussing so far is akin to your current setup, not > > to the one you're planning to change to, isn't it? Are there any > > problems with the new setup? > > The change above is functional because the CPU port ends up being egress > untagged in all of the bridge's default_pvid, whether vlan_filtering is 0 or > 1 and that works. Yeah, heard you about one mistake cancelling another one out. > The slightly confusing part is that a vlan_filtering=1 bridge accepts the > default_pvid either tagged or untagged whereas a vlan_filtering=0 bridge > does not, except for DHCP for instance. I would have to add a br0.1 802.1Q > upper to take care of the default_pvid being egress tagged on the CPU port. > > We could solve this in the DSA receive path, or the Broadcom tag receive > path as you say since that is dependent on the tagging format and switch > properties. > > With Broadcom tags enabled now, all is well since we can differentiate > traffic from different source ports using that 4 bytes tag. > > Where this broke was when using a 802.1Q separation because all frames that > egressed the CPU were egress tagged and it was no longer possible to > differentiate whether they came from the LAN group in VID 1 or the WAN group > in VID 2. But all of this should be a thing of the past now, ok, all is > clear again now. Or we could do this, what do you think? From 178a46f0f96555e17f3fcefa356e324a92dafab2 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean <olteanv@gmail.com> Date: Fri, 11 Sep 2020 18:16:48 +0300 Subject: [PATCH] net: bridge: pop vlan from skb if filtering is disabled but it's a pvid Currently the bridge untags VLANs from its VLAN group in __allowed_ingress() only when VLAN filtering is enabled. When installing a pvid in egress-tagged mode: ip link add dev br0 type bridge vlan_filtering 0 ip link set swp0 master br0 bridge vlan del dev swp0 vid 1 bridge vlan add dev swp0 vid 1 pvid When adding a VLAN on a DSA switch interface, DSA configures the VLAN membership of the CPU port using the same flags as swp0 (in this case "pvid and not untagged"), in an attempt to copy the frame as-is from ingress to the CPU. However, in this case, the packet may arrive untagged on ingress, it will be pvid-tagged by the ingress port, and will be sent as egress-tagged towards the CPU. Otherwise stated, the CPU will see a VLAN tag where there was none to speak of on ingress. When vlan_filtering is 1, this is not a problem, as stated in the first paragraph, because __allowed_ingress() will pop it. But currently, when vlan_filtering is 0 and we have such a VLAN configuration, we need an 8021q upper (br0.1) to be able to ping over that VLAN. Make the 2 cases (vlan_filtering 0 and 1) behave the same way as popping the pvid, if the skb happens to be tagged with it, when vlan_filtering is 0. Signed-off-by: Vladimir Oltean <olteanv@gmail.com> --- net/bridge/br_vlan.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c index d2b8737f9fc0..b1e7211bae51 100644 --- a/net/bridge/br_vlan.c +++ b/net/bridge/br_vlan.c @@ -580,7 +580,23 @@ bool br_allowed_ingress(const struct net_bridge *br, * permitted. */ if (!br_opt_get(br, BROPT_VLAN_ENABLED)) { + u16 vid; + BR_INPUT_SKB_CB(skb)->vlan_filtered = false; + + /* See comment in __allowed_ingress about how skb can end up + * here not having a hwaccel tag + */ + if (unlikely(!skb_vlan_tag_present(skb) && + skb->protocol == br->vlan_proto)) { + skb = skb_vlan_untag(skb); + if (unlikely(!skb)) + return false; + } + + if (!br_vlan_get_tag(skb, &vid) && vid == br_get_pvid(vg)) + __vlan_hwaccel_clear_tag(skb); + return true; }
Hi Vladimir, On Tue Sep 08 2020, Vladimir Oltean wrote: > On Tue, Sep 08, 2020 at 12:14:12PM +0200, Kurt Kanzenbach wrote: >> On Mon Sep 07 2020, Vladimir Oltean wrote: >> > New drivers always seem to omit setting this flag, for some reason. >> >> Yes, because it's not well documented, or is it? Before writing the >> hellcreek DSA driver, I've read dsa.rst documentation to find out what >> callback function should to what. Did I miss something? > > Honestly, Documentation/networking/dsa/dsa.rst is out of date by quite a > bit. And this trend of having boolean flags in struct dsa_switch started > after the documentation stopped being updated. Maybe it would be good to document new flags when they're introduced :). > > But I didn't say it's your fault for not setting the flag, it is easy to > miss, and that's what this patch is trying to improve. > >> > So let's reverse the logic: the DSA core sets it by default to true >> > before the .setup() callback, and legacy drivers can turn it off. This >> > way, new drivers get the new behavior by default, unless they >> > explicitly set the flag to false, which is more obvious during review. >> >> Yeah, that behavior makes more sense to me. Thank you. > > Ok, thanks. Is this merged? I don't see it. Do I have to set `configure_vlan_while_not_filtering' explicitly to true for the next hellcreek version? Thanks, Kurt
On Fri, Oct 02, 2020 at 10:06:28AM +0200, Kurt Kanzenbach wrote: > Hi Vladimir, > > On Tue Sep 08 2020, Vladimir Oltean wrote: > > On Tue, Sep 08, 2020 at 12:14:12PM +0200, Kurt Kanzenbach wrote: > >> On Mon Sep 07 2020, Vladimir Oltean wrote: > >> > New drivers always seem to omit setting this flag, for some reason. > >> > >> Yes, because it's not well documented, or is it? Before writing the > >> hellcreek DSA driver, I've read dsa.rst documentation to find out what > >> callback function should to what. Did I miss something? > > > > Honestly, Documentation/networking/dsa/dsa.rst is out of date by quite a > > bit. And this trend of having boolean flags in struct dsa_switch started > > after the documentation stopped being updated. > > Maybe it would be good to document new flags when they're introduced :). Yup, will definitely do that when I resend. > > > > But I didn't say it's your fault for not setting the flag, it is easy to > > miss, and that's what this patch is trying to improve. > > > >> > So let's reverse the logic: the DSA core sets it by default to true > >> > before the .setup() callback, and legacy drivers can turn it off. This > >> > way, new drivers get the new behavior by default, unless they > >> > explicitly set the flag to false, which is more obvious during review. > >> > >> Yeah, that behavior makes more sense to me. Thank you. > > > > Ok, thanks. > > Is this merged? I don't see it. Do I have to set > `configure_vlan_while_not_filtering' explicitly to true for the next > hellcreek version? Yes, please set it to true. The refactoring change didn't get merged in time, I don't want it to interfere with your series. Thanks, -Vladimir
Hi Kurt, On Fri, Oct 02, 2020 at 11:15:27AM +0300, Vladimir Oltean wrote: > > Is this merged? I don't see it. Do I have to set > > `configure_vlan_while_not_filtering' explicitly to true for the next > > hellcreek version? > > Yes, please set it to true. The refactoring change didn't get merged in > time, I don't want it to interfere with your series. Do you plan on resending hellcreek for 5.10? Thanks, -Vladimir
Hi Vladimir, On Sat Oct 03 2020, Vladimir Oltean wrote: > Hi Kurt, > > On Fri, Oct 02, 2020 at 11:15:27AM +0300, Vladimir Oltean wrote: >> > Is this merged? I don't see it. Do I have to set >> > `configure_vlan_while_not_filtering' explicitly to true for the next >> > hellcreek version? >> >> Yes, please set it to true. The refactoring change didn't get merged in >> time, I don't want it to interfere with your series. > > Do you plan on resending hellcreek for 5.10? I've implemented the configure_vlan_while_not_filtering behaviour yesterday with caching and lists like the sja driver does. It needs some testing and then I'll post it. Maybe next week. Thanks, Kurt
On Sat, Oct 03, 2020 at 11:45:27AM +0200, Kurt Kanzenbach wrote:
> Maybe next week.
The merge window opens next week. This means you can still resend as
RFC, but it can only be accepted in net-next after ~2 weeks.
Thanks,
-Vladimir
On Sun, Oct 04, 2020 at 01:56:17PM +0300, Vladimir Oltean wrote: > On Sat, Oct 03, 2020 at 11:45:27AM +0200, Kurt Kanzenbach wrote: > > Maybe next week. > > The merge window opens next week. This means you can still resend as > RFC, but it can only be accepted in net-next after ~2 weeks. False alarm, looks like we have an rc8. I guess that means you can resend some time this week. Thanks, -Vladimir
diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c index 26fcff85d881..d127cdda16e8 100644 --- a/drivers/net/dsa/b53/b53_common.c +++ b/drivers/net/dsa/b53/b53_common.c @@ -1011,6 +1011,7 @@ static int b53_setup(struct dsa_switch *ds) * devices. (not hardware supported) */ ds->vlan_filtering_is_global = true; + ds->configure_vlan_while_not_filtering = false; return ret; } diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c index 3263e8a0ae67..f9587a73fe54 100644 --- a/drivers/net/dsa/bcm_sf2.c +++ b/drivers/net/dsa/bcm_sf2.c @@ -1242,6 +1242,7 @@ static int bcm_sf2_sw_probe(struct platform_device *pdev) /* Advertise the 8 egress queues */ ds->num_tx_queues = SF2_NUM_EGRESS_QUEUES; + ds->configure_vlan_while_not_filtering = false; dev_set_drvdata(&pdev->dev, priv); diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c index b588614d1e5e..65b5c1a50282 100644 --- a/drivers/net/dsa/dsa_loop.c +++ b/drivers/net/dsa/dsa_loop.c @@ -343,7 +343,6 @@ static int dsa_loop_drv_probe(struct mdio_device *mdiodev) ds->dev = &mdiodev->dev; ds->ops = &dsa_loop_driver; ds->priv = ps; - ds->configure_vlan_while_not_filtering = true; ps->bus = mdiodev->bus; dev_set_drvdata(&mdiodev->dev, ds); diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c index 521ebc072903..8622d836cbd3 100644 --- a/drivers/net/dsa/lantiq_gswip.c +++ b/drivers/net/dsa/lantiq_gswip.c @@ -837,6 +837,9 @@ static int gswip_setup(struct dsa_switch *ds) } gswip_port_enable(ds, cpu_port, NULL); + + ds->configure_vlan_while_not_filtering = false; + return 0; } diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c index 8f1d15ea15d9..03d65dc5a304 100644 --- a/drivers/net/dsa/microchip/ksz8795.c +++ b/drivers/net/dsa/microchip/ksz8795.c @@ -1087,6 +1087,8 @@ static int ksz8795_setup(struct dsa_switch *ds) ksz_init_mib_timer(dev); + ds->configure_vlan_while_not_filtering = false; + return 0; } diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c index 3cb22d149813..fea265ca6f82 100644 --- a/drivers/net/dsa/microchip/ksz9477.c +++ b/drivers/net/dsa/microchip/ksz9477.c @@ -1357,6 +1357,8 @@ static int ksz9477_setup(struct dsa_switch *ds) ksz_init_mib_timer(dev); + ds->configure_vlan_while_not_filtering = false; + return 0; } diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 1aaf47a0da2b..4698e740f8fc 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -1220,7 +1220,6 @@ mt7530_setup(struct dsa_switch *ds) * as two netdev instances. */ dn = dsa_to_port(ds, MT7530_CPU_PORT)->master->dev.of_node->parent; - ds->configure_vlan_while_not_filtering = true; if (priv->id == ID_MT7530) { regulator_set_voltage(priv->core_pwr, 1000000, 1000000); diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c index 15b97a4f8d93..6948c6980092 100644 --- a/drivers/net/dsa/mv88e6xxx/chip.c +++ b/drivers/net/dsa/mv88e6xxx/chip.c @@ -3090,6 +3090,7 @@ static int mv88e6xxx_setup(struct dsa_switch *ds) chip->ds = ds; ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip); + ds->configure_vlan_while_not_filtering = false; mv88e6xxx_reg_lock(chip); diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index a1e1d3824110..2032c046a056 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -612,7 +612,6 @@ static int felix_setup(struct dsa_switch *ds) ANA_FLOODING, tc); ds->mtu_enforcement_ingress = true; - ds->configure_vlan_while_not_filtering = true; return 0; } diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c index e24a99031b80..20ac64219df2 100644 --- a/drivers/net/dsa/qca/ar9331.c +++ b/drivers/net/dsa/qca/ar9331.c @@ -328,6 +328,8 @@ static int ar9331_sw_setup(struct dsa_switch *ds) if (ret) goto error; + ds->configure_vlan_while_not_filtering = false; + return 0; error: dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret); diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c index f1e484477e35..e05b9cc39231 100644 --- a/drivers/net/dsa/qca8k.c +++ b/drivers/net/dsa/qca8k.c @@ -1442,7 +1442,6 @@ qca8k_sw_probe(struct mdio_device *mdiodev) priv->ds->dev = &mdiodev->dev; priv->ds->num_ports = QCA8K_NUM_PORTS; - priv->ds->configure_vlan_while_not_filtering = true; priv->ds->priv = priv; priv->ops = qca8k_switch_ops; priv->ds->ops = &priv->ops; diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c index f763f93f600f..c518ab49b968 100644 --- a/drivers/net/dsa/rtl8366rb.c +++ b/drivers/net/dsa/rtl8366rb.c @@ -958,6 +958,8 @@ static int rtl8366rb_setup(struct dsa_switch *ds) return -ENODEV; } + ds->configure_vlan_while_not_filtering = false; + return 0; } diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c index 045077252799..e2cee36f578f 100644 --- a/drivers/net/dsa/sja1105/sja1105_main.c +++ b/drivers/net/dsa/sja1105/sja1105_main.c @@ -3047,8 +3047,6 @@ static int sja1105_setup(struct dsa_switch *ds) ds->mtu_enforcement_ingress = true; - ds->configure_vlan_while_not_filtering = true; - rc = sja1105_setup_devlink_params(ds); if (rc < 0) return rc; diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index c0ffc7a2b65f..4a5e2832009b 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -414,6 +414,8 @@ static int dsa_switch_setup(struct dsa_switch *ds) if (err) goto unregister_devlink; + ds->configure_vlan_while_not_filtering = true; + err = ds->ops->setup(ds); if (err < 0) goto unregister_notifier; diff --git a/net/dsa/slave.c b/net/dsa/slave.c index e429c71df854..e0c86e97bb78 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -314,11 +314,14 @@ static int dsa_slave_vlan_add(struct net_device *dev, if (obj->orig_dev != dev) return -EOPNOTSUPP; - if (dsa_port_skip_vlan_configuration(dp)) - return 0; - vlan = *SWITCHDEV_OBJ_PORT_VLAN(obj); + if (dsa_port_skip_vlan_configuration(dp)) { + netdev_warn(dev, "skipping configuration of VLAN %d-%d\n", + vlan.vid_begin, vlan.vid_end); + return 0; + } + err = dsa_port_vlan_add(dp, &vlan, trans); if (err) return err; @@ -377,17 +380,23 @@ static int dsa_slave_vlan_del(struct net_device *dev, const struct switchdev_obj *obj) { struct dsa_port *dp = dsa_slave_to_port(dev); + struct switchdev_obj_port_vlan *vlan; if (obj->orig_dev != dev) return -EOPNOTSUPP; - if (dsa_port_skip_vlan_configuration(dp)) + vlan = SWITCHDEV_OBJ_PORT_VLAN(obj); + + if (dsa_port_skip_vlan_configuration(dp)) { + netdev_warn(dev, "skipping deletion of VLAN %d-%d\n", + vlan->vid_begin, vlan->vid_end); return 0; + } /* Do not deprogram the CPU port as it may be shared with other user * ports which can be members of this VLAN as well. */ - return dsa_port_vlan_del(dp, SWITCHDEV_OBJ_PORT_VLAN(obj)); + return dsa_port_vlan_del(dp, vlan); } static int dsa_slave_port_obj_del(struct net_device *dev, @@ -1248,8 +1257,11 @@ 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 (dsa_port_skip_vlan_configuration(dp)) + if (dsa_port_skip_vlan_configuration(dp)) { + netdev_warn(dev, "skipping configuration of VLAN %d\n", + vid); return 0; + } /* br_vlan_get_info() returns -EINVAL or -ENOENT if the * device, respectively the VID is not found, returning @@ -1302,8 +1314,11 @@ 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 (dsa_port_skip_vlan_configuration(dp)) + if (dsa_port_skip_vlan_configuration(dp)) { + netdev_warn(dev, "skipping deletion of VLAN %d\n", + vid); return 0; + } /* br_vlan_get_info() returns -EINVAL or -ENOENT if the * device, respectively the VID is not found, returning