RE: [PATCH v6 net-next 11/15] net: dsa: netc: add phylink MAC operations

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to
consider:
- [High] Flawed read-modify-write logic in `netc_mac_port_rmw()` causes state
desynchronization and clobbers PMAC registers.
--

commit e2073763516dc99734e9f1436d4b8e960d8ce680
Author: Wei Fang [off-list ref]

    net: dsa: netc: add phylink MAC operations

This commit implements phylink MAC operations for NETC switch ports,
including
capabilities retrieval and link configuration callbacks.

quoted

diff --git a/drivers/net/dsa/netc/netc_main.c

b/drivers/net/dsa/netc/netc_main.c

quoted

index 8e3a3230226c3..8c1317b72a81d 100644

--- a/drivers/net/dsa/netc/netc_main.c
+++ b/drivers/net/dsa/netc/netc_main.c

[ ... ]

quoted

@@ -43,6 +43,26 @@ static void netc_mac_port_wr(struct netc_port *np,

u32 reg, u32 val)

quoted

 		netc_port_wr(np, reg + NETC_PMAC_OFFSET, val);
 }

+static void netc_mac_port_rmw(struct netc_port *np, u32 reg,
+			      u32 mask, u32 val)
+{
+	u32 old, new;
+
+	if (is_netc_pseudo_port(np))
+		return;
+
+	WARN_ON((mask | val) != mask);
+
+	old = netc_port_rd(np, reg);
+	new = (old & ~mask) | val;
+	if (new == old)
+		return;
+
+	netc_port_wr(np, reg, new);
+	if (np->caps.pmac)
+		netc_port_wr(np, reg + NETC_PMAC_OFFSET, new);

Does this code lead to desynchronized state and clobbered PMAC registers?

Because old is read exclusively from the MAC 0 register, if new == old, the
function returns immediately. If the preemptable MAC 1 register had a
different state (e.g. from the bootloader), it would not be updated and could
remain permanently out of sync.

Additionally, if new != old, this writes the modified MAC 0 state to MAC 1.
Could this clobber any independent state in the MAC 1 register by overwriting
unmasked bits with MAC 0's values?

Would it be safer to perform independent read-modify-write cycles for each
MAC's register to preserve their respective unmasked bits?

The purpose of netc_mac_port_rmw() is to make pMAC and eMAC configurations
the same. This function will not be used if differentiated settings are required. So
please ignore this false positive.