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