[net-next v4 2/3] net: dsa: tag_yt921x: add support for Motorcomm YT921x tags

David Yang posted 3 patches 1 month, 2 weeks ago
There is a newer version of this series
[net-next v4 2/3] net: dsa: tag_yt921x: add support for Motorcomm YT921x tags
Posted by David Yang 1 month, 2 weeks ago
Add support for Motorcomm YT921x tags, which includes a proper
configurable ethertype field (default to 0x9988).

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 include/net/dsa.h    |   2 +
 net/dsa/Kconfig      |   6 ++
 net/dsa/Makefile     |   1 +
 net/dsa/tag_yt921x.c | 128 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 137 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..d10b9a00c0b6
--- /dev/null
+++ b/net/dsa/tag_yt921x.c
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Motorcomm YT921x Switch Extend CPU Port
+ *
+ * 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;
+	struct dsa_port *partner;
+	__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);
+	if (dp->hsr_dev)
+		dsa_hsr_foreach_port(partner, dp->ds, dp->hsr_dev)
+			tx |= YT921X_TAG_TX_PORTn(partner->index);
+	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)) {
+		netdev_err(netdev, "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)) {
+		netdev_err(netdev, "Cannot locate rx 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
Re: [net-next v4 2/3] net: dsa: tag_yt921x: add support for Motorcomm YT921x tags
Posted by Andrew Lunn 1 month, 2 weeks ago
> +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;
> +	struct dsa_port *partner;
> +	__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);
> +	if (dp->hsr_dev)
> +		dsa_hsr_foreach_port(partner, dp->ds, dp->hsr_dev)
> +			tx |= YT921X_TAG_TX_PORTn(partner->index);

As far as i remember, this was not in v1. When i spotting this in v2
that made me comment you should not add new features in revision of a
patch.

Does the current version of the DSA driver support hsr? Is this
useful? Maybe it would be better to add hsr support as a follow up
patch?

> +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)) {
> +		netdev_err(netdev, "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)) {
> +		netdev_err(netdev, "Cannot locate rx port %u\n", port);
> +		return NULL;
> +	}

O.K. Stop. Think.

You changed the rate limiting to an unlimiting netdev_err().

What is the difference? Under what conditions would you want to use
rate limiting? When would you not use rate limiting?

Please reply and explain why you made this change.

	Andrew
Re: [net-next v4 2/3] net: dsa: tag_yt921x: add support for Motorcomm YT921x tags
Posted by Yangfl 1 month, 2 weeks ago
On Tue, Aug 19, 2025 at 1:07 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;
> > +     struct dsa_port *partner;
> > +     __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);
> > +     if (dp->hsr_dev)
> > +             dsa_hsr_foreach_port(partner, dp->ds, dp->hsr_dev)
> > +                     tx |= YT921X_TAG_TX_PORTn(partner->index);
>
> As far as i remember, this was not in v1. When i spotting this in v2
> that made me comment you should not add new features in revision of a
> patch.
>
> Does the current version of the DSA driver support hsr? Is this
> useful? Maybe it would be better to add hsr support as a follow up
> patch?

Sorry, this was forgotten to undo.


> > +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)) {
> > +             netdev_err(netdev, "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)) {
> > +             netdev_err(netdev, "Cannot locate rx port %u\n", port);
> > +             return NULL;
> > +     }
>
> O.K. Stop. Think.
>
> You changed the rate limiting to an unlimiting netdev_err().
>
> What is the difference? Under what conditions would you want to use
> rate limiting? When would you not use rate limiting?
>
> Please reply and explain why you made this change.
>
>         Andrew

I copied the limited version from tag_vsc73xx_8021q.

Under no conditions I expect either of them to appear: it is the case
when I did my own tests; unless something really bad happens, like
pouring a cup of coffee over your device.

I know rate limiting is a way to prevent flooding the same message
over dmesg, but if an event never happens, I would consider two
methods are exchangeable. Theoretically if an event never happens, no
warnings would ever be needed, but I placed one here in case you
destroy your device accidentally.

Thus if you think rate limiting is not appropriate here, I would fix
it with another.
Re: [net-next v4 2/3] net: dsa: tag_yt921x: add support for Motorcomm YT921x tags
Posted by Andrew Lunn 1 month, 2 weeks ago
> > > +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)) {
> > > +             netdev_err(netdev, "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)) {
> > > +             netdev_err(netdev, "Cannot locate rx port %u\n", port);
> > > +             return NULL;
> > > +     }
> >
> > O.K. Stop. Think.
> >
> > You changed the rate limiting to an unlimiting netdev_err().
> >
> > What is the difference? Under what conditions would you want to use
> > rate limiting? When would you not use rate limiting?
> >
> > Please reply and explain why you made this change.
> >
> >         Andrew
> 
> I copied the limited version from tag_vsc73xx_8021q.
> 
> Under no conditions I expect either of them to appear: it is the case
> when I did my own tests; unless something really bad happens, like
> pouring a cup of coffee over your device.
> 
> I know rate limiting is a way to prevent flooding the same message
> over dmesg, but if an event never happens, I would consider two
> methods are exchangeable. Theoretically if an event never happens, no
> warnings would ever be needed, but I placed one here in case you
> destroy your device accidentally.
> 
> Thus if you think rate limiting is not appropriate here, I would fix
> it with another.

My thinking is, every single packet come through here. That could be
100,000 packets per second. We have no control over these packets,
they are from other hosts, they can contain anything, included
specially crafted packets, ping of death packets, etc. If there is a
way to trigger these prints, and they print 100,000 times per second,
it makes a good DoS. The box is completely useless. With rate
limiting, the box keeps working, there is no DoS, but you get to see
the error messages.

The basic networking philosophy is that anything per packet related
which could cause a print should be rate limited.

      Andrew