Re: [PATCH net v4] 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: 1 · Low: 1

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 fix covers only the QCA8337 entry of qca83xx_driver[] in
  drivers/net/phy/qcom/qca83xx.c.
- [Low] In qca8337_read_status() (drivers/net/phy/qcom/qca83xx.c), the
  link is forced down when at803x_read_specific_status() leaves speed or…

--

net: phy: qca83xx: read resolved QCA8337 link status

This adds a read_status callback for the QCA8337 internal PHY. It wraps
genphy_read_status() and, when an autonegotiated link comes up, replaces
speed and duplex with the values from at803x_read_specific_status(). It
also reports the link as down until the PHY-specific status says it is
resolved.

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

[ ... ]

> +	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete &&
> +	    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;
> +
> +		/* Report the link down until speed and duplex are resolved,
> +		 * so that the next poll reads them again.
> +		 */
> +		if (phydev->speed == SPEED_UNKNOWN ||
> +		    phydev->duplex == DUPLEX_UNKNOWN) {
> +			phydev->link = 0;
> +			phydev->mdix = ETH_TP_MDI_INVALID;

[Severity: Low]
Should more of the link-up state be cleared here?

By the time qca8337_read_status() forces link to 0, genphy_read_status()
has already run with link=1 and autoneg_complete=1. That means:

  genphy_read_master_slave() has set master_slave_state to MASTER or SLAVE
  genphy_read_lpa() has filled lp_advertising

There is also the case where the 2-bit speed field holds the reserved
value 3. There, at803x_read_specific_status() leaves speed as
SPEED_UNKNOWN but still sets duplex to DUPLEX_FULL or DUPLEX_HALF.

Only link and mdix are reset. Until the next poll, could ethtool report
"Link detected: no" together with a master/slave state, a link partner
advertisement and possibly a concrete duplex?

The commit message names marvell_read_status_page_an() as the model. Its
caller, marvell_read_status_page(), clears lp_advertising first:

drivers/net/phy/marvell.c:marvell_read_status_page() {
    ...
	linkmode_zero(phydev->lp_advertising);
	phydev->pause = 0;
	phydev->asym_pause = 0;
	phydev->speed = SPEED_UNKNOWN;
	phydev->duplex = DUPLEX_UNKNOWN;
    ...
}

The PHY_HALTED path in _phy_state_machine() also resets
master_slave_state, mdix and lp_advertising together when it forces the
link down.

> +		} else {
> +			phy_resolve_aneg_pause(phydev);
> +		}
> +	}

[ ... ]

> @@ -220,6 +272,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]
Does the QCA8327 internal PHY need the same fix?

In the same qca83xx_driver[] table, the QCA8327-A and QCA8327-B entries
still have no .read_status, so phylib uses genphy_read_status() for them.

If those PHYs can also do a SmartSpeed downshift, the same path would
report 1000 Mbit/s while the PHY runs at 100 Mbit/s:

genphy_read_status()
  phy_resolve_aneg_linkmode()     picks 1000
phylink
  qca8k_phylink_mac_link_up()
    case SPEED_1000:
        reg = QCA8K_PORT_STATUS_SPEED_1000;

The Fixes: commit (the qca8k switch to phylink) covers every qca8k
switch, not only QCA8337.

On QCA8327 the wrong speed has a second user.
qca83xx_link_change_notify() only applies the 100M DAC amplitude
adjustment when phydev->speed is SPEED_100:

drivers/net/phy/qcom/qca83xx.c:qca83xx_link_change_notify() {
	if (phydev->state == PHY_RUNNING) {
		if (phydev->speed == SPEED_100)
			at803x_debug_reg_mask(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL,
					      QCA8327_DEBUG_MANU_CTRL_EN,
					      QCA8327_DEBUG_MANU_CTRL_EN);
    ...
}

If the link is misreported as 1000, would that adjustment be skipped?

The commit message does not mention QCA8327. Could it say whether these
PHYs can downshift, or why they were left out?

>  	.get_sset_count		= qca83xx_get_sset_count,
>  	.get_strings		= qca83xx_get_strings,
>  	.get_stats		= qca83xx_get_stats,

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