Re: [PATCH net v5] net: dsa: qca8k: preserve port state across MTU changes

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [Medium] When qca8k_port_change_mtu()
  (drivers/net/dsa/qca/qca8k-common.c) fails partway, the hardware can…

--

net: dsa: qca8k: preserve port state across MTU changes

This patch adds a port_status_lock mutex. The lock serializes the
MTU update in qca8k_port_change_mtu() with the PORT_STATUS writers,
including the phylink mac_link_up callback. The MTU path now saves the
TXMAC/RXMAC bits of enabled ports 0 and 6 and restores them, where it
used to force them back on. It also returns the first error it hits
during the read, pause, write or restore steps.

> diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
> index 13005f10edb7d..084df92c53c40 100644
> --- a/drivers/net/dsa/qca/qca8k-common.c
> +++ b/drivers/net/dsa/qca/qca8k-common.c

[ ... ]

> @@ -782,27 +805,54 @@ int qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  	if (!dsa_is_cpu_port(ds, port))
>  		return 0;
>  
> -	/* To change the MAX_FRAME_SIZE the cpu ports must be off or
> -	 * the switch panics.
> -	 * Turn off both cpu ports before applying the new value to prevent
> -	 * this.
> +	/* To change MAX_FRAME_SIZE, pause enabled ports 0 and 6.
> +	 * Serialize with phylink and restore the saved MAC state.
>  	 */
> -	if (priv->port_enabled_map & BIT(0))
> -		qca8k_port_set_status(priv, 0, 0);
> +	mutex_lock(&priv->port_status_lock);
> +
> +	ports = (BIT(0) | BIT(6)) & priv->port_enabled_map;
>  
> -	if (priv->port_enabled_map & BIT(6))
> -		qca8k_port_set_status(priv, 6, 0);
> +	for_each_set_bit(i, &ports, QCA8K_NUM_PORTS) {
> +		ret = regmap_read(priv->regmap, QCA8K_REG_PORT_STATUS(i),
> +				  &status[i]);
> +		if (ret)
> +			goto unlock;
> +	}
> +
> +	for_each_set_bit(i, &ports, QCA8K_NUM_PORTS) {
> +		u32 mask = qca8k_port_status_mask(i);
> +
> +		if (!(status[i] & mask))
> +			continue;
> +
> +		__set_bit(i, &stopped);
> +		ret = regmap_clear_bits(priv->regmap, QCA8K_REG_PORT_STATUS(i),
> +					mask);
> +		if (ret)
> +			goto restore;
> +	}
>  
>  	/* Include L2 header / FCS length */
>  	ret = qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, new_mtu +
>  			  ETH_HLEN + ETH_FCS_LEN);
>  
> -	if (priv->port_enabled_map & BIT(0))
> -		qca8k_port_set_status(priv, 0, 1);
> -
> -	if (priv->port_enabled_map & BIT(6))
> -		qca8k_port_set_status(priv, 6, 1);
> +restore:
> +	for_each_set_bit(i, &stopped, QCA8K_NUM_PORTS) {
> +		u32 mask = qca8k_port_status_mask(i);
> +
> +		restore_ret = regmap_update_bits(priv->regmap,
> +						 QCA8K_REG_PORT_STATUS(i),
> +						 mask, status[i] & mask);
> +		if (restore_ret) {
> +			dev_err(priv->dev, "failed to restore MAC state on port %d: %d\n",
> +				i, restore_ret);
> +			if (!ret)
> +				ret = restore_ret;
                                ^^^^

[Severity: Medium]
What happens when the qca8k_write() of QCA8K_MAX_FRAME_SIZE succeeds
but the restore regmap_update_bits() fails for port 0 or 6?

qca8k_regmap_update_bits() can fall back to
qca8k_regmap_update_bits_mii(). That function returns an error if
qca8k_set_page() or qca8k_mii_read32() fails. In that case, the
successful frame-size change is reported to the caller as an error.

The CPU-port call comes from:

dsa_user_change_mtu()
  dsa_port_mtu_change(cpu_dp, cpu_mtu)
    dsa_tree_notify()
      dsa_switch_mtu()
        qca8k_port_change_mtu()

On error, dsa_user_change_mtu() in net/dsa/user.c takes this path:

	err = dsa_port_mtu_change(cpu_dp, cpu_mtu);
	if (err)
		goto out_cpu_failed;
	...
out_cpu_failed:
	if (new_conduit_mtu != old_conduit_mtu)
		dev_set_mtu(conduit, old_conduit_mtu);

That path only reverts the conduit MTU. Does the switch-global
MAX_FRAME_SIZE register then keep the new value, while DSA and the
conduit keep the old MTU? Nothing seems to write the old frame size
back.

The same failure also leaves TXMAC/RXMAC cleared on the CPU port.
port_enabled_map still has the bit set, and phylink still thinks the
link is up. A fixed-link CPU port gets no further mac_link_up(), so
nothing rewrites PORT_STATUS.

Before this patch, the next MTU change called
qca8k_port_set_status(priv, i, 1), which forced the MAC back on and
recovered the port. With this patch, the next MTU change reads
PORT_STATUS, sees the MAC bits as 0, and skips the port:

		if (!(status[i] & mask))
			continue;

Can this leave the CPU port MAC off until a link flap or reboot? This
check cannot tell "phylink turned the MAC off" apart from "an earlier
restore failed".

Two options: keep returning success once MAX_FRAME_SIZE has been
written, or roll MAX_FRAME_SIZE back on failure. Either way, should a
failed restore stop being treated as the saved state on later calls?

> +		}
> +	}
>  
> +unlock:
> +	mutex_unlock(&priv->port_status_lock);
>  	return ret;
>  }

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