Message-ID: <20260112210716.vhznted6ojxca6bz@skbuf>
Date: Mon, 12 Jan 2026 23:07:16 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Linus Walleij <linusw@...nel.org>
Cc: Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/2] net: dsa: tag_ks8995: Add the KS8995 tag
 handling

Hi Linus,

It's nice to see old hardware being consolidated into the DSA framework.
Thanks for that effort.

On Wed, Jan 07, 2026 at 01:57:14PM +0100, Linus Walleij wrote:
> The KS8995 100Mbit switch can do proper DSA per-port tagging
> with the proper set-up. This adds the code to handle ingress
> and egress KS8995 tags.
> 
> The tag is a modified 0x8100 ethertype tag where a bit in the
> last byte is set for each target port.
> 
> Signed-off-by: Linus Walleij <linusw@...nel.org>
> ---
>  include/net/dsa.h    |   2 +
>  net/dsa/Kconfig      |   6 +++
>  net/dsa/Makefile     |   1 +
>  net/dsa/tag_ks8995.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 123 insertions(+)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index cced1a866757..b4c1ac14d051 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -57,6 +57,7 @@ struct tc_action;
>  #define DSA_TAG_PROTO_BRCM_LEGACY_FCS_VALUE	29
>  #define DSA_TAG_PROTO_YT921X_VALUE		30
>  #define DSA_TAG_PROTO_MXL_GSW1XX_VALUE		31
> +#define DSA_TAG_PROTO_KS8995_VALUE		32
>  
>  enum dsa_tag_protocol {
>  	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
> @@ -91,6 +92,7 @@ enum dsa_tag_protocol {
>  	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,
> +	DSA_TAG_PROTO_KS8995		= DSA_TAG_PROTO_KS8995_VALUE,
>  };
>  
>  struct dsa_switch;
> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index f86b30742122..c5272dc7af88 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -112,6 +112,12 @@ config NET_DSA_TAG_MXL_GSW1XX
>  	  Say Y or M if you want to enable support for tagging frames for
>  	  MaxLinear GSW1xx switches.
>  
> +config NET_DSA_TAG_KS8995
> +	tristate "Tag driver for Micrel KS8995 switch"
> +	help
> +	  Say Y if you want to enable support for tagging frames for the
> +	  Micrel KS8995 switch.
> +
>  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 42d173f5a701..03eed7653a34 100644
> --- a/net/dsa/Makefile
> +++ b/net/dsa/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_NET_DSA_TAG_BRCM_COMMON) += tag_brcm.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_KS8995) += tag_ks8995.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
> diff --git a/net/dsa/tag_ks8995.c b/net/dsa/tag_ks8995.c
> new file mode 100644
> index 000000000000..a5adda4767a3
> --- /dev/null
> +++ b/net/dsa/tag_ks8995.c
> @@ -0,0 +1,114 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2025 Linus Walleij <linusw@...nel.org>

You can update to 2026.

> + */
> +#include <linux/etherdevice.h>
> +#include <linux/log2.h>
> +#include <linux/list.h>
> +#include <linux/slab.h>
> +
> +#include "tag.h"
> +
> +/* The KS8995 Special Tag Packet ID (STPID)
> + * pushes its tag in a way similar to a VLAN tag
> + * -----------------------------------------------------------
> + * | MAC DA | MAC SA | 2 bytes tag | 2 bytes TCI | EtherType |
> + * -----------------------------------------------------------
> + * The tag is: 0x8100 |= BIT(port), ports 0,1,2,3
> + */
> +
> +#define KS8995_NAME "ks8995"
> +
> +#define KS8995_TAG_LEN 4
> +
> +static struct sk_buff *ks8995_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct dsa_port *dp = dsa_user_to_port(dev);
> +	u16 ks8995_tag;
> +	__be16 *p;
> +	u16 port;
> +	u16 tci;
> +
> +	/* Prepare the special KS8995 tags */
> +	port = dsa_xmit_port_mask(skb, dev);
> +	/* The manual says to set this to the CPU port if no port is indicated */
> +	if (!port)
> +		port = BIT(5);
> +
> +	ks8995_tag = ETH_P_8021Q | port;
> +	tci = port & VLAN_VID_MASK;

I think this is incorrect on multiple counts.

The first red flag is that the port mask is packed twice into the tag,
once into the TPID and second into the TCI. I opened the reference
manual and my reading is that the TCI portion is unnecessary/incorrect;
it remains processed by the switch as a TCI (aka VLAN ID + PCP), with no
port semantics overlaid on top.

Regarding the sentence "The manual says to set this to the CPU port if
no port is indicated" - I did not find that. I just found these
sentences instead:
- No change to TCI if not null VID
- Replace VID with ingress (port 5) port VID if null VID

which say a different story.

If VID==0, the packet will be processed in the CPU port's PVID,
otherwise the VID is preserved during the forwarding process. In both
cases, the VID may be stripped on egress, depending on VLAN table
settings, or not.

Practically, this means you have a combined DSA+VLAN tag.
On xmit, you'd need logic like this (not compiled, sorry):

#define KS8995M_STPID_STD	GENMASK(15, 4)
#define KS8995M_STPID_PORTMASK	GENMASK(3, 0)
#define KS8995M_STPID(portmask)	htons(ETH_P_8021Q | FIELD_PREP(KS8995M_STPID_PORTMASK, portmask))

	struct vlan_ethhdr *hdr = vlan_eth_hdr(skb);
	bool have_hwaccel_tag = false;
	int tci = 0, portmask;

	portmask = dsa_xmit_port_mask(skb, dev);

	if (skb_vlan_tag_present(skb) && skb->vlan_proto == htons(ETH_P_8021Q)) {
		tci = skb_vlan_tag_get(skb);
		__vlan_hwaccel_clear_tag(skb);
		have_hwaccel_tag = true;
	}

	if (have_hwaccel_tag || hdr->h_vlan_proto != htons(ETH_P_8021Q)) {
		skb = vlan_insert_tag(skb, KS8995M_STPID(portmask), tci);
		if (!skb)
			return NULL;
	} else {
		/* VLAN tag already exists in skb head, modify it in place */
		hdr->h_vlan_proto = KS8995M_STPID(portmask);
		hdr->h_vlan_TCI = htons(tci);
	}

and on rcv, something like this:

	/* We are expecting all received packets to have a mangled VLAN
	 * TPID, so drop anything else. Because of the non-standard TPID,
	 * don't even bother looking for a tag in the hwaccel area
	 */
	if (FIELD_GET(KS8995M_STPID_STD, ntohs(skb->protocol)) != ETH_P_8021Q)
		return NULL;

	/* Move the custom DSA+VLAN tag into the hwaccel area and strip
	 * it from the skb head
	 */
	skb = skb_vlan_untag(skb);
	if (!skb)
		return NULL;

	portmask = FIELD_GET(KS8995M_STPID_PORTMASK, ntohs(skb->vlan_proto));
	skb->dev = dsa_conduit_find_user(dev, 0, ilog2(portmask));
	if (!skb->dev)
		return NULL;

	/* Preserve the VLAN tag if it contains a non-zero VID or PCP,
	 * and restore its TPID to the standard value
	 */
	skb->vlan_proto = htons(ETH_P_8021Q);
	if (!skb->vlan_tci)
		__vlan_hwaccel_clear_tag(skb);

	dsa_default_offload_fwd_mark(skb);

Lastly, dsa_xmit_port_mask() never returns 0, so the extra code is dead.

The reason why you didn't notice anything out of place when transmitting
packets with VID=BIT(port) is, I believe, because you're configuring the
user ports as egress-untagged for all VLANs. Contrary to more advanced
switches where the untagged port mask is per VLAN, here it is per port
(the same "Tag insertion" and "Tag removal" bits from the Port Control 0
registers that you're also enabling on the CPU port). So any blunder in
the tagging protocol is being wiped by the switch.

> +
> +	/* Push in a tag between MAC and ethertype */
> +	netdev_dbg(dev, "egress packet tag: add tag %04x %04x to port %d\n",
> +		   ks8995_tag, tci, dp->index);
> +
> +	skb_push(skb, KS8995_TAG_LEN);
> +	dsa_alloc_etype_header(skb, KS8995_TAG_LEN);
> +
> +	p = dsa_etype_header_pos_tx(skb);
> +	p[0] = htons(ks8995_tag);
> +	p[1] = htons(tci);
> +
> +	return skb;
> +}
> +
> +static struct sk_buff *ks8995_rcv(struct sk_buff *skb, struct net_device *dev)
> +{
> +	unsigned int port;
> +	__be16 *p;
> +	u16 etype;
> +	u16 tci;
> +
> +	if (unlikely(!pskb_may_pull(skb, KS8995_TAG_LEN))) {
> +		netdev_err(dev, "dropping packet, cannot pull\n");
> +		return NULL;
> +	}
> +
> +	p = dsa_etype_header_pos_rx(skb);
> +	etype = ntohs(p[0]);
> +
> +	if (etype == ETH_P_8021Q) {
> +		/* That's just an ordinary VLAN tag, pass through */

I hope you don't have a use case for such packets passing through.

> +		return skb;
> +	}
> +
> +	if ((etype & 0xFFF0U) != ETH_P_8021Q) {
> +		/* Not custom, just pass through */
> +		netdev_dbg(dev, "non-KS8995 ethertype 0x%04x\n", etype);
> +		return skb;
> +	}
> +
> +	port = ilog2(etype & 0xF);
> +	tci = ntohs(p[1]);
> +	netdev_dbg(dev, "ingress packet tag: %04x %04x, port %d\n",
> +		   etype, tci, port);
> +
> +	skb->dev = dsa_conduit_find_user(dev, 0, port);
> +	if (!skb->dev) {
> +		netdev_err(dev, "could not find user for port %d\n", port);
> +		return NULL;
> +	}
> +
> +	/* Remove KS8995 tag and recalculate checksum */
> +	skb_pull_rcsum(skb, KS8995_TAG_LEN);
> +
> +	dsa_strip_etype_header(skb, KS8995_TAG_LEN);
> +
> +	dsa_default_offload_fwd_mark(skb);
> +
> +	return skb;
> +}
> +
> +static const struct dsa_device_ops ks8995_netdev_ops = {
> +	.name = KS8995_NAME,
> +	.proto	= DSA_TAG_PROTO_KS8995,
> +	.xmit = ks8995_xmit,
> +	.rcv = ks8995_rcv,
> +	.needed_headroom = KS8995_TAG_LEN,

VLAN_HLEN

> +};
> +
> +MODULE_DESCRIPTION("DSA tag driver for Micrel KS8995 family of switches");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KS8995, KS8995_NAME);
> +
> +module_dsa_tag_driver(ks8995_netdev_ops);
> 
> -- 
> 2.52.0
> 

Powered by blists - more mailing lists