Re: [PATCH net-next v3 5/6] net: phy: Introduce Airoha AN8801/R Gigabit Ethernet PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Race condition and early return in PHY interrupt handler causes permanently lost interrupts and stalled link state.
- [High] Uninitialized variable `ret` returned from `an8801r_of_init_leds()` can cause random probe failures.
- [Medium] LED blink duration calculation overflows `phy_write_mmd` 16-bit value, breaking hardware blink cycle.
- [Medium] Silent acceptance of unsupported LED triggers violates PHY LED offload API contract.
- [Medium] Link change interrupts are incorrectly left enabled during suspend if system wakeup is disabled.
--

commit 95666fa3cde2e5306e8a143ecfdca2c9d58dbf2d
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

net: phy: Introduce Airoha AN8801/R Gigabit Ethernet PHY driver

Introduce a driver for the Airoha AN8801R Series Gigabit Ethernet PHY.
This supports setting up PHY LEDs, 10/100M, 1000M speeds, Wake on LAN
and PHY interrupts.

> diff --git a/drivers/net/phy/air_an8801.c b/drivers/net/phy/air_an8801.c
> new file mode 100644
> index 0000000000000..a0f164cbf92bc
> --- /dev/null
> +++ b/drivers/net/phy/air_an8801.c
[ ... ]
> +static int an8801r_led_blink_set(struct phy_device *phydev, u8 index,
> +				 unsigned long *delay_on,
> +				 unsigned long *delay_off)
> +{
> +	u32 hw_delay_on, hw_delay_off;
> +	bool blink;
> +	int ret;
[ ... ]
> +	if (blink) {
> +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LED_BLINK_DUR,
> +				    hw_delay_on + hw_delay_off);

Is it possible for the sum of hw_delay_on and hw_delay_off to overflow here?

Looking at an8801r_led_blink_ms_to_hw(), the delays can be up to 65521.
If their sum is greater than 65535, it will be silently truncated since
phy_write_mmd() takes a u16 for the value argument. 

Because LED_ON_DUR is then written as the non-truncated hw_delay_on, could
this result in the ON duration exceeding the total blink period and breaking
the hardware blink cycle?

> +		if (ret)
> +			goto error;
> +
> +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LED_ON_DUR,
> +				    hw_delay_on);
> +		if (ret)
> +			goto error;
> +	}
[ ... ]
> +static int an8801r_led_trig_to_hw(unsigned long rules, u16 *on, u16 *blink)
> +{
[ ... ]
> +	if (test_bit(TRIGGER_NETDEV_TX, &rules)) {
> +		*blink |= LED_BLINK_EVT_10M_TX;
> +		*blink |= LED_BLINK_EVT_100M_TX;
> +		*blink |= LED_BLINK_EVT_1000M_TX;
> +	}
> +
> +	if (test_bit(TRIGGER_NETDEV_RX_ERR, &rules))
> +		*blink |= LED_BLINK_EVT_RX_CRC_ERR;
> +
> +	if (rules && !*on && !*blink)
> +		return -EOPNOTSUPP;

Does this correctly reject unsupported LED triggers when mixed with supported
ones?

If the requested rules contain a combination of supported triggers and
unsupported triggers, *on or *blink will be non-zero. The condition will
evaluate to false and the function will return 0. 

Does this silently drop the unsupported portion of the rules while falsely
reporting to the LED subsystem that the entire offload combination is
supported?

> +
> +	return 0;
> +}
[ ... ]
> +static int an8801r_of_init_leds(struct phy_device *phydev, u8 *led_cfg)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *leds;
> +	u32 function_enum_idx;
> +	int ret;
> +
> +	if (!np)
> +		return 0;
> +
> +	/* If devicetree is present, leds configuration is required */
> +	leds = of_get_child_by_name(np, "leds");
> +	if (!leds)
> +		return 0;
> +
> +	for_each_available_child_of_node_scoped(leds, led) {
> +		u32 led_idx;
> +
> +		ret = of_property_read_u32(led, "reg", &led_idx);
[ ... ]
> +		led_cfg[led_idx] = function_enum_idx;
> +	}
> +out:
> +	of_node_put(leds);
> +	return ret;

Can ret be returned uninitialized here?

If a leds node exists in the device tree but has no available children, the
for_each_available_child_of_node_scoped loop body will never be executed.

The function would then jump to the out label and return the uninitialized
stack value of ret, which could cause random probe failures.

> +}
[ ... ]
> +static irqreturn_t an8801r_handle_interrupt(struct phy_device *phydev)
> +{
> +	u32 irq_status = 0;
> +	int ret;
> +
> +	ret = an8801_buckpbus_reg_read(phydev, AN8801_BPBUS_REG_WAKE_IRQ_STS,
> +				       &irq_status);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	ret = an8801r_ack_interrupt(phydev);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	if (irq_status & AN8801_IRQ_WAKE_MAGICPKT) {
> +		pm_wakeup_event(&phydev->mdio.dev, 0);
> +		return IRQ_HANDLED;
> +	}
> +
> +	if (irq_status & AN8801_IRQ_WAKE_LNKCHG) {
> +		phy_trigger_machine(phydev);
> +		return IRQ_HANDLED;
> +	}

Can this lead to permanently lost link change interrupts?

The an8801r_ack_interrupt() function unconditionally writes
AN8801_IRQ_WAKE_ALL to clear all pending PHY interrupts in hardware.

If irq_status has both AN8801_IRQ_WAKE_MAGICPKT and AN8801_IRQ_WAKE_LNKCHG
set, the function returns IRQ_HANDLED early for the magic packet.

Since the link change event would be skipped here but was already globally
cleared in hardware, it seems phy_trigger_machine() will never run,
potentially stalling the PHY link state.

> +
> +	return IRQ_NONE;
> +}
[ ... ]
> +static int an8801r_suspend(struct phy_device *phydev)
> +{
> +	struct an8801r_priv *priv = phydev->priv;
> +	int ret;
> +
> +	/* If the PHY may wake up by a wake-on-line event, disable the link
> +	 * interrupt to only keep the  WOL magic interrupt enabled
> +	 */
> +	if (device_may_wakeup(&phydev->mdio.dev)) {
> +		priv->wake_lnkchg_enabled = false;
> +
> +		if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> +			ret = an8801_buckpbus_reg_clear_bits(phydev,
> +							     AN8801_BPBUS_REG_WAKE_IRQ_EN,
> +							     AN8801_IRQ_WAKE_LNKCHG);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		/* Reset Wol status */
> +		ret = an8801r_reset_wake(phydev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (!phydev->wol_enabled)
> +		return genphy_suspend(phydev);

Are link change interrupts incorrectly left enabled during suspend if system
wakeup is disabled?

If wake-on-LAN is enabled via ethtool (wol_enabled is true) but system
wakeup is disabled via sysfs (device_may_wakeup is false), the block
disabling the LNKCHG interrupt is skipped. Since wol_enabled is true,
genphy_suspend is also skipped.

Will this leave the PHY awake with the link change interrupt remaining
active, potentially causing spurious IRQs or interrupt storms if the link
changes while the system is suspending?

> +
> +	return 0;
> +}

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