Re: [PATCH net-next 1/2] net: dsa: tag_brcm: add support for legacy tags

[Jonas Gorski <jonas.gorski@gmail.com>]

On Wed, 17 Mar 2021 at 10:16, Álvaro Fernández Rojas <noltari@gmail.com> wrote:
>
> Hi Vladimir,
>
> > El 15 mar 2021, a las 22:28, Vladimir Oltean <olteanv@gmail.com> escribió:
> >
> > On Mon, Mar 15, 2021 at 03:27:35PM +0100, Álvaro Fernández Rojas wrote:
> >> Add support for legacy Broadcom tags, which are similar to DSA_TAG_PROTO_BRCM.
> >> These tags are used on BCM5325, BCM5365 and BCM63xx switches.
> >>
> >> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> >> ---
> >> include/net/dsa.h  |  2 +
> >> net/dsa/Kconfig    |  7 ++++
> >> net/dsa/tag_brcm.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 105 insertions(+)
> >>
> >> diff --git a/include/net/dsa.h b/include/net/dsa.h
> >> index 83a933e563fe..dac303edd33d 100644
> >> --- a/include/net/dsa.h
> >> +++ b/include/net/dsa.h
> >> @@ -49,10 +49,12 @@ struct phylink_link_state;
> >> #define DSA_TAG_PROTO_XRS700X_VALUE          19
> >> #define DSA_TAG_PROTO_OCELOT_8021Q_VALUE     20
> >> #define DSA_TAG_PROTO_SEVILLE_VALUE          21
> >> +#define DSA_TAG_PROTO_BRCM_LEGACY_VALUE             22
> >>
> >> enum dsa_tag_protocol {
> >>      DSA_TAG_PROTO_NONE              = DSA_TAG_PROTO_NONE_VALUE,
> >>      DSA_TAG_PROTO_BRCM              = DSA_TAG_PROTO_BRCM_VALUE,
> >> +    DSA_TAG_PROTO_BRCM_LEGACY       = DSA_TAG_PROTO_BRCM_LEGACY_VALUE,
> >
> > Is there no better qualifier for this tagging protocol name than "legacy"?
>
> It’s always referred to as “legacy”, so that’s what I used.
> Maybe @Florian can suggest a better name for this...

Broadcom refers to both as "the BRCM tag" or "the Broadcom Management
Header/Tag" in documentation with no versioning at all.

Codewise, the brcm963xx code names the old one BRCM_TAG and the newer
one BRCM_TAG_TYPE2. Not really better IMHO.

Maybe BRCM_OLD? less characters than Legacy, and doesn't need to be abbreviated.

To make matters worse, there seem to exist different versions of the
tag variants where some opcodes mean different things, e.g. BCM5325
might set the opcode to 1 for Multicast frames.

I would probably suggest enabling it only for switch models we
verified to be working with it.

On a different side node, should the dsa_tag_protocol be ordered
numerically, i.e. should DSA_TAG_PROTO_BRCM_PREPEND be the last one
since it is the highest with 22?

> >
> >>      DSA_TAG_PROTO_BRCM_PREPEND      = DSA_TAG_PROTO_BRCM_PREPEND_VALUE,
> >>      DSA_TAG_PROTO_DSA               = DSA_TAG_PROTO_DSA_VALUE,
> >>      DSA_TAG_PROTO_EDSA              = DSA_TAG_PROTO_EDSA_VALUE,
> >> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> >> index 58b8fc82cd3c..aaf8a452fd5b 100644
> >> --- a/net/dsa/Kconfig
> >> +++ b/net/dsa/Kconfig
> >> @@ -48,6 +48,13 @@ config NET_DSA_TAG_BRCM
> >>        Say Y if you want to enable support for tagging frames for the
> >>        Broadcom switches which place the tag after the MAC source address.
> >>
> >> +config NET_DSA_TAG_BRCM_LEGACY
> >> +    tristate "Tag driver for Broadcom legacy switches using in-frame headers"
> >
> > Aren't all headers in-frame?
>
> I copied that from NET_DSA_TAG_BRCM:
> https://github.com/torvalds/linux/blob/1df27313f50a57497c1faeb6a6ae4ca939c85a7d/net/dsa/Kconfig#L45
>
> Do you want me to change it to "Tag driver for Broadcom legacy switches” or  “Legacy tag driver for Broadcom switches"?

This means that the tag is inserted after the SRC/DST mac addresses,
in contrast to BRCM_PREPEND that gets prepended to the full frame.

> >> +    select NET_DSA_TAG_BRCM_COMMON
> >> +    help
> >> +      Say Y if you want to enable support for tagging frames for the
> >> +      Broadcom legacy switches which place the tag after the MAC source
> >> +      address.
> >>
> >> config NET_DSA_TAG_BRCM_PREPEND
> >>      tristate "Tag driver for Broadcom switches using prepended headers"
> >> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> >> index e2577a7dcbca..9dbff771c9b3 100644
> >> --- a/net/dsa/tag_brcm.c
> >> +++ b/net/dsa/tag_brcm.c
> >> @@ -9,9 +9,23 @@
> >> #include <linux/etherdevice.h>
> >> #include <linux/list.h>
> >> #include <linux/slab.h>
> >> +#include <linux/types.h>
> >>
> >> #include "dsa_priv.h"
> >>
> >> +struct bcm_legacy_tag {
> >> +    uint16_t type;
> >> +#define BRCM_LEG_TYPE       0x8874
> >> +
> >> +    uint32_t tag;
> >> +#define BRCM_LEG_TAG_PORT_ID        (0xf)
> >> +#define BRCM_LEG_TAG_MULTICAST      (1 << 29)
> >> +#define BRCM_LEG_TAG_EGRESS (2 << 29)
> >> +#define BRCM_LEG_TAG_INGRESS        (3 << 29)
> >> +} __attribute__((packed));
> >> +
> >> +#define BRCM_LEG_TAG_LEN    sizeof(struct bcm_legacy_tag)
> >> +
> >
> > As Florian pointed out, tagging protocol parsing should be
> > endian-independent, and mapping a struct over the frame header is pretty
> > much not that.
>
> Ok, I will change that in v2.
>
> >
> >> /* This tag length is 4 bytes, older ones were 6 bytes, we do not
> >>  * handle them

You might want to update this comment, since you now handle them ;-)

Best Regards
Jonas