Re: [PATCH net-next v4 2/3] net: dsa: qca8k: serialize CPU MAC pause during MTU changes

[netdev-bot+sashiko@kernel.org]

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

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

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] The commit message says the new error handling works: it will
  "abort before updating the frame size if ... pausing the MAC fails"…
- [Medium] For internal-PHY CPU ports (1-5), which this patch adds to the
  pause set, qca8k_port_change_mtu() clears only TXMAC|RXMAC (`mask`,…
- [Low] The concern: the commit message treats a CPU port on internal-PHY
  ports 1-5 as a live configuration, though the driver in this tree only…

--

net: dsa: qca8k: serialize CPU MAC pause during MTU changes

This commit adds internal PHY CPU ports 1-5 to the pause sequence that
runs in qca8k_port_change_mtu() before QCA8K_MAX_FRAME_SIZE is written.
It takes reg_mutex to serialize that sequence against port enable, port
disable and phylink link transitions. It also saves each port's
TXMAC/RXMAC bits and restores them afterwards, with new error handling
that aborts and restores on failure.

> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 60f2a615a5cc2..dfe18b6e936bf 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -1483,7 +1483,9 @@ qca8k_phylink_mac_link_up(struct phylink_config *config,
>  
>  	reg |= QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
>  
> +	mutex_lock(&priv->reg_mutex);
>  	qca8k_write(priv, QCA8K_REG_PORT_STATUS(port), reg);
> +	mutex_unlock(&priv->reg_mutex);
>  }

[Severity: Low]
This isn't a bug, but should part of this change carry a Fixes: tag?

qca8k_phylink_mac_link_up() and the link-down callback run from the
phylink resolve work without rtnl. qca8k_port_change_mtu() runs under
rtnl.

Before this patch, a concurrent link-up could set TXMAC/RXMAC on port 0
or 6 after the pause but before the QCA8K_MAX_FRAME_SIZE write. The same
goes for restoring ports 0 and 6 to their previous state instead of
always turning them back on. Both look like fixes for existing behavior.

The commit message also treats a CPU port on internal PHY ports 1-5 as a
configuration that exists today. In this tree, qca8k_find_cpu_port()
only accepts ports 0 or 6. That support arrives later in the series in
"net: dsa: qca8k: support QCA8337 internal PHY CPU links". Could the
message say this part is preparation for that commit?

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

[ ... ]

> @@ -764,14 +774,20 @@ void qca8k_port_disable(struct dsa_switch *ds, int port)

[ ... ]

>  int qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  {
> +	u32 mask = QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
>  	struct qca8k_priv *priv = ds->priv;

[ ... ]

> @@ -784,25 +800,58 @@ int qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  
>  	/* 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.
> +	 * Include internal PHY CPU ports as well as the two MAC-only ports.
> +	 * Toggle only MAC enables, preserving the phylink link-control mode.
>  	 */
> -	if (priv->port_enabled_map & BIT(0))
> -		qca8k_port_set_status(priv, 0, 0);
> +	ports = BIT(0) | BIT(6);
> +	for (i = 1; i < 6; i++)
> +		if (dsa_is_cpu_port(ds, i))
> +			ports |= BIT(i);

[Severity: Medium]
For internal PHY CPU ports 1-5, which are now in the pause set, does
clearing only TXMAC and RXMAC actually stop the MAC?

The driver's existing disable path also clears LINK_AUTO for those
ports:

qca8k_port_set_status_locked() {
	...
	/* Port 0 and 6 have no internal PHY */
	if (port > 0 && port < 6)
		mask |= QCA8K_PORT_STATUS_LINK_AUTO;
	...
}

If LINK_AUTO (LINK_EN) stays set, the MAC may keep following the
internal PHY's link state. It could then keep passing traffic while
QCA8K_MAX_FRAME_SIZE is rewritten.

One case: qca8k_port_enable() sets TXMAC|RXMAC|LINK_AUTO on the CPU port
during setup. Then the MTU is changed while the user port is created,
possibly before phylink's mac_link_up has rewritten the register:

dsa_user_create()
  dsa_user_change_mtu(user_dev, ETH_DATA_LEN)
    dsa_port_mtu_change(cpu_dp)
      qca8k_port_change_mtu()

qca8k_phylink_mac_link_up() also writes LINK_AUTO in in-band mode and in
the default branch for unknown speeds.

Whether LINK_EN overrides cleared TXMAC/RXMAC is hardware behavior that
can't be confirmed from the code. Since status[] already saves the
original bits, could LINK_AUTO be added to the pause mask for ports 1-5
and restored afterwards?

[ ... ]

> +	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> +		if (!(ports & BIT(i)) || !(status[i] & mask))
> +			continue;
> +
> +		stopped |= BIT(i);
> +		ret = regmap_clear_bits(priv->regmap, QCA8K_REG_PORT_STATUS(i),
> +					mask);
> +		if (ret)
> +			goto restore;
> +	}

[Severity: Medium]
The commit message says:

  Abort before updating the frame size if reading port status or pausing
  the MAC fails. Attempt to restore all ports already modified, and report
  any restoration failures even if an earlier error occurred.

Can this goto restore fire on the MDIO register path?
qca8k_mii_write32() in qca8k-8xxx.c returns void:

static void
qca8k_mii_write32(struct mii_bus *bus, int phy_id, u32 regnum, u32 val)
{
	if (qca8k_mii_write_lo(bus, phy_id, regnum, val) < 0)
		return;

	qca8k_mii_write_hi(bus, phy_id, regnum + 1, val);
}

qca8k_regmap_update_bits_mii() returns the read result and ignores the
write:

	val &= ~mask;
	val |= write_val;
	qca8k_mii_write32(bus, 0x10 | r2, r1, val);

exit:
	mutex_unlock(&bus->mdio_lock);

	return ret;

qca8k_write_mii() only returns the qca8k_set_page() result.

Without mgmt_conduit, or when the Ethernet management path falls back to
MDIO, three calls report success even if the bus write fails:
regmap_clear_bits() here, qca8k_write() of QCA8K_MAX_FRAME_SIZE, and
regmap_update_bits() in the restore loop.

Wouldn't that let MAX_FRAME_SIZE be written while a CPU MAC is still
enabled? It would also mean the "failed to restore MAC state" message
never prints for a failed MDIO write.

The commit message also says:

  The standalone qca8k MDIO error-propagation fix is a prerequisite for
  this series; that error-handling bug predates this locking change.

That fix is not in the tree or in this series. The MDIO write helpers
are still unchanged at the end of the series.

Could the message name the prerequisite commit or link to it, so
maintainers and stable backporters know what must be applied first?

[ ... ]

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