[PATCH net-next v2] net: dsa: b53: Do not force CPU to be always tagged

[PATCH net-next v2] net: dsa: b53: Do not force CPU to be always tagged

[Florian Fainelli]

Commit ca8931948344 ("net: dsa: b53: Keep CPU port as tagged in all
VLANs") forced the CPU port to be always tagged in any VLAN membership.
This was necessary back then because we did not support Broadcom tags
for all configurations so the only way to differentiate tagged and
untagged traffic while DSA_TAG_PROTO_NONE was used was to force the CPU
port into being always tagged.

With most configurations enabling Broadcom tags, especially after
8fab459e69ab ("net: dsa: b53: Enable Broadcom tags for 531x5/539x
families") we do not need to apply this unconditional force tagging of
the CPU port in all VLANs.

A helper function is introduced to faciliate the encapsulation of the
specific condition requiring the CPU port to be tagged in all VLANs and
the dsa_switch_ops::untag_bridge_pvid boolean is moved to when
dsa_switch_ops::setup is called when we have already determined the
tagging protocol we will be using.

Reported-by: Matthew Hagan <mnhagan88@gmail.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v2:

- properly deal with DSA_TAG_PROTO_NONE so we continue to support
  that mode on older chips like 5325 and 5365 until they gain Broadcom
  tag support

 drivers/net/dsa/b53/b53_common.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 3ca6b394dd5f..6e199454e41d 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1084,6 +1084,11 @@ static int b53_setup(struct dsa_switch *ds)
 	unsigned int port;
 	int ret;
 
+	/* Request bridge PVID untagged when DSA_TAG_PROTO_NONE is set
+	 * which forces the CPU port to be tagged in all VLANs.
+	 */
+	ds->untag_bridge_pvid = dev->tag_protocol == DSA_TAG_PROTO_NONE;
+
 	ret = b53_reset_switch(dev);
 	if (ret) {
 		dev_err(ds->dev, "failed to reset switch\n");
@@ -1455,6 +1460,13 @@ static int b53_vlan_prepare(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static bool b53_vlan_port_needs_forced_tagged(struct dsa_switch *ds, int port)
+{
+	struct b53_device *dev = ds->priv;
+
+	return dev->tag_protocol == DSA_TAG_PROTO_NONE && dsa_is_cpu_port(ds, port);
+}
+
 int b53_vlan_add(struct dsa_switch *ds, int port,
 		 const struct switchdev_obj_port_vlan *vlan,
 		 struct netlink_ext_ack *extack)
@@ -1477,7 +1489,7 @@ int b53_vlan_add(struct dsa_switch *ds, int port,
 		untagged = true;
 
 	vl->members |= BIT(port);
-	if (untagged && !dsa_is_cpu_port(ds, port))
+	if (untagged && !b53_vlan_port_needs_forced_tagged(ds, port))
 		vl->untag |= BIT(port);
 	else
 		vl->untag &= ~BIT(port);
@@ -1514,7 +1526,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port,
 	if (pvid == vlan->vid)
 		pvid = b53_default_pvid(dev);
 
-	if (untagged && !dsa_is_cpu_port(ds, port))
+	if (untagged && !b53_vlan_port_needs_forced_tagged(ds, port))
 		vl->untag &= ~(BIT(port));
 
 	b53_set_vlan_entry(dev, vlan->vid, vl);
@@ -2660,7 +2672,6 @@ struct b53_device *b53_switch_alloc(struct device *base,
 	dev->priv = priv;
 	dev->ops = ops;
 	ds->ops = &b53_switch_ops;
-	ds->untag_bridge_pvid = true;
 	dev->vlan_enabled = true;
 	/* Let DSA handle the case were multiple bridges span the same switch
 	 * device and different VLAN awareness settings are requested, which
-- 
2.25.1

Re: [PATCH net-next v2] net: dsa: b53: Do not force CPU to be always tagged

[Florian Fainelli]

On 6/8/2021 2:22 PM, Florian Fainelli wrote:
> Commit ca8931948344 ("net: dsa: b53: Keep CPU port as tagged in all
> VLANs") forced the CPU port to be always tagged in any VLAN membership.
> This was necessary back then because we did not support Broadcom tags
> for all configurations so the only way to differentiate tagged and
> untagged traffic while DSA_TAG_PROTO_NONE was used was to force the CPU
> port into being always tagged.
> 
> With most configurations enabling Broadcom tags, especially after
> 8fab459e69ab ("net: dsa: b53: Enable Broadcom tags for 531x5/539x
> families") we do not need to apply this unconditional force tagging of
> the CPU port in all VLANs.
> 
> A helper function is introduced to faciliate the encapsulation of the
> specific condition requiring the CPU port to be tagged in all VLANs and
> the dsa_switch_ops::untag_bridge_pvid boolean is moved to when
> dsa_switch_ops::setup is called when we have already determined the
> tagging protocol we will be using.
> 
> Reported-by: Matthew Hagan <mnhagan88@gmail.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---

Matthew, here is a tcpdump capture showing that there is no VLAN 0 tag
being added, unlike before:

00:00:42.191113 b8:ac:6f:80:af:7e (oui Unknown) > 00:10:18:cd:c9:c2 (oui
Unknown), BRCM tag OP: EG, CID: 0, RC: exception, TC: 0, port: 0,
ethertype IPv4 (0x0800), length 102: (tos 0x0, ttl 64, id 25041, offset
0, flags [none], proto ICMP (1), length 84)
    192.168.1.254 > 192.168.1.10: ICMP echo reply, id 1543, seq 12,
length 64
        0x0000:  0010 18cd c9c2 b8ac 6f80 af7e 0000 2000  ........o..~....
        0x0010:  0800 4500 0054 61d1 0000 4001 947f c0a8  ..E..Ta...@.....
        0x0020:  01fe c0a8 010a 0000 4522 0607 000c 31c8  ........E"....1.
        0x0030:  8302 0000 0000 0000 0000 0000 0000 0000  ................
        0x0040:  0000 0000 0000 0000 0000 0000 0000 0000  ................
        0x0050:  0000 0000 0000 0000 0000 0000 0000 0000  ................
        0x0060:  0000 0000 0000

Let me know how this patch goes.
-- 
Florian

Re: [PATCH net-next v2] net: dsa: b53: Do not force CPU to be always tagged

[Vladimir Oltean]

On Tue, Jun 08, 2021 at 02:22:04PM -0700, Florian Fainelli wrote:
> Commit ca8931948344 ("net: dsa: b53: Keep CPU port as tagged in all
> VLANs") forced the CPU port to be always tagged in any VLAN membership.
> This was necessary back then because we did not support Broadcom tags
> for all configurations so the only way to differentiate tagged and
> untagged traffic while DSA_TAG_PROTO_NONE was used was to force the CPU
> port into being always tagged.
> 
> With most configurations enabling Broadcom tags, especially after
> 8fab459e69ab ("net: dsa: b53: Enable Broadcom tags for 531x5/539x
> families") we do not need to apply this unconditional force tagging of
> the CPU port in all VLANs.
> 
> A helper function is introduced to faciliate the encapsulation of the
> specific condition requiring the CPU port to be tagged in all VLANs and
> the dsa_switch_ops::untag_bridge_pvid boolean is moved to when
> dsa_switch_ops::setup is called when we have already determined the
> tagging protocol we will be using.
> 
> Reported-by: Matthew Hagan <mnhagan88@gmail.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v2] net: dsa: b53: Do not force CPU to be always tagged

[Matthew Hagan]

Hi Florian,

On 08/06/2021 22:26, Florian Fainelli wrote:

>
> On 6/8/2021 2:22 PM, Florian Fainelli wrote:
>> Commit ca8931948344 ("net: dsa: b53: Keep CPU port as tagged in all
>> VLANs") forced the CPU port to be always tagged in any VLAN membership.
>> This was necessary back then because we did not support Broadcom tags
>> for all configurations so the only way to differentiate tagged and
>> untagged traffic while DSA_TAG_PROTO_NONE was used was to force the CPU
>> port into being always tagged.
>>
>> With most configurations enabling Broadcom tags, especially after
>> 8fab459e69ab ("net: dsa: b53: Enable Broadcom tags for 531x5/539x
>> families") we do not need to apply this unconditional force tagging of
>> the CPU port in all VLANs.
>>
>> A helper function is introduced to faciliate the encapsulation of the
>> specific condition requiring the CPU port to be tagged in all VLANs and
>> the dsa_switch_ops::untag_bridge_pvid boolean is moved to when
>> dsa_switch_ops::setup is called when we have already determined the
>> tagging protocol we will be using.
>>
>> Reported-by: Matthew Hagan <mnhagan88@gmail.com>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
> Matthew, here is a tcpdump capture showing that there is no VLAN 0 tag
> being added, unlike before:
>
> 00:00:42.191113 b8:ac:6f:80:af:7e (oui Unknown) > 00:10:18:cd:c9:c2 (oui
> Unknown), BRCM tag OP: EG, CID: 0, RC: exception, TC: 0, port: 0,
> ethertype IPv4 (0x0800), length 102: (tos 0x0, ttl 64, id 25041, offset
> 0, flags [none], proto ICMP (1), length 84)
>     192.168.1.254 > 192.168.1.10: ICMP echo reply, id 1543, seq 12,
> length 64
>         0x0000:  0010 18cd c9c2 b8ac 6f80 af7e 0000 2000  ........o..~....
>         0x0010:  0800 4500 0054 61d1 0000 4001 947f c0a8  ..E..Ta...@.....
>         0x0020:  01fe c0a8 010a 0000 4522 0607 000c 31c8  ........E"....1.
>         0x0030:  8302 0000 0000 0000 0000 0000 0000 0000  ................
>         0x0040:  0000 0000 0000 0000 0000 0000 0000 0000  ................
>         0x0050:  0000 0000 0000 0000 0000 0000 0000 0000  ................
>         0x0060:  0000 0000 0000
>
> Let me know how this patch goes.

tcpdump capture on eth0 of inbound DHCP requests on port 4 of QCA switch sw1
which is attached to BCM switch port 5. Packet is VLAN tagged, VID 10.

Without patch (and without tag_qca hack):
0000   00 00 20 05 ff ff ff ff ff ff e0 cb bc 88 c9 a5   .. .............
0010   81 00 00 00 be 4c 81 00 00 0a 08 00 45 00 01 48   .....L......E..H
0020   00 00 00 00 40 11 79 a6 00 00 00 00 ff ff ff ff   ....@.y.........

With patch applied:
0000   00 00 20 05 ff ff ff ff ff ff e0 cb bc 88 c9 a5   .. .............
0010   be 4c 81 00 00 0a 08 00 45 00 01 48 00 00 00 00   .L......E..H....
0020   40 11 79 a6 00 00 00 00 ff ff ff ff 00 44 00 43   @.y..........D.C

Everything seems fine. Looks good!

Matthew

Re: [PATCH net-next v2] net: dsa: b53: Do not force CPU to be always tagged

[Matthew Hagan]

On 08/06/2021 22:22, Florian Fainelli wrote:

> Commit ca8931948344 ("net: dsa: b53: Keep CPU port as tagged in all
> VLANs") forced the CPU port to be always tagged in any VLAN membership.
> This was necessary back then because we did not support Broadcom tags
> for all configurations so the only way to differentiate tagged and
> untagged traffic while DSA_TAG_PROTO_NONE was used was to force the CPU
> port into being always tagged.
>
> With most configurations enabling Broadcom tags, especially after
> 8fab459e69ab ("net: dsa: b53: Enable Broadcom tags for 531x5/539x
> families") we do not need to apply this unconditional force tagging of
> the CPU port in all VLANs.
>
> A helper function is introduced to faciliate the encapsulation of the
> specific condition requiring the CPU port to be tagged in all VLANs and
> the dsa_switch_ops::untag_bridge_pvid boolean is moved to when
> dsa_switch_ops::setup is called when we have already determined the
> tagging protocol we will be using.
>
> Reported-by: Matthew Hagan <mnhagan88@gmail.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---

Tested-by: Matthew Hagan <mnhagan88@gmail.com>

Re: [PATCH net-next v2] net: dsa: b53: Do not force CPU to be always tagged

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Tue,  8 Jun 2021 14:22:04 -0700 you wrote:
> Commit ca8931948344 ("net: dsa: b53: Keep CPU port as tagged in all
> VLANs") forced the CPU port to be always tagged in any VLAN membership.
> This was necessary back then because we did not support Broadcom tags
> for all configurations so the only way to differentiate tagged and
> untagged traffic while DSA_TAG_PROTO_NONE was used was to force the CPU
> port into being always tagged.
> 
> [...]

Here is the summary with links:
  - [net-next,v2] net: dsa: b53: Do not force CPU to be always tagged
    https://git.kernel.org/netdev/net-next/c/2c32a3d3c233

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v2] net: dsa: b53: Do not force CPU to be always tagged

[Vladimir Oltean]

On Tue, Jun 08, 2021 at 02:22:04PM -0700, Florian Fainelli wrote:
> Commit ca8931948344 ("net: dsa: b53: Keep CPU port as tagged in all
> VLANs") forced the CPU port to be always tagged in any VLAN membership.
> This was necessary back then because we did not support Broadcom tags
> for all configurations so the only way to differentiate tagged and
> untagged traffic while DSA_TAG_PROTO_NONE was used was to force the CPU
> port into being always tagged.
> 
> With most configurations enabling Broadcom tags, especially after
> 8fab459e69ab ("net: dsa: b53: Enable Broadcom tags for 531x5/539x
> families") we do not need to apply this unconditional force tagging of
> the CPU port in all VLANs.
> 
> A helper function is introduced to faciliate the encapsulation of the
> specific condition requiring the CPU port to be tagged in all VLANs and
> the dsa_switch_ops::untag_bridge_pvid boolean is moved to when
> dsa_switch_ops::setup is called when we have already determined the
> tagging protocol we will be using.
> 
> Reported-by: Matthew Hagan <mnhagan88@gmail.com>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---

Florian, does the hardware behave in the same way if you disable
CONFIG_VLAN_8021Q?

Re: [PATCH net-next v2] net: dsa: b53: Do not force CPU to be always tagged

[Florian Fainelli]

On 6/9/2021 3:30 PM, Vladimir Oltean wrote:
> On Tue, Jun 08, 2021 at 02:22:04PM -0700, Florian Fainelli wrote:
>> Commit ca8931948344 ("net: dsa: b53: Keep CPU port as tagged in all
>> VLANs") forced the CPU port to be always tagged in any VLAN membership.
>> This was necessary back then because we did not support Broadcom tags
>> for all configurations so the only way to differentiate tagged and
>> untagged traffic while DSA_TAG_PROTO_NONE was used was to force the CPU
>> port into being always tagged.
>>
>> With most configurations enabling Broadcom tags, especially after
>> 8fab459e69ab ("net: dsa: b53: Enable Broadcom tags for 531x5/539x
>> families") we do not need to apply this unconditional force tagging of
>> the CPU port in all VLANs.
>>
>> A helper function is introduced to faciliate the encapsulation of the
>> specific condition requiring the CPU port to be tagged in all VLANs and
>> the dsa_switch_ops::untag_bridge_pvid boolean is moved to when
>> dsa_switch_ops::setup is called when we have already determined the
>> tagging protocol we will be using.
>>
>> Reported-by: Matthew Hagan <mnhagan88@gmail.com>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
> 
> Florian, does the hardware behave in the same way if you disable
> CONFIG_VLAN_8021Q?

Unfortunately it does not because there is no more code calling into
b53_vlan_add() with the desired egress untagged attribute:

00:01:23.015477 b8:ac:6f:80:af:7e (oui Unknown) > 00:10:18:cd:c9:c2 (oui
Unknown), BRCM tag OP: EG, CID: 0, RC: exception, TC: 0, port: 0,
ethertype 802.1Q (0x8100), length 106: vlan 0, p 0, ethertype IPv4
(0x0800), (tos 0x0, ttl 64, id 3662, offset 0, flags [none], proto ICMP
(1), length 84)
    192.168.1.254 > 192.168.1.10: ICMP echo reply, id 5127, seq 17,
length 64
        0x0000:  0010 18cd c9c2 b8ac 6f80 af7e 0000 2000  ........o..~....
        0x0010:  8100 0000 0800 4500 0054 0e4e 0000 4001  ......E..T.N..@.
        0x0020:  e802 c0a8 01fe c0a8 010a 0000 bc2c 1407  .............,..
        0x0030:  0011 3db6 f204 0000 0000 0000 0000 0000  ..=.............
        0x0040:  0000 0000 0000 0000 0000 0000 0000 0000  ................
        0x0050:  0000 0000 0000 0000 0000 0000 0000 0000  ................
        0x0060:  0000 0000 0000 0000 0000

Something like this works:

diff --git a/drivers/net/dsa/b53/b53_common.c
b/drivers/net/dsa/b53/b53_common.c
index 6e199454e41d..ac3bb5ef17e2 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -748,9 +748,13 @@ int b53_configure_vlan(struct dsa_switch *ds)

        b53_enable_vlan(dev, -1, dev->vlan_enabled, ds->vlan_filtering);

-       b53_for_each_port(dev, i)
+       b53_for_each_port(dev, i) {
+               v = &dev->vlans[def_vid];
+               v->members |= BIT(i);
+               v->untag = v->members;
                b53_write16(dev, B53_VLAN_PAGE,
                            B53_VLAN_PORT_DEF_TAG(i), def_vid);
+       }

        /* Upon initial call we have not set-up any VLANs, but upon
         * system resume, we need to restore all VLAN entries.
-- 
Florian

[PATCH net-next v2] net: dsa: b53: Do not force CPU to be always tagged

[Florian Fainelli]

Commit ca8931948344 ("net: dsa: b53: Keep CPU port as tagged in all
VLANs") forced the CPU port to be always tagged in any VLAN membership.
This was necessary back then because we did not support Broadcom tags
for all configurations so the only way to differentiate tagged and
untagged traffic while DSA_TAG_PROTO_NONE was used was to force the CPU
port into being always tagged.

With most configurations enabling Broadcom tags, especially after
8fab459e69ab ("net: dsa: b53: Enable Broadcom tags for 531x5/539x
families") we do not need to apply this unconditional force tagging of
the CPU port in all VLANs.

A helper function is introduced to faciliate the encapsulation of the
specific condition requiring the CPU port to be tagged in all VLANs and
the dsa_switch_ops::untag_bridge_pvid boolean is moved to when
dsa_switch_ops::setup is called when we have already determined the
tagging protocol we will be using.

Reported-by: Matthew Hagan <mnhagan88@gmail.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v2:

- properly deal with DSA_TAG_PROTO_NONE so we continue to support
  that mode on older chips like 5325 and 5365 until they gain Broadcom
  tag support

 drivers/net/dsa/b53/b53_common.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 3ca6b394dd5f..6e199454e41d 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1084,6 +1084,11 @@ static int b53_setup(struct dsa_switch *ds)
 	unsigned int port;
 	int ret;
 
+	/* Request bridge PVID untagged when DSA_TAG_PROTO_NONE is set
+	 * which forces the CPU port to be tagged in all VLANs.
+	 */
+	ds->untag_bridge_pvid = dev->tag_protocol == DSA_TAG_PROTO_NONE;
+
 	ret = b53_reset_switch(dev);
 	if (ret) {
 		dev_err(ds->dev, "failed to reset switch\n");
@@ -1455,6 +1460,13 @@ static int b53_vlan_prepare(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static bool b53_vlan_port_needs_forced_tagged(struct dsa_switch *ds, int port)
+{
+	struct b53_device *dev = ds->priv;
+
+	return dev->tag_protocol == DSA_TAG_PROTO_NONE && dsa_is_cpu_port(ds, port);
+}
+
 int b53_vlan_add(struct dsa_switch *ds, int port,
 		 const struct switchdev_obj_port_vlan *vlan,
 		 struct netlink_ext_ack *extack)
@@ -1477,7 +1489,7 @@ int b53_vlan_add(struct dsa_switch *ds, int port,
 		untagged = true;
 
 	vl->members |= BIT(port);
-	if (untagged && !dsa_is_cpu_port(ds, port))
+	if (untagged && !b53_vlan_port_needs_forced_tagged(ds, port))
 		vl->untag |= BIT(port);
 	else
 		vl->untag &= ~BIT(port);
@@ -1514,7 +1526,7 @@ int b53_vlan_del(struct dsa_switch *ds, int port,
 	if (pvid == vlan->vid)
 		pvid = b53_default_pvid(dev);
 
-	if (untagged && !dsa_is_cpu_port(ds, port))
+	if (untagged && !b53_vlan_port_needs_forced_tagged(ds, port))
 		vl->untag &= ~(BIT(port));
 
 	b53_set_vlan_entry(dev, vlan->vid, vl);
@@ -2660,7 +2672,6 @@ struct b53_device *b53_switch_alloc(struct device *base,
 	dev->priv = priv;
 	dev->ops = ops;
 	ds->ops = &b53_switch_ops;
-	ds->untag_bridge_pvid = true;
 	dev->vlan_enabled = true;
 	/* Let DSA handle the case were multiple bridges span the same switch
 	 * device and different VLAN awareness settings are requested, which
-- 
2.25.1