[PATCH net-next v3 0/3] net: dsa: qca8k: fix MDIO error handling

[PATCH net-next v3 0/3] net: dsa: qca8k: fix MDIO error handling

[Yongzhao Chen]

This series fixes qca8k MDIO error-handling bugs found by code
inspection and the Sashiko review of v1 [1]. I am not aware of reports
of actual MDIO failures, so v3 targets net-next without Fixes tags, as
Andrew requested [2].

The patches propagate register and PHY access errors, skip MASTER_EN
cleanup after a failed page select, invalidate the uncertain page cache,
and return management Ethernet busy-wait errors so MDIO fallback can run.
They apply directly to net-next; the separate qca8k MTU series depends on
the write-error propagation provided here.

Testing: the base and each patch build with W=1 for arm64
(drivers/net/dsa/qca, net/dsa, drivers/net/phy/qcom). A userspace model
compiles the driver's own MDIO functions against a simulated paged
switch and a scripted management Ethernet responder, with phylib's
read-modify-write helper as a consumer. Fault injection leaves 15 failing
cases on the base, 10 after patch 1, seven after patch 2, and none after
patch 3. These are model tests, not kernel selftests or hardware error
injection.

A Redmi AX5400 (RA74, IPQ5018 + QCA8337) passed connectivity checks
with a 6.18.52 OpenWrt backport of v2 after boot, reboot, one cold boot,
link re-initialization and MTU changes. Its switch PHYs are on the SoC
MDIO bus and it has no CPU port 0. This covers normal register access
through page selection; the internal MDIO master and management
Ethernet paths remain tested only in the model.

v3:
- Target net-next and drop the Fixes: tags as requested [2]. Historical
  commits are identified in the commit messages instead.
- Order local variable declarations in reverse Christmas tree order in
  the functions the series touches [3]. No other code change.

Previous versions:
v2 [4], v1 [5].

[1] https://lore.kernel.org/netdev/179054715863.3145.10179961093285192493@kernel.org/
[2] https://lore.kernel.org/netdev/12875133-1659-4507-a007-c3cfa425ec76@lunn.ch/
[3] https://lore.kernel.org/netdev/f7889e9e-8ca8-47b1-b902-c31e7c016858@lunn.ch/
[4] https://lore.kernel.org/netdev/20260928220629.238-1-yongzhao.derek@gmail.com/
[5] https://lore.kernel.org/netdev/20260923215748.1336-1-yongzhao.derek@gmail.com/

Yongzhao Chen (3):
  net: dsa: qca8k: propagate MDIO errors
  net: dsa: qca8k: do not clear MASTER_EN after a failed page select
  net: dsa: qca8k: fail mgmt Ethernet MDIO access on busy wait errors

 drivers/net/dsa/qca/qca8k-8xxx.c | 68 +++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 28 deletions(-)


base-commit: cfb7793d1bc0f7d90571611979654cf1b3886b29
-- 
2.43.0

[PATCH net-next v3 1/3] net: dsa: qca8k: propagate MDIO errors

[Yongzhao Chen]

qca8k_mii_write32() ignores errors from both 16-bit half-word writes.
Its callers (register writes and read-modify-write helpers) then return
the earlier successful page selection or read result, incorrectly
reporting success even when writing the switch register failed.

Propagate write errors from both the low and high half-words through the
regmap paths and internal MDIO master transactions. Attempt to clear
MASTER_EN even if a transaction fails, preserving the initial error, and
report a cleanup failure if the transaction itself succeeded. Preserve
the existing Ethernet-to-MDIO fallback order.

Propagate read errors through the internal and legacy MDIO bus callbacks
instead of masking them as 0xffff. Returning 0xffff causes PHY
read-modify-write callers to treat the failed read as valid register
data, which can corrupt unrelated bits on writeback.

Stop polling when reading the MDIO busy status fails, and return the
error immediately. Because the read helper clears its output on failure,
checking only the BUSY bit mistakes an unsuccessful read for transaction
completion.

Code inspection found these problems. Fault injection into a userspace
model of the driver's functions reproduces them; the corresponding cases
pass after this change.

qca8k_mii_write32() has not returned write errors since its introduction
in commit 6b93fb46480a ("net-next: dsa: add new driver for qca8xxx
family").

Signed-off-by: Yongzhao Chen <yongzhao.derek@gmail.com>
Assisted-by: LLM
---
v3:
- Target net-next and drop the Fixes: tag: no actual MDIO error has been
  reported. The commit that introduced the problem is named in the text.
- Order local variable declarations in reverse Christmas tree order.

v2:
- Removed the blank line between the Fixes: tag and the other trailers.
- Cc John Crispin, author of the commit named in the Fixes: tag.

v1: https://lore.kernel.org/netdev/20260923215748.1336-1-yongzhao.derek@gmail.com/

 drivers/net/dsa/qca/qca8k-8xxx.c | 53 +++++++++++++++++---------------
 1 file changed, 29 insertions(+), 24 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 60f2a615a5c..b73c7c52e0f 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -129,13 +129,16 @@ qca8k_mii_read32(struct mii_bus *bus, int phy_id, u32 regnum, u32 *val)
 	return ret;
 }
 
-static void
+static int
 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;
+	int ret;
+
+	ret = qca8k_mii_write_lo(bus, phy_id, regnum, val);
+	if (ret < 0)
+		return ret;
 
-	qca8k_mii_write_hi(bus, phy_id, regnum + 1, val);
+	return qca8k_mii_write_hi(bus, phy_id, regnum + 1, val);
 }
 
 static int
@@ -462,7 +465,7 @@ qca8k_write_mii(struct qca8k_priv *priv, uint32_t reg, uint32_t val)
 	if (ret < 0)
 		goto exit;
 
-	qca8k_mii_write32(bus, 0x10 | r2, r1, val);
+	ret = qca8k_mii_write32(bus, 0x10 | r2, r1, val);
 
 exit:
 	mutex_unlock(&bus->mdio_lock);
@@ -492,7 +495,7 @@ qca8k_regmap_update_bits_mii(struct qca8k_priv *priv, uint32_t reg,
 
 	val &= ~mask;
 	val |= write_val;
-	qca8k_mii_write32(bus, 0x10 | r2, r1, val);
+	ret = qca8k_mii_write32(bus, 0x10 | r2, r1, val);
 
 exit:
 	mutex_unlock(&bus->mdio_lock);
@@ -794,19 +797,18 @@ static int
 qca8k_mdio_busy_wait(struct mii_bus *bus, u32 reg, u32 mask)
 {
 	u16 r1, r2, page;
-	u32 val;
 	int ret, ret1;
+	u32 val;
 
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
-	ret = read_poll_timeout(qca8k_mii_read_hi, ret1, !(val & mask), 0,
+	ret = read_poll_timeout(qca8k_mii_read_hi, ret1,
+				ret1 < 0 || !(val & mask), 0,
 				QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC, false,
 				bus, 0x10 | r2, r1 + 1, &val);
 
-	/* Check if qca8k_read has failed for a different reason
-	 * before returnting -ETIMEDOUT
-	 */
-	if (ret < 0 && ret1 < 0)
+	/* Preserve an MDIO read error instead of treating it as ready. */
+	if (ret1 < 0)
 		return ret1;
 
 	return ret;
@@ -817,8 +819,8 @@ qca8k_mdio_write(struct qca8k_priv *priv, int phy, int regnum, u16 data)
 {
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
+	int ret, ret1;
 	u32 val;
-	int ret;
 
 	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
 		return -EINVAL;
@@ -836,14 +838,18 @@ qca8k_mdio_write(struct qca8k_priv *priv, int phy, int regnum, u16 data)
 	if (ret)
 		goto exit;
 
-	qca8k_mii_write32(bus, 0x10 | r2, r1, val);
+	ret = qca8k_mii_write32(bus, 0x10 | r2, r1, val);
+	if (ret < 0)
+		goto exit;
 
 	ret = qca8k_mdio_busy_wait(bus, QCA8K_MDIO_MASTER_CTRL,
 				   QCA8K_MDIO_MASTER_BUSY);
 
 exit:
 	/* even if the busy_wait timeouts try to clear the MASTER_EN */
-	qca8k_mii_write_hi(bus, 0x10 | r2, r1 + 1, 0);
+	ret1 = qca8k_mii_write_hi(bus, 0x10 | r2, r1 + 1, 0);
+	if (!ret)
+		ret = ret1;
 
 	mutex_unlock(&bus->mdio_lock);
 
@@ -855,8 +861,8 @@ qca8k_mdio_read(struct qca8k_priv *priv, int phy, int regnum)
 {
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
+	int ret, ret1;
 	u32 val;
-	int ret;
 
 	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
 		return -EINVAL;
@@ -873,7 +879,9 @@ qca8k_mdio_read(struct qca8k_priv *priv, int phy, int regnum)
 	if (ret)
 		goto exit;
 
-	qca8k_mii_write_hi(bus, 0x10 | r2, r1 + 1, val);
+	ret = qca8k_mii_write_hi(bus, 0x10 | r2, r1 + 1, val);
+	if (ret < 0)
+		goto exit;
 
 	ret = qca8k_mdio_busy_wait(bus, QCA8K_MDIO_MASTER_CTRL,
 				   QCA8K_MDIO_MASTER_BUSY);
@@ -884,7 +892,9 @@ qca8k_mdio_read(struct qca8k_priv *priv, int phy, int regnum)
 
 exit:
 	/* even if the busy_wait timeouts try to clear the MASTER_EN */
-	qca8k_mii_write_hi(bus, 0x10 | r2, r1 + 1, 0);
+	ret1 = qca8k_mii_write_hi(bus, 0x10 | r2, r1 + 1, 0);
+	if (!ret)
+		ret = ret1;
 
 	mutex_unlock(&bus->mdio_lock);
 
@@ -919,12 +929,7 @@ qca8k_internal_mdio_read(struct mii_bus *slave_bus, int phy, int regnum)
 	if (ret >= 0)
 		return ret;
 
-	ret = qca8k_mdio_read(priv, phy, regnum);
-
-	if (ret < 0)
-		return 0xffff;
-
-	return ret;
+	return qca8k_mdio_read(priv, phy, regnum);
 }
 
 static int
-- 
2.43.0

[PATCH net-next v3 2/3] net: dsa: qca8k: do not clear MASTER_EN after a failed page select

[Yongzhao Chen]

qca8k_mdio_write() and qca8k_mdio_read() take the common exit path when
selecting the page of QCA8K_MDIO_MASTER_CTRL fails. That path clears
MASTER_CTRL[31:16] with a raw write to phy 0x10, reg 0x1f, which only
addresses MASTER_CTRL while page 0 is selected. After a failed page
select that did not reach the switch, the previous page remains selected.
The write can then clear the upper half of an unrelated register at
page * 0x200 + 0x3c.

No MDIO master transaction has been started at that point, so there is
nothing to clean up. Unlock and return the error directly.

qca8k_set_page() also keeps the old cached page when the page write
fails. If the write did reach the switch, a later access to the cached
page skips the page select and reaches the wrong register. Invalidate
the cache on failure so that the next access selects the page again.

The Sashiko review of v1 identified the stray cleanup write. A userspace
model of the driver's functions reproduces both problems by injecting
page-select failures with and without a hardware page change; these cases
pass after this change.

The cleanup write comes from commit 759bafb8a322 ("net: dsa: qca8k: add
support for internal phy and internal mdio"), and the stale page cache
from commit ba5707ec58cf ("net: dsa: qca8k: handle qca8k_set_page
errors").

Signed-off-by: Yongzhao Chen <yongzhao.derek@gmail.com>
Assisted-by: LLM
---
v3: Target net-next and drop the Fixes: tags. The commits that
introduced the problems are named in the text.

v2: new patch, addressing the Sashiko review of v1:
https://lore.kernel.org/netdev/179054715863.3145.10179961093285192493@kernel.org/

 drivers/net/dsa/qca/qca8k-8xxx.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index b73c7c52e0f..2708430413d 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -153,6 +153,8 @@ qca8k_set_page(struct qca8k_priv *priv, u16 page)
 
 	ret = bus->write(bus, 0x18, 0, page);
 	if (ret < 0) {
+		/* The switch may or may not have switched pages. */
+		*cached_page = 0xffff;
 		dev_err_ratelimited(&bus->dev,
 				    "failed to set qca8k page\n");
 		return ret;
@@ -836,7 +838,7 @@ qca8k_mdio_write(struct qca8k_priv *priv, int phy, int regnum, u16 data)
 
 	ret = qca8k_set_page(priv, page);
 	if (ret)
-		goto exit;
+		goto unlock;
 
 	ret = qca8k_mii_write32(bus, 0x10 | r2, r1, val);
 	if (ret < 0)
@@ -851,6 +853,7 @@ qca8k_mdio_write(struct qca8k_priv *priv, int phy, int regnum, u16 data)
 	if (!ret)
 		ret = ret1;
 
+unlock:
 	mutex_unlock(&bus->mdio_lock);
 
 	return ret;
@@ -877,7 +880,7 @@ qca8k_mdio_read(struct qca8k_priv *priv, int phy, int regnum)
 
 	ret = qca8k_set_page(priv, page);
 	if (ret)
-		goto exit;
+		goto unlock;
 
 	ret = qca8k_mii_write_hi(bus, 0x10 | r2, r1 + 1, val);
 	if (ret < 0)
@@ -896,6 +899,7 @@ qca8k_mdio_read(struct qca8k_priv *priv, int phy, int regnum)
 	if (!ret)
 		ret = ret1;
 
+unlock:
 	mutex_unlock(&bus->mdio_lock);
 
 	if (ret >= 0)
-- 
2.43.0

[PATCH net-next v3 3/3] net: dsa: qca8k: fail mgmt Ethernet MDIO access on busy wait errors

[Yongzhao Chen]

qca8k_phy_eth_command() polls MASTER_CTRL over management Ethernet until
BUSY clears, but only bails out when the poll timed out and the last
poll request also failed. If every request succeeds while BUSY stays
set, the -ETIMEDOUT is dropped. A read then returns MASTER_CTRL data
from a transaction that has not completed, and
qca8k_internal_mdio_read() does not fall back to the MDIO bus.

A failed poll request does not stop the loop either.
qca8k_phy_eth_busy_wait() leaves the value unchanged on failure, so the
BUSY test then uses the previous value, or an uninitialized one if the
first request fails.

Stop polling when a request fails and return the poll error or timeout.
The poll-error path also leaked read_skb, which is only consumed when the
read request is sent; free it on this path.

The Sashiko review of v1 identified the dropped timeout. A userspace
model of the driver's functions reproduces it and the poll-error path
with scripted BUSY responses and failed poll requests; these cases pass
after this change.

The polling loop comes from commit 2cd548566384 ("net: dsa: qca8k: add
support for phy read/write with mgmt Ethernet").

Signed-off-by: Yongzhao Chen <yongzhao.derek@gmail.com>
Assisted-by: LLM
---
v3: Target net-next and drop the Fixes: tag. The commit that
introduced the problem is named in the text.

v2: new patch, addressing the Sashiko review of v1:
https://lore.kernel.org/netdev/179054715863.3145.10179961093285192493@kernel.org/

 drivers/net/dsa/qca/qca8k-8xxx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 2708430413d..07227ae8bc0 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -728,12 +728,15 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	}
 
 	ret = read_poll_timeout(qca8k_phy_eth_busy_wait, ret1,
-				!(val & QCA8K_MDIO_MASTER_BUSY), 0,
+				ret1 < 0 || !(val & QCA8K_MDIO_MASTER_BUSY), 0,
 				QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC, false,
 				mgmt_eth_data, read_skb, &val);
 
-	if (ret < 0 && ret1 < 0) {
+	if (ret1 < 0)
 		ret = ret1;
+
+	if (ret < 0) {
+		kfree_skb(read_skb);
 		goto exit;
 	}
 
-- 
2.43.0