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

[Wei Fang <wei.fang@nxp.com>]

> 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 <wei.fang@nxp.com>
> 
>     net: dsa: netc: add phylink MAC operations
> 
> This commit implements phylink MAC operations for NETC switch ports,
> including
> capabilities retrieval and link configuration callbacks.
> 
> > diff --git a/drivers/net/dsa/netc/netc_main.c
> b/drivers/net/dsa/netc/netc_main.c
> > index 8e3a3230226c3..8c1317b72a81d 100644
> > --- a/drivers/net/dsa/netc/netc_main.c
> > +++ b/drivers/net/dsa/netc/netc_main.c
> [ ... ]
> > @@ -43,6 +43,26 @@ static void netc_mac_port_wr(struct netc_port *np,
> u32 reg, u32 val)
> >  		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.