[PATCH net] net: dsa: qca8k: propagate MDIO errors

[PATCH net] 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.

Fixes: 6b93fb46480a ("net-next: dsa: add new driver for qca8xxx family")

Signed-off-by: Yongzhao Chen <yongzhao.derek@gmail.com>
Assisted-by: LLM
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 51 ++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 60f2a615a..f9e2eb0b9 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);
@@ -799,14 +802,13 @@ qca8k_mdio_busy_wait(struct mii_bus *bus, u32 reg, u32 mask)
 
 	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;
@@ -818,7 +820,7 @@ qca8k_mdio_write(struct qca8k_priv *priv, int phy, int regnum, u16 data)
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	u32 val;
-	int ret;
+	int ret, ret1;
 
 	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);
 
@@ -856,7 +862,7 @@ qca8k_mdio_read(struct qca8k_priv *priv, int phy, int regnum)
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	u32 val;
-	int ret;
+	int ret, ret1;
 
 	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

Re: [PATCH net] net: dsa: qca8k: propagate MDIO errors

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (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] The Ethernet MDIO read path can still return a timed-out PHY
  transaction as valid register data, so the MII fallback this patch…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the patch
  reworks the code around it.

--

net: dsa: qca8k: propagate MDIO errors

This patch changes qca8k_mii_write32() to return errors from the low and
high half-word writes. Those errors are then propagated through the
regmap and internal MDIO master paths. It also stops the MII busy poll
on read errors, and has the internal MDIO read callback return the real
error instead of 0xffff.

> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 4c928983b8623..0851e4d65b81c 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c

[ ... ]

> @@ -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;

[Severity: Medium]
This problem was not introduced by this patch, but this exit path is
being reworked here. What happens when qca8k_set_page() fails just above?

    ret = qca8k_set_page(priv, page);
    if (ret)
        goto exit;

qca8k_split_addr(QCA8K_MDIO_MASTER_CTRL) gives r1 = 0x1e, r2 = 0 and
page = 0. The cleanup write to phy 0x10, reg 0x1f therefore only hits
MDIO_MASTER_CTRL[31:16] when page 0 is latched in the switch.

qca8k_set_page() returns early when the page matches the cached one. It
only updates the cache after bus->write() succeeds:

    ret = bus->write(bus, 0x18, 0, page);
    if (ret < 0) {
        ...
        return ret;
    }

    *cached_page = page;

So if qca8k_set_page() fails here, the previously selected page P was
not 0.

If the failed page selector write never reached the hardware, could the
cleanup write clear bits [31:16] of an unrelated register at
P * 0x200 + 0x3c?

No master transaction was started on this path, so there is no
MASTER_EN to clear. The result of the stray write (ret1) is also
dropped, because ret is already non-zero.

qca8k_mdio_read() has the same pattern. Could a set_page failure unlock
and return directly, or use a separate label, so that the MASTER_EN
clear only runs after page 0 has been selected?

[ ... ]

> @@ -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);
>  }

[Severity: Medium]
Can the Ethernet leg above still return a timed-out PHY read as valid
data? If so, this corrected fallback would never run.

Suppose every status read in qca8k_phy_eth_command() succeeds
(ret1 == 0), but MASTER_CTRL keeps reporting BUSY for the whole
QCA8K_BUSY_WAIT_TIMEOUT:

    ret = read_poll_timeout(qca8k_phy_eth_busy_wait, ret1,
                            !(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) {
        ret = ret1;
        goto exit;
    }

    if (read) {
        ...
        ret = mgmt_eth_data->data[0] & QCA8K_MDIO_MASTER_DATA_MASK;

Here the -ETIMEDOUT from read_poll_timeout() is ignored. ret is then
overwritten with the data field of a transaction that never completed.
qca8k_internal_mdio_read() sees ret >= 0, returns that value and skips
qca8k_mdio_read().

The commit message says:

    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.

Doesn't the same outcome remain on this Ethernet path? Should the check
after read_poll_timeout() in qca8k_phy_eth_command() also bail out when
ret < 0 and ret1 == 0?

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

Re: [PATCH net] net: dsa: qca8k: propagate MDIO errors

[Yongzhao Chen]

On Sun, 27 Sep 2026 22:12:38 +0000, Sashiko wrote:
> qca8k_mdio_read() has the same pattern. Could a set_page failure unlock
> and return directly, or use a separate label, so that the MASTER_EN
> clear only runs after page 0 has been selected?

Yes. No transaction has started at that point, so there is nothing to
clean up, and the write can hit a register on another page. The next
version returns right after a failed page select. It also invalidates
the cached page, since a failed page write may still have reached the
switch.

> Doesn't the same outcome remain on this Ethernet path? Should the check
> after read_poll_timeout() in qca8k_phy_eth_command() also bail out when
> ret < 0 and ret1 == 0?

Yes. The next version stops polling on the first failed request and
returns the poll error or the timeout, so qca8k_internal_mdio_read()
falls back to the MDIO bus. The same exit path also leaked read_skb;
that is fixed too.

Both problems predate this patch. I reproduced them with a userspace
model that compiles the driver's own functions. My board (QCA8337 with
its PHYs on the SoC MDIO bus and no CPU port 0) never uses these paths,
so they are not hardware-tested. v2 will be a three-patch series with
one patch per issue.

pw-bot: cr

Thanks,
Yongzhao