Re: [PATCH net-next v4 4/8] net: dsa: motorcomm: Remove local cpu_ports_mask

[netdev-bot+sashiko@kernel.org]

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

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

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] This commit was meant to be a pure refactor, but it reverses the
  target mask in yt921x_bridge() (drivers/net/dsa/motorcomm/chip.c).
- [Medium] In yt921x_isolate() (drivers/net/dsa/motorcomm/chip.c), the
  per-iteration check `(BIT(i) & priv->cpu_ports_mask)` was replaced…

--

net: dsa: motorcomm: Remove local cpu_ports_mask

This commit drops the cached cpu_ports_mask field from struct yt921x_priv.
Its users now call dsa_cpu_ports() or dsa_is_cpu_port() instead. The
commit describes this as a readability change.

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

[ ... ]

> @@ -1215,7 +1216,7 @@ static int yt921x_isolate(struct yt921x_priv *priv, int port)
>  
>  	mask = BIT(port);
>  	for (int i = 0; i < YT921X_PORT_NUM; i++) {
> -		if ((BIT(i) & priv->cpu_ports_mask) || i == port)
> +		if (dsa_is_cpu_port(&priv->ds, port) || i == port)
>  			continue;

[Severity: Medium]
Should this be dsa_is_cpu_port(&priv->ds, i) instead of port?

The old check tested the loop index i against the CPU port mask. The new
check tests the function argument port, which is the same on every
iteration.

The only caller path is:

yt921x_dsa_port_bridge_leave()
  yt921x_bridge_leave()
    yt921x_isolate()

yt921x_dsa_port_bridge_leave() already returns early for CPU ports:

	if (dsa_is_cpu_port(ds, port))
		return;

So the new condition is always false, and the loop no longer skips CPU
ports. On every bridge leave, BIT(port) now gets set in
YT921X_PORTn_ISOLATION() of each CPU port. The original code left those
registers alone, because port setup programs them.

If yt921x_isolate() were ever called with a CPU port, the condition would
be true on every iteration and no port would be updated.

>  
>  		res = yt921x_reg_set_bits(priv, YT921X_PORTn_ISOLATION(i),
> @@ -1232,7 +1233,7 @@ static int yt921x_isolate(struct yt921x_priv *priv, int port)
>   */
>  static int yt921x_bridge(struct yt921x_priv *priv, u16 ports_mask)
>  {
> -	unsigned long targets_mask = ports_mask & ~priv->cpu_ports_mask;
> +	unsigned long targets_mask = ports_mask & dsa_cpu_ports(&priv->ds);

[Severity: High]
Is a ~ missing here? The old code selected the bridge members that are
not CPU ports. The new code selects only the CPU ports.

Both callers add the CPU ports to ports_mask before calling
yt921x_bridge():

yt921x_dsa_port_bridge_join():
	ports_mask = dsa_bridge_ports(ds, bridge.dev);
	ports_mask |= dsa_cpu_ports(ds);

yt921x_bridge_flags():
	ports_mask = dsa_bridge_ports(ds, bdev);
	ports_mask |= dsa_cpu_ports(ds);

So targets_mask always ends up equal to dsa_cpu_ports(ds). The loops under
the "Block from non-cpu bridge ports" comment then write only the CPU
ports' YT921X_PORTn_ISOLATION() registers.

This has three effects:

The ~(u32)0 isolation value that yt921x_port_setup() installs on the CPU
port is overwritten with ~ports_mask | BIT(cpu).

The user ports in the bridge never have their isolation registers
rewritten. They keep the standalone value ~dsa_cpu_ports(ds) from
yt921x_userport_standalone(), so they stay blocked from each other.

BR_HAIRPIN_MODE and BR_ISOLATED updates from yt921x_bridge_flags() go to
the CPU port, not the user ports.

yt921x_userport_bridge() clears YT921X_PORT_LEARN_DIS on join, so the
switch learns FDB entries for bridge members. The comment in
yt921x_chip_setup_dsa() says port isolation is applied after FDB lookups.

Would known-unicast frames between two bridged user ports then be dropped
once their FDB entries are learned?

Should this be ports_mask & ~dsa_cpu_ports(&priv->ds)?

>  	u32 isolated_mask;
>  	u32 ctrl;
>  	int port;

[ ... ]

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