Re: [PATCH net-next v4 6/8] net: dsa: motorcomm: Rely on port isolation for conduit port

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

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

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 inverts the isolation logic in
  yt921x_userport_standalone()…
- [High] This patch makes the CPU port's PORTn_ISOLATION register the
  only hardware guard against untagged conduit traffic.
- [Low] yt921x_isolate() (drivers/net/dsa/motorcomm/chip.c:1225-1227)
  skips with `if (dsa_is_cpu_port(&priv->ds, port) || i == port)…

--

net: dsa: motorcomm: Rely on port isolation for conduit port

This patch takes the ~0 isolation that yt921x_chip_reset() writes to the
conduit port and makes it the only drop mechanism for untagged traffic
sent on the conduit. To do that it removes the explicit CPU port
isolation rewrite from yt921x_port_setup(), drops the ACT_UNK drop action
for CPU ports, and takes CPU ports out of FILTER_UNK_UCAST/MCAST.

[ ... ]

[Severity: High]
Is this condition inverted? The comment says conduit ports are skipped so
they keep the isolation from yt921x_chip_reset(). With
if (dsa_is_cpu_port(ds, port)), though, only CPU ports get written with
~dsa_cpu_ports(ds), and every user port is skipped.

At probe, yt921x_chip_reset() writes ~0 to every PORTn_ISOLATION register.
yt921x_port_setup()->yt921x_userport_standalone() then runs for each port:

  - CPU ports are overwritten with ~dsa_cpu_ports(). That clears their
    block bits toward themselves and toward other CPU ports. This patch
    also removes the ~0 rewrite from yt921x_port_setup(), so nothing
    restores the one-way isolation.

  - User ports are never written and stay at ~0. That also blocks them
    toward the CPU port.

Unknown unicast and multicast are still trapped. However, the driver notes
in yt921x_bridge_flags() that broadcast can be filtered but not trapped,
and isolation is applied after the FDB lookup. Would broadcast frames
received on standalone user ports, such as ARP requests or DHCP, be
dropped before they reach the host?

This patch also removes the ACT_UNK drop action and the FILTER_UNK
coverage for CPU ports in yt921x_chip_setup_dsa(). The CPU port isolation
is now the only guard for untagged conduit traffic, and it is no longer
~0. In multi-CPU setups, could untagged frames from one conduit be
forwarded to another CPU port, or hairpinned back?

yt921x_bridge_leave() also relies on this function to reset the
isolation register of the leaving port:

yt921x_bridge_leave()
    yt921x_userport_standalone(priv, port);
    yt921x_isolate(priv, port);

That reset no longer happens for user ports. This is masked for now
because yt921x_bridge() never programs user port registers (see below).
Once that is fixed, wouldn't a port leaving a bridge keep its bridge-time
value? It could then still forward to its former peers, because
yt921x_isolate() only blocks the peer->port direction.

The condition is unchanged at the end of the series, in "net: dsa:
motorcomm: Support BR_FLOOD and BR_MCAST_FLOOD offloading". Was the
intended test if (!dsa_is_cpu_port(ds, port))?

 
 	/* Turn off FDB learning to prevent FDB pollution */
 	mask = YT921X_PORT_LEARN_DIS;

[ ... ]

[Severity: High]
This removes the drop action for CPU ports, and the hunk above takes CPU
ports out of FILTER_UNK_UCAST/MCAST. Both changes assume the conduit keeps
the ~0 isolation from yt921x_chip_reset(). Does that still hold after a
bridge join?

yt921x_bridge() has:

static int yt921x_bridge(struct yt921x_priv *priv, u16 ports_mask)
{
	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) {
		...
		/* to non-bridge ports */
		ctrl = ~ports_mask;
		...
		res = yt921x_reg_write(priv, YT921X_PORTn_ISOLATION(port),
				       ctrl);

In the baseline this mask was ports_mask & ~priv->cpu_ports_mask, which
matches the "Block from non-cpu bridge ports" comment. The ~ was lost in
the earlier "net: dsa: motorcomm: Remove local cpu_ports_mask" commit in
this series.

Both yt921x_dsa_port_bridge_join() and yt921x_bridge_flags() (on
BR_HAIRPIN_MODE or BR_ISOLATED changes) OR dsa_cpu_ports() into
ports_mask. The loop therefore rewrites only the CPU port register, with
~ports_mask | BIT(cpu), which unblocks the conduit toward every bridge
member.

With the drop action and FILTER_UNK coverage gone, can untagged
broadcast and unknown unicast/multicast sent on the conduit now reach the
bridged user ports without going through the DSA tag? IPv6 link-local
NDP/MLD/RS that the host sends on the conduit netdev would be one source
of such frames.

The isolation registers of the bridged user ports themselves are never
programmed, so they stay at ~0. Would learned unicast between bridge
members then be dropped by isolation after the FDB lookup? And would
broadcast from bridged ports be unable to reach the CPU?

This mask is also unchanged at the end of the series, in "net: dsa:
motorcomm: Support BR_FLOOD and BR_MCAST_FLOOD offloading".

[Severity: Low]
On the same subject of CPU port isolation registers, yt921x_isolate() has:

	for (int i = 0; i < YT921X_PORT_NUM; i++) {
		if (dsa_is_cpu_port(&priv->ds, port) || i == port)
			continue;

Should this test i instead of port? The baseline was
(BIT(i) & priv->cpu_ports_mask) || i == port.

yt921x_dsa_port_bridge_leave() returns early for CPU ports, so port is
always a user port here. As a result the CPU port registers are no longer
skipped, and BIT(port) gets set in them. Those are the registers this
patch says should stay as yt921x_chip_reset() left them. The extra bit
moves the conduit toward ~0, so there is no visible forwarding regression.

The wrong variable came in with "net: dsa: motorcomm: Remove local
cpu_ports_mask" and is still there at the end of the series.

 	res = yt921x_reg_write(priv, YT921X_ACT_UNK_UCAST, ctrl);
 	if (res)
 		return res;

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