[PATCH net-next v8 0/2] net: dsa: qca8k: preserve port state across MTU changes

[PATCH net-next v8 0/2] net: dsa: qca8k: preserve port state across MTU changes

[Yongzhao Chen]

The qca8k MTU callback pauses the CPU MACs on ports 0 and 6, writes
MAX_FRAME_SIZE and then enables both MACs unconditionally. That can undo
a phylink link-down decision, and phylink callbacks, which run without
RTNL, can enable a MAC during the update. Patch 2 saves and restores the
port state under a new lock that all PORT_STATUS writers take.

Sashiko's review of v7 [1] showed that readers that do not take that
lock, such as the PCS poll, can still corrupt the saved state: the qca8k
regmap is created with disable_locking, so concurrent accesses share
regmap's work_buf. Patch 1, from Stanislaw Pal, lets regmap take its own
lock. Stanislaw posted it to net [2] and offered that I carry it here
[3].

As Andrew asked [4], the series targets net-next: the problem was found
by review and has not been seen in production. Both patches keep their
Fixes tags.

LLM coding assistants helped write both patches. Stanislaw, checkpatch
on net-next reports the Assisted-by trailer as non-standard, so I
dropped it from patch 1 and mention the assistance here instead; nothing
else in your patch changed.

Testing: the affected arm64 objects build with W=1, with CONFIG_PM_SLEEP
enabled and disabled. A local KUnit test on UML, not part of this
series, runs the driver's regmap config, register, PCS and phylink
callbacks, MTU and suspend code against the real regmap core, with a
register-file MDIO bus. It forces the work_buf value interleaving from
[1] on that bus. It also covers both CPU MACs running, port 6 down or
disabled, failed pause and restore writes, the second CPU callback after
a commit, retries and a newer link-down decision. All cases pass, and
PROVE_LOCKING reports nothing on the exercised paths. Without patch 1,
the two interleaving cases fail: MAX_FRAME_SIZE is written with the port
0 MAC enabled, and a PCS poll of port 6 reports port 0's link. Not
tested on hardware or over the Ethernet management path.

Changes in v8:
- Target net-next [4].
- Add patch 1 from Stanislaw [2], which v7 was missing [1].
- Rebase on net-next. qca8k_port_set_status() needed manual resolution
  against regmap_assign_bits(); the MTU and PORT_STATUS code is the same
  as v7.
- Say in patch 2's commit message how the problem was found [5].
- Drop the Assisted-by trailers from both patches and describe the LLM
  assistance in this letter instead.

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

[1] https://lore.kernel.org/netdev/179303171241.3134.15019965307136674652@kernel.org/
[2] https://lore.kernel.org/netdev/20261022192933.384898-2-kuncy7@gmail.com/
[3] https://lore.kernel.org/netdev/20261028145622.195730-1-kuncy7@gmail.com/
[4] https://lore.kernel.org/netdev/47e9713f-815e-4caf-8928-a7ec12d0087b@lunn.ch/
[5] https://lore.kernel.org/netdev/179268501671.3290.6623232109527554630@kernel.org/

Stanislaw Pal (1):
  net: dsa: qca8k: let regmap serialize register access

Yongzhao Chen (1):
  net: dsa: qca8k: preserve port state across MTU changes

 drivers/net/dsa/qca/qca8k-8xxx.c   |  34 ++++++-
 drivers/net/dsa/qca/qca8k-common.c | 149 +++++++++++++++++++++++++----
 drivers/net/dsa/qca/qca8k.h        |  15 +++
 3 files changed, 175 insertions(+), 23 deletions(-)


base-commit: 7b6ab4dbc156e2f96419715130b3d5738a306c93
-- 
2.43.0

[PATCH net-next v8 1/2] net: dsa: qca8k: let regmap serialize register access

[Yongzhao Chen]

From: Stanislaw Pal <kuncy7@gmail.com>

The regmap is created with .disable_locking, on the ground that the
qca8k read and write callbacks do their own locking. They only lock the
bus transaction, though. Since the driver moved to the regmap
read/write API, regmap formats the register and parses the value in the
map's shared work_buf, outside of the callbacks, so two concurrent
users can swap or corrupt each other's register and value.

Most register access runs under rtnl, but not all of it: phylink's
resolve work (mac_link_up/down) and the LED class callbacks do not, so
a link change can race with an ethtool or LED access and write the
wrong switch register or read back the wrong value.

Let regmap take its own lock. Every user of the map can already sleep,
on the MDIO bus or on the management frame round trip.

Fixes: c766e077d927 ("net: dsa: qca8k: convert to regmap read/write API")
Signed-off-by: Stanislaw Pal <kuncy7@gmail.com>
Signed-off-by: Yongzhao Chen <yongzhao.derek@gmail.com>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 07227ae8b..1e4f83e9e 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -579,7 +579,6 @@ static const struct regmap_config qca8k_regmap_config = {
 	.write = qca8k_bulk_write,
 	.reg_update_bits = qca8k_regmap_update_bits,
 	.rd_table = &qca8k_readable_table,
-	.disable_locking = true, /* Locking is handled by qca8k read/write */
 	.cache_type = REGCACHE_NONE, /* Explicitly disable CACHE */
 	.max_raw_read = 32, /* mgmt eth can read up to 8 registers at time */
 	/* ATU regs suffer from a bug where some data are not correctly
-- 
2.43.0

[PATCH net-next v8 2/2] 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.
Mask partial status values before passing them to the regmap callback.
The saved state depends on regmap serializing register access, because
readers such as the PCS poll do not take the new mutex.

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. Record successful frame-size writes and skip
requests for the same value. DSA invokes the callback for every CPU port,
so a second invocation must not reject an already committed global value.

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 changing the frame size again. Disable and drain retries for
suspend, shutdown and removal, with managed cleanup on probe failure.

The problem was found during development by an LLM-assisted review. It
has been reproduced only in test harnesses, not on hardware.

Fixes: 386228c694bf ("net: dsa: qca8k: reset cpu port on MTU change")
Signed-off-by: Yongzhao Chen <yongzhao.derek@gmail.com>
---
 drivers/net/dsa/qca/qca8k-8xxx.c   |  33 ++++++-
 drivers/net/dsa/qca/qca8k-common.c | 149 +++++++++++++++++++++++++----
 drivers/net/dsa/qca/qca8k.h        |  15 +++
 3 files changed, 175 insertions(+), 22 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 1e4f83e9e..288cfab81 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -1494,7 +1494,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)
@@ -2006,6 +2008,7 @@ qca8k_setup(struct dsa_switch *ds)
 
 	/* Setup our port MTUs to match power on defaults */
 	ret = qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, ETH_FRAME_LEN + ETH_FCS_LEN);
+	priv->max_frame_size = ret ? 0 : ETH_FRAME_LEN + ETH_FCS_LEN;
 	if (ret)
 		dev_warn(priv->dev, "failed setting MTU settings");
 
@@ -2066,6 +2069,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)
 {
@@ -2127,6 +2137,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);
@@ -2141,6 +2156,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);
 
@@ -2154,6 +2170,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);
@@ -2176,13 +2193,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)
@@ -2190,6 +2218,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 ab978cc83..d1fc46f88 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,7 +471,77 @@ void qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
 	if (port > 0 && port < 6)
 		mask |= QCA8K_PORT_STATUS_LINK_AUTO;
 
-	regmap_assign_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask, enable);
+	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
+		ret = regmap_update_bits(priv->regmap,
+					 QCA8K_REG_PORT_STATUS(port), mask,
+					 priv->port_status_value[port] & mask);
+	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 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,
@@ -742,8 +812,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);
@@ -755,14 +827,19 @@ 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 frame_size = new_mtu + ETH_HLEN + ETH_FCS_LEN;
+	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
@@ -773,27 +850,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);
+
+	/* DSA calls us for every CPU port, but the frame size is global. */
+	if (priv->max_frame_size == frame_size) {
+		ret = 0;
+		goto unlock;
+	}
 
-	if (priv->port_enabled_map & BIT(6))
-		qca8k_port_set_status(priv, 6, 0);
+	ports = BIT(0) | BIT(6);
 
-	/* Include L2 header / FCS length */
-	ret = qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, new_mtu +
-			  ETH_HLEN + ETH_FCS_LEN);
+	/* 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;
+	}
+
+	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;
+	}
 
-	if (priv->port_enabled_map & BIT(0))
-		qca8k_port_set_status(priv, 0, 1);
+	ret = qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, frame_size);
+	if (!ret)
+		priv->max_frame_size = frame_size;
 
-	if (priv->port_enabled_map & BIT(6))
-		qca8k_port_set_status(priv, 6, 1);
+restore:
+	/* A failed restore must not reject a committed frame size. */
+	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..f4ab46475 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,17 @@ 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;
+	/* Committed frame size, 0 if unknown; protected by port_status_lock. */
+	u32 max_frame_size;
+	struct delayed_work port_status_work;
+	/* Nonzero masks mark 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 +514,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);
 
-- 
2.43.0

Re: [PATCH net-next v8 1/2] net: dsa: qca8k: let regmap serialize register access

[Andrew Lunn]

On Wed, Nov 04, 2026 at 01:01:28PM +0100, Yongzhao Chen wrote:
> From: Stanislaw Pal <kuncy7@gmail.com>
> 
> The regmap is created with .disable_locking, on the ground that the
> qca8k read and write callbacks do their own locking. They only lock the
> bus transaction, though. Since the driver moved to the regmap
> read/write API, regmap formats the register and parses the value in the
> map's shared work_buf, outside of the callbacks, so two concurrent
> users can swap or corrupt each other's register and value.
> 
> Most register access runs under rtnl, but not all of it: phylink's
> resolve work (mac_link_up/down) and the LED class callbacks do not, so
> a link change can race with an ethtool or LED access and write the
> wrong switch register or read back the wrong value.

Do any of these operations which don't take RTNL do an open coded
read/modify/write? Does the regmap read/modify/write operations hold
the regmap mutex over the entire operation?

	Andrew

Re: [PATCH net-next v8 1/2] net: dsa: qca8k: let regmap serialize register access

[Christian Marangi]

On Fri, Nov 06, 2026 at 02:38:37PM +0100, Andrew Lunn wrote:
> On Wed, Nov 04, 2026 at 01:01:28PM +0100, Yongzhao Chen wrote:
> > From: Stanislaw Pal <kuncy7@gmail.com>
> > 
> > The regmap is created with .disable_locking, on the ground that the
> > qca8k read and write callbacks do their own locking. They only lock the
> > bus transaction, though. Since the driver moved to the regmap
> > read/write API, regmap formats the register and parses the value in the
> > map's shared work_buf, outside of the callbacks, so two concurrent
> > users can swap or corrupt each other's register and value.
> > 
> > Most register access runs under rtnl, but not all of it: phylink's
> > resolve work (mac_link_up/down) and the LED class callbacks do not, so
> > a link change can race with an ethtool or LED access and write the
> > wrong switch register or read back the wrong value.
> 
> Do any of these operations which don't take RTNL do an open coded
> read/modify/write? Does the regmap read/modify/write operations hold
> the regmap mutex over the entire operation?
> 

Maybe I'm just tired from destroying board from my HW project but... I'm a
bit confused of when read/write are not serialized.

Operation of the LED and on the port have their own dedicated register per
port and for shared stuff, there is a dedicated mutex for it.

So I'm not sure what case this patch is fixing...

Both mgmt and mdio operation locks at the start and unlock at the end of
the read/write or rmw operation.

Using the regmap lock seems redundant and this is why I disabled
originally. 

I would like to have more explaination for the 

"regmap formats the register and parses the value in the
map's shared work_buf, outside of the callbacks, so two concurrent
users can swap or corrupt each other's register and value."

case.

-- 
	Ansuel

Re: [PATCH net-next v8 1/2] net: dsa: qca8k: let regmap serialize register access

[Stanislaw Pal]

On Fri, Nov 06, 2026 at 07:07:19PM +0100, Christian Marangi wrote:
> Both mgmt and mdio operation locks at the start and unlock at the end of
> the read/write or rmw operation.
> Using the regmap lock seems redundant and this is why I disabled
> originally.
> I would like to have more explaination for the
> "regmap formats the register and parses the value in the
> map's shared work_buf, outside of the callbacks, so two concurrent
> users can swap or corrupt each other's register and value."
> case.

The qca8k locks cover what runs inside the callbacks, but since the
map uses .read/.write, part of every access runs in the regmap core
before and after them, in map->work_buf, which there is one of per
map:

- regmap_read() goes through _regmap_bus_read() and _regmap_raw_read():
  format_reg() writes the register into map->work_buf, ->read() is
  called with reg_buf = map->work_buf and val_buf = work_buf +
  reg_bytes, and after it returns parse_val() reads the value back
  from work_buf + reg_bytes.
- regmap_write() goes through _regmap_bus_raw_write() and
  _regmap_raw_write_impl() (no format_write for 16-bit registers with
  32-bit values): format_val() puts the value into map->work_buf
  behind the register bytes, format_reg() puts the register in front
  of it, and ->write() is called on map->work_buf as one buffer.

qca8k_bulk_read() takes reg = *(u16 *)reg_buf before it locks
anything, and the value it stores is parsed after it has unlocked;
qca8k_bulk_write() likewise takes the register and the value out of
work_buf before its lock. With .disable_locking nothing orders two
callers around those steps, so caller A can format its register,
caller B format its own over it, and A's callback then reads or
writes B's register; or A's value is overwritten in work_buf before A
parses it.

Two callers that can meet there without rtnl: phylink's resolve work,
which holds only phylink's own mutexes when it calls
qca8k_phylink_mac_link_up() -> qca8k_write(PORT_STATUS), and the LEDs,
e.g. activating the netdev trigger, which calls ->hw_control_get() ->
regmap_read() under the trigger's locks only. ethtool -S and the bridge
callbacks hold rtnl, but the two above do not.

On Fri, Nov 06, 2026 at 02:38:37PM +0100, Andrew Lunn wrote:
> Do any of these operations which don't take RTNL do an open coded
> read/modify/write? Does the regmap read/modify/write operations hold
> the regmap mutex over the entire operation?

No open-coded RMW that I found: the phylink callbacks do plain writes
(and regmap_set_bits/clear_bits on link down), the LEDs use
regmap_update_bits() or plain reads. Whether regmap_update_bits() holds
a lock over the whole operation depends on the path. With REGCACHE_NONE
every register is volatile, so it goes to qca8k_regmap_update_bits().
Over MDIO that holds the bus lock across the read and the write. Over
the management frames it does not: qca8k_regmap_update_bits_eth() is
qca8k_read_eth() followed by qca8k_write_eth(), each taking
mgmt_eth_data->mutex on its own, so two updates of the same register
can interleave between the read and the write and one loses its bits.
The LED control registers hold the rule bits of ports 0-3 and port 4
in one register, and the LED callbacks run without rtnl, so two LEDs
can meet there. With the map lock enabled, regmap_update_bits_base()
holds it around the whole update, which closes that one too.

To be clear about where this comes from: I found it reading the regmap
path, not from corrupted registers on a board, and I have not seen it
happen. If that changes the case for net-next versus a lighter fix, I'm
happy to rework the commit message, or the patch, whichever you prefer.

Stanislaw