Add support for a new DSA tagging protocol driver for the MaxLinear
GSW1xx switch family. The GSW1xx switches use a proprietary 8-byte
special tag inserted between the source MAC address and the EtherType
field to indicate the source and destination ports for frames
traversing the CPU port.

Implement the tag handling logic to insert the special tag on transmit
and parse it on receive.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
since RFC:
 * use dsa etype header macros instead of open coding them
 * maintain alphabetic order in Kconfig and Makefile

 MAINTAINERS              |   3 +-
 include/net/dsa.h        |   2 +
 net/dsa/Kconfig          |   8 +++
 net/dsa/Makefile         |   1 +
 net/dsa/tag_mxl-gsw1xx.c | 141 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 154 insertions(+), 1 deletion(-)
 create mode 100644 net/dsa/tag_mxl-gsw1xx.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d652f4f27756..4ddff0b0a547 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14038,7 +14038,7 @@ F:	tools/testing/selftests/landlock/
 K:	landlock
 K:	LANDLOCK
 
-LANTIQ / INTEL Ethernet drivers
+LANTIQ / MAXLINEAR / INTEL Ethernet DSA drivers
 M:	Hauke Mehrtens <hauke@hauke-m.de>
 L:	netdev@vger.kernel.org
 S:	Maintained
@@ -14046,6 +14046,7 @@ F:	Documentation/devicetree/bindings/net/dsa/lantiq,gswip.yaml
 F:	drivers/net/dsa/lantiq/*
 F:	drivers/net/ethernet/lantiq_xrx200.c
 F:	net/dsa/tag_gswip.c
+F:	net/dsa/tag_mxl-gsw1xx.c
 
 LANTIQ MIPS ARCHITECTURE
 M:	John Crispin <john@phrozen.org>
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 67762fdaf3c7..2df2e2ead9a8 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -56,6 +56,7 @@ struct tc_action;
 #define DSA_TAG_PROTO_VSC73XX_8021Q_VALUE	28
 #define DSA_TAG_PROTO_BRCM_LEGACY_FCS_VALUE	29
 #define DSA_TAG_PROTO_YT921X_VALUE		30
+#define DSA_TAG_PROTO_MXL_GSW1XX_VALUE		31
 
 enum dsa_tag_protocol {
 	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
@@ -89,6 +90,7 @@ enum dsa_tag_protocol {
 	DSA_TAG_PROTO_LAN937X		= DSA_TAG_PROTO_LAN937X_VALUE,
 	DSA_TAG_PROTO_VSC73XX_8021Q	= DSA_TAG_PROTO_VSC73XX_8021Q_VALUE,
 	DSA_TAG_PROTO_YT921X		= DSA_TAG_PROTO_YT921X_VALUE,
+	DSA_TAG_PROTO_MXL_GSW1XX	= DSA_TAG_PROTO_MXL_GSW1XX_VALUE,
 };
 
 struct dsa_switch;
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index 6b94028b1fcc..f86b30742122 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -104,6 +104,14 @@ config NET_DSA_TAG_MTK
 	  Say Y or M if you want to enable support for tagging frames for
 	  Mediatek switches.
 
+config NET_DSA_TAG_MXL_GSW1XX
+	tristate "Tag driver for MaxLinear GSW1xx switches"
+	help
+	  The GSW1xx family of switches supports an 8-byte special tag which
+	  can be used on the CPU port of the switch.
+	  Say Y or M if you want to enable support for tagging frames for
+	  MaxLinear GSW1xx switches.
+
 config NET_DSA_TAG_KSZ
 	tristate "Tag driver for Microchip 8795/937x/9477/9893 families of switches"
 	help
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index 4b011a1d5c87..42d173f5a701 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_NET_DSA_TAG_HELLCREEK) += tag_hellcreek.o
 obj-$(CONFIG_NET_DSA_TAG_KSZ) += tag_ksz.o
 obj-$(CONFIG_NET_DSA_TAG_LAN9303) += tag_lan9303.o
 obj-$(CONFIG_NET_DSA_TAG_MTK) += tag_mtk.o
+obj-$(CONFIG_NET_DSA_TAG_MXL_GSW1XX) += tag_mxl-gsw1xx.o
 obj-$(CONFIG_NET_DSA_TAG_NONE) += tag_none.o
 obj-$(CONFIG_NET_DSA_TAG_OCELOT) += tag_ocelot.o
 obj-$(CONFIG_NET_DSA_TAG_OCELOT_8021Q) += tag_ocelot_8021q.o
diff --git a/net/dsa/tag_mxl-gsw1xx.c b/net/dsa/tag_mxl-gsw1xx.c
new file mode 100644
index 000000000000..9efec6deb494
--- /dev/null
+++ b/net/dsa/tag_mxl-gsw1xx.c
@@ -0,0 +1,141 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * DSA driver Special Tag support for MaxLinear GSW1xx switch chips
+ *
+ * Copyright (C) 2025 Daniel Golle <daniel@makrotopia.org>
+ * Copyright (C) 2023 - 2024 MaxLinear Inc.
+ */
+
+#include <linux/bitops.h>
+#include <linux/etherdevice.h>
+#include <linux/skbuff.h>
+#include <net/dsa.h>
+
+#include "tag.h"
+
+/* To define the outgoing port and to discover the incoming port a special
+ * tag is used by the GSW1xx.
+ *
+ *       Dest MAC       Src MAC    special TAG        EtherType
+ * ...| 1 2 3 4 5 6 | 1 2 3 4 5 6 | 1 2 3 4 5 6 7 8 | 1 2 |...
+ *                                |<--------------->|
+ */
+
+#define GSW1XX_TAG_NAME		"gsw1xx"
+
+/* special tag in TX path header */
+#define GSW1XX_TX_HEADER_LEN	8
+
+/* Byte 0 = Ethertype byte 1 -> 0x88 */
+/* Byte 1 = Ethertype byte 2 -> 0xC3*/
+
+/* Byte 2 */
+#define GSW1XX_TX_PORT_MAP_EN		BIT(7)
+#define GSW1XX_TX_CLASS_EN		BIT(6)
+#define GSW1XX_TX_TIME_STAMP_EN		BIT(5)
+#define GSW1XX_TX_LRN_DIS		BIT(4)
+#define GSW1XX_TX_CLASS_SHIFT		0
+#define GSW1XX_TX_CLASS_MASK		GENMASK(3, 0)
+
+/* Byte 3 */
+#define GSW1XX_TX_PORT_MAP_LOW_SHIFT	0
+#define GSW1XX_TX_PORT_MAP_LOW_MASK	GENMASK(7, 0)
+
+/* Byte 4 */
+#define GSW1XX_TX_PORT_MAP_HIGH_SHIFT	0
+#define GSW1XX_TX_PORT_MAP_HIGH_MASK	GENMASK(7, 0)
+
+#define GSW1XX_RX_HEADER_LEN		8
+
+/* special tag in RX path header */
+/* Byte 4 */
+#define GSW1XX_RX_PORT_MAP_LOW_SHIFT	0
+#define GSW1XX_RX_PORT_MAP_LOW_MASK	GENMASK(7, 0)
+
+/* Byte 5 */
+#define GSW1XX_RX_PORT_MAP_HIGH_SHIFT	0
+#define GSW1XX_RX_PORT_MAP_HIGH_MASK	GENMASK(7, 0)
+
+static struct sk_buff *gsw1xx_tag_xmit(struct sk_buff *skb,
+				       struct net_device *dev)
+{
+	struct dsa_port *dp = dsa_user_to_port(dev);
+	u8 *gsw1xx_tag;
+
+	/* provide additional space 'GSW1XX_TX_HEADER_LEN' bytes */
+	skb_push(skb, GSW1XX_TX_HEADER_LEN);
+
+	/* add space between MAC address and Ethertype */
+	dsa_alloc_etype_header(skb, GSW1XX_TX_HEADER_LEN);
+
+	/* special tag ingress */
+	gsw1xx_tag = dsa_etype_header_pos_tx(skb);
+	gsw1xx_tag[0] = 0x88;
+	gsw1xx_tag[1] = 0xc3;
+	gsw1xx_tag[2] = GSW1XX_TX_PORT_MAP_EN | GSW1XX_TX_LRN_DIS;
+	gsw1xx_tag[3] = BIT(dp->index + GSW1XX_TX_PORT_MAP_LOW_SHIFT) &
+			GSW1XX_TX_PORT_MAP_LOW_MASK;
+	gsw1xx_tag[4] = 0;
+	gsw1xx_tag[5] = 0;
+	gsw1xx_tag[6] = 0;
+	gsw1xx_tag[7] = 0;
+
+	return skb;
+}
+
+static struct sk_buff *gsw1xx_tag_rcv(struct sk_buff *skb,
+				      struct net_device *dev)
+{
+	int port;
+	u8 *gsw1xx_tag;
+
+	if (unlikely(!pskb_may_pull(skb, GSW1XX_RX_HEADER_LEN))) {
+		dev_warn_ratelimited(&dev->dev, "Dropping packet, cannot pull SKB\n");
+		return NULL;
+	}
+
+	gsw1xx_tag = dsa_etype_header_pos_rx(skb);
+
+	if (gsw1xx_tag[0] != 0x88 && gsw1xx_tag[1] != 0xc3) {
+		dev_warn_ratelimited(&dev->dev, "Dropping packet due to invalid special tag\n");
+		dev_warn_ratelimited(&dev->dev,
+				     "Tag: 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x\n",
+				     gsw1xx_tag[0], gsw1xx_tag[1], gsw1xx_tag[2], gsw1xx_tag[3],
+				     gsw1xx_tag[4], gsw1xx_tag[5], gsw1xx_tag[6], gsw1xx_tag[7]);
+		return NULL;
+	}
+
+	/* Get source port information */
+	port = (gsw1xx_tag[2] & GSW1XX_RX_PORT_MAP_LOW_MASK) >> GSW1XX_RX_PORT_MAP_LOW_SHIFT;
+	skb->dev = dsa_conduit_find_user(dev, 0, port);
+	if (!skb->dev) {
+		dev_warn_ratelimited(&dev->dev, "Dropping packet due to invalid source port\n");
+		dev_warn_ratelimited(&dev->dev,
+				     "Tag: 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x\n",
+				     gsw1xx_tag[0], gsw1xx_tag[1], gsw1xx_tag[2], gsw1xx_tag[3],
+				     gsw1xx_tag[4], gsw1xx_tag[5], gsw1xx_tag[6], gsw1xx_tag[7]);
+		return NULL;
+	}
+
+	/* remove the GSW1xx special tag between MAC addresses and the current
+	 * ethertype field.
+	 */
+	skb_pull_rcsum(skb, GSW1XX_RX_HEADER_LEN);
+	dsa_strip_etype_header(skb, GSW1XX_RX_HEADER_LEN);
+
+	return skb;
+}
+
+static const struct dsa_device_ops gsw1xx_netdev_ops = {
+	.name = GSW1XX_TAG_NAME,
+	.proto	= DSA_TAG_PROTO_MXL_GSW1XX,
+	.xmit = gsw1xx_tag_xmit,
+	.rcv = gsw1xx_tag_rcv,
+	.needed_headroom = GSW1XX_RX_HEADER_LEN,
+};
+
+MODULE_DESCRIPTION("DSA tag driver for MaxLinear GSW1xx 8 byte protocol");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_MXL_GSW1XX, GSW1XX_TAG_NAME);
+
+module_dsa_tag_driver(gsw1xx_netdev_ops);
-- 
2.51.1

On Sun, Oct 26, 2025 at 11:48:23PM +0000, Daniel Golle wrote:
> Add support for a new DSA tagging protocol driver for the MaxLinear
> GSW1xx switch family. The GSW1xx switches use a proprietary 8-byte
> special tag inserted between the source MAC address and the EtherType
> field to indicate the source and destination ports for frames
> traversing the CPU port.
> 
> Implement the tag handling logic to insert the special tag on transmit
> and parse it on receive.
> 
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
> since RFC:
>  * use dsa etype header macros instead of open coding them
>  * maintain alphabetic order in Kconfig and Makefile
> 
>  MAINTAINERS              |   3 +-
>  include/net/dsa.h        |   2 +
>  net/dsa/Kconfig          |   8 +++
>  net/dsa/Makefile         |   1 +
>  net/dsa/tag_mxl-gsw1xx.c | 141 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 154 insertions(+), 1 deletion(-)
>  create mode 100644 net/dsa/tag_mxl-gsw1xx.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d652f4f27756..4ddff0b0a547 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14038,7 +14038,7 @@ F:	tools/testing/selftests/landlock/
>  K:	landlock
>  K:	LANDLOCK
>  
> -LANTIQ / INTEL Ethernet drivers
> +LANTIQ / MAXLINEAR / INTEL Ethernet DSA drivers
>  M:	Hauke Mehrtens <hauke@hauke-m.de>
>  L:	netdev@vger.kernel.org
>  S:	Maintained
> @@ -14046,6 +14046,7 @@ F:	Documentation/devicetree/bindings/net/dsa/lantiq,gswip.yaml
>  F:	drivers/net/dsa/lantiq/*
>  F:	drivers/net/ethernet/lantiq_xrx200.c
>  F:	net/dsa/tag_gswip.c
> +F:	net/dsa/tag_mxl-gsw1xx.c
>  
>  LANTIQ MIPS ARCHITECTURE
>  M:	John Crispin <john@phrozen.org>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 67762fdaf3c7..2df2e2ead9a8 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -56,6 +56,7 @@ struct tc_action;
>  #define DSA_TAG_PROTO_VSC73XX_8021Q_VALUE	28
>  #define DSA_TAG_PROTO_BRCM_LEGACY_FCS_VALUE	29
>  #define DSA_TAG_PROTO_YT921X_VALUE		30
> +#define DSA_TAG_PROTO_MXL_GSW1XX_VALUE		31
>  
>  enum dsa_tag_protocol {
>  	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
> @@ -89,6 +90,7 @@ enum dsa_tag_protocol {
>  	DSA_TAG_PROTO_LAN937X		= DSA_TAG_PROTO_LAN937X_VALUE,
>  	DSA_TAG_PROTO_VSC73XX_8021Q	= DSA_TAG_PROTO_VSC73XX_8021Q_VALUE,
>  	DSA_TAG_PROTO_YT921X		= DSA_TAG_PROTO_YT921X_VALUE,
> +	DSA_TAG_PROTO_MXL_GSW1XX	= DSA_TAG_PROTO_MXL_GSW1XX_VALUE,
>  };
>  
>  struct dsa_switch;
> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index 6b94028b1fcc..f86b30742122 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -104,6 +104,14 @@ config NET_DSA_TAG_MTK
>  	  Say Y or M if you want to enable support for tagging frames for
>  	  Mediatek switches.
>  
> +config NET_DSA_TAG_MXL_GSW1XX
> +	tristate "Tag driver for MaxLinear GSW1xx switches"
> +	help
> +	  The GSW1xx family of switches supports an 8-byte special tag which
> +	  can be used on the CPU port of the switch.
> +	  Say Y or M if you want to enable support for tagging frames for
> +	  MaxLinear GSW1xx switches.
> +
>  config NET_DSA_TAG_KSZ
>  	tristate "Tag driver for Microchip 8795/937x/9477/9893 families of switches"
>  	help
> diff --git a/net/dsa/Makefile b/net/dsa/Makefile
> index 4b011a1d5c87..42d173f5a701 100644
> --- a/net/dsa/Makefile
> +++ b/net/dsa/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_NET_DSA_TAG_HELLCREEK) += tag_hellcreek.o
>  obj-$(CONFIG_NET_DSA_TAG_KSZ) += tag_ksz.o
>  obj-$(CONFIG_NET_DSA_TAG_LAN9303) += tag_lan9303.o
>  obj-$(CONFIG_NET_DSA_TAG_MTK) += tag_mtk.o
> +obj-$(CONFIG_NET_DSA_TAG_MXL_GSW1XX) += tag_mxl-gsw1xx.o
>  obj-$(CONFIG_NET_DSA_TAG_NONE) += tag_none.o
>  obj-$(CONFIG_NET_DSA_TAG_OCELOT) += tag_ocelot.o
>  obj-$(CONFIG_NET_DSA_TAG_OCELOT_8021Q) += tag_ocelot_8021q.o
> diff --git a/net/dsa/tag_mxl-gsw1xx.c b/net/dsa/tag_mxl-gsw1xx.c
> new file mode 100644
> index 000000000000..9efec6deb494
> --- /dev/null
> +++ b/net/dsa/tag_mxl-gsw1xx.c
> @@ -0,0 +1,141 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * DSA driver Special Tag support for MaxLinear GSW1xx switch chips
> + *
> + * Copyright (C) 2025 Daniel Golle <daniel@makrotopia.org>
> + * Copyright (C) 2023 - 2024 MaxLinear Inc.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/etherdevice.h>
> +#include <linux/skbuff.h>
> +#include <net/dsa.h>
> +
> +#include "tag.h"
> +
> +/* To define the outgoing port and to discover the incoming port a special
> + * tag is used by the GSW1xx.
> + *
> + *       Dest MAC       Src MAC    special TAG        EtherType
> + * ...| 1 2 3 4 5 6 | 1 2 3 4 5 6 | 1 2 3 4 5 6 7 8 | 1 2 |...
> + *                                |<--------------->|
> + */
> +
> +#define GSW1XX_TAG_NAME		"gsw1xx"
> +
> +/* special tag in TX path header */
> +#define GSW1XX_TX_HEADER_LEN	8
> +
> +/* Byte 0 = Ethertype byte 1 -> 0x88 */
> +/* Byte 1 = Ethertype byte 2 -> 0xC3*/
> +
> +/* Byte 2 */
> +#define GSW1XX_TX_PORT_MAP_EN		BIT(7)
> +#define GSW1XX_TX_CLASS_EN		BIT(6)
> +#define GSW1XX_TX_TIME_STAMP_EN		BIT(5)
> +#define GSW1XX_TX_LRN_DIS		BIT(4)
> +#define GSW1XX_TX_CLASS_SHIFT		0
> +#define GSW1XX_TX_CLASS_MASK		GENMASK(3, 0)

Using FIELD_PREP() would eliminate these _SHIFT definitions and _MASK
would also go away from the macro names.

> +
> +/* Byte 3 */
> +#define GSW1XX_TX_PORT_MAP_LOW_SHIFT	0
> +#define GSW1XX_TX_PORT_MAP_LOW_MASK	GENMASK(7, 0)
> +
> +/* Byte 4 */
> +#define GSW1XX_TX_PORT_MAP_HIGH_SHIFT	0
> +#define GSW1XX_TX_PORT_MAP_HIGH_MASK	GENMASK(7, 0)
> +
> +#define GSW1XX_RX_HEADER_LEN		8

Usually you use two separate macros when the lengths are not equal, and
you set .needed_headroom to the largest value.

> +
> +/* special tag in RX path header */
> +/* Byte 4 */
> +#define GSW1XX_RX_PORT_MAP_LOW_SHIFT	0
> +#define GSW1XX_RX_PORT_MAP_LOW_MASK	GENMASK(7, 0)
> +
> +/* Byte 5 */
> +#define GSW1XX_RX_PORT_MAP_HIGH_SHIFT	0
> +#define GSW1XX_RX_PORT_MAP_HIGH_MASK	GENMASK(7, 0)
> +
> +static struct sk_buff *gsw1xx_tag_xmit(struct sk_buff *skb,
> +				       struct net_device *dev)
> +{
> +	struct dsa_port *dp = dsa_user_to_port(dev);
> +	u8 *gsw1xx_tag;
> +
> +	/* provide additional space 'GSW1XX_TX_HEADER_LEN' bytes */
> +	skb_push(skb, GSW1XX_TX_HEADER_LEN);
> +
> +	/* add space between MAC address and Ethertype */
> +	dsa_alloc_etype_header(skb, GSW1XX_TX_HEADER_LEN);
> +
> +	/* special tag ingress */
> +	gsw1xx_tag = dsa_etype_header_pos_tx(skb);
> +	gsw1xx_tag[0] = 0x88;
> +	gsw1xx_tag[1] = 0xc3;

Could you write this as a u16 pointer, to make it obvious to everyone
it's an EtherType, and define the EtherType constant in
include/uapi/linux/if_ether.h, to make it a bit more visible that it's
in use?

> +	gsw1xx_tag[2] = GSW1XX_TX_PORT_MAP_EN | GSW1XX_TX_LRN_DIS;
> +	gsw1xx_tag[3] = BIT(dp->index + GSW1XX_TX_PORT_MAP_LOW_SHIFT) &
> +			GSW1XX_TX_PORT_MAP_LOW_MASK;
> +	gsw1xx_tag[4] = 0;
> +	gsw1xx_tag[5] = 0;
> +	gsw1xx_tag[6] = 0;
> +	gsw1xx_tag[7] = 0;
> +
> +	return skb;
> +}
> +
> +static struct sk_buff *gsw1xx_tag_rcv(struct sk_buff *skb,
> +				      struct net_device *dev)
> +{
> +	int port;
> +	u8 *gsw1xx_tag;
> +
> +	if (unlikely(!pskb_may_pull(skb, GSW1XX_RX_HEADER_LEN))) {
> +		dev_warn_ratelimited(&dev->dev, "Dropping packet, cannot pull SKB\n");
> +		return NULL;
> +	}
> +
> +	gsw1xx_tag = dsa_etype_header_pos_rx(skb);
> +
> +	if (gsw1xx_tag[0] != 0x88 && gsw1xx_tag[1] != 0xc3) {
> +		dev_warn_ratelimited(&dev->dev, "Dropping packet due to invalid special tag\n");
> +		dev_warn_ratelimited(&dev->dev,
> +				     "Tag: 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x\n",
> +				     gsw1xx_tag[0], gsw1xx_tag[1], gsw1xx_tag[2], gsw1xx_tag[3],
> +				     gsw1xx_tag[4], gsw1xx_tag[5], gsw1xx_tag[6], gsw1xx_tag[7]);

I think you could print the tag with %*ph, according to
https://elixir.bootlin.com/linux/v6.17.5/source/lib/vsprintf.c#L2453
(needs testing)

> +		return NULL;
> +	}
> +
> +	/* Get source port information */
> +	port = (gsw1xx_tag[2] & GSW1XX_RX_PORT_MAP_LOW_MASK) >> GSW1XX_RX_PORT_MAP_LOW_SHIFT;
> +	skb->dev = dsa_conduit_find_user(dev, 0, port);
> +	if (!skb->dev) {
> +		dev_warn_ratelimited(&dev->dev, "Dropping packet due to invalid source port\n");
> +		dev_warn_ratelimited(&dev->dev,
> +				     "Tag: 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x\n",
> +				     gsw1xx_tag[0], gsw1xx_tag[1], gsw1xx_tag[2], gsw1xx_tag[3],
> +				     gsw1xx_tag[4], gsw1xx_tag[5], gsw1xx_tag[6], gsw1xx_tag[7]);
> +		return NULL;
> +	}
> +
> +	/* remove the GSW1xx special tag between MAC addresses and the current
> +	 * ethertype field.
> +	 */
> +	skb_pull_rcsum(skb, GSW1XX_RX_HEADER_LEN);
> +	dsa_strip_etype_header(skb, GSW1XX_RX_HEADER_LEN);

You're not setting skb->offload_fwd_mark but you implement
port_bridge_join() so you offload L2 switching. If a packet gets flooded
from port A to the CPU and also to port B, don't you see that the
software bridge also creates a packet copy that it sends to port B a
second time?

> +
> +	return skb;
> +}
> +
> +static const struct dsa_device_ops gsw1xx_netdev_ops = {
> +	.name = GSW1XX_TAG_NAME,
> +	.proto	= DSA_TAG_PROTO_MXL_GSW1XX,
> +	.xmit = gsw1xx_tag_xmit,
> +	.rcv = gsw1xx_tag_rcv,
> +	.needed_headroom = GSW1XX_RX_HEADER_LEN,
> +};
> +
> +MODULE_DESCRIPTION("DSA tag driver for MaxLinear GSW1xx 8 byte protocol");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_MXL_GSW1XX, GSW1XX_TAG_NAME);
> +
> +module_dsa_tag_driver(gsw1xx_netdev_ops);
> -- 
> 2.51.1

On Tue, Oct 28, 2025 at 02:28:41AM +0200, Vladimir Oltean wrote:
> On Sun, Oct 26, 2025 at 11:48:23PM +0000, Daniel Golle wrote:
> > Add support for a new DSA tagging protocol driver for the MaxLinear
> > GSW1xx switch family. The GSW1xx switches use a proprietary 8-byte
> > special tag inserted between the source MAC address and the EtherType
> > field to indicate the source and destination ports for frames
> > traversing the CPU port.
> > 
> > Implement the tag handling logic to insert the special tag on transmit
> > and parse it on receive.
> > [...]
> > --- /dev/null
> > +++ b/net/dsa/tag_mxl-gsw1xx.c
> > [...]
> > +#define GSW1XX_TX_CLASS_SHIFT		0
> > +#define GSW1XX_TX_CLASS_MASK		GENMASK(3, 0)
> 
> Using FIELD_PREP() would eliminate these _SHIFT definitions and _MASK
> would also go away from the macro names.

Ack, using FIELD_PREP() and FIELD_GET() does improve readability and
I'll use that.

> 
> > +
> > +/* Byte 3 */
> > +#define GSW1XX_TX_PORT_MAP_LOW_SHIFT	0
> > +#define GSW1XX_TX_PORT_MAP_LOW_MASK	GENMASK(7, 0)
> > +
> > +/* Byte 4 */
> > +#define GSW1XX_TX_PORT_MAP_HIGH_SHIFT	0
> > +#define GSW1XX_TX_PORT_MAP_HIGH_MASK	GENMASK(7, 0)
> > +
> > +#define GSW1XX_RX_HEADER_LEN		8
> 
> Usually you use two separate macros when the lengths are not equal, and
> you set .needed_headroom to the largest value.

A single macro
#define GSW1XX_HEADER_LEN			8
will do the trick as they are anyway equal, right?

> > [...]
> > +	u8 *gsw1xx_tag;
> > +
> > +	/* provide additional space 'GSW1XX_TX_HEADER_LEN' bytes */
> > +	skb_push(skb, GSW1XX_TX_HEADER_LEN);
> > +
> > +	/* add space between MAC address and Ethertype */
> > +	dsa_alloc_etype_header(skb, GSW1XX_TX_HEADER_LEN);
> > +
> > +	/* special tag ingress */
> > +	gsw1xx_tag = dsa_etype_header_pos_tx(skb);
> > +	gsw1xx_tag[0] = 0x88;
> > +	gsw1xx_tag[1] = 0xc3;
> 
> Could you write this as a u16 pointer, to make it obvious to everyone
> it's an EtherType, and define the EtherType constant in
> include/uapi/linux/if_ether.h, to make it a bit more visible that it's
> in use?

Defining the EtherType in the appropriate header makes sense (even though
0x88c3 is just the default and configuration of the chip allows to set it
to anything else, or even have it omitted entirely).

Using __be16 to access the tag fields will make the whole thing
sensitive to endianess, which is a bit messy. I would prefer to keep
using u8 type and some shifting and masking of the EtherType constant
similar to how it is done in tag_dsa.c. Also note that the datasheet
describes the special tag byte-by-byte, and there is even a 16-bit field
which crosses word boundaries, GSW1XX_TX_PORT_MAP_LOW and
GSW1XX_TX_PORT_MAP_HIGH (ie. it is obvious that this wasn't intended to
be accessed as 16-bit words). So I'd rather make it easy to understand
how the tag driver matches the datasheet instead of using __be16 just
for the sake of the EtherType.

I've implemented and tested using __be16 now, and it doesn't look very
bad either, especially when skipping the PORT_MAP_HIGH/LOW part because
on the actually produced chips there anyway aren't ever more than 6
ports, so one anyway always only accesses the LOW part of the portmap.

If you like to use __be16 (like eg. the realtek taggers) I will proceed
like that in v4.

> > [...] 
> > +	if (gsw1xx_tag[0] != 0x88 && gsw1xx_tag[1] != 0xc3) {
> > +		dev_warn_ratelimited(&dev->dev, "Dropping packet due to invalid special tag\n");
> > +		dev_warn_ratelimited(&dev->dev,
> > +				     "Tag: 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x\n",
> > +				     gsw1xx_tag[0], gsw1xx_tag[1], gsw1xx_tag[2], gsw1xx_tag[3],
> > +				     gsw1xx_tag[4], gsw1xx_tag[5], gsw1xx_tag[6], gsw1xx_tag[7]);
> 
> I think you could print the tag with %*ph, according to
> https://elixir.bootlin.com/linux/v6.17.5/source/lib/vsprintf.c#L2453
> (needs testing)

I've tested that and it works fine (looks slightly different of course due
to the missing '0x' prefix, but that doesn't matter for debugging)

> > [...]
> > +	/* remove the GSW1xx special tag between MAC addresses and the current
> > +	 * ethertype field.
> > +	 */
> > +	skb_pull_rcsum(skb, GSW1XX_RX_HEADER_LEN);
> > +	dsa_strip_etype_header(skb, GSW1XX_RX_HEADER_LEN);
> 
> You're not setting skb->offload_fwd_mark but you implement
> port_bridge_join() so you offload L2 switching. If a packet gets flooded
> from port A to the CPU and also to port B, don't you see that the
> software bridge also creates a packet copy that it sends to port B a
> second time?

No, the opposite is true. If I set
dsa_default_offload_fwd_mark(skb);
forwarding between the ports no longer works.
It can well be that this is an existing flaw in the driver, as tag_gswip.c
also doesn't set offload_fwd_mark.