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

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

[Jonas Gorski]

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@gmail.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

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

[Simon Horman]

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@gmail.com>

...

> @@ -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.

> +	 */
> +	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
> 

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

[Jonas Gorski]

On Wed, Oct 15, 2025 at 6:12 PM Simon Horman <horms@kernel.org> 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@gmail.com>
>
> ...
>
> > @@ -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).

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).

Best regards,
Jonas

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@kernel.org> 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@gmail.com>
> >
> > ...
> >
> > > @@ -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.

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

[Jonas Gorski]

On Wed, Oct 15, 2025 at 6:43 PM Simon Horman <horms@kernel.org> wrote:
>
> 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@kernel.org> 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@gmail.com>
> > >
> > > ...
> > >
> > > > @@ -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.

While I can't send patches, I can at least send emails at work:

How about:

"The internal switch in BCM63XX SoCs ignores the CPU port's egress
untag bit. We use VID 0 internally for untagged traffic, so it becomes
tagged with VID 0. Fix this up by removing the tag if the TCI field is
all 0, and keep it otherwise."

Best regards,
Jonas

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

[Vladimir Oltean]

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@gmail.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)?

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.

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

[Jonas Gorski]

Hi,

On Thu, Oct 16, 2025 at 12:27 PM Vladimir Oltean <olteanv@gmail.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@gmail.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

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

[Paolo Abeni]

On 10/16/25 1:50 PM, Jonas Gorski wrote:
> On Thu, Oct 16, 2025 at 12:27 PM Vladimir Oltean <olteanv@gmail.com> 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@gmail.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.

I think this patch makes sense for 'net' and reaching stable trees,
where most b53 users sits (I think/guess).

The DSA-core base solution could be a follow-up IMHO.

@Jonas, please still clarify a bit the comment, as per Simon's request.

Thanks,

Paolo

> 
> 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.

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

[Jonas Gorski]

On Tue, Oct 21, 2025 at 9:08 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On 10/16/25 1:50 PM, Jonas Gorski wrote:
> > On Thu, Oct 16, 2025 at 12:27 PM Vladimir Oltean <olteanv@gmail.com> 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@gmail.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.
>
> I think this patch makes sense for 'net' and reaching stable trees,
> where most b53 users sits (I think/guess).
>
> The DSA-core base solution could be a follow-up IMHO.
>
> @Jonas, please still clarify a bit the comment, as per Simon's request.

That was my plan until my internet connection at home went down
yesterday, and hasn't been fixed since. Once the ISP fixed it I will
send out a V2 (+/- me being back home from work)

Best regards,
Jonas