[PATCH net v3] net: phy: qca83xx: read resolved QCA8337 link status

[PATCH net v3] net: phy: qca83xx: read resolved QCA8337 link status

[Yongzhao Chen]

After a QCA8337 SmartSpeed downshift, genphy_read_status() can report
1000 Mbit/s from the advertised modes when the PHY is running at
100 Mbit/s. On a Redmi AX5400 running downstream OpenWrt Linux 6.18.52,
this caused qca8k to program the user-port MAC for 1000 Mbit/s and
traffic failed. An earlier prototype that read the resolved speed
from the PHY restored traffic.

Wrap genphy_read_status() and use at803x_read_specific_status() to
replace its speed and duplex result when autonegotiation is complete
and the link has not stayed up. Resolve pause for the resulting duplex.
QCA8337's register layout matches the helper (data sheet 80-Y0619-3
Rev. D, pages 327-328).

Keep the generic master/slave read order and the early return for a
steady autonegotiated link. This avoids additional reads of the
clear-on-read idle error count in MII_STAT1000. Forced mode continues
to use BMCR. Report the MDI-X state, but clear mdix_ctrl because this
driver does not implement MDI-X configuration.

Fixes: b3591c2a3661 ("net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB")
Suggested-by: Andrew Lunn <andrew@lunn.ch>
Assisted-by: LLM
Signed-off-by: Yongzhao Chen <yongzhao.derek@gmail.com>
---
v3:
 - Keep genphy_read_status() instead of calling at803x_read_status()
   followed by genphy_read_master_slave(). This restores the generic
   MII_STAT1000 read frequency and master/slave-before-LPA order,
   addressing the idle-error and latched-fault issues raised by Sashiko.
 - Report mdix_ctrl as ETH_TP_MDI_INVALID, including after a steady
   poll, since QCA8337 has no MDI-X configuration callback.
 - Keep the generic BMCR result in forced mode. QCA8337's resolved-bit
   behavior in that mode has not been verified.

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

Testing: the source-extraction model passes 26 checks covering speed
and duplex at the phylib notification boundary, latch-low transitions,
pause, forced mode, unresolved/reserved status, MDIO errors and the
three reported issues. The baseline fails the downshift checks; v2
reproduces the idle-error loss, one-shot MSFAIL ordering difference and
valid mdix_ctrl report. The MDIO model follows the read-clear and
saturating-counter definitions in Table 5-352, pages 324-325. An arm64
W=1 build of qca83xx.o, at803x.o and qcom-phy-lib.o is clean.

v3 has not been tested on hardware. The hardware result above used
v2's at803x_read_status() plus genphy_read_master_slave() on one board,
with an Intel igb peer, a two-pair cable and default autonegotiation.
Ping changed from 0/20 to 20/20 in each direction at resolved 100 Full.
No idle errors or MSFAIL were injected on hardware; whether MSFAIL can
remain latched while AN complete is set is still unknown.

 drivers/net/phy/qcom/qca83xx.c | 37 ++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/net/phy/qcom/qca83xx.c b/drivers/net/phy/qcom/qca83xx.c
index bc70ed8efd8..e70da092f42 100644
--- a/drivers/net/phy/qcom/qca83xx.c
+++ b/drivers/net/phy/qcom/qca83xx.c
@@ -92,6 +92,42 @@ static int qca83xx_probe(struct phy_device *phydev)
 	return 0;
 }
 
+static int qca8337_read_status(struct phy_device *phydev)
+{
+	struct at803x_ss_mask ss_mask = {
+		.speed_mask = AT803X_SS_SPEED_MASK,
+		.speed_shift = __bf_shf(AT803X_SS_SPEED_MASK),
+	};
+	int ret, old_link = phydev->link;
+
+	ret = genphy_read_status(phydev);
+	if (ret)
+		return ret;
+
+	/* genphy_read_status() derives speed and duplex from the
+	 * advertisements, which is wrong after a SmartSpeed downshift.
+	 * Whenever it has just done so, use what the PHY resolved instead.
+	 */
+	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete &&
+	    !(old_link && phydev->link)) {
+		phydev->speed = SPEED_UNKNOWN;
+		phydev->duplex = DUPLEX_UNKNOWN;
+		phydev->pause = false;
+		phydev->asym_pause = false;
+
+		ret = at803x_read_specific_status(phydev, ss_mask);
+		if (ret)
+			return ret;
+
+		phy_resolve_aneg_pause(phydev);
+	}
+
+	/* MDI-X configuration is not implemented */
+	phydev->mdix_ctrl = ETH_TP_MDI_INVALID;
+
+	return 0;
+}
+
 static int qca83xx_config_init(struct phy_device *phydev)
 {
 	u8 switch_revision;
@@ -220,6 +256,7 @@ static struct phy_driver qca83xx_driver[] = {
 	.flags			= PHY_IS_INTERNAL,
 	.config_init		= qca83xx_config_init,
 	.soft_reset		= genphy_soft_reset,
+	.read_status		= qca8337_read_status,
 	.get_sset_count		= qca83xx_get_sset_count,
 	.get_strings		= qca83xx_get_strings,
 	.get_stats		= qca83xx_get_stats,

base-commit: 6dc989ea46b96ce170840174b4a38c4a387fb005
-- 
2.43.0

Re: [PATCH net v3] net: phy: qca83xx: read resolved QCA8337 link status

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · 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] In qca8337_read_status()
  (drivers/net/phy/qcom/qca83xx.c:111-123), the link-up poll throws away…
- [Medium] qca8337_read_status() now fills phydev->mdix through
  at803x_read_specific_status(), but only in the autoneg-complete, link-…

--

net: phy: qca83xx: read resolved QCA8337 link status

This adds a read_status callback for the QCA8337 internal PHY. The callback
wraps genphy_read_status() and, on an autoneg link transition, replaces
speed and duplex with the values from the PHY-specific status register via
at803x_read_specific_status(). It also resolves pause for the resulting
duplex and reports mdix_ctrl as ETH_TP_MDI_INVALID.

> diff --git a/drivers/net/phy/qcom/qca83xx.c b/drivers/net/phy/qcom/qca83xx.c
> index bc70ed8efd86a..e70da092f424f 100644
> --- a/drivers/net/phy/qcom/qca83xx.c
> +++ b/drivers/net/phy/qcom/qca83xx.c
> @@ -92,6 +92,42 @@ static int qca83xx_probe(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int qca8337_read_status(struct phy_device *phydev)
> +{

[ ... ]

> +	ret = genphy_read_status(phydev);
> +	if (ret)
> +		return ret;
> +
> +	/* genphy_read_status() derives speed and duplex from the
> +	 * advertisements, which is wrong after a SmartSpeed downshift.
> +	 * Whenever it has just done so, use what the PHY resolved instead.
> +	 */
> +	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete &&
> +	    !(old_link && phydev->link)) {
> +		phydev->speed = SPEED_UNKNOWN;
> +		phydev->duplex = DUPLEX_UNKNOWN;
> +		phydev->pause = false;
> +		phydev->asym_pause = false;
> +
> +		ret = at803x_read_specific_status(phydev, ss_mask);
> +		if (ret)
> +			return ret;

[Severity: Medium]
Suppose BMSR shows link up and autoneg complete before
AT803X_SS_SPEED_DUPLEX_RESOLVED is set in AT803X_SPECIFIC_STATUS. What ends
up in phydev->speed and phydev->duplex in that case?

at803x_read_specific_status() only writes speed and duplex inside this
block, and otherwise returns 0:

drivers/net/phy/qcom/qcom-phy-lib.c:at803x_read_specific_status() {
    ...
	if (ss & AT803X_SS_SPEED_DUPLEX_RESOLVED) {
		...
	}

	return 0;
}

The speed switch also has no case for the reserved field value 3.
QCA808X_SS_SPEED_2500 is 4, and a 2-bit field can never hold 4.

In both cases the genphy result has already been replaced with
SPEED_UNKNOWN (and DUPLEX_UNKNOWN if the resolved bit is clear). The wrapper
then returns 0 with link set, and phy_check_link_status() calls
phy_link_up() without checking speed or duplex.

On the next poll old_link && phydev->link is true, so genphy_read_status()
returns early:

	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
		return 0;

The !(old_link && phydev->link) check here also skips the override. Would
the unknown speed and duplex then stay until the link drops?

With phylink, qca8k_phylink_mac_link_up() would get SPEED_UNKNOWN and fall
into its default case (QCA8K_PORT_STATUS_LINK_AUTO). The duplex bit would
not be set and flow control would stay off.

I couldn't confirm that QCA8337 can actually show link up before the
resolved bit is set, and the changelog says v3 has not been tested on
hardware. However, marvell_read_status_page_an() handles the same register
layout by forcing link down while the resolved bit is clear:

	if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) {
		phydev->link = 0;
		return 0;
	}

Commit 3b72f84f8fb6 added that check because the datasheet does not specify
the timing between link status and the "Speed and Duplex Resolved" bit.

qca808x_read_status() takes a different approach. It does not clear the
genphy result before calling at803x_read_specific_status(), so an
unresolved read keeps a valid fallback.

Would either approach fit here?

> +
> +		phy_resolve_aneg_pause(phydev);
> +	}
> +
> +	/* MDI-X configuration is not implemented */
> +	phydev->mdix_ctrl = ETH_TP_MDI_INVALID;

[Severity: Medium]
at803x_read_specific_status() now fills in phydev->mdix as well. Can that
value go stale?

It is only refreshed inside the autoneg link-transition block above.
genphy_read_status_fixed() never touches mdix. In phylib, mdix is only reset
to ETH_TP_MDI_INVALID in the PHY_HALTED state.

So once an autoneg link has reported MDI or MDI-X, the old value would stay
in two cases:

  - The link drops. autoneg_complete or the resolved bit is then clear, so
    the helper leaves mdix alone.

  - Autoneg is turned off with ethtool. The override block is then skipped
    on every poll.

phy_ethtool_ksettings_get() copies phydev->mdix straight into eth_tp_mdix.
The ethtool uAPI says that field should be ETH_TP_MDI_INVALID when the
status is unknown or not applicable.

Before this patch, the driver never set mdix, so it stayed
ETH_TP_MDI_INVALID.

Should mdix also be reset to ETH_TP_MDI_INVALID when the resolved status is
not read?

> +
> +	return 0;
> +}

[ ... ]

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

Re: [PATCH net v3] net: phy: qca83xx: read resolved QCA8337 link status

[Yongzhao Chen]

On Thu, 08 Oct 2026 16:03:44 +0000 netdev-bot+sashiko@kernel.org wrote:
> Suppose BMSR shows link up and autoneg complete before
> AT803X_SS_SPEED_DUPLEX_RESOLVED is set in AT803X_SPECIFIC_STATUS. What ends
> up in phydev->speed and phydev->duplex in that case?

Both points are valid. Thanks.

In that case v3 reports the link up with SPEED_UNKNOWN, and the early
return in genphy_read_status() keeps it that way until the link
drops. The reserved speed value ends up the same way. I don't know
whether QCA8337 can report link up before the resolved bit is set,
but the driver should not depend on that.

v4 follows marvell_read_status_page_an(): if speed or duplex is still
unknown after at803x_read_specific_status(), it reports the link as
down. This driver has no interrupt support, so phylib polls the PHY
and reads the status again on the next poll. The qca808x approach does
not fit here. The value it falls back to is the advertisement-derived
speed this patch replaces, so after a downshift it would keep
1000 Mbit/s.

> Should mdix also be reset to ETH_TP_MDI_INVALID when the resolved status is
> not read?

Yes. v4 sets mdix to ETH_TP_MDI_INVALID on every poll except during
a steady autonegotiated link and reads it again only at link-up. That
covers both a link drop and autonegotiation being disabled.

at803x_read_status(), used by the other AT803x PHYs, clears speed and
duplex the same way before it calls the helper. I have not changed it
in this patch.

The model test now covers both cases, and v3 fails them. v4 has not
been tested on hardware. I will send it separately.

Thanks,
Yongzhao Chen

pw-bot: cr

Re: [PATCH net v3] net: phy: qca83xx: read resolved QCA8337 link status

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: phy: qca83xx: read resolved QCA8337 link status

This adds a read_status callback for the QCA8337 internal PHY. The callback
wraps genphy_read_status() and, on an autoneg link transition, replaces
speed and duplex with the values from the PHY-specific status register via
at803x_read_specific_status(). It also resolves pause for the resulting
duplex and reports mdix_ctrl as ETH_TP_MDI_INVALID.

> diff --git a/drivers/net/phy/qcom/qca83xx.c b/drivers/net/phy/qcom/qca83xx.c
> index bc70ed8efd86a..e70da092f424f 100644
> --- a/drivers/net/phy/qcom/qca83xx.c
> +++ b/drivers/net/phy/qcom/qca83xx.c
> @@ -92,6 +92,42 @@ static int qca83xx_probe(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int qca8337_read_status(struct phy_device *phydev)
> +{

[ ... ]

> +	ret = genphy_read_status(phydev);
> +	if (ret)
> +		return ret;
> +
> +	/* genphy_read_status() derives speed and duplex from the
> +	 * advertisements, which is wrong after a SmartSpeed downshift.
> +	 * Whenever it has just done so, use what the PHY resolved instead.
> +	 */
> +	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete &&
> +	    !(old_link && phydev->link)) {
> +		phydev->speed = SPEED_UNKNOWN;
> +		phydev->duplex = DUPLEX_UNKNOWN;
> +		phydev->pause = false;
> +		phydev->asym_pause = false;
> +
> +		ret = at803x_read_specific_status(phydev, ss_mask);
> +		if (ret)
> +			return ret;

[Severity: Medium]
Suppose BMSR shows link up and autoneg complete before
AT803X_SS_SPEED_DUPLEX_RESOLVED is set in AT803X_SPECIFIC_STATUS. What ends
up in phydev->speed and phydev->duplex in that case?

at803x_read_specific_status() only writes speed and duplex inside this
block, and otherwise returns 0:

drivers/net/phy/qcom/qcom-phy-lib.c:at803x_read_specific_status() {
    ...
	if (ss & AT803X_SS_SPEED_DUPLEX_RESOLVED) {
		...
	}

	return 0;
}

The speed switch also has no case for the reserved field value 3.
QCA808X_SS_SPEED_2500 is 4, and a 2-bit field can never hold 4.

In both cases the genphy result has already been replaced with
SPEED_UNKNOWN (and DUPLEX_UNKNOWN if the resolved bit is clear). The wrapper
then returns 0 with link set, and phy_check_link_status() calls
phy_link_up() without checking speed or duplex.

On the next poll old_link && phydev->link is true, so genphy_read_status()
returns early:

	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
		return 0;

The !(old_link && phydev->link) check here also skips the override. Would
the unknown speed and duplex then stay until the link drops?

With phylink, qca8k_phylink_mac_link_up() would get SPEED_UNKNOWN and fall
into its default case (QCA8K_PORT_STATUS_LINK_AUTO). The duplex bit would
not be set and flow control would stay off.

I couldn't confirm that QCA8337 can actually show link up before the
resolved bit is set, and the changelog says v3 has not been tested on
hardware. However, marvell_read_status_page_an() handles the same register
layout by forcing link down while the resolved bit is clear:

	if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) {
		phydev->link = 0;
		return 0;
	}

Commit 3b72f84f8fb6 added that check because the datasheet does not specify
the timing between link status and the "Speed and Duplex Resolved" bit.

qca808x_read_status() takes a different approach. It does not clear the
genphy result before calling at803x_read_specific_status(), so an
unresolved read keeps a valid fallback.

Would either approach fit here?

> +
> +		phy_resolve_aneg_pause(phydev);
> +	}
> +
> +	/* MDI-X configuration is not implemented */
> +	phydev->mdix_ctrl = ETH_TP_MDI_INVALID;

[Severity: Medium]
at803x_read_specific_status() now fills in phydev->mdix as well. Can that
value go stale?

It is only refreshed inside the autoneg link-transition block above.
genphy_read_status_fixed() never touches mdix. In phylib, mdix is only reset
to ETH_TP_MDI_INVALID in the PHY_HALTED state.

So once an autoneg link has reported MDI or MDI-X, the old value would stay
in two cases:

  - The link drops. autoneg_complete or the resolved bit is then clear, so
    the helper leaves mdix alone.

  - Autoneg is turned off with ethtool. The override block is then skipped
    on every poll.

phy_ethtool_ksettings_get() copies phydev->mdix straight into eth_tp_mdix.
The ethtool uAPI says that field should be ETH_TP_MDI_INVALID when the
status is unknown or not applicable.

Before this patch, the driver never set mdix, so it stayed
ETH_TP_MDI_INVALID.

Should mdix also be reset to ETH_TP_MDI_INVALID when the resolved status is
not read?

> +
> +	return 0;
> +}

[ ... ]

Re: [PATCH net v3] net: phy: qca83xx: read resolved QCA8337 link status

[Jakub Kicinski]

On Thu,  8 Oct 2026 09:05:58 -0700 Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.

Ugh, the auto-email did come out in the end, ignore this duplicate and
pls respond to the other email.