[net-next PATCH 0/5 v4] RTL8366RB DSA tagging and fixes

[net-next PATCH 0/5 v4] RTL8366RB DSA tagging and fixes

[Linus Walleij]

This adds rudimentary DSA tagging and fixes up some VLAN
issues in the RTL8366RB driver and in the RTL8366 core
in general.

This v4 fixes a single unused variable in patch 1/5.

Linus Walleij (5):
  net: dsa: tag_rtl4_a: Implement Realtek 4 byte A tag
  net: dsa: rtl8366rb: Support the CPU DSA tag
  net: dsa: rtl8366: Split out default VLAN config
  net: dsa: rtl8366: VLAN 0 as disable tagging
  net: dsa: rtl8366: Use top VLANs for default

 drivers/net/dsa/Kconfig     |   1 +
 drivers/net/dsa/rtl8366.c   | 135 +++++++++++++++++++++++----------
 drivers/net/dsa/rtl8366rb.c |  33 +++------
 include/net/dsa.h           |   2 +
 net/dsa/Kconfig             |   7 ++
 net/dsa/Makefile            |   1 +
 net/dsa/tag_rtl4_a.c        | 144 ++++++++++++++++++++++++++++++++++++
 7 files changed, 259 insertions(+), 64 deletions(-)
 create mode 100644 net/dsa/tag_rtl4_a.c

-- 
2.26.2

[net-next PATCH 1/5 v4] net: dsa: tag_rtl4_a: Implement Realtek 4 byte A tag

[Linus Walleij]

This implements the known parts of the Realtek 4 byte
tag protocol version 0xA, as found in the RTL8366RB
DSA switch.

It is designated as protocol version 0xA as a
different Realtek 4 byte tag format with protocol
version 0x9 is known to exist in the Realtek RTL8306
chips.

The tag and switch chip lacks public documentation, so
the tag format has been reverse-engineered from
packet dumps. As only ingress traffic has been available
for analysis an egress tag has not been possible to
develop (even using educated guesses about bit fields)
so this is as far as it gets. It is not known if the
switch even supports egress tagging.

Excessive attempts to figure out the egress tag format
was made. When nothing else worked, I just tried all bit
combinations with 0xannp where a is protocol and p is
port. I looped through all values several times trying
to get a response from ping, without any positive
result.

Using just these ingress tags however, the switch
functionality is vastly improved and the packets find
their way into the destination port without any
tricky VLAN configuration. On the D-Link DIR-685 the
LAN ports now come up and respond to ping without
any command line configuration so this is a real
improvement for users.

Egress packets need to be restricted to the proper
target ports using VLAN, which the RTL8366RB DSA
switch driver already sets up.

Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v3->v4:
- Drop an unused variable found with -Wunused-variable
ChangeLog v2->v3:
- Collect Andrew's review tag.
ChangeLog v1->v2:
- Drop some netdev_dbg() calls that was just littering.
- Rebase on v5.8-rc1
---
 include/net/dsa.h    |   2 +
 net/dsa/Kconfig      |   7 +++
 net/dsa/Makefile     |   1 +
 net/dsa/tag_rtl4_a.c | 130 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 140 insertions(+)
 create mode 100644 net/dsa/tag_rtl4_a.c

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 50389772c597..2b37943f09a4 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -44,6 +44,7 @@ struct phylink_link_state;
 #define DSA_TAG_PROTO_KSZ8795_VALUE		14
 #define DSA_TAG_PROTO_OCELOT_VALUE		15
 #define DSA_TAG_PROTO_AR9331_VALUE		16
+#define DSA_TAG_PROTO_RTL4_A_VALUE		17
 
 enum dsa_tag_protocol {
 	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
@@ -63,6 +64,7 @@ enum dsa_tag_protocol {
 	DSA_TAG_PROTO_KSZ8795		= DSA_TAG_PROTO_KSZ8795_VALUE,
 	DSA_TAG_PROTO_OCELOT		= DSA_TAG_PROTO_OCELOT_VALUE,
 	DSA_TAG_PROTO_AR9331		= DSA_TAG_PROTO_AR9331_VALUE,
+	DSA_TAG_PROTO_RTL4_A		= DSA_TAG_PROTO_RTL4_A_VALUE,
 };
 
 struct packet_type;
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index d5bc6ac599ef..1f9b9b11008c 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -86,6 +86,13 @@ config NET_DSA_TAG_KSZ
 	  Say Y if you want to enable support for tagging frames for the
 	  Microchip 8795/9477/9893 families of switches.
 
+config NET_DSA_TAG_RTL4_A
+	tristate "Tag driver for Realtek 4 byte protocol A tags"
+	help
+	  Say Y or M if you want to enable support for tagging frames for the
+	  Realtek switches with 4 byte protocol A tags, sich as found in
+	  the Realtek RTL8366RB.
+
 config NET_DSA_TAG_OCELOT
 	tristate "Tag driver for Ocelot family of switches"
 	select PACKING
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index 108486cfdeef..4f47b2025ff5 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_NET_DSA_TAG_DSA) += tag_dsa.o
 obj-$(CONFIG_NET_DSA_TAG_EDSA) += tag_edsa.o
 obj-$(CONFIG_NET_DSA_TAG_GSWIP) += tag_gswip.o
 obj-$(CONFIG_NET_DSA_TAG_KSZ) += tag_ksz.o
+obj-$(CONFIG_NET_DSA_TAG_RTL4_A) += tag_rtl4_a.o
 obj-$(CONFIG_NET_DSA_TAG_LAN9303) += tag_lan9303.o
 obj-$(CONFIG_NET_DSA_TAG_MTK) += tag_mtk.o
 obj-$(CONFIG_NET_DSA_TAG_OCELOT) += tag_ocelot.o
diff --git a/net/dsa/tag_rtl4_a.c b/net/dsa/tag_rtl4_a.c
new file mode 100644
index 000000000000..36af1f49fd75
--- /dev/null
+++ b/net/dsa/tag_rtl4_a.c
@@ -0,0 +1,130 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Handler for Realtek 4 byte DSA switch tags
+ * Currently only supports protocol "A" found in RTL8366RB
+ * Copyright (c) 2020 Linus Walleij <linus.walleij@linaro.org>
+ *
+ * This "proprietary tag" header looks like so:
+ *
+ * -------------------------------------------------
+ * | MAC DA | MAC SA | 0x8899 | 2 bytes tag | Type |
+ * -------------------------------------------------
+ *
+ * The 2 bytes tag form a 16 bit big endian word. The exact
+ * meaning has been guess from packet dumps from ingress
+ * frames, as no working egress traffic has been available
+ * we do not know the format of the egress tags or if they
+ * are even supported.
+ */
+
+#include <linux/etherdevice.h>
+#include <linux/bits.h>
+
+#include "dsa_priv.h"
+
+#define RTL4_A_HDR_LEN		4
+#define RTL4_A_ETHERTYPE	0x8899
+#define RTL4_A_PROTOCOL_SHIFT	12
+/*
+ * 0x1 = Realtek Remote Control protocol (RRCP)
+ * 0x2/0x3 seems to be used for loopback testing
+ * 0x9 = RTL8306 DSA protocol
+ * 0xa = RTL8366RB DSA protocol
+ */
+#define RTL4_A_PROTOCOL_RTL8366RB	0xa
+
+static struct sk_buff *rtl4a_tag_xmit(struct sk_buff *skb,
+				      struct net_device *dev)
+{
+	/*
+	 * Just let it pass thru, we don't know if it is possible
+	 * to tag a frame with the 0x8899 ethertype and direct it
+	 * to a specific port, all attempts at reverse-engineering have
+	 * ended up with the frames getting dropped.
+	 *
+	 * The VLAN set-up needs to restrict the frames to the right port.
+	 *
+	 * If you have documentation on the tagging format for RTL8366RB
+	 * (tag type A) then please contribute.
+	 */
+	return skb;
+}
+
+static struct sk_buff *rtl4a_tag_rcv(struct sk_buff *skb,
+				     struct net_device *dev,
+				     struct packet_type *pt)
+{
+	u16 protport;
+	__be16 *p;
+	u16 etype;
+	u8 *tag;
+	u8 prot;
+	u8 port;
+
+	if (unlikely(!pskb_may_pull(skb, RTL4_A_HDR_LEN)))
+		return NULL;
+
+	/* The RTL4 header has its own custom Ethertype 0x8899 and that
+	 * starts right at the beginning of the packet, after the src
+	 * ethernet addr. Apparantly skb->data always points 2 bytes in,
+	 * behind the Ethertype.
+	 */
+	tag = skb->data - 2;
+	p = (__be16 *)tag;
+	etype = ntohs(*p);
+	if (etype != RTL4_A_ETHERTYPE) {
+		/* Not custom, just pass through */
+		netdev_dbg(dev, "non-realtek ethertype 0x%04x\n", etype);
+		return skb;
+	}
+	p = (__be16 *)(tag + 2);
+	protport = ntohs(*p);
+	/* The 4 upper bits are the protocol */
+	prot = (protport >> RTL4_A_PROTOCOL_SHIFT) & 0x0f;
+	if (prot != RTL4_A_PROTOCOL_RTL8366RB) {
+		netdev_err(dev, "unknown realtek protocol 0x%01x\n", prot);
+		return NULL;
+	}
+	port = protport & 0xff;
+
+	/* Remove RTL4 tag and recalculate checksum */
+	skb_pull_rcsum(skb, RTL4_A_HDR_LEN);
+
+	/* Move ethernet DA and SA in front of the data */
+	memmove(skb->data - ETH_HLEN,
+		skb->data - ETH_HLEN - RTL4_A_HDR_LEN,
+		2 * ETH_ALEN);
+
+	skb->dev = dsa_master_find_slave(dev, 0, port);
+	if (!skb->dev) {
+		netdev_dbg(dev, "could not find slave for port %d\n", port);
+		return NULL;
+	}
+
+	skb->offload_fwd_mark = 1;
+
+	return skb;
+}
+
+static int rtl4a_tag_flow_dissect(const struct sk_buff *skb, __be16 *proto,
+				  int *offset)
+{
+	*offset = RTL4_A_HDR_LEN;
+	/* Skip past the tag and fetch the encapsulated Ethertype */
+	*proto = ((__be16 *)skb->data)[1];
+
+	return 0;
+}
+
+static const struct dsa_device_ops rtl4a_netdev_ops = {
+	.name	= "rtl4a",
+	.proto	= DSA_TAG_PROTO_RTL4_A,
+	.xmit	= rtl4a_tag_xmit,
+	.rcv	= rtl4a_tag_rcv,
+	.flow_dissect = rtl4a_tag_flow_dissect,
+	.overhead = RTL4_A_HDR_LEN,
+};
+module_dsa_tag_driver(rtl4a_netdev_ops);
+
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_RTL4_A);
-- 
2.26.2

[net-next PATCH 2/5 v4] net: dsa: rtl8366rb: Support the CPU DSA tag

[Linus Walleij]

This activates the support to use the CPU tag to properly
direct ingress traffic to the right port.

Bit 15 in register RTL8368RB_CPU_CTRL_REG can be set to
1 to disable the insertion of the CPU tag which is what
the code currently does. The bit 15 define calls this
setting RTL8368RB_CPU_INSTAG which is confusing since the
inverse meaning is implied: programmers may think that
setting this bit to 1 will *enable* inserting the tag
rather than disabling it, so rename this setting in
bit 15 to RTL8368RB_CPU_NO_TAG which is more to the
point.

After this e.g. ping works out-of-the-box with the
RTL8366RB.

Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v3->v4:
- Resend with the rest
ChangeLog v2->v3:
- Fix up the commit message.
- Collect Andrew's review tag.
ChangeLog v1->v2:
- Update the commit message to explain why we are renaming
  bit 15 in RTL8368RB_CPU_CTRL_REG.
---
 drivers/net/dsa/Kconfig     |  1 +
 drivers/net/dsa/rtl8366rb.c | 31 ++++++++-----------------------
 2 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index d0024cb30a7b..468b3c4273c5 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -70,6 +70,7 @@ config NET_DSA_QCA8K
 config NET_DSA_REALTEK_SMI
 	tristate "Realtek SMI Ethernet switch family support"
 	depends on NET_DSA
+	select NET_DSA_TAG_RTL4_A
 	select FIXED_PHY
 	select IRQ_DOMAIN
 	select REALTEK_PHY
diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index fd1977590cb4..48f1ff746799 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -109,8 +109,8 @@
 /* CPU port control reg */
 #define RTL8368RB_CPU_CTRL_REG		0x0061
 #define RTL8368RB_CPU_PORTS_MSK		0x00FF
-/* Enables inserting custom tag length/type 0x8899 */
-#define RTL8368RB_CPU_INSTAG		BIT(15)
+/* Disables inserting custom tag length/type 0x8899 */
+#define RTL8368RB_CPU_NO_TAG		BIT(15)
 
 #define RTL8366RB_SMAR0			0x0070 /* bits 0..15 */
 #define RTL8366RB_SMAR1			0x0071 /* bits 16..31 */
@@ -844,16 +844,14 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
-	/* Enable CPU port and enable inserting CPU tag
+	/* Enable CPU port with custom DSA tag 8899.
 	 *
-	 * Disabling RTL8368RB_CPU_INSTAG here will change the behaviour
-	 * of the switch totally and it will start talking Realtek RRCP
-	 * internally. It is probably possible to experiment with this,
-	 * but then the kernel needs to understand and handle RRCP first.
+	 * If you set RTL8368RB_CPU_NO_TAG (bit 15) in this registers
+	 * the custom tag is turned off.
 	 */
 	ret = regmap_update_bits(smi->map, RTL8368RB_CPU_CTRL_REG,
 				 0xFFFF,
-				 RTL8368RB_CPU_INSTAG | BIT(smi->cpu_port));
+				 BIT(smi->cpu_port));
 	if (ret)
 		return ret;
 
@@ -967,21 +965,8 @@ static enum dsa_tag_protocol rtl8366_get_tag_protocol(struct dsa_switch *ds,
 						      int port,
 						      enum dsa_tag_protocol mp)
 {
-	/* For now, the RTL switches are handled without any custom tags.
-	 *
-	 * It is possible to turn on "custom tags" by removing the
-	 * RTL8368RB_CPU_INSTAG flag when enabling the port but what it
-	 * does is unfamiliar to DSA: ethernet frames of type 8899, the Realtek
-	 * Remote Control Protocol (RRCP) start to appear on the CPU port of
-	 * the device. So this is not the ordinary few extra bytes in the
-	 * frame. Instead it appears that the switch starts to talk Realtek
-	 * RRCP internally which means a pretty complex RRCP implementation
-	 * decoding and responding the RRCP protocol is needed to exploit this.
-	 *
-	 * The OpenRRCP project (dormant since 2009) have reverse-egineered
-	 * parts of the protocol.
-	 */
-	return DSA_TAG_PROTO_NONE;
+	/* This switch uses the 4 byte protocol A Realtek DSA tag */
+	return DSA_TAG_PROTO_RTL4_A;
 }
 
 static void rtl8366rb_adjust_link(struct dsa_switch *ds, int port,
-- 
2.26.2

[net-next PATCH 3/5 v4] net: dsa: rtl8366: Split out default VLAN config

[Linus Walleij]

We loop over the ports to initialize the default VLAN
and PVID for each port. As we need to reuse the
code to reinitialize a single port, break out the
function rtl8366_set_default_vlan_and_pvid().

Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v3->v4:
- Resend with the rest
ChangeLog v2->v3:
- Rebased on Andrew's patch for the (int) compile warning
  on GENMASK(). change is carried over.
- Collect Andrew's review tag.
ChangeLog v1->v2:
- Rebased on v5.8-rc1 and other changes.
---
 drivers/net/dsa/rtl8366.c | 70 ++++++++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 27 deletions(-)

diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index 993cf3ac59d9..b907c0ed9697 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -253,6 +253,48 @@ int rtl8366_reset_vlan(struct realtek_smi *smi)
 }
 EXPORT_SYMBOL_GPL(rtl8366_reset_vlan);
 
+static int rtl8366_set_default_vlan_and_pvid(struct realtek_smi *smi,
+					     int port)
+{
+	u32 mask;
+	u16 vid;
+	int ret;
+
+	/* This is the reserved default VLAN for this port */
+	vid = port + 1;
+
+	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, "Set VLAN %04x portmask to %08x (port %d %s)\n",
+		 vid, mask, port, (port == smi->cpu_port) ?
+		 "CPU PORT and all other ports" : "and CPU port");
+	ret = rtl8366_set_vlan(smi, vid, mask, mask, 0);
+	if (ret)
+		return ret;
+
+	dev_info(smi->dev, "Set PVID %04x on port %d\n",
+		 vid, port);
+	ret = rtl8366_set_pvid(smi, port, vid);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 int rtl8366_init_vlan(struct realtek_smi *smi)
 {
 	int port;
@@ -266,33 +308,7 @@ int rtl8366_init_vlan(struct realtek_smi *smi)
 	 * 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));
+		ret = rtl8366_set_default_vlan_and_pvid(smi, port);
 		if (ret)
 			return ret;
 	}
-- 
2.26.2

[net-next PATCH 4/5 v4] net: dsa: rtl8366: VLAN 0 as disable tagging

[Linus Walleij]

The code in net/8021q/vlan.c, vlan_device_event() sets
VLAN 0 for a VLAN-capable ethernet device when it
comes up.

Since the RTL8366 DSA switches must have a VLAN and
PVID set up for any packets to come through we have
already set up default VLAN for each port as part of
bringing the switch online.

Make sure that setting VLAN 0 has the same effect
and does not try to actually tell the hardware to use
VLAN 0 on the port because that will not work.

Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v3->v4:
- Resend with the rest
ChangeLog v2->v3:
- Collected Andrew's review tag.
ChangeLog v1->v2:
- Rebased on v5.8-rc1 and other changes.
---
 drivers/net/dsa/rtl8366.c | 65 +++++++++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 13 deletions(-)

diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index b907c0ed9697..a000d458d121 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -355,15 +355,25 @@ int rtl8366_vlan_prepare(struct dsa_switch *ds, int port,
 			 const struct switchdev_obj_port_vlan *vlan)
 {
 	struct realtek_smi *smi = ds->priv;
+	u16 vid_begin = vlan->vid_begin;
+	u16 vid_end = vlan->vid_end;
 	u16 vid;
 	int ret;
 
-	for (vid = vlan->vid_begin; vid < vlan->vid_end; vid++)
+	if (vid_begin == 0) {
+		dev_info(smi->dev, "prepare VLAN 0 - ignored\n");
+		if (vid_end == 0)
+			return 0;
+		/* Skip VLAN 0 and start with VLAN 1 */
+		vid_begin = 1;
+	}
+
+	for (vid = vid_begin; vid < vid_end; vid++)
 		if (!smi->ops->is_vlan_valid(smi, vid))
 			return -EINVAL;
 
 	dev_info(smi->dev, "prepare VLANs %04x..%04x\n",
-		 vlan->vid_begin, vlan->vid_end);
+		 vid_begin, vid_end);
 
 	/* Enable VLAN in the hardware
 	 * FIXME: what's with this 4k business?
@@ -383,27 +393,46 @@ void rtl8366_vlan_add(struct dsa_switch *ds, int port,
 	bool untagged = !!(vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED);
 	bool pvid = !!(vlan->flags & BRIDGE_VLAN_INFO_PVID);
 	struct realtek_smi *smi = ds->priv;
+	u16 vid_begin = vlan->vid_begin;
+	u16 vid_end = vlan->vid_end;
 	u32 member = 0;
 	u32 untag = 0;
 	u16 vid;
 	int ret;
 
-	for (vid = vlan->vid_begin; vid < vlan->vid_end; vid++)
-		if (!smi->ops->is_vlan_valid(smi, vid))
+	if (vid_begin == 0) {
+		dev_info(smi->dev, "set VLAN 0 on port %d = default VLAN\n",
+			 port);
+		/* Set up default tagging */
+		ret = rtl8366_set_default_vlan_and_pvid(smi, port);
+		if (ret) {
+			dev_err(smi->dev,
+				"error setting default VLAN on port %d\n",
+				port);
 			return;
+		}
+		if (vid_end == 0)
+			return;
+		/* Skip VLAN 0 and start with VLAN 1 */
+		vid_begin = 1;
+	}
 
-	dev_info(smi->dev, "add VLAN on port %d, %s, %s\n",
-		 port,
-		 untagged ? "untagged" : "tagged",
-		 pvid ? " PVID" : "no PVID");
+	for (vid = vid_begin; vid < vid_end; vid++)
+		if (!smi->ops->is_vlan_valid(smi, vid))
+			return;
 
 	if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
 		dev_err(smi->dev, "port is DSA or CPU port\n");
 
-	for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid) {
+	for (vid = vid_begin; vid <= vid_end; ++vid) {
 		int pvid_val = 0;
 
-		dev_info(smi->dev, "add VLAN %04x\n", vid);
+		dev_info(smi->dev, "add VLAN %04x to port %d, %s, %s\n",
+			 vid,
+			 port,
+			 untagged ? "untagged" : "tagged",
+			 pvid ? " PVID" : "no PVID");
+
 		member |= BIT(port);
 
 		if (untagged)
@@ -437,15 +466,25 @@ int rtl8366_vlan_del(struct dsa_switch *ds, int port,
 		     const struct switchdev_obj_port_vlan *vlan)
 {
 	struct realtek_smi *smi = ds->priv;
+	u16 vid_begin = vlan->vid_begin;
+	u16 vid_end = vlan->vid_end;
 	u16 vid;
 	int ret;
 
-	dev_info(smi->dev, "del VLAN on port %d\n", port);
+	if (vid_begin == 0) {
+		dev_info(smi->dev, "remove port %d from VLAN 0 (no-op)\n",
+			 port);
+		if (vid_end == 0)
+			return 0;
+		/* Skip VLAN 0 and start with VLAN 1 */
+		vid_begin = 1;
+	}
 
-	for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid) {
+	for (vid = vid_begin; vid <= vid_end; ++vid) {
 		int i;
 
-		dev_info(smi->dev, "del VLAN %04x\n", vid);
+		dev_info(smi->dev, "remove VLAN %04x from port %d\n",
+			 vid, port);
 
 		for (i = 0; i < smi->num_vlan_mc; i++) {
 			struct rtl8366_vlan_mc vlanmc;
-- 
2.26.2

[net-next PATCH 5/5 v4] net: dsa: rtl8366: Use top VLANs for default

[Linus Walleij]

The RTL8366 DSA switches will not work unless we set
up a default VLAN for each port. We are currently using
e.g. VLAN 1..6 for a 5-port switch as default VLANs.

This is not very helpful for users, move it to allocate
the top VLANs for default instead, for example on
RTL8366RB there are 16 VLANs so instead of using
VLAN 1..6 as default use VLAN 10..15 so VLAN 1
thru VLAN 9 is available for users.

Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v3->v4:
- Resend with the rest
ChangeLog v2->v3:
- Collect Andrew's reviewed-by.
ChangeLog v1->v2:
- Rebase on v5.8-rc1.
---
 drivers/net/dsa/rtl8366.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index a000d458d121..06adcf68ff8d 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -260,8 +260,8 @@ static int rtl8366_set_default_vlan_and_pvid(struct realtek_smi *smi,
 	u16 vid;
 	int ret;
 
-	/* This is the reserved default VLAN for this port */
-	vid = port + 1;
+	/* Use the top VLANs for per-port default VLAN */
+	vid = smi->num_vlan_mc - smi->num_ports + port;
 
 	if (port == smi->cpu_port)
 		/* For the CPU port, make all ports members of this
-- 
2.26.2

Re: [net-next PATCH 4/5 v4] net: dsa: rtl8366: VLAN 0 as disable tagging

[Florian Fainelli]

On 7/6/2020 1:52 PM, Linus Walleij wrote:
> The code in net/8021q/vlan.c, vlan_device_event() sets
> VLAN 0 for a VLAN-capable ethernet device when it
> comes up.
> 
> Since the RTL8366 DSA switches must have a VLAN and
> PVID set up for any packets to come through we have
> already set up default VLAN for each port as part of
> bringing the switch online.
> 
> Make sure that setting VLAN 0 has the same effect
> and does not try to actually tell the hardware to use
> VLAN 0 on the port because that will not work.
> 
> Cc: DENG Qingfang <dqfext@gmail.com>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v3->v4:
> - Resend with the rest
> ChangeLog v2->v3:
> - Collected Andrew's review tag.
> ChangeLog v1->v2:
> - Rebased on v5.8-rc1 and other changes.
> ---
>  drivers/net/dsa/rtl8366.c | 65 +++++++++++++++++++++++++++++++--------
>  1 file changed, 52 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
> index b907c0ed9697..a000d458d121 100644
> --- a/drivers/net/dsa/rtl8366.c
> +++ b/drivers/net/dsa/rtl8366.c
> @@ -355,15 +355,25 @@ int rtl8366_vlan_prepare(struct dsa_switch *ds, int port,
>  			 const struct switchdev_obj_port_vlan *vlan)
>  {
>  	struct realtek_smi *smi = ds->priv;
> +	u16 vid_begin = vlan->vid_begin;
> +	u16 vid_end = vlan->vid_end;
>  	u16 vid;
>  	int ret;
>  
> -	for (vid = vlan->vid_begin; vid < vlan->vid_end; vid++)
> +	if (vid_begin == 0) {
> +		dev_info(smi->dev, "prepare VLAN 0 - ignored\n");
> +		if (vid_end == 0)
> +			return 0;
> +		/* Skip VLAN 0 and start with VLAN 1 */
> +		vid_begin = 1;
> +	}

Humm I still don't understand why you are doing that. Upon DSA network
device creation, VID 0 will be pushed because we advertise support for
NETIF_F_HW_VLAN_CTAG_FILTER, so if nothing else, we will be getting the
"prepare VLAN 0 -ignored" message which is not relevant nor a good idea
to print.

You can force this VLAN to be programmed as untagged, in fact you should
be doing that per the 802.1Q specification.

There are no other cases other than the initial network device creation
that will lead to programming this VLAN ID. The bridge will always
specify a VID range within 1 through 4094 and the VLAN RX filter offload
will not add or remove VID 0 other than at creation/destruction.

As mentioned before, if you need VLAN awareness into the switch from the
get go, you need to set configure_vlan_while_not_filtering and that
would ensure that all ports belong to a VID at startup. Later on, when
the bridge gets set-up, it will be requesting the ports added as bridge
ports to be programmed into VID 1 as PVID untagged. And this should
still be fine.
-- 
Florian

Re: [net-next PATCH 4/5 v4] net: dsa: rtl8366: VLAN 0 as disable tagging

[Vladimir Oltean]

Hi Linus,

On Mon, Jul 06, 2020 at 02:23:11PM -0700, Florian Fainelli wrote:
> 
> 
> On 7/6/2020 1:52 PM, Linus Walleij wrote:
> > The code in net/8021q/vlan.c, vlan_device_event() sets
> > VLAN 0 for a VLAN-capable ethernet device when it
> > comes up.
> > 
> > Since the RTL8366 DSA switches must have a VLAN and
> > PVID set up for any packets to come through we have
> > already set up default VLAN for each port as part of
> > bringing the switch online.
> > 
> > Make sure that setting VLAN 0 has the same effect
> > and does not try to actually tell the hardware to use
> > VLAN 0 on the port because that will not work.
> > 
> > Cc: DENG Qingfang <dqfext@gmail.com>
> > Cc: Mauri Sandberg <sandberg@mailfence.com>
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > ChangeLog v3->v4:
> > - Resend with the rest
> > ChangeLog v2->v3:
> > - Collected Andrew's review tag.
> > ChangeLog v1->v2:
> > - Rebased on v5.8-rc1 and other changes.
> > ---
> >  drivers/net/dsa/rtl8366.c | 65 +++++++++++++++++++++++++++++++--------
> >  1 file changed, 52 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
> > index b907c0ed9697..a000d458d121 100644
> > --- a/drivers/net/dsa/rtl8366.c
> > +++ b/drivers/net/dsa/rtl8366.c
> > @@ -355,15 +355,25 @@ int rtl8366_vlan_prepare(struct dsa_switch *ds, int port,
> >  			 const struct switchdev_obj_port_vlan *vlan)
> >  {
> >  	struct realtek_smi *smi = ds->priv;
> > +	u16 vid_begin = vlan->vid_begin;
> > +	u16 vid_end = vlan->vid_end;
> >  	u16 vid;
> >  	int ret;
> >  
> > -	for (vid = vlan->vid_begin; vid < vlan->vid_end; vid++)
> > +	if (vid_begin == 0) {
> > +		dev_info(smi->dev, "prepare VLAN 0 - ignored\n");
> > +		if (vid_end == 0)
> > +			return 0;
> > +		/* Skip VLAN 0 and start with VLAN 1 */
> > +		vid_begin = 1;
> > +	}
> 
> Humm I still don't understand why you are doing that. Upon DSA network
> device creation, VID 0 will be pushed because we advertise support for
> NETIF_F_HW_VLAN_CTAG_FILTER, so if nothing else, we will be getting the
> "prepare VLAN 0 -ignored" message which is not relevant nor a good idea
> to print.
> 
> You can force this VLAN to be programmed as untagged, in fact you should
> be doing that per the 802.1Q specification.
> 
> There are no other cases other than the initial network device creation
> that will lead to programming this VLAN ID. The bridge will always
> specify a VID range within 1 through 4094 and the VLAN RX filter offload
> will not add or remove VID 0 other than at creation/destruction.
> 
> As mentioned before, if you need VLAN awareness into the switch from the
> get go, you need to set configure_vlan_while_not_filtering and that
> would ensure that all ports belong to a VID at startup. Later on, when
> the bridge gets set-up, it will be requesting the ports added as bridge
> ports to be programmed into VID 1 as PVID untagged. And this should
> still be fine.
> -- 
> Florian

To add to what Florian said, you should basically try to enable and test
"ds->configure_vlan_while_not_filtering = true" regardless, while you're
at it. The whole reason why it's there is because we didn't want to
introduce breakage when changing behavior of the DSA core. But ideally,
all drivers would use this setting, and then it could get deleted and so
would the old behavior of DSA.

Thanks,
-Vladimir

Re: [net-next PATCH 4/5 v4] net: dsa: rtl8366: VLAN 0 as disable tagging

[Linus Walleij]

On Mon, Jul 6, 2020 at 11:23 PM Florian Fainelli <f.fainelli@gmail.com> wrote:

> As mentioned before, if you need VLAN awareness into the switch from the
> get go, you need to set configure_vlan_while_not_filtering and that
> would ensure that all ports belong to a VID at startup. Later on, when
> the bridge gets set-up, it will be requesting the ports added as bridge
> ports to be programmed into VID 1 as PVID untagged. And this should
> still be fine.

I actually managed to figure this out. There were some confusing bugs in
the code that needed fixing but after that it works smoothly.

It is maybe not the most optimal setup - using one VLAN 1 for the whole
bridge and all ports denies the bridge to optimize the packet flow
per-port, but that is something we should fix in generic code and I
will describe it in detail in some patches I'm cooking.

Yours,
Linus Walleij