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

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

[Yongzhao Chen]

The MTU update pauses enabled ports 0 and 6 and then unconditionally
enables their MACs. A port can remain enabled while phylink has disabled
its MAC after a link-down event, so an MTU change can undo that decision.

The sequence can also interleave with phylink MAC callbacks, which run
without RTNL. A link-up can re-enable a MAC before the frame-size write,
or the restore can overwrite a link-down. Commit 386228c694bf found that
changing MAX_FRAME_SIZE while a CPU port is on can stop the switch from
sending packets until it is reset; a MAC re-enabled by the link-up leaves
that window open. This can happen at boot, when the CPU port's first
link-up is still pending as DSA sets the user ports' MTU.

Serialize the MTU transaction and PORT_STATUS writers with a dedicated
mutex. Save and restore the MAC bits instead of forcing them on. Abort
on detected read or pause errors, attempt to restore every touched port,
and return the first error. Keep PHY and phylink calls outside the lock.

Fixes: 386228c694bf ("net: dsa: qca8k: reset cpu port on MTU change")
Assisted-by: LLM
Signed-off-by: Yongzhao Chen <yongzhao.derek@gmail.com>
---
v5 (split from net-next v4 2/3):
- Separate the existing port 0/6 fix from internal CPU port support and
  target net, with a Fixes tag for the pause/restore sequence.
- Use a dedicated port_status_lock, as Christian suggested [1].
- Keep the pause scope at enabled ports 0/6, including user ports.

My earlier reply ruled out races in port_enabled_map under RTNL, not
the MTU transaction defects fixed here. These were found by code
inspection, not from a port 0/6 hardware failure.

The net base still has MDIO accesses which can swallow write errors.
This patch handles errors returned by the register API; it does not fix
that transport limitation or depend on the MDIO series in net-next.

net-next has since switched qca8k_port_set_status() to
regmap_assign_bits(). When merging, keep that call in
__qca8k_port_set_status().

The affected arm64 objects build with W=1. A userspace harness compiles
the actual driver functions and checks saved-state restoration,
MTU/callback interleavings and detected-error recovery. The old code and
10 mutations fail the relevant checks. These are software checks; this
standalone revision has not been tested on hardware.

[1] https://lore.kernel.org/netdev/6abb535c.8fb0a6ce.10321.5538@mx.google.com/
v4: https://lore.kernel.org/netdev/20260928220811.1880-3-yongzhao.derek@gmail.com/

 drivers/net/dsa/qca/qca8k-8xxx.c   |  3 ++
 drivers/net/dsa/qca/qca8k-common.c | 88 ++++++++++++++++++++++++++++++--------
 drivers/net/dsa/qca/qca8k.h        |  5 +++
 3 files changed, 77 insertions(+), 19 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 4c928983b..9aadca8b9 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->port_status_lock);
 	qca8k_write(priv, QCA8K_REG_PORT_STATUS(port), reg);
+	mutex_unlock(&priv->port_status_lock);
 }
 
 static struct qca8k_pcs *pcs_to_qca8k_pcs(struct phylink_pcs *pcs)
@@ -2116,6 +2118,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 	priv->ds->ops = &qca8k_switch_ops;
 	priv->ds->phylink_mac_ops = &qca8k_phylink_mac_ops;
 	mutex_init(&priv->reg_mutex);
+	mutex_init(&priv->port_status_lock);
 	dev_set_drvdata(&mdiodev->dev, priv);
 
 	return dsa_register_switch(priv->ds);
diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
index 13005f10e..084df92c5 100644
--- a/drivers/net/dsa/qca/qca8k-common.c
+++ b/drivers/net/dsa/qca/qca8k-common.c
@@ -463,7 +463,7 @@ int qca8k_mib_init(struct qca8k_priv *priv)
 	return ret;
 }
 
-void qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
+static u32 qca8k_port_status_mask(int port)
 {
 	u32 mask = QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
 
@@ -471,10 +471,27 @@ void qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
 	if (port > 0 && port < 6)
 		mask |= QCA8K_PORT_STATUS_LINK_AUTO;
 
+	return mask;
+}
+
+static void __qca8k_port_set_status(struct qca8k_priv *priv, int port,
+				    int enable)
+{
+	lockdep_assert_held(&priv->port_status_lock);
+
 	if (enable)
-		regmap_set_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask);
+		regmap_set_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port),
+				qca8k_port_status_mask(port));
 	else
-		regmap_clear_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask);
+		regmap_clear_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port),
+				  qca8k_port_status_mask(port));
+}
+
+void qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
+{
+	mutex_lock(&priv->port_status_lock);
+	__qca8k_port_set_status(priv, port, enable);
+	mutex_unlock(&priv->port_status_lock);
 }
 
 void qca8k_get_strings(struct dsa_switch *ds, int port, u32 stringset,
@@ -751,8 +768,10 @@ int qca8k_port_enable(struct dsa_switch *ds, int port,
 {
 	struct qca8k_priv *priv = ds->priv;
 
-	qca8k_port_set_status(priv, port, 1);
+	mutex_lock(&priv->port_status_lock);
+	__qca8k_port_set_status(priv, port, 1);
 	priv->port_enabled_map |= BIT(port);
+	mutex_unlock(&priv->port_status_lock);
 
 	if (dsa_is_user_port(ds, port))
 		phy_support_asym_pause(phy);
@@ -764,14 +783,18 @@ void qca8k_port_disable(struct dsa_switch *ds, int port)
 {
 	struct qca8k_priv *priv = ds->priv;
 
-	qca8k_port_set_status(priv, port, 0);
+	mutex_lock(&priv->port_status_lock);
+	__qca8k_port_set_status(priv, port, 0);
 	priv->port_enabled_map &= ~BIT(port);
+	mutex_unlock(&priv->port_status_lock);
 }
 
 int qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
 {
+	u32 status[QCA8K_NUM_PORTS] = { 0 };
 	struct qca8k_priv *priv = ds->priv;
-	int ret;
+	unsigned long ports, stopped = 0;
+	int ret, restore_ret, i;
 
 	/* We have only have a general MTU setting.
 	 * DSA always set the CPU port's MTU to the largest MTU of the user
@@ -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;
+		}
+	}
 
+unlock:
+	mutex_unlock(&priv->port_status_lock);
 	return ret;
 }
 
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 956338893..9225f4c40 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -459,6 +459,11 @@ struct qca8k_priv {
 	struct mii_bus *internal_mdio_bus;
 	struct dsa_switch *ds;
 	struct mutex reg_mutex;
+	/* Serializes writes to QCA8K_REG_PORT_STATUS and the MTU update. Taken
+	 * from phylink MAC callbacks: do not access a PHY or call into phylink
+	 * with it held.
+	 */
+	struct mutex port_status_lock;
 	struct device *dev;
 	struct gpio_desc *reset_gpio;
 	struct net_device *mgmt_conduit; /* Track if mdio/mib Ethernet is available */
base-commit: f88bb3606e4bd11ee035b682cddc2999e604609a
-- 
2.43.0

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

[netdev-bot+sashiko]

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

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

[Yongzhao Chen]

On Mon, 19 Oct 2026 11:02:30 +0000 netdev-bot+sashiko@kernel.org wrote:
> 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.

Yes. With v5, a failed restore after a successful frame-size write
leaves the new frame size in the switch while DSA keeps the old MTU.
On a fixed-link CPU port the same failure can keep the MAC off until
a link flap or reboot.

> 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?

v6 takes the first option: it returns the result of the frame-size
write, and a later restore failure no longer replaces it. A failed
PORT_STATUS write now keeps its target and is retried every second
from delayed work, so a fixed-link port does not depend on another
mac_link_up(). Newer port decisions replace the pending bits. The
next MTU change first recovers pending writes on ports 0 and 6 and
returns an error if that fails, so a failed restore is not taken as
the saved state.

A userspace harness that compiles the driver functions and
dsa_user_change_mtu() now covers both cases, and v5 fails it. v6 has
not been tested on hardware. I will send it separately.

Thanks,
Yongzhao Chen

pw-bot: cr