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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The PHY interrupt handler unconditionally acknowledges all interrupts, leading to lost events and breaking shared IRQ lines.
- [Medium] LED blink duration logic incorrectly caps the period without scaling the ON duration, breaking duty cycles for large periods.
--

commit 32eee39b7254042921bcc2f3287430a78ed383c7
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

net: phy: Introduce Airoha AN8801R Gigabit Ethernet PHY driver

This commit introduces a new PHY driver for the Airoha AN8801R Series Gigabit
Ethernet PHY. It adds support for configuring PHY LEDs, link speeds (10/100M
and 1000M), Wake on LAN capabilities, 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..aa24bb182d645
> --- /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 (blink) {
> +		blink_dur = (u16)min(hw_delay_on + hw_delay_off,
> +				     LED_BLINK_DUR_MASK);

[Severity: Medium]
Will this distort the duty cycle for large delay values?

If the requested delays are large (e.g., 2000ms ON and 2000ms OFF), both
hw_delay_on and hw_delay_off will evaluate to 61035. The total blink_dur will
be capped to 65535, while LED_ON_DUR is programmed with the unscaled
hw_delay_on (61035) below.

This results in the hardware remaining ON for 61035 units and OFF for the
remaining 4500 units, skewing a 50/50 duty cycle into a ~93% ON duty cycle.
When capping the maximum duration, should the ON and OFF periods be scaled
proportionally?

> +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LED_BLINK_DUR,
> +				    blink_dur);
> +		if (ret)
> +			goto error;
> +
> +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, LED_ON_DUR,
> +				    hw_delay_on);
> +		if (ret)
> +			goto error;
> +	}
[ ... ]
> +static irqreturn_t an8801r_handle_interrupt(struct phy_device *phydev)
> +{
> +	u32 irq_status = 0;
> +	bool irq_handled = false;
> +	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);

[Severity: High]
Is it safe to unconditionally acknowledge interrupts here even if irq_status is
0?

This unconditionally writes AN8801_IRQ_WAKE_ALL to the status register, which
could clear and permanently lose any interrupt that fires between the status
read and this write.

Additionally, if the PHY shares an IRQ line with another device, every
interrupt generated by the other device will cause the PHY to execute a series
of expensive MDIO/PBUS writes to reset its wakeup configuration, adding latency
to the shared IRQ line and momentarily disabling PHY interrupts.

Should this return IRQ_NONE early if irq_status is 0, and only acknowledge the
specific bits that were actually read?

> +	if (ret)
> +		return IRQ_NONE;
> +
> +	if (irq_status & AN8801_IRQ_WAKE_MAGICPKT) {
> +		pm_wakeup_event(&phydev->mdio.dev, 0);
> +		irq_handled = true;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526-add-airoha-an8801-support-v5-0-01aea8dee69b@collabora.com?part=5