Re: [PATCH net-next v4 5/6] net: phy: Introduce Airoha AN8801R Gigabit Ethernet PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unconditional clearing of all interrupt status bits causes TOCTOU races and lost interrupts.
- [High] Unconditional LED GPIO initialization overwrites and breaks the PHY interrupt line.
- [Medium] Setting a solid LED brightness does not clear previously active hardware blink events.
- [Medium] The total blink duration can overflow the 16-bit hardware register LED_BLINK_DUR.
--

commit 54d90f97d5a09b09ae8c12ef66db44dd7f252412
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

net: phy: Introduce Airoha AN8801R Gigabit Ethernet PHY driver

Introduce a driver for the Airoha AN8801R Series Gigabit Ethernet
PHY; this currently supports setting up PHY LEDs, 10/100M, 1000M
speeds, and 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..73f94e84da07f
> --- /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)
> +{

[ ... ]

> +	if (delay_on && delay_off) {
> +		blink = true;
> +
> +		if (*delay_on == 0 || *delay_off == 0) {
> +			*delay_on = 64;
> +			*delay_off = 64;
> +		}
> +
> +		hw_delay_on = an8801r_led_blink_ms_to_hw(*delay_on);
> +		hw_delay_off = an8801r_led_blink_ms_to_hw(*delay_off);
> +	} else {
> +		blink = false;
> +	}
> +
> +	if (blink) {
> +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LED_BLINK_DUR,
> +				    (hw_delay_on + hw_delay_off) &
> +				    LED_BLINK_DUR_MASK);

[Severity: Medium]
Could the combined hw_delay_on and hw_delay_off durations overflow the 16-bit
LED_BLINK_DUR register?

The an8801r_led_blink_ms_to_hw function clamps individual delay values to
2147ms (about 65521 in hardware units). However, the sum can reach roughly
131042, exceeding the 65535 limit. When truncated by LED_BLINK_DUR_MASK,
the resulting cycle duration may be smaller than the LED_ON_DUR value.
Programming an ON duration longer than the total cycle could place the
hardware in an invalid state.

> +		if (ret)
> +			goto error;
> +
> +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LED_ON_DUR,
> +				    hw_delay_on);

[ ... ]

> +static int an8801r_led_brightness_set(struct phy_device *phydev, u8 index,
> +				      enum led_brightness value)
> +{
> +	int ret;
> +
> +	if (index >= AN8801R_NUM_LEDS)
> +		return -EINVAL;
> +
> +	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2, LED_ON_CTRL(index),
> +			     LED_ON_EVT_MASK,
> +			     (value == LED_OFF) ? 0 : LED_ON_EVT_FORCE);
> +	if (ret)
> +		return ret;
> +
> +	return phy_modify_mmd(phydev, MDIO_MMD_VEND2, LED_ON_CTRL(index),
> +			      LED_ON_EN, (value == LED_OFF) ? 0 : LED_ON_EN);
> +}

[Severity: Medium]
Is there a chance the LED might continue blinking if a solid brightness is
requested after a blink state?

This function configures LED_ON_CTRL but leaves the LED_BLINK_EVT_MASK bits
in LED_BLINK_CTRL unchanged. If hardware triggers or software blinking were
previously active, those blink events appear to remain set in the hardware.

[ ... ]

> +static int an8801r_led_init(struct phy_device *phydev, u8 *led_cfg)
> +{
> +	int led_id, ret;
> +
> +	/* Set LED BCR Enable */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, LED_BCR,
> +			       LED_BCR_EXT_CTRL | LED_BCR_CLK_EN);
> +	if (ret)
> +		return ret;
> +
> +	for (led_id = 0; led_id < AN8801R_NUM_LEDS; led_id++) {
> +		unsigned long led_trigger = 0;
> +		u32 led_gpio = led_id + 1;
> +
[ ... ]
> +		ret = an8801_buckpbus_reg_set_bits(phydev,
> +						   AN8801_BPBUS_REG_LED_GPIO,
> +						   BIT(led_gpio));

[Severity: High]
Does unconditionally configuring these GPIOs for LED usage conflict with the
PHY interrupt line?

This loop configures GPIOs 1 to 3 as LED outputs by setting the LED_GPIO
register and clearing GPIO_MODE, even if the LED is not requested in the
device tree. At the same time, an8801r_config_intr() hardcodes the PHY
interrupt to use GPIO 1. It looks like the LED initialization will override
the interrupt multiplexing and break the PHY interrupt.

> +		if (ret)
> +			return ret;
> +

[ ... ]

> +static int an8801r_ack_interrupt(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* Reset wake status */
> +	ret = an8801r_reset_wake(phydev);
> +	if (ret)
> +		return ret;
> +
> +	/* Clear the interrupts by writing the reg */
> +	return an8801_buckpbus_reg_write(phydev, AN8801_BPBUS_REG_WAKE_IRQ_STS,
> +					 AN8801_IRQ_WAKE_ALL);
> +}

[Severity: High]
Is it safe to unconditionally clear all interrupt bits here?

In an8801r_handle_interrupt(), the WAKE_IRQ_STS register is read into
irq_status, and then this function unconditionally clears all bits using
AN8801_IRQ_WAKE_ALL.

This can cause a race condition where an interrupt triggered right after the
read is permanently lost. Additionally, if irq_status is 0 (which can happen
on a shared interrupt line), it still clears the PHY interrupts. It might be
safer to only write back the specific bits that were read in irq_status.

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