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

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.