[net-bext PATCH 0/3 v1] Rectify RTL8366 default VLAN Behaviour

[net-bext PATCH 0/3 v1] Rectify RTL8366 default VLAN Behaviour

[Linus Walleij]

This fixes up the way the RTL8366 core handles VLANs:

- Fix two bugs

- Use the flag configure_vlan_while_not_filtering

Linus Walleij (3):
  net: dsa: rtl8366: Fix VLAN semantics
  net: dsa: rtl8366: Fix VLAN set-up
  net: dsa: rtl8366: Use DSA core to set up VLAN

 drivers/net/dsa/rtl8366.c   | 70 +++++++++++++------------------------
 drivers/net/dsa/rtl8366rb.c |  3 ++
 2 files changed, 27 insertions(+), 46 deletions(-)

-- 
2.26.2

[net-next PATCH 1/3 v1] net: dsa: rtl8366: Fix VLAN semantics

[Linus Walleij]

The RTL8366 would not handle adding new members (ports) to
a VLAN: the code assumed that ->port_vlan_add() was only
called once for a single port. When intializing the
switch with .configure_vlan_while_not_filtering set to
true, the function is called numerous times for adding
all ports to VLAN1, which was something the code could
not handle.

Alter rtl8366_set_vlan() to just |= new members and
untagged flags to 4k and MC VLAN table entries alike.
This makes it possible to just add new ports to a
VLAN.

Put in some helpful debug code that can be used to find
any further bugs here.

Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/dsa/rtl8366.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index 993cf3ac59d9..2997abeecc4a 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -43,18 +43,26 @@ int rtl8366_set_vlan(struct realtek_smi *smi, int vid, u32 member,
 	int ret;
 	int i;
 
+	dev_dbg(smi->dev,
+		"setting VLAN%d 4k members: 0x%02x, untagged: 0x%02x\n",
+		vid, member, untag);
+
 	/* Update the 4K table */
 	ret = smi->ops->get_vlan_4k(smi, vid, &vlan4k);
 	if (ret)
 		return ret;
 
-	vlan4k.member = member;
-	vlan4k.untag = untag;
+	vlan4k.member |= member;
+	vlan4k.untag |= untag;
 	vlan4k.fid = fid;
 	ret = smi->ops->set_vlan_4k(smi, &vlan4k);
 	if (ret)
 		return ret;
 
+	dev_dbg(smi->dev,
+		"resulting VLAN%d 4k members: 0x%02x, untagged: 0x%02x\n",
+		vid, vlan4k.member, vlan4k.untag);
+
 	/* Try to find an existing MC entry for this VID */
 	for (i = 0; i < smi->num_vlan_mc; i++) {
 		struct rtl8366_vlan_mc vlanmc;
@@ -65,11 +73,16 @@ int rtl8366_set_vlan(struct realtek_smi *smi, int vid, u32 member,
 
 		if (vid == vlanmc.vid) {
 			/* update the MC entry */
-			vlanmc.member = member;
-			vlanmc.untag = untag;
+			vlanmc.member |= member;
+			vlanmc.untag |= untag;
 			vlanmc.fid = fid;
 
 			ret = smi->ops->set_vlan_mc(smi, i, &vlanmc);
+
+			dev_dbg(smi->dev,
+				"resulting VLAN%d MC members: 0x%02x, untagged: 0x%02x\n",
+				vid, vlanmc.member, vlanmc.untag);
+
 			break;
 		}
 	}
-- 
2.26.2

[net-next PATCH 2/3 v1] net: dsa: rtl8366: Fix VLAN set-up

[Linus Walleij]

Alter the rtl8366_vlan_add() to call rtl8366_set_vlan()
inside the loop that goes over all VIDs since we now
properly support calling that function more than once.
Augment the loop to postincrement as this is more
intuitive.

The loop moved past the last VID but called
rtl8366_set_vlan() with the port number instead of
the VID, assuming a 1-to-1 correspondence between
ports and VIDs. This was also a bug.

Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/dsa/rtl8366.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index 2997abeecc4a..8f40fbf70a82 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -397,7 +397,7 @@ void rtl8366_vlan_add(struct dsa_switch *ds, int port,
 	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 = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
 		int pvid_val = 0;
 
 		dev_info(smi->dev, "add VLAN %04x\n", vid);
@@ -420,13 +420,13 @@ void rtl8366_vlan_add(struct dsa_switch *ds, int port,
 			if (ret < 0)
 				return;
 		}
-	}
 
-	ret = rtl8366_set_vlan(smi, port, member, untag, 0);
-	if (ret)
-		dev_err(smi->dev,
-			"failed to set up VLAN %04x",
-			vid);
+		ret = rtl8366_set_vlan(smi, vid, member, untag, 0);
+		if (ret)
+			dev_err(smi->dev,
+				"failed to set up VLAN %04x",
+				vid);
+	}
 }
 EXPORT_SYMBOL_GPL(rtl8366_vlan_add);
 
-- 
2.26.2

[net-next PATCH 3/3 v1] net: dsa: rtl8366: Use DSA core to set up VLAN

[Linus Walleij]

The current code in the RTL8366 VLAN handling code
initializes the default VLANs like this:

Ingress packets:

 port 0   ---> VLAN 1 ---> CPU port (5)
 port 1   ---> VLAN 2 ---> CPU port (5)
 port 2   ---> VLAN 3 ---> CPU port (5)
 port 3   ---> VLAN 4 ---> CPU port (5)
 port 4   ---> VLAN 5 ---> CPU port (5)

Egress packets:
 port 5 (CPU) ---> VLAN 6 ---> port 0, 1, 2, 3, 4

So 5 VLANs for ingress packets and one VLAN for
egress packets. Further it sets the PVID
for each port to further restrict the packets to
this VLAN only, and sets them as untagged.

This is a neat set-up in a way and a leftover
from the OpenWrt driver and the vendor code drop.

However the DSA core can be instructed to assign
all ports to a default VLAN, which will be
VLAN 1. This patch will change the above picture to
this:

Ingress packets:

 port 0   ---> VLAN 1 ---> CPU port (5)
 port 1   ---> VLAN 1 ---> CPU port (5)
 port 2   ---> VLAN 1 ---> CPU port (5)
 port 3   ---> VLAN 1 ---> CPU port (5)
 port 4   ---> VLAN 1 ---> CPU port (5)

Egress packets:
 port 5 (CPU) ---> VLAN 1 ---> port 0, 1, 2, 3, 4

So all traffic in the switch will by default pass
on VLAN 1. No PVID is set for ports by the DSA
core in this case.

This might have performance impact since the switch
hardware probably can sort packets into VLANs as
they pass through the fabric, but it is better
to fix the above set-up using generic code in that
case so that it can be reused by other switches.

The tested scenarios sure work fine with this
set-up including video streaming from a NAS device.

Cc: DENG Qingfang <dqfext@gmail.com>
Cc: Mauri Sandberg <sandberg@mailfence.com>
Suggested-by: Florian Fainelli <f.fainelli@gmail.com>
Suggested-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/net/dsa/rtl8366.c   | 35 -----------------------------------
 drivers/net/dsa/rtl8366rb.c |  3 +++
 2 files changed, 3 insertions(+), 35 deletions(-)

diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c
index 8f40fbf70a82..e549b1167ddc 100644
--- a/drivers/net/dsa/rtl8366.c
+++ b/drivers/net/dsa/rtl8366.c
@@ -275,41 +275,6 @@ int rtl8366_init_vlan(struct realtek_smi *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);
diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
index 48f1ff746799..226851e57c1b 100644
--- a/drivers/net/dsa/rtl8366rb.c
+++ b/drivers/net/dsa/rtl8366rb.c
@@ -743,6 +743,9 @@ static int rtl8366rb_setup(struct dsa_switch *ds)
 	dev_info(smi->dev, "RTL%04x ver %u chip found\n",
 		 chip_id, chip_ver & RTL8366RB_CHIP_VERSION_MASK);
 
+	/* This chip requires that a VLAN be set up for each port */
+	ds->configure_vlan_while_not_filtering = true;
+
 	/* Do the init dance using the right jam table */
 	switch (chip_ver) {
 	case 0:
-- 
2.26.2

Re: [net-next PATCH 1/3 v1] net: dsa: rtl8366: Fix VLAN semantics

[Florian Fainelli]

On 7/8/2020 1:44 PM, Linus Walleij wrote:
> The RTL8366 would not handle adding new members (ports) to
> a VLAN: the code assumed that ->port_vlan_add() was only
> called once for a single port. When intializing the
> switch with .configure_vlan_while_not_filtering set to
> true, the function is called numerous times for adding
> all ports to VLAN1, which was something the code could
> not handle.
> 
> Alter rtl8366_set_vlan() to just |= new members and
> untagged flags to 4k and MC VLAN table entries alike.
> This makes it possible to just add new ports to a
> VLAN.
> 
> Put in some helpful debug code that can be used to find
> any further bugs here.
> 
> Cc: DENG Qingfang <dqfext@gmail.com>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

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

Re: [net-next PATCH 2/3 v1] net: dsa: rtl8366: Fix VLAN set-up

[Florian Fainelli]

On 7/8/2020 1:44 PM, Linus Walleij wrote:
> Alter the rtl8366_vlan_add() to call rtl8366_set_vlan()
> inside the loop that goes over all VIDs since we now
> properly support calling that function more than once.
> Augment the loop to postincrement as this is more
> intuitive.
> 
> The loop moved past the last VID but called
> rtl8366_set_vlan() with the port number instead of
> the VID, assuming a 1-to-1 correspondence between
> ports and VIDs. This was also a bug.
> 
> Cc: DENG Qingfang <dqfext@gmail.com>
> Cc: Mauri Sandberg <sandberg@mailfence.com>
> Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

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

Re: [net-next PATCH 3/3 v1] net: dsa: rtl8366: Use DSA core to set up VLAN

[Florian Fainelli]

On 7/8/2020 1:44 PM, Linus Walleij wrote:
> The current code in the RTL8366 VLAN handling code
> initializes the default VLANs like this:
> 
> Ingress packets:
> 
>  port 0   ---> VLAN 1 ---> CPU port (5)
>  port 1   ---> VLAN 2 ---> CPU port (5)
>  port 2   ---> VLAN 3 ---> CPU port (5)
>  port 3   ---> VLAN 4 ---> CPU port (5)
>  port 4   ---> VLAN 5 ---> CPU port (5)
> 
> Egress packets:
>  port 5 (CPU) ---> VLAN 6 ---> port 0, 1, 2, 3, 4
> 
> So 5 VLANs for ingress packets and one VLAN for
> egress packets. Further it sets the PVID
> for each port to further restrict the packets to
> this VLAN only, and sets them as untagged.
> 
> This is a neat set-up in a way and a leftover
> from the OpenWrt driver and the vendor code drop.
> 
> However the DSA core can be instructed to assign
> all ports to a default VLAN, which will be
> VLAN 1. This patch will change the above picture to
> this:
> 
> Ingress packets:
> 
>  port 0   ---> VLAN 1 ---> CPU port (5)
>  port 1   ---> VLAN 1 ---> CPU port (5)
>  port 2   ---> VLAN 1 ---> CPU port (5)
>  port 3   ---> VLAN 1 ---> CPU port (5)
>  port 4   ---> VLAN 1 ---> CPU port (5)
> 
> Egress packets:
>  port 5 (CPU) ---> VLAN 1 ---> port 0, 1, 2, 3, 4
> 
> So all traffic in the switch will by default pass
> on VLAN 1. No PVID is set for ports by the DSA
> core in this case.
> 
> This might have performance impact since the switch
> hardware probably can sort packets into VLANs as
> they pass through the fabric, but it is better
> to fix the above set-up using generic code in that
> case so that it can be reused by other switches.
> 
> The tested scenarios sure work fine with this
> set-up including video streaming from a NAS device.

Does this maintain the requirement that by default, all DSA ports must
be isolated from one another? For instance, if you have broadcast
traffic on port 2, by virtue of having port 1 and port 2 now in VLAN ID
1, do you see that broadcast traffic from port 1?

If you do, then you need to find a way to maintain isolation between ports.

It looks like the FID is used for implementing VLAN filtering so maybe
you need to dedicate a FID per port number here, and add them all to VLAN 1?
-- 
Florian

Re: [net-next PATCH 3/3 v1] net: dsa: rtl8366: Use DSA core to set up VLAN

[Linus Walleij]

On Thu, Jul 9, 2020 at 4:29 AM Florian Fainelli <f.fainelli@gmail.com> wrote:
> Me
> > The tested scenarios sure work fine with this
> > set-up including video streaming from a NAS device.
>
> Does this maintain the requirement that by default, all DSA ports must
> be isolated from one another? For instance, if you have broadcast
> traffic on port 2, by virtue of having port 1 and port 2 now in VLAN ID
> 1, do you see that broadcast traffic from port 1?

Unfortunately yes :(

I test this by setting a host (169.254.1.1) to ping the router
(169.254.1.2) and if I connect a machine to one of the other
ports I can see the ARP requests on that machine
as "who-has (...) tell 169.254.1.1"

> If you do, then you need to find a way to maintain isolation between ports.
>
> It looks like the FID is used for implementing VLAN filtering so maybe
> you need to dedicate a FID per port number here, and add them all to VLAN 1?

The FID exist in the source code but neither the vendor driver
not the OpenWrt driver make any use of them, their way of
separating the ports is by using one VLAN per port and setting
the PVID for each port to that VLAN, in the way described
in the commit message.

Is there an example of some driver using a FID for this?

What do you think about the option to teach the core to set
up VLANs like the driver currently does with one VLAN per
port and PVID set for each? I haven't even been able to
locate the code that associates all ports with VLAN1 but
I figured it can't be too hard? (Famous last words.)

Yours,
Linus Walleij