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

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

[Yongzhao Chen]

After SmartSpeed lowers the speed of a link, the generic status code
still reports the advertised speed, because it derives speed from the
advertised and link partner modes.

This breaks traffic on a QCA8337 user port. With a downstream OpenWrt
6.18.52 kernel, a port connected to an Intel igb NIC over a two-pair
cable downshifted to 100 Mbit/s, but phylib reported 1000 Mbit/s. qca8k
then programmed the MAC for 1000 Mbit/s and no traffic passed. With
at803x_read_status() and genphy_read_master_slave(), as in this change,
the port reported 100 Mbit/s and traffic passed.

Use at803x_read_status() for QCA8337, so speed and duplex come from the
PHY-Specific Status register. The register layout matches what the
helper expects: speed encoding 3 is reserved and bits 9:7 read as zero
(QCA8337 data sheet 80-Y0619-3 Rev. D, page 328), and the MDI crossover
field of the function control register is the same as on AT803X (page
327).

Compared with genphy_read_status(), MDI-X is now reported. A reserved
speed encoding or an unresolved status leaves the speed at SPEED_UNKNOWN
with the link up. The new wrapper also calls genphy_read_master_slave(),
to keep the master/slave status that genphy_read_status() provides.
This refreshes master/slave status on each poll, including while the
link stays up. In forced mode, speed and duplex also come from the
PHY-Specific Status register instead of BMCR.

Fixes: b3591c2a3661 ("net: dsa: qca8k: Switch to PHYLINK instead of PHYLIB")
Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Yongzhao Chen <yongzhao.derek@gmail.com>
Assisted-by: LLM
---
v2:
 - Target net as Andrew requested and describe the observed user-port
   failure. The Fixes tag identifies the switch to phylink, after which
   the reported speed is used to program the user-port MAC.
 - Use at803x_read_status() instead of decoding the vendor status here. It
   reads that status only when the link changes, so the code that forced the
   link down for an unresolved status or a reserved speed encoding, and the
   code that cleared the state again when there is no link, is gone. That
   also answers Andrew's two questions on v1: the helper needs no special
   handling of those cases, and page 328 of the data sheet lists speed
   encoding 3 as reserved.
 - Keep the master/slave status of genphy_read_status() with a small wrapper
   around the helper.
 - Cite the data sheet pages in the commit message.
 - v1 could change speed and duplex while the link stayed up, without
   PHYLIB telling its consumers. The helper does not.

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

Testing: a model test compiles the real genphy_update_link(),
at803x_read_status(), at803x_read_specific_status(),
genphy_read_master_slave(), phy_resolve_aneg_pause() and
phy_check_link_status() against an MDIO stub. It covers speed and duplex on
link-up, no rereads of the vendor status while the link stays up, a BMSR
latch-low renegotiation, unresolved and reserved speed encodings, MDIO
errors, pause, forced mode and master/slave status. An arm64 W=1 build of
qca83xx.o, at803x.o and qcom-phy-lib.o is clean.

The hardware comparison in the commit message used one board, QCA8337
lan2 and default autonegotiation; the downshift resolved to 100 Mbit/s
full duplex. Ping was 0/20 in both directions with genphy_read_status()
and 20/20 each way with the new path. Each path ran two warm boots and
three port down/up cycles. I have not tested a cold boot, other boards,
forced mode, or an unresolved status on hardware. The latter two cases
are only modelled; whether QCA8337 sets the resolved bit on a forced
link has not been verified.

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

diff --git a/drivers/net/phy/qcom/qca83xx.c b/drivers/net/phy/qcom/qca83xx.c
index bc70ed8efd8..b8da6a41788 100644
--- a/drivers/net/phy/qcom/qca83xx.c
+++ b/drivers/net/phy/qcom/qca83xx.c
@@ -92,6 +92,17 @@ static int qca83xx_probe(struct phy_device *phydev)
 	return 0;
 }
 
+static int qca8337_read_status(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = at803x_read_status(phydev);
+	if (ret)
+		return ret;
+
+	return genphy_read_master_slave(phydev);
+}
+
 static int qca83xx_config_init(struct phy_device *phydev)
 {
 	u8 switch_revision;
@@ -220,6 +231,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 v2] net: phy: qca83xx: read resolved QCA8337 link status

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · 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] qca8337_read_status() (drivers/net/phy/qcom/qca83xx.c:95-104)
  calls genphy_read_master_slave() on every PHY poll.
- [Medium] qca8337_read_status() reverses the order of the two
  MII_STAT1000 reads compared with genphy_read_status().
- [Medium] On QCA8337, at803x_read_specific_status() now fills
  phydev->mdix_ctrl (ETH_TP_MDI / ETH_TP_MDI_X / ETH_TP_MDI_AUTO) from…

--

net: phy: qca83xx: read resolved QCA8337 link status

This adds a qca8337_read_status() wrapper for the QCA8337 internal PHY. The
wrapper calls at803x_read_status() so that speed and duplex come from the
PHY-Specific Status register after a SmartSpeed downshift. It then calls
genphy_read_master_slave() to keep reporting master/slave status.

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

[Severity: Medium]
The two MII_STAT1000 reads here happen in the opposite order from
genphy_read_status(). Could that change how a latched master/slave
configuration fault is handled?

genphy_read_status() calls genphy_read_master_slave() before
genphy_read_lpa(). That first read consumes the latched-high, self-clearing
LPA_1000MSFAIL bit, records MASTER_SLAVE_STATE_ERR and clears the latch.

In this wrapper, at803x_read_status()->genphy_read_lpa() reads
MII_STAT1000 first. If the fault is latched and autoneg_complete is set:

drivers/net/phy/phy_device.c:genphy_read_lpa() {
    ...
			if (lpagb & LPA_1000MSFAIL) {
				...
				return -ENOLINK;
			}
    ...
}

The early return above then skips genphy_read_master_slave(), so
master_slave_state is never set to MASTER_SLAVE_STATE_ERR.

The -ENOLINK goes up through phy_check_link_status() to
_phy_state_machine(). That calls phy_error_precise(), which warns and
moves the PHY to PHY_ERROR. The PHY is then powered down through
qca8337_suspend()->genphy_suspend() until an administrative down/up.

While the link is down, the master/slave read in this wrapper clears a
latched fault on every poll. So the difference only shows up when a fault
latches and autoneg completes within the same poll interval. It is not
documented whether QCA8337 can report ANEGCOMPLETE while MSFAIL is still
latched. A persistent fault ends in -ENOLINK with both the old and the
new code.

The commit message says the wrapper keeps "the master/slave status that
genphy_read_status() provides". Does that hold on this path, given that
the error state is never recorded?

> +
> +	return genphy_read_master_slave(phydev);

[Severity: Medium]
Does this clear the phy_idle_errors counter on every poll?

genphy_read_master_slave() always reads MII_STAT1000 (register 0x0a). This
call runs on every poll, including polls where at803x_read_status()
returned early because the link stayed up.

Bits 7:0 of that register hold the 1000BASE-T idle error count. IEEE
802.3 Table 40-3 defines it as clear-on-read. This driver exposes that
field as a private ethtool stat:

static struct at803x_hw_stat qca83xx_hw_stats[] = {
	{ "phy_idle_errors", 0xa, GENMASK(7, 0), PHY},
	...
};

qca83xx_get_stat() adds up the values it reads:

		val = val & stat.mask;
		priv->stats[i] += val;

That total is only correct if qca83xx_get_stat() is the only reader of
register 0x0a. qca8k does not set up PHY interrupts, so phylib polls
about once a second. Each poll now reads the count and throws it away.

Before this patch, genphy_read_status() read MII_STAT1000 only when the
link state changed. With this change, won't ethtool -S phy_idle_errors
miss nearly all idle errors?

[ ... ]

> @@ -220,6 +231,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,

[Severity: Medium]
Through at803x_read_status(), at803x_read_specific_status() now fills
phydev->mdix_ctrl from the Specific Function Control register:

drivers/net/phy/qcom/qcom-phy-lib.c:at803x_read_specific_status() {
    ...
		switch (FIELD_GET(AT803X_SFC_MDI_CROSSOVER_MODE_M, sfc)) {
		case AT803X_SFC_MANUAL_MDI:
			phydev->mdix_ctrl = ETH_TP_MDI;
    ...
}

The QCA8337 entry has no .config_aneg, so genphy_config_aneg() is used.
Nothing writes mdix_ctrl back to the crossover bits. Does userspace now
see MDI-X control as available even though it is not implemented?

Before this patch, mdix_ctrl stayed at ETH_TP_MDI_INVALID, so
phy_ethtool_ksettings_get() reported MDI-X control as unsupported. Now it
reports a valid value.

phy_ethtool_ksettings_set() then accepts "ethtool -s <dev> mdix on|off",
stores the value and restarts aneg, but the hardware does not change. On
the next link transition, at803x_read_status() overwrites mdix_ctrl from
the SFC register, and the user's setting is dropped without an error.

The eth_tp_mdix_ctrl description in include/uapi/linux/ethtool.h says
that if MDI(-X) control is not implemented, reads yield
ETH_TP_MDI_INVALID.

The at803x.c entries that use at803x_read_status() (AR8035, AR9331,
QCA9561) pair it with at803x_config_aneg(), which applies mdix_ctrl
through at803x_config_mdix(). Should QCA8337 get a similar .config_aneg,
or should mdix_ctrl not be reported for this PHY?

IPQ5018 already has the same gap, but this patch adds QCA8337 to it.

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