[PATCH net] net: dsa: b53: Fix VLAN usage and how we treat CPU port

[PATCH net] net: dsa: b53: Fix VLAN usage and how we treat CPU port

[Florian Fainelli]

We currently have a fundamental problem in how we treat the CPU port and
its VLAN membership. As soon as a second VLAN is configured to be
untagged, the CPU automatically becomes untagged for that VLAN as well,
and yet, we don't gracefully make sure that the CPU becomes tagged in
the other VLANs it could be a member of. This results in only one VLAN
being effectively usable from the CPU's perspective.

Instead of having some pretty complex logic which tries to maintain the
CPU port's default VLAN and its untagged properties, just do something
very simple which consists in neither altering the CPU port's PVID
settings, nor its untagged settings:

- whenever a VLAN is added, the CPU is automatically a member of this
  VLAN group, as a tagged member
- PVID settings for downstream ports do not alter the CPU port's PVID
  since it now is part of all VLANs in the system

This means that a typical example where e.g: LAN ports are in VLAN1, and
WAN port is in VLAN2, now require having two VLAN interfaces for the
host to properly terminate and send traffic from/to.

Fixes: Fixes: a2482d2ce349 ("net: dsa: b53: Plug in VLAN support")
Reported-by: Hartmut Knaack <knaack.h@gmx.de>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
David,

Can you queue this for -stable so it makes it into 4.8.4?

Thank you!

 drivers/net/dsa/b53/b53_common.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 7717b19dc806..947adda3397d 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -962,9 +962,10 @@ static void b53_vlan_add(struct dsa_switch *ds, int port,
 
 		vl->members |= BIT(port) | BIT(cpu_port);
 		if (untagged)
-			vl->untag |= BIT(port) | BIT(cpu_port);
+			vl->untag |= BIT(port);
 		else
-			vl->untag &= ~(BIT(port) | BIT(cpu_port));
+			vl->untag &= ~BIT(port);
+		vl->untag &= ~BIT(cpu_port);
 
 		b53_set_vlan_entry(dev, vid, vl);
 		b53_fast_age_vlan(dev, vid);
@@ -973,8 +974,6 @@ static void b53_vlan_add(struct dsa_switch *ds, int port,
 	if (pvid) {
 		b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port),
 			    vlan->vid_end);
-		b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(cpu_port),
-			    vlan->vid_end);
 		b53_fast_age_vlan(dev, vid);
 	}
 }
@@ -984,7 +983,6 @@ static int b53_vlan_del(struct dsa_switch *ds, int port,
 {
 	struct b53_device *dev = ds->priv;
 	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
-	unsigned int cpu_port = dev->cpu_port;
 	struct b53_vlan *vl;
 	u16 vid;
 	u16 pvid;
@@ -997,8 +995,6 @@ static int b53_vlan_del(struct dsa_switch *ds, int port,
 		b53_get_vlan_entry(dev, vid, vl);
 
 		vl->members &= ~BIT(port);
-		if ((vl->members & BIT(cpu_port)) == BIT(cpu_port))
-			vl->members = 0;
 
 		if (pvid == vid) {
 			if (is5325(dev) || is5365(dev))
@@ -1007,18 +1003,14 @@ static int b53_vlan_del(struct dsa_switch *ds, int port,
 				pvid = 0;
 		}
 
-		if (untagged) {
+		if (untagged)
 			vl->untag &= ~(BIT(port));
-			if ((vl->untag & BIT(cpu_port)) == BIT(cpu_port))
-				vl->untag = 0;
-		}
 
 		b53_set_vlan_entry(dev, vid, vl);
 		b53_fast_age_vlan(dev, vid);
 	}
 
 	b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(port), pvid);
-	b53_write16(dev, B53_VLAN_PAGE, B53_VLAN_PORT_DEF_TAG(cpu_port), pvid);
 	b53_fast_age_vlan(dev, pvid);
 
 	return 0;
-- 
2.9.3

CPU port VLAN configuration [Was: Re: [PATCH net] net: dsa: b53: Fix VLAN usage and how we treat CPU port]

[Florian Fainelli]

Hi Andrew, Vivien,

On 11/15/2016 03:58 PM, Florian Fainelli wrote:
> We currently have a fundamental problem in how we treat the CPU port and
> its VLAN membership. As soon as a second VLAN is configured to be
> untagged, the CPU automatically becomes untagged for that VLAN as well,
> and yet, we don't gracefully make sure that the CPU becomes tagged in
> the other VLANs it could be a member of. This results in only one VLAN
> being effectively usable from the CPU's perspective.
> 
> Instead of having some pretty complex logic which tries to maintain the
> CPU port's default VLAN and its untagged properties, just do something
> very simple which consists in neither altering the CPU port's PVID
> settings, nor its untagged settings:
> 
> - whenever a VLAN is added, the CPU is automatically a member of this
>   VLAN group, as a tagged member
> - PVID settings for downstream ports do not alter the CPU port's PVID
>   since it now is part of all VLANs in the system
> 
> This means that a typical example where e.g: LAN ports are in VLAN1, and
> WAN port is in VLAN2, now require having two VLAN interfaces for the
> host to properly terminate and send traffic from/to.

Do you know how mv88e6xxx deals with this case? In the use case
mentioned, what we have is this:

- Ports0-3 -> VLAN1 (LAN segment)
- Port4 -> WAN (WAN segment)

With OpenWrt/swconfig you would typically have eth0.1 and eth0.2
interfaces that would take care of terminating VLAN tags, this is the
behavior that I am bringing back here because it is the only way you can
have the downstream ports configured as untagged, and yet have proper
segregation appear from the CPU port's perspective (unless you use
Marvell/Broadcom/QCA tags, which we don't do yet here, but still).

A more general issue that I am still working on is having the ability to
control the CPU port's VLAN properties by configuring the master bridge
device, since it is inherently "a view" (if multiple bridges, multiple
views) of the CPU port of an Ethernet switch. The general idea would be
that if you do:

bridge vlan add vid 2 dev br0 self

this calls down into the switch driver to actually configure VLAN ID 2
on the CPU port to be tagged.

I have a prototype of this that nearly works [1], except upon the
initial bridge master device creation, since we have not had the ability
to enslave port members yet, we are not able to propagate the bridge's
default VLAN to the CPU port. More to come in the next weeks!

[1]: https://github.com/ffainelli/linux/commits/bridge-master
-- 
Florian

Re: CPU port VLAN configuration [Was: Re: [PATCH net] net: dsa: b53: Fix VLAN usage and how we treat CPU port]

[Vivien Didelot]

Hi Florian,

Florian Fainelli <f.fainelli@gmail.com> writes:

>> We currently have a fundamental problem in how we treat the CPU port and
>> its VLAN membership. As soon as a second VLAN is configured to be
>> untagged, the CPU automatically becomes untagged for that VLAN as well,
>> and yet, we don't gracefully make sure that the CPU becomes tagged in
>> the other VLANs it could be a member of. This results in only one VLAN
>> being effectively usable from the CPU's perspective.
>> 
>> Instead of having some pretty complex logic which tries to maintain the
>> CPU port's default VLAN and its untagged properties, just do something
>> very simple which consists in neither altering the CPU port's PVID
>> settings, nor its untagged settings:
>> 
>> - whenever a VLAN is added, the CPU is automatically a member of this
>>   VLAN group, as a tagged member

mv88e6xxx currently does that.

>> - PVID settings for downstream ports do not alter the CPU port's PVID
>>   since it now is part of all VLANs in the system

If I'm not mistaken, the CPU port's PVID defaults to 0, since no other
value really makes sense...

>> This means that a typical example where e.g: LAN ports are in VLAN1, and
>> WAN port is in VLAN2, now require having two VLAN interfaces for the
>> host to properly terminate and send traffic from/to.
>
> Do you know how mv88e6xxx deals with this case? In the use case
> mentioned, what we have is this:
>
> - Ports0-3 -> VLAN1 (LAN segment)
> - Port4 -> WAN (WAN segment)
>
> With OpenWrt/swconfig you would typically have eth0.1 and eth0.2
> interfaces that would take care of terminating VLAN tags, this is the
> behavior that I am bringing back here because it is the only way you can
> have the downstream ports configured as untagged, and yet have proper
> segregation appear from the CPU port's perspective (unless you use
> Marvell/Broadcom/QCA tags, which we don't do yet here, but still).
>
> A more general issue that I am still working on is having the ability to
> control the CPU port's VLAN properties by configuring the master bridge
> device, since it is inherently "a view" (if multiple bridges, multiple
> views) of the CPU port of an Ethernet switch. The general idea would be
> that if you do:
>
> bridge vlan add vid 2 dev br0 self
>
> this calls down into the switch driver to actually configure VLAN ID 2
> on the CPU port to be tagged.

This is the good approach IMO.

As a side note, a good first thing to do here would be to fix the actual
behavior of switchdev calls from iproute2 commands. It doesn't respect
what the bridge's manpage describes in the VLAN section:

       self   the vlan is configured on the specified physical device.
              Required if the device is the bridge device.

       master the vlan is configured on the software bridge (default).

If I'm not mistaken, "bridge vlan add vid 2 dev lan2 [master]" currently
programs the hardware.

> I have a prototype of this that nearly works [1], except upon the
> initial bridge master device creation, since we have not had the ability
> to enslave port members yet, we are not able to propagate the bridge's
> default VLAN to the CPU port. More to come in the next weeks!

For this case, maybe the way to go would be to let DSA query the bridge
VLAN membership when enslaving a port and configure the CPU port
accordingly if needed.

Thanks,

        Vivien

Re: [PATCH net] net: dsa: b53: Fix VLAN usage and how we treat CPU port

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue, 15 Nov 2016 15:58:15 -0800

> We currently have a fundamental problem in how we treat the CPU port and
> its VLAN membership. As soon as a second VLAN is configured to be
> untagged, the CPU automatically becomes untagged for that VLAN as well,
> and yet, we don't gracefully make sure that the CPU becomes tagged in
> the other VLANs it could be a member of. This results in only one VLAN
> being effectively usable from the CPU's perspective.
> 
> Instead of having some pretty complex logic which tries to maintain the
> CPU port's default VLAN and its untagged properties, just do something
> very simple which consists in neither altering the CPU port's PVID
> settings, nor its untagged settings:
> 
> - whenever a VLAN is added, the CPU is automatically a member of this
>   VLAN group, as a tagged member
> - PVID settings for downstream ports do not alter the CPU port's PVID
>   since it now is part of all VLANs in the system
> 
> This means that a typical example where e.g: LAN ports are in VLAN1, and
> WAN port is in VLAN2, now require having two VLAN interfaces for the
> host to properly terminate and send traffic from/to.
> 
> Fixes: Fixes: a2482d2ce349 ("net: dsa: b53: Plug in VLAN support")
> Reported-by: Hartmut Knaack <knaack.h@gmx.de>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> David,
> 
> Can you queue this for -stable so it makes it into 4.8.4?

Applied and queued up for -stable, thanks.