[PATCH v2 net-next 0/2] net: dsa: tag_dsa: Unify regular and ethertype DSA taggers

[PATCH v2 net-next 0/2] net: dsa: tag_dsa: Unify regular and ethertype DSA taggers

[Tobias Waldekranz]

The first commit does the heavy lifting of actually fusing tag_dsa.c
and tag_edsa.c. Second commit just follows up with some clean up of
existing comments.

v1 -> v2:
  - Fixed some grammar and whitespace errors.
  - Removed unnecessary default value in Kconfig.
  - Removed unnecessary #ifdef.
  - Split out comment fixes from functional changes.
  - Fully document enum dsa_code.

Tobias Waldekranz (2):
  net: dsa: tag_dsa: Unify regular and ethertype DSA taggers
  net: dsa: tag_dsa: Use a consistent comment style

 net/dsa/Kconfig    |   5 +
 net/dsa/Makefile   |   3 +-
 net/dsa/tag_dsa.c  | 315 ++++++++++++++++++++++++++++++++++++---------
 net/dsa/tag_edsa.c | 202 -----------------------------
 4 files changed, 257 insertions(+), 268 deletions(-)
 delete mode 100644 net/dsa/tag_edsa.c

-- 
2.17.1

[PATCH v2 net-next 1/2] net: dsa: tag_dsa: Unify regular and ethertype DSA taggers

[Tobias Waldekranz]

Ethertype DSA encodes exactly the same information in the DSA tag as
the non-ethertype variety. So refactor out the common parts and reuse
them for both protocols.

This is ensures tag parsing and generation is always consistent across
all mv88e6xxx chips.

While we are at it, explicitly deal with all possible CPU codes on
receive, making sure to set offload_fwd_mark as appropriate.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 net/dsa/Kconfig    |   5 +
 net/dsa/Makefile   |   3 +-
 net/dsa/tag_dsa.c  | 287 ++++++++++++++++++++++++++++++++++++++-------
 net/dsa/tag_edsa.c | 202 -------------------------------
 4 files changed, 249 insertions(+), 248 deletions(-)
 delete mode 100644 net/dsa/tag_edsa.c

diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index d975614f7dd6..dfecd7b22fd7 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -68,14 +68,19 @@ config NET_DSA_TAG_GSWIP
 	  Say Y or M if you want to enable support for tagging frames for the
 	  Lantiq / Intel GSWIP switches.
 
+config NET_DSA_TAG_DSA_COMMON
+	tristate
+
 config NET_DSA_TAG_DSA
 	tristate "Tag driver for Marvell switches using DSA headers"
+	select NET_DSA_TAG_DSA_COMMON
 	help
 	  Say Y or M if you want to enable support for tagging frames for the
 	  Marvell switches which use DSA headers.
 
 config NET_DSA_TAG_EDSA
 	tristate "Tag driver for Marvell switches using EtherType DSA headers"
+	select NET_DSA_TAG_DSA_COMMON
 	help
 	  Say Y or M if you want to enable support for tagging frames for the
 	  Marvell switches which use EtherType DSA headers.
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index e25d5457964a..0fb2b75a7ae3 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -7,8 +7,7 @@ dsa_core-y += dsa.o dsa2.o master.o port.o slave.o switch.o
 obj-$(CONFIG_NET_DSA_TAG_8021Q) += tag_8021q.o
 obj-$(CONFIG_NET_DSA_TAG_AR9331) += tag_ar9331.o
 obj-$(CONFIG_NET_DSA_TAG_BRCM_COMMON) += tag_brcm.o
-obj-$(CONFIG_NET_DSA_TAG_DSA) += tag_dsa.o
-obj-$(CONFIG_NET_DSA_TAG_EDSA) += tag_edsa.o
+obj-$(CONFIG_NET_DSA_TAG_DSA_COMMON) += tag_dsa.o
 obj-$(CONFIG_NET_DSA_TAG_GSWIP) += tag_gswip.o
 obj-$(CONFIG_NET_DSA_TAG_HELLCREEK) += tag_hellcreek.o
 obj-$(CONFIG_NET_DSA_TAG_KSZ) += tag_ksz.o
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index 63d690a0fca6..b44b75a4c809 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -1,7 +1,47 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
- * net/dsa/tag_dsa.c - (Non-ethertype) DSA tagging
+ * Regular and Ethertype DSA tagging
  * Copyright (c) 2008-2009 Marvell Semiconductor
+ *
+ * Regular DSA
+ * -----------
+ * For untagged (in 802.1Q terms) packes, the swich will splice in the
+ * tag between the SA and the ethertype of the original packet. Tagged
+ * frames will instead have their outermost .1Q tag converted to a DSA
+ * tag. It expects the same layout when receiving packets from the
+ * CPU.
+ *
+ * Example:
+ *
+ *     .----.----.----.---------
+ * Pu: | DA | SA | ET | Payload ...
+ *     '----'----'----'---------
+ *       6    6    2       N
+ *     .----.----.--------.-----.----.---------
+ * Pt: | DA | SA | 0x8100 | TCI | ET | Payload ...
+ *     '----'----'--------'-----'----'---------
+ *       6    6       2      2    2       N
+ *     .----.----.-----.----.---------
+ * Pd: | DA | SA | DSA | ET | Payload ...
+ *     '----'----'-----'----'---------
+ *       6    6     4    2       N
+ *
+ * No matter if a packet is received untagged (Pu) or tagged (Pt),
+ * they will both have the same layout (Pd) when they are sent to the
+ * CPU. This is done by ignoring 802.3, replacing the ethertype field
+ * with more metadata, among which is a bit to signal if the original
+ * packet was tagged or not.
+ *
+ * Ethertype DSA
+ * -------------
+ * Uses the exact same tag format as regular DSA, but also includes a
+ * proper ethertype field (which the mv88e6xxx driver sets to
+ * ETH_P_EDSA/0xdada) followed by two zero bytes:
+ *
+ * .----.----.--------.--------.-----.----.---------
+ * | DA | SA | 0xdada | 0x0000 | DSA | ET | Payload ...
+ * '----'----'--------'--------'-----'----'---------
+ *   6    6       2        2      4    2       N
  */
 
 #include <linux/etherdevice.h>
@@ -12,22 +52,77 @@
 
 #define DSA_HLEN	4
 
-static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
+/**
+ * enum dsa_cmd - DSA Command
+ * @DSA_CMD_TO_CPU: Set on packets that were trapped or mirrored to
+ *     the CPU port. This is needed to implement control protocols,
+ *     e.g. STP and LLDP, that must not allow those control packets to
+ *     be switched according to the normal rules.
+ * @DSA_CMD_FROM_CPU: Used by the CPU to send a packet to a specific
+ *     port, ignoring all the barriers that the switch normally
+ *     enforces (VLANs, STP port states etc.). "sudo send packet"
+ * @DSA_CMD_TO_SNIFFER: Set on packets that where mirrored to the CPU
+ *     as a result of matching some user configured ingress or egress
+ *     monitor criteria.
+ * @DSA_CMD_FORWARD: Everything else, i.e. the bulk data traffic.
+ */
+enum dsa_cmd {
+	DSA_CMD_TO_CPU     = 0,
+	DSA_CMD_FROM_CPU   = 1,
+	DSA_CMD_TO_SNIFFER = 2,
+	DSA_CMD_FORWARD    = 3
+};
+
+/**
+ * enum dsa_code - TO_CPU Code
+ *
+ * @DSA_CODE_MGMT_TRAP: DA was classified as a management
+ *     address. Typical examples include STP BPDUs and LLDP.
+ * @DSA_CODE_FRAME2REG: Response to a "remote management" request.
+ * @DSA_CODE_IGMP_MLD_TRAP: IGMP/MLD signaling.
+ * @DSA_CODE_POLICY_TRAP: Frame matched some policy configuration on
+ *     the device. Typical examples are matching on DA/SA/VID and DHCP
+ *     snooping.
+ * @DSA_CODE_ARP_MIRROR: The name says it all really.
+ * @DSA_CODE_POLICY_MIRROR: Same as @DSA_CODE_POLICY_TRAP, but the
+ *     particular policy was set to trigger a mirror instead of a
+ *     trap.
+ * @DSA_CODE_RESERVED_6: Unused on all devices up to at least 6393X.
+ * @DSA_CODE_RESERVED_7: Unused on all devices up to at least 6393X.
+ *
+ * A 3-bit code is used to relay why a particular frame was sent to
+ * the CPU. We only use this to determine if the packet was mirrored
+ * or trapped, i.e. whether the packet has been forwarded by hardware
+ * or not.
+ */
+enum dsa_code {
+	DSA_CODE_MGMT_TRAP     = 0,
+	DSA_CODE_FRAME2REG     = 1,
+	DSA_CODE_IGMP_MLD_TRAP = 2,
+	DSA_CODE_POLICY_TRAP   = 3,
+	DSA_CODE_ARP_MIRROR    = 4,
+	DSA_CODE_POLICY_MIRROR = 5,
+	DSA_CODE_RESERVED_6    = 6,
+	DSA_CODE_RESERVED_7    = 7
+};
+
+static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
+				   u8 extra)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
 	u8 *dsa_header;
 
-	/*
-	 * Convert the outermost 802.1q tag to a DSA tag for tagged
-	 * packets, or insert a DSA tag between the addresses and
-	 * the ethertype field for untagged packets.
-	 */
 	if (skb->protocol == htons(ETH_P_8021Q)) {
 		/*
 		 * Construct tagged FROM_CPU DSA tag from 802.1q tag.
 		 */
-		dsa_header = skb->data + 2 * ETH_ALEN;
-		dsa_header[0] = 0x60 | dp->ds->index;
+		if (extra) {
+			skb_push(skb, extra);
+			memmove(skb->data, skb->data + extra, 2 * ETH_ALEN);
+		}
+
+		dsa_header = skb->data + 2 * ETH_ALEN + extra;
+		dsa_header[0] = (DSA_CMD_FROM_CPU << 6) | 0x20 | dp->ds->index;
 		dsa_header[1] = dp->index << 3;
 
 		/*
@@ -38,15 +133,14 @@ static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
 			dsa_header[2] &= ~0x10;
 		}
 	} else {
-		skb_push(skb, DSA_HLEN);
-
-		memmove(skb->data, skb->data + DSA_HLEN, 2 * ETH_ALEN);
+		skb_push(skb, DSA_HLEN + extra);
+		memmove(skb->data, skb->data + DSA_HLEN + extra, 2 * ETH_ALEN);
 
 		/*
 		 * Construct untagged FROM_CPU DSA tag.
 		 */
-		dsa_header = skb->data + 2 * ETH_ALEN;
-		dsa_header[0] = 0x40 | dp->ds->index;
+		dsa_header = skb->data + 2 * ETH_ALEN + extra;
+		dsa_header[0] = (DSA_CMD_FROM_CPU << 6) | dp->ds->index;
 		dsa_header[1] = dp->index << 3;
 		dsa_header[2] = 0x00;
 		dsa_header[3] = 0x00;
@@ -55,30 +149,64 @@ static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
 	return skb;
 }
 
-static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev,
-			       struct packet_type *pt)
+static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
+				  u8 extra)
 {
+	int source_device, source_port;
+	enum dsa_code code;
+	enum dsa_cmd cmd;
 	u8 *dsa_header;
-	int source_device;
-	int source_port;
-
-	if (unlikely(!pskb_may_pull(skb, DSA_HLEN)))
-		return NULL;
 
 	/*
 	 * The ethertype field is part of the DSA header.
 	 */
 	dsa_header = skb->data - 2;
 
-	/*
-	 * Check that frame type is either TO_CPU or FORWARD.
-	 */
-	if ((dsa_header[0] & 0xc0) != 0x00 && (dsa_header[0] & 0xc0) != 0xc0)
+	cmd = dsa_header[0] >> 6;
+	switch (cmd) {
+	case DSA_CMD_FORWARD:
+		skb->offload_fwd_mark = 1;
+		break;
+
+	case DSA_CMD_TO_CPU:
+		code = (dsa_header[1] & 0x6) | ((dsa_header[2] >> 4) & 1);
+
+		switch (code) {
+		case DSA_CODE_FRAME2REG:
+			/* Remote management frames originate from the
+			 * switch itself, there is no DSA port for us
+			 * to ingress it on (the port field is always
+			 * 0 in these tags).
+			 */
+			return NULL;
+		case DSA_CODE_ARP_MIRROR:
+		case DSA_CODE_POLICY_MIRROR:
+			/* Mark mirrored packets to notify any upper
+			 * device (like a bridge) that forwarding has
+			 * already been done by hardware.
+			 */
+			skb->offload_fwd_mark = 1;
+			break;
+		case DSA_CODE_MGMT_TRAP:
+		case DSA_CODE_IGMP_MLD_TRAP:
+		case DSA_CODE_POLICY_TRAP:
+			/* Traps have, by definition, not been
+			 * forwarded by hardware, so don't mark them.
+			 */
+			break;
+		default:
+			/* Reserved code, this could be anything. Drop
+			 * seems like the safest option.
+			 */
+			return NULL;
+		}
+
+		break;
+
+	default:
 		return NULL;
+	}
 
-	/*
-	 * Determine source device and port.
-	 */
 	source_device = dsa_header[0] & 0x1f;
 	source_port = (dsa_header[1] >> 3) & 0x1f;
 
@@ -86,10 +214,10 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!skb->dev)
 		return NULL;
 
-	/*
-	 * Convert the DSA header to an 802.1q header if the 'tagged'
-	 * bit in the DSA header is set.  If the 'tagged' bit is clear,
-	 * delete the DSA header entirely.
+	/* If the 'tagged' bit is set; convert the DSA tag to a 802.1Q
+	 * tag, and delete the ethertype (extra) if applicable. If the
+	 * 'tagged' bit is cleared; delete the DSA tag, and ethertype
+	 * if applicable.
 	 */
 	if (dsa_header[0] & 0x20) {
 		u8 new_header[4];
@@ -122,30 +250,101 @@ static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev,
 		}
 
 		memcpy(dsa_header, new_header, DSA_HLEN);
+
+		if (extra)
+			memmove(skb->data - ETH_HLEN,
+				skb->data - ETH_HLEN - extra,
+				2 * ETH_ALEN);
 	} else {
-		/*
-		 * Remove DSA tag and update checksum.
-		 */
 		skb_pull_rcsum(skb, DSA_HLEN);
 		memmove(skb->data - ETH_HLEN,
-			skb->data - ETH_HLEN - DSA_HLEN,
+			skb->data - ETH_HLEN - DSA_HLEN - extra,
 			2 * ETH_ALEN);
 	}
 
-	skb->offload_fwd_mark = 1;
-
 	return skb;
 }
 
+#if IS_ENABLED(CONFIG_NET_DSA_TAG_DSA)
+
+static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	return dsa_xmit_ll(skb, dev, 0);
+}
+
+static struct sk_buff *dsa_rcv(struct sk_buff *skb, struct net_device *dev,
+			       struct packet_type *pt)
+{
+	if (unlikely(!pskb_may_pull(skb, DSA_HLEN)))
+		return NULL;
+
+	return dsa_rcv_ll(skb, dev, 0);
+}
+
 static const struct dsa_device_ops dsa_netdev_ops = {
-	.name	= "dsa",
-	.proto	= DSA_TAG_PROTO_DSA,
-	.xmit	= dsa_xmit,
-	.rcv	= dsa_rcv,
+	.name	  = "dsa",
+	.proto	  = DSA_TAG_PROTO_DSA,
+	.xmit	  = dsa_xmit,
+	.rcv	  = dsa_rcv,
 	.overhead = DSA_HLEN,
 };
 
-MODULE_LICENSE("GPL");
+DSA_TAG_DRIVER(dsa_netdev_ops);
 MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_DSA);
+#endif	/* CONFIG_NET_DSA_TAG_DSA */
+
+#if IS_ENABLED(CONFIG_NET_DSA_TAG_EDSA)
+
+#define EDSA_HLEN 8
+
+static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	u8 *edsa_header;
+
+	skb = dsa_xmit_ll(skb, dev, EDSA_HLEN - DSA_HLEN);
+	if (!skb)
+		return NULL;
 
-module_dsa_tag_driver(dsa_netdev_ops);
+	edsa_header = skb->data + 2 * ETH_ALEN;
+	edsa_header[0] = (ETH_P_EDSA >> 8) & 0xff;
+	edsa_header[1] = ETH_P_EDSA & 0xff;
+	edsa_header[2] = 0x00;
+	edsa_header[3] = 0x00;
+	return skb;
+}
+
+static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev,
+				struct packet_type *pt)
+{
+	if (unlikely(!pskb_may_pull(skb, EDSA_HLEN)))
+		return NULL;
+
+	skb_pull_rcsum(skb, EDSA_HLEN - DSA_HLEN);
+
+	return dsa_rcv_ll(skb, dev, EDSA_HLEN - DSA_HLEN);
+}
+
+static const struct dsa_device_ops edsa_netdev_ops = {
+	.name	  = "edsa",
+	.proto	  = DSA_TAG_PROTO_EDSA,
+	.xmit	  = edsa_xmit,
+	.rcv	  = edsa_rcv,
+	.overhead = EDSA_HLEN,
+};
+
+DSA_TAG_DRIVER(edsa_netdev_ops);
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_EDSA);
+#endif	/* CONFIG_NET_DSA_TAG_EDSA */
+
+static struct dsa_tag_driver *dsa_tag_drivers[] = {
+#if IS_ENABLED(CONFIG_NET_DSA_TAG_DSA)
+	&DSA_TAG_DRIVER_NAME(dsa_netdev_ops),
+#endif
+#if IS_ENABLED(CONFIG_NET_DSA_TAG_EDSA)
+	&DSA_TAG_DRIVER_NAME(edsa_netdev_ops),
+#endif
+};
+
+module_dsa_tag_drivers(dsa_tag_drivers);
+
+MODULE_LICENSE("GPL");
diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
deleted file mode 100644
index abf70a29deb4..000000000000
--- a/net/dsa/tag_edsa.c
+++ /dev/null
@@ -1,202 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * net/dsa/tag_edsa.c - Ethertype DSA tagging
- * Copyright (c) 2008-2009 Marvell Semiconductor
- */
-
-#include <linux/etherdevice.h>
-#include <linux/list.h>
-#include <linux/slab.h>
-
-#include "dsa_priv.h"
-
-#define DSA_HLEN	4
-#define EDSA_HLEN	8
-
-#define FRAME_TYPE_TO_CPU	0x00
-#define FRAME_TYPE_FORWARD	0x03
-
-#define TO_CPU_CODE_MGMT_TRAP		0x00
-#define TO_CPU_CODE_FRAME2REG		0x01
-#define TO_CPU_CODE_IGMP_MLD_TRAP	0x02
-#define TO_CPU_CODE_POLICY_TRAP		0x03
-#define TO_CPU_CODE_ARP_MIRROR		0x04
-#define TO_CPU_CODE_POLICY_MIRROR	0x05
-
-static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
-{
-	struct dsa_port *dp = dsa_slave_to_port(dev);
-	u8 *edsa_header;
-
-	/*
-	 * Convert the outermost 802.1q tag to a DSA tag and prepend
-	 * a DSA ethertype field is the packet is tagged, or insert
-	 * a DSA ethertype plus DSA tag between the addresses and the
-	 * current ethertype field if the packet is untagged.
-	 */
-	if (skb->protocol == htons(ETH_P_8021Q)) {
-		skb_push(skb, DSA_HLEN);
-
-		memmove(skb->data, skb->data + DSA_HLEN, 2 * ETH_ALEN);
-
-		/*
-		 * Construct tagged FROM_CPU DSA tag from 802.1q tag.
-		 */
-		edsa_header = skb->data + 2 * ETH_ALEN;
-		edsa_header[0] = (ETH_P_EDSA >> 8) & 0xff;
-		edsa_header[1] = ETH_P_EDSA & 0xff;
-		edsa_header[2] = 0x00;
-		edsa_header[3] = 0x00;
-		edsa_header[4] = 0x60 | dp->ds->index;
-		edsa_header[5] = dp->index << 3;
-
-		/*
-		 * Move CFI field from byte 6 to byte 5.
-		 */
-		if (edsa_header[6] & 0x10) {
-			edsa_header[5] |= 0x01;
-			edsa_header[6] &= ~0x10;
-		}
-	} else {
-		skb_push(skb, EDSA_HLEN);
-
-		memmove(skb->data, skb->data + EDSA_HLEN, 2 * ETH_ALEN);
-
-		/*
-		 * Construct untagged FROM_CPU DSA tag.
-		 */
-		edsa_header = skb->data + 2 * ETH_ALEN;
-		edsa_header[0] = (ETH_P_EDSA >> 8) & 0xff;
-		edsa_header[1] = ETH_P_EDSA & 0xff;
-		edsa_header[2] = 0x00;
-		edsa_header[3] = 0x00;
-		edsa_header[4] = 0x40 | dp->ds->index;
-		edsa_header[5] = dp->index << 3;
-		edsa_header[6] = 0x00;
-		edsa_header[7] = 0x00;
-	}
-
-	return skb;
-}
-
-static struct sk_buff *edsa_rcv(struct sk_buff *skb, struct net_device *dev,
-				struct packet_type *pt)
-{
-	u8 *edsa_header;
-	int frame_type;
-	int code;
-	int source_device;
-	int source_port;
-
-	if (unlikely(!pskb_may_pull(skb, EDSA_HLEN)))
-		return NULL;
-
-	/*
-	 * Skip the two null bytes after the ethertype.
-	 */
-	edsa_header = skb->data + 2;
-
-	/*
-	 * Check that frame type is either TO_CPU or FORWARD.
-	 */
-	frame_type = edsa_header[0] >> 6;
-
-	switch (frame_type) {
-	case FRAME_TYPE_TO_CPU:
-		code = (edsa_header[1] & 0x6) | ((edsa_header[2] >> 4) & 1);
-
-		/*
-		 * Mark the frame to never egress on any port of the same switch
-		 * unless it's a trapped IGMP/MLD packet, in which case the
-		 * bridge might want to forward it.
-		 */
-		if (code != TO_CPU_CODE_IGMP_MLD_TRAP)
-			skb->offload_fwd_mark = 1;
-
-		break;
-
-	case FRAME_TYPE_FORWARD:
-		skb->offload_fwd_mark = 1;
-		break;
-
-	default:
-		return NULL;
-	}
-
-	/*
-	 * Determine source device and port.
-	 */
-	source_device = edsa_header[0] & 0x1f;
-	source_port = (edsa_header[1] >> 3) & 0x1f;
-
-	skb->dev = dsa_master_find_slave(dev, source_device, source_port);
-	if (!skb->dev)
-		return NULL;
-
-	/*
-	 * If the 'tagged' bit is set, convert the DSA tag to a 802.1q
-	 * tag and delete the ethertype part.  If the 'tagged' bit is
-	 * clear, delete the ethertype and the DSA tag parts.
-	 */
-	if (edsa_header[0] & 0x20) {
-		u8 new_header[4];
-
-		/*
-		 * Insert 802.1q ethertype and copy the VLAN-related
-		 * fields, but clear the bit that will hold CFI (since
-		 * DSA uses that bit location for another purpose).
-		 */
-		new_header[0] = (ETH_P_8021Q >> 8) & 0xff;
-		new_header[1] = ETH_P_8021Q & 0xff;
-		new_header[2] = edsa_header[2] & ~0x10;
-		new_header[3] = edsa_header[3];
-
-		/*
-		 * Move CFI bit from its place in the DSA header to
-		 * its 802.1q-designated place.
-		 */
-		if (edsa_header[1] & 0x01)
-			new_header[2] |= 0x10;
-
-		skb_pull_rcsum(skb, DSA_HLEN);
-
-		/*
-		 * Update packet checksum if skb is CHECKSUM_COMPLETE.
-		 */
-		if (skb->ip_summed == CHECKSUM_COMPLETE) {
-			__wsum c = skb->csum;
-			c = csum_add(c, csum_partial(new_header + 2, 2, 0));
-			c = csum_sub(c, csum_partial(edsa_header + 2, 2, 0));
-			skb->csum = c;
-		}
-
-		memcpy(edsa_header, new_header, DSA_HLEN);
-
-		memmove(skb->data - ETH_HLEN,
-			skb->data - ETH_HLEN - DSA_HLEN,
-			2 * ETH_ALEN);
-	} else {
-		/*
-		 * Remove DSA tag and update checksum.
-		 */
-		skb_pull_rcsum(skb, EDSA_HLEN);
-		memmove(skb->data - ETH_HLEN,
-			skb->data - ETH_HLEN - EDSA_HLEN,
-			2 * ETH_ALEN);
-	}
-
-	return skb;
-}
-
-static const struct dsa_device_ops edsa_netdev_ops = {
-	.name	= "edsa",
-	.proto	= DSA_TAG_PROTO_EDSA,
-	.xmit	= edsa_xmit,
-	.rcv	= edsa_rcv,
-	.overhead = EDSA_HLEN,
-};
-
-MODULE_LICENSE("GPL");
-MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_EDSA);
-
-module_dsa_tag_driver(edsa_netdev_ops);
-- 
2.17.1

[PATCH v2 net-next 2/2] net: dsa: tag_dsa: Use a consistent comment style

[Tobias Waldekranz]

Use a consistent style of one-line/multi-line comments throughout the
file.

Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
---
 net/dsa/tag_dsa.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index b44b75a4c809..4d5d605f0f57 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -113,21 +113,17 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
 	u8 *dsa_header;
 
 	if (skb->protocol == htons(ETH_P_8021Q)) {
-		/*
-		 * Construct tagged FROM_CPU DSA tag from 802.1q tag.
-		 */
 		if (extra) {
 			skb_push(skb, extra);
 			memmove(skb->data, skb->data + extra, 2 * ETH_ALEN);
 		}
 
+		/* Construct tagged FROM_CPU DSA tag from 802.1Q tag. */
 		dsa_header = skb->data + 2 * ETH_ALEN + extra;
 		dsa_header[0] = (DSA_CMD_FROM_CPU << 6) | 0x20 | dp->ds->index;
 		dsa_header[1] = dp->index << 3;
 
-		/*
-		 * Move CFI field from byte 2 to byte 1.
-		 */
+		/* Move CFI field from byte 2 to byte 1. */
 		if (dsa_header[2] & 0x10) {
 			dsa_header[1] |= 0x01;
 			dsa_header[2] &= ~0x10;
@@ -136,9 +132,7 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
 		skb_push(skb, DSA_HLEN + extra);
 		memmove(skb->data, skb->data + DSA_HLEN + extra, 2 * ETH_ALEN);
 
-		/*
-		 * Construct untagged FROM_CPU DSA tag.
-		 */
+		/* Construct untagged FROM_CPU DSA tag. */
 		dsa_header = skb->data + 2 * ETH_ALEN + extra;
 		dsa_header[0] = (DSA_CMD_FROM_CPU << 6) | dp->ds->index;
 		dsa_header[1] = dp->index << 3;
@@ -157,9 +151,7 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 	enum dsa_cmd cmd;
 	u8 *dsa_header;
 
-	/*
-	 * The ethertype field is part of the DSA header.
-	 */
+	/* The ethertype field is part of the DSA header. */
 	dsa_header = skb->data - 2;
 
 	cmd = dsa_header[0] >> 6;
@@ -222,8 +214,7 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 	if (dsa_header[0] & 0x20) {
 		u8 new_header[4];
 
-		/*
-		 * Insert 802.1q ethertype and copy the VLAN-related
+		/* Insert 802.1Q ethertype and copy the VLAN-related
 		 * fields, but clear the bit that will hold CFI (since
 		 * DSA uses that bit location for another purpose).
 		 */
@@ -232,16 +223,13 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 		new_header[2] = dsa_header[2] & ~0x10;
 		new_header[3] = dsa_header[3];
 
-		/*
-		 * Move CFI bit from its place in the DSA header to
-		 * its 802.1q-designated place.
+		/* Move CFI bit from its place in the DSA header to
+		 * its 802.1Q-designated place.
 		 */
 		if (dsa_header[1] & 0x01)
 			new_header[2] |= 0x10;
 
-		/*
-		 * Update packet checksum if skb is CHECKSUM_COMPLETE.
-		 */
+		/* Update packet checksum if skb is CHECKSUM_COMPLETE. */
 		if (skb->ip_summed == CHECKSUM_COMPLETE) {
 			__wsum c = skb->csum;
 			c = csum_add(c, csum_partial(new_header + 2, 2, 0));
-- 
2.17.1

Re: [PATCH v2 net-next 1/2] net: dsa: tag_dsa: Unify regular and ethertype DSA taggers

[Jakub Kicinski]

On Wed, 11 Nov 2020 14:11:52 +0100 Tobias Waldekranz wrote:
> Ethertype DSA encodes exactly the same information in the DSA tag as
> the non-ethertype variety. So refactor out the common parts and reuse
> them for both protocols.
> 
> This is ensures tag parsing and generation is always consistent across
> all mv88e6xxx chips.
> 
> While we are at it, explicitly deal with all possible CPU codes on
> receive, making sure to set offload_fwd_mark as appropriate.

Uncharacteristically unreviewed for a DSA patch :)

> + *
> + * Regular DSA
> + * -----------
> + * For untagged (in 802.1Q terms) packes, the swich will splice in the

packes -> packets
swich  -> switch

> + * tag between the SA and the ethertype of the original packet. Tagged
> + * frames will instead have their outermost .1Q tag converted to a DSA
> + * tag. It expects the same layout when receiving packets from the
> + * CPU.

Re: [PATCH v2 net-next 1/2] net: dsa: tag_dsa: Unify regular and ethertype DSA taggers

[Vladimir Oltean]

On Fri, Nov 13, 2020 at 04:27:09PM -0800, Jakub Kicinski wrote:
> On Wed, 11 Nov 2020 14:11:52 +0100 Tobias Waldekranz wrote:
> > Ethertype DSA encodes exactly the same information in the DSA tag as
> > the non-ethertype variety. So refactor out the common parts and reuse
> > them for both protocols.
> >
> > This is ensures tag parsing and generation is always consistent across
> > all mv88e6xxx chips.
> >
> > While we are at it, explicitly deal with all possible CPU codes on
> > receive, making sure to set offload_fwd_mark as appropriate.
>
> Uncharacteristically unreviewed for a DSA patch :)

Yeah, I don't know what happened...

Re: [PATCH v2 net-next 1/2] net: dsa: tag_dsa: Unify regular and ethertype DSA taggers

[Vladimir Oltean]

On Wed, Nov 11, 2020 at 02:11:52PM +0100, Tobias Waldekranz wrote:
> Ethertype DSA encodes exactly the same information in the DSA tag as
> the non-ethertype variety. So refactor out the common parts and reuse
> them for both protocols.
> 
> This is ensures tag parsing and generation is always consistent across
> all mv88e6xxx chips.
> 
> While we are at it, explicitly deal with all possible CPU codes on
> receive, making sure to set offload_fwd_mark as appropriate.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH v2 net-next 2/2] net: dsa: tag_dsa: Use a consistent comment style

[Vladimir Oltean]

On Wed, Nov 11, 2020 at 02:11:53PM +0100, Tobias Waldekranz wrote:
> Use a consistent style of one-line/multi-line comments throughout the
> file.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH v2 net-next 1/2] net: dsa: tag_dsa: Unify regular and ethertype DSA taggers

[Andrew Lunn]

Hi Tobias

> +/**
> + * enum dsa_cmd - DSA Command
> + * @DSA_CMD_TO_CPU: Set on packets that were trapped or mirrored to
> + *     the CPU port. This is needed to implement control protocols,
> + *     e.g. STP and LLDP, that must not allow those control packets to
> + *     be switched according to the normal rules.

Maybe we want to mention that this only makes sense for packets
egressing the switch?

> + * @DSA_CMD_FROM_CPU: Used by the CPU to send a packet to a specific
> + *     port, ignoring all the barriers that the switch normally
> + *     enforces (VLANs, STP port states etc.). "sudo send packet"

This only make sense for packets ingressing the switch. The
TO_CPU/FROM_CPU kind of make that clear but..

> + * @DSA_CMD_TO_SNIFFER: Set on packets that where mirrored to the CPU
> + *     as a result of matching some user configured ingress or egress
> + *     monitor criteria.
> + * @DSA_CMD_FORWARD: Everything else, i.e. the bulk data traffic.

I assume this can be used in both direction?

> + *
> + * A 3-bit code is used to relay why a particular frame was sent to
> + * the CPU. We only use this to determine if the packet was mirrored
> + * or trapped, i.e. whether the packet has been forwarded by hardware
> + * or not.

Maybe add that, not all generations support all codes. 

> +static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
> +				  u8 extra)
>  {
> +	int source_device, source_port;
> +	enum dsa_code code;
> +	enum dsa_cmd cmd;
>  	u8 *dsa_header;
> -	int source_device;
> -	int source_port;
> -
> -	if (unlikely(!pskb_may_pull(skb, DSA_HLEN)))
> -		return NULL;
>  
>  	/*
>  	 * The ethertype field is part of the DSA header.
>  	 */
>  	dsa_header = skb->data - 2;
>  
> -	/*
> -	 * Check that frame type is either TO_CPU or FORWARD.
> -	 */
> -	if ((dsa_header[0] & 0xc0) != 0x00 && (dsa_header[0] & 0xc0) != 0xc0)
> +	cmd = dsa_header[0] >> 6;
> +	switch (cmd) {
> +	case DSA_CMD_FORWARD:
> +		skb->offload_fwd_mark = 1;
> +		break;
> +
> +	case DSA_CMD_TO_CPU:
> +		code = (dsa_header[1] & 0x6) | ((dsa_header[2] >> 4) & 1);
> +
> +		switch (code) {
> +		case DSA_CODE_FRAME2REG:
> +			/* Remote management frames originate from the
> +			 * switch itself, there is no DSA port for us
> +			 * to ingress it on (the port field is always
> +			 * 0 in these tags).

If/when we get around to implementing this, i doubt we will ingress it
like a frame. It will get picked up here and probably added to some
queue and a thread woken up. So maybe just say, not implemented yet,
so drop it.

> +			 */
> +			return NULL;
> +		case DSA_CODE_ARP_MIRROR:
> +		case DSA_CODE_POLICY_MIRROR:
> +			/* Mark mirrored packets to notify any upper
> +			 * device (like a bridge) that forwarding has
> +			 * already been done by hardware.
> +			 */
> +			skb->offload_fwd_mark = 1;
> +			break;
> +		case DSA_CODE_MGMT_TRAP:
> +		case DSA_CODE_IGMP_MLD_TRAP:
> +		case DSA_CODE_POLICY_TRAP:
> +			/* Traps have, by definition, not been
> +			 * forwarded by hardware, so don't mark them.
> +			 */

Humm, yes, they have not been forwarded by hardware. But is the
software bridge going to do the right thing and not flood them? Up
until now, i think we did mark them. So this is a clear change in
behaviour. I wonder if we want to break this out into a separate
patch? If something breaks, we can then bisect was it the combining
which broke it, or the change of this mark.

I will try to test this on my hardware, but it is probably same as
your 6390X and 6352.

     Andrew

Re: [PATCH v2 net-next 2/2] net: dsa: tag_dsa: Use a consistent comment style

[Andrew Lunn]

On Wed, Nov 11, 2020 at 02:11:53PM +0100, Tobias Waldekranz wrote:
> Use a consistent style of one-line/multi-line comments throughout the
> file.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 net-next 1/2] net: dsa: tag_dsa: Unify regular and ethertype DSA taggers

[Tobias Waldekranz]

On Sat Nov 14, 2020 at 4:08 AM CET, Andrew Lunn wrote:
> Hi Tobias
>
> > +/**
> > + * enum dsa_cmd - DSA Command
> > + * @DSA_CMD_TO_CPU: Set on packets that were trapped or mirrored to
> > + *     the CPU port. This is needed to implement control protocols,
> > + *     e.g. STP and LLDP, that must not allow those control packets to
> > + *     be switched according to the normal rules.
>
> Maybe we want to mention that this only makes sense for packets
> egressing the switch?
>
> > + * @DSA_CMD_FROM_CPU: Used by the CPU to send a packet to a specific
> > + *     port, ignoring all the barriers that the switch normally
> > + *     enforces (VLANs, STP port states etc.). "sudo send packet"
>
> This only make sense for packets ingressing the switch. The
> TO_CPU/FROM_CPU kind of make that clear but..

Honestly yes, I think it is pretty clear. But I am happy to change it
if you have any particular formulation you would like in there.

> > + * @DSA_CMD_TO_SNIFFER: Set on packets that where mirrored to the CPU
> > + *     as a result of matching some user configured ingress or egress
> > + *     monitor criteria.
> > + * @DSA_CMD_FORWARD: Everything else, i.e. the bulk data traffic.
>
> I assume this can be used in both direction?

Yes. I can add a sentence about that.

> > + *
> > + * A 3-bit code is used to relay why a particular frame was sent to
> > + * the CPU. We only use this to determine if the packet was mirrored
> > + * or trapped, i.e. whether the packet has been forwarded by hardware
> > + * or not.
>
> Maybe add that, not all generations support all codes.

Not sure I have that information. The oldest chipset I've worked with
is Jade (6095/6185) and in that datasheet the TO_CPU tag is not even
documented. From Opal+(6097) all the way through Agate, Peridot, and
Amethyst, the definitions have not changed from what I can see?

> > +			/* Remote management frames originate from the
> > +			 * switch itself, there is no DSA port for us
> > +			 * to ingress it on (the port field is always
> > +			 * 0 in these tags).
>
> If/when we get around to implementing this, i doubt we will ingress it
> like a frame. It will get picked up here and probably added to some
> queue and a thread woken up. So maybe just say, not implemented yet,
> so drop it.

v1 actually had a sentence about this :) I can put it back.

> > +			 */
> > +			return NULL;
> > +		case DSA_CODE_ARP_MIRROR:
> > +		case DSA_CODE_POLICY_MIRROR:
> > +			/* Mark mirrored packets to notify any upper
> > +			 * device (like a bridge) that forwarding has
> > +			 * already been done by hardware.
> > +			 */
> > +			skb->offload_fwd_mark = 1;
> > +			break;
> > +		case DSA_CODE_MGMT_TRAP:
> > +		case DSA_CODE_IGMP_MLD_TRAP:
> > +		case DSA_CODE_POLICY_TRAP:
> > +			/* Traps have, by definition, not been
> > +			 * forwarded by hardware, so don't mark them.
> > +			 */
>
> Humm, yes, they have not been forwarded by hardware. But is the
> software bridge going to do the right thing and not flood them? Up

The bridge is free to flood them if it wants to (e.g. IGMP/MLD
snooping is off) or not (e.g. IGMP/MLD snooping enabled). The point
is, that is not for a lowly switchdev driver to decide. Our job is to
relay to the bridge if this skb has been forwarded or not, the end.

> until now, i think we did mark them. So this is a clear change in
> behaviour. I wonder if we want to break this out into a separate
> patch? If something breaks, we can then bisect was it the combining
> which broke it, or the change of this mark.

Since mv88e6xxx can not configure anything that generates
DSA_CODE_MGMT_TRAP or DSA_CODE_POLICY_TRAP yet, we do not have to
worry about any change in behavior there.

That leaves us with DSA_CODE_IGMP_MLD_TRAP. Here is the problem:

Currenly, tag_dsa.c will set skb->offload_fwd_mark for IGMP/MLD
packets, whereas tag_edsa.c will exempt them. So we can not unify the
two without changing the behavior of one.

I posit that tag_edsa does the right thing, the packet has not been
forwarded, so we should go with that.

This is precisely the reason why we want to unify these! :)

> I will try to test this on my hardware, but it is probably same as
> your 6390X and 6352.

Thank you!

Re: [PATCH v2 net-next 1/2] net: dsa: tag_dsa: Unify regular and ethertype DSA taggers

[Vladimir Oltean]

On Sat, Nov 14, 2020 at 12:29:32PM +0100, Tobias Waldekranz wrote:
> > Humm, yes, they have not been forwarded by hardware. But is the
> > software bridge going to do the right thing and not flood them? Up
>
> The bridge is free to flood them if it wants to (e.g. IGMP/MLD
> snooping is off) or not (e.g. IGMP/MLD snooping enabled). The point
> is, that is not for a lowly switchdev driver to decide. Our job is to
> relay to the bridge if this skb has been forwarded or not, the end.
>
> > until now, i think we did mark them. So this is a clear change in
> > behaviour. I wonder if we want to break this out into a separate
> > patch? If something breaks, we can then bisect was it the combining
> > which broke it, or the change of this mark.
>
> Since mv88e6xxx can not configure anything that generates
> DSA_CODE_MGMT_TRAP or DSA_CODE_POLICY_TRAP yet, we do not have to
> worry about any change in behavior there.
>
> That leaves us with DSA_CODE_IGMP_MLD_TRAP. Here is the problem:
>
> Currenly, tag_dsa.c will set skb->offload_fwd_mark for IGMP/MLD
> packets, whereas tag_edsa.c will exempt them. So we can not unify the
> two without changing the behavior of one.
>
> I posit that tag_edsa does the right thing, the packet has not been
> forwarded, so we should go with that.
>
> This is precisely the reason why we want to unify these! :)

Shouldn't the correct approach be to monitor the
SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute in order to figure out
whether IGMP/MLD snooping is enabled or not?

Re: [PATCH v2 net-next 1/2] net: dsa: tag_dsa: Unify regular and ethertype DSA taggers

[Tobias Waldekranz]

On Sat Nov 14, 2020 at 3:20 PM CET, Vladimir Oltean wrote:
> On Sat, Nov 14, 2020 at 12:29:32PM +0100, Tobias Waldekranz wrote:
> > > Humm, yes, they have not been forwarded by hardware. But is the
> > > software bridge going to do the right thing and not flood them? Up
> >
> > The bridge is free to flood them if it wants to (e.g. IGMP/MLD
> > snooping is off) or not (e.g. IGMP/MLD snooping enabled). The point
> > is, that is not for a lowly switchdev driver to decide. Our job is to
> > relay to the bridge if this skb has been forwarded or not, the end.
> >
> > > until now, i think we did mark them. So this is a clear change in
> > > behaviour. I wonder if we want to break this out into a separate
> > > patch? If something breaks, we can then bisect was it the combining
> > > which broke it, or the change of this mark.
> >
> > Since mv88e6xxx can not configure anything that generates
> > DSA_CODE_MGMT_TRAP or DSA_CODE_POLICY_TRAP yet, we do not have to
> > worry about any change in behavior there.
> >
> > That leaves us with DSA_CODE_IGMP_MLD_TRAP. Here is the problem:
> >
> > Currenly, tag_dsa.c will set skb->offload_fwd_mark for IGMP/MLD
> > packets, whereas tag_edsa.c will exempt them. So we can not unify the
> > two without changing the behavior of one.
> >
> > I posit that tag_edsa does the right thing, the packet has not been
> > forwarded, so we should go with that.
> >
> > This is precisely the reason why we want to unify these! :)
>
> Shouldn't the correct approach be to monitor the
> SWITCHDEV_ATTR_ID_BRIDGE_MC_DISABLED attribute in order to figure out
> whether IGMP/MLD snooping is enabled or not?

The correct thing is to do both.

Independent of wheter IGMP/MLD snooping is statically or dynamically
enabled, it is always true that a TO_CPU with code 2 has _never_ been
forwarded in hardware and a FORWARD _always_ has been (which is the
tag you would see on IGMP frames reaching the CPU in that case).

In the static case, we would always get TO_CPUs with code 2, and thus
never set offload_fwd_mark. If snooping is enabled on the bridge, they
won't be forwarded. If snooping is disabled, the bridge will want to
forward, which it can since the mark is not set.

In the dynamic case, we would get TO_CPUs with code 2 when snooping is
enabled. The mark won't be set, but the bridge does not want to
forward anyway. When snooping is disabled, we would get FORWARDs. The
bridge will want to forward those, see that the mark is set, and skip
it.

The only combination that does not work is of course to have snooping
permanently _off_ in hardware and expect to use snooping correctly on
the software bridge. In that case you would always get FORWARDs, and
always set the mark. The bridge will not want to flood, but it is too
late since the hardware has already done it.

Re: [PATCH v2 net-next 1/2] net: dsa: tag_dsa: Unify regular and ethertype DSA taggers

[Andrew Lunn]

> > > + *
> > > + * A 3-bit code is used to relay why a particular frame was sent to
> > > + * the CPU. We only use this to determine if the packet was mirrored
> > > + * or trapped, i.e. whether the packet has been forwarded by hardware
> > > + * or not.
> >
> > Maybe add that, not all generations support all codes.
> 
> Not sure I have that information.

I'm not asking you list per code which switches support it. I'm just
think we should add a warning, it cannot be assumed all switches
support all codes. I just looked at the 6161 for example, and it is
missing 5 from its list.

> > > +			 */
> > > +			return NULL;
> > > +		case DSA_CODE_ARP_MIRROR:
> > > +		case DSA_CODE_POLICY_MIRROR:
> > > +			/* Mark mirrored packets to notify any upper
> > > +			 * device (like a bridge) that forwarding has
> > > +			 * already been done by hardware.
> > > +			 */
> > > +			skb->offload_fwd_mark = 1;
> > > +			break;
> > > +		case DSA_CODE_MGMT_TRAP:
> > > +		case DSA_CODE_IGMP_MLD_TRAP:
> > > +		case DSA_CODE_POLICY_TRAP:
> > > +			/* Traps have, by definition, not been
> > > +			 * forwarded by hardware, so don't mark them.
> > > +			 */
> >
> > Humm, yes, they have not been forwarded by hardware. But is the
> > software bridge going to do the right thing and not flood them? Up
> 
> The bridge is free to flood them if it wants to (e.g. IGMP/MLD
> snooping is off) or not (e.g. IGMP/MLD snooping enabled). The point
> is, that is not for a lowly switchdev driver to decide. Our job is to
> relay to the bridge if this skb has been forwarded or not, the end.
> 
> > until now, i think we did mark them. So this is a clear change in
> > behaviour. I wonder if we want to break this out into a separate
> > patch? If something breaks, we can then bisect was it the combining
> > which broke it, or the change of this mark.
> 
> Since mv88e6xxx can not configure anything that generates
> DSA_CODE_MGMT_TRAP or DSA_CODE_POLICY_TRAP yet, we do not have to
> worry about any change in behavior there.
> 
> That leaves us with DSA_CODE_IGMP_MLD_TRAP. Here is the problem:
> 
> Currenly, tag_dsa.c will set skb->offload_fwd_mark for IGMP/MLD
> packets, whereas tag_edsa.c will exempt them. So we can not unify the
> two without changing the behavior of one.

I'm not saying that this change is wrong. I'm just afraid as a
behaviour change, it might break something. If something does break,
it will be easier to track down, if it is a change on its own. So
please look if we can add a simple patch to tag_dsa.c which removes
the marking of such frames. And then the next patch can combine the
two into one driver. If it does break, git bisect will then tell us
which patch broke it.

     Andrew

Re: [PATCH v2 net-next 1/2] net: dsa: tag_dsa: Unify regular and ethertype DSA taggers

[Tobias Waldekranz]

On Sat Nov 14, 2020 at 5:44 PM CET, Andrew Lunn wrote:
> > > > + *
> > > > + * A 3-bit code is used to relay why a particular frame was sent to
> > > > + * the CPU. We only use this to determine if the packet was mirrored
> > > > + * or trapped, i.e. whether the packet has been forwarded by hardware
> > > > + * or not.
> > >
> > > Maybe add that, not all generations support all codes.
> > 
> > Not sure I have that information.
>
> I'm not asking you list per code which switches support it. I'm just
> think we should add a warning, it cannot be assumed all switches
> support all codes. I just looked at the 6161 for example, and it is
> missing 5 from its list.

I see, yeah sure I can do that.

> > That leaves us with DSA_CODE_IGMP_MLD_TRAP. Here is the problem:
> > 
> > Currenly, tag_dsa.c will set skb->offload_fwd_mark for IGMP/MLD
> > packets, whereas tag_edsa.c will exempt them. So we can not unify the
> > two without changing the behavior of one.
>
> I'm not saying that this change is wrong. I'm just afraid as a
> behaviour change, it might break something. If something does break,
> it will be easier to track down, if it is a change on its own. So
> please look if we can add a simple patch to tag_dsa.c which removes
> the marking of such frames. And then the next patch can combine the
> two into one driver. If it does break, git bisect will then tell us
> which patch broke it.

Ahh, I think I see what you are saying now. So I would copy the
IGMP/MLD exemption from tag_edsa.c to tag_dsa.c first, and then apply
the unify patch. Yeah that makes alot of sense, will do!