Re: [PATCH net-next v4 6/6] net: phy: air_an8801: ensure maximum available speed link use

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Returning an error from the `read_status` callback halts the PHY state machine instead of triggering a retry, and the fallback state restoration is fundamentally flawed.
--

commit 32e383d76ce14ad1627b69049f0a1d86c192aa5b
Author: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

net: phy: air_an8801: ensure maximum available speed link use

This commit implements a custom read_status callback for the Airoha AN8801R
PHY. After link detection, it configures the function mode for either 1G or
100M/10M operation to ensure the PHY uses the maximum available link speed.

> diff --git a/drivers/net/phy/air_an8801.c b/drivers/net/phy/air_an8801.c
> index 73f94e84da07f..c64fa153b7385 100644
> --- a/drivers/net/phy/air_an8801.c
> +++ b/drivers/net/phy/air_an8801.c
[ ... ]
> @@ -977,6 +977,48 @@ static int an8801r_config_init(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int an8801r_read_status(struct phy_device *phydev)
> +{
[ ... ]
> +	if (prev_speed != phydev->speed) {
> +		/* Ensure that PHY switches to 1G speed when available,
> +		 * by configuring the function mode for either 1G or 100M/10M
> +		 * operation.
> +		 * Therefore, set the link mode register, after read_status
> +		 * determines the link speed.
> +		 */
> +		val = phydev->speed == SPEED_1000 ?
> +		      AN8801_BPBUS_LINK_MODE_1000 : 0;
> +
> +		ret = an8801_buckpbus_reg_rmw(phydev,
> +					      AN8801_BPBUS_REG_LINK_MODE,
> +					      AN8801_BPBUS_LINK_MODE_1000,
> +					      val);
> +		if (ret) {
> +			/* Restore previous speed on failure to ensure this
> +			 * register setting is retried on the next poll
> +			 */
> +			phydev->speed = prev_speed;
> +			return ret;

[Severity: High]
Does returning an error here halt the PHY state machine instead of triggering
a retry?

When a read_status callback returns an error, the error propagates up to
phy_state_machine() in the PHY core. This triggers phy_error(phydev), which
transitions the PHY into the PHY_ERROR state. This brings the link down
permanently and completely stops all state machine polling.

Consequently, there will be no next poll, and the interface will be dead
until manually brought down and up by the user.

Additionally, is the retry logic structurally sound? Even if the driver
swallowed the error instead of returning it, restoring phydev->speed to
prev_speed (which is SPEED_UNKNOWN when coming from a link-down state) without
clearing phydev->link would cause a permanent failure.

On the next poll, genphy_read_status() would see that both old_link and
phydev->link are up and short-circuit:

drivers/net/phy/phy_device.c:genphy_read_status() {
	...
	/* why bother the PHY if nothing can have changed */
	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
		return 0;
	...
}

As a result, phydev->speed would be left permanently at SPEED_UNKNOWN, the
condition (prev_speed != phydev->speed) would remain false, and the register
write would be skipped forever.

> +		}
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-add-airoha-an8801-support-v4-0-1e4837d30ef4@collabora.com?part=6