[PATCH] net: dsa: tag_brcm: legacy: fix daisy-chained switches

[PATCH] net: dsa: tag_brcm: legacy: fix daisy-chained switches

[Álvaro Fernández Rojas]

When BCM63xx internal switches are connected to switches with a 4-byte
Broadcom tag, it does not identify the packet as VLAN tagged, so it adds one
based on its PVID (which is likely 0).
Right now, the packet is received by the BCM63xx internal switch and the 6-byte
tag is properly processed. The next step would to decode the corresponding
4-byte tag. However, the internal switch adds an invalid VLAN tag after the
6-byte tag and the 4-byte tag handling fails.
In order to fix this we need to remove the invalid VLAN tag after the 6-byte
tag before passing it to the 4-byte tag decoding.

Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 net/dsa/tag_brcm.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 10239daa5745..cacdafb41200 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -7,6 +7,7 @@
 
 #include <linux/dsa/brcm.h>
 #include <linux/etherdevice.h>
+#include <linux/if_vlan.h>
 #include <linux/list.h>
 #include <linux/slab.h>
 
@@ -252,6 +253,7 @@ static struct sk_buff *brcm_leg_tag_xmit(struct sk_buff *skb,
 static struct sk_buff *brcm_leg_tag_rcv(struct sk_buff *skb,
 					struct net_device *dev)
 {
+	int len = BRCM_LEG_TAG_LEN;
 	int source_port;
 	u8 *brcm_tag;
 
@@ -266,12 +268,16 @@ 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))
+		len += VLAN_HLEN;
+
 	/* Remove Broadcom tag and update checksum */
-	skb_pull_rcsum(skb, BRCM_LEG_TAG_LEN);
+	skb_pull_rcsum(skb, len);
 
 	dsa_default_offload_fwd_mark(skb);
 
-	dsa_strip_etype_header(skb, BRCM_LEG_TAG_LEN);
+	dsa_strip_etype_header(skb, len);
 
 	return skb;
 }
-- 
2.30.2

Re: [PATCH] net: dsa: tag_brcm: legacy: fix daisy-chained switches

[Michal Swiatkowski]

On Fri, Mar 17, 2023 at 01:08:15PM +0100, Álvaro Fernández Rojas wrote:
> When BCM63xx internal switches are connected to switches with a 4-byte
> Broadcom tag, it does not identify the packet as VLAN tagged, so it adds one
> based on its PVID (which is likely 0).
> Right now, the packet is received by the BCM63xx internal switch and the 6-byte
> tag is properly processed. The next step would to decode the corresponding
> 4-byte tag. However, the internal switch adds an invalid VLAN tag after the
> 6-byte tag and the 4-byte tag handling fails.
> In order to fix this we need to remove the invalid VLAN tag after the 6-byte
> tag before passing it to the 4-byte tag decoding.
> 
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
>  net/dsa/tag_brcm.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> index 10239daa5745..cacdafb41200 100644
> --- a/net/dsa/tag_brcm.c
> +++ b/net/dsa/tag_brcm.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/dsa/brcm.h>
>  #include <linux/etherdevice.h>
> +#include <linux/if_vlan.h>
>  #include <linux/list.h>
>  #include <linux/slab.h>
>  
> @@ -252,6 +253,7 @@ static struct sk_buff *brcm_leg_tag_xmit(struct sk_buff *skb,
>  static struct sk_buff *brcm_leg_tag_rcv(struct sk_buff *skb,
>  					struct net_device *dev)
>  {
> +	int len = BRCM_LEG_TAG_LEN;
>  	int source_port;
>  	u8 *brcm_tag;
>  
> @@ -266,12 +268,16 @@ 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))
> +		len += VLAN_HLEN;
> +
>  	/* Remove Broadcom tag and update checksum */
> -	skb_pull_rcsum(skb, BRCM_LEG_TAG_LEN);
> +	skb_pull_rcsum(skb, len);
>  
>  	dsa_default_offload_fwd_mark(skb);
>  
> -	dsa_strip_etype_header(skb, BRCM_LEG_TAG_LEN);
> +	dsa_strip_etype_header(skb, len);
>  
>  	return skb;
>  }
LGTM, but You can add fixes tag.
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

> -- 
> 2.30.2
> 

Re: [PATCH] net: dsa: tag_brcm: legacy: fix daisy-chained switches

[Álvaro Fernández Rojas]

El vie, 17 mar 2023 a las 13:26, Michal Swiatkowski
(<michal.swiatkowski@linux.intel.com>) escribió:
>
> On Fri, Mar 17, 2023 at 01:08:15PM +0100, Álvaro Fernández Rojas wrote:
> > When BCM63xx internal switches are connected to switches with a 4-byte
> > Broadcom tag, it does not identify the packet as VLAN tagged, so it adds one
> > based on its PVID (which is likely 0).
> > Right now, the packet is received by the BCM63xx internal switch and the 6-byte
> > tag is properly processed. The next step would to decode the corresponding
> > 4-byte tag. However, the internal switch adds an invalid VLAN tag after the
> > 6-byte tag and the 4-byte tag handling fails.
> > In order to fix this we need to remove the invalid VLAN tag after the 6-byte
> > tag before passing it to the 4-byte tag decoding.
> >
> > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> > ---
> >  net/dsa/tag_brcm.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> > index 10239daa5745..cacdafb41200 100644
> > --- a/net/dsa/tag_brcm.c
> > +++ b/net/dsa/tag_brcm.c
> > @@ -7,6 +7,7 @@
> >
> >  #include <linux/dsa/brcm.h>
> >  #include <linux/etherdevice.h>
> > +#include <linux/if_vlan.h>
> >  #include <linux/list.h>
> >  #include <linux/slab.h>
> >
> > @@ -252,6 +253,7 @@ static struct sk_buff *brcm_leg_tag_xmit(struct sk_buff *skb,
> >  static struct sk_buff *brcm_leg_tag_rcv(struct sk_buff *skb,
> >                                       struct net_device *dev)
> >  {
> > +     int len = BRCM_LEG_TAG_LEN;
> >       int source_port;
> >       u8 *brcm_tag;
> >
> > @@ -266,12 +268,16 @@ 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))
> > +             len += VLAN_HLEN;
> > +
> >       /* Remove Broadcom tag and update checksum */
> > -     skb_pull_rcsum(skb, BRCM_LEG_TAG_LEN);
> > +     skb_pull_rcsum(skb, len);
> >
> >       dsa_default_offload_fwd_mark(skb);
> >
> > -     dsa_strip_etype_header(skb, BRCM_LEG_TAG_LEN);
> > +     dsa_strip_etype_header(skb, len);
> >
> >       return skb;
> >  }
> LGTM, but You can add fixes tag.
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

Thanks Michal, I will give time for others to review this before
sending v2 with the fixes tag.

>
> > --
> > 2.30.2
> >

--
Álvaro

Re: [PATCH] net: dsa: tag_brcm: legacy: fix daisy-chained switches

[Andrew Lunn]

On Fri, Mar 17, 2023 at 01:08:15PM +0100, Álvaro Fernández Rojas wrote:
> When BCM63xx internal switches are connected to switches with a 4-byte
> Broadcom tag, it does not identify the packet as VLAN tagged, so it adds one
> based on its PVID (which is likely 0).
> Right now, the packet is received by the BCM63xx internal switch and the 6-byte
> tag is properly processed. The next step would to decode the corresponding
> 4-byte tag. However, the internal switch adds an invalid VLAN tag after the
> 6-byte tag and the 4-byte tag handling fails.
> In order to fix this we need to remove the invalid VLAN tag after the 6-byte
> tag before passing it to the 4-byte tag decoding.

Is there an errata for this invalid VLAN tag? Or is the driver simply
missing some configuration for it to produce a valid VLAN tag?

The description does not convince me you are fixing the correct
problem.

	Andrew

Re: [PATCH] net: dsa: tag_brcm: legacy: fix daisy-chained switches

[Florian Fainelli]

On 3/17/23 09:32, Andrew Lunn wrote:
> On Fri, Mar 17, 2023 at 01:08:15PM +0100, Álvaro Fernández Rojas wrote:
>> When BCM63xx internal switches are connected to switches with a 4-byte
>> Broadcom tag, it does not identify the packet as VLAN tagged, so it adds one
>> based on its PVID (which is likely 0).
>> Right now, the packet is received by the BCM63xx internal switch and the 6-byte
>> tag is properly processed. The next step would to decode the corresponding
>> 4-byte tag. However, the internal switch adds an invalid VLAN tag after the
>> 6-byte tag and the 4-byte tag handling fails.
>> In order to fix this we need to remove the invalid VLAN tag after the 6-byte
>> tag before passing it to the 4-byte tag decoding.
> 
> Is there an errata for this invalid VLAN tag? Or is the driver simply
> missing some configuration for it to produce a valid VLAN tag?
> 
> The description does not convince me you are fixing the correct
> problem.

I do not think this is going to work, except in the very narrow 
configuration that it has been tested with. If you configure VLANs on 
the BCM63xx internal switch, this will be stripping of any VLAN tag, 
thus breaking the data path.

It is not clear to me how to best resolve this, short of trying to 
coerce the BCM63xx internal switch into *not* tagging all ingress frames 
by default.
-- 
Florian

Re: [PATCH] net: dsa: tag_brcm: legacy: fix daisy-chained switches

[Álvaro Fernández Rojas]

El vie, 17 mar 2023 a las 17:32, Andrew Lunn (<andrew@lunn.ch>) escribió:
>
> On Fri, Mar 17, 2023 at 01:08:15PM +0100, Álvaro Fernández Rojas wrote:
> > When BCM63xx internal switches are connected to switches with a 4-byte
> > Broadcom tag, it does not identify the packet as VLAN tagged, so it adds one
> > based on its PVID (which is likely 0).
> > Right now, the packet is received by the BCM63xx internal switch and the 6-byte
> > tag is properly processed. The next step would to decode the corresponding
> > 4-byte tag. However, the internal switch adds an invalid VLAN tag after the
> > 6-byte tag and the 4-byte tag handling fails.
> > In order to fix this we need to remove the invalid VLAN tag after the 6-byte
> > tag before passing it to the 4-byte tag decoding.
>
> Is there an errata for this invalid VLAN tag? Or is the driver simply
> missing some configuration for it to produce a valid VLAN tag?

Yes, this is a HW issue due to the fact that Broadcom Legacy switches
which use a 6-byte tag cannot identify the tag of newer Broadcom
switches using a 4-byte tag and therefore adding their own tag along
with a default VLAN tag ignoring the corresponding untag bit in those
ports.
But Florian and Jonas can probably provide more information about this
HW issue, and also this link too:
https://github.com/openwrt/openwrt/issues/10313

>
> The description does not convince me you are fixing the correct
> problem.

Well, if you think that I should fix the configuration then I'm afraid
you're wrong.

>
>         Andrew

--
Álvaro

Re: [PATCH] net: dsa: tag_brcm: legacy: fix daisy-chained switches

[Jonas Gorski]

On Fri, 17 Mar 2023 at 17:32, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Fri, Mar 17, 2023 at 01:08:15PM +0100, Álvaro Fernández Rojas wrote:
> > When BCM63xx internal switches are connected to switches with a 4-byte
> > Broadcom tag, it does not identify the packet as VLAN tagged, so it adds one
> > based on its PVID (which is likely 0).
> > Right now, the packet is received by the BCM63xx internal switch and the 6-byte
> > tag is properly processed. The next step would to decode the corresponding
> > 4-byte tag. However, the internal switch adds an invalid VLAN tag after the
> > 6-byte tag and the 4-byte tag handling fails.
> > In order to fix this we need to remove the invalid VLAN tag after the 6-byte
> > tag before passing it to the 4-byte tag decoding.
>
> Is there an errata for this invalid VLAN tag? Or is the driver simply
> missing some configuration for it to produce a valid VLAN tag?
>
> The description does not convince me you are fixing the correct
> problem.

This isn't a bug per se, it's just the interaction of a packet going
through two tagging CPU ports.

My understanding of the behaviour is:

1. The external switch inserts a 4-byte Broadcom header before the
VLAN tag, and sends it to the internal switch.
2. The internal switch looks at the EtherType, finds it is not a VLAN
EtherType, so assumes it is untagged, and adds a VLAN tag based on the
configured PVID (which 0 in the default case).
3. The internal switch inserts a legacy 6-byte Broadcom header before
the VLAN tag when forwarding to its CPU port.

The internal switch does not know how to handle the (non-legacy)
Broadcom tag, so it does not know that there is a VLAN tag after it.

The internal switch enforces VLAN tags on its CPU port when it is in
VLAN enabled mode, regardless what the VLAN table's untag bit says.

The result is a bogus VID 0 and priority 0 tag between the two
Broadcom Headers. The VID would likely change based on the PVID of the
port of the external switch.

Jonas

Re: [PATCH] net: dsa: tag_brcm: legacy: fix daisy-chained switches

[Florian Fainelli]

On 3/17/23 09:49, Jonas Gorski wrote:
> On Fri, 17 Mar 2023 at 17:32, Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> On Fri, Mar 17, 2023 at 01:08:15PM +0100, Álvaro Fernández Rojas wrote:
>>> When BCM63xx internal switches are connected to switches with a 4-byte
>>> Broadcom tag, it does not identify the packet as VLAN tagged, so it adds one
>>> based on its PVID (which is likely 0).
>>> Right now, the packet is received by the BCM63xx internal switch and the 6-byte
>>> tag is properly processed. The next step would to decode the corresponding
>>> 4-byte tag. However, the internal switch adds an invalid VLAN tag after the
>>> 6-byte tag and the 4-byte tag handling fails.
>>> In order to fix this we need to remove the invalid VLAN tag after the 6-byte
>>> tag before passing it to the 4-byte tag decoding.
>>
>> Is there an errata for this invalid VLAN tag? Or is the driver simply
>> missing some configuration for it to produce a valid VLAN tag?
>>
>> The description does not convince me you are fixing the correct
>> problem.
> 
> This isn't a bug per se, it's just the interaction of a packet going
> through two tagging CPU ports.
> 
> My understanding of the behaviour is:
> 
> 1. The external switch inserts a 4-byte Broadcom header before the
> VLAN tag, and sends it to the internal switch.
> 2. The internal switch looks at the EtherType, finds it is not a VLAN
> EtherType, so assumes it is untagged, and adds a VLAN tag based on the
> configured PVID (which 0 in the default case).
> 3. The internal switch inserts a legacy 6-byte Broadcom header before
> the VLAN tag when forwarding to its CPU port.
> 
> The internal switch does not know how to handle the (non-legacy)
> Broadcom tag, so it does not know that there is a VLAN tag after it.
> 
> The internal switch enforces VLAN tags on its CPU port when it is in
> VLAN enabled mode, regardless what the VLAN table's untag bit says.
> 
> The result is a bogus VID 0 and priority 0 tag between the two
> Broadcom Headers. The VID would likely change based on the PVID of the
> port of the external switch.

My understanding matches yours, at the very least, we should only strip 
off the VLAN tag == 0, in case we are stacked onto a 4-bytes Broadcom 
tag speaking switch, otherwise it seems to me we are stripping of VLAN 
tags a bait too greedily.
-- 
Florian

Re: [PATCH] net: dsa: tag_brcm: legacy: fix daisy-chained switches

[Álvaro Fernández Rojas]

El vie, 17 mar 2023 a las 17:55, Florian Fainelli
(<f.fainelli@gmail.com>) escribió:
>
> On 3/17/23 09:49, Jonas Gorski wrote:
> > On Fri, 17 Mar 2023 at 17:32, Andrew Lunn <andrew@lunn.ch> wrote:
> >>
> >> On Fri, Mar 17, 2023 at 01:08:15PM +0100, Álvaro Fernández Rojas wrote:
> >>> When BCM63xx internal switches are connected to switches with a 4-byte
> >>> Broadcom tag, it does not identify the packet as VLAN tagged, so it adds one
> >>> based on its PVID (which is likely 0).
> >>> Right now, the packet is received by the BCM63xx internal switch and the 6-byte
> >>> tag is properly processed. The next step would to decode the corresponding
> >>> 4-byte tag. However, the internal switch adds an invalid VLAN tag after the
> >>> 6-byte tag and the 4-byte tag handling fails.
> >>> In order to fix this we need to remove the invalid VLAN tag after the 6-byte
> >>> tag before passing it to the 4-byte tag decoding.
> >>
> >> Is there an errata for this invalid VLAN tag? Or is the driver simply
> >> missing some configuration for it to produce a valid VLAN tag?
> >>
> >> The description does not convince me you are fixing the correct
> >> problem.
> >
> > This isn't a bug per se, it's just the interaction of a packet going
> > through two tagging CPU ports.
> >
> > My understanding of the behaviour is:
> >
> > 1. The external switch inserts a 4-byte Broadcom header before the
> > VLAN tag, and sends it to the internal switch.
> > 2. The internal switch looks at the EtherType, finds it is not a VLAN
> > EtherType, so assumes it is untagged, and adds a VLAN tag based on the
> > configured PVID (which 0 in the default case).
> > 3. The internal switch inserts a legacy 6-byte Broadcom header before
> > the VLAN tag when forwarding to its CPU port.
> >
> > The internal switch does not know how to handle the (non-legacy)
> > Broadcom tag, so it does not know that there is a VLAN tag after it.
> >
> > The internal switch enforces VLAN tags on its CPU port when it is in
> > VLAN enabled mode, regardless what the VLAN table's untag bit says.
> >
> > The result is a bogus VID 0 and priority 0 tag between the two
> > Broadcom Headers. The VID would likely change based on the PVID of the
> > port of the external switch.
>
> My understanding matches yours, at the very least, we should only strip
> off the VLAN tag == 0, in case we are stacked onto a 4-bytes Broadcom
> tag speaking switch, otherwise it seems to me we are stripping of VLAN
> tags a bait too greedily.

Maybe I'm wrong here, but we're only removing the VLAN tag for a
specific case in which we shouldn't have any kind of VLAN tag, right?

For example, let's say we have an internal switch with the following ports:
- 0: LAN 1
- 4: RGMII -> External switch
- 8: CPU -> enetsw controller

And the external switch has the following ports:
- 0: LAN 2
- 1: LAN 3
...
- 8: CPU -> Internal switch RGMII

A. If we get a packet from LAN 1, it will only have the 6-bytes tag
(and optionally the VLAN tag).
When dsa_master_find_slave() is called, the net_device returned won't
have any kind of DSA protocol and therefore netdev_uses_dsa() will
return FALSE.

B. However, when a packet is received from LAN 2/3, the first tag
processed will be the 6-byte tag (corresponding to the internal
switch).
The 6-byte tag will identify this as coming from port 4 of the
internal switch (RGMII) and therefore dsa_master_find_slave() will
return the extsw interface which will have the DSA protocol of the
4-byte tag and netdev_uses_dsa() will return TRUE.

Only for the second case the invalid VLAN tag will be removed and
since extsw (RGMI) will never have VLANs enabled, I don't see the
problem that you suggest about removing the VLAN tags too greedily.

Am I wrong here?

> --
> Florian
>

Best regards,
Álvaro.

Re: [PATCH] net: dsa: tag_brcm: legacy: fix daisy-chained switches

[Florian Fainelli]

On 3/17/23 14:51, Álvaro Fernández Rojas wrote:
> El vie, 17 mar 2023 a las 17:55, Florian Fainelli
> (<f.fainelli@gmail.com>) escribió:
>>
>> On 3/17/23 09:49, Jonas Gorski wrote:
>>> On Fri, 17 Mar 2023 at 17:32, Andrew Lunn <andrew@lunn.ch> wrote:
>>>>
>>>> On Fri, Mar 17, 2023 at 01:08:15PM +0100, Álvaro Fernández Rojas wrote:
>>>>> When BCM63xx internal switches are connected to switches with a 4-byte
>>>>> Broadcom tag, it does not identify the packet as VLAN tagged, so it adds one
>>>>> based on its PVID (which is likely 0).
>>>>> Right now, the packet is received by the BCM63xx internal switch and the 6-byte
>>>>> tag is properly processed. The next step would to decode the corresponding
>>>>> 4-byte tag. However, the internal switch adds an invalid VLAN tag after the
>>>>> 6-byte tag and the 4-byte tag handling fails.
>>>>> In order to fix this we need to remove the invalid VLAN tag after the 6-byte
>>>>> tag before passing it to the 4-byte tag decoding.
>>>>
>>>> Is there an errata for this invalid VLAN tag? Or is the driver simply
>>>> missing some configuration for it to produce a valid VLAN tag?
>>>>
>>>> The description does not convince me you are fixing the correct
>>>> problem.
>>>
>>> This isn't a bug per se, it's just the interaction of a packet going
>>> through two tagging CPU ports.
>>>
>>> My understanding of the behaviour is:
>>>
>>> 1. The external switch inserts a 4-byte Broadcom header before the
>>> VLAN tag, and sends it to the internal switch.
>>> 2. The internal switch looks at the EtherType, finds it is not a VLAN
>>> EtherType, so assumes it is untagged, and adds a VLAN tag based on the
>>> configured PVID (which 0 in the default case).
>>> 3. The internal switch inserts a legacy 6-byte Broadcom header before
>>> the VLAN tag when forwarding to its CPU port.
>>>
>>> The internal switch does not know how to handle the (non-legacy)
>>> Broadcom tag, so it does not know that there is a VLAN tag after it.
>>>
>>> The internal switch enforces VLAN tags on its CPU port when it is in
>>> VLAN enabled mode, regardless what the VLAN table's untag bit says.
>>>
>>> The result is a bogus VID 0 and priority 0 tag between the two
>>> Broadcom Headers. The VID would likely change based on the PVID of the
>>> port of the external switch.
>>
>> My understanding matches yours, at the very least, we should only strip
>> off the VLAN tag == 0, in case we are stacked onto a 4-bytes Broadcom
>> tag speaking switch, otherwise it seems to me we are stripping of VLAN
>> tags a bait too greedily.
> 
> Maybe I'm wrong here, but we're only removing the VLAN tag for a
> specific case in which we shouldn't have any kind of VLAN tag, right?
> 
> For example, let's say we have an internal switch with the following ports:
> - 0: LAN 1
> - 4: RGMII -> External switch
> - 8: CPU -> enetsw controller
> 
> And the external switch has the following ports:
> - 0: LAN 2
> - 1: LAN 3
> ...
> - 8: CPU -> Internal switch RGMII
> 
> A. If we get a packet from LAN 1, it will only have the 6-bytes tag
> (and optionally the VLAN tag).
> When dsa_master_find_slave() is called, the net_device returned won't
> have any kind of DSA protocol and therefore netdev_uses_dsa() will
> return FALSE.
> 
> B. However, when a packet is received from LAN 2/3, the first tag
> processed will be the 6-byte tag (corresponding to the internal
> switch).
> The 6-byte tag will identify this as coming from port 4 of the
> internal switch (RGMII) and therefore dsa_master_find_slave() will
> return the extsw interface which will have the DSA protocol of the
> 4-byte tag and netdev_uses_dsa() will return TRUE.
> 
> Only for the second case the invalid VLAN tag will be removed and
> since extsw (RGMI) will never have VLANs enabled, I don't see the
> problem that you suggest about removing the VLAN tags too greedily.
> 
> Am I wrong here?

I totally missed the netdev_uses_dsa() check you added on the looked up 
net_device, your explanation makes sense to me, thanks!
-- 
Florian

[PATCH v2] net: dsa: tag_brcm: legacy: fix daisy-chained switches

[Álvaro Fernández Rojas]

When BCM63xx internal switches are connected to switches with a 4-byte
Broadcom tag, it does not identify the packet as VLAN tagged, so it adds one
based on its PVID (which is likely 0).
Right now, the packet is received by the BCM63xx internal switch and the 6-byte
tag is properly processed. The next step would to decode the corresponding
4-byte tag. However, the internal switch adds an invalid VLAN tag after the
6-byte tag and the 4-byte tag handling fails.
In order to fix this we need to remove the invalid VLAN tag after the 6-byte
tag before passing it to the 4-byte tag decoding.

Fixes: 964dbf186eaa ("net: dsa: tag_brcm: add support for legacy tags")
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
---
 v2: add missing fixes tag.

 net/dsa/tag_brcm.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 10239daa5745..cacdafb41200 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -7,6 +7,7 @@
 
 #include <linux/dsa/brcm.h>
 #include <linux/etherdevice.h>
+#include <linux/if_vlan.h>
 #include <linux/list.h>
 #include <linux/slab.h>
 
@@ -252,6 +253,7 @@ static struct sk_buff *brcm_leg_tag_xmit(struct sk_buff *skb,
 static struct sk_buff *brcm_leg_tag_rcv(struct sk_buff *skb,
 					struct net_device *dev)
 {
+	int len = BRCM_LEG_TAG_LEN;
 	int source_port;
 	u8 *brcm_tag;
 
@@ -266,12 +268,16 @@ 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))
+		len += VLAN_HLEN;
+
 	/* Remove Broadcom tag and update checksum */
-	skb_pull_rcsum(skb, BRCM_LEG_TAG_LEN);
+	skb_pull_rcsum(skb, len);
 
 	dsa_default_offload_fwd_mark(skb);
 
-	dsa_strip_etype_header(skb, BRCM_LEG_TAG_LEN);
+	dsa_strip_etype_header(skb, len);
 
 	return skb;
 }
-- 
2.30.2

Re: [PATCH v2] net: dsa: tag_brcm: legacy: fix daisy-chained switches

[Jakub Kicinski]

On Sun, 19 Mar 2023 10:55:40 +0100 Álvaro Fernández Rojas wrote:
> When BCM63xx internal switches are connected to switches with a 4-byte
> Broadcom tag, it does not identify the packet as VLAN tagged, so it adds one
> based on its PVID (which is likely 0).
> Right now, the packet is received by the BCM63xx internal switch and the 6-byte
> tag is properly processed. The next step would to decode the corresponding
> 4-byte tag. However, the internal switch adds an invalid VLAN tag after the
> 6-byte tag and the 4-byte tag handling fails.
> In order to fix this we need to remove the invalid VLAN tag after the 6-byte
> tag before passing it to the 4-byte tag decoding.

Is it good to go in, Florian?

Re: [PATCH v2] net: dsa: tag_brcm: legacy: fix daisy-chained switches

[Florian Fainelli]

On 3/19/23 02:55, Álvaro Fernández Rojas wrote:
> When BCM63xx internal switches are connected to switches with a 4-byte
> Broadcom tag, it does not identify the packet as VLAN tagged, so it adds one
> based on its PVID (which is likely 0).
> Right now, the packet is received by the BCM63xx internal switch and the 6-byte
> tag is properly processed. The next step would to decode the corresponding
> 4-byte tag. However, the internal switch adds an invalid VLAN tag after the
> 6-byte tag and the 4-byte tag handling fails.
> In order to fix this we need to remove the invalid VLAN tag after the 6-byte
> tag before passing it to the 4-byte tag decoding.
> 
> Fixes: 964dbf186eaa ("net: dsa: tag_brcm: add support for legacy tags")
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v2] net: dsa: tag_brcm: legacy: fix daisy-chained switches

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Sun, 19 Mar 2023 10:55:40 +0100 you wrote:
> When BCM63xx internal switches are connected to switches with a 4-byte
> Broadcom tag, it does not identify the packet as VLAN tagged, so it adds one
> based on its PVID (which is likely 0).
> Right now, the packet is received by the BCM63xx internal switch and the 6-byte
> tag is properly processed. The next step would to decode the corresponding
> 4-byte tag. However, the internal switch adds an invalid VLAN tag after the
> 6-byte tag and the 4-byte tag handling fails.
> In order to fix this we need to remove the invalid VLAN tag after the 6-byte
> tag before passing it to the 4-byte tag decoding.
> 
> [...]

Here is the summary with links:
  - [v2] net: dsa: tag_brcm: legacy: fix daisy-chained switches
    https://git.kernel.org/netdev/net/c/032a954061af

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

Re: [PATCH] net: dsa: tag_brcm: legacy: fix daisy-chained switches

[Florian Fainelli]

On 3/17/23 05:08, Álvaro Fernández Rojas wrote:
> When BCM63xx internal switches are connected to switches with a 4-byte
> Broadcom tag, it does not identify the packet as VLAN tagged, so it adds one
> based on its PVID (which is likely 0).
> Right now, the packet is received by the BCM63xx internal switch and the 6-byte
> tag is properly processed. The next step would to decode the corresponding
> 4-byte tag. However, the internal switch adds an invalid VLAN tag after the
> 6-byte tag and the 4-byte tag handling fails.
> In order to fix this we need to remove the invalid VLAN tag after the 6-byte
> tag before passing it to the 4-byte tag decoding.
> 
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian