Re: [PATCH net-next v4 5/8] net: dsa: motorcomm: Isolate all ports during chip setup

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] yt921x_bridge() in drivers/net/dsa/motorcomm/chip.c computes
  `targets_mask = ports_mask & dsa_cpu_ports(&priv->ds)`.
- [Low] yt921x_isolate() in drivers/net/dsa/motorcomm/chip.c loops over
  `i` but checks `dsa_is_cpu_port(&priv->ds, port)` instead of…
- [Low] The commit message says every port is isolated "right after the
  hardware reset", and the new comment says "Block all traffic as soon…

--

net: dsa: motorcomm: Isolate all ports during chip setup

This patch writes ~0 to the PORTn_ISOLATION register of every port at the
end of yt921x_chip_reset(). All traffic is then blocked once the hardware
reset has turned the chip back into a dumb switch. It prepares for the
isolation refactoring later in the series.

> diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c
> index 8ab499611fbf7..32701f9e687fa 100644
> --- a/drivers/net/dsa/motorcomm/chip.c
> +++ b/drivers/net/dsa/motorcomm/chip.c

[ ... ]

> @@ -2264,6 +2265,17 @@ static int yt921x_chip_reset(struct yt921x_priv *priv)
>  		return -EINVAL;
>  	}
>  
> +	/* Block all traffic as soon as we reset the hardware since it now acts
> +	 * as a dumb switch.
> +	 */
> +	for (int port = 0; port < YT921X_PORT_NUM; port++) {

[Severity: Low]
The commit message says ports are isolated "right after the hardware
reset". The new comment says "as soon as we reset the hardware". In
yt921x_chip_reset(), though, this loop runs after the TPID check:

	res = yt921x_reg_read(priv, YT921X_CPU_TAG_TPID, &val);
	if (res)
		return res;
	eth_p_tag = FIELD_GET(YT921X_CPU_TAG_TPID_TPID_M, val);
	if (eth_p_tag != ETH_P_YT921X) {
		...
		return -EINVAL;
	}

The TPID read can fail, or the value can differ from ETH_P_YT921X. In
either case the function returns after YT921X_RST_HW has already reset
the chip, and no PORTn_ISOLATION register is written.

yt921x_dsa_setup() passes the error up, setup fails, and .port_setup
never runs. The switch keeps forwarding between all ports, including the
conduit, and there are no netdevs to manage it.

The behaviour here is the same as before the patch, so this isn't a
regression. The TPID check doesn't depend on isolation, though.

Could the loop move to directly after the yt921x_reg_wait() on
YT921X_RST? That would cover this error path. It would also make the
window on the success path one bus transaction shorter.

> +		ctrl = ~(u32)0;
> +		res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port),
> +				       ctrl);
> +		if (res)
> +			return res;
> +	}

[Severity: High]
This code isn't part of this diff, but does yt921x_bridge() undo this
isolation for the CPU port as soon as a port joins a bridge?

Earlier in the series, "net: dsa: motorcomm: Remove local cpu_ports_mask"
changed the mask computation in yt921x_bridge() to this:

drivers/net/dsa/motorcomm/chip.c:yt921x_bridge() {
	unsigned long targets_mask = ports_mask & dsa_cpu_ports(&priv->ds);
	...
	/* Block from non-cpu bridge ports ... */
	for_each_set_bit(port, &targets_mask, YT921X_PORT_NUM) {
		...
		ctrl = ~ports_mask;
		...
		res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port),
				       ctrl);
	...
}

Before that commit it was ports_mask & ~priv->cpu_ports_mask. Is a '~'
missing in front of dsa_cpu_ports()?

Both yt921x_bridge_join() and yt921x_bridge_flags() OR the CPU ports into
ports_mask. That makes targets_mask exactly the CPU port mask, so both
loops visit only the CPU port.

There are two effects. First, the CPU port's PORTn_ISOLATION is replaced
with ~ports_mask | BIT(cpu). This removes the one-way ~0 isolation set
here and in yt921x_port_setup(). Untagged frames from the conduit can
then be forwarded to bridge members.

Second, the user bridge ports are never reprogrammed. They keep the
~dsa_cpu_ports() value written by yt921x_userport_standalone().
Hardware forwarding between bridge members is never enabled, and
pp->isolated and pp->hairpin are never applied to these ports.

yt921x_userport_bridge() clears LEARN_DIS on join, so the switch learns
MACs. Known unicast is then sent toward the peer port, where the source
port's isolation blocks it.

The tagger only sets offload_fwd_mark for frames the hardware has already
forwarded. So the software bridge doesn't reliably recover these frames
either.

Running "ip link set lanX master br0" on any YT921x switch takes this
path:

  yt921x_dsa_port_bridge_join()
    yt921x_bridge_join()
      yt921x_bridge()

Bridge flag changes reach the same code through yt921x_bridge_flags().
The line is still unchanged at the end of the series.

[Severity: Low]
This one also comes from "net: dsa: motorcomm: Remove local
cpu_ports_mask". Is the CPU port check in yt921x_isolate() testing the
wrong loop variable?

drivers/net/dsa/motorcomm/chip.c:yt921x_isolate() {
	...
	for (int i = 0; i < YT921X_PORT_NUM; i++) {
		if (dsa_is_cpu_port(&priv->ds, port) || i == port)
			continue;
	...
}

The original check was BIT(i) & priv->cpu_ports_mask, so this should
probably be dsa_is_cpu_port(&priv->ds, i). As written, the result is the
same on every iteration.

yt921x_dsa_port_bridge_leave() returns early for CPU ports, so the
condition is always false. The CPU port's PORTn_ISOLATION then gets
BIT(port) set on every bridge leave.

By itself, that matches the ~0 policy this patch sets up. Combined with
the yt921x_bridge() problem above, the CPU port's isolation state ends up
depending on the order of bridge joins and leaves. This check is also
unchanged at the end of the series.

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107121631.1040612-1-mmyangfl%40gmail.com