| Message ID | 20200708204456.1365855-4-linus.walleij@linaro.org |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | Rectify RTL8366 default VLAN Behaviour | expand |
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?
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
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:
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(-)