[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAOiHx=mNnMJTnAN35D6=LPYVTQB+oEmedwqrkA6VRLRVi13Kjw@mail.gmail.com>
Date: Thu, 16 Oct 2025 13:50:53 +0200
From: Jonas Gorski <jonas.gorski@...il.com>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Simon Horman <horms@...nel.org>, Álvaro Fernández Rojas <noltari@...il.com>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: dsa: tag_brcm: legacy: fix untagged rx on
unbridged ports for bcm63xx
Hi,
On Thu, Oct 16, 2025 at 12:27 PM Vladimir Oltean <olteanv@...il.com> wrote:
>
> Hi Jonas,
>
> On Wed, Oct 15, 2025 at 09:08:54AM +0200, Jonas Gorski wrote:
> > The internal switch on BCM63XX SoCs will unconditionally add 802.1Q VLAN
> > tags on egress to CPU when 802.1Q mode is enabled. We do this
> > unconditionally since commit ed409f3bbaa5 ("net: dsa: b53: Configure
> > VLANs while not filtering").
> >
> > This is fine for VLAN aware bridges, but for standalone ports and vlan
> > unaware bridges this means all packets are tagged with the default VID,
> > which is 0.
> >
> > While the kernel will treat that like untagged, this can break userspace
> > applications processing raw packets, expecting untagged traffic, like
> > STP daemons.
> >
> > This also breaks several bridge tests, where the tcpdump output then
> > does not match the expected output anymore.
> >
> > Since 0 isn't a valid VID, just strip out the VLAN tag if we encounter
> > it, unless the priority field is set, since that would be a valid tag
> > again.
> >
> > Fixes: 964dbf186eaa ("net: dsa: tag_brcm: add support for legacy tags")
> > Signed-off-by: Jonas Gorski <jonas.gorski@...il.com>
> > ---
> > net/dsa/tag_brcm.c | 12 ++++++++++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> > index 26bb657ceac3..32879d1b908b 100644
> > --- a/net/dsa/tag_brcm.c
> > +++ b/net/dsa/tag_brcm.c
> > @@ -224,12 +224,14 @@ static struct sk_buff *brcm_leg_tag_rcv(struct sk_buff *skb,
> > {
> > int len = BRCM_LEG_TAG_LEN;
> > int source_port;
> > + __be16 *proto;
> > u8 *brcm_tag;
> >
> > if (unlikely(!pskb_may_pull(skb, BRCM_LEG_TAG_LEN + VLAN_HLEN)))
> > return NULL;
> >
> > brcm_tag = dsa_etype_header_pos_rx(skb);
> > + proto = (__be16 *)(brcm_tag + BRCM_LEG_TAG_LEN);
> >
> > source_port = brcm_tag[5] & BRCM_LEG_PORT_ID;
> >
> > @@ -237,8 +239,14 @@ static struct sk_buff *brcm_leg_tag_rcv(struct sk_buff *skb,
> > if (!skb->dev)
> > return NULL;
> >
> > - /* VLAN tag is added by BCM63xx internal switch */
> > - if (netdev_uses_dsa(skb->dev))
> > + /* The internal switch in BCM63XX SoCs will add a 802.1Q VLAN tag on
> > + * egress to the CPU port for all packets, regardless of the untag bit
> > + * in the VLAN table. VID 0 is used for untagged traffic on unbridged
> > + * ports and vlan unaware bridges. If we encounter a VID 0 tagged
> > + * packet, we know it is supposed to be untagged, so strip the VLAN
> > + * tag as well in that case.
> > + */
> > + if (proto[0] == htons(ETH_P_8021Q) && proto[1] == 0)
> > len += VLAN_HLEN;
> >
> > /* Remove Broadcom tag and update checksum */
> >
> > base-commit: 7f0fddd817ba6daebea1445ae9fab4b6d2294fa8
> > --
> > 2.43.0
> >
>
> Do I understand correctly the following:
>
> - b53_default_pvid() returns 0 for this switch
> - dsa_software_untag_vlan_unaware_bridge() does not remove it, because,
> as the FIXME says, 0 is not the PVID of the VLAN-unaware bridge (and
> even if it were, the same problem exists for standalone ports and is
> not tackled by that logic)?
In general yes. And it happens to work for vlan aware bridges because
br_get_pvid() returns 0 if a port has no PVID configured.
Also b53 doesn't set untag_bridge_pvid except in very weird edge
cases, so dsa_software_untag_vlan_unaware_bridge() isn't even called
;-)
> I'm trying to gauge the responsibility split between taggers and
> dsa_software_vlan_untag(). We could consider implementing the missing
> bits in that function and letting the generic untagging logic do it.
If there are more devices that need this, it might make sense. Not
sure if this has any negative performance impact compared to directly
stripping it along the proprietary tag.
And to sidetrack the discussion a bit, I wonder if calling
__vlan_hwaccel_clear_tag() in
dsa_software_untag_vlan_(un)aware_bridge() without checking the
vlan_tci field strips 802.1p information from packets that have it. I
fail to find if this is already parsed and stored somewhere at a first
glance.
Best regards,
Jonas
Powered by blists - more mailing lists