Re: [PATCH net-next v4 3/8] net: dsa: motorcomm: Reset port flags on bridge leaving

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] This patch resets pp->isolated and pp->hairpin in
  yt921x_userport_standalone() when a port leaves a bridge, but nothing…
- [Low] The patch fixes a user-visible forwarding bug: a port that
  rejoins a bridge is programmed with stale hairpin/isolation state in…
- [Low] The new comment in the shared helper yt921x_userport_standalone()
  says "The return value got ignored by DSA core".

--

net: dsa: motorcomm: Reset port flags on bridge leaving

This patch clears the cached pp->hairpin and pp->isolated flags in
yt921x_userport_standalone() when a port leaves a bridge. The goal is to
keep stale BR_HAIRPIN_MODE and BR_ISOLATED state from being programmed
on the next bridge join.

DSA does not clear BR_HAIRPIN_MODE and BR_ISOLATED when a port leaves
a bridge, so the cached flags survive into the next bridge join and
program stale hairpin and port isolation state.

Reset them when leaving the bridge.

Signed-off-by: David Yang <mmyangfl@gmail.com>

[Severity: Low]
Should this have a Fixes: tag? The caching of pp->hairpin and
pp->isolated in yt921x_bridge_flags(), with no reset on leave, comes
from the original driver commit:

Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")

[Severity: Low]
This isn't a bug, but is this comment accurate for both callers of
yt921x_userport_standalone()?

It holds for yt921x_bridge_leave(), which is reached from the void
.port_bridge_leave op. yt921x_port_setup() also calls this helper and
returns its error:

yt921x_port_setup() {
    ...
	res = yt921x_userport_standalone(priv, port);
	if (res)
		return res;
    ...
}

That error goes back through .port_setup, and dsa_port_devlink_setup()
in net/dsa/devlink.c propagates it:

	if (ds->ops->port_setup) {
		err = ds->ops->port_setup(ds, dp->index);
		if (err)
			return err;

Could the reset and this comment go into yt921x_bridge_leave() instead?

+	pp->hairpin = false;
+	pp->isolated = false;

[Severity: High]
Now that the cached flags are cleared on leave, does anything restore
them on the next join?

dsa_port_inherit_brport_flags() in net/dsa/port.c only replays this
set:

	const unsigned long mask = BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
				   BR_BCAST_FLOOD | BR_PORT_LOCKED;

So BR_ISOLATED and BR_HAIRPIN_MODE on the bridge port never reach the
driver on join. The bridge doesn't replay brport flags either.
nbp_switchdev_sync_objs() only replays VLAN, MDB and FDB objects.

This driver implements .port_lag_join. Take a bond that is a bridge
port and already has "bridge link set dev bond0 isolated on" (or
"hairpin on") applied, so yt921x_bridge_flags() has cached the flag on
each yt921x member.

Then a member leaves the bond and rejoins, or a new member is added:

dsa_port_lag_leave()
  dsa_port_bridge_leave()
    yt921x_dsa_port_bridge_leave()
      yt921x_bridge_leave()
        yt921x_userport_standalone()
          pp->hairpin = false;
          pp->isolated = false;

dsa_port_lag_join()
  dsa_port_bridge_join()
    yt921x_dsa_port_bridge_join()
      yt921x_bridge_join()
        yt921x_bridge()

yt921x_bridge() builds YT921X_PORTn_ISOLATION(port) only from the
cache:

		ctrl = ~ports_mask;
		if (pp->isolated)
			ctrl |= isolated_mask;
		if (!pp->hairpin)
			ctrl |= BIT(port);

Wouldn't this leave the port without isolation and hairpin in hardware?
It would also be dropped from isolated_mask for the other isolated
ports. Meanwhile "bridge link show" still reports bond0 as isolated, and
the hardware stays wrong until the user toggles the flag again.

Before this patch, the stale cache happened to keep a rejoining member
correct, so the rejoin case looks like a regression. The commit message
says the aim is correct hairpin and isolation state on the next bridge
join, and this LAG path still doesn't get there.

Would it be better to sync these flags on join? One way is to add
BR_HAIRPIN_MODE | BR_ISOLATED to the inherit and clear masks in
net/dsa/port.c, which would make the driver-local reset unnecessary.
Another is to read br_port_flag_is_set() on the bridge port in the
driver's bridge join path.

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