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

[Vladimir Oltean <olteanv@gmail.com>]

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".

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
>