Re: [PATCH net-next 4/4] net: dsa: set configure_vlan_while_not_filtering to true by default

[Vladimir Oltean <olteanv@gmail.com>]

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