Message ID | b6ec9450-6b3e-0473-a2f9-b57016f010c1@gmail.com |
---|---|
State | RFC |
Delegated to: | David Miller |
Headers | show |
Series | None | expand |
On Fri, Sep 11, 2020 at 11:23:28AM -0700, Florian Fainelli wrote: > On 9/11/2020 8:43 AM, Vladimir Oltean wrote: > > > 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? > > Yes, this would be working, and I just tested it with the following delta on > top of my b53 patch: > > diff --git a/drivers/net/dsa/b53/b53_common.c > b/drivers/net/dsa/b53/b53_common.c > index 46ac8875f870..73507cff3bc4 100644 > --- a/drivers/net/dsa/b53/b53_common.c > +++ b/drivers/net/dsa/b53/b53_common.c > @@ -1427,7 +1427,7 @@ void b53_vlan_add(struct dsa_switch *ds, int port, > untagged = true; > > vl->members |= BIT(port); > - if (untagged) > + if (untagged && !dsa_is_cpu_port(ds, port)) > vl->untag |= BIT(port); > else > vl->untag &= ~BIT(port); > @@ -1465,7 +1465,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port, > if (pvid == vid) > pvid = b53_default_pvid(dev); > > - if (untagged) > + if (untagged && !dsa_is_cpu_port(ds, port)) > vl->untag &= ~(BIT(port)); > > b53_set_vlan_entry(dev, vid, vl); > > and it works, thanks! > I'm conflicted. So you prefer having the CPU port as egress-tagged? Also, I think I'll also experiment with a version of this patch that is local to the DSA RX path. The bridge people may not like it, and as far as I understand, only DSA has this situation where pvid-tagged traffic may end up with a vlan tag on ingress. Thanks, -Vladimir
On 9/11/2020 11:35 AM, Vladimir Oltean wrote: > On Fri, Sep 11, 2020 at 11:23:28AM -0700, Florian Fainelli wrote: >> On 9/11/2020 8:43 AM, Vladimir Oltean wrote: >>>> 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? >> >> Yes, this would be working, and I just tested it with the following delta on >> top of my b53 patch: >> >> diff --git a/drivers/net/dsa/b53/b53_common.c >> b/drivers/net/dsa/b53/b53_common.c >> index 46ac8875f870..73507cff3bc4 100644 >> --- a/drivers/net/dsa/b53/b53_common.c >> +++ b/drivers/net/dsa/b53/b53_common.c >> @@ -1427,7 +1427,7 @@ void b53_vlan_add(struct dsa_switch *ds, int port, >> untagged = true; >> >> vl->members |= BIT(port); >> - if (untagged) >> + if (untagged && !dsa_is_cpu_port(ds, port)) >> vl->untag |= BIT(port); >> else >> vl->untag &= ~BIT(port); >> @@ -1465,7 +1465,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port, >> if (pvid == vid) >> pvid = b53_default_pvid(dev); >> >> - if (untagged) >> + if (untagged && !dsa_is_cpu_port(ds, port)) >> vl->untag &= ~(BIT(port)); >> >> b53_set_vlan_entry(dev, vid, vl); >> >> and it works, thanks! >> > > I'm conflicted. So you prefer having the CPU port as egress-tagged? I do, because I realized that some of the switches we support are still configured in DSA_TAG_NONE mode because the CPU port they chose is not Broadcom tag capable and there is an user out there who cares a lot about that case not to "break". > > Also, I think I'll also experiment with a version of this patch that is > local to the DSA RX path. The bridge people may not like it, and as far > as I understand, only DSA has this situation where pvid-tagged traffic > may end up with a vlan tag on ingress. OK so something along the lines of: port is bridged, and bridge has vlan_filtering=0 and switch does egress tagging and VID is bridge's default_pvid then pop the tag? Should this be a helper function that is utilized by the relevant tagger drivers or do you want this in dsa_switch_rcv()?
On 9/11/2020 12:39 PM, Florian Fainelli wrote: > > > On 9/11/2020 11:35 AM, Vladimir Oltean wrote: >> On Fri, Sep 11, 2020 at 11:23:28AM -0700, Florian Fainelli wrote: >>> On 9/11/2020 8:43 AM, Vladimir Oltean wrote: >>>>> 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? >>> >>> Yes, this would be working, and I just tested it with the following >>> delta on >>> top of my b53 patch: >>> >>> diff --git a/drivers/net/dsa/b53/b53_common.c >>> b/drivers/net/dsa/b53/b53_common.c >>> index 46ac8875f870..73507cff3bc4 100644 >>> --- a/drivers/net/dsa/b53/b53_common.c >>> +++ b/drivers/net/dsa/b53/b53_common.c >>> @@ -1427,7 +1427,7 @@ void b53_vlan_add(struct dsa_switch *ds, int port, >>> untagged = true; >>> >>> vl->members |= BIT(port); >>> - if (untagged) >>> + if (untagged && !dsa_is_cpu_port(ds, port)) >>> vl->untag |= BIT(port); >>> else >>> vl->untag &= ~BIT(port); >>> @@ -1465,7 +1465,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port, >>> if (pvid == vid) >>> pvid = b53_default_pvid(dev); >>> >>> - if (untagged) >>> + if (untagged && !dsa_is_cpu_port(ds, port)) >>> vl->untag &= ~(BIT(port)); >>> >>> b53_set_vlan_entry(dev, vid, vl); >>> >>> and it works, thanks! >>> >> >> I'm conflicted. So you prefer having the CPU port as egress-tagged? > > I do, because I realized that some of the switches we support are still > configured in DSA_TAG_NONE mode because the CPU port they chose is not > Broadcom tag capable and there is an user out there who cares a lot > about that case not to "break". > >> >> Also, I think I'll also experiment with a version of this patch that is >> local to the DSA RX path. The bridge people may not like it, and as far >> as I understand, only DSA has this situation where pvid-tagged traffic >> may end up with a vlan tag on ingress. > > OK so something along the lines of: port is bridged, and bridge has > vlan_filtering=0 and switch does egress tagging and VID is bridge's > default_pvid then pop the tag? > > Should this be a helper function that is utilized by the relevant tagger > drivers or do you want this in dsa_switch_rcv()? The two drivers that appear to be untagging the CPU port unconditionally are b53 and kzs9477.
On Fri, Sep 11, 2020 at 12:48:37PM -0700, Florian Fainelli wrote: > > > I'm conflicted. So you prefer having the CPU port as egress-tagged? > > > > I do, because I realized that some of the switches we support are still > > configured in DSA_TAG_NONE mode because the CPU port they chose is not > > Broadcom tag capable and there is an user out there who cares a lot > > about that case not to "break". > > Ok. > > > Also, I think I'll also experiment with a version of this patch that is > > > local to the DSA RX path. The bridge people may not like it, and as far > > > as I understand, only DSA has this situation where pvid-tagged traffic > > > may end up with a vlan tag on ingress. > > > > OK so something along the lines of: port is bridged, and bridge has > > vlan_filtering=0 and switch does egress tagging and VID is bridge's > > default_pvid then pop the tag? > > > > Should this be a helper function that is utilized by the relevant tagger > > drivers or do you want this in dsa_switch_rcv()? > > The two drivers that appear to be untagging the CPU port unconditionally are > b53 and kzs9477. So, a helper in DSA would look something like this: diff --git a/include/net/dsa.h b/include/net/dsa.h index 75c8fac82017..c0bb978c6ff7 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -204,6 +204,7 @@ struct dsa_port { const char *mac; struct device_node *dn; unsigned int ageing_time; + int pvid; bool vlan_filtering; u8 stp_state; struct net_device *bridge_dev; diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index 4a5e2832009b..84d47f838b4e 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -273,6 +273,8 @@ static int dsa_port_setup(struct dsa_port *dp) if (dp->setup) return 0; + dp->pvid = -1; + switch (dp->type) { case DSA_PORT_TYPE_UNUSED: dsa_port_disable(dp); diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h index 2da656d984ef..d1dec232fc45 100644 --- a/net/dsa/dsa_priv.h +++ b/net/dsa/dsa_priv.h @@ -7,6 +7,7 @@ #ifndef __DSA_PRIV_H #define __DSA_PRIV_H +#include <linux/if_bridge.h> #include <linux/phy.h> #include <linux/netdevice.h> #include <linux/netpoll.h> @@ -194,6 +195,40 @@ dsa_slave_to_master(const struct net_device *dev) return dp->cpu_dp->master; } +/* If under a bridge with vlan_filtering=0, make sure to send pvid-tagged + * frames as untagged, since the bridge will not untag them. + */ +static inline struct sk_buff *dsa_untag_bridge_pvid(struct sk_buff *skb) +{ + struct dsa_port *dp = dsa_slave_to_port(skb->dev); + struct vlan_ethhdr *hdr = vlan_eth_hdr(skb); + struct net_device *br = dp->bridge_dev; + u16 proto; + int err; + + if (!br || br_vlan_enabled(br)) + return skb; + + err = br_vlan_get_proto(br, &proto); + if (err) + return skb; + + if (!skb_vlan_tag_present(skb) && hdr->h_vlan_proto == htons(proto)) { + skb = skb_vlan_untag(skb); + if (!skb) + return NULL; + } + + if (!skb_vlan_tag_present(skb)) + return skb; + + /* Cannot use br_vlan_get_pvid here as that requires RTNL */ + if (skb_vlan_tag_get_id(skb) == dp->pvid) + __vlan_hwaccel_clear_tag(skb); + + return skb; +} + /* switch.c */ int dsa_switch_register_notifier(struct dsa_switch *ds); void dsa_switch_unregister_notifier(struct dsa_switch *ds); diff --git a/net/dsa/switch.c b/net/dsa/switch.c index 86c8dc5c32a0..9167cc678f41 100644 --- a/net/dsa/switch.c +++ b/net/dsa/switch.c @@ -315,21 +315,45 @@ static int dsa_switch_vlan_add(struct dsa_switch *ds, if (!ds->ops->port_vlan_add) return 0; - for (port = 0; port < ds->num_ports; port++) - if (dsa_switch_vlan_match(ds, port, info)) + for (port = 0; port < ds->num_ports; port++) { + if (dsa_switch_vlan_match(ds, port, info)) { ds->ops->port_vlan_add(ds, port, info->vlan); + if (info->vlan->flags & BRIDGE_VLAN_INFO_PVID) { + struct dsa_port *dp = dsa_to_port(ds, port); + + dp->pvid = info->vlan->vid_end; + } + } + } + return 0; } static int dsa_switch_vlan_del(struct dsa_switch *ds, struct dsa_notifier_vlan_info *info) { + int err; + if (!ds->ops->port_vlan_del) return -EOPNOTSUPP; - if (ds->index == info->sw_index) - return ds->ops->port_vlan_del(ds, info->port, info->vlan); + if (ds->index == info->sw_index) { + const struct switchdev_obj_port_vlan *vlan = info->vlan; + struct dsa_port *dp = dsa_to_port(ds, info->port); + int vid; + + err = ds->ops->port_vlan_del(ds, info->port, info->vlan); + if (err) + return err; + + for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) { + if (vid == dp->pvid) { + dp->pvid = -1; + break; + } + } + } /* Do not deprogram the DSA links as they may be used as conduit * for other VLAN members in the fabric. It's quite a bit more complex, I don't like it. Thanks, -Vladimir
diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c index 46ac8875f870..73507cff3bc4 100644 --- a/drivers/net/dsa/b53/b53_common.c +++ b/drivers/net/dsa/b53/b53_common.c @@ -1427,7 +1427,7 @@ void b53_vlan_add(struct dsa_switch *ds, int port, untagged = true; vl->members |= BIT(port); - if (untagged) + if (untagged && !dsa_is_cpu_port(ds, port)) vl->untag |= BIT(port); else vl->untag &= ~BIT(port); @@ -1465,7 +1465,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port, if (pvid == vid) pvid = b53_default_pvid(dev); - if (untagged) + if (untagged && !dsa_is_cpu_port(ds, port)) vl->untag &= ~(BIT(port)); b53_set_vlan_entry(dev, vid, vl);