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

[Vladimir Oltean <olteanv@gmail.com>]

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
>