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

[PATCH net v6] 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. This can undo a phylink link-down decision. Phylink
callbacks also run without RTNL and can enable a MAC during the update.
Changing MAX_FRAME_SIZE with a CPU MAC running can stop the switch from
sending packets until reset, as described in commit 386228c694bf.

Serialize the MTU transaction and PORT_STATUS writers with a dedicated
mutex. Save and restore the MAC bits, abort before the frame-size write
on detected snapshot or pause errors, and restore every touched port.

Return the result of the frame-size write even if a later port restore
fails: DSA does not roll back the switch's frame size when the CPU MTU
callback returns an error. Retain failed status writes and retry them
every second, because a fixed-link port may receive no further link-up
callback. Merge newer port decisions into the pending target, and
recover pending writes on ports 0 and 6 before taking another MTU
snapshot. Disable and drain retries for suspend, shutdown and removal,
with managed cleanup on probe failure.

Fixes: 386228c694bf ("net: dsa: qca8k: reset cpu port on MTU change")
Assisted-by: LLM
Signed-off-by: Yongzhao Chen <yongzhao.derek@gmail.com>
---
v6:
- Address Sashiko's review [1]:
  - Keep the MTU result once MAX_FRAME_SIZE is committed.
  - Retain failed PORT_STATUS targets and retry them without requiring
    a fixed-link transition. Newer MAC decisions supersede pending bits.
  - Recover pending writes before the next MTU snapshot, so a failed
    restore is not saved as MAC-off state. This includes a failed
    disable whose port_enabled_map bit has already been cleared.
- Disable and drain the retry work for suspend and device teardown;
  retry pending writes after resume or a failed suspend.

v5: https://lore.kernel.org/netdev/20261015104550.43169-1-yongzhao.derek@gmail.com/
v4: https://lore.kernel.org/netdev/20260928220811.1880-3-yongzhao.derek@gmail.com/

This remains the standalone net fix for existing ports 0 and 6. Internal
CPU support stays in the separate net-next series. The defects were found
by code inspection; this revision has not been tested on hardware.

The affected arm64 objects build with W=1, with CONFIG_PM_SLEEP both
enabled and disabled. A userspace harness compiles the driver functions
and dsa_user_change_mtu(), with mocked register, notifier and workqueue
boundaries. It checks MTU shrink, restore failures and retries without a
new link event, superseding callbacks, callback interleavings and lifecycle
control. The sent v5 and six deliberately broken variants of this patch
fail these checks.

The net base can still swallow MDIO write errors. The retry handles only
errors reported by the register API; it cannot detect a write that
falsely reports success.

[1] https://lore.kernel.org/netdev/179240775070.3135.2434822808609533404@kernel.org/

 drivers/net/dsa/qca/qca8k-8xxx.c   |  32 ++++++-
 drivers/net/dsa/qca/qca8k-common.c | 136 ++++++++++++++++++++++++-----
 drivers/net/dsa/qca/qca8k.h        |  13 +++
 3 files changed, 159 insertions(+), 22 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 4c928983b..4e6dc04ca 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;
 
-	qca8k_write(priv, QCA8K_REG_PORT_STATUS(port), reg);
+	mutex_lock(&priv->port_status_lock);
+	qca8k_port_status_update(priv, port, U32_MAX, reg);
+	mutex_unlock(&priv->port_status_lock);
 }
 
 static struct qca8k_pcs *pcs_to_qca8k_pcs(struct phylink_pcs *pcs)
@@ -2055,6 +2057,13 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.connect_tag_protocol	= qca8k_connect_tag_protocol,
 };
 
+static void qca8k_port_status_stop(void *data)
+{
+	struct qca8k_priv *priv = data;
+
+	disable_delayed_work_sync(&priv->port_status_work);
+}
+
 static int
 qca8k_sw_probe(struct mdio_device *mdiodev)
 {
@@ -2116,6 +2125,11 @@ 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);
+	INIT_DELAYED_WORK(&priv->port_status_work, qca8k_port_status_work);
+	ret = devm_add_action_or_reset(priv->dev, qca8k_port_status_stop, priv);
+	if (ret)
+		return ret;
 	dev_set_drvdata(&mdiodev->dev, priv);
 
 	return dsa_register_switch(priv->ds);
@@ -2130,6 +2144,7 @@ qca8k_sw_remove(struct mdio_device *mdiodev)
 	if (!priv)
 		return;
 
+	qca8k_port_status_stop(priv);
 	for (i = 0; i < QCA8K_NUM_PORTS; i++)
 		qca8k_port_set_status(priv, i, 0);
 
@@ -2143,6 +2158,7 @@ static void qca8k_sw_shutdown(struct mdio_device *mdiodev)
 	if (!priv)
 		return;
 
+	qca8k_port_status_stop(priv);
 	dsa_switch_shutdown(priv->ds);
 
 	dev_set_drvdata(&mdiodev->dev, NULL);
@@ -2165,13 +2181,24 @@ qca8k_set_pm(struct qca8k_priv *priv, int enable)
 	}
 }
 
+static void qca8k_port_status_start(struct qca8k_priv *priv)
+{
+	enable_delayed_work(&priv->port_status_work);
+	schedule_delayed_work(&priv->port_status_work, 0);
+}
+
 static int qca8k_suspend(struct device *dev)
 {
 	struct qca8k_priv *priv = dev_get_drvdata(dev);
+	int ret;
 
+	qca8k_port_status_stop(priv);
 	qca8k_set_pm(priv, 0);
+	ret = dsa_switch_suspend(priv->ds);
+	if (ret)
+		qca8k_port_status_start(priv);
 
-	return dsa_switch_suspend(priv->ds);
+	return ret;
 }
 
 static int qca8k_resume(struct device *dev)
@@ -2179,6 +2206,7 @@ static int qca8k_resume(struct device *dev)
 	struct qca8k_priv *priv = dev_get_drvdata(dev);
 
 	qca8k_set_pm(priv, 1);
+	qca8k_port_status_start(priv);
 
 	return dsa_switch_resume(priv->ds);
 }
diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
index 13005f10e..04001c345 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,74 @@ void qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
 	if (port > 0 && port < 6)
 		mask |= QCA8K_PORT_STATUS_LINK_AUTO;
 
-	if (enable)
-		regmap_set_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask);
+	return mask;
+}
+
+static int qca8k_port_status_restore(struct qca8k_priv *priv, int port)
+{
+	u32 mask = priv->port_status_mask[port];
+	int ret;
+
+	lockdep_assert_held(&priv->port_status_lock);
+
+	if (!mask)
+		return 0;
+
+	if (mask == U32_MAX)
+		ret = qca8k_write(priv, QCA8K_REG_PORT_STATUS(port),
+				  priv->port_status_value[port]);
 	else
-		regmap_clear_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask);
+		ret = regmap_update_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port),
+					 mask, priv->port_status_value[port]);
+	if (ret) {
+		dev_err_ratelimited(priv->dev, "failed to restore port %d status: %d\n",
+				    port, ret);
+		schedule_delayed_work(&priv->port_status_work, HZ);
+	} else {
+		priv->port_status_mask[port] = 0;
+	}
+
+	return ret;
+}
+
+void qca8k_port_status_update(struct qca8k_priv *priv, int port,
+			      u32 mask, u32 val)
+{
+	lockdep_assert_held(&priv->port_status_lock);
+
+	/* A newer decision supersedes the pending bits, even if it fails too. */
+	priv->port_status_value[port] &= ~mask;
+	priv->port_status_value[port] |= val & mask;
+	priv->port_status_mask[port] |= mask;
+	qca8k_port_status_restore(priv, port);
+}
+
+void qca8k_port_status_work(struct work_struct *work)
+{
+	struct qca8k_priv *priv = container_of(to_delayed_work(work),
+					     struct qca8k_priv, port_status_work);
+	int port;
+
+	mutex_lock(&priv->port_status_lock);
+	for (port = 0; port < QCA8K_NUM_PORTS; port++)
+		qca8k_port_status_restore(priv, port);
+	mutex_unlock(&priv->port_status_lock);
+}
+
+static void __qca8k_port_set_status(struct qca8k_priv *priv, int port,
+				    int enable)
+{
+	u32 mask = qca8k_port_status_mask(port);
+
+	lockdep_assert_held(&priv->port_status_lock);
+	qca8k_port_status_update(priv, port, mask, enable ? mask : 0);
+}
+
+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 +815,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 +830,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, 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 +852,53 @@ 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);
+
+	/* Do not mistake a failed restore for a new link-down decision. */
+	for_each_set_bit(i, &ports, QCA8K_NUM_PORTS) {
+		ret = qca8k_port_status_restore(priv, i);
+		if (ret)
+			goto unlock;
+	}
+
+	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;
+	}
 
-	if (priv->port_enabled_map & BIT(6))
-		qca8k_port_set_status(priv, 6, 0);
+	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:
+	/* A restore failure must not report a committed frame size as rejected. */
+	for_each_set_bit(i, &stopped, QCA8K_NUM_PORTS)
+		qca8k_port_status_update(priv, i, qca8k_port_status_mask(i),
+					 status[i]);
 
+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..e2d2e63fb 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -10,6 +10,7 @@
 
 #include <linux/delay.h>
 #include <linux/regmap.h>
+#include <linux/workqueue.h>
 #include <linux/gpio/consumer.h>
 #include <linux/leds.h>
 #include <linux/dsa/tag_qca.h>
@@ -459,6 +460,15 @@ 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 delayed_work port_status_work;
+	/* Nonzero masks identify pending writes, protected by port_status_lock. */
+	u32 port_status_mask[QCA8K_NUM_PORTS];
+	u32 port_status_value[QCA8K_NUM_PORTS];
 	struct device *dev;
 	struct gpio_desc *reset_gpio;
 	struct net_device *mgmt_conduit; /* Track if mdio/mib Ethernet is available */
@@ -502,6 +512,9 @@ static inline u32 qca8k_port_to_phy(int port)
 extern const struct qca8k_mib_desc ar8327_mib[];
 extern const struct regmap_access_table qca8k_readable_table;
 int qca8k_mib_init(struct qca8k_priv *priv);
+void qca8k_port_status_work(struct work_struct *work);
+void qca8k_port_status_update(struct qca8k_priv *priv, int port,
+			      u32 mask, u32 val);
 void qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable);
 int qca8k_read_switch_id(struct qca8k_priv *priv);
 

base-commit: 78731b46967e3e7102b5cd5062bef185f3404b5c
-- 
2.43.0

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 2 · 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] On a dual-CPU qca8k (ports 0 and 6 both declared as DSA CPU
  ports), the patch does not keep its own promise that an MTU change…
- [Medium] qca8k_port_status_restore() (qca8k-common.c:491-492) calls
  regmap_update_bits(priv->regmap, PORT_STATUS(port), mask,…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but bringing up a
  qca8k user port that has no PHY dereferences a NULL phy_device.

--

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

This patch adds a dedicated mutex that serializes PORT_STATUS writers with
the MTU update in qca8k_port_change_mtu(). The MTU path now saves and
restores the MAC bits on ports 0 and 6 instead of enabling them
unconditionally. Failed PORT_STATUS writes are cached and retried from a
delayed work item.

> diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
> index 13005f10edb7..04001c345dd6 100644
> --- a/drivers/net/dsa/qca/qca8k-common.c
> +++ b/drivers/net/dsa/qca/qca8k-common.c

[ ... ]

> @@ -471,10 +471,74 @@ void qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
>  	if (port > 0 && port < 6)
>  		mask |= QCA8K_PORT_STATUS_LINK_AUTO;
>  
> -	if (enable)
> -		regmap_set_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask);
> +	return mask;
> +}
> +
> +static int qca8k_port_status_restore(struct qca8k_priv *priv, int port)
> +{
> +	u32 mask = priv->port_status_mask[port];
> +	int ret;
> +
> +	lockdep_assert_held(&priv->port_status_lock);
> +
> +	if (!mask)
> +		return 0;
> +
> +	if (mask == U32_MAX)
> +		ret = qca8k_write(priv, QCA8K_REG_PORT_STATUS(port),
> +				  priv->port_status_value[port]);
>  	else
> -		regmap_clear_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask);
> +		ret = regmap_update_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port),
> +					 mask, priv->port_status_value[port]);

[Severity: Medium]
Can this write bits outside of mask into PORT_STATUS?

For a partial mask, the whole cached priv->port_status_value[port] is
passed as the value. qca8k_port_status_update() below only refreshes the
bits inside the current mask. That leaves the bits from the last full
link_up write (mask U32_MAX) in the cache: the forced SPEED field, DUPLEX,
TXFLOW, RXFLOW, and LINK_AUTO on ports 0 and 6.

The qca8k regmap has no cache and provides a .reg_update_bits callback.
PORT_STATUS is therefore treated as volatile, and _regmap_update_bits()
passes the raw value to the driver without applying val & mask:

drivers/base/regmap/regmap.c:_regmap_update_bits() {
    if (regmap_volatile(map, reg) && map->reg_update_bits) {
        ...
        ret = map->reg_update_bits(map->bus_context, reg, mask, val);
    ...
}

qca8k_regmap_update_bits_eth() and qca8k_regmap_update_bits_mii() then
both do this without masking write_val:

	val &= ~mask;
	val |= write_val;

The old regmap_set_bits() and regmap_clear_bits() calls only passed mask
or 0. With this change, every partial PORT_STATUS update ORs the cached
non-mask fields into the live register. That covers link_down,
port_enable/disable, qca8k_set_pm(), the MTU pause restore and the retry
worker.

The live non-mask bits can differ from the cache. For example, the switch
may lose state across suspend before qca8k_set_pm(priv, 1). Or a port 1-5
may be in LINK_AUTO with hardware-updated speed bits while the cache
still holds forced speed bits.

In those cases, could this turn on flow control or duplex bits that no
phylink decision requested? Could it also combine two SPEED encodings
into the reserved value 3, until the next full link_up write?

Would passing priv->port_status_value[port] & mask avoid this?

[ ... ]

> @@ -751,8 +815,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);

[Severity: High]
This isn't a bug introduced by this patch, but can phy be NULL here for a
user port that has no PHY?

Take a fixed-link port, or an in-band/SFP port on 0 or 6 without a
phy-handle. For these, phylink_fwnode_phy_connect() returns 0 without
attaching a PHY:

	if (!phylink_expects_phy(pl))
		return 0;

dsa_user_phy_setup() then succeeds without taking its -ENODEV fallback,
so dev->phydev stays NULL. Bringing the interface up then goes:

dsa_user_open()
  dsa_port_enable_rt(dp, dev->phydev)
    qca8k_port_enable(ds, port, NULL)
      phy_support_asym_pause(NULL)
        phy_copy_pause_bits(phydev->advertising, phydev->supported)

phy_support_asym_pause() has no NULL check. This would oops in ndo_open
with RTNL held. The code has looked like this since commit b3a302b171f7
("net: dsa: qca8k: move set age/MTU/port enable/disable functions to
common code").

Would it be enough to check phy as well as dsa_is_user_port() here?

[ ... ]

> @@ -782,27 +852,53 @@ int qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>  	if (!dsa_is_cpu_port(ds, port))
>  		return 0;
>  

[ ... ]

> +	mutex_lock(&priv->port_status_lock);
> +
> +	ports = BIT(0) | BIT(6);
> +
> +	/* Do not mistake a failed restore for a new link-down decision. */
> +	for_each_set_bit(i, &ports, QCA8K_NUM_PORTS) {
> +		ret = qca8k_port_status_restore(priv, i);
> +		if (ret)
> +			goto unlock;
> +	}

[Severity: Medium]
If ports 0 and 6 are both CPU ports, can qca8k_port_change_mtu() return
an error after the frame size has already been committed?

dsa_switch_mtu() calls port_change_mtu once for every port that
dsa_port_mtu_match() accepts, and that includes every CPU port. It returns
the first error:

net/dsa/switch.c:dsa_switch_mtu() {
    ...
		if (dsa_port_mtu_match(dp, info)) {
			ret = ds->ops->port_change_mtu(ds, dp->index,
						       info->mtu);
			if (ret)
				return ret;
    ...
}

QCA8K_MAX_FRAME_SIZE is a single global register, so the sequence could
be:

call 1, port 0:
  qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, ...) succeeds
  qca8k_port_status_update() restore fails, mask stays pending
  returns 0

call 2, port 6:
  qca8k_port_status_restore() on the pending port fails again
  goto unlock, returns the error

If the regmap_read() snapshot or regmap_clear_bits() fails in call 2,
the result is the same.

dsa_user_change_mtu() then jumps to out_cpu_failed, which only does:

	if (new_conduit_mtu != old_conduit_mtu)
		dev_set_mtu(conduit, old_conduit_mtu);

The user and conduit netdevs keep their old MTU while MAX_FRAME_SIZE
keeps the new value. If the MTU was being shrunk, the switch would drop
frames that the netdev MTUs still allow, until the next MTU change.

Isn't this the mismatch that the "A restore failure must not report a
committed frame size as rejected" comment and the commit message are
meant to prevent?

[ ... ]

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

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

[Yongzhao Chen]

On Thu, 22 Oct 2026 08:37:21 +0000 netdev-bot+sashiko@kernel.org wrote:
> Can this write bits outside of mask into PORT_STATUS?

Yes. The qca8k regmap passes the raw value to its update_bits callbacks,
which OR write_val without masking it. My previous register model
applied val & mask and hid this difference. v7 passes
port_status_value[port] & mask for partial updates.

> If ports 0 and 6 are both CPU ports, can qca8k_port_change_mtu() return
> an error after the frame size has already been committed?

Yes. My previous test called the driver once per MTU change instead of
going through dsa_switch_mtu(), which calls it for every CPU port. v7
records the committed frame size and returns success for a repeated
request with the same value, so the second CPU port cannot reject the
first port's commit.

The NULL PHY call in qca8k_port_enable() is already present in the v6
base. From the source, a user port without an attached PHY reaches it,
but I found no in-tree qca8k DTS with such a user port. It is
independent of this MTU change, so v7 leaves it alone. If it is fixed
later, dropping the call looks better than a NULL check:
phylink_bringup_phy() already calls phy_support_asym_pause() for every
PHY it attaches.

v7: https://lore.kernel.org/netdev/20261022155837.760-1-yongzhao.derek@gmail.com/

Yongzhao Chen

pw-bot: cr