Re: [RFC PATCH net-next 3/3] net: dsa: never skip VLAN configuration

[<Arun.Ramadoss@microchip.com>]

Hi Vladimir,
On Mon, 2022-07-18 at 16:24 +0000, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Hi Arun,
> 
> On Mon, Jul 18, 2022 at 02:34:33PM +0000, Arun.Ramadoss@microchip.com
>  wrote:
> > Hi Vladimir,
> > There was a mistake in our testing on the latest code base of net-
> > next.
> > Today we tried in the latest net-next and following are the
> > observation.
> > 
> > Scenario 1: Before applying the patch
> > ------------------------------
> > ip link set dev br0 type bridge vlan_filtering 0
> > 
> > bridge vlan add vid 10 dev lan1 pvid untagged
> > bridge vlan add vid 10 dev lan2 pvid untagged
> 
> "bridge vlan add" on lan2 was never part of the test instructions.
> 
> > 
> > We got warning skipping configuration of VLAN and
> > ksz_port_vlan_add()
> > is not called.
> > 
> > Packet is received in Host2 when transmitted from Host1. So there
> > is no
> > breakage in the forwarding.
> 
> Nonetheless it's good to know that the control scenario behaves as
> expected, i.e. the VLANs are skipped while VLAN-unaware (and
> therefore,
> the port hardware PVID remains what it was).
> 
> > Scenario 2: After applying the patch
> > ----------------------------
> > ip link set dev br0 type bridge vlan_filtering 0
> > 
> > bridge vlan add vid 10 dev lan1 pvid untagged
> > 
> > --> Packet is not received in the Host2
> 
> The problem is exactly here. The driver should pass packets at this
> stage.
> This is because the bridge is still VLAN-unaware, despite its VLAN-
> aware
> pvid VLAN having been changed from 1 (vlan_default_pvid) to 10.
> 
> > bridge vlan add vid 10 dev lan2 pvid untagged
> > 
> > --> packet is received in the Host2
> 
> This only goes to prove that the VLAN in which the switch processes
> traffic while VLAN-unaware is the PVID of the port. So when the PVID
> on
> the ingress and egress ports matches, forwarding is naturally
> restored.
> 
> > bridge vlan del vid 10 dev lan1
> > 
> > --> packet is received in the Host2
> > 
> > bridge vlan del vid 10 dev lan2
> > 
> > --> packet is received in the Host2
> > 
> >  * Let us know, do we need to test anything further on this.
> 
> Yes, now you need to go fix the driver :) Please read the comments
> from
> this patch and the series in general (including cover letter), they
> point to similar issues in other drivers and to commits which have
> solved them. I need the ksz driver to work properly before I can
> delete
> the configure_vlan_while_not_filtering workaround. Generally
> speaking,
> the PVID needs to be committed to hardware based on a smarter logic,
> see
> this for example (and all the places from which it is called):
> 
> static int mv88e6xxx_port_commit_pvid(struct mv88e6xxx_chip *chip,
> int port)
> {
>         struct dsa_port *dp = dsa_to_port(chip->ds, port);
>         struct net_device *br = dsa_port_bridge_dev_get(dp);
>         struct mv88e6xxx_port *p = &chip->ports[port];
>         u16 pvid = MV88E6XXX_VID_STANDALONE; // Dedicated PVID for
> standalone mode
>         bool drop_untagged = false;
>         int err;
> 
>         if (br) {
>                 if (br_vlan_enabled(br)) {
>                         pvid = p->bridge_pvid.vid; // PVID is
> inherited from bridge only if the bridge is *currently* VLAN-aware
>                         drop_untagged = !p->bridge_pvid.valid;
>                 } else {
>                         pvid = MV88E6XXX_VID_BRIDGED; // Dedicated
> PVID for VLAN-unaware bridging
>                 }
>         }
> 
>         err = mv88e6xxx_port_set_pvid(chip, port, pvid);
>         if (err)
>                 return err;
> 
>         return mv88e6xxx_port_drop_untagged(chip, port,
> drop_untagged);
> }
> 
> Additionally, DaveM has just merged some DSA documentation updates
> which
> I think are very relevant to this discussion. See the new "Address
> databases"
> chapter for a review of how things are supposed to actually work when
> done carefully:
> 
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/tree/Documentation/networking/dsa/dsa.rst#n730
> 
> Thanks!
I tried to update the ksz code and tested after applying this patch
series. Following are the observation,

Scenario 1: bridge_vlan_unaware default pvid = 1
---------------------------------
ip link set dev br0 type bridge vlan_filtering 0

--> packet is received in the Host 2                       

bridge vlan add vid 10 dev lan1 pvid untagged

--> packet is received in the Host 2

bridge vlan add vid 10 dev lan2 pvid untagged

--> packet is received in the Host 2

bridge vlan del vid 10 dev lan1

--> packet is received in the Host 2

bridge vlan del vid 10 dev lan2

--> packet is received in the Host2

Scenario 2: bridge_vlan_unaware default pvid other than 1
---------------------------------
ip link set dev br0 type bridge vlan_filtering 0

--> packet is not received in the Host 2. Only after enabling the
vlan_filtering and bridge vlan add packets are receiving.

ip link set dev br0 type bridge vlan_filtering 1
bridge vlan add vid 10 dev lan2 pvid untagged   

In summary, only for pvid 1 below patch is working. Initially I tried
with pvid 0, 21, 4095, it were not working, only for pvid 1 it is
working. Kindly suggest whether any changes to be done in patch or
testing methodology.

Updated code patch
------------------
diff --git a/drivers/net/dsa/microchip/ksz8.h
b/drivers/net/dsa/microchip/ksz8.h
index 42c50cc4d853..e8e1590a78c0 100644
--- a/drivers/net/dsa/microchip/ksz8.h
+++ b/drivers/net/dsa/microchip/ksz8.h
@@ -43,6 +43,7 @@ int ksz8_port_vlan_add(struct ksz_device *dev, int
port,
 		       struct netlink_ext_ack *extack);
 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 c79a5128235f..4f4e26001957 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,
@@ -968,8 +968,8 @@ int ksz8_port_vlan_add(struct ksz_device *dev, int
port,
 {
 	bool untagged = vlan->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;
@@ -1016,36 +1016,18 @@ int ksz8_port_vlan_add(struct ksz_device *dev,
int port,
 	ksz8_to_vlan(dev, fid, member, valid, &data);
 	ksz8_w_vlan_table(dev, vlan->vid, data);
 
-	/* change PVID */
-	if (vlan->flags & BRIDGE_VLAN_INFO_PVID)
-		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);
-
-		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_pread16(dev, port, REG_PORT_CTRL_VID, &pvid);
-	pvid = pvid & 0xFFF;
-
 	ksz8_r_vlan_table(dev, vlan->vid, &data);
 	ksz8_from_vlan(dev, data, &fid, &member, &valid);
 
@@ -1060,9 +1042,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 4b14d80d27ed..6a39d6893142 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)
 {
@@ -399,10 +405,6 @@ int ksz9477_port_vlan_add(struct ksz_device *dev,
int port,
 		return err;
 	}
 
-	/* change PVID */
-	if (vlan->flags & BRIDGE_VLAN_INFO_PVID)
-		ksz_pwrite16(dev, port, REG_PORT_DEFAULT_VID, vlan-
>vid);
-
 	return 0;
 }
 
@@ -411,10 +413,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_pread16(dev, port, REG_PORT_DEFAULT_VID, &pvid);
-	pvid = pvid & 0xFFF;
 
 	if (ksz9477_get_vlan_table(dev, vlan->vid, vlan_table)) {
 		dev_dbg(dev->dev, "Failed to get vlan table\n");
@@ -423,9 +421,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);
 
@@ -434,8 +429,6 @@ int ksz9477_port_vlan_del(struct ksz_device *dev,
int port,
 		return -ETIMEDOUT;
 	}
 
-	ksz_pwrite16(dev, port, REG_PORT_DEFAULT_VID, pvid);
-
 	return 0;
 }
 
diff --git a/drivers/net/dsa/microchip/ksz9477.h
b/drivers/net/dsa/microchip/ksz9477.h
index cd278b307b3c..005c510ee6c2 100644
--- a/drivers/net/dsa/microchip/ksz9477.h
+++ b/drivers/net/dsa/microchip/ksz9477.h
@@ -30,6 +30,7 @@ int ksz9477_port_vlan_add(struct ksz_device *dev, int
port,
 			  struct netlink_ext_ack *extack);
 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 ed7d137cba99..8711be6155e6 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>
@@ -160,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,
@@ -186,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,
@@ -219,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,
@@ -258,6 +262,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 +335,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 +378,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[] = {
@@ -1333,38 +1340,120 @@ static enum dsa_tag_protocol
ksz_get_tag_protocol(struct dsa_switch *ds,
 	return proto;
 }
 
+static 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;
+}
+
+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;
+	bool drop_untagged = false;
+	struct ksz_port *p;
+	u16 pvid = 1;       /* bridge vlan unaware pvid */
+
+	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, extack);
+	ret = dev->dev_ops->vlan_add(dev, port, vlan, extack);
+	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 764ada3a0f42..c583cda6cc3d 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -63,6 +63,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 +86,7 @@ struct ksz_port {
 	u16 max_frame;
 	u32 rgmii_tx_val;
 	u32 rgmii_rx_val;
+	struct ksz_vlan bridge_pvid;
 };
 
 struct ksz_device {
@@ -175,6 +181,7 @@ enum ksz_regs {
 	S_MULTICAST_CTRL,
 	P_XMII_CTRL_0,
 	P_XMII_CTRL_1,
+	P_DEFAULT_PVID,
 };
 
 enum ksz_masks {
@@ -272,6 +279,7 @@ struct ksz_dev_ops {
 			 struct netlink_ext_ack *extack);
 	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);
@@ -434,6 +442,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