[PATCH net-next 0/8] RTL8366(RB) cleanups part 1

This is a first set of patches making the RTL8366RB work out of
the box with a default OpenWrt userspace.

We achieve bridge port isolation with the first patch, and the
next 7 patches removes the very weird VLAN set-up with one
VLAN with PVID per port that has been in this driver in all
vendor trees and in OpenWrt for years.

The switch is now managed the way a modern bridge/DSA switch
shall be managed.

After these patches are merged, I will send the next set which
adds new features, some which have circulated before.

DENG Qingfang (1):
  net: dsa: rtl8366rb: Support bridge offloading

Linus Walleij (7):
  net: dsa: rtl8366: Drop custom VLAN set-up
  net: dsa: rtl8366rb: Rewrite weird VLAN filering enablement
  net: dsa: rtl8366rb: Always treat VLAN 0 as untagged
  net: dsa: rtl8366: Disable "4K" VLANs
  net: dsa: rtl8366rb: Fix off-by-one bug
  net: dsa: rtl8366: Fix a bug in deleting VLANs
  net: dsa: rtl8366: Drop and depromote pointless prints

 drivers/net/dsa/realtek-smi-core.h |   3 -
 drivers/net/dsa/rtl8366.c          | 113 ++++---------------------
 drivers/net/dsa/rtl8366rb.c        | 128 ++++++++++++++++++++++++++---
 3 files changed, 135 insertions(+), 109 deletions(-)

-- 
2.31.1

From: DENG Qingfang <dqfext@gmail.com>

Use port isolation registers to configure bridge offloading.

Tested on the D-Link DIR-685, switching between ports and
sniffing ports to make sure no packets leak.

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <redacted>
Reviewed-by: Alvin Šipraga <redacted>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <redacted>
---
ChangeLog v3->v4:
- Fix a bug where I managed to mask off the CPU port
  from the ports we could access leading to numb
  bridge.
- Reword some comments.
ChangeLog v2->v3:
- Parens around the (pmask) in the port isolation macro.
- Do not exit join/leave functions on regmap failures,
  print an error and continue.
- Clarify comments around the port in join/leave
  functions.
ChangeLog v1->v2:
- introduce RTL8366RB_PORT_ISO_PORTS() to shift the port
  mask into place so we are not confused by the enable
  bit.
- Use this with dsa_user_ports() to isolate the CPU port
  from itself.
---
 drivers/net/dsa/rtl8366rb.c | 86 +++++++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index a89093bc6c6a..b930050cfd1b 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c

@@ -300,6 +300,13 @@
 #define RTL8366RB_INTERRUPT_STATUS_REG	0x0442
 #define RTL8366RB_NUM_INTERRUPT		14 /* 0..13 */
 
+/* Port isolation registers */
+#define RTL8366RB_PORT_ISO_BASE		0x0F08
+#define RTL8366RB_PORT_ISO(pnum)	(RTL8366RB_PORT_ISO_BASE + (pnum))
+#define RTL8366RB_PORT_ISO_EN		BIT(0)
+#define RTL8366RB_PORT_ISO_PORTS_MASK	GENMASK(7, 1)
+#define RTL8366RB_PORT_ISO_PORTS(pmask)	((pmask) << 1)
+
 /* bits 0..5 enable force when cleared */
 #define RTL8366RB_MAC_FORCE_CTRL_REG	0x0F11
 

@@ -835,6 +842,21 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	/* Isolate all user ports so they can only send packets to itself and the CPU port */
+	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
+		ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(i),
+				   RTL8366RB_PORT_ISO_PORTS(BIT(RTL8366RB_PORT_NUM_CPU)) |
+				   RTL8366RB_PORT_ISO_EN);
+		if (ret)
+			return ret;
+	}
+	/* CPU port can send packets to all ports */
+	ret = regmap_write(smi->map, RTL8366RB_PORT_ISO(RTL8366RB_PORT_NUM_CPU),
+			   RTL8366RB_PORT_ISO_PORTS(dsa_user_ports(ds)) |
+			   RTL8366RB_PORT_ISO_EN);
+	if (ret)
+		return ret;
+
 	/* Set up the "green ethernet" feature */
 	ret = rtl8366rb_jam_table(rtl8366rb_green_jam,
 				  ARRAY_SIZE(rtl8366rb_green_jam), smi, false);

@@ -1127,6 +1149,68 @@ rtl8366rb_port_disable(struct dsa_switch *ds, int port)
 	rb8366rb_set_port_led(smi, port, false);
 }
 
+static int
+rtl8366rb_port_bridge_join(struct dsa_switch *ds, int port,
+			   struct net_device *bridge)
+{
+	struct realtek_smi *smi = ds->priv;
+	unsigned int port_bitmap = 0;
+	int ret, i;
+
+	/* Loop over all other ports than the current one */
+	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
+		/* Current port handled last */
+		if (i == port)
+			continue;
+		/* Not on this bridge */
+		if (dsa_to_port(ds, i)->bridge_dev != bridge)
+			continue;
+		/* Join this port to each other port on the bridge */
+		ret = regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(i),
+					 RTL8366RB_PORT_ISO_PORTS(BIT(port)),
+					 RTL8366RB_PORT_ISO_PORTS(BIT(port)));
+		if (ret)
+			dev_err(smi->dev, "failed to join port %d\n", port);
+
+		port_bitmap |= BIT(i);
+	}
+
+	/* Set the bits for the ports we can access */
+	return regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(port),
+				  RTL8366RB_PORT_ISO_PORTS(port_bitmap),
+				  RTL8366RB_PORT_ISO_PORTS(port_bitmap));
+}
+
+static void
+rtl8366rb_port_bridge_leave(struct dsa_switch *ds, int port,
+			    struct net_device *bridge)
+{
+	struct realtek_smi *smi = ds->priv;
+	unsigned int port_bitmap = 0;
+	int ret, i;
+
+	/* Loop over all other ports than this one */
+	for (i = 0; i < RTL8366RB_PORT_NUM_CPU; i++) {
+		/* Current port handled last */
+		if (i == port)
+			continue;
+		/* Not on this bridge */
+		if (dsa_to_port(ds, i)->bridge_dev != bridge)
+			continue;
+		/* Remove this port from any other port on the bridge */
+		ret = regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(i),
+					 RTL8366RB_PORT_ISO_PORTS(BIT(port)), 0);
+		if (ret)
+			dev_err(smi->dev, "failed to leave port %d\n", port);
+
+		port_bitmap |= BIT(i);
+	}
+
+	/* Clear the bits for the ports we can not access, leave ourselves */
+	regmap_update_bits(smi->map, RTL8366RB_PORT_ISO(port),
+			   RTL8366RB_PORT_ISO_PORTS(port_bitmap), 0);
+}
+
 static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
 {
 	struct realtek_smi *smi = ds->priv;

@@ -1510,6 +1594,8 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = {
 	.get_strings = rtl8366_get_strings,
 	.get_ethtool_stats = rtl8366_get_ethtool_stats,
 	.get_sset_count = rtl8366_get_sset_count,
+	.port_bridge_join = rtl8366rb_port_bridge_join,
+	.port_bridge_leave = rtl8366rb_port_bridge_leave,
 	.port_vlan_filtering = rtl8366_vlan_filtering,
 	.port_vlan_add = rtl8366_vlan_add,
 	.port_vlan_del = rtl8366_vlan_del,

-- 
2.31.1

This hacky default VLAN setup was done in order to direct
packets to the right ports and provide port isolation, both
which we now support properly using custom tags and proper
bridge port isolation.

We can drop the custom VLAN code and leave all VLAN handling
alone, as users expect things to be. We can also drop
ds->configure_vlan_while_not_filtering = false; and let
the core deal with any VLANs it wants.

Cc: Mauri Sandberg <redacted>
Cc: DENG Qingfang <dqfext@gmail.com>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
Reviewed-by: Alvin Šipraga <redacted>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Linus Walleij <redacted>
---
ChangeLog v3->v4:
- No changes
ChangeLog v2->v3:
- Collect a bunch of reviewed-by tags
ChangeLog v1->v2:
- No changes.
---
 drivers/net/dsa/realtek-smi-core.h |  1 -
 drivers/net/dsa/rtl8366.c          | 48 ------------------------------
 drivers/net/dsa/rtl8366rb.c        |  4 +--
 3 files changed, 1 insertion(+), 52 deletions(-)

diff --git a/drivers/net/dsa/realtek-smi-core.h b/drivers/net/dsa/realtek-smi-core.h
index fcf465f7f922..c8fbd7b9fd0b 100644
--- a/drivers/net/dsa/realtek-smi-core.h
+++ b/drivers/net/dsa/realtek-smi-core.h

@@ -129,7 +129,6 @@ int rtl8366_set_pvid(struct realtek_smi *smi, unsigned int port,
 int rtl8366_enable_vlan4k(struct realtek_smi *smi, bool enable);
 int rtl8366_enable_vlan(struct realtek_smi *smi, bool enable);
 int rtl8366_reset_vlan(struct realtek_smi *smi);
-int rtl8366_init_vlan(struct realtek_smi *smi);
 int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
 			   struct netlink_ext_ack *extack);
 int rtl8366_vlan_add(struct dsa_switch *ds, int port,

diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index 75897a369096..59c5bc4f7b71 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c

@@ -292,54 +292,6 @@ int rtl8366_reset_vlan(struct realtek_smi *smi)
 }
 EXPORT_SYMBOL_GPL(rtl8366_reset_vlan);
 
-int rtl8366_init_vlan(struct realtek_smi *smi)
-{
-	int port;
-	int ret;
-
-	ret = rtl8366_reset_vlan(smi);
-	if (ret)
-		return ret;
-
-	/* Loop over the available ports, for each port, associate
-	 * it with the VLAN (port+1)
-	 */
-	for (port = 0; port < smi->num_ports; port++) {
-		u32 mask;
-
-		if (port == smi->cpu_port)
-			/* For the CPU port, make all ports members of this
-			 * VLAN.
-			 */
-			mask = GENMASK((int)smi->num_ports - 1, 0);
-		else
-			/* For all other ports, enable itself plus the
-			 * CPU port.
-			 */
-			mask = BIT(port) | BIT(smi->cpu_port);
-
-		/* For each port, set the port as member of VLAN (port+1)
-		 * and untagged, except for the CPU port: the CPU port (5) is
-		 * member of VLAN 6 and so are ALL the other ports as well.
-		 * Use filter 0 (no filter).
-		 */
-		dev_info(smi->dev, "VLAN%d port mask for port %d, %08x\n",
-			 (port + 1), port, mask);
-		ret = rtl8366_set_vlan(smi, (port + 1), mask, mask, 0);
-		if (ret)
-			return ret;
-
-		dev_info(smi->dev, "VLAN%d port %d, PVID set to %d\n",
-			 (port + 1), port, (port + 1));
-		ret = rtl8366_set_pvid(smi, port, (port + 1));
-		if (ret)
-			return ret;
-	}
-
-	return rtl8366_enable_vlan(smi, true);
-}
-EXPORT_SYMBOL_GPL(rtl8366_init_vlan);
-
 int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
 			   struct netlink_ext_ack *extack)
 {

diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index b930050cfd1b..a5b7d7ff8884 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c

@@ -985,7 +985,7 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 			return ret;
 	}
 
-	ret = rtl8366_init_vlan(smi);
+	ret = rtl8366_reset_vlan(smi);
 	if (ret)
 		return ret;
 

@@ -999,8 +999,6 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 		return -ENODEV;
 	}
 
-	ds->configure_vlan_while_not_filtering = false;
-
 	return 0;
 }
 

-- 
2.31.1

While we were defining one VLAN per port for isolating the ports
the port_vlan_filtering() callback was implemented to enable a
VLAN on the port + 1. This function makes no sense, not only is
it incomplete as it only enables the VLAN, it doesn't do what
the callback is supposed to do, which is to selectively enable
and disable filtering on a certain port.

Implement the correct callback: we have two registers dealing
with filtering on the RTL9366RB, so we implement an ASIC-specific
callback, describe these and activate both.

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <redacted>
Cc: Alvin Šipraga <redacted>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <redacted>
---
ChangeLog v1->v4:
- New patch after discovering that this weirdness of mine is
  causing problems.
---
 drivers/net/dsa/realtek-smi-core.h |  2 --
 drivers/net/dsa/rtl8366.c          | 35 -----------------------------
 drivers/net/dsa/rtl8366rb.c        | 36 +++++++++++++++++++++++++-----
 3 files changed, 30 insertions(+), 43 deletions(-)

diff --git a/drivers/net/dsa/realtek-smi-core.h b/drivers/net/dsa/realtek-smi-core.h
index c8fbd7b9fd0b..214f710d7dd5 100644
--- a/drivers/net/dsa/realtek-smi-core.h
+++ b/drivers/net/dsa/realtek-smi-core.h

@@ -129,8 +129,6 @@ int rtl8366_set_pvid(struct realtek_smi *smi, unsigned int port,
 int rtl8366_enable_vlan4k(struct realtek_smi *smi, bool enable);
 int rtl8366_enable_vlan(struct realtek_smi *smi, bool enable);
 int rtl8366_reset_vlan(struct realtek_smi *smi);
-int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
-			   struct netlink_ext_ack *extack);
 int rtl8366_vlan_add(struct dsa_switch *ds, int port,
 		     const struct switchdev_obj_port_vlan *vlan,
 		     struct netlink_ext_ack *extack);

diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index 59c5bc4f7b71..0672dd56c698 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c

@@ -292,41 +292,6 @@ int rtl8366_reset_vlan(struct realtek_smi *smi)
 }
 EXPORT_SYMBOL_GPL(rtl8366_reset_vlan);
 
-int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering,
-			   struct netlink_ext_ack *extack)
-{
-	struct realtek_smi *smi = ds->priv;
-	struct rtl8366_vlan_4k vlan4k;
-	int ret;
-
-	/* Use VLAN nr port + 1 since VLAN0 is not valid */
-	if (!smi->ops->is_vlan_valid(smi, port + 1))
-		return -EINVAL;
-
-	dev_info(smi->dev, "%s filtering on port %d\n",
-		 vlan_filtering ? "enable" : "disable",
-		 port);
-
-	/* TODO:
-	 * The hardware support filter ID (FID) 0..7, I have no clue how to
-	 * support this in the driver when the callback only says on/off.
-	 */
-	ret = smi->ops->get_vlan_4k(smi, port + 1, &vlan4k);
-	if (ret)
-		return ret;
-
-	/* Just set the filter to FID 1 for now then */
-	ret = rtl8366_set_vlan(smi, port + 1,
-			       vlan4k.member,
-			       vlan4k.untag,
-			       1);
-	if (ret)
-		return ret;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(rtl8366_vlan_filtering);
-
 int rtl8366_vlan_add(struct dsa_switch *ds, int port,
 		     const struct switchdev_obj_port_vlan *vlan,
 		     struct netlink_ext_ack *extack)

diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index a5b7d7ff8884..6c35e1ed49aa 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c

@@ -143,6 +143,8 @@
 #define RTL8366RB_PHY_NO_OFFSET			9
 #define RTL8366RB_PHY_NO_MASK			(0x1f << 9)
 
+/* VLAN Ingress Control Register */
+#define RTL8366RB_VLAN_INGRESS_CTRL1_REG	0x037E
 #define RTL8366RB_VLAN_INGRESS_CTRL2_REG	0x037f
 
 /* LED control registers */

@@ -933,11 +935,13 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
-	/* Discard VLAN tagged packets if the port is not a member of
-	 * the VLAN with which the packets is associated.
-	 */
+	/* Accept all packets by default, we enable filtering on-demand */
+	ret = regmap_write(smi->map, RTL8366RB_VLAN_INGRESS_CTRL1_REG,
+			   0);
+	if (ret)
+		return ret;
 	ret = regmap_write(smi->map, RTL8366RB_VLAN_INGRESS_CTRL2_REG,
-			   RTL8366RB_PORT_ALL);
+			   0);
 	if (ret)
 		return ret;
 

@@ -1209,6 +1213,26 @@ rtl8366rb_port_bridge_leave(struct dsa_switch *ds, int port,
 			   RTL8366RB_PORT_ISO_PORTS(port_bitmap), 0);
 }
 
+static int rtl8366rb_vlan_filtering(struct dsa_switch *ds, int port,
+				    bool vlan_filtering,
+				    struct netlink_ext_ack *extack)
+{
+	struct realtek_smi *smi = ds->priv;
+	int ret;
+
+	dev_dbg(smi->dev, "port %d: %s VLAN filtering\n", port,
+		vlan_filtering ? "enable" : "disable");
+
+	/* Any incoming frames without VID (untagged) will be dropped */
+	ret = regmap_update_bits(smi->map, RTL8366RB_VLAN_INGRESS_CTRL1_REG,
+				 BIT(port), vlan_filtering ? BIT(port) : 0);
+	if (ret)
+		return ret;
+	/* If the port is not in the member set, the frame will be dropped */
+	return regmap_update_bits(smi->map, RTL8366RB_VLAN_INGRESS_CTRL2_REG,
+				  BIT(port), vlan_filtering ? BIT(port) : 0);
+}
+
 static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
 {
 	struct realtek_smi *smi = ds->priv;

@@ -1437,7 +1461,7 @@ static bool rtl8366rb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan)
 	if (smi->vlan4k_enabled)
 		max = RTL8366RB_NUM_VIDS - 1;
 
-	if (vlan == 0 || vlan > max)
+	if (vlan > max)
 		return false;
 
 	return true;

@@ -1594,7 +1618,7 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = {
 	.get_sset_count = rtl8366_get_sset_count,
 	.port_bridge_join = rtl8366rb_port_bridge_join,
 	.port_bridge_leave = rtl8366rb_port_bridge_leave,
-	.port_vlan_filtering = rtl8366_vlan_filtering,
+	.port_vlan_filtering = rtl8366rb_vlan_filtering,
 	.port_vlan_add = rtl8366_vlan_add,
 	.port_vlan_del = rtl8366_vlan_del,
 	.port_enable = rtl8366rb_port_enable,

-- 
2.31.1

VLAN 0 shall always be treated as untagged, as per example
from other drivers (I guess from the spec).

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <redacted>
Cc: Alvin Šipraga <redacted>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <redacted>
---
ChangeLog v1->v4:
- New patch after noting that other drivers always sets VLAN 0
  as untagged.
---
 drivers/net/dsa/rtl8366.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index 0672dd56c698..fae14c448fe4 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c

@@ -308,6 +308,11 @@ int rtl8366_vlan_add(struct dsa_switch *ds, int port,
 		return -EINVAL;
 	}
 
+	/* Note that VLAN 0 shall always be treated as untagged */
+	if (vlan->vid == 0)
+		untagged = true;
+
+
 	/* Enable VLAN in the hardware
 	 * FIXME: what's with this 4k business?
 	 * Just rtl8366_enable_vlan() seems inconclusive.

-- 
2.31.1

I have to disable this feature to have working VLANs on the
RTL8366RB at least, probably on all of them.

It appears that the very custom VLAN set-up was using this
feature by setting up one VLAN per port for a reason: when
using "4K" VLAN, every frame transmitted by the switch
MUST have a VLAN tag.

This is the reason that every port had its own VLAN,
including the CPU port, and all of them had PVID turned on:
this way every frame going in or out of the switch will
indeed have a VLAN tag.

However the way Linux userspace like to use VLANs such as
by default assigning all ports on a bridge to the same VLAN
this does not work at all because PVID is not set for these,
and all packets get lost.

Therefore we have to do with 16 VLAN for now, the "4K"
4096 VLAN feature is clearly only for switches in
environments where everything is a VLAN.

This was discovered when testing with OpenWrt that join
the LAN ports lan0 ... lan3 into a bridge and then assign
each of them into VLAN 1 with PVID set on each port: without
this patch this will not work and the bridge goes numb.

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <redacted>
Cc: Alvin Šipraga <redacted>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <redacted>
---
ChangeLog v1->v4:
- New patch after discovering that the VLAN configuration in
  OpenWrt was not working.
---
 drivers/net/dsa/rtl8366.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index fae14c448fe4..9652323167c2 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c

@@ -313,13 +313,15 @@ int rtl8366_vlan_add(struct dsa_switch *ds, int port,
 		untagged = true;
 
 
-	/* Enable VLAN in the hardware
-	 * FIXME: what's with this 4k business?
-	 * Just rtl8366_enable_vlan() seems inconclusive.
+	/* Enable VLAN in the hardware, do NOT enable VLAN4K, because the
+	 * 4K VLAN will activate a 4096 entries VID table, but has the side
+	 * effect that every processed frame MUST have a VID, meaning non-VLAN
+	 * traffic will now work at all. So we will let the 16 VLAN entries
+	 * suffice.
 	 */
-	ret = rtl8366_enable_vlan4k(smi, true);
+	ret = rtl8366_enable_vlan(smi, true);
 	if (ret) {
-		NL_SET_ERR_MSG_MOD(extack, "Failed to enable VLAN 4K");
+		NL_SET_ERR_MSG_MOD(extack, "Failed to enable VLAN");
 		return ret;
 	}
 

-- 
2.31.1

We were checking that the MC (member config) was != 0
for some reason, all we need to check is that the config
has no ports, i.e. no members. Then it can be recycled.
This must be some misunderstanding.

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <redacted>
Cc: Alvin Šipraga <redacted>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <redacted>
---
ChangeLog v1->v4:
- New patch for a bug found while fixing the other issues.
---
 drivers/net/dsa/rtl8366.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index 9652323167c2..fd725cfa18e7 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c

@@ -381,7 +381,7 @@ int rtl8366_vlan_del(struct dsa_switch *ds, int port,
 			 * anymore then clear the whole member
 			 * config so it can be reused.
 			 */
-			if (!vlanmc.member && vlanmc.untag) {
+			if (!vlanmc.member) {
 				vlanmc.vid = 0;
 				vlanmc.priority = 0;
 				vlanmc.fid = 0;

-- 
2.31.1

The max VLAN number with non-4K VLAN activated is 15, and the
range is 0..15. Not 16.

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <redacted>
Cc: Alvin Šipraga <redacted>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <redacted>
---
ChangeLog v1->v4:
- New patch for a bug discovered when fixing the other issues.
---
 drivers/net/dsa/rtl8366rb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index 6c35e1ed49aa..dfc8ef470972 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c

@@ -1456,7 +1456,7 @@ static int rtl8366rb_set_mc_index(struct realtek_smi *smi, int port, int index)
 
 static bool rtl8366rb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan)
 {
-	unsigned int max = RTL8366RB_NUM_VLANS;
+	unsigned int max = RTL8366RB_NUM_VLANS - 1;
 
 	if (smi->vlan4k_enabled)
 		max = RTL8366RB_NUM_VIDS - 1;

-- 
2.31.1

We don't need a message for every VLAN association, dbg
is fine. The message about adding the DSA or CPU
port to a VLAN is directly misleading, this is perfectly
fine.

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <redacted>
Cc: Alvin Šipraga <redacted>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <redacted>
---
ChangeLog v1->v4:
- New patch to deal with confusing messages and too talkative
  DSA bridge.
---
 drivers/net/dsa/rtl8366.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index fd725cfa18e7..cf2e9d91d62d 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c

@@ -325,12 +325,9 @@ int rtl8366_vlan_add(struct dsa_switch *ds, int port,
 		return ret;
 	}
 
-	dev_info(smi->dev, "add VLAN %d on port %d, %s, %s\n",
-		 vlan->vid, port, untagged ? "untagged" : "tagged",
-		 pvid ? " PVID" : "no PVID");
-
-	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
-		dev_err(smi->dev, "port is DSA or CPU port\n");
+	dev_dbg(smi->dev, "add VLAN %d on port %d, %s, %s\n",
+		vlan->vid, port, untagged ? "untagged" : "tagged",
+		pvid ? " PVID" : "no PVID");
 
 	member |= BIT(port);
 

@@ -363,7 +360,7 @@ int rtl8366_vlan_del(struct dsa_switch *ds, int port,
 	struct realtek_smi *smi = ds->priv;
 	int ret, i;
 
-	dev_info(smi->dev, "del VLAN %04x on port %d\n", vlan->vid, port);
+	dev_dbg(smi->dev, "del VLAN %d on port %d\n", vlan->vid, port);
 
 	for (i = 0; i < smi->num_vlan_mc; i++) {
 		struct rtl8366_vlan_mc vlanmc;

-- 
2.31.1

On Mon, Sep 13, 2021 at 04:42:57PM +0200, Linus Walleij wrote:

I have to disable this feature to have working VLANs on the
RTL8366RB at least, probably on all of them.

It appears that the very custom VLAN set-up was using this
feature by setting up one VLAN per port for a reason: when
using "4K" VLAN, every frame transmitted by the switch
MUST have a VLAN tag.

This is the reason that every port had its own VLAN,
including the CPU port, and all of them had PVID turned on:
this way every frame going in or out of the switch will
indeed have a VLAN tag.

However the way Linux userspace like to use VLANs such as
by default assigning all ports on a bridge to the same VLAN
this does not work at all because PVID is not set for these,
and all packets get lost.

Therefore we have to do with 16 VLAN for now, the "4K"
4096 VLAN feature is clearly only for switches in
environments where everything is a VLAN.

This was discovered when testing with OpenWrt that join
the LAN ports lan0 ... lan3 into a bridge and then assign
each of them into VLAN 1 with PVID set on each port: without
this patch this will not work and the bridge goes numb.

It is important to explain _why_ the switch will go "numb" and not pass
packets if the Linux bridge assigns all ports to VLAN ID 1 as pvid. It
is certainly not expected for that to happen.

The purpose of the PVID feature is specifically to classify untagged
packets to a port-based VLAN ID. So "everything is a VLAN" even for
Linux user space, not sure what you're talking about.

When the Linux bridge has the vlan_filtering attribute set to 1, the
hardware should follow suit by making untagged packets get classified to
the VLAN ID that the software bridge wants to see, on the ports that are
members of that bridge.

When the Linux bridge has the vlan_filtering attribute set to 0, the
software bridge very much ignores any VLAN tags from packets, and does
not perform any VLAN-based ingress admission checks. If the hardware
classifies all packets to a VLAN even when VLAN "filtering" (i.e.
ingress dropping on mismatch) is disabled, that is perfectly fine too,
although the software bridge doesn't care. You need to set up a private
VLAN ID for your VLAN-unaware ports, and make it the pvid on those ports,
and somehow force the hardware to classify any packet towards that pvid
on those VLAN-unaware ports, regardless of whether the packets are
untagged or 802.1Q-tagged or 802.1ad-tagged or whatever. That is simply
the way things are supposed to work.

VLAN ID 0 and 4095 are good candidates to use privately within your
driver as the pvid on VLAN-unaware ports, and you can/must manually
bring up these VLANs, since the bridge will refuse to install these
VLANs in its database.

Other VLAN IDs like the range 4000-4094 are also potentially ok as long
as you document the fact that your driver crops that range out of the
usable range of the bridge, and you make sure that no packet leaks
inside or outside of those private VLANs are possible ("attackers" could
still try to send a packet tagged with VLAN ID 4094 towards a port that
is under a VLAN-aware bridge. Since that port is VLAN-aware, it will
recognize the VLAN ID as 4094, so unless you configure that port to drop
VLAN ID 4094, it might well leak into the VLAN domain 4094 which is
privately used by your driver to ensure VLAN-unaware forwarding between
the ports of a nearby VLAN-unaware bridge.

I know there are lots of things to think about, but this patch is way
too simplistic and does not really offer solid explanations.

On Mon, Sep 13, 2021 at 04:42:56PM +0200, Linus Walleij wrote:

VLAN 0 shall always be treated as untagged, as per example
from other drivers (I guess from the spec).

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <redacted>
Cc: Alvin Šipraga <redacted>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <redacted>
---
ChangeLog v1->v4:
- New patch after noting that other drivers always sets VLAN 0
  as untagged.
---

"Other drivers" are not always a good example.

Technically speaking, IEEE 802.1Q-2018 wants switches to _preserve_ the
VID 0 found inside packets when forwarding them, but treat them the same
as untagged packets otherwise (aka classify them to the port's PVID, and
forward them according to the forwarding domain of the $(PVID) VLAN in
that bridge).

"Preserve" the VID 0 tag means "mark it as egress-tagged", so the
opposite of the change you are making.

Now, I know all too well it is not always possible to satisfy that
expectation, and we have had some back-and-forth on other drivers about
this, and ended up accepting the fact that the processing of VID 0 is
more or less broken. User space deals with that the best it can
(read as: sometimes it can't):
https://sourceforge.net/p/linuxptp/mailman/message/37318312/

But the justification given here to make VID 0 egress-untagged is pretty
weak as it is.

On Mon, Sep 13, 2021 at 04:42:58PM +0200, Linus Walleij wrote:

The max VLAN number with non-4K VLAN activated is 15, and the
range is 0..15. Not 16.

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <redacted>
Cc: Alvin Šipraga <redacted>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <redacted>
---

What is the impact of the bug? Should it be backported?

On 9/13/2021 7:42 AM, Linus Walleij wrote:

But this is not exactly what you want though, an untagged frame for 
which there is a VLAN entry should be accepted, think about the bridge's 
default_pvid. Is the code wrong or the comment wrong?

+	ret = regmap_update_bits(smi->map, RTL8366RB_VLAN_INGRESS_CTRL1_REG,
+				 BIT(port), vlan_filtering ? BIT(port) : 0);
+	if (ret)
+		return ret;
+	/* If the port is not in the member set, the frame will be dropped */

OK that part does make sense and is how it should behave.

-- 
Florian

On 9/13/2021 7:42 AM, Linus Walleij wrote:

We were checking that the MC (member config) was != 0
for some reason, all we need to check is that the config
has no ports, i.e. no members. Then it can be recycled.
This must be some misunderstanding.

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <redacted>
Cc: Alvin Šipraga <redacted>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <redacted>

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

Might be worth adding a Fixes tag?
-- 
Florian

On 9/13/2021 7:43 AM, Linus Walleij wrote:

We don't need a message for every VLAN association, dbg
is fine. The message about adding the DSA or CPU
port to a VLAN is directly misleading, this is perfectly
fine.

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <redacted>
Cc: Alvin Šipraga <redacted>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <redacted>

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

Maybe at some point we should think about moving that kind of debugging 
messages towards the DSA core, and just leave drivers with debug prints 
that track an internal state not visible to the DSA framework.
-- 
Florian

On Mon, Sep 13, 2021 at 10:35:53AM -0700, Florian Fainelli wrote:

On 9/13/2021 7:43 AM, Linus Walleij wrote:

quoted

We don't need a message for every VLAN association, dbg
is fine. The message about adding the DSA or CPU
port to a VLAN is directly misleading, this is perfectly
fine.

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <redacted>
Cc: Alvin Šipraga <redacted>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <redacted>

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

Maybe at some point we should think about moving that kind of debugging
messages towards the DSA core, and just leave drivers with debug prints that
track an internal state not visible to the DSA framework.

I have some trace points on the bridge driver and in DSA for FDB
entries, maybe something along those lines?

On 9/13/2021 10:38 AM, Vladimir Oltean wrote:

On Mon, Sep 13, 2021 at 10:35:53AM -0700, Florian Fainelli wrote:

quoted

On 9/13/2021 7:43 AM, Linus Walleij wrote:

quoted

We don't need a message for every VLAN association, dbg
is fine. The message about adding the DSA or CPU
port to a VLAN is directly misleading, this is perfectly
fine.

Cc: Vladimir Oltean <olteanv@gmail.com>
Cc: Mauri Sandberg <redacted>
Cc: Alvin Šipraga <redacted>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: DENG Qingfang <dqfext@gmail.com>
Signed-off-by: Linus Walleij <redacted>

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

Maybe at some point we should think about moving that kind of debugging
messages towards the DSA core, and just leave drivers with debug prints that
track an internal state not visible to the DSA framework.

I have some trace points on the bridge driver and in DSA for FDB
entries, maybe something along those lines?

Yes trace points would work really well, thanks!
-- 
Florian

Hi Vladimir,

first, thanks for your help and patience. I learned a lot the recent
weeks, much thanks to your questions and explanations!

On Mon, Sep 13, 2021 at 5:34 PM Vladimir Oltean [off-list ref] wrote:

quoted

This was discovered when testing with OpenWrt that join
the LAN ports lan0 ... lan3 into a bridge and then assign
each of them into VLAN 1 with PVID set on each port: without
this patch this will not work and the bridge goes numb.

It is important to explain _why_ the switch will go "numb" and not pass
packets if the Linux bridge assigns all ports to VLAN ID 1 as pvid. It
is certainly not expected for that to happen.

Yeah it is pretty weird. What happens now is that this is a regression
when using OpenWrt userspace as it sets up the VLANs like this,
but if I boot a clean system and just manually do e.g.
ifconfig lan0 169.254.1.2 netmask 255.255.255.0 up
it works fine because the default VLANs that were set up by the
driver (removed by patch 2/8) will tag all packets using PVID and
send packets on 5 ingress and 1 egress VLANs.

The purpose of the PVID feature is specifically to classify untagged
packets to a port-based VLAN ID. So "everything is a VLAN" even for
Linux user space, not sure what you're talking about.

I think what happens is that OpenWrts userspace sets VLAN 1
for all ingress ports with PVID, so all packets from ingress ports
get tagged nicely with VID 1.

But as the CPU port is hidden inside the bridge
it can't join the CPU port into that VLAN (userspace does not
know it exist I think?) and thus no packets
can go into or out of the CPU port. But you can still pass packets
between the lan ports.

When the Linux bridge has the vlan_filtering attribute set to 1, the
hardware should follow suit by making untagged packets get classified to
the VLAN ID that the software bridge wants to see, on the ports that are
members of that bridge.

This is what it does, I think.

But the "4K" VLAN feature is so strict that it will restrict also the CPU
port from this (in hardware) with no way to turn it off.

It seems the "4K" mode is a "VLAN with filtering only mode" so no
matter whether we turned on filtering or not, the CPU port
will not see any packets from any other ports unless we add also
that port (port 5) into the VLAN.

One solution I could try would be to just add the CPU port to all
VLANs by default, but .. is that right?

I suppose this would work as software will add the right
VID to the packets so they will only propagate to the right
ports anyway. It could test it.

When the Linux bridge has the vlan_filtering attribute set to 0, the
software bridge very much ignores any VLAN tags from packets, and does
not perform any VLAN-based ingress admission checks. If the hardware
classifies all packets to a VLAN even when VLAN "filtering" (i.e.
ingress dropping on mismatch) is disabled, that is perfectly fine too,

I think this is what happens in this hardware.

although the software bridge doesn't care. You need to set up a private
VLAN ID for your VLAN-unaware ports, and make it the pvid on those ports,

Would the CPU port be a VLAN-unaware port?

My problem is that in the "4K" mode, the CPU port will not see packets
from any VLAN it is not a member of.

and somehow force the hardware to classify any packet towards that pvid
on those VLAN-unaware ports, regardless of whether the packets are
untagged or 802.1Q-tagged or 802.1ad-tagged or whatever. That is simply
the way things are supposed to work.

VLAN ID 0 and 4095 are good candidates to use privately within your
driver as the pvid on VLAN-unaware ports, and you can/must manually
bring up these VLANs, since the bridge will refuse to install these
VLANs in its database.

Other VLAN IDs like the range 4000-4094 are also potentially ok as long
as you document the fact that your driver crops that range out of the
usable range of the bridge, and you make sure that no packet leaks
inside or outside of those private VLANs are possible ("attackers" could
still try to send a packet tagged with VLAN ID 4094 towards a port that
is under a VLAN-aware bridge. Since that port is VLAN-aware, it will
recognize the VLAN ID as 4094, so unless you configure that port to drop
VLAN ID 4094, it might well leak into the VLAN domain 4094 which is
privately used by your driver to ensure VLAN-unaware forwarding between
the ports of a nearby VLAN-unaware bridge.

I don't have any VLAN-unaware ports other than the CPU
port.

What happened before patch 2/8 was that it was given its own VLAN
with PVID and all other ports were assigned members of that VLAN as
well, and thus egress traffic from the CPU port could go out.

For ingress traffic, the CPU port was member of all ingress
VLANs. (Also removed by patch 2/8)

I know there are lots of things to think about, but this patch is way
too simplistic and does not really offer solid explanations.

I'm trying my best and learning along the way :)
It's fine, no hurry and let's get this right so that the other
RTL switches can look at this a good example.

Yours,
Linus Walleij

Hi Linus,

On Tue, Sep 14, 2021 at 01:20:14AM +0200, Linus Walleij wrote:

Hi Vladimir,

first, thanks for your help and patience. I learned a lot the recent
weeks, much thanks to your questions and explanations!

On Mon, Sep 13, 2021 at 5:34 PM Vladimir Oltean [off-list ref] wrote:

quoted

quoted

This was discovered when testing with OpenWrt that join
the LAN ports lan0 ... lan3 into a bridge and then assign
each of them into VLAN 1 with PVID set on each port: without
this patch this will not work and the bridge goes numb.

It is important to explain _why_ the switch will go "numb" and not pass
packets if the Linux bridge assigns all ports to VLAN ID 1 as pvid. It
is certainly not expected for that to happen.

Yeah it is pretty weird. What happens now is that this is a regression
when using OpenWrt userspace as it sets up the VLANs like this,

Were you running net-next kernel?

There have been major changes to DSA since 5.10, so you'd better test your
driver on net-next.

but if I boot a clean system and just manually do e.g.
ifconfig lan0 169.254.1.2 netmask 255.255.255.0 up
it works fine because the default VLANs that were set up by the
driver (removed by patch 2/8) will tag all packets using PVID and
send packets on 5 ingress and 1 egress VLANs.

quoted

The purpose of the PVID feature is specifically to classify untagged
packets to a port-based VLAN ID. So "everything is a VLAN" even for
Linux user space, not sure what you're talking about.

I think what happens is that OpenWrts userspace sets VLAN 1
for all ingress ports with PVID, so all packets from ingress ports
get tagged nicely with VID 1.

But as the CPU port is hidden inside the bridge
it can't join the CPU port into that VLAN (userspace does not
know it exist I think?) and thus no packets
can go into or out of the CPU port. But you can still pass packets
between the lan ports.

quoted

When the Linux bridge has the vlan_filtering attribute set to 1, the
hardware should follow suit by making untagged packets get classified to
the VLAN ID that the software bridge wants to see, on the ports that are
members of that bridge.

This is what it does, I think.

But the "4K" VLAN feature is so strict that it will restrict also the CPU
port from this (in hardware) with no way to turn it off.

It seems the "4K" mode is a "VLAN with filtering only mode" so no
matter whether we turned on filtering or not, the CPU port
will not see any packets from any other ports unless we add also
that port (port 5) into the VLAN.

One solution I could try would be to just add the CPU port to all
VLANs by default, but .. is that right?

The DSA core already adds the CPU port to VLAN members for you.
In file net/dsa/slave.c, function dsa_slave_vlan_add:
...
	err = dsa_port_vlan_add(dp, &vlan, extack);
	if (err)
		return err;

	/* We need the dedicated CPU port to be a member of the VLAN as well.
	 * Even though drivers often handle CPU membership in special ways,
	 * it doesn't make sense to program a PVID, so clear this flag.
	 */
	vlan.flags &= ~BRIDGE_VLAN_INFO_PVID;

	err = dsa_port_vlan_add(dp->cpu_dp, &vlan, extack);
	if (err)
		return err;
...

If it does not work, you may have done something wrong.

On Tue, Sep 14, 2021 at 8:29 AM DENG Qingfang [off-list ref] wrote:

quoted

Yeah it is pretty weird. What happens now is that this is a regression
when using OpenWrt userspace as it sets up the VLANs like this,

Were you running net-next kernel?

Yep just the userspace is from OpenWrt, the kernel is latest mainline.

The DSA core already adds the CPU port to VLAN members for you.
In file net/dsa/slave.c, function dsa_slave_vlan_add:

Hmmmmm I will look into this. Put in some debug prints etc.
Certainly the callbacks to the driver for doing this aren't getting
called even with v5.15-rc1 AFAICT.

Yours,
Linus Walleij