[Patch RFC net-next 0/4] net: dsa: microchip: vlan configuration for bridge_vlan_unaware ports

[Patch RFC net-next 0/4] net: dsa: microchip: vlan configuration for bridge_vlan_unaware ports

[Arun Ramadoss]

During refactoring of ksz spi probe, ds->configure_vlan_while_filtering flag is
set in the ksz_setup function. It makes the extack warning when vlan_add
command is used when vlan_filtering is turned off for the port.
To allow the vlan_add for the bridge vlan unaware ports, private pvid is used
for it. This pvid is used for the vlan_unaware ports. The private pvid is added
to the vlan_table during the port_setup function.
For the vlan aware ports, pvid issued by the user application will be used.
Bridge vlan pvid is stored in the ksz port structure which can be used during
transition between the vlan aware to unaware and vice versa.

Tested using the following sequence for ksz9477 and lan937x
----------------
Setup -> Host1 --- LAN1 -- LAN2 --- Host2

ip link set dev br0 type bridge vlan_filtering 0
bridge vlan add vid 5 dev lan1 pvid untagged
bridge vlan add vid 5 dev lan2 pvid untagged
bridge vlan del vid 5 dev lan1
bridge vlan del vid 5 dev lan2

After each step of execution, transmitted the packets from Host1 and
successfully received by the Host2.

Arun Ramadoss (4):
  net: dsa: microchip: modify vlan_add function prototype
  net: dsa: microchip: lan937x: remove vlan_filtering_is_global flag
  net: dsa: microchip: common ksz pvid get and set function
  net: dsa: microchip: use private pvid for bridge_vlan_unwaware

 drivers/net/dsa/microchip/ksz8.h         |  6 +-
 drivers/net/dsa/microchip/ksz8795.c      | 42 +++-------
 drivers/net/dsa/microchip/ksz9477.c      | 40 ++++------
 drivers/net/dsa/microchip/ksz9477.h      |  5 +-
 drivers/net/dsa/microchip/ksz_common.c   | 98 +++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz_common.h   | 21 ++++-
 drivers/net/dsa/microchip/lan937x_main.c |  5 --
 7 files changed, 142 insertions(+), 75 deletions(-)


base-commit: ba323f6bee1d1e70aed280f8c89ac06959559855
-- 
2.36.1

[Patch RFC net-next 1/4] net: dsa: microchip: modify vlan_add function prototype

[Arun Ramadoss]

To use the vlan_add from the function other the .port_vlan_add dsa hook,
changed the ksz8_vlan_add and ksz9477_vlan_del function prototype which
uses only the vlan->vid and vlan->flags.

Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
---
 drivers/net/dsa/microchip/ksz8.h       |  5 ++---
 drivers/net/dsa/microchip/ksz8795.c    | 19 +++++++++----------
 drivers/net/dsa/microchip/ksz9477.c    | 25 ++++++++++---------------
 drivers/net/dsa/microchip/ksz9477.h    |  4 +---
 drivers/net/dsa/microchip/ksz_common.c |  2 +-
 drivers/net/dsa/microchip/ksz_common.h |  4 +---
 6 files changed, 24 insertions(+), 35 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h
index 42c50cc4d853..6529f2e2426a 100644
--- a/drivers/net/dsa/microchip/ksz8.h
+++ b/drivers/net/dsa/microchip/ksz8.h
@@ -38,9 +38,8 @@ int ksz8_mdb_del(struct ksz_device *dev, int port,
 		 const struct switchdev_obj_port_mdb *mdb, struct dsa_db db);
 int ksz8_port_vlan_filtering(struct ksz_device *dev, int port, bool flag,
 			     struct netlink_ext_ack *extack);
-int ksz8_port_vlan_add(struct ksz_device *dev, int port,
-		       const struct switchdev_obj_port_vlan *vlan,
-		       struct netlink_ext_ack *extack);
+int ksz8_port_vlan_add(struct ksz_device *dev, int port, u16 vlan_vid,
+		       u16 flags);
 int ksz8_port_vlan_del(struct ksz_device *dev, int port,
 		       const struct switchdev_obj_port_vlan *vlan);
 int ksz8_port_mirror_add(struct ksz_device *dev, int port,
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index c79a5128235f..5dd73e994142 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -962,11 +962,10 @@ static void ksz8_port_enable_pvid(struct ksz_device *dev, int port, bool state)
 	}
 }
 
-int ksz8_port_vlan_add(struct ksz_device *dev, int port,
-		       const struct switchdev_obj_port_vlan *vlan,
-		       struct netlink_ext_ack *extack)
+int ksz8_port_vlan_add(struct ksz_device *dev, int port, u16 vlan_vid,
+		       u16 flags)
 {
-	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
+	bool untagged = flags & BRIDGE_VLAN_INFO_UNTAGGED;
 	struct ksz_port *p = &dev->ports[port];
 	u16 data, new_pvid = 0;
 	u8 fid, member, valid;
@@ -979,7 +978,7 @@ int ksz8_port_vlan_add(struct ksz_device *dev, int port,
 	 * Ignore VID 0, which is always untagged.
 	 * Ignore CPU port, which will always be tagged.
 	 */
-	if (untagged != p->remove_tag && vlan->vid != 0 &&
+	if (untagged != p->remove_tag && vlan_vid != 0 &&
 	    port != dev->cpu_port) {
 		unsigned int vid;
 
@@ -989,7 +988,7 @@ int ksz8_port_vlan_add(struct ksz_device *dev, int port,
 		 */
 		for (vid = 1; vid < dev->info->num_vlans; ++vid) {
 			/* Skip the VID we are going to add or reconfigure */
-			if (vid == vlan->vid)
+			if (vid == vlan_vid)
 				continue;
 
 			ksz8_from_vlan(dev, dev->vlan_cache[vid].table[0],
@@ -1002,7 +1001,7 @@ int ksz8_port_vlan_add(struct ksz_device *dev, int port,
 		p->remove_tag = untagged;
 	}
 
-	ksz8_r_vlan_table(dev, vlan->vid, &data);
+	ksz8_r_vlan_table(dev, vlan_vid, &data);
 	ksz8_from_vlan(dev, data, &fid, &member, &valid);
 
 	/* First time to setup the VLAN entry. */
@@ -1014,11 +1013,11 @@ int ksz8_port_vlan_add(struct ksz_device *dev, int port,
 	member |= BIT(port);
 
 	ksz8_to_vlan(dev, fid, member, valid, &data);
-	ksz8_w_vlan_table(dev, vlan->vid, data);
+	ksz8_w_vlan_table(dev, vlan_vid, data);
 
 	/* change PVID */
-	if (vlan->flags & BRIDGE_VLAN_INFO_PVID)
-		new_pvid = vlan->vid;
+	if (flags & BRIDGE_VLAN_INFO_PVID)
+		new_pvid = vlan_vid;
 
 	if (new_pvid) {
 		u16 vid;
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 4b14d80d27ed..81d24b89958b 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -370,21 +370,18 @@ int ksz9477_port_vlan_filtering(struct ksz_device *dev, int port,
 	return 0;
 }
 
-int ksz9477_port_vlan_add(struct ksz_device *dev, int port,
-			  const struct switchdev_obj_port_vlan *vlan,
-			  struct netlink_ext_ack *extack)
+int ksz9477_port_vlan_add(struct ksz_device *dev, int port, u16 vlan_vid,
+			  u16 flags)
 {
 	u32 vlan_table[3];
-	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
+	bool untagged = flags & BRIDGE_VLAN_INFO_UNTAGGED;
 	int err;
 
-	err = ksz9477_get_vlan_table(dev, vlan->vid, vlan_table);
-	if (err) {
-		NL_SET_ERR_MSG_MOD(extack, "Failed to get vlan table");
+	err = ksz9477_get_vlan_table(dev, vlan_vid, vlan_table);
+	if (err)
 		return err;
-	}
 
-	vlan_table[0] = VLAN_VALID | (vlan->vid & VLAN_FID_M);
+	vlan_table[0] = VLAN_VALID | (vlan_vid & VLAN_FID_M);
 	if (untagged)
 		vlan_table[1] |= BIT(port);
 	else
@@ -393,15 +390,13 @@ int ksz9477_port_vlan_add(struct ksz_device *dev, int port,
 
 	vlan_table[2] |= BIT(port) | BIT(dev->cpu_port);
 
-	err = ksz9477_set_vlan_table(dev, vlan->vid, vlan_table);
-	if (err) {
-		NL_SET_ERR_MSG_MOD(extack, "Failed to set vlan table");
+	err = ksz9477_set_vlan_table(dev, vlan_vid, vlan_table);
+	if (err)
 		return err;
-	}
 
 	/* change PVID */
-	if (vlan->flags & BRIDGE_VLAN_INFO_PVID)
-		ksz_pwrite16(dev, port, REG_PORT_DEFAULT_VID, vlan->vid);
+	if (flags & BRIDGE_VLAN_INFO_PVID)
+		ksz_pwrite16(dev, port, REG_PORT_DEFAULT_VID, vlan_vid);
 
 	return 0;
 }
diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h
index cd278b307b3c..30a1fff9b8ec 100644
--- a/drivers/net/dsa/microchip/ksz9477.h
+++ b/drivers/net/dsa/microchip/ksz9477.h
@@ -25,9 +25,7 @@ void ksz9477_freeze_mib(struct ksz_device *dev, int port, bool freeze);
 void ksz9477_port_init_cnt(struct ksz_device *dev, int port);
 int ksz9477_port_vlan_filtering(struct ksz_device *dev, int port,
 				bool flag, struct netlink_ext_ack *extack);
-int ksz9477_port_vlan_add(struct ksz_device *dev, int port,
-			  const struct switchdev_obj_port_vlan *vlan,
-			  struct netlink_ext_ack *extack);
+int ksz9477_port_vlan_add(struct ksz_device *dev, int port, u16 vid, u16 flags);
 int ksz9477_port_vlan_del(struct ksz_device *dev, int port,
 			  const struct switchdev_obj_port_vlan *vlan);
 int ksz9477_port_mirror_add(struct ksz_device *dev, int port,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 02e58760cf7b..97dbccb065a9 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1351,7 +1351,7 @@ static int ksz_port_vlan_add(struct dsa_switch *ds, int port,
 	if (!dev->dev_ops->vlan_add)
 		return -EOPNOTSUPP;
 
-	return dev->dev_ops->vlan_add(dev, port, vlan, extack);
+	return dev->dev_ops->vlan_add(dev, port, vlan->vid, vlan->flags);
 }
 
 static int ksz_port_vlan_del(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 764ada3a0f42..9bb378b79a94 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -267,9 +267,7 @@ struct ksz_dev_ops {
 	void (*r_mib_stat64)(struct ksz_device *dev, int port);
 	int  (*vlan_filtering)(struct ksz_device *dev, int port,
 			       bool flag, struct netlink_ext_ack *extack);
-	int  (*vlan_add)(struct ksz_device *dev, int port,
-			 const struct switchdev_obj_port_vlan *vlan,
-			 struct netlink_ext_ack *extack);
+	int  (*vlan_add)(struct ksz_device *dev, int port, u16 vid, u16 flags);
 	int  (*vlan_del)(struct ksz_device *dev, int port,
 			 const struct switchdev_obj_port_vlan *vlan);
 	int (*mirror_add)(struct ksz_device *dev, int port,
-- 
2.36.1

[Patch RFC net-next 2/4] net: dsa: microchip: lan937x: remove vlan_filtering_is_global flag

[Arun Ramadoss]

To have the similar implementation among the ksz switches, removed the
vlan_filtering_is_global flag which is only present in the lan937x.

Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
---
 drivers/net/dsa/microchip/lan937x_main.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c
index daedd2bf20c1..9c1fe38efd1a 100644
--- a/drivers/net/dsa/microchip/lan937x_main.c
+++ b/drivers/net/dsa/microchip/lan937x_main.c
@@ -401,11 +401,6 @@ int lan937x_setup(struct dsa_switch *ds)
 		return ret;
 	}
 
-	/* The VLAN aware is a global setting. Mixed vlan
-	 * filterings are not supported.
-	 */
-	ds->vlan_filtering_is_global = true;
-
 	/* Enable aggressive back off for half duplex & UNH mode */
 	lan937x_cfg(dev, REG_SW_MAC_CTRL_0,
 		    (SW_PAUSE_UNH_MODE | SW_NEW_BACKOFF | SW_AGGR_BACKOFF),
-- 
2.36.1

[Patch RFC net-next 3/4] net: dsa: microchip: common ksz pvid get and set function

[Arun Ramadoss]

Add the helper function for getting and setting the pvid which will be
common for all ksz switches

Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
---
 drivers/net/dsa/microchip/ksz8795.c    |  8 ++------
 drivers/net/dsa/microchip/ksz9477.c    |  6 +++---
 drivers/net/dsa/microchip/ksz_common.c | 21 +++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h | 11 +++++++++++
 4 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 5dd73e994142..b8843697c5a5 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -1020,12 +1020,8 @@ int ksz8_port_vlan_add(struct ksz_device *dev, int port, u16 vlan_vid,
 		new_pvid = vlan_vid;
 
 	if (new_pvid) {
-		u16 vid;
 
-		ksz_pread16(dev, port, REG_PORT_CTRL_VID, &vid);
-		vid &= ~VLAN_VID_MASK;
-		vid |= new_pvid;
-		ksz_pwrite16(dev, port, REG_PORT_CTRL_VID, vid);
+		ksz_set_pvid(dev, port, new_pvid);
 
 		ksz8_port_enable_pvid(dev, port, true);
 	}
@@ -1042,7 +1038,7 @@ int ksz8_port_vlan_del(struct ksz_device *dev, int port,
 	if (ksz_is_ksz88x3(dev))
 		return -ENOTSUPP;
 
-	ksz_pread16(dev, port, REG_PORT_CTRL_VID, &pvid);
+	ksz_get_pvid(dev, port, &pvid);
 	pvid = pvid & 0xFFF;
 
 	ksz8_r_vlan_table(dev, vlan->vid, &data);
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 81d24b89958b..a43a581520fb 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -396,7 +396,7 @@ int ksz9477_port_vlan_add(struct ksz_device *dev, int port, u16 vlan_vid,
 
 	/* change PVID */
 	if (flags & BRIDGE_VLAN_INFO_PVID)
-		ksz_pwrite16(dev, port, REG_PORT_DEFAULT_VID, vlan_vid);
+		ksz_set_pvid(dev, port, vlan_vid);
 
 	return 0;
 }
@@ -408,7 +408,7 @@ int ksz9477_port_vlan_del(struct ksz_device *dev, int port,
 	u32 vlan_table[3];
 	u16 pvid;
 
-	ksz_pread16(dev, port, REG_PORT_DEFAULT_VID, &pvid);
+	ksz_get_pvid(dev, port, &pvid);
 	pvid = pvid & 0xFFF;
 
 	if (ksz9477_get_vlan_table(dev, vlan->vid, vlan_table)) {
@@ -429,7 +429,7 @@ int ksz9477_port_vlan_del(struct ksz_device *dev, int port,
 		return -ETIMEDOUT;
 	}
 
-	ksz_pwrite16(dev, port, REG_PORT_DEFAULT_VID, pvid);
+	ksz_set_pvid(dev, port, pvid);
 
 	return 0;
 }
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 97dbccb065a9..516fb9d35c87 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -14,6 +14,7 @@
 #include <linux/phy.h>
 #include <linux/etherdevice.h>
 #include <linux/if_bridge.h>
+#include <linux/if_vlan.h>
 #include <linux/of_device.h>
 #include <linux/of_net.h>
 #include <linux/micrel_phy.h>
@@ -258,6 +259,7 @@ static const u16 ksz8795_regs[] = {
 	[S_MULTICAST_CTRL]		= 0x04,
 	[P_XMII_CTRL_0]			= 0x06,
 	[P_XMII_CTRL_1]			= 0x56,
+	[P_DEFAULT_PVID]		= 0x03,
 };
 
 static const u32 ksz8795_masks[] = {
@@ -330,6 +332,7 @@ static const u16 ksz8863_regs[] = {
 	[S_START_CTRL]			= 0x01,
 	[S_BROADCAST_CTRL]		= 0x06,
 	[S_MULTICAST_CTRL]		= 0x04,
+	[P_DEFAULT_PVID]		= 0x03,
 };
 
 static const u32 ksz8863_masks[] = {
@@ -372,6 +375,7 @@ static const u16 ksz9477_regs[] = {
 	[S_MULTICAST_CTRL]		= 0x0331,
 	[P_XMII_CTRL_0]			= 0x0300,
 	[P_XMII_CTRL_1]			= 0x0301,
+	[P_DEFAULT_PVID]		= 0x0000,
 };
 
 static const u32 ksz9477_masks[] = {
@@ -1331,6 +1335,23 @@ static enum dsa_tag_protocol ksz_get_tag_protocol(struct dsa_switch *ds,
 	return proto;
 }
 
+void ksz_get_pvid(struct ksz_device *dev, int port, u16 *pvid)
+{
+	const u16 *regs = dev->info->regs;
+	u16 val;
+
+	ksz_pread16(dev, port, regs[P_DEFAULT_PVID], &val);
+
+	*pvid = val & VLAN_VID_MASK;
+}
+
+void ksz_set_pvid(struct ksz_device *dev, int port, u16 pvid)
+{
+	const u16 *regs = dev->info->regs;
+
+	ksz_prmw16(dev, port, regs[P_DEFAULT_PVID], VLAN_VID_MASK, pvid);
+}
+
 static int ksz_port_vlan_filtering(struct dsa_switch *ds, int port,
 				   bool flag, struct netlink_ext_ack *extack)
 {
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 9bb378b79a94..3bcd4e20bfaa 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -175,6 +175,7 @@ enum ksz_regs {
 	S_MULTICAST_CTRL,
 	P_XMII_CTRL_0,
 	P_XMII_CTRL_1,
+	P_DEFAULT_PVID,
 };
 
 enum ksz_masks {
@@ -319,6 +320,8 @@ void ksz_port_stp_state_set(struct dsa_switch *ds, int port, u8 state);
 bool ksz_get_gbit(struct ksz_device *dev, int port);
 phy_interface_t ksz_get_xmii(struct ksz_device *dev, int port, bool gbit);
 extern const struct ksz_chip_data ksz_switch_chips[];
+void ksz_get_pvid(struct ksz_device *dev, int port, u16 *pvid);
+void ksz_set_pvid(struct ksz_device *dev, int port, u16 pvid);
 
 /* Common register access functions */
 
@@ -432,6 +435,14 @@ static inline void ksz_prmw8(struct ksz_device *dev, int port, int offset,
 			   mask, val);
 }
 
+static inline void ksz_prmw16(struct ksz_device *dev, int port, int offset,
+			      u16 mask, u16 val)
+{
+	regmap_update_bits(dev->regmap[1],
+			   dev->dev_ops->get_port_addr(port, offset),
+			   mask, val);
+}
+
 static inline void ksz_regmap_lock(void *__mtx)
 {
 	struct mutex *mtx = __mtx;
-- 
2.36.1

[Patch RFC net-next 4/4] net: dsa: microchip: use private pvid for bridge_vlan_unwaware

[Arun Ramadoss]

To remove ds->configure_vlan_while_not_filtering, for the bridge vlan
unaware the private pvid of 4095 is used. The bridged vlan pvid is
stored in the ksz port structure and it is used only when vlan_filtering
is enabled. If vlan_filtering is disabled then private pvid is used.

Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
---
 drivers/net/dsa/microchip/ksz8.h       |  1 +
 drivers/net/dsa/microchip/ksz8795.c    | 23 +-------
 drivers/net/dsa/microchip/ksz9477.c    | 19 ++----
 drivers/net/dsa/microchip/ksz9477.h    |  1 +
 drivers/net/dsa/microchip/ksz_common.c | 81 ++++++++++++++++++++++++--
 drivers/net/dsa/microchip/ksz_common.h | 10 +++-
 6 files changed, 95 insertions(+), 40 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h
index 6529f2e2426a..c5b0ab031427 100644
--- a/drivers/net/dsa/microchip/ksz8.h
+++ b/drivers/net/dsa/microchip/ksz8.h
@@ -42,6 +42,7 @@ int ksz8_port_vlan_add(struct ksz_device *dev, int port, u16 vlan_vid,
 		       u16 flags);
 int ksz8_port_vlan_del(struct ksz_device *dev, int port,
 		       const struct switchdev_obj_port_vlan *vlan);
+void ksz8_port_enable_pvid(struct ksz_device *dev, int port, bool state);
 int ksz8_port_mirror_add(struct ksz_device *dev, int port,
 			 struct dsa_mall_mirror_tc_entry *mirror,
 			 bool ingress, struct netlink_ext_ack *extack);
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index b8843697c5a5..16709949d079 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -952,7 +952,7 @@ int ksz8_port_vlan_filtering(struct ksz_device *dev, int port, bool flag,
 	return 0;
 }
 
-static void ksz8_port_enable_pvid(struct ksz_device *dev, int port, bool state)
+void ksz8_port_enable_pvid(struct ksz_device *dev, int port, bool state)
 {
 	if (ksz_is_ksz88x3(dev)) {
 		ksz_cfg(dev, REG_SW_INSERT_SRC_PVID,
@@ -967,8 +967,8 @@ int ksz8_port_vlan_add(struct ksz_device *dev, int port, u16 vlan_vid,
 {
 	bool untagged = flags & BRIDGE_VLAN_INFO_UNTAGGED;
 	struct ksz_port *p = &dev->ports[port];
-	u16 data, new_pvid = 0;
 	u8 fid, member, valid;
+	u16 data;
 
 	if (ksz_is_ksz88x3(dev))
 		return -ENOTSUPP;
@@ -1015,32 +1015,18 @@ int ksz8_port_vlan_add(struct ksz_device *dev, int port, u16 vlan_vid,
 	ksz8_to_vlan(dev, fid, member, valid, &data);
 	ksz8_w_vlan_table(dev, vlan_vid, data);
 
-	/* change PVID */
-	if (flags & BRIDGE_VLAN_INFO_PVID)
-		new_pvid = vlan_vid;
-
-	if (new_pvid) {
-
-		ksz_set_pvid(dev, port, new_pvid);
-
-		ksz8_port_enable_pvid(dev, port, true);
-	}
-
 	return 0;
 }
 
 int ksz8_port_vlan_del(struct ksz_device *dev, int port,
 		       const struct switchdev_obj_port_vlan *vlan)
 {
-	u16 data, pvid;
+	u16 data;
 	u8 fid, member, valid;
 
 	if (ksz_is_ksz88x3(dev))
 		return -ENOTSUPP;
 
-	ksz_get_pvid(dev, port, &pvid);
-	pvid = pvid & 0xFFF;
-
 	ksz8_r_vlan_table(dev, vlan->vid, &data);
 	ksz8_from_vlan(dev, data, &fid, &member, &valid);
 
@@ -1055,9 +1041,6 @@ int ksz8_port_vlan_del(struct ksz_device *dev, int port,
 	ksz8_to_vlan(dev, fid, member, valid, &data);
 	ksz8_w_vlan_table(dev, vlan->vid, data);
 
-	if (pvid == vlan->vid)
-		ksz8_port_enable_pvid(dev, port, false);
-
 	return 0;
 }
 
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index a43a581520fb..b423aebe4473 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -354,6 +354,12 @@ void ksz9477_flush_dyn_mac_table(struct ksz_device *dev, int port)
 	}
 }
 
+void ksz9477_port_drop_untagged(struct ksz_device *dev, int port, bool state)
+{
+	ksz_port_cfg(dev, port, REG_PORT_MRI_MAC_CTRL, PORT_DROP_NON_VLAN,
+		     state);
+}
+
 int ksz9477_port_vlan_filtering(struct ksz_device *dev, int port,
 				bool flag, struct netlink_ext_ack *extack)
 {
@@ -394,10 +400,6 @@ int ksz9477_port_vlan_add(struct ksz_device *dev, int port, u16 vlan_vid,
 	if (err)
 		return err;
 
-	/* change PVID */
-	if (flags & BRIDGE_VLAN_INFO_PVID)
-		ksz_set_pvid(dev, port, vlan_vid);
-
 	return 0;
 }
 
@@ -406,10 +408,6 @@ int ksz9477_port_vlan_del(struct ksz_device *dev, int port,
 {
 	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
 	u32 vlan_table[3];
-	u16 pvid;
-
-	ksz_get_pvid(dev, port, &pvid);
-	pvid = pvid & 0xFFF;
 
 	if (ksz9477_get_vlan_table(dev, vlan->vid, vlan_table)) {
 		dev_dbg(dev->dev, "Failed to get vlan table\n");
@@ -418,9 +416,6 @@ int ksz9477_port_vlan_del(struct ksz_device *dev, int port,
 
 	vlan_table[2] &= ~BIT(port);
 
-	if (pvid == vlan->vid)
-		pvid = 1;
-
 	if (untagged)
 		vlan_table[1] &= ~BIT(port);
 
@@ -429,8 +424,6 @@ int ksz9477_port_vlan_del(struct ksz_device *dev, int port,
 		return -ETIMEDOUT;
 	}
 
-	ksz_set_pvid(dev, port, pvid);
-
 	return 0;
 }
 
diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h
index 30a1fff9b8ec..2bd88319a2c0 100644
--- a/drivers/net/dsa/microchip/ksz9477.h
+++ b/drivers/net/dsa/microchip/ksz9477.h
@@ -28,6 +28,7 @@ int ksz9477_port_vlan_filtering(struct ksz_device *dev, int port,
 int ksz9477_port_vlan_add(struct ksz_device *dev, int port, u16 vid, u16 flags);
 int ksz9477_port_vlan_del(struct ksz_device *dev, int port,
 			  const struct switchdev_obj_port_vlan *vlan);
+void ksz9477_port_drop_untagged(struct ksz_device *dev, int port, bool state);
 int ksz9477_port_mirror_add(struct ksz_device *dev, int port,
 			    struct dsa_mall_mirror_tc_entry *mirror,
 			    bool ingress, struct netlink_ext_ack *extack);
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 516fb9d35c87..8a5583b1f2f4 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -161,6 +161,7 @@ static const struct ksz_dev_ops ksz8_dev_ops = {
 	.vlan_filtering = ksz8_port_vlan_filtering,
 	.vlan_add = ksz8_port_vlan_add,
 	.vlan_del = ksz8_port_vlan_del,
+	.drop_untagged = ksz8_port_enable_pvid,
 	.mirror_add = ksz8_port_mirror_add,
 	.mirror_del = ksz8_port_mirror_del,
 	.get_caps = ksz8_get_caps,
@@ -187,6 +188,7 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 	.vlan_filtering = ksz9477_port_vlan_filtering,
 	.vlan_add = ksz9477_port_vlan_add,
 	.vlan_del = ksz9477_port_vlan_del,
+	.drop_untagged = ksz9477_port_drop_untagged,
 	.mirror_add = ksz9477_port_mirror_add,
 	.mirror_del = ksz9477_port_mirror_del,
 	.get_caps = ksz9477_get_caps,
@@ -220,6 +222,7 @@ static const struct ksz_dev_ops lan937x_dev_ops = {
 	.vlan_filtering = ksz9477_port_vlan_filtering,
 	.vlan_add = ksz9477_port_vlan_add,
 	.vlan_del = ksz9477_port_vlan_del,
+	.drop_untagged = ksz9477_port_drop_untagged,
 	.mirror_add = ksz9477_port_mirror_add,
 	.mirror_del = ksz9477_port_mirror_del,
 	.get_caps = lan937x_phylink_get_caps,
@@ -1254,6 +1257,9 @@ static int ksz_enable_port(struct dsa_switch *ds, int port,
 {
 	struct ksz_device *dev = ds->priv;
 
+	dev->dev_ops->vlan_add(dev, port, KSZ_DEFAULT_VLAN,
+			       BRIDGE_VLAN_INFO_UNTAGGED);
+
 	if (!dsa_is_user_port(ds, port))
 		return 0;
 
@@ -1335,7 +1341,7 @@ static enum dsa_tag_protocol ksz_get_tag_protocol(struct dsa_switch *ds,
 	return proto;
 }
 
-void ksz_get_pvid(struct ksz_device *dev, int port, u16 *pvid)
+static void ksz_get_pvid(struct ksz_device *dev, int port, u16 *pvid)
 {
 	const u16 *regs = dev->info->regs;
 	u16 val;
@@ -1345,45 +1351,110 @@ void ksz_get_pvid(struct ksz_device *dev, int port, u16 *pvid)
 	*pvid = val & VLAN_VID_MASK;
 }
 
-void ksz_set_pvid(struct ksz_device *dev, int port, u16 pvid)
+static void ksz_set_pvid(struct ksz_device *dev, int port, u16 pvid)
 {
 	const u16 *regs = dev->info->regs;
 
 	ksz_prmw16(dev, port, regs[P_DEFAULT_PVID], VLAN_VID_MASK, pvid);
 }
 
+static int ksz_commit_pvid(struct dsa_switch *ds, int port)
+{
+	struct dsa_port *dp = dsa_to_port(ds, port);
+	struct net_device *br = dsa_port_bridge_dev_get(dp);
+	struct ksz_device *dev = ds->priv;
+	u16 pvid = KSZ_DEFAULT_VLAN;
+	bool drop_untagged = false;
+	struct ksz_port *p;
+
+	p = &dev->ports[port];
+
+	if (br && br_vlan_enabled(br)) {
+		pvid = p->bridge_pvid.vid;
+		drop_untagged = !p->bridge_pvid.valid;
+	}
+
+	ksz_set_pvid(dev, port, pvid);
+
+	if (dev->dev_ops->drop_untagged)
+		dev->dev_ops->drop_untagged(dev, port, drop_untagged);
+
+	return 0;
+}
+
 static int ksz_port_vlan_filtering(struct dsa_switch *ds, int port,
 				   bool flag, struct netlink_ext_ack *extack)
 {
 	struct ksz_device *dev = ds->priv;
+	int ret;
 
 	if (!dev->dev_ops->vlan_filtering)
 		return -EOPNOTSUPP;
 
-	return dev->dev_ops->vlan_filtering(dev, port, flag, extack);
+	ret = dev->dev_ops->vlan_filtering(dev, port, flag, extack);
+	if (ret)
+		return ret;
+
+	return ksz_commit_pvid(ds, port);
 }
 
 static int ksz_port_vlan_add(struct dsa_switch *ds, int port,
 			     const struct switchdev_obj_port_vlan *vlan,
 			     struct netlink_ext_ack *extack)
 {
+	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
 	struct ksz_device *dev = ds->priv;
+	struct ksz_port *p;
+	int ret;
+
+	p = &dev->ports[port];
 
 	if (!dev->dev_ops->vlan_add)
 		return -EOPNOTSUPP;
 
-	return dev->dev_ops->vlan_add(dev, port, vlan->vid, vlan->flags);
+	ret = dev->dev_ops->vlan_add(dev, port, vlan->vid, vlan->flags);
+	if (ret)
+		return ret;
+
+	if (pvid) {
+		p->bridge_pvid.vid = vlan->vid;
+		p->bridge_pvid.valid = true;
+	} else if (vlan->vid && p->bridge_pvid.vid == vlan->vid) {
+		/* The old pvid was reinstalled as a non-pvid VLAN */
+		p->bridge_pvid.valid = false;
+	}
+
+	return ksz_commit_pvid(ds, port);
 }
 
 static int ksz_port_vlan_del(struct dsa_switch *ds, int port,
 			     const struct switchdev_obj_port_vlan *vlan)
 {
 	struct ksz_device *dev = ds->priv;
+	struct ksz_port *p;
+	u16 pvid;
+	int ret;
+
+	p = &dev->ports[port];
 
 	if (!dev->dev_ops->vlan_del)
 		return -EOPNOTSUPP;
 
-	return dev->dev_ops->vlan_del(dev, port, vlan);
+	ksz_get_pvid(dev, port, &pvid);
+
+	ret = dev->dev_ops->vlan_del(dev, port, vlan);
+	if (ret)
+		return ret;
+
+	if (vlan->vid == pvid) {
+		p->bridge_pvid.valid = false;
+
+		ret = ksz_commit_pvid(ds, port);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
 }
 
 static int ksz_port_mirror_add(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 3bcd4e20bfaa..3d7eb170e3ec 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -15,6 +15,7 @@
 #include <net/dsa.h>
 
 #define KSZ_MAX_NUM_PORTS 8
+#define KSZ_DEFAULT_VLAN			(VLAN_N_VID - 1)
 
 struct vlan_table {
 	u32 table[3];
@@ -63,6 +64,11 @@ struct ksz_chip_data {
 	bool internal_phy[KSZ_MAX_NUM_PORTS];
 };
 
+struct ksz_vlan {
+	u16	vid;
+	bool	valid;
+};
+
 struct ksz_port {
 	bool remove_tag;		/* Remove Tag flag set, for ksz8795 only */
 	int stp_state;
@@ -81,6 +87,7 @@ struct ksz_port {
 	u16 max_frame;
 	u32 rgmii_tx_val;
 	u32 rgmii_rx_val;
+	struct ksz_vlan bridge_pvid;
 };
 
 struct ksz_device {
@@ -271,6 +278,7 @@ struct ksz_dev_ops {
 	int  (*vlan_add)(struct ksz_device *dev, int port, u16 vid, u16 flags);
 	int  (*vlan_del)(struct ksz_device *dev, int port,
 			 const struct switchdev_obj_port_vlan *vlan);
+	void (*drop_untagged)(struct ksz_device *dev, int port, bool untagged);
 	int (*mirror_add)(struct ksz_device *dev, int port,
 			  struct dsa_mall_mirror_tc_entry *mirror,
 			  bool ingress, struct netlink_ext_ack *extack);
@@ -320,8 +328,6 @@ void ksz_port_stp_state_set(struct dsa_switch *ds, int port, u8 state);
 bool ksz_get_gbit(struct ksz_device *dev, int port);
 phy_interface_t ksz_get_xmii(struct ksz_device *dev, int port, bool gbit);
 extern const struct ksz_chip_data ksz_switch_chips[];
-void ksz_get_pvid(struct ksz_device *dev, int port, u16 *pvid);
-void ksz_set_pvid(struct ksz_device *dev, int port, u16 pvid);
 
 /* Common register access functions */
 
-- 
2.36.1

Re: [Patch RFC net-next 1/4] net: dsa: microchip: modify vlan_add function prototype

[Vladimir Oltean]

On Fri, Jul 29, 2022 at 08:47:30PM +0530, Arun Ramadoss wrote:
> diff --git a/drivers/net/dsa/microchip/ksz8.h b/drivers/net/dsa/microchip/ksz8.h
> index 42c50cc4d853..6529f2e2426a 100644
> --- a/drivers/net/dsa/microchip/ksz8.h
> +++ b/drivers/net/dsa/microchip/ksz8.h
> @@ -38,9 +38,8 @@ int ksz8_mdb_del(struct ksz_device *dev, int port,
>  		 const struct switchdev_obj_port_mdb *mdb, struct dsa_db db);
>  int ksz8_port_vlan_filtering(struct ksz_device *dev, int port, bool flag,
>  			     struct netlink_ext_ack *extack);
> -int ksz8_port_vlan_add(struct ksz_device *dev, int port,
> -		       const struct switchdev_obj_port_vlan *vlan,
> -		       struct netlink_ext_ack *extack);
> +int ksz8_port_vlan_add(struct ksz_device *dev, int port, u16 vlan_vid,

I don't see an impediment to naming "vlan_vid" just "vid".

> +		       u16 flags);
>  int ksz8_port_vlan_del(struct ksz_device *dev, int port,
>  		       const struct switchdev_obj_port_vlan *vlan);

Won't you convert vlan_del too?

Re: [Patch RFC net-next 2/4] net: dsa: microchip: lan937x: remove vlan_filtering_is_global flag

[Vladimir Oltean]

On Fri, Jul 29, 2022 at 08:47:31PM +0530, Arun Ramadoss wrote:
> To have the similar implementation among the ksz switches, removed the
> vlan_filtering_is_global flag which is only present in the lan937x.
> 
> Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> ---
>  drivers/net/dsa/microchip/lan937x_main.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c
> index daedd2bf20c1..9c1fe38efd1a 100644
> --- a/drivers/net/dsa/microchip/lan937x_main.c
> +++ b/drivers/net/dsa/microchip/lan937x_main.c
> @@ -401,11 +401,6 @@ int lan937x_setup(struct dsa_switch *ds)
>  		return ret;
>  	}
>  
> -	/* The VLAN aware is a global setting. Mixed vlan
> -	 * filterings are not supported.
> -	 */
> -	ds->vlan_filtering_is_global = true;
> -

You understand what this flag does, right? It ensures that if you have
lan0 and lan1 under VLAN-aware br0, then lan2 which is standalone will
declare NETIF_F_HW_VLAN_CTAG_FILTER. In turn, this makes the network
stack know that lan2 won't accept VLAN-tagged packets unless there is an
8021q interface with the given VID on top of it. This 8021q interface
calls vlan_vid_add() to populate the driver's VLAN RX filter with its
VID, and this gets translated into dsa_slave_vlan_rx_add_vid() which
ultimately reaches the driver's ->port_vlan_add() function.

If VLAN filtering *is* a global setting, and looking at this call from
ksz9477_port_vlan_filtering() which is not per port, I'd say it is:

		ksz_cfg(dev, REG_SW_LUE_CTRL_0, SW_VLAN_ENABLE, true);

then what would happen is that all VLAN tagged traffic would be dropped
on the standalone lan2.

I'd say that the ksz9477 is buggy for not declaring vlan_filtering_is_global,
rather than encouraging you to delete it from lan937x. In turn, fixing
ksz9477 would make setting this flag from a common location possible,
because ksz8 needs it too.

Re: [Patch RFC net-next 3/4] net: dsa: microchip: common ksz pvid get and set function

[Vladimir Oltean]

On Fri, Jul 29, 2022 at 08:47:32PM +0530, Arun Ramadoss wrote:
> Add the helper function for getting and setting the pvid which will be
> common for all ksz switches
> 
> Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [Patch RFC net-next 4/4] net: dsa: microchip: use private pvid for bridge_vlan_unwaware

[Vladimir Oltean]

On Fri, Jul 29, 2022 at 08:47:33PM +0530, Arun Ramadoss wrote:
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 516fb9d35c87..8a5583b1f2f4 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -161,6 +161,7 @@ static const struct ksz_dev_ops ksz8_dev_ops = {
>  	.vlan_filtering = ksz8_port_vlan_filtering,
>  	.vlan_add = ksz8_port_vlan_add,
>  	.vlan_del = ksz8_port_vlan_del,
> +	.drop_untagged = ksz8_port_enable_pvid,

You'll have to explain this one. What impact does PVID insertion on KSZ8
have upon dropping/not dropping untagged packets? This patch is saying
that when untagged packets should be dropped, PVID insertion should be
enabled, and when untagged packets should be accepted, PVID insertion
should be disabled. How come?

>  	.mirror_add = ksz8_port_mirror_add,
>  	.mirror_del = ksz8_port_mirror_del,
>  	.get_caps = ksz8_get_caps,
> @@ -187,6 +188,7 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
>  	.vlan_filtering = ksz9477_port_vlan_filtering,
>  	.vlan_add = ksz9477_port_vlan_add,
>  	.vlan_del = ksz9477_port_vlan_del,
> +	.drop_untagged = ksz9477_port_drop_untagged,
>  	.mirror_add = ksz9477_port_mirror_add,
>  	.mirror_del = ksz9477_port_mirror_del,
>  	.get_caps = ksz9477_get_caps,
> @@ -220,6 +222,7 @@ static const struct ksz_dev_ops lan937x_dev_ops = {
>  	.vlan_filtering = ksz9477_port_vlan_filtering,
>  	.vlan_add = ksz9477_port_vlan_add,
>  	.vlan_del = ksz9477_port_vlan_del,
> +	.drop_untagged = ksz9477_port_drop_untagged,
>  	.mirror_add = ksz9477_port_mirror_add,
>  	.mirror_del = ksz9477_port_mirror_del,
>  	.get_caps = lan937x_phylink_get_caps,
> @@ -1254,6 +1257,9 @@ static int ksz_enable_port(struct dsa_switch *ds, int port,
>  {
>  	struct ksz_device *dev = ds->priv;
>  
> +	dev->dev_ops->vlan_add(dev, port, KSZ_DEFAULT_VLAN,
> +			       BRIDGE_VLAN_INFO_UNTAGGED);
> +

How many times can this be executed before the VLAN add operation fails
due to the VLAN already being present on the port? I notice you're
ignoring the return code. Wouldn't it be better to do this at
port_setup() time instead?

(side note, the PVID for standalone mode can be added at port_setup
time. The PVID to use for bridges in VLAN-unaware mode can be allocated
at port_bridge_join time)

>  	if (!dsa_is_user_port(ds, port))
>  		return 0;
>  
> +static int ksz_commit_pvid(struct dsa_switch *ds, int port)
> +{
> +	struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct net_device *br = dsa_port_bridge_dev_get(dp);
> +	struct ksz_device *dev = ds->priv;
> +	u16 pvid = KSZ_DEFAULT_VLAN;
> +	bool drop_untagged = false;
> +	struct ksz_port *p;
> +
> +	p = &dev->ports[port];
> +
> +	if (br && br_vlan_enabled(br)) {
> +		pvid = p->bridge_pvid.vid;
> +		drop_untagged = !p->bridge_pvid.valid;
> +	}

This is better in the sense that it resolves the need for the
configure_vlan_while_not_filtering hack. But standalone and VLAN-unaware
bridge ports still share the same PVID. Even more so, standalone ports
have address learning enabled, which will poison the address database of
VLAN-unaware bridge ports (and of other standalone ports):
https://patchwork.kernel.org/project/netdevbpf/patch/20220802002636.3963025-1-vladimir.oltean@nxp.com/

Are you going to do further work in this area?

> +
> +	ksz_set_pvid(dev, port, pvid);
> +
> +	if (dev->dev_ops->drop_untagged)
> +		dev->dev_ops->drop_untagged(dev, port, drop_untagged);
> +
> +	return 0;
> +}

Re: [Patch RFC net-next 1/4] net: dsa: microchip: modify vlan_add function prototype

[Arun.Ramadoss]

Hi Vladimir,
Thanks for the comment

On Tue, 2022-08-02 at 13:32 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Fri, Jul 29, 2022 at 08:47:30PM +0530, Arun Ramadoss wrote:
> > diff --git a/drivers/net/dsa/microchip/ksz8.h
> > b/drivers/net/dsa/microchip/ksz8.h
> > index 42c50cc4d853..6529f2e2426a 100644
> > --- a/drivers/net/dsa/microchip/ksz8.h
> > +++ b/drivers/net/dsa/microchip/ksz8.h
> > @@ -38,9 +38,8 @@ int ksz8_mdb_del(struct ksz_device *dev, int
> > port,
> >                const struct switchdev_obj_port_mdb *mdb, struct
> > dsa_db db);
> >  int ksz8_port_vlan_filtering(struct ksz_device *dev, int port,
> > bool flag,
> >                            struct netlink_ext_ack *extack);
> > -int ksz8_port_vlan_add(struct ksz_device *dev, int port,
> > -                    const struct switchdev_obj_port_vlan *vlan,
> > -                    struct netlink_ext_ack *extack);
> > +int ksz8_port_vlan_add(struct ksz_device *dev, int port, u16
> > vlan_vid,
> 
> I don't see an impediment to naming "vlan_vid" just "vid".

Ok, I will name it as vid.

> 
> > +                    u16 flags);
> >  int ksz8_port_vlan_del(struct ksz_device *dev, int port,
> >                      const struct switchdev_obj_port_vlan *vlan);
> 
> Won't you convert vlan_del too?

I will change it too.

Re: [Patch RFC net-next 4/4] net: dsa: microchip: use private pvid for bridge_vlan_unwaware

[Arun.Ramadoss]

On Tue, 2022-08-02 at 13:59 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Fri, Jul 29, 2022 at 08:47:33PM +0530, Arun Ramadoss wrote:
> > diff --git a/drivers/net/dsa/microchip/ksz_common.c
> > b/drivers/net/dsa/microchip/ksz_common.c
> > index 516fb9d35c87..8a5583b1f2f4 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.c
> > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > @@ -161,6 +161,7 @@ static const struct ksz_dev_ops ksz8_dev_ops =
> > {
> >       .vlan_filtering = ksz8_port_vlan_filtering,
> >       .vlan_add = ksz8_port_vlan_add,
> >       .vlan_del = ksz8_port_vlan_del,
> > +     .drop_untagged = ksz8_port_enable_pvid,
> 
> You'll have to explain this one. What impact does PVID insertion on
> KSZ8
> have upon dropping/not dropping untagged packets? This patch is
> saying
> that when untagged packets should be dropped, PVID insertion should
> be
> enabled, and when untagged packets should be accepted, PVID insertion
> should be disabled. How come?

Its my mistake. I referred KSZ87xx datasheet but I couldn't find the
register for the dropping the untagged packet. If that is the case,
shall I remove the dropping of untagged packet feature from the ksz8
switches?

> 
> >       .mirror_add = ksz8_port_mirror_add,
> >       .mirror_del = ksz8_port_mirror_del,
> >       .get_caps = ksz8_get_caps,
> > @@ -187,6 +188,7 @@ static const struct ksz_dev_ops ksz9477_dev_ops
> > = {
> >       .vlan_filtering = ksz9477_port_vlan_filtering,
> >       .vlan_add = ksz9477_port_vlan_add,
> >       .vlan_del = ksz9477_port_vlan_del,
> > +     .drop_untagged = ksz9477_port_drop_untagged,
> >       .mirror_add = ksz9477_port_mirror_add,
> >       .mirror_del = ksz9477_port_mirror_del,
> >       .get_caps = ksz9477_get_caps,
> > @@ -220,6 +222,7 @@ static const struct ksz_dev_ops lan937x_dev_ops
> > = {
> >       .vlan_filtering = ksz9477_port_vlan_filtering,
> >       .vlan_add = ksz9477_port_vlan_add,
> >       .vlan_del = ksz9477_port_vlan_del,
> > +     .drop_untagged = ksz9477_port_drop_untagged,
> >       .mirror_add = ksz9477_port_mirror_add,
> >       .mirror_del = ksz9477_port_mirror_del,
> >       .get_caps = lan937x_phylink_get_caps,
> > @@ -1254,6 +1257,9 @@ static int ksz_enable_port(struct dsa_switch
> > *ds, int port,
> >  {
> >       struct ksz_device *dev = ds->priv;
> > 
> > +     dev->dev_ops->vlan_add(dev, port, KSZ_DEFAULT_VLAN,
> > +                            BRIDGE_VLAN_INFO_UNTAGGED);
> > +
> 
> How many times can this be executed before the VLAN add operation
> fails
> due to the VLAN already being present on the port? I notice you're
> ignoring the return code. Wouldn't it be better to do this at
> port_setup() time instead?
> 
> (side note, the PVID for standalone mode can be added at port_setup
> time. The PVID to use for bridges in VLAN-unaware mode can be
> allocated
> at port_bridge_join time)

Ok. I will add in port_bridge_join function for bridge vlan_aware
ports.

> 
> >       if (!dsa_is_user_port(ds, port))
> >               return 0;
> > 
> > +static int ksz_commit_pvid(struct dsa_switch *ds, int port)
> > +{
> > +     struct dsa_port *dp = dsa_to_port(ds, port);
> > +     struct net_device *br = dsa_port_bridge_dev_get(dp);
> > +     struct ksz_device *dev = ds->priv;
> > +     u16 pvid = KSZ_DEFAULT_VLAN;
> > +     bool drop_untagged = false;
> > +     struct ksz_port *p;
> > +
> > +     p = &dev->ports[port];
> > +
> > +     if (br && br_vlan_enabled(br)) {
> > +             pvid = p->bridge_pvid.vid;
> > +             drop_untagged = !p->bridge_pvid.valid;
> > +     }
> 
> This is better in the sense that it resolves the need for the
> configure_vlan_while_not_filtering hack. But standalone and VLAN-
> unaware
> bridge ports still share the same PVID. Even more so, standalone
> ports
> have address learning enabled, which will poison the address database
> of
> VLAN-unaware bridge ports (and of other standalone ports):
> 
https://patchwork.kernel.org/project/netdevbpf/patch/20220802002636.3963025-1-vladimir.oltean@nxp.com/
> 
> Are you going to do further work in this area?

For now, I thought I can fix the issue for bridge vlan unaware port. I
have few other patch series to be submitted like gPTP, tc commands. If
standalone port fix also needed for your patch series I can work on it
otherwise I can take up later stage.


> 
> > +
> > +     ksz_set_pvid(dev, port, pvid);
> > +
> > +     if (dev->dev_ops->drop_untagged)
> > +             dev->dev_ops->drop_untagged(dev, port,
> > drop_untagged);
> > +
> > +     return 0;
> > +}

Re: [Patch RFC net-next 2/4] net: dsa: microchip: lan937x: remove vlan_filtering_is_global flag

[Arun.Ramadoss]

Hi Vladimir,
Thanks for the comment.

On Tue, 2022-08-02 at 13:40 +0300, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Fri, Jul 29, 2022 at 08:47:31PM +0530, Arun Ramadoss wrote:
> > To have the similar implementation among the ksz switches, removed
> > the
> > vlan_filtering_is_global flag which is only present in the lan937x.
> > 
> > Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>
> > ---
> >  drivers/net/dsa/microchip/lan937x_main.c | 5 -----
> >  1 file changed, 5 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/lan937x_main.c
> > b/drivers/net/dsa/microchip/lan937x_main.c
> > index daedd2bf20c1..9c1fe38efd1a 100644
> > --- a/drivers/net/dsa/microchip/lan937x_main.c
> > +++ b/drivers/net/dsa/microchip/lan937x_main.c
> > @@ -401,11 +401,6 @@ int lan937x_setup(struct dsa_switch *ds)
> >               return ret;
> >       }
> > 
> > -     /* The VLAN aware is a global setting. Mixed vlan
> > -      * filterings are not supported.
> > -      */
> > -     ds->vlan_filtering_is_global = true;
> > -
> 
> You understand what this flag does, right? It ensures that if you
> have
> lan0 and lan1 under VLAN-aware br0, then lan2 which is standalone
> will
> declare NETIF_F_HW_VLAN_CTAG_FILTER. In turn, this makes the network
> stack know that lan2 won't accept VLAN-tagged packets unless there is
> an
> 8021q interface with the given VID on top of it. This 8021q interface
> calls vlan_vid_add() to populate the driver's VLAN RX filter with its
> VID, and this gets translated into dsa_slave_vlan_rx_add_vid() which
> ultimately reaches the driver's ->port_vlan_add() function.
> 
> If VLAN filtering *is* a global setting, and looking at this call
> from
> ksz9477_port_vlan_filtering() which is not per port, I'd say it is:
> 
>                 ksz_cfg(dev, REG_SW_LUE_CTRL_0, SW_VLAN_ENABLE,
> true);
> 
> then what would happen is that all VLAN tagged traffic would be
> dropped
> on the standalone lan2.
> 
> I'd say that the ksz9477 is buggy for not declaring
> vlan_filtering_is_global,
> rather than encouraging you to delete it from lan937x. In turn,
> fixing
> ksz9477 would make setting this flag from a common location possible,
> because ksz8 needs it too.

I have done some study on this SW_VLAN_ENABLE bit. By default the pvid
of the port is 1 and vlan port membership (0xNA04) is 0xff. So if the
bit is 0, then unknown dest addr packets are broadcasted which is the
default behaviour of switch.
Now consider when the bit is 1,
- If the invalid vlan packet is received, then based on drop invalid
vid or unknown vid forward bit, packets are discarded or forwarded.
- If the valid vlan packet is received, then broadcast to ports in vlan
port membership list.
The port membership register set using the ksz9477_cfg_port_member
function.
In summary, I infer that we can enable this bit in the port_setup and
based on the port membership packets can be forwarded. Is my
understanding correct?
Can I remove this patch from this series and submit as the separate
one?

Re: [Patch RFC net-next 2/4] net: dsa: microchip: lan937x: remove vlan_filtering_is_global flag

[Vladimir Oltean]

On Tue, Aug 02, 2022 at 04:09:35PM +0000, Arun.Ramadoss@microchip.com wrote:
> I have done some study on this SW_VLAN_ENABLE bit. By default the pvid
> of the port is 1 and vlan port membership (0xNA04) is 0xff. So if the
> bit is 0, then unknown dest addr packets are broadcasted which is the
> default behaviour of switch.
> Now consider when the bit is 1,
> - If the invalid vlan packet is received, then based on drop invalid
> vid or unknown vid forward bit, packets are discarded or forwarded.
> - If the valid vlan packet is received, then broadcast to ports in vlan
> port membership list.
> The port membership register set using the ksz9477_cfg_port_member
> function.
> In summary, I infer that we can enable this bit in the port_setup and
> based on the port membership packets can be forwarded. Is my
> understanding correct?
> Can I remove this patch from this series and submit as the separate
> one?

I'm not sure that the switch's capabilities are quite in line with the
Linux kernel expectations if you always force the 802.1Q VLAN Enable bit
to true.

I am looking at Table 4-8: VLAN forwarding from the KSZ9563 datasheet,
and it says that when the "Unknown VID forward" bit is set and we are in
VLAN enable mode, packets are forwarded to the Unknown VID packet
forward list regardless of ALU match (which is "don't care" in this table).
In essence this is because the switch failed to resolve the unknown VID
to a FID. Other switches have a default FID for this case, but it
doesn't appear to hold true for KSZ.

The last thing you want is for packets under a VLAN-unaware bridge to be
always flooded to the ports in the Unknown VID packet forward list, and
bypass ALU lookups.

Re: [Patch RFC net-next 4/4] net: dsa: microchip: use private pvid for bridge_vlan_unwaware

[Vladimir Oltean]

On Tue, Aug 02, 2022 at 02:40:09PM +0000, Arun.Ramadoss@microchip.com wrote:
> On Tue, 2022-08-02 at 13:59 +0300, Vladimir Oltean wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > On Fri, Jul 29, 2022 at 08:47:33PM +0530, Arun Ramadoss wrote:
> > > diff --git a/drivers/net/dsa/microchip/ksz_common.c
> > > b/drivers/net/dsa/microchip/ksz_common.c
> > > index 516fb9d35c87..8a5583b1f2f4 100644
> > > --- a/drivers/net/dsa/microchip/ksz_common.c
> > > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > > @@ -161,6 +161,7 @@ static const struct ksz_dev_ops ksz8_dev_ops =
> > > {
> > >       .vlan_filtering = ksz8_port_vlan_filtering,
> > >       .vlan_add = ksz8_port_vlan_add,
> > >       .vlan_del = ksz8_port_vlan_del,
> > > +     .drop_untagged = ksz8_port_enable_pvid,
> > 
> > You'll have to explain this one. What impact does PVID insertion on KSZ8
> > have upon dropping/not dropping untagged packets? This patch is saying
> > that when untagged packets should be dropped, PVID insertion should be
> > enabled, and when untagged packets should be accepted, PVID insertion
> > should be disabled. How come?
> 
> Its my mistake. I referred KSZ87xx datasheet but I couldn't find the
> register for the dropping the untagged packet. If that is the case,
> shall I remove the dropping of untagged packet feature from the ksz8
> switches?

You'll have to see how KSZ8 behaves when the ingress port is configured
with a PVID (through REG_PORT_CTRL_VID) which isn't present in the VLAN
table. If untagged packets are dropped, that's your "drop untagged"
setting. Some other switches will not do this, and accept untagged
packets even if the VLAN table doesn't contain an entry for the PVID
(or doesn't have this port as a member of that VLAN), but have a
separate knob for dropping untagged traffic.

> > This is better in the sense that it resolves the need for the
> > configure_vlan_while_not_filtering hack. But standalone and VLAN-unaware
> > bridge ports still share the same PVID. Even more so, standalone ports
> > have address learning enabled, which will poison the address database of
> > VLAN-unaware bridge ports (and of other standalone ports):
> > 
> https://patchwork.kernel.org/project/netdevbpf/patch/20220802002636.3963025-1-vladimir.oltean@nxp.com/
> > 
> > Are you going to do further work in this area?
> 
> For now, I thought I can fix the issue for bridge vlan unaware port. I
> have few other patch series to be submitted like gPTP, tc commands. If
> standalone port fix also needed for your patch series I can work on it
> otherwise I can take up later stage.

I think the most imperative thing for you to do is to make sure you are
not introducing regressions with the port default VID change - this can
be done by running the bridge related selftests (and making them pass).

Something which I forgot to mention is that normally, I'd expect a
change of VLAN-unaware PVID to also need a change in the way that
VLAN-unaware FDB entries are added (other drivers need to remap vid=0
from port_fdb_add() to the PVID that they use for that VLAN-unaware
bridge, in your case 4095, for those FDB entries to continue matching
properly).

However, I see that currently, ksz9477_fdb_add() sets the "USE FID" bit
only for VLAN-aware FDB entries (vid != 0), which leaves me with more
questions than answers.

It isn't very well explained what it means to not use FID: let's say
there are 2 entries in the static address table, one has "USE FID"=false,
and the other has "USE FID"=true and FID=127, and a packet is received
which is classified to FID 127. On which entry will this packet match?

The bridge driver gives you all FDB entries at once (VLAN-aware and
VLAN-unaware), so if the USE_FID=false entries that the ksz9477 driver
uses for VLAN-unaware mode will shadow the VLAN-aware FDB entries, this
is going to be a problem.

Also, the way in which the ksz9477 driver translates a 12-bit VID into a
7-bit FID also has me incredibly confused (FID is vlan->vid & VLAN_FID_M,
or otherwise said, a simple truncation). This means that your
VLAN-unaware PVID of 4095 uses a FID of 127, which is also the same FID
as VLANs 127, 255, 383 etc, right? So there is potentially still full
address database leakage between VLAN-unaware and VLAN-aware bridges.

I think this phrase from the documentation is under-appreciated in
understanding how the hardware works:

| Table 4-8 details the forwarding and discarding actions that are taken
| for the various VLAN scenarios. The first entry in the table is
| explained by the fact that VLAN Table lookup is enabled even when 802.1Q
| VLAN is not enabled.

The last part ("VLAN Table lookup is enabled even when 802.1Q VLAN is
not enabled") is what makes it so that the PVID of the port must be
present in the VLAN table or otherwise you get packet drops. In turn,
if the VLAN table is being looked up, it means that regardless of
whether the switch is VLAN-unaware or not, the VID will be transformed
into a 7-bit FID.

I want that the FID that is being used for standalone ports and
VLAN-unaware bridges (127) to be a fully conscious decision, with the
implications understood, and not just something done for me to shut up.
There is a risk here that you may think things are fine and work on
other features, but things are not fine at all. And in this area,
standalone ports/bridge VLANs/ FDB entries/FIDs are very inter-related
things. When you change one, you may find that the entire scheme needs
to be re-thought.

Re: [Patch RFC net-next 3/4] net: dsa: microchip: common ksz pvid get and set function

[Florian Fainelli]

On 7/29/2022 8:17 AM, Arun Ramadoss wrote:
> Add the helper function for getting and setting the pvid which will be
> common for all ksz switches
> 
> Signed-off-by: Arun Ramadoss <arun.ramadoss@microchip.com>

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