Re: [PATCH net] net: dsa: tag_brcm: legacy: fix untagged rx on unbridged ports for bcm63xx

[Simon Horman]

On Wed, Oct 15, 2025 at 06:24:33PM +0200, Jonas Gorski wrote:
> On Wed, Oct 15, 2025 at 6:12 PM Simon Horman <horms@xxxxxxxxxx> wrote:
> >
> > 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@xxxxxxxxx>
> >
> > ...
> >
> > > @@ -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.
> >
> > Maybe it isn't important, but here it is a TCI 0 that is being checked:
> > VID 0, PCP 0, and DEI 0.
> 
> Right, that is intentional (I tried to convey it in the commit
> message, though should probably also extend it here).

Thanks, I see that more clearly now.

> If any of the fields is non-zero, then the tag is meaningful, and we
> don't want to strip it (e.g. 802.1p tagged packets).

I guess there are already a lot of words there. But, FWIIW, I would lean
to wards tightening up the comment a bit.