| Message ID | 20210608212204.3978634-1-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2c32a3d3c233b855943677609fe388f82b1f0975 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] net: dsa: b53: Do not force CPU to be always tagged | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 84 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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.
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>
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
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>
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
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?
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.
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
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(-)