Re: [PATCH net v9 1/4] net: usb: lan78xx: register the PHY interrupt with the MDIO bus

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Commit message hides a user-visible precedence inversion and
  is factually wrong about it.

--

net: usb: lan78xx: register the PHY interrupt with the MDIO bus

The driver's mapped PHY interrupt number is now written into every entry of
mdiobus->irq[] in lan78xx_mdio_init() before of_mdiobus_register(), and the
later assignment to phydev->irq in lan78xx_phy_init() is removed so that
phy_device_create() seeds the number from the bus table instead.

> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index cb782d81d84f..d7472d894c8d 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -2092,6 +2092,7 @@ static int lan78xx_mdio_init(struct lan78xx_net *dev)
>  {
>  	struct device_node *node;
>  	int ret;
> +	int i;
>  
>  	dev->mdiobus = mdiobus_alloc();
>  	if (!dev->mdiobus) {
> @@ -2118,6 +2119,10 @@ static int lan78xx_mdio_init(struct lan78xx_net *dev)
>  		break;
>  	}
>  
> +	if (dev->domain_data.phyirq > 0)
> +		for (i = 0; i < PHY_MAX_ADDR; i++)
> +			dev->mdiobus->irq[i] = dev->domain_data.phyirq;
> +
>  	node = of_get_child_by_name(dev->udev->dev.of_node, "mdio");
>  	ret = of_mdiobus_register(dev->mdiobus, node);
>  	of_node_put(node);

[ ... ]

> @@ -2892,13 +2897,6 @@ static int lan78xx_phy_init(struct lan78xx_net *dev)
>  		return 0;
>  	}
>  
> -	/* if phyirq is not set, use polling mode in phylib */
> -	if (dev->domain_data.phyirq > 0)
> -		phydev->irq = dev->domain_data.phyirq;
> -	else
> -		phydev->irq = PHY_POLL;
> -	netdev_dbg(dev->net, "phydev->irq = %d\n", phydev->irq);
> -
>  	ret = phylink_connect_phy(dev->phylink, phydev);

[Severity: Medium]
Does moving the number into mdiobus->irq[] change which value ends up in
phydev->irq for LAN7800/LAN7850?

Before this patch the assignment above ran after of_mdiobus_register() had
created and probed the PHY, so the driver's number was written last. Now
the number is present at scan time, and phy_probe() runs after it:

drivers/net/phy/phy_device.c:phy_probe() {
	...
	if (!phy_drv_supports_irq(phydev->drv) && phy_interrupt_is_valid(phydev))
		phydev->irq = PHY_POLL;
	...
}

The lan88xx entry in drivers/net/phy/microchip.c deliberately defines no
interrupt callbacks:

	/* Interrupt handling is broken, do not define related
	 * functions to force polling.
	 */

So for the internal PHY at address 1 (phy_mask = ~(1 << 1)) phydev->irq is
seeded from the table, phy_probe() sees a valid interrupt with a driver that
does not support one, and demotes it back to PHY_POLL. The old late write in
lan78xx_phy_init() happened after phy_probe() and defeated that demotion.
Does this move LAN7515 boards such as Raspberry Pi 3B+/4 from the chip's
INT_EP_PHY interrupt to phylib polling?

If so, the post-commit note "Neither in-tree lan78xx PHY node carries an
interrupts property, so nothing in tree changes" does not cover this path,
since the change here comes from phy_probe() rather than from devicetree.

The commit message also says:

	A devicetree PHY node still overrides that.

Is "still" accurate? Previously fwnode_mdiobus_phy_device_register() wrote a
devicetree interrupt into both phy->irq and mdio->irq[addr]:

drivers/net/mdio/fwnode_mdio.c:fwnode_mdiobus_phy_device_register() {
	...
	if (rc > 0) {
		phy->irq = rc;
		mdio->irq[addr] = rc;
	} else {
		phy->irq = mdio->irq[addr];
	}
	...
}

and lan78xx_phy_init() then overwrote phy->irq with the driver's number, so
the devicetree value was discarded. After this patch the devicetree value
wins. That is a precedence inversion rather than unchanged behaviour, so
could the commit body state it directly?

The explanation of all of this currently lives in the notes below the ---
line, which is dropped when the patch is applied. Could the permanent commit
message carry both behaviour changes, and could the "On its own it fixes
nothing" reasoning for omitting a Fixes tag be revisited given that
LAN7800/LAN7850 interrupt handling changes with this patch alone?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919015326.499479-1-f%40lex.la