Add support for Motorcomm YT921x tags, which includes a proper
configurable ethertype field (default to 0x9988).
Signed-off-by: David Yang <mmyangfl@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
include/net/dsa.h | 2 +
net/dsa/Kconfig | 6 +++
net/dsa/Makefile | 1 +
net/dsa/tag_yt921x.c | 126 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 135 insertions(+)
create mode 100644 net/dsa/tag_yt921x.c
diff --git a/include/net/dsa.h b/include/net/dsa.h
index d73ea0880066..67762fdaf3c7 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -55,6 +55,7 @@ struct tc_action;
#define DSA_TAG_PROTO_LAN937X_VALUE 27
#define DSA_TAG_PROTO_VSC73XX_8021Q_VALUE 28
#define DSA_TAG_PROTO_BRCM_LEGACY_FCS_VALUE 29
+#define DSA_TAG_PROTO_YT921X_VALUE 30
enum dsa_tag_protocol {
DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE,
@@ -87,6 +88,7 @@ enum dsa_tag_protocol {
DSA_TAG_PROTO_RZN1_A5PSW = DSA_TAG_PROTO_RZN1_A5PSW_VALUE,
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,
};
struct dsa_switch;
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
index 869cbe57162f..6b94028b1fcc 100644
--- a/net/dsa/Kconfig
+++ b/net/dsa/Kconfig
@@ -190,4 +190,10 @@ config NET_DSA_TAG_XRS700X
Say Y or M if you want to enable support for tagging frames for
Arrow SpeedChips XRS700x switches that use a single byte tag trailer.
+config NET_DSA_TAG_YT921X
+ tristate "Tag driver for Motorcomm YT921x switches"
+ help
+ Say Y or M if you want to enable support for tagging frames for
+ Motorcomm YT921x switches.
+
endif
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index 555c07cfeb71..4b011a1d5c87 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_NET_DSA_TAG_SJA1105) += tag_sja1105.o
obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
obj-$(CONFIG_NET_DSA_TAG_VSC73XX_8021Q) += tag_vsc73xx_8021q.o
obj-$(CONFIG_NET_DSA_TAG_XRS700X) += tag_xrs700x.o
+obj-$(CONFIG_NET_DSA_TAG_YT921X) += tag_yt921x.o
# for tracing framework to find trace.h
CFLAGS_trace.o := -I$(src)
diff --git a/net/dsa/tag_yt921x.c b/net/dsa/tag_yt921x.c
new file mode 100644
index 000000000000..ab7f97367e76
--- /dev/null
+++ b/net/dsa/tag_yt921x.c
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Motorcomm YT921x Switch Extended CPU Port Tagging
+ *
+ * Copyright (c) 2025 David Yang <mmyangfl@gmail.com>
+ *
+ * +----+----+-------+-----+----+---------
+ * | DA | SA | TagET | Tag | ET | Payload ...
+ * +----+----+-------+-----+----+---------
+ * 6 6 2 6 2 N
+ *
+ * Tag Ethertype: CPU_TAG_TPID_TPID (default: 0x9988)
+ * Tag:
+ * 2: Service VLAN Tag
+ * 2: Rx Port
+ * 15b: Rx Port Valid
+ * 14b-11b: Rx Port
+ * 10b-0b: Unknown value 0x80
+ * 2: Tx Port(s)
+ * 15b: Tx Port(s) Valid
+ * 10b-0b: Tx Port(s) Mask
+ */
+
+#include <linux/etherdevice.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+
+#include "tag.h"
+
+#define YT921X_NAME "yt921x"
+
+#define YT921X_TAG_LEN 8
+
+#define ETH_P_YT921X 0x9988
+
+#define YT921X_TAG_PORT_EN BIT(15)
+#define YT921X_TAG_RX_PORT_M GENMASK(14, 11)
+#define YT921X_TAG_RX_CMD_M GENMASK(10, 0)
+#define YT921X_TAG_RX_CMD(x) FIELD_PREP(YT921X_TAG_RX_CMD_M, (x))
+#define YT921X_TAG_RX_CMD_UNK_NORMAL 0x80
+#define YT921X_TAG_TX_PORTS_M GENMASK(10, 0)
+#define YT921X_TAG_TX_PORTn(port) BIT(port)
+
+static struct sk_buff *
+yt921x_tag_xmit(struct sk_buff *skb, struct net_device *netdev)
+{
+ struct dsa_port *dp = dsa_user_to_port(netdev);
+ unsigned int port = dp->index;
+ __be16 *tag;
+ u16 tx;
+
+ skb_push(skb, YT921X_TAG_LEN);
+ dsa_alloc_etype_header(skb, YT921X_TAG_LEN);
+
+ tag = dsa_etype_header_pos_tx(skb);
+
+ /* We might use yt921x_priv::tag_eth_p, but
+ * 1. CPU_TAG_TPID could be configured anyway;
+ * 2. Are you using the right chip?
+ */
+ tag[0] = htons(ETH_P_YT921X);
+ /* Service VLAN tag not used */
+ tag[1] = 0;
+ tag[2] = 0;
+ tx = YT921X_TAG_PORT_EN | YT921X_TAG_TX_PORTn(port);
+ tag[3] = htons(tx);
+
+ /* Now tell the conduit network device about the desired output queue
+ * as well
+ */
+ skb_set_queue_mapping(skb, port);
+
+ return skb;
+}
+
+static struct sk_buff *
+yt921x_tag_rcv(struct sk_buff *skb, struct net_device *netdev)
+{
+ unsigned int port;
+ __be16 *tag;
+ u16 rx;
+
+ if (unlikely(!pskb_may_pull(skb, YT921X_TAG_LEN)))
+ return NULL;
+
+ tag = (__be16 *)skb->data;
+
+ /* Locate which port this is coming from */
+ rx = ntohs(tag[1]);
+ if (unlikely((rx & YT921X_TAG_PORT_EN) == 0)) {
+ dev_warn_ratelimited(&netdev->dev,
+ "Unexpected rx tag 0x%04x\n", rx);
+ return NULL;
+ }
+
+ port = FIELD_GET(YT921X_TAG_RX_PORT_M, rx);
+ skb->dev = dsa_conduit_find_user(netdev, 0, port);
+ if (unlikely(!skb->dev)) {
+ dev_warn_ratelimited(&netdev->dev,
+ "Couldn't decode source port %u\n", port);
+ return NULL;
+ }
+
+ /* Remove YT921x tag and update checksum */
+ skb_pull_rcsum(skb, YT921X_TAG_LEN);
+
+ dsa_default_offload_fwd_mark(skb);
+
+ dsa_strip_etype_header(skb, YT921X_TAG_LEN);
+
+ return skb;
+}
+
+static const struct dsa_device_ops yt921x_netdev_ops = {
+ .name = YT921X_NAME,
+ .proto = DSA_TAG_PROTO_YT921X,
+ .xmit = yt921x_tag_xmit,
+ .rcv = yt921x_tag_rcv,
+ .needed_headroom = YT921X_TAG_LEN,
+};
+
+MODULE_DESCRIPTION("DSA tag driver for Motorcomm YT921x switches");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_YT921X, YT921X_NAME);
+
+module_dsa_tag_driver(yt921x_netdev_ops);
--
2.50.1On Sun, Aug 24, 2025 at 08:51:10AM +0800, David Yang wrote:
> diff --git a/net/dsa/Makefile b/net/dsa/Makefile
> index 555c07cfeb71..4b011a1d5c87 100644
> --- a/net/dsa/Makefile
> +++ b/net/dsa/Makefile
> @@ -39,6 +39,7 @@ obj-$(CONFIG_NET_DSA_TAG_SJA1105) += tag_sja1105.o
> obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
> obj-$(CONFIG_NET_DSA_TAG_VSC73XX_8021Q) += tag_vsc73xx_8021q.o
> obj-$(CONFIG_NET_DSA_TAG_XRS700X) += tag_xrs700x.o
> +obj-$(CONFIG_NET_DSA_TAG_YT921X) += tag_yt921x.o
>
> # for tracing framework to find trace.h
> CFLAGS_trace.o := -I$(src)
> diff --git a/net/dsa/tag_yt921x.c b/net/dsa/tag_yt921x.c
> new file mode 100644
> index 000000000000..ab7f97367e76
> --- /dev/null
> +++ b/net/dsa/tag_yt921x.c
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Motorcomm YT921x Switch Extended CPU Port Tagging
> + *
> + * Copyright (c) 2025 David Yang <mmyangfl@gmail.com>
> + *
> + * +----+----+-------+-----+----+---------
> + * | DA | SA | TagET | Tag | ET | Payload ...
> + * +----+----+-------+-----+----+---------
> + * 6 6 2 6 2 N
> + *
> + * Tag Ethertype: CPU_TAG_TPID_TPID (default: 0x9988)
> + * Tag:
> + * 2: Service VLAN Tag
> + * 2: Rx Port
> + * 15b: Rx Port Valid
> + * 14b-11b: Rx Port
> + * 10b-0b: Unknown value 0x80
> + * 2: Tx Port(s)
> + * 15b: Tx Port(s) Valid
> + * 10b-0b: Tx Port(s) Mask
> + */
> +
> +#include <linux/etherdevice.h>
> +#include <linux/list.h>
> +#include <linux/slab.h>
Why include list.h and slab.h?
> +
> +#include "tag.h"
> +
> +#define YT921X_NAME "yt921x"
> +
> +#define YT921X_TAG_LEN 8
> +
> +#define ETH_P_YT921X 0x9988
You can add a header in include/linux/dsa/ which is shared with the
switch driver, to avoid duplicate definitions.
> +
> +#define YT921X_TAG_PORT_EN BIT(15)
> +#define YT921X_TAG_RX_PORT_M GENMASK(14, 11)
> +#define YT921X_TAG_RX_CMD_M GENMASK(10, 0)
> +#define YT921X_TAG_RX_CMD(x) FIELD_PREP(YT921X_TAG_RX_CMD_M, (x))
> +#define YT921X_TAG_RX_CMD_UNK_NORMAL 0x80
> +#define YT921X_TAG_TX_PORTS_M GENMASK(10, 0)
> +#define YT921X_TAG_TX_PORTn(port) BIT(port)
> +
> +static struct sk_buff *
> +yt921x_tag_xmit(struct sk_buff *skb, struct net_device *netdev)
> +{
> + struct dsa_port *dp = dsa_user_to_port(netdev);
> + unsigned int port = dp->index;
> + __be16 *tag;
> + u16 tx;
> +
> + skb_push(skb, YT921X_TAG_LEN);
> + dsa_alloc_etype_header(skb, YT921X_TAG_LEN);
> +
> + tag = dsa_etype_header_pos_tx(skb);
> +
> + /* We might use yt921x_priv::tag_eth_p, but
> + * 1. CPU_TAG_TPID could be configured anyway;
> + * 2. Are you using the right chip?
The tag format sort of becomes fixed ABI as soon as user space is able
to run "cat /sys/class/net/eth0/dsa/tagging", see "yt921x", and record
it to a pcap file. Unless the EtherType bears some other meaning rather
than being a fixed value, then if you change it later to some other
value than 0x9988, you'd better also change the protocol name to
distinguish it from "yt921x".
Also, you can _not_ use yt921x_priv :: tag_eth_p, because doing so would
assume that typeof(ds->priv) == struct yt921x_priv. In principle we
would like to be able to run the tagging protocols on the dsa_loop
driver as well, which can be attached to any network interface. Very
few, if any, tagging protocol drivers don't work on dsa_loop.
> + */
> + tag[0] = htons(ETH_P_YT921X);
> + /* Service VLAN tag not used */
> + tag[1] = 0;
> + tag[2] = 0;
> + tx = YT921X_TAG_PORT_EN | YT921X_TAG_TX_PORTn(port);
> + tag[3] = htons(tx);
> +
> + /* Now tell the conduit network device about the desired output queue
> + * as well
> + */
> + skb_set_queue_mapping(skb, port);
This is generally used for integrated DSA switches, for lossless
backpressure during CPU transmission, where the conduit interface driver
is known, and has set up its queues in a special way, as a result of the
fact that it is attached to a known DSA switch (made by the same vendor).
What do you need it for, in a discrete MDIO-controlled switch?
> +
> + return skb;
> +}
> +
> +static struct sk_buff *
> +yt921x_tag_rcv(struct sk_buff *skb, struct net_device *netdev)
> +{
> + unsigned int port;
> + __be16 *tag;
> + u16 rx;
> +
> + if (unlikely(!pskb_may_pull(skb, YT921X_TAG_LEN)))
> + return NULL;
> +
> + tag = (__be16 *)skb->data;
Use dsa_etype_header_pos_rx() and validate the CPU_TAG_TPID_TPID as well.
> +
> + /* Locate which port this is coming from */
> + rx = ntohs(tag[1]);
> + if (unlikely((rx & YT921X_TAG_PORT_EN) == 0)) {
> + dev_warn_ratelimited(&netdev->dev,
> + "Unexpected rx tag 0x%04x\n", rx);
> + return NULL;
> + }
> +
> + port = FIELD_GET(YT921X_TAG_RX_PORT_M, rx);
> + skb->dev = dsa_conduit_find_user(netdev, 0, port);
> + if (unlikely(!skb->dev)) {
> + dev_warn_ratelimited(&netdev->dev,
> + "Couldn't decode source port %u\n", port);
> + return NULL;
> + }
> +
> + /* Remove YT921x tag and update checksum */
> + skb_pull_rcsum(skb, YT921X_TAG_LEN);
> +
> + dsa_default_offload_fwd_mark(skb);
> +
> + dsa_strip_etype_header(skb, YT921X_TAG_LEN);
> +
> + return skb;
> +}
> +
> +static const struct dsa_device_ops yt921x_netdev_ops = {
> + .name = YT921X_NAME,
> + .proto = DSA_TAG_PROTO_YT921X,
> + .xmit = yt921x_tag_xmit,
> + .rcv = yt921x_tag_rcv,
> + .needed_headroom = YT921X_TAG_LEN,
> +};
> +
> +MODULE_DESCRIPTION("DSA tag driver for Motorcomm YT921x switches");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_YT921X, YT921X_NAME);
> +
> +module_dsa_tag_driver(yt921x_netdev_ops);
> --
> 2.50.1
>
On Tue, Aug 26, 2025 at 6:15 AM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Sun, Aug 24, 2025 at 08:51:10AM +0800, David Yang wrote:
> > diff --git a/net/dsa/Makefile b/net/dsa/Makefile
> > index 555c07cfeb71..4b011a1d5c87 100644
> > --- a/net/dsa/Makefile
> > +++ b/net/dsa/Makefile
> > @@ -39,6 +39,7 @@ obj-$(CONFIG_NET_DSA_TAG_SJA1105) += tag_sja1105.o
> > obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o
> > obj-$(CONFIG_NET_DSA_TAG_VSC73XX_8021Q) += tag_vsc73xx_8021q.o
> > obj-$(CONFIG_NET_DSA_TAG_XRS700X) += tag_xrs700x.o
> > +obj-$(CONFIG_NET_DSA_TAG_YT921X) += tag_yt921x.o
> >
> > # for tracing framework to find trace.h
> > CFLAGS_trace.o := -I$(src)
> > diff --git a/net/dsa/tag_yt921x.c b/net/dsa/tag_yt921x.c
> > new file mode 100644
> > index 000000000000..ab7f97367e76
> > --- /dev/null
> > +++ b/net/dsa/tag_yt921x.c
> > @@ -0,0 +1,126 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Motorcomm YT921x Switch Extended CPU Port Tagging
> > + *
> > + * Copyright (c) 2025 David Yang <mmyangfl@gmail.com>
> > + *
> > + * +----+----+-------+-----+----+---------
> > + * | DA | SA | TagET | Tag | ET | Payload ...
> > + * +----+----+-------+-----+----+---------
> > + * 6 6 2 6 2 N
> > + *
> > + * Tag Ethertype: CPU_TAG_TPID_TPID (default: 0x9988)
> > + * Tag:
> > + * 2: Service VLAN Tag
> > + * 2: Rx Port
> > + * 15b: Rx Port Valid
> > + * 14b-11b: Rx Port
> > + * 10b-0b: Unknown value 0x80
> > + * 2: Tx Port(s)
> > + * 15b: Tx Port(s) Valid
> > + * 10b-0b: Tx Port(s) Mask
> > + */
> > +
> > +#include <linux/etherdevice.h>
> > +#include <linux/list.h>
> > +#include <linux/slab.h>
>
> Why include list.h and slab.h?
>
> > +
> > +#include "tag.h"
> > +
> > +#define YT921X_NAME "yt921x"
> > +
> > +#define YT921X_TAG_LEN 8
> > +
> > +#define ETH_P_YT921X 0x9988
>
> You can add a header in include/linux/dsa/ which is shared with the
> switch driver, to avoid duplicate definitions.
>
> > +
> > +#define YT921X_TAG_PORT_EN BIT(15)
> > +#define YT921X_TAG_RX_PORT_M GENMASK(14, 11)
> > +#define YT921X_TAG_RX_CMD_M GENMASK(10, 0)
> > +#define YT921X_TAG_RX_CMD(x) FIELD_PREP(YT921X_TAG_RX_CMD_M, (x))
> > +#define YT921X_TAG_RX_CMD_UNK_NORMAL 0x80
> > +#define YT921X_TAG_TX_PORTS_M GENMASK(10, 0)
> > +#define YT921X_TAG_TX_PORTn(port) BIT(port)
> > +
> > +static struct sk_buff *
> > +yt921x_tag_xmit(struct sk_buff *skb, struct net_device *netdev)
> > +{
> > + struct dsa_port *dp = dsa_user_to_port(netdev);
> > + unsigned int port = dp->index;
> > + __be16 *tag;
> > + u16 tx;
> > +
> > + skb_push(skb, YT921X_TAG_LEN);
> > + dsa_alloc_etype_header(skb, YT921X_TAG_LEN);
> > +
> > + tag = dsa_etype_header_pos_tx(skb);
> > +
> > + /* We might use yt921x_priv::tag_eth_p, but
> > + * 1. CPU_TAG_TPID could be configured anyway;
> > + * 2. Are you using the right chip?
>
> The tag format sort of becomes fixed ABI as soon as user space is able
> to run "cat /sys/class/net/eth0/dsa/tagging", see "yt921x", and record
> it to a pcap file. Unless the EtherType bears some other meaning rather
> than being a fixed value, then if you change it later to some other
> value than 0x9988, you'd better also change the protocol name to
> distinguish it from "yt921x".
>
"EtherType" here does not necessarily become EtherType; better to
think it is a key to enable port control over the switch. It could be
a dynamic random value as long as everyone gets the same value all
over the kernel, see the setup process of the switch driver. Ideally
only the remaining content of the tag should become the ABI (and is
actually enforced by the switch), but making a dynamic "EtherType" is
clearly a worse idea so I don't know how to clarify the fact...
> Also, you can _not_ use yt921x_priv :: tag_eth_p, because doing so would
> assume that typeof(ds->priv) == struct yt921x_priv. In principle we
> would like to be able to run the tagging protocols on the dsa_loop
> driver as well, which can be attached to any network interface. Very
> few, if any, tagging protocol drivers don't work on dsa_loop.
>
> > + */
> > + tag[0] = htons(ETH_P_YT921X);
> > + /* Service VLAN tag not used */
> > + tag[1] = 0;
> > + tag[2] = 0;
> > + tx = YT921X_TAG_PORT_EN | YT921X_TAG_TX_PORTn(port);
> > + tag[3] = htons(tx);
> > +
> > + /* Now tell the conduit network device about the desired output queue
> > + * as well
> > + */
> > + skb_set_queue_mapping(skb, port);
>
> This is generally used for integrated DSA switches, for lossless
> backpressure during CPU transmission, where the conduit interface driver
> is known, and has set up its queues in a special way, as a result of the
> fact that it is attached to a known DSA switch (made by the same vendor).
> What do you need it for, in a discrete MDIO-controlled switch?
>
> > +
> > + return skb;
> > +}
> > +
> > +static struct sk_buff *
> > +yt921x_tag_rcv(struct sk_buff *skb, struct net_device *netdev)
> > +{
> > + unsigned int port;
> > + __be16 *tag;
> > + u16 rx;
> > +
> > + if (unlikely(!pskb_may_pull(skb, YT921X_TAG_LEN)))
> > + return NULL;
> > +
> > + tag = (__be16 *)skb->data;
>
> Use dsa_etype_header_pos_rx() and validate the CPU_TAG_TPID_TPID as well.
>
See the above explanation why rx "EtherType" is not considered part of ABI.
> > +
> > + /* Locate which port this is coming from */
> > + rx = ntohs(tag[1]);
> > + if (unlikely((rx & YT921X_TAG_PORT_EN) == 0)) {
> > + dev_warn_ratelimited(&netdev->dev,
> > + "Unexpected rx tag 0x%04x\n", rx);
> > + return NULL;
> > + }
> > +
> > + port = FIELD_GET(YT921X_TAG_RX_PORT_M, rx);
> > + skb->dev = dsa_conduit_find_user(netdev, 0, port);
> > + if (unlikely(!skb->dev)) {
> > + dev_warn_ratelimited(&netdev->dev,
> > + "Couldn't decode source port %u\n", port);
> > + return NULL;
> > + }
> > +
> > + /* Remove YT921x tag and update checksum */
> > + skb_pull_rcsum(skb, YT921X_TAG_LEN);
> > +
> > + dsa_default_offload_fwd_mark(skb);
> > +
> > + dsa_strip_etype_header(skb, YT921X_TAG_LEN);
> > +
> > + return skb;
> > +}
> > +
> > +static const struct dsa_device_ops yt921x_netdev_ops = {
> > + .name = YT921X_NAME,
> > + .proto = DSA_TAG_PROTO_YT921X,
> > + .xmit = yt921x_tag_xmit,
> > + .rcv = yt921x_tag_rcv,
> > + .needed_headroom = YT921X_TAG_LEN,
> > +};
> > +
> > +MODULE_DESCRIPTION("DSA tag driver for Motorcomm YT921x switches");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_YT921X, YT921X_NAME);
> > +
> > +module_dsa_tag_driver(yt921x_netdev_ops);
> > --
> > 2.50.1
> >
>
On Tue, Aug 26, 2025 at 09:47:34AM +0800, Yangfl wrote:
> > The tag format sort of becomes fixed ABI as soon as user space is able
> > to run "cat /sys/class/net/eth0/dsa/tagging", see "yt921x", and record
> > it to a pcap file. Unless the EtherType bears some other meaning rather
> > than being a fixed value, then if you change it later to some other
> > value than 0x9988, you'd better also change the protocol name to
> > distinguish it from "yt921x".
>
> "EtherType" here does not necessarily become EtherType; better to
> think it is a key to enable port control over the switch. It could be
> a dynamic random value as long as everyone gets the same value all
> over the kernel, see the setup process of the switch driver. Ideally
> only the remaining content of the tag should become the ABI (and is
> actually enforced by the switch), but making a dynamic "EtherType" is
> clearly a worse idea so I don't know how to clarify the fact...
>
> > Also, you can _not_ use yt921x_priv :: tag_eth_p, because doing so would
> > assume that typeof(ds->priv) == struct yt921x_priv. In principle we
> > would like to be able to run the tagging protocols on the dsa_loop
> > driver as well, which can be attached to any network interface. Very
> > few, if any, tagging protocol drivers don't work on dsa_loop.
> > > +
> > > +static struct sk_buff *
> > > +yt921x_tag_rcv(struct sk_buff *skb, struct net_device *netdev)
> > > +{
> > > + unsigned int port;
> > > + __be16 *tag;
> > > + u16 rx;
> > > +
> > > + if (unlikely(!pskb_may_pull(skb, YT921X_TAG_LEN)))
> > > + return NULL;
> > > +
> > > + tag = (__be16 *)skb->data;
> >
> > Use dsa_etype_header_pos_rx() and validate the CPU_TAG_TPID_TPID as well.
>
> See the above explanation why rx "EtherType" is not considered part of ABI.
So what I don't understand is: it's impossible to separate RX from TX in
current DSA taggers. If TX hardcodes the assumption that the switch was
configured to use EtherType 0x9988, why would we expect RX to use anything else?
What valid configuration would we prevent, if we ensured that RX packets
have the same EtherType?
It should be ok if you documented that the EtherType is self-assigned
and must be hardcoded to 0x9988 by local convention for the moment, but
is otherwise configurable, and the meaning of a different value is
undefined. Even if self-assigned, I suppose you could still add the
value to include/uapi/linux/if_ether.h to avoid conflicts with other
uses.
Even better if you clarify the expectations by writing a libpcap
dissector for the new tagging protocol, which you need to do anyway for
recent tcpdump versions to work on the conduit interface (otherwise it
fails with "unsupported protocol"). See
https://github.com/the-tcpdump-group/libpcap/pull/1463
The libpcap dissector uses the /sys/class/net/eth0/dsa/tagging text to
identify the protocol in use, not the EtherType.
> > > +static struct sk_buff *
> > > +yt921x_tag_xmit(struct sk_buff *skb, struct net_device *netdev)
> > > +{
> > > + struct dsa_port *dp = dsa_user_to_port(netdev);
> > > + unsigned int port = dp->index;
> > > + __be16 *tag;
> > > + u16 tx;
> > > +
> > > + skb_push(skb, YT921X_TAG_LEN);
> > > + dsa_alloc_etype_header(skb, YT921X_TAG_LEN);
> > > +
> > > + tag = dsa_etype_header_pos_tx(skb);
> > > +
> > > + /* We might use yt921x_priv::tag_eth_p, but
> > > + * 1. CPU_TAG_TPID could be configured anyway;
> > > + * 2. Are you using the right chip?
> >
> > The tag format sort of becomes fixed ABI as soon as user space is able
> > to run "cat /sys/class/net/eth0/dsa/tagging", see "yt921x", and record
> > it to a pcap file. Unless the EtherType bears some other meaning rather
> > than being a fixed value, then if you change it later to some other
> > value than 0x9988, you'd better also change the protocol name to
> > distinguish it from "yt921x".
> >
>
> "EtherType" here does not necessarily become EtherType; better to
> think it is a key to enable port control over the switch. It could be
> a dynamic random value as long as everyone gets the same value all
> over the kernel, see the setup process of the switch driver. Ideally
> only the remaining content of the tag should become the ABI (and is
> actually enforced by the switch), but making a dynamic "EtherType" is
> clearly a worse idea so I don't know how to clarify the fact...
If i remember correctly, the Marvell switches allow you to set the
EtherType they use. We just use the reset default value. It has been
like this since somewhere around 2008, and nobody has needed another
value.
What use case do you have for using a different value?
Andrew
On Tue, Aug 26, 2025 at 10:18 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > > +static struct sk_buff *
> > > > +yt921x_tag_xmit(struct sk_buff *skb, struct net_device *netdev)
> > > > +{
> > > > + struct dsa_port *dp = dsa_user_to_port(netdev);
> > > > + unsigned int port = dp->index;
> > > > + __be16 *tag;
> > > > + u16 tx;
> > > > +
> > > > + skb_push(skb, YT921X_TAG_LEN);
> > > > + dsa_alloc_etype_header(skb, YT921X_TAG_LEN);
> > > > +
> > > > + tag = dsa_etype_header_pos_tx(skb);
> > > > +
> > > > + /* We might use yt921x_priv::tag_eth_p, but
> > > > + * 1. CPU_TAG_TPID could be configured anyway;
> > > > + * 2. Are you using the right chip?
> > >
> > > The tag format sort of becomes fixed ABI as soon as user space is able
> > > to run "cat /sys/class/net/eth0/dsa/tagging", see "yt921x", and record
> > > it to a pcap file. Unless the EtherType bears some other meaning rather
> > > than being a fixed value, then if you change it later to some other
> > > value than 0x9988, you'd better also change the protocol name to
> > > distinguish it from "yt921x".
> > >
> >
> > "EtherType" here does not necessarily become EtherType; better to
> > think it is a key to enable port control over the switch. It could be
> > a dynamic random value as long as everyone gets the same value all
> > over the kernel, see the setup process of the switch driver. Ideally
> > only the remaining content of the tag should become the ABI (and is
> > actually enforced by the switch), but making a dynamic "EtherType" is
> > clearly a worse idea so I don't know how to clarify the fact...
>
> If i remember correctly, the Marvell switches allow you to set the
> EtherType they use. We just use the reset default value. It has been
> like this since somewhere around 2008, and nobody has needed another
> value.
>
> What use case do you have for using a different value?
>
> Andrew
I don't. I'm just giving reasons why EtherType should not be
considered "fixed ABI" (or it is the reason it can be ABI if all of
you are fine with the reset default value).
© 2016 - 2025 Red Hat, Inc.