Re: [PATCH net v7] 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] In qca8k_port_change_mtu()
  (drivers/net/dsa/qca/qca8k-common.c), the decision to pause a CPU MAC…

--

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

This patch adds a dedicated mutex that serializes the MTU update with all
PORT_STATUS writers. During the update, the MAC bits of the enabled CPU
ports are saved and restored instead of being forced back on afterwards.
Failed status writes are recorded and retried from a delayed work, and the
committed MAX_FRAME_SIZE is tracked so a duplicate call for the same value
returns early.

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

[ ... ]

> @@ -782,27 +856,59 @@ 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 &= priv->port_enabled_map;
> +	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;

[Severity: Medium]
Can a concurrent register reader corrupt the status[] snapshot here and
make this skip the pause?

qca8k_regmap_config in qca8k-8xxx.c has:

    .disable_locking = true, /* Locking is handled by qca8k read/write */

So regmap_read() goes through _regmap_bus_read() and takes no regmap lock.
The register address is formatted into the shared map->work_buf, and
qca8k_bulk_read() reads it back from there:

    u32 reg = *(u16 *)reg_buf;

The value is parsed from the shared work_val only after the transport has
released mdio_lock:

drivers/base/regmap/regmap.c:_regmap_bus_read() {
    ...
    ret = _regmap_raw_read(map, reg, work_val, map->format.val_bytes, false);
    if (ret == 0)
        *val = map->format.parse_val(work_val);
    ...
}

port_status_lock does not cover readers such as qca8k_pcs_get_state().
Phylink calls it from its resolve worker without RTNL, and it is polled
because qca8k_setup_pcs() sets pcs.poll = true. The LED hw_control_get
path is another reader that takes no lock.

For example, with port 0 enabled and port 6 in in-band mode after a
link_down:

CPU1 (RTNL held)
qca8k_port_change_mtu()
  regmap_read(QCA8K_REG_PORT_STATUS(0))

CPU2
phylink_resolve()
  qca8k_pcs_get_state(port 6)
    qca8k_read(priv, QCA8K_REG_PORT_STATUS(port), &reg)

Suppose port 6's address or value lands in work_buf before port 0's value
is parsed. Then status[0] would have no TXMAC/RXMAC bits, the continue
above would skip port 0, and this write would run with the port 0 MAC
still enabled:

> +		__set_bit(i, &stopped);
> +		ret = regmap_clear_bits(priv->regmap, QCA8K_REG_PORT_STATUS(i),
> +					mask);
> +		if (ret)
> +			goto restore;
> +	}
>  
> -	if (priv->port_enabled_map & BIT(0))
> -		qca8k_port_set_status(priv, 0, 1);
> +	ret = qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, frame_size);

Commit 386228c694bf says this state stops the switch from sending packets
until reset.

The shared work_buf race itself predates this patch. However, the old pause
was an unconditional regmap_clear_bits(), which goes through
qca8k_regmap_update_bits() and does not use work_buf. So it could not be
skipped this way.

The window is small, but should the pause avoid depending on this unlocked
read? One option would be to clear the MAC bits of the enabled CPU ports
unconditionally, as before.

> +	if (!ret)
> +		priv->max_frame_size = frame_size;

[ ... ]

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