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

Message ID 20200708204456.1365855-4-linus.walleij@linaro.org
State New
Headers show
Series
  • Rectify RTL8366 default VLAN Behaviour
Related show

Commit Message

Linus Walleij July 8, 2020, 8:44 p.m.
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(-)

-- 
2.26.2

Comments

Florian Fainelli July 9, 2020, 2:29 a.m. | #1
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
Linus Walleij July 9, 2020, 7:44 a.m. | #2
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

Patch

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: