[PATCH net-next 0/2] net: dsa: b53: support legacy tags

Legacy Broadcom tags are needed for older switches.

Álvaro Fernández Rojas (2):
  net: dsa: tag_brcm: add support for legacy tags
  net: dsa: b53: support legacy tags

 drivers/net/dsa/b53/Kconfig      |  1 +
 drivers/net/dsa/b53/b53_common.c |  9 ++-
 include/net/dsa.h                |  2 +
 net/dsa/Kconfig                  |  7 +++
 net/dsa/tag_brcm.c               | 96 ++++++++++++++++++++++++++++++++
 5 files changed, 113 insertions(+), 2 deletions(-)

-- 
2.20.1

These tags are used on BCM5325, BCM5365 and BCM63xx switches.

Signed-off-by: Álvaro Fernández Rojas <redacted>
---
 drivers/net/dsa/b53/Kconfig      | 1 +
 drivers/net/dsa/b53/b53_common.c | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/b53/Kconfig b/drivers/net/dsa/b53/Kconfig
index f9891a81c808..90b525160b71 100644
--- a/drivers/net/dsa/b53/Kconfig
+++ b/drivers/net/dsa/b53/Kconfig

@@ -3,6 +3,7 @@ menuconfig B53
 	tristate "Broadcom BCM53xx managed switch support"
 	depends on NET_DSA
 	select NET_DSA_TAG_BRCM
+	select NET_DSA_TAG_BRCM_LEGACY
 	select NET_DSA_TAG_BRCM_PREPEND
 	help
 	  This driver adds support for Broadcom managed switch chips. It supports

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index a162499bcafc..a583948cdf4f 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c

@@ -2034,6 +2034,7 @@ static bool b53_can_enable_brcm_tags(struct dsa_switch *ds, int port,
 
 	switch (tag_protocol) {
 	case DSA_TAG_PROTO_BRCM:
+	case DSA_TAG_PROTO_BRCM_LEGACY:
 	case DSA_TAG_PROTO_BRCM_PREPEND:
 		dev_warn(ds->dev,
 			 "Port %d is stacked to Broadcom tag switch\n", port);

@@ -2055,12 +2056,16 @@ enum dsa_tag_protocol b53_get_tag_protocol(struct dsa_switch *ds, int port,
 	/* Older models (5325, 5365) support a different tag format that we do
 	 * not support in net/dsa/tag_brcm.c yet.
 	 */
-	if (is5325(dev) || is5365(dev) ||
-	    !b53_can_enable_brcm_tags(ds, port, mprot)) {
+	if (!b53_can_enable_brcm_tags(ds, port, mprot)) {
 		dev->tag_protocol = DSA_TAG_PROTO_NONE;
 		goto out;
 	}
 
+	if (is5325(dev) || is5365(dev) || is63xx(dev)) {
+		dev->tag_protocol = DSA_TAG_PROTO_BRCM_LEGACY;
+		goto out;
+	}
+
 	/* Broadcom BCM58xx chips have a flow accelerator on Port 8
 	 * which requires us to use the prepended Broadcom tag type
 	 */

-- 
2.20.1

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 <redacted>
---
 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,
 	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"
+	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)
+
 /* This tag length is 4 bytes, older ones were 6 bytes, we do not
  * handle them
  */

@@ -195,6 +209,85 @@ DSA_TAG_DRIVER(brcm_netdev_ops);
 MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_BRCM);
 #endif
 
+#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_LEGACY)
+static struct sk_buff *brcm_leg_tag_xmit(struct sk_buff *skb,
+					 struct net_device *dev)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct bcm_legacy_tag *brcm_tag;
+
+	if (skb_cow_head(skb, BRCM_LEG_TAG_LEN) < 0)
+		return NULL;
+
+	/* The Ethernet switch we are interfaced with needs packets to be at
+	 * least 64 bytes (including FCS) otherwise they will be discarded when
+	 * they enter the switch port logic. When Broadcom tags are enabled, we
+	 * need to make sure that packets are at least 70 bytes
+	 * (including FCS and tag) because the length verification is done after
+	 * the Broadcom tag is stripped off the ingress packet.
+	 *
+	 * Let dsa_slave_xmit() free the SKB
+	 */
+	if (__skb_put_padto(skb, ETH_ZLEN + BRCM_LEG_TAG_LEN, false))
+		return NULL;
+
+	skb_push(skb, BRCM_LEG_TAG_LEN);
+
+	memmove(skb->data, skb->data + BRCM_LEG_TAG_LEN, 2 * ETH_ALEN);
+
+	brcm_tag = (struct bcm_legacy_tag *) (skb->data + 2 * ETH_ALEN);
+
+	brcm_tag->type = BRCM_LEG_TYPE;
+	brcm_tag->tag = BRCM_LEG_TAG_EGRESS;
+	brcm_tag->tag |= dp->index & BRCM_LEG_TAG_PORT_ID;
+
+	return skb;
+}
+
+
+static struct sk_buff *brcm_leg_tag_rcv(struct sk_buff *skb,
+					struct net_device *dev,
+					struct packet_type *pt)
+{
+	int source_port;
+	struct bcm_legacy_tag *brcm_tag;
+
+	if (unlikely(!pskb_may_pull(skb, BRCM_LEG_TAG_LEN)))
+		return NULL;
+
+	brcm_tag = (struct bcm_legacy_tag *) (skb->data - 2);
+
+	source_port = brcm_tag->tag & BRCM_LEG_TAG_PORT_ID;
+
+	skb->dev = dsa_master_find_slave(dev, 0, source_port);
+	if (!skb->dev)
+		return NULL;
+
+	/* Remove Broadcom tag and update checksum */
+	skb_pull_rcsum(skb, BRCM_LEG_TAG_LEN);
+
+	skb->offload_fwd_mark = 1;
+
+	/* Move the Ethernet DA and SA */
+	memmove(skb->data - ETH_HLEN,
+		skb->data - ETH_HLEN - BRCM_LEG_TAG_LEN,
+		2 * ETH_ALEN);
+
+	return skb;
+}
+
+static const struct dsa_device_ops brcm_legacy_netdev_ops = {
+	.name	= "brcm-legacy",
+	.proto	= DSA_TAG_PROTO_BRCM_LEGACY,
+	.xmit	= brcm_leg_tag_xmit,
+	.rcv	= brcm_leg_tag_rcv,
+	.overhead = BRCM_LEG_TAG_LEN,
+};
+
+DSA_TAG_DRIVER(brcm_legacy_netdev_ops);
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_BRCM_LEGACY);
+#endif /* CONFIG_NET_DSA_TAG_BRCM_LEGACY */
+
 #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
 static struct sk_buff *brcm_tag_xmit_prepend(struct sk_buff *skb,
 					     struct net_device *dev)

@@ -227,6 +320,9 @@ static struct dsa_tag_driver *dsa_tag_driver_array[] =	{
 #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM)
 	&DSA_TAG_DRIVER_NAME(brcm_netdev_ops),
 #endif
+#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_LEGACY)
+	&DSA_TAG_DRIVER_NAME(brcm_legacy_netdev_ops),
+#endif
 #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
 	&DSA_TAG_DRIVER_NAME(brcm_prepend_netdev_ops),
 #endif

-- 
2.20.1

On 3/15/2021 7:27 AM, Álvaro Fernández Rojas wrote:

I am not sure about that one, so for now we can probably be
conservative. You can definitively not "stack" two or more switches that
are configured with DSA_TAG_PROTO_BRCM because the first switch
receiving the Broadcom tag will terminate it locally and not pass it up.
The legacy Broadcom tag however is different and has a "Scr Dev ID"
field which is intended to support cascading. Whether that works with
only DSA_TAG_PROTO_BRCM_LEGACY or across DSA_PROTO_BRCM_LEGACY +
DSA_TAG_PROTO_BRCM may be something you will have to determine.

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

On 3/15/2021 7:27 AM, Álvaro Fernández Rojas wrote:

And you could define UNICAST with (0 << 29) just for documentation purposes.

+#define BRCM_LEG_TAG_EGRESS	(2 << 29)
+#define BRCM_LEG_TAG_INGRESS	(3 << 29)
+} __attribute__((packed));

Please define these as relatives within a byte such that only byte
accesses are done, thus eliminating any endian issues, your code for
instance will work fine on a big-endian machine (like the 63xx you have
tested) but not on a little-endian machine.

Other than that, this looks good, thanks!

-- 
Florian

On Mon, Mar 15, 2021 at 03:27:35PM +0100, Álvaro Fernández Rojas wrote:

Is there no better qualifier for this tagging protocol name than "legacy"?

Aren't all headers in-frame?

As Florian pointed out, tagging protocol parsing should be
endian-independent, and mapping a struct over the frame header is pretty
much not that.

This is not needed since commit 2f0d030c5ffe ("net: dsa: tag_brcm: let
DSA core deal with TX reallocation").

+	/* The Ethernet switch we are interfaced with needs packets to be at
+	 * least 64 bytes (including FCS) otherwise they will be discarded when
+	 * they enter the switch port logic. When Broadcom tags are enabled, we
+	 * need to make sure that packets are at least 70 bytes
+	 * (including FCS and tag) because the length verification is done after
+	 * the Broadcom tag is stripped off the ingress packet.
+	 *
+	 * Let dsa_slave_xmit() free the SKB
+	 */
+	if (__skb_put_padto(skb, ETH_ZLEN + BRCM_LEG_TAG_LEN, false))
+		return NULL;

Are you sure the switches you're working on need this, or is it just
another copy-pasta?

+	skb_push(skb, BRCM_LEG_TAG_LEN);
+
+	memmove(skb->data, skb->data + BRCM_LEG_TAG_LEN, 2 * ETH_ALEN);
+
+	brcm_tag = (struct bcm_legacy_tag *) (skb->data + 2 * ETH_ALEN);
+
+	brcm_tag->type = BRCM_LEG_TYPE;
+	brcm_tag->tag = BRCM_LEG_TAG_EGRESS;
+	brcm_tag->tag |= dp->index & BRCM_LEG_TAG_PORT_ID;
+
+	return skb;
+}
+
+

Please remove the extra newline.

+static struct sk_buff *brcm_leg_tag_rcv(struct sk_buff *skb,
+					struct net_device *dev,
+					struct packet_type *pt)
+{
+	int source_port;
+	struct bcm_legacy_tag *brcm_tag;

Please declare the local variables in the order of decreasing line length.

+
+	if (unlikely(!pskb_may_pull(skb, BRCM_LEG_TAG_LEN)))
+		return NULL;
+
+	brcm_tag = (struct bcm_legacy_tag *) (skb->data - 2);

Nitpick: the space between the *) and the (skb-> is not needed.

Hi Vladimir,

El 15 mar 2021, a las 22:28, Vladimir Oltean [off-list ref] escribió:

On Mon, Mar 15, 2021 at 03:27:35PM +0100, Álvaro Fernández Rojas wrote:

quoted

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

quoted

	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"?

quoted

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

quoted

/* This tag length is 4 bytes, older ones were 6 bytes, we do not
 * handle them
 */

@@ -195,6 +209,85 @@ DSA_TAG_DRIVER(brcm_netdev_ops);

MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_BRCM);
#endif

+#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_LEGACY)
+static struct sk_buff *brcm_leg_tag_xmit(struct sk_buff *skb,
+					 struct net_device *dev)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct bcm_legacy_tag *brcm_tag;
+
+	if (skb_cow_head(skb, BRCM_LEG_TAG_LEN) < 0)
+		return NULL;

This is not needed since commit 2f0d030c5ffe ("net: dsa: tag_brcm: let
DSA core deal with TX reallocation").

I’m testing this on v5.10 and I forgot to remove it, sorry :$.

quoted

+	/* The Ethernet switch we are interfaced with needs packets to be at
+	 * least 64 bytes (including FCS) otherwise they will be discarded when
+	 * they enter the switch port logic. When Broadcom tags are enabled, we
+	 * need to make sure that packets are at least 70 bytes
+	 * (including FCS and tag) because the length verification is done after
+	 * the Broadcom tag is stripped off the ingress packet.
+	 *
+	 * Let dsa_slave_xmit() free the SKB
+	 */
+	if (__skb_put_padto(skb, ETH_ZLEN + BRCM_LEG_TAG_LEN, false))
+		return NULL;

Are you sure the switches you're working on need this, or is it just
another copy-pasta?

Yes, I need that.

quoted

+	skb_push(skb, BRCM_LEG_TAG_LEN);
+
+	memmove(skb->data, skb->data + BRCM_LEG_TAG_LEN, 2 * ETH_ALEN);
+
+	brcm_tag = (struct bcm_legacy_tag *) (skb->data + 2 * ETH_ALEN);
+
+	brcm_tag->type = BRCM_LEG_TYPE;
+	brcm_tag->tag = BRCM_LEG_TAG_EGRESS;
+	brcm_tag->tag |= dp->index & BRCM_LEG_TAG_PORT_ID;
+
+	return skb;
+}
+
+

Please remove the extra newline.

Ok.

quoted

+static struct sk_buff *brcm_leg_tag_rcv(struct sk_buff *skb,
+					struct net_device *dev,
+					struct packet_type *pt)
+{
+	int source_port;
+	struct bcm_legacy_tag *brcm_tag;

Please declare the local variables in the order of decreasing line length.

Ok.

quoted

+
+	if (unlikely(!pskb_may_pull(skb, BRCM_LEG_TAG_LEN)))
+		return NULL;
+
+	brcm_tag = (struct bcm_legacy_tag *) (skb->data - 2);

Nitpick: the space between the *) and the (skb-> is not needed.

Ok, but this is going to disappear when I remove the struct usage.

quoted

+
+	source_port = brcm_tag->tag & BRCM_LEG_TAG_PORT_ID;
+
+	skb->dev = dsa_master_find_slave(dev, 0, source_port);
+	if (!skb->dev)
+		return NULL;
+
+	/* Remove Broadcom tag and update checksum */
+	skb_pull_rcsum(skb, BRCM_LEG_TAG_LEN);
+
+	skb->offload_fwd_mark = 1;
+
+	/* Move the Ethernet DA and SA */
+	memmove(skb->data - ETH_HLEN,
+		skb->data - ETH_HLEN - BRCM_LEG_TAG_LEN,
+		2 * ETH_ALEN);
+
+	return skb;
+}
+
+static const struct dsa_device_ops brcm_legacy_netdev_ops = {
+	.name	= "brcm-legacy",
+	.proto	= DSA_TAG_PROTO_BRCM_LEGACY,
+	.xmit	= brcm_leg_tag_xmit,
+	.rcv	= brcm_leg_tag_rcv,
+	.overhead = BRCM_LEG_TAG_LEN,
+};
+
+DSA_TAG_DRIVER(brcm_legacy_netdev_ops);
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_BRCM_LEGACY);
+#endif /* CONFIG_NET_DSA_TAG_BRCM_LEGACY */
+
#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
static struct sk_buff *brcm_tag_xmit_prepend(struct sk_buff *skb,
					     struct net_device *dev)

@@ -227,6 +320,9 @@ static struct dsa_tag_driver *dsa_tag_driver_array[] =	{

#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM)
	&DSA_TAG_DRIVER_NAME(brcm_netdev_ops),
#endif
+#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_LEGACY)
+	&DSA_TAG_DRIVER_NAME(brcm_legacy_netdev_ops),
+#endif
#if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM_PREPEND)
	&DSA_TAG_DRIVER_NAME(brcm_prepend_netdev_ops),
#endif
-- 
2.20.1

Best regards,
Álvaro.

On Wed, 17 Mar 2021 at 10:16, Álvaro Fernández Rojas [off-list ref] wrote:

Hi Vladimir,

quoted

El 15 mar 2021, a las 22:28, Vladimir Oltean [off-list ref] escribió:

On Mon, Mar 15, 2021 at 03:27:35PM +0100, Álvaro Fernández Rojas wrote:

quoted

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 <redacted>
---
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?

quoted

quoted

     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.

quoted

quoted

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

quoted

quoted

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