Re: [PATCH net-next v3 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 2 potential issue(s) to consider:
- [High] Hardware link mode configuration is permanently bypassed when autonegotiation is disabled or during rapid link flaps.
- [High] Hardware and software state permanently desynchronize on an MDIO write error.
--

commit 21302fcf08bdf185bc4b77e3f3a5329b1531d7b0
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 to configure the function mode register for 1Gbps or 100M/10M operation
after link speed detection.

> diff --git a/drivers/net/phy/air_an8801.c b/drivers/net/phy/air_an8801.c
> index a0f164cbf92bc..af46951b1a33d 100644
> --- a/drivers/net/phy/air_an8801.c
> +++ b/drivers/net/phy/air_an8801.c

[ ... ]

> +static int an8801r_read_status(struct phy_device *phydev)
> +{
> +	int prev_speed, ret;
> +	u32 val;
> +
> +	prev_speed = phydev->speed;
> +
> +	ret = genphy_read_status(phydev);
> +	if (ret)
> +		return ret;
> +
> +	if (phydev->link && prev_speed != phydev->speed) {

Will the hardware link mode configuration be bypassed when autonegotiation
is disabled or during rapid link flaps?

In forced speed mode (AUTONEG_DISABLE), genphy_read_status() delegates to
genphy_read_status_fixed(), which unconditionally sets phydev->speed to the
forced speed even while the link is physically down.

While the cable is unplugged, the state machine polls read_status, and
prev_speed captures the forced speed. When the link finally comes up,
phydev->link becomes true, but prev_speed already matches phydev->speed.
The check evaluates to false, and the register write is bypassed.

Similarly, if the PHY loses hardware state but the link quickly comes back
before the state machine detects a link-down event, phydev->speed remains
unchanged in memory, and the configuration write is skipped.

Should the driver track link state transitions independently by capturing
the old link state before the call (e.g., old_link = phydev->link) and
checking if the link just came up?

> +		/* 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;
> +
> +		return an8801_buckpbus_reg_rmw(phydev,
> +					       AN8801_BPBUS_REG_LINK_MODE,
> +					       AN8801_BPBUS_LINK_MODE_1000,
> +					       val);

Can the hardware and software states permanently desynchronize if the
MDIO write fails?

If an8801_buckpbus_reg_rmw() returns an error, phydev->speed has already
been mutated by genphy_read_status(). During the next execution of the
PHY state machine polling cycle, prev_speed will be assigned this
already-updated speed.

Consequently, the prev_speed != phydev->speed condition will evaluate to
false, and the failed hardware update will never be retried, leaving the
PHY hardware permanently misconfigured for the current speed.

Does the driver need to restore phydev->speed = prev_speed on failure to
ensure a retry on the next poll?

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

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