[PATCH net v3 0/5] net: dsa: ks8995: Post-move fixes

[PATCH net v3 0/5] net: dsa: ks8995: Post-move fixes

[Linus Walleij]

This fixes some glearing issues in the Micrel KS8995 driver
pointed out by Vladimir.

This patch series implements some required functionality
and strips the driver down to just KS8995 deeming the other
"micrel" variants to be actually handled by the Microchip
KSZ driver.

If the KS8995 should actually *also* be managed by the Microchip
driver and this driver deleted remains to be seen. It is clearly
the origin chip for that hardware: it is very close to the
"KSZ8 family" but there are differences.

It definitely has a different custom tag format for proper DSA
tagging, but I have implemented that: I now have to figure out
whether to do that on top of this driver or the KSZ driver before
continuing.

In the meantime, this patch series makes the situation better.

Signed-off-by: Linus Walleij <linusw@kernel.org>
---
Changes in v3:
- Re-tag the series for net since they are fixes.
- Implement mutual exclusion between .remove() and .shutdown()
  as found by the AI reviewer.
- Fix a typo in the patch dropping the KSZ chips support, 8955->8995,
  found by the AI reviewer.
- Add fallback compatibles for the deleted chips to the KSZ
  SPI driver.
- Implement port isolation.
- Link to v2: https://lore.kernel.org/r/20260119-ks8995-fixups-v2-0-98bd034a0d12@kernel.org

Changes in v2:
- Do port_bitmask in another way and fix a bug where BIT(port_bitmask)
  was used instead of just port_bitmask.
- Link to v1: https://lore.kernel.org/r/20260118-ks8995-fixups-v1-0-10a493f0339d@kernel.org

---
Linus Walleij (5):
      net: dsa: ks8995: Add shutdown callback
      net: dsa: microchip: Add fallback Micrel compatibles
      net: dsa: ks8955: Delete KSZ8864 and KSZ8795 support
      net: dsa: ks8995: Add stub bridge join/leave
      net: dsa: ks8995: Implement port isolation

 drivers/net/dsa/ks8995.c            | 325 +++++++++++++++++++++---------------
 drivers/net/dsa/microchip/ksz_spi.c |  15 ++
 2 files changed, 208 insertions(+), 132 deletions(-)
---
base-commit: 3841d04322c568c47d1effc869fd79fd6c967bd0
change-id: 20260118-ks8995-fixups-84f25ac3f407

Best regards,
-- 
Linus Walleij <linusw@kernel.org>

[PATCH net v3 1/5] net: dsa: ks8995: Add shutdown callback

[Linus Walleij]

The DSA framework requires that dsa_switch_shutdown() be
called when the driver is shut down.

Make sure to also implement the mutual exclusion required
by checking the platform data pointer to be NULL in
.remove() and .shutdown().

Fixes: a7fe8b266f65 ("net: dsa: ks8995: Add basic switch set-up")
Reported-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/dsa/ks8995.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/dsa/ks8995.c b/drivers/net/dsa/ks8995.c
index 77d8b842693c..2522c9057382 100644
--- a/drivers/net/dsa/ks8995.c
+++ b/drivers/net/dsa/ks8995.c
@@ -833,11 +833,26 @@ static void ks8995_remove(struct spi_device *spi)
 {
 	struct ks8995_switch *ks = spi_get_drvdata(spi);
 
+	if (!ks)
+		return;
+
 	dsa_unregister_switch(ks->ds);
 	/* assert reset */
 	gpiod_set_value_cansleep(ks->reset_gpio, 1);
 }
 
+static void ks8995_shutdown(struct spi_device *spi)
+{
+	struct ks8995_switch *ks = spi_get_drvdata(spi);
+
+	if (!ks)
+		return;
+
+	dsa_switch_shutdown(ks->ds);
+
+	spi_set_drvdata(spi, NULL);
+}
+
 /* ------------------------------------------------------------------------ */
 static struct spi_driver ks8995_driver = {
 	.driver = {
@@ -846,6 +861,7 @@ static struct spi_driver ks8995_driver = {
 	},
 	.probe	  = ks8995_probe,
 	.remove	  = ks8995_remove,
+	.shutdown = ks8995_shutdown,
 	.id_table = ks8995_id,
 };
 

-- 
2.53.0

[PATCH net v3 2/5] net: dsa: microchip: Add fallback Micrel compatibles

[Linus Walleij]

Because of forking paths when Micrel was acquire by Microchip,
two devices also exist with the micrel,* prefix bindings.
Ass these to the KSZ SPI driver so users can use the more capable
driver.

Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/dsa/microchip/ksz_spi.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz_spi.c b/drivers/net/dsa/microchip/ksz_spi.c
index d8001734b057..e3bb1ccf2331 100644
--- a/drivers/net/dsa/microchip/ksz_spi.c
+++ b/drivers/net/dsa/microchip/ksz_spi.c
@@ -312,6 +312,21 @@ static const struct of_device_id ksz_dt_ids[] = {
 		.compatible = "microchip,lan9646",
 		.data = &ksz_switch_chips[LAN9646]
 	},
+	/*
+	 * Legacy Micrel bindings. In 2015 Microchip acquired
+	 * Micrel which is the originator of the KSZ series, and
+	 * devices branded for Micrel already existed, as well as
+	 * some device tree bindings. These two products are identical
+	 * to the same Microchip products.
+	 */
+	{
+		.compatible = "micrel,ksz8864",
+		.data = &ksz_switch_chips[KSZ8864]
+	},
+	{
+		.compatible = "micrel,ksz8795",
+		.data = &ksz_switch_chips[KSZ8795]
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, ksz_dt_ids);

-- 
2.53.0

[PATCH net v3 3/5] net: dsa: ks8955: Delete KSZ8864 and KSZ8795 support

[Linus Walleij]

After studying the datasheets for a bit, I can conclude that
the register maps for the two KSZ variants explicitly said to
be supported by this driver are fully supported by the newer
Micrel KSZ driver, including full VLAN support and a different
custom tag than what the KS8995 is using.

Delete this support, users should be using the KSZ driver
CONFIG_NET_DSA_MICROCHIP_KSZ_SPI and any new device trees should
use:
micrel,ksz8864 -> microchip,ksz8864
micrel,ksz8795 -> microchip,ksz8795

Apparently Microchip acquired Micrel at some point and this
created the confusion.

Fixes: a7fe8b266f65 ("net: dsa: ks8995: Add basic switch set-up")
Reported-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/dsa/ks8995.c | 160 +++++++++--------------------------------------
 1 file changed, 28 insertions(+), 132 deletions(-)

diff --git a/drivers/net/dsa/ks8995.c b/drivers/net/dsa/ks8995.c
index 2522c9057382..b7c9de39f68e 100644
--- a/drivers/net/dsa/ks8995.c
+++ b/drivers/net/dsa/ks8995.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * SPI driver for Micrel/Kendin KS8995M and KSZ8864RMN ethernet switches
+ * SPI driver for Micrel/Kendin KS8995M ethernet switch.
  *
  * Copyright (C) 2008 Gabor Juhos <juhosg at openwrt.org>
  * Copyright (C) 2025 Linus Walleij <linus.walleij@linaro.org>
@@ -114,11 +114,7 @@
 #define KS8995_REG_IAD1		0x76    /* Indirect Access Data 1 */
 #define KS8995_REG_IAD0		0x77    /* Indirect Access Data 0 */
 
-#define KSZ8864_REG_ID1		0xfe	/* Chip ID in bit 7 */
-
 #define KS8995_REGS_SIZE	0x80
-#define KSZ8864_REGS_SIZE	0x100
-#define KSZ8795_REGS_SIZE	0x100
 
 #define ID1_CHIPID_M		0xf
 #define ID1_CHIPID_S		4
@@ -127,11 +123,8 @@
 #define ID1_START_SW		1	/* start the switch */
 
 #define FAMILY_KS8995		0x95
-#define FAMILY_KSZ8795		0x87
 #define CHIPID_M		0
 #define KS8995_CHIP_ID		0x00
-#define KSZ8864_CHIP_ID		0x01
-#define KSZ8795_CHIP_ID		0x09
 
 #define KS8995_CMD_WRITE	0x02U
 #define KS8995_CMD_READ		0x03U
@@ -140,49 +133,6 @@
 #define KS8995_NUM_PORTS	5 /* 5 ports including the CPU port */
 #define KS8995_RESET_DELAY	10 /* usec */
 
-enum ks8995_chip_variant {
-	ks8995,
-	ksz8864,
-	ksz8795,
-	max_variant
-};
-
-struct ks8995_chip_params {
-	char *name;
-	int family_id;
-	int chip_id;
-	int regs_size;
-	int addr_width;
-	int addr_shift;
-};
-
-static const struct ks8995_chip_params ks8995_chip[] = {
-	[ks8995] = {
-		.name = "KS8995MA",
-		.family_id = FAMILY_KS8995,
-		.chip_id = KS8995_CHIP_ID,
-		.regs_size = KS8995_REGS_SIZE,
-		.addr_width = 8,
-		.addr_shift = 0,
-	},
-	[ksz8864] = {
-		.name = "KSZ8864RMN",
-		.family_id = FAMILY_KS8995,
-		.chip_id = KSZ8864_CHIP_ID,
-		.regs_size = KSZ8864_REGS_SIZE,
-		.addr_width = 8,
-		.addr_shift = 0,
-	},
-	[ksz8795] = {
-		.name = "KSZ8795CLX",
-		.family_id = FAMILY_KSZ8795,
-		.chip_id = KSZ8795_CHIP_ID,
-		.regs_size = KSZ8795_REGS_SIZE,
-		.addr_width = 12,
-		.addr_shift = 1,
-	},
-};
-
 struct ks8995_switch {
 	struct spi_device	*spi;
 	struct device		*dev;
@@ -190,23 +140,18 @@ struct ks8995_switch {
 	struct mutex		lock;
 	struct gpio_desc	*reset_gpio;
 	struct bin_attribute	regs_attr;
-	const struct ks8995_chip_params	*chip;
 	int			revision_id;
 	unsigned int max_mtu[KS8995_NUM_PORTS];
 };
 
 static const struct spi_device_id ks8995_id[] = {
-	{"ks8995", ks8995},
-	{"ksz8864", ksz8864},
-	{"ksz8795", ksz8795},
+	{"ks8995", 0},
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, ks8995_id);
 
 static const struct of_device_id ks8995_spi_of_match[] = {
 	{ .compatible = "micrel,ks8995" },
-	{ .compatible = "micrel,ksz8864" },
-	{ .compatible = "micrel,ksz8795" },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, ks8995_spi_of_match);
@@ -237,10 +182,10 @@ static inline __be16 create_spi_cmd(struct ks8995_switch *ks, int cmd,
 {
 	u16 result = cmd;
 
-	/* make room for address (incl. address shift) */
-	result <<= ks->chip->addr_width + ks->chip->addr_shift;
+	/* make room for address */
+	result <<= 8;
 	/* add address */
-	result |= address << ks->chip->addr_shift;
+	result |= address;
 	/* SPI protocol needs big endian */
 	return cpu_to_be16(result);
 }
@@ -346,79 +291,37 @@ static int ks8995_reset(struct ks8995_switch *ks)
 static int ks8995_get_revision(struct ks8995_switch *ks)
 {
 	int err;
-	u8 id0, id1, ksz8864_id;
+	u8 id0, id1;
 
 	/* read family id */
 	err = ks8995_read_reg(ks, KS8995_REG_ID0, &id0);
-	if (err) {
-		err = -EIO;
-		goto err_out;
-	}
+	if (err)
+		return -EIO;
 
 	/* verify family id */
-	if (id0 != ks->chip->family_id) {
+	if (id0 != FAMILY_KS8995) {
 		dev_err(&ks->spi->dev, "chip family id mismatch: expected 0x%02x but 0x%02x read\n",
-			ks->chip->family_id, id0);
-		err = -ENODEV;
-		goto err_out;
+			FAMILY_KS8995, id0);
+		return -ENODEV;
 	}
 
-	switch (ks->chip->family_id) {
-	case FAMILY_KS8995:
-		/* try reading chip id at CHIP ID1 */
-		err = ks8995_read_reg(ks, KS8995_REG_ID1, &id1);
-		if (err) {
-			err = -EIO;
-			goto err_out;
-		}
-
-		/* verify chip id */
-		if ((get_chip_id(id1) == CHIPID_M) &&
-		    (get_chip_id(id1) == ks->chip->chip_id)) {
-			/* KS8995MA */
-			ks->revision_id = get_chip_rev(id1);
-		} else if (get_chip_id(id1) != CHIPID_M) {
-			/* KSZ8864RMN */
-			err = ks8995_read_reg(ks, KS8995_REG_ID1, &ksz8864_id);
-			if (err) {
-				err = -EIO;
-				goto err_out;
-			}
-
-			if ((ksz8864_id & 0x80) &&
-			    (ks->chip->chip_id == KSZ8864_CHIP_ID)) {
-				ks->revision_id = get_chip_rev(id1);
-			}
-
-		} else {
-			dev_err(&ks->spi->dev, "unsupported chip id for KS8995 family: 0x%02x\n",
-				id1);
-			err = -ENODEV;
-		}
-		break;
-	case FAMILY_KSZ8795:
-		/* try reading chip id at CHIP ID1 */
-		err = ks8995_read_reg(ks, KS8995_REG_ID1, &id1);
-		if (err) {
-			err = -EIO;
-			goto err_out;
-		}
+	/* try reading chip id at CHIP ID1 */
+	err = ks8995_read_reg(ks, KS8995_REG_ID1, &id1);
+	if (err)
+		return -EIO;
 
-		if (get_chip_id(id1) == ks->chip->chip_id) {
-			ks->revision_id = get_chip_rev(id1);
-		} else {
-			dev_err(&ks->spi->dev, "unsupported chip id for KSZ8795 family: 0x%02x\n",
-				id1);
-			err = -ENODEV;
-		}
-		break;
-	default:
-		dev_err(&ks->spi->dev, "unsupported family id: 0x%02x\n", id0);
-		err = -ENODEV;
-		break;
+	/* verify chip id */
+	if ((get_chip_id(id1) == CHIPID_M) &&
+	    (get_chip_id(id1) == KS8995_CHIP_ID)) {
+		/* KS8995MA */
+		ks->revision_id = get_chip_rev(id1);
+	} else {
+		dev_err(&ks->spi->dev, "unsupported chip id for KS8995 family: 0x%02x\n",
+			id1);
+		return -ENODEV;
 	}
-err_out:
-	return err;
+
+	return 0;
 }
 
 static int ks8995_check_config(struct ks8995_switch *ks)
@@ -747,12 +650,6 @@ static int ks8995_probe(struct spi_device *spi)
 {
 	struct ks8995_switch *ks;
 	int err;
-	int variant = spi_get_device_id(spi)->driver_data;
-
-	if (variant >= max_variant) {
-		dev_err(&spi->dev, "bad chip variant %d\n", variant);
-		return -ENODEV;
-	}
 
 	ks = devm_kzalloc(&spi->dev, sizeof(*ks), GFP_KERNEL);
 	if (!ks)
@@ -761,7 +658,6 @@ static int ks8995_probe(struct spi_device *spi)
 	mutex_init(&ks->lock);
 	ks->spi = spi;
 	ks->dev = &spi->dev;
-	ks->chip = &ks8995_chip[variant];
 
 	ks->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
 						 GPIOD_OUT_HIGH);
@@ -804,8 +700,8 @@ static int ks8995_probe(struct spi_device *spi)
 	if (err)
 		return err;
 
-	dev_info(&spi->dev, "%s device found, Chip ID:%x, Revision:%x\n",
-		 ks->chip->name, ks->chip->chip_id, ks->revision_id);
+	dev_info(&spi->dev, "KS8995MA device found, Chip ID:%x, Revision:%x\n",
+		 KS8995_CHIP_ID, ks->revision_id);
 
 	err = ks8995_check_config(ks);
 	if (err)

-- 
2.53.0

[PATCH net v3 4/5] net: dsa: ks8995: Add stub bridge join/leave

[Linus Walleij]

Implementing ks8995_port_pre_bridge_flags() and
ks8995_port_bridge_flags() without port_bridge_join()
is a no-op.

This adds stubs for bridge join/leave callbacks following
the pattern of drivers/net/dsa/microchip/ksz_common.c:
as we have STP callbacks and these will be called right
after bridge join/leave these will take care of the
job of setting up the learning which is all we support.

Fixes: a7fe8b266f65 ("net: dsa: ks8995: Add basic switch set-up")
Reported-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/dsa/ks8995.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/dsa/ks8995.c b/drivers/net/dsa/ks8995.c
index b7c9de39f68e..1ed81a8f7d21 100644
--- a/drivers/net/dsa/ks8995.c
+++ b/drivers/net/dsa/ks8995.c
@@ -461,6 +461,26 @@ static void ks8995_port_disable(struct dsa_switch *ds, int port)
 	dev_dbg(ks->dev, "disable port %d\n", port);
 }
 
+static int ks8995_port_bridge_join(struct dsa_switch *ds, int port,
+				   struct dsa_bridge bridge,
+				   bool *tx_fwd_offload,
+				   struct netlink_ext_ack *extack)
+{
+	/* port_stp_state_set() will be called after to put the port in
+	 * appropriate state so there is no need to do anything.
+	 */
+
+	return 0;
+}
+
+static void ks8995_port_bridge_leave(struct dsa_switch *ds, int port,
+				     struct dsa_bridge bridge)
+{
+	/* port_stp_state_set() will be called after to put the port in
+	 * forwarding state so there is no need to do anything.
+	 */
+}
+
 static int ks8995_port_pre_bridge_flags(struct dsa_switch *ds, int port,
 					struct switchdev_brport_flags flags,
 					struct netlink_ext_ack *extack)
@@ -635,6 +655,8 @@ static int ks8995_get_max_mtu(struct dsa_switch *ds, int port)
 static const struct dsa_switch_ops ks8995_ds_ops = {
 	.get_tag_protocol = ks8995_get_tag_protocol,
 	.setup = ks8995_setup,
+	.port_bridge_join = ks8995_port_bridge_join,
+	.port_bridge_leave = ks8995_port_bridge_leave,
 	.port_pre_bridge_flags = ks8995_port_pre_bridge_flags,
 	.port_bridge_flags = ks8995_port_bridge_flags,
 	.port_enable = ks8995_port_enable,

-- 
2.53.0

[PATCH net v3 5/5] net: dsa: ks8995: Implement port isolation

[Linus Walleij]

It is unsound to not have proper port isolation on a
switch which supports it.

Set each port as isolated by default in the setup callback
and de-isolate and isolate the ports in the bridge join/leave
callbacks.

Fixes: a7fe8b266f65 ("net: dsa: ks8995: Add basic switch set-up")
Reported-by: Vladimir Oltean <olteanv@gmail.com>
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/net/dsa/ks8995.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 129 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/ks8995.c b/drivers/net/dsa/ks8995.c
index 1ed81a8f7d21..dbae218fb015 100644
--- a/drivers/net/dsa/ks8995.c
+++ b/drivers/net/dsa/ks8995.c
@@ -80,6 +80,11 @@
 #define KS8995_PC0_TAG_REM	BIT(1)	/* Enable tag removal on port */
 #define KS8995_PC0_PRIO_EN	BIT(0)	/* Enable priority handling */
 
+#define KS8995_PC1_SNIFF_PORT	BIT(7)	/* This port is a sniffer port */
+#define KS8995_PC1_RCV_SNIFF	BIT(6)	/* Packets received goes to sniffer port(s) */
+#define KS8995_PC1_XMIT_SNIFF	BIT(5)	/* Packets transmitted goes to sniffer port(s) */
+#define KS8995_PC1_PORT_VLAN	GENMASK(4, 0)	/* Port isolation mask */
+
 #define KS8995_PC2_TXEN		BIT(2)	/* Enable TX on port */
 #define KS8995_PC2_RXEN		BIT(1)	/* Enable RX on port */
 #define KS8995_PC2_LEARN_DIS	BIT(0)	/* Disable learning on port */
@@ -441,6 +446,44 @@ dsa_tag_protocol ks8995_get_tag_protocol(struct dsa_switch *ds,
 
 static int ks8995_setup(struct dsa_switch *ds)
 {
+	struct ks8995_switch *ks = ds->priv;
+	int ret;
+	u8 val;
+	int i;
+
+	/* Isolate all user ports so they can only send packets to itself and the CPU port */
+	for (i = 0; i < KS8995_CPU_PORT; i++) {
+		ret = ks8995_read_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), &val);
+		if (ret) {
+			dev_err(ks->dev, "failed to read KS8995_REG_PC1 on port %d\n", i);
+			return ret;
+		}
+
+		val &= ~KS8995_PC1_PORT_VLAN;
+		val |= (BIT(i) | BIT(KS8995_CPU_PORT));
+
+		ret = ks8995_write_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), val);
+		if (ret) {
+			dev_err(ks->dev, "failed to write KS8995_REG_PC1 on port %d\n", i);
+			return ret;
+		}
+	}
+
+	/* The CPU port should be able to talk to all ports */
+	ret = ks8995_read_reg(ks, KS8995_REG_PC(KS8995_CPU_PORT, KS8995_REG_PC1), &val);
+	if (ret) {
+		dev_err(ks->dev, "failed to read KS8995_REG_PC1 on CPU port\n");
+		return ret;
+	}
+
+	val |= KS8995_PC1_PORT_VLAN;
+
+	ret = ks8995_write_reg(ks, KS8995_REG_PC(KS8995_CPU_PORT, KS8995_REG_PC1), val);
+	if (ret) {
+		dev_err(ks->dev, "failed to write KS8995_REG_PC1 on CPU port\n");
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -466,8 +509,44 @@ static int ks8995_port_bridge_join(struct dsa_switch *ds, int port,
 				   bool *tx_fwd_offload,
 				   struct netlink_ext_ack *extack)
 {
+	struct ks8995_switch *ks = ds->priv;
+	u8 port_bitmap = 0;
+	int ret;
+	u8 val;
+	int i;
+
+	/* De-isolate this port from any other port on the bridge */
+	port_bitmap |= BIT(port);
+	for (i = 0; i < KS8995_CPU_PORT; i++) {
+		if (i == port)
+			continue;
+		if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
+			continue;
+		port_bitmap |= BIT(i);
+	}
+
+	/* Update all affected ports with the new bitmask */
+	for (i = 0; i < KS8995_CPU_PORT; i++) {
+		if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
+			continue;
+
+		ret = ks8995_read_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), &val);
+		if (ret) {
+			dev_err(ks->dev, "failed to read KS8995_REG_PC1 on port %d\n", i);
+			return ret;
+		}
+
+		val |= port_bitmap;
+
+		ret = ks8995_write_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), val);
+		if (ret) {
+			dev_err(ks->dev, "failed to write KS8995_REG_PC1 on port %d\n", i);
+			return ret;
+		}
+	}
+
 	/* port_stp_state_set() will be called after to put the port in
-	 * appropriate state so there is no need to do anything.
+	 * appropriate state.
 	 */
 
 	return 0;
@@ -476,8 +555,56 @@ static int ks8995_port_bridge_join(struct dsa_switch *ds, int port,
 static void ks8995_port_bridge_leave(struct dsa_switch *ds, int port,
 				     struct dsa_bridge bridge)
 {
+	struct ks8995_switch *ks = ds->priv;
+	u8 port_bitmap = 0;
+	int ret;
+	u8 val;
+	int i;
+
+	/* Isolate this port from any other port on the bridge */
+	for (i = 0; i < KS8995_CPU_PORT; i++) {
+		/* Current port handled last */
+		if (i == port)
+			continue;
+		/* Not on this bridge */
+		if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
+			continue;
+
+		ret = ks8995_read_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), &val);
+		if (ret) {
+			dev_err(ks->dev, "failed to read KS8995_REG_PC1 on port %d\n", i);
+			return;
+		}
+
+		val &= ~BIT(port);
+
+		ret = ks8995_write_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), val);
+		if (ret) {
+			dev_err(ks->dev, "failed to write KS8995_REG_PC1 on port %d\n", i);
+			return;
+		}
+
+		/* Accumulate this port for access by current */
+		port_bitmap |= BIT(i);
+	}
+
+	/* Isolate this port from all other ports formerly on the bridge */
+	ret = ks8995_read_reg(ks, KS8995_REG_PC(port, KS8995_REG_PC1), &val);
+	if (ret) {
+		dev_err(ks->dev, "failed to read KS8995_REG_PC1 on port %d\n", port);
+		return;
+	}
+
+	val &= ~port_bitmap;
+
+	ret = ks8995_write_reg(ks, KS8995_REG_PC(port, KS8995_REG_PC1), val);
+	if (ret) {
+		dev_err(ks->dev, "failed to write KS8995_REG_PC1 on port %d\n", port);
+		return;
+	}
+
 	/* port_stp_state_set() will be called after to put the port in
-	 * forwarding state so there is no need to do anything.
+	 * forwarding state.
 	 */
 }
 

-- 
2.53.0

Re: [PATCH net v3 5/5] net: dsa: ks8995: Implement port isolation

[Vladimir Oltean]

Hi Linus,

Some excerpts from AI review which I considered relevant.

On Thu, Feb 19, 2026 at 03:24:21PM +0100, Linus Walleij wrote:
> It is unsound to not have proper port isolation on a
> switch which supports it.
> 
> Set each port as isolated by default in the setup callback
> and de-isolate and isolate the ports in the bridge join/leave
> callbacks.
> 
> Fixes: a7fe8b266f65 ("net: dsa: ks8995: Add basic switch set-up")
> Reported-by: Vladimir Oltean <olteanv@gmail.com>
> Signed-off-by: Linus Walleij <linusw@kernel.org>
> ---
>  drivers/net/dsa/ks8995.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 129 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/ks8995.c b/drivers/net/dsa/ks8995.c
> index 1ed81a8f7d21..dbae218fb015 100644
> --- a/drivers/net/dsa/ks8995.c
> +++ b/drivers/net/dsa/ks8995.c
> @@ -80,6 +80,11 @@
>  #define KS8995_PC0_TAG_REM	BIT(1)	/* Enable tag removal on port */
>  #define KS8995_PC0_PRIO_EN	BIT(0)	/* Enable priority handling */
>  
> +#define KS8995_PC1_SNIFF_PORT	BIT(7)	/* This port is a sniffer port */
> +#define KS8995_PC1_RCV_SNIFF	BIT(6)	/* Packets received goes to sniffer port(s) */
> +#define KS8995_PC1_XMIT_SNIFF	BIT(5)	/* Packets transmitted goes to sniffer port(s) */
> +#define KS8995_PC1_PORT_VLAN	GENMASK(4, 0)	/* Port isolation mask */
> +
>  #define KS8995_PC2_TXEN		BIT(2)	/* Enable TX on port */
>  #define KS8995_PC2_RXEN		BIT(1)	/* Enable RX on port */
>  #define KS8995_PC2_LEARN_DIS	BIT(0)	/* Disable learning on port */
> @@ -441,6 +446,44 @@ dsa_tag_protocol ks8995_get_tag_protocol(struct dsa_switch *ds,
>  
>  static int ks8995_setup(struct dsa_switch *ds)
>  {
> +	struct ks8995_switch *ks = ds->priv;
> +	int ret;
> +	u8 val;
> +	int i;
> +
> +	/* Isolate all user ports so they can only send packets to itself and the CPU port */

This isn't a bug, but the comment terminology might be clearer. The use
of "isolation" could be confused with the BR_ISOLATED bridge port flag
(from "man bridge", isolated bridge ports "will be able to communicate
with non-isolated ports only"). Vladimir Oltean recommended avoiding
"isolation" related terminology for user port separation in v2.

Reference: https://lore.kernel.org/netdev/20260119224805.hjvh5xdjfhd6c6kf@skbuf/

[ you walked past most of my review comments on v2 with no pushback as
to why you won't apply them ]

> +	for (i = 0; i < KS8995_CPU_PORT; i++) {
> +		ret = ks8995_read_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), &val);
> +		if (ret) {
> +			dev_err(ks->dev, "failed to read KS8995_REG_PC1 on port %d\n", i);
> +			return ret;
> +		}

Do you need to perform a register read at probe time, or could you just
call ks8995_write_reg() with known good values for the sniff port bits
too? Vladimir Oltean questioned this in v2.

> +
> +		val &= ~KS8995_PC1_PORT_VLAN;
> +		val |= (BIT(i) | BIT(KS8995_CPU_PORT));
> +
> +		ret = ks8995_write_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), val);
> +		if (ret) {
> +			dev_err(ks->dev, "failed to write KS8995_REG_PC1 on port %d\n", i);
> +			return ret;
> +		}
> +	}
> +
> +	/* The CPU port should be able to talk to all ports */
> +	ret = ks8995_read_reg(ks, KS8995_REG_PC(KS8995_CPU_PORT, KS8995_REG_PC1), &val);
> +	if (ret) {
> +		dev_err(ks->dev, "failed to read KS8995_REG_PC1 on CPU port\n");
> +		return ret;
> +	}
> +
> +	val |= KS8995_PC1_PORT_VLAN;

Does this enable hairpinning on the CPU port? Writing
KS8995_PC1_PORT_VLAN (GENMASK(4, 0)) includes KS8995_CPU_PORT (4), which
would enable reflection of forwarded traffic on the CPU port. Vladimir
Oltean raised this concern in v2.

> +
> +	ret = ks8995_write_reg(ks, KS8995_REG_PC(KS8995_CPU_PORT, KS8995_REG_PC1), val);
> +	if (ret) {
> +		dev_err(ks->dev, "failed to write KS8995_REG_PC1 on CPU port\n");
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -466,8 +509,44 @@ static int ks8995_port_bridge_join(struct dsa_switch *ds, int port,
>  				   bool *tx_fwd_offload,
>  				   struct netlink_ext_ack *extack)
>  {
> +	struct ks8995_switch *ks = ds->priv;
> +	u8 port_bitmap = 0;
> +	int ret;
> +	u8 val;
> +	int i;
> +
> +	/* De-isolate this port from any other port on the bridge */
> +	port_bitmap |= BIT(port);
> +	for (i = 0; i < KS8995_CPU_PORT; i++) {
> +		if (i == port)
> +			continue;
> +		if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
> +			continue;
> +		port_bitmap |= BIT(i);
> +	}
> +
> +	/* Update all affected ports with the new bitmask */
> +	for (i = 0; i < KS8995_CPU_PORT; i++) {
> +		if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
> +			continue;
> +
> +		ret = ks8995_read_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), &val);
> +		if (ret) {
> +			dev_err(ks->dev, "failed to read KS8995_REG_PC1 on port %d\n", i);
> +			return ret;
> +		}
> +
> +		val |= port_bitmap;

Can this create a hairpinning problem? When a new port joins a bridge,
the existing ports start enabling forwarding to themselves as well since
dsa_port_offloads_bridge() returns true for the currently joining port,
and the port_bitmap written to port i contains BIT(i) set. Vladimir
Oltean raised this concern in v2.

> +
> +		ret = ks8995_write_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), val);
> +		if (ret) {
> +			dev_err(ks->dev, "failed to write KS8995_REG_PC1 on port %d\n", i);
> +			return ret;
> +		}
> +	}
> +
>  	/* port_stp_state_set() will be called after to put the port in
> -	 * appropriate state so there is no need to do anything.
> +	 * appropriate state.
>  	 */
>  
>  	return 0;
> @@ -476,8 +555,56 @@ static int ks8995_port_bridge_join(struct dsa_switch *ds, int port,
>  static void ks8995_port_bridge_leave(struct dsa_switch *ds, int port,
>  				     struct dsa_bridge bridge)
>  {
> +	struct ks8995_switch *ks = ds->priv;
> +	u8 port_bitmap = 0;
> +	int ret;
> +	u8 val;
> +	int i;
> +
> +	/* Isolate this port from any other port on the bridge */
> +	for (i = 0; i < KS8995_CPU_PORT; i++) {
> +		/* Current port handled last */
> +		if (i == port)
> +			continue;
> +		/* Not on this bridge */
> +		if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge))
> +			continue;
> +
> +		ret = ks8995_read_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), &val);
> +		if (ret) {
> +			dev_err(ks->dev, "failed to read KS8995_REG_PC1 on port %d\n", i);
> +			return;
> +		}
> +
> +		val &= ~BIT(port);
> +
> +		ret = ks8995_write_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), val);
> +		if (ret) {
> +			dev_err(ks->dev, "failed to write KS8995_REG_PC1 on port %d\n", i);
> +			return;
> +		}
> +
> +		/* Accumulate this port for access by current */
> +		port_bitmap |= BIT(i);
> +	}
> +
> +	/* Isolate this port from all other ports formerly on the bridge */
> +	ret = ks8995_read_reg(ks, KS8995_REG_PC(port, KS8995_REG_PC1), &val);
> +	if (ret) {
> +		dev_err(ks->dev, "failed to read KS8995_REG_PC1 on port %d\n", port);
> +		return;
> +	}
> +
> +	val &= ~port_bitmap;
> +
> +	ret = ks8995_write_reg(ks, KS8995_REG_PC(port, KS8995_REG_PC1), val);
> +	if (ret) {
> +		dev_err(ks->dev, "failed to write KS8995_REG_PC1 on port %d\n", port);
> +		return;
> +	}
> +
>  	/* port_stp_state_set() will be called after to put the port in
> -	 * forwarding state so there is no need to do anything.
> +	 * forwarding state.
>  	 */
>  }
>  
> 
> -- 
> 2.53.0
>