[PATCH net-next 0/4] Some VLAN handling cleanup in DSA

[PATCH net-next 0/4] Some VLAN handling cleanup in DSA

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

This small series tries to consolidate the VLAN handling in DSA a little
bit.

First, tag_8021q is reworked to be minimally invasive to the
dsa_switch_ops structure. This makes the rest of the code a bit easier
to follow.

Then, the configure_vlan_while_not_filtering flag is enabled by default,
so new drivers won't use the legacy behavior by mistake. This was done
after the recent conversation with Kurt Kanzenbach on the Hirschmann
Hellcreek review patches.

Vladimir Oltean (4):
  net: dsa: tag_8021q: include missing refcount.h
  net: dsa: tag_8021q: add a context structure
  Revert "net: dsa: Add more convenient functions for installing port
    VLANs"
  net: dsa: set configure_vlan_while_not_filtering to true by default

 drivers/net/dsa/b53/b53_common.c       |   1 +
 drivers/net/dsa/bcm_sf2.c              |   1 +
 drivers/net/dsa/dsa_loop.c             |   1 -
 drivers/net/dsa/lantiq_gswip.c         |   3 +
 drivers/net/dsa/microchip/ksz8795.c    |   2 +
 drivers/net/dsa/microchip/ksz9477.c    |   2 +
 drivers/net/dsa/mt7530.c               |   1 -
 drivers/net/dsa/mv88e6xxx/chip.c       |   1 +
 drivers/net/dsa/ocelot/felix.c         |   1 -
 drivers/net/dsa/qca/ar9331.c           |   2 +
 drivers/net/dsa/qca8k.c                |   1 -
 drivers/net/dsa/rtl8366rb.c            |   2 +
 drivers/net/dsa/sja1105/sja1105.h      |   3 +-
 drivers/net/dsa/sja1105/sja1105_main.c | 246 +++++++++++++++----------
 include/linux/dsa/8021q.h              |  49 ++---
 net/dsa/dsa2.c                         |   2 +
 net/dsa/dsa_priv.h                     |   2 -
 net/dsa/port.c                         |  33 ----
 net/dsa/slave.c                        |  63 ++++++-
 net/dsa/tag_8021q.c                    | 138 ++++++++------
 20 files changed, 319 insertions(+), 235 deletions(-)

-- 
2.25.1

[PATCH net-next 1/4] net: dsa: tag_8021q: include missing refcount.h

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The previous assumption was that the caller would already have this
header file included.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 include/linux/dsa/8021q.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/dsa/8021q.h b/include/linux/dsa/8021q.h
index 311aa04e7520..804750122c66 100644
--- a/include/linux/dsa/8021q.h
+++ b/include/linux/dsa/8021q.h
@@ -5,6 +5,7 @@
 #ifndef _NET_DSA_8021Q_H
 #define _NET_DSA_8021Q_H
 
+#include <linux/refcount.h>
 #include <linux/types.h>
 
 struct dsa_switch;
-- 
2.25.1

[PATCH net-next 2/4] net: dsa: tag_8021q: add a context structure

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

While working on another tag_8021q driver implementation, some things
became apparent:

- It is not mandatory for a DSA driver to offload the tag_8021q VLANs by
  using the VLAN table per se. For example, it can add custom TCAM rules
  that simply encapsulate RX traffic, and redirect & decapsulate rules
  for TX traffic. For such a driver, it makes no sense to receive the
  tag_8021q configuration through the same callback as it receives the
  VLAN configuration from the bridge and the 8021q modules.

- Currently, sja1105 (the only tag_8021q user) sets a
  priv->expect_dsa_8021q variable to distinguish between the bridge
  calling, and tag_8021q calling. That can be improved, to say the
  least.

- The crosschip bridging operations are, in fact, stateful already. The
  list of crosschip_links must be kept by the caller and passed to the
  relevant tag_8021q functions.

So it would be nice if the tag_8021q configuration was more
self-contained. This patch attempts to do that.

Create a struct dsa_8021q_context which encapsulates a struct
dsa_switch, and has 2 function pointers for adding and deleting a VLAN.
These will replace the previous channel to the driver, which was through
the .port_vlan_add and .port_vlan_del callbacks of dsa_switch_ops.

Also put the list of crosschip_links into this dsa_8021q_context.
Drivers that don't support cross-chip bridging can simply omit to
initialize this list, as long as they dont call any cross-chip function.

The rest of the patch is mostly plain refactoring of "ds" -> "ctx". The
dsa_8021q_context structure needs to be propagated because adding a VLAN
is now done through the ops function pointers inside of it.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/dsa/sja1105/sja1105.h      |   3 +-
 drivers/net/dsa/sja1105/sja1105_main.c | 244 +++++++++++++++----------
 include/linux/dsa/8021q.h              |  48 ++---
 net/dsa/tag_8021q.c                    | 138 +++++++-------
 4 files changed, 249 insertions(+), 184 deletions(-)

diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index ba70b40a9a95..40b492cb1454 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -210,14 +210,13 @@ struct sja1105_private {
 	struct dsa_switch *ds;
 	struct list_head dsa_8021q_vlans;
 	struct list_head bridge_vlans;
-	struct list_head crosschip_links;
 	struct sja1105_flow_block flow_block;
 	struct sja1105_port ports[SJA1105_NUM_PORTS];
 	/* Serializes transmission of management frames so that
 	 * the switch doesn't confuse them with one another.
 	 */
 	struct mutex mgmt_lock;
-	bool expect_dsa_8021q;
+	struct dsa_8021q_context dsa_8021q_ctx;
 	enum sja1105_vlan_state vlan_state;
 	struct sja1105_cbs_entry *cbs;
 	struct sja1105_tagger_data tagger_data;
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 5a28dfb36ec3..045077252799 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -1880,19 +1880,17 @@ static int sja1105_crosschip_bridge_join(struct dsa_switch *ds,
 		if (dsa_to_port(ds, port)->bridge_dev != br)
 			continue;
 
-		other_priv->expect_dsa_8021q = true;
-		rc = dsa_8021q_crosschip_bridge_join(ds, port, other_ds,
-						     other_port,
-						     &priv->crosschip_links);
-		other_priv->expect_dsa_8021q = false;
+		rc = dsa_8021q_crosschip_bridge_join(&priv->dsa_8021q_ctx,
+						     port,
+						     &other_priv->dsa_8021q_ctx,
+						     other_port);
 		if (rc)
 			return rc;
 
-		priv->expect_dsa_8021q = true;
-		rc = dsa_8021q_crosschip_bridge_join(other_ds, other_port, ds,
-						     port,
-						     &other_priv->crosschip_links);
-		priv->expect_dsa_8021q = false;
+		rc = dsa_8021q_crosschip_bridge_join(&other_priv->dsa_8021q_ctx,
+						     other_port,
+						     &priv->dsa_8021q_ctx,
+						     port);
 		if (rc)
 			return rc;
 	}
@@ -1919,33 +1917,24 @@ static void sja1105_crosschip_bridge_leave(struct dsa_switch *ds,
 		if (dsa_to_port(ds, port)->bridge_dev != br)
 			continue;
 
-		other_priv->expect_dsa_8021q = true;
-		dsa_8021q_crosschip_bridge_leave(ds, port, other_ds, other_port,
-						 &priv->crosschip_links);
-		other_priv->expect_dsa_8021q = false;
+		dsa_8021q_crosschip_bridge_leave(&priv->dsa_8021q_ctx, port,
+						 &other_priv->dsa_8021q_ctx,
+						 other_port);
 
-		priv->expect_dsa_8021q = true;
-		dsa_8021q_crosschip_bridge_leave(other_ds, other_port, ds, port,
-						 &other_priv->crosschip_links);
-		priv->expect_dsa_8021q = false;
+		dsa_8021q_crosschip_bridge_leave(&other_priv->dsa_8021q_ctx,
+						 other_port,
+						 &priv->dsa_8021q_ctx, port);
 	}
 }
 
 static int sja1105_setup_8021q_tagging(struct dsa_switch *ds, bool enabled)
 {
 	struct sja1105_private *priv = ds->priv;
-	int rc, i;
+	int rc;
 
-	for (i = 0; i < SJA1105_NUM_PORTS; i++) {
-		priv->expect_dsa_8021q = true;
-		rc = dsa_port_setup_8021q_tagging(ds, i, enabled);
-		priv->expect_dsa_8021q = false;
-		if (rc < 0) {
-			dev_err(ds->dev, "Failed to setup VLAN tagging for port %d: %d\n",
-				i, rc);
-			return rc;
-		}
-	}
+	rc = dsa_8021q_setup(&priv->dsa_8021q_ctx, enabled);
+	if (rc)
+		return rc;
 
 	dev_info(ds->dev, "%s switch tagging\n",
 		 enabled ? "Enabled" : "Disabled");
@@ -2149,12 +2138,12 @@ struct sja1105_crosschip_vlan {
 	bool untagged;
 	int port;
 	int other_port;
-	struct dsa_switch *other_ds;
+	struct dsa_8021q_context *other_ctx;
 };
 
 struct sja1105_crosschip_switch {
 	struct list_head list;
-	struct dsa_switch *other_ds;
+	struct dsa_8021q_context *other_ctx;
 };
 
 static int sja1105_commit_pvid(struct sja1105_private *priv)
@@ -2308,6 +2297,9 @@ static int sja1105_build_subvlans(struct sja1105_private *priv,
 	return 0;
 }
 
+#define ctx_to_sja1105(ctx) \
+		container_of((ctx), struct sja1105_private, dsa_8021q_ctx)
+
 /* Sadly, in crosschip scenarios where the CPU port is also the link to another
  * switch, we should retag backwards (the dsa_8021q vid to the original vid) on
  * the CPU port of neighbour switches.
@@ -2330,8 +2322,12 @@ sja1105_build_crosschip_subvlans(struct sja1105_private *priv,
 
 	INIT_LIST_HEAD(&crosschip_vlans);
 
-	list_for_each_entry(c, &priv->crosschip_links, list) {
-		struct sja1105_private *other_priv = c->other_ds->priv;
+	list_for_each_entry(c, &priv->dsa_8021q_ctx.crosschip_links, list) {
+		struct sja1105_private *other_priv;
+		struct dsa_switch *other_ds;
+
+		other_priv = ctx_to_sja1105(c->other_ctx);
+		other_ds = priv->ds;
 
 		if (other_priv->vlan_state == SJA1105_VLAN_FILTERING_FULL)
 			continue;
@@ -2341,7 +2337,7 @@ sja1105_build_crosschip_subvlans(struct sja1105_private *priv,
 		 */
 		if (!dsa_is_user_port(priv->ds, c->port))
 			continue;
-		if (!dsa_is_user_port(c->other_ds, c->other_port))
+		if (!dsa_is_user_port(other_ds, c->other_port))
 			continue;
 
 		/* Search for VLANs on the remote port */
@@ -2376,7 +2372,7 @@ sja1105_build_crosschip_subvlans(struct sja1105_private *priv,
 				    tmp->untagged == v->untagged &&
 				    tmp->port == c->port &&
 				    tmp->other_port == v->port &&
-				    tmp->other_ds == c->other_ds) {
+				    tmp->other_ctx == c->other_ctx) {
 					already_added = true;
 					break;
 				}
@@ -2394,18 +2390,22 @@ sja1105_build_crosschip_subvlans(struct sja1105_private *priv,
 			tmp->vid = v->vid;
 			tmp->port = c->port;
 			tmp->other_port = v->port;
-			tmp->other_ds = c->other_ds;
+			tmp->other_ctx = c->other_ctx;
 			tmp->untagged = v->untagged;
 			list_add(&tmp->list, &crosschip_vlans);
 		}
 	}
 
 	list_for_each_entry(tmp, &crosschip_vlans, list) {
-		struct sja1105_private *other_priv = tmp->other_ds->priv;
-		int upstream = dsa_upstream_port(priv->ds, tmp->port);
-		int match, subvlan;
+		struct sja1105_private *other_priv;
+		int upstream, match, subvlan;
+		struct dsa_switch *other_ds;
 		u16 rx_vid;
 
+		upstream = dsa_upstream_port(priv->ds, tmp->port);
+		other_priv = ctx_to_sja1105(tmp->other_ctx);
+		other_ds = other_priv->ds;
+
 		subvlan = sja1105_find_committed_subvlan(other_priv,
 							 tmp->other_port,
 							 tmp->vid);
@@ -2418,7 +2418,7 @@ sja1105_build_crosschip_subvlans(struct sja1105_private *priv,
 			goto out;
 		}
 
-		rx_vid = dsa_8021q_rx_vid_subvlan(tmp->other_ds,
+		rx_vid = dsa_8021q_rx_vid_subvlan(other_ds,
 						  tmp->other_port,
 						  subvlan);
 
@@ -2493,11 +2493,11 @@ static int sja1105_notify_crosschip_switches(struct sja1105_private *priv)
 
 	INIT_LIST_HEAD(&crosschip_switches);
 
-	list_for_each_entry(c, &priv->crosschip_links, list) {
+	list_for_each_entry(c, &priv->dsa_8021q_ctx.crosschip_links, list) {
 		bool already_added = false;
 
 		list_for_each_entry(s, &crosschip_switches, list) {
-			if (s->other_ds == c->other_ds) {
+			if (s->other_ctx == c->other_ctx) {
 				already_added = true;
 				break;
 			}
@@ -2512,12 +2512,14 @@ static int sja1105_notify_crosschip_switches(struct sja1105_private *priv)
 			rc = -ENOMEM;
 			goto out;
 		}
-		s->other_ds = c->other_ds;
+		s->other_ctx = c->other_ctx;
 		list_add(&s->list, &crosschip_switches);
 	}
 
 	list_for_each_entry(s, &crosschip_switches, list) {
-		struct sja1105_private *other_priv = s->other_ds->priv;
+		struct sja1105_private *other_priv;
+
+		other_priv = ctx_to_sja1105(s->other_ctx);
 
 		rc = sja1105_build_vlan_table(other_priv, false);
 		if (rc)
@@ -2618,16 +2620,6 @@ static int sja1105_build_vlan_table(struct sja1105_private *priv, bool notify)
 	return rc;
 }
 
-/* Select the list to which we should add this VLAN. */
-static struct list_head *sja1105_classify_vlan(struct sja1105_private *priv,
-					       u16 vid)
-{
-	if (priv->expect_dsa_8021q)
-		return &priv->dsa_8021q_vlans;
-
-	return &priv->bridge_vlans;
-}
-
 static int sja1105_vlan_prepare(struct dsa_switch *ds, int port,
 				const struct switchdev_obj_port_vlan *vlan)
 {
@@ -2642,7 +2634,7 @@ static int sja1105_vlan_prepare(struct dsa_switch *ds, int port,
 	 * configuration done by dsa_8021q.
 	 */
 	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
-		if (!priv->expect_dsa_8021q && vid_is_dsa_8021q(vid)) {
+		if (vid_is_dsa_8021q(vid)) {
 			dev_err(ds->dev, "Range 1024-3071 reserved for dsa_8021q operation\n");
 			return -EBUSY;
 		}
@@ -2762,6 +2754,60 @@ static int sja1105_vlan_filtering(struct dsa_switch *ds, int port, bool enabled)
 	return sja1105_setup_8021q_tagging(ds, want_tagging);
 }
 
+/* Returns number of VLANs added (0 or 1) on success,
+ * or a negative error code.
+ */
+static int sja1105_vlan_add_one(struct dsa_switch *ds, int port, u16 vid,
+				u16 flags, struct list_head *vlan_list)
+{
+	bool untagged = flags & BRIDGE_VLAN_INFO_UNTAGGED;
+	bool pvid = flags & BRIDGE_VLAN_INFO_PVID;
+	struct sja1105_bridge_vlan *v;
+	bool already_added = false;
+
+	list_for_each_entry(v, vlan_list, list) {
+		if (v->port == port && v->vid == vid &&
+		    v->untagged == untagged && v->pvid == pvid) {
+			already_added = true;
+			break;
+		}
+	}
+
+	if (already_added)
+		return 0;
+
+	v = kzalloc(sizeof(*v), GFP_KERNEL);
+	if (!v) {
+		dev_err(ds->dev, "Out of memory while storing VLAN\n");
+		return -ENOMEM;
+	}
+
+	v->port = port;
+	v->vid = vid;
+	v->untagged = untagged;
+	v->pvid = pvid;
+	list_add(&v->list, vlan_list);
+
+	return 1;
+}
+
+/* Returns number of VLANs deleted (0 or 1) */
+static int sja1105_vlan_del_one(struct dsa_switch *ds, int port, u16 vid,
+				struct list_head *vlan_list)
+{
+	struct sja1105_bridge_vlan *v, *n;
+
+	list_for_each_entry_safe(v, n, vlan_list, list) {
+		if (v->port == port && v->vid == vid) {
+			list_del(&v->list);
+			kfree(v);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
 static void sja1105_vlan_add(struct dsa_switch *ds, int port,
 			     const struct switchdev_obj_port_vlan *vlan)
 {
@@ -2771,38 +2817,12 @@ static void sja1105_vlan_add(struct dsa_switch *ds, int port,
 	int rc;
 
 	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
-		bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
-		bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
-		struct sja1105_bridge_vlan *v;
-		struct list_head *vlan_list;
-		bool already_added = false;
-
-		vlan_list = sja1105_classify_vlan(priv, vid);
-
-		list_for_each_entry(v, vlan_list, list) {
-			if (v->port == port && v->vid == vid &&
-			    v->untagged == untagged && v->pvid == pvid) {
-				already_added = true;
-				break;
-			}
-		}
-
-		if (already_added)
-			continue;
-
-		v = kzalloc(sizeof(*v), GFP_KERNEL);
-		if (!v) {
-			dev_err(ds->dev, "Out of memory while storing VLAN\n");
+		rc = sja1105_vlan_add_one(ds, port, vid, vlan->flags,
+					  &priv->bridge_vlans);
+		if (rc < 0)
 			return;
-		}
-
-		v->port = port;
-		v->vid = vid;
-		v->untagged = untagged;
-		v->pvid = pvid;
-		list_add(&v->list, vlan_list);
-
-		vlan_table_changed = true;
+		if (rc > 0)
+			vlan_table_changed = true;
 	}
 
 	if (!vlan_table_changed)
@@ -2819,21 +2839,12 @@ static int sja1105_vlan_del(struct dsa_switch *ds, int port,
 	struct sja1105_private *priv = ds->priv;
 	bool vlan_table_changed = false;
 	u16 vid;
+	int rc;
 
 	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
-		struct sja1105_bridge_vlan *v, *n;
-		struct list_head *vlan_list;
-
-		vlan_list = sja1105_classify_vlan(priv, vid);
-
-		list_for_each_entry_safe(v, n, vlan_list, list) {
-			if (v->port == port && v->vid == vid) {
-				list_del(&v->list);
-				kfree(v);
-				vlan_table_changed = true;
-				break;
-			}
-		}
+		rc = sja1105_vlan_del_one(ds, port, vid, &priv->bridge_vlans);
+		if (rc > 0)
+			vlan_table_changed = true;
 	}
 
 	if (!vlan_table_changed)
@@ -2842,6 +2853,36 @@ static int sja1105_vlan_del(struct dsa_switch *ds, int port,
 	return sja1105_build_vlan_table(priv, true);
 }
 
+static int sja1105_dsa_8021q_vlan_add(struct dsa_switch *ds, int port, u16 vid,
+				      u16 flags)
+{
+	struct sja1105_private *priv = ds->priv;
+	int rc;
+
+	rc = sja1105_vlan_add_one(ds, port, vid, flags, &priv->dsa_8021q_vlans);
+	if (rc <= 0)
+		return rc;
+
+	return sja1105_build_vlan_table(priv, true);
+}
+
+static int sja1105_dsa_8021q_vlan_del(struct dsa_switch *ds, int port, u16 vid)
+{
+	struct sja1105_private *priv = ds->priv;
+	int rc;
+
+	rc = sja1105_vlan_del_one(ds, port, vid, &priv->dsa_8021q_vlans);
+	if (!rc)
+		return 0;
+
+	return sja1105_build_vlan_table(priv, true);
+}
+
+static const struct dsa_8021q_ops sja1105_dsa_8021q_ops = {
+	.vlan_add	= sja1105_dsa_8021q_vlan_add,
+	.vlan_del	= sja1105_dsa_8021q_vlan_del,
+};
+
 static int sja1105_best_effort_vlan_filtering_get(struct sja1105_private *priv,
 						  bool *be_vlan)
 {
@@ -3504,7 +3545,10 @@ static int sja1105_probe(struct spi_device *spi)
 	mutex_init(&priv->ptp_data.lock);
 	mutex_init(&priv->mgmt_lock);
 
-	INIT_LIST_HEAD(&priv->crosschip_links);
+	priv->dsa_8021q_ctx.ops = &sja1105_dsa_8021q_ops;
+	priv->dsa_8021q_ctx.ds = ds;
+
+	INIT_LIST_HEAD(&priv->dsa_8021q_ctx.crosschip_links);
 	INIT_LIST_HEAD(&priv->bridge_vlans);
 	INIT_LIST_HEAD(&priv->dsa_8021q_vlans);
 
diff --git a/include/linux/dsa/8021q.h b/include/linux/dsa/8021q.h
index 804750122c66..2b003ae9fb38 100644
--- a/include/linux/dsa/8021q.h
+++ b/include/linux/dsa/8021q.h
@@ -12,31 +12,40 @@ struct dsa_switch;
 struct sk_buff;
 struct net_device;
 struct packet_type;
+struct dsa_8021q_context;
 
 struct dsa_8021q_crosschip_link {
 	struct list_head list;
 	int port;
-	struct dsa_switch *other_ds;
+	struct dsa_8021q_context *other_ctx;
 	int other_port;
 	refcount_t refcount;
 };
 
+struct dsa_8021q_ops {
+	int (*vlan_add)(struct dsa_switch *ds, int port, u16 vid, u16 flags);
+	int (*vlan_del)(struct dsa_switch *ds, int port, u16 vid);
+};
+
+struct dsa_8021q_context {
+	const struct dsa_8021q_ops *ops;
+	struct dsa_switch *ds;
+	struct list_head crosschip_links;
+};
+
 #define DSA_8021Q_N_SUBVLAN			8
 
 #if IS_ENABLED(CONFIG_NET_DSA_TAG_8021Q)
 
-int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int index,
-				 bool enabled);
+int dsa_8021q_setup(struct dsa_8021q_context *ctx, bool enabled);
 
-int dsa_8021q_crosschip_bridge_join(struct dsa_switch *ds, int port,
-				    struct dsa_switch *other_ds,
-				    int other_port,
-				    struct list_head *crosschip_links);
+int dsa_8021q_crosschip_bridge_join(struct dsa_8021q_context *ctx, int port,
+				    struct dsa_8021q_context *other_ctx,
+				    int other_port);
 
-int dsa_8021q_crosschip_bridge_leave(struct dsa_switch *ds, int port,
-				     struct dsa_switch *other_ds,
-				     int other_port,
-				     struct list_head *crosschip_links);
+int dsa_8021q_crosschip_bridge_leave(struct dsa_8021q_context *ctx, int port,
+				     struct dsa_8021q_context *other_ctx,
+				     int other_port);
 
 struct sk_buff *dsa_8021q_xmit(struct sk_buff *skb, struct net_device *netdev,
 			       u16 tpid, u16 tci);
@@ -57,24 +66,21 @@ bool vid_is_dsa_8021q(u16 vid);
 
 #else
 
-int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int index,
-				 bool enabled)
+int dsa_8021q_setup(struct dsa_8021q_context *ctx, bool enabled)
 {
 	return 0;
 }
 
-int dsa_8021q_crosschip_bridge_join(struct dsa_switch *ds, int port,
-				    struct dsa_switch *other_ds,
-				    int other_port,
-				    struct list_head *crosschip_links)
+int dsa_8021q_crosschip_bridge_join(struct dsa_8021q_context *ctx, int port,
+				    struct dsa_8021q_context *other_ctx,
+				    int other_port)
 {
 	return 0;
 }
 
-int dsa_8021q_crosschip_bridge_leave(struct dsa_switch *ds, int port,
-				     struct dsa_switch *other_ds,
-				     int other_port,
-				     struct list_head *crosschip_links)
+int dsa_8021q_crosschip_bridge_leave(struct dsa_8021q_context *ctx, int port,
+				     struct dsa_8021q_context *other_ctx,
+				     int other_port)
 {
 	return 0;
 }
diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
index 780b2a15ac9b..5baeb0893950 100644
--- a/net/dsa/tag_8021q.c
+++ b/net/dsa/tag_8021q.c
@@ -146,15 +146,15 @@ EXPORT_SYMBOL_GPL(vid_is_dsa_8021q);
  * user explicitly configured this @vid through the bridge core, then the @vid
  * is installed again, but this time with the flags from the bridge layer.
  */
-static int dsa_8021q_vid_apply(struct dsa_switch *ds, int port, u16 vid,
+static int dsa_8021q_vid_apply(struct dsa_8021q_context *ctx, int port, u16 vid,
 			       u16 flags, bool enabled)
 {
-	struct dsa_port *dp = dsa_to_port(ds, port);
+	struct dsa_port *dp = dsa_to_port(ctx->ds, port);
 
 	if (enabled)
-		return dsa_port_vid_add(dp, vid, flags);
+		return ctx->ops->vlan_add(ctx->ds, dp->index, vid, flags);
 
-	return dsa_port_vid_del(dp, vid);
+	return ctx->ops->vlan_del(ctx->ds, dp->index, vid);
 }
 
 /* RX VLAN tagging (left) and TX VLAN tagging (right) setup shown for a single
@@ -209,17 +209,18 @@ static int dsa_8021q_vid_apply(struct dsa_switch *ds, int port, u16 vid,
  * +-+-----+-+-----+-+-----+-+-----+-+    +-+-----+-+-----+-+-----+-+-----+-+
  *   swp0    swp1    swp2    swp3           swp0    swp1    swp2    swp3
  */
-int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
+static int dsa_8021q_setup_port(struct dsa_8021q_context *ctx, int port,
+				bool enabled)
 {
-	int upstream = dsa_upstream_port(ds, port);
-	u16 rx_vid = dsa_8021q_rx_vid(ds, port);
-	u16 tx_vid = dsa_8021q_tx_vid(ds, port);
+	int upstream = dsa_upstream_port(ctx->ds, port);
+	u16 rx_vid = dsa_8021q_rx_vid(ctx->ds, port);
+	u16 tx_vid = dsa_8021q_tx_vid(ctx->ds, port);
 	int i, err;
 
 	/* The CPU port is implicitly configured by
 	 * configuring the front-panel ports
 	 */
-	if (!dsa_is_user_port(ds, port))
+	if (!dsa_is_user_port(ctx->ds, port))
 		return 0;
 
 	/* Add this user port's RX VID to the membership list of all others
@@ -227,7 +228,7 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
 	 * L2 forwarding rules still take precedence when there are no VLAN
 	 * restrictions, so there are no concerns about leaking traffic.
 	 */
-	for (i = 0; i < ds->num_ports; i++) {
+	for (i = 0; i < ctx->ds->num_ports; i++) {
 		u16 flags;
 
 		if (i == upstream)
@@ -240,9 +241,10 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
 			/* The RX VID is a regular VLAN on all others */
 			flags = BRIDGE_VLAN_INFO_UNTAGGED;
 
-		err = dsa_8021q_vid_apply(ds, i, rx_vid, flags, enabled);
+		err = dsa_8021q_vid_apply(ctx, i, rx_vid, flags, enabled);
 		if (err) {
-			dev_err(ds->dev, "Failed to apply RX VID %d to port %d: %d\n",
+			dev_err(ctx->ds->dev,
+				"Failed to apply RX VID %d to port %d: %d\n",
 				rx_vid, port, err);
 			return err;
 		}
@@ -251,80 +253,100 @@ int dsa_port_setup_8021q_tagging(struct dsa_switch *ds, int port, bool enabled)
 	/* CPU port needs to see this port's RX VID
 	 * as tagged egress.
 	 */
-	err = dsa_8021q_vid_apply(ds, upstream, rx_vid, 0, enabled);
+	err = dsa_8021q_vid_apply(ctx, upstream, rx_vid, 0, enabled);
 	if (err) {
-		dev_err(ds->dev, "Failed to apply RX VID %d to port %d: %d\n",
+		dev_err(ctx->ds->dev,
+			"Failed to apply RX VID %d to port %d: %d\n",
 			rx_vid, port, err);
 		return err;
 	}
 
 	/* Finally apply the TX VID on this port and on the CPU port */
-	err = dsa_8021q_vid_apply(ds, port, tx_vid, BRIDGE_VLAN_INFO_UNTAGGED,
+	err = dsa_8021q_vid_apply(ctx, port, tx_vid, BRIDGE_VLAN_INFO_UNTAGGED,
 				  enabled);
 	if (err) {
-		dev_err(ds->dev, "Failed to apply TX VID %d on port %d: %d\n",
+		dev_err(ctx->ds->dev,
+			"Failed to apply TX VID %d on port %d: %d\n",
 			tx_vid, port, err);
 		return err;
 	}
-	err = dsa_8021q_vid_apply(ds, upstream, tx_vid, 0, enabled);
+	err = dsa_8021q_vid_apply(ctx, upstream, tx_vid, 0, enabled);
 	if (err) {
-		dev_err(ds->dev, "Failed to apply TX VID %d on port %d: %d\n",
+		dev_err(ctx->ds->dev,
+			"Failed to apply TX VID %d on port %d: %d\n",
 			tx_vid, upstream, err);
 		return err;
 	}
 
 	return err;
 }
-EXPORT_SYMBOL_GPL(dsa_port_setup_8021q_tagging);
 
-static int dsa_8021q_crosschip_link_apply(struct dsa_switch *ds, int port,
-					  struct dsa_switch *other_ds,
+int dsa_8021q_setup(struct dsa_8021q_context *ctx, bool enabled)
+{
+	int rc, port;
+
+	for (port = 0; port < ctx->ds->num_ports; port++) {
+		rc = dsa_8021q_setup_port(ctx, port, enabled);
+		if (rc < 0) {
+			dev_err(ctx->ds->dev,
+				"Failed to setup VLAN tagging for port %d: %d\n",
+				port, rc);
+			return rc;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dsa_8021q_setup);
+
+static int dsa_8021q_crosschip_link_apply(struct dsa_8021q_context *ctx,
+					  int port,
+					  struct dsa_8021q_context *other_ctx,
 					  int other_port, bool enabled)
 {
-	u16 rx_vid = dsa_8021q_rx_vid(ds, port);
+	u16 rx_vid = dsa_8021q_rx_vid(ctx->ds, port);
 
 	/* @rx_vid of local @ds port @port goes to @other_port of
 	 * @other_ds
 	 */
-	return dsa_8021q_vid_apply(other_ds, other_port, rx_vid,
+	return dsa_8021q_vid_apply(other_ctx, other_port, rx_vid,
 				   BRIDGE_VLAN_INFO_UNTAGGED, enabled);
 }
 
-static int dsa_8021q_crosschip_link_add(struct dsa_switch *ds, int port,
-					struct dsa_switch *other_ds,
-					int other_port,
-					struct list_head *crosschip_links)
+static int dsa_8021q_crosschip_link_add(struct dsa_8021q_context *ctx, int port,
+					struct dsa_8021q_context *other_ctx,
+					int other_port)
 {
 	struct dsa_8021q_crosschip_link *c;
 
-	list_for_each_entry(c, crosschip_links, list) {
-		if (c->port == port && c->other_ds == other_ds &&
+	list_for_each_entry(c, &ctx->crosschip_links, list) {
+		if (c->port == port && c->other_ctx == other_ctx &&
 		    c->other_port == other_port) {
 			refcount_inc(&c->refcount);
 			return 0;
 		}
 	}
 
-	dev_dbg(ds->dev, "adding crosschip link from port %d to %s port %d\n",
-		port, dev_name(other_ds->dev), other_port);
+	dev_dbg(ctx->ds->dev,
+		"adding crosschip link from port %d to %s port %d\n",
+		port, dev_name(other_ctx->ds->dev), other_port);
 
 	c = kzalloc(sizeof(*c), GFP_KERNEL);
 	if (!c)
 		return -ENOMEM;
 
 	c->port = port;
-	c->other_ds = other_ds;
+	c->other_ctx = other_ctx;
 	c->other_port = other_port;
 	refcount_set(&c->refcount, 1);
 
-	list_add(&c->list, crosschip_links);
+	list_add(&c->list, &ctx->crosschip_links);
 
 	return 0;
 }
 
-static void dsa_8021q_crosschip_link_del(struct dsa_switch *ds,
+static void dsa_8021q_crosschip_link_del(struct dsa_8021q_context *ctx,
 					 struct dsa_8021q_crosschip_link *c,
-					 struct list_head *crosschip_links,
 					 bool *keep)
 {
 	*keep = !refcount_dec_and_test(&c->refcount);
@@ -332,9 +354,9 @@ static void dsa_8021q_crosschip_link_del(struct dsa_switch *ds,
 	if (*keep)
 		return;
 
-	dev_dbg(ds->dev,
+	dev_dbg(ctx->ds->dev,
 		"deleting crosschip link from port %d to %s port %d\n",
-		c->port, dev_name(c->other_ds->dev), c->other_port);
+		c->port, dev_name(c->other_ctx->ds->dev), c->other_port);
 
 	list_del(&c->list);
 	kfree(c);
@@ -347,64 +369,58 @@ static void dsa_8021q_crosschip_link_del(struct dsa_switch *ds,
  * or untagged: it doesn't matter, since it should never egress a frame having
  * our @rx_vid.
  */
-int dsa_8021q_crosschip_bridge_join(struct dsa_switch *ds, int port,
-				    struct dsa_switch *other_ds,
-				    int other_port,
-				    struct list_head *crosschip_links)
+int dsa_8021q_crosschip_bridge_join(struct dsa_8021q_context *ctx, int port,
+				    struct dsa_8021q_context *other_ctx,
+				    int other_port)
 {
 	/* @other_upstream is how @other_ds reaches us. If we are part
 	 * of disjoint trees, then we are probably connected through
 	 * our CPU ports. If we're part of the same tree though, we should
 	 * probably use dsa_towards_port.
 	 */
-	int other_upstream = dsa_upstream_port(other_ds, other_port);
+	int other_upstream = dsa_upstream_port(other_ctx->ds, other_port);
 	int rc;
 
-	rc = dsa_8021q_crosschip_link_add(ds, port, other_ds,
-					  other_port, crosschip_links);
+	rc = dsa_8021q_crosschip_link_add(ctx, port, other_ctx, other_port);
 	if (rc)
 		return rc;
 
-	rc = dsa_8021q_crosschip_link_apply(ds, port, other_ds,
+	rc = dsa_8021q_crosschip_link_apply(ctx, port, other_ctx,
 					    other_port, true);
 	if (rc)
 		return rc;
 
-	rc = dsa_8021q_crosschip_link_add(ds, port, other_ds,
-					  other_upstream,
-					  crosschip_links);
+	rc = dsa_8021q_crosschip_link_add(ctx, port, other_ctx, other_upstream);
 	if (rc)
 		return rc;
 
-	return dsa_8021q_crosschip_link_apply(ds, port, other_ds,
+	return dsa_8021q_crosschip_link_apply(ctx, port, other_ctx,
 					      other_upstream, true);
 }
 EXPORT_SYMBOL_GPL(dsa_8021q_crosschip_bridge_join);
 
-int dsa_8021q_crosschip_bridge_leave(struct dsa_switch *ds, int port,
-				     struct dsa_switch *other_ds,
-				     int other_port,
-				     struct list_head *crosschip_links)
+int dsa_8021q_crosschip_bridge_leave(struct dsa_8021q_context *ctx, int port,
+				     struct dsa_8021q_context *other_ctx,
+				     int other_port)
 {
-	int other_upstream = dsa_upstream_port(other_ds, other_port);
+	int other_upstream = dsa_upstream_port(other_ctx->ds, other_port);
 	struct dsa_8021q_crosschip_link *c, *n;
 
-	list_for_each_entry_safe(c, n, crosschip_links, list) {
-		if (c->port == port && c->other_ds == other_ds &&
+	list_for_each_entry_safe(c, n, &ctx->crosschip_links, list) {
+		if (c->port == port && c->other_ctx == other_ctx &&
 		    (c->other_port == other_port ||
 		     c->other_port == other_upstream)) {
-			struct dsa_switch *other_ds = c->other_ds;
+			struct dsa_8021q_context *other_ctx = c->other_ctx;
 			int other_port = c->other_port;
 			bool keep;
 			int rc;
 
-			dsa_8021q_crosschip_link_del(ds, c, crosschip_links,
-						     &keep);
+			dsa_8021q_crosschip_link_del(ctx, c, &keep);
 			if (keep)
 				continue;
 
-			rc = dsa_8021q_crosschip_link_apply(ds, port,
-							    other_ds,
+			rc = dsa_8021q_crosschip_link_apply(ctx, port,
+							    other_ctx,
 							    other_port,
 							    false);
 			if (rc)
-- 
2.25.1

[PATCH net-next 3/4] Revert "net: dsa: Add more convenient functions for installing port VLANs"

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

This reverts commit 314f76d7a68bab0516aa52877944e6aacfa0fc3f.

Citing that commit message, the call graph was:

    dsa_slave_vlan_rx_add_vid   dsa_port_setup_8021q_tagging
                |                        |
                |                        |
                |          +-------------+
                |          |
                v          v
               dsa_port_vid_add      dsa_slave_port_obj_add
                      |                         |
                      +-------+         +-------+
                              |         |
                              v         v
                           dsa_port_vlan_add

Now that tag_8021q has its own ops structure, it no longer relies on
dsa_port_vid_add, and therefore on the dsa_switch_ops to install its
VLANs.

So dsa_port_vid_add now only has one single caller. So we can simplify
the call graph to what it was before, aka:

        dsa_slave_vlan_rx_add_vid     dsa_slave_port_obj_add
                      |                         |
                      +-------+         +-------+
                              |         |
                              v         v
                           dsa_port_vlan_add

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/dsa_priv.h |  2 --
 net/dsa/port.c     | 33 ---------------------------------
 net/dsa/slave.c    | 34 +++++++++++++++++++++++++++++++---
 3 files changed, 31 insertions(+), 38 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 1653e3377cb3..2da656d984ef 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -164,8 +164,6 @@ int dsa_port_vlan_add(struct dsa_port *dp,
 		      struct switchdev_trans *trans);
 int dsa_port_vlan_del(struct dsa_port *dp,
 		      const struct switchdev_obj_port_vlan *vlan);
-int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 flags);
-int dsa_port_vid_del(struct dsa_port *dp, u16 vid);
 int dsa_port_link_register_of(struct dsa_port *dp);
 void dsa_port_link_unregister_of(struct dsa_port *dp);
 extern const struct phylink_mac_ops dsa_port_phylink_mac_ops;
diff --git a/net/dsa/port.c b/net/dsa/port.c
index e23ece229c7e..46c9bf709683 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -433,39 +433,6 @@ int dsa_port_vlan_del(struct dsa_port *dp,
 	return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
 }
 
-int dsa_port_vid_add(struct dsa_port *dp, u16 vid, u16 flags)
-{
-	struct switchdev_obj_port_vlan vlan = {
-		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
-		.flags = flags,
-		.vid_begin = vid,
-		.vid_end = vid,
-	};
-	struct switchdev_trans trans;
-	int err;
-
-	trans.ph_prepare = true;
-	err = dsa_port_vlan_add(dp, &vlan, &trans);
-	if (err)
-		return err;
-
-	trans.ph_prepare = false;
-	return dsa_port_vlan_add(dp, &vlan, &trans);
-}
-EXPORT_SYMBOL(dsa_port_vid_add);
-
-int dsa_port_vid_del(struct dsa_port *dp, u16 vid)
-{
-	struct switchdev_obj_port_vlan vlan = {
-		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
-		.vid_begin = vid,
-		.vid_end = vid,
-	};
-
-	return dsa_port_vlan_del(dp, &vlan);
-}
-EXPORT_SYMBOL(dsa_port_vid_del);
-
 static struct phy_device *dsa_port_get_phy_device(struct dsa_port *dp)
 {
 	struct device_node *phy_dn;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 9af1a2d0cec4..e429c71df854 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1233,7 +1233,15 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
 				     u16 vid)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct switchdev_obj_port_vlan vlan = {
+		.obj.id = SWITCHDEV_OBJ_ID_PORT_VLAN,
+		.vid_begin = vid,
+		.vid_end = vid,
+		/* This API only allows programming tagged, non-PVID VIDs */
+		.flags = 0,
+	};
 	struct bridge_vlan_info info;
+	struct switchdev_trans trans;
 	int ret;
 
 	/* Check for a possible bridge VLAN entry now since there is no
@@ -1252,11 +1260,25 @@ static int dsa_slave_vlan_rx_add_vid(struct net_device *dev, __be16 proto,
 			return -EBUSY;
 	}
 
-	ret = dsa_port_vid_add(dp, vid, 0);
+	/* User port... */
+	trans.ph_prepare = true;
+	ret = dsa_port_vlan_add(dp, &vlan, &trans);
+	if (ret)
+		return ret;
+
+	trans.ph_prepare = false;
+	ret = dsa_port_vlan_add(dp, &vlan, &trans);
 	if (ret)
 		return ret;
 
-	ret = dsa_port_vid_add(dp->cpu_dp, vid, 0);
+	/* And CPU port... */
+	trans.ph_prepare = true;
+	ret = dsa_port_vlan_add(dp->cpu_dp, &vlan, &trans);
+	if (ret)
+		return ret;
+
+	trans.ph_prepare = false;
+	ret = dsa_port_vlan_add(dp->cpu_dp, &vlan, &trans);
 	if (ret)
 		return ret;
 
@@ -1267,6 +1289,12 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
 				      u16 vid)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct switchdev_obj_port_vlan vlan = {
+		.vid_begin = vid,
+		.vid_end = vid,
+		/* This API only allows programming tagged, non-PVID VIDs */
+		.flags = 0,
+	};
 	struct bridge_vlan_info info;
 	int ret;
 
@@ -1289,7 +1317,7 @@ static int dsa_slave_vlan_rx_kill_vid(struct net_device *dev, __be16 proto,
 	/* 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_vid_del(dp, vid);
+	return dsa_port_vlan_del(dp, &vlan);
 }
 
 struct dsa_hw_port {
-- 
2.25.1

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

[Vladimir Oltean]

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>
---
 drivers/net/dsa/b53/b53_common.c       |  1 +
 drivers/net/dsa/bcm_sf2.c              |  1 +
 drivers/net/dsa/dsa_loop.c             |  1 -
 drivers/net/dsa/lantiq_gswip.c         |  3 +++
 drivers/net/dsa/microchip/ksz8795.c    |  2 ++
 drivers/net/dsa/microchip/ksz9477.c    |  2 ++
 drivers/net/dsa/mt7530.c               |  1 -
 drivers/net/dsa/mv88e6xxx/chip.c       |  1 +
 drivers/net/dsa/ocelot/felix.c         |  1 -
 drivers/net/dsa/qca/ar9331.c           |  2 ++
 drivers/net/dsa/qca8k.c                |  1 -
 drivers/net/dsa/rtl8366rb.c            |  2 ++
 drivers/net/dsa/sja1105/sja1105_main.c |  2 --
 net/dsa/dsa2.c                         |  2 ++
 net/dsa/slave.c                        | 29 +++++++++++++++++++-------
 15 files changed, 38 insertions(+), 13 deletions(-)

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
-- 
2.25.1

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

[Florian Fainelli]

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.
-- 
Florian

Re: [PATCH net-next 1/4] net: dsa: tag_8021q: include missing refcount.h

[Florian Fainelli]

On 9/7/2020 11:29 AM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> The previous assumption was that the caller would already have this
> header file included.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

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

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 1285 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Vladimir Oltean]

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

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

[Vladimir Oltean]

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

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

[Florian Fainelli]

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.
-- 
Florian

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

[Florian Fainelli]

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);


-- 
Florian

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

[Vladimir Oltean]

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

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

[Florian Fainelli]

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!
-- 
Florian

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

[Vladimir Oltean]

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.

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

[Florian Fainelli]

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.
> 

-- 
Florian

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

[Florian Fainelli]

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));

-- 
Florian

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

[Vladimir Oltean]

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

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

[Florian Fainelli]

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!
-- 
Florian

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

[Vladimir Oltean]

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;
 	}
 
-- 
2.25.1

Thanks,
-Vladimir

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

[Florian Fainelli]

On 9/11/2020 8:43 AM, Vladimir Oltean wrote:
> 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.

Yes, this is a problem for any switch that forces the CPU port to be 
tagged in all VLANs added to user-ports I would say be it a driver 
decision or hardware limitation.

The customers I support with bcm_sf2 mostly use the switch ports as 
standalone network devices (which reminds me I need to test your DSA RX 
filtering series, sigh) with 802.1Q uppers on top. Bridging works, but 
does not seem to be their main use case at all that comes from having 
migrated from 3 independent Ethernet MACs to an integrated switch, and 
now back to integrated Ethernet MACs.

> 
>>>
>>>> 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?

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!

> 
>  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.

We could also indicate that some DSA switch drivers systematically 
configure their CPU port to be tagged in all VLANs, so even in the 
following case:

ip link add dev br0 type bridge vlan_filtering 0
ip link set swp0 master br0

we will be receiving egress tagged frames for default_pvid because the 
CPU port is configured as egress tagged for that VLAN. In the case of a 
vlan_filtering=1, the default_pvid is accepted as tagged or untagged by 
the bridge master device. (something along those lines).

> 
> 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>

Tested-by: Florian Fainelli <f.fainelli@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;
>   	}
>   
> 

-- 
Florian

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

[Vladimir Oltean]

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

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

[Florian Fainelli]

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()?
-- 
Florian

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

[Florian Fainelli]

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.
-- 
Florian

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

[Vladimir Oltean]

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

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

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 1383 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Vladimir Oltean]

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

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

[Vladimir Oltean]

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

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

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Vladimir Oltean]

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

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

[Vladimir Oltean]

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