MIME-Version: 1.0
References: <20230317120815.321871-1-noltari@gmail.com> <00783066-a99c-4bab-ae60-514f4bce687b@lunn.ch>
 <CAOiHx==TiSZKE4AP3PZ9Ah4zuAsrfpOTvRADWpT2kMS9UVRH9Q@mail.gmail.com> <9f771318-5a59-ac31-a333-e2ad9947679f@gmail.com>
In-Reply-To: <9f771318-5a59-ac31-a333-e2ad9947679f@gmail.com>
From:   =?UTF-8?B?w4FsdmFybyBGZXJuw6FuZGV6IFJvamFz?= <noltari@gmail.com>
Date:   Fri, 17 Mar 2023 22:51:45 +0100
Message-ID: <CAKR-sGdgfztvXCymeNSPSoR=C466NzQ-6siiWSUukSAR_-c4-Q@mail.gmail.com>
Subject: Re: [PATCH] net: dsa: tag_brcm: legacy: fix daisy-chained switches
To:     Florian Fainelli <f.fainelli@gmail.com>
Cc:     Jonas Gorski <jonas.gorski@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>, olteanv@gmail.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

El vie, 17 mar 2023 a las 17:55, Florian Fainelli
(<f.fainelli@gmail.com>) escribi=C3=B3:
>
> 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, =C3=81lvaro Fern=C3=A1ndez R=
ojas wrote:
> >>> When BCM63xx internal switches are connected to switches with a 4-byt=
e
> >>> Broadcom tag, it does not identify the packet as VLAN tagged, so it a=
dds 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 correspo=
nding
> >>> 4-byte tag. However, the internal switch adds an invalid VLAN tag aft=
er 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 =3D=3D 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,
=C3=81lvaro.