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

[Wei Fang]

There are some comments from Sashiko and see my response inline.

> +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 risk corrupting the pseudo-MAC (PMAC) configuration?

By reading the original register value exclusively from the primary MAC
(MAC 0) and writing the resulting new value to both MAC 0 and the PMAC,
any independent configuration stored in the unmasked bits of the PMAC could
be blindly overwritten.

Additionally, if the new value matches the old value for MAC 0, the function
returns early. Could this leave the PMAC permanently out of sync if its state
needed updating but MAC 0 did not?

The pmac means preemption MAC not the pseudo MAC, I will add a comment
to the pmac definition. The purpose of netc_mac_port_rmw() is to make
pMAC and eMAC configurations the same. So this is a false positive.

[...]

> +static void netc_port_mac_tx_graceful_stop(struct netc_port *np) {
> +	netc_port_rmw(np, NETC_POR, POR_TXDIS, POR_TXDIS);
> +
> +	if (is_netc_pseudo_port(np))
> +		return;
> +
> +	netc_port_wait_tx_empty(np, 0);
> +	if (np->caps.pmac)
> +		netc_port_wait_tx_empty(np, 1);
> +
> +	netc_mac_port_rmw(np, NETC_PM_CMD_CFG(0), PM_CMD_CFG_TX_EN,
> 0); }

Can this sequence race with the Port actively pushing the remainder of a packet
to the MAC?

The code disables the Port TX (POR_TXDIS) and then immediately waits for the
MAC TX FIFO to empty. However, without polling the PSR_TX_BUSY bit in NETC_PSR
to ensure the port has finished its work (similar to how PSR_RX_BUSY is checked
in netc_port_mac_rx_graceful_stop), it appears the MAC TX FIFO could
momentarily empty while the port is still pushing data.

Would this lead to prematurely disabling the MAC TX (PM_CMD_CFG_TX_EN = 0) and
potentially truncating egress packets or leaving the hardware in an invalid
state?

This is also a false positive, the procedures for configuring graceful stop for TX
and RX are different, and they are all configured in accordance with the NETC
reference manual.