Re: [PATCH net-next v2 2/6] r8169: create a virtual interrupt for linkchg

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
r8169: create a virtual interrupt for linkchg

Creates a virtual interrupt for linkchg. To support phylink, we should try
to decouple most of tp->phydev, so we add virtual interrupt for mac
interrupt to inform the change of link status.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index c60710f9bd21f..560f987437b63 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
[ ... ]
> @@ -4869,7 +4872,7 @@ static irqreturn_t rtl8169_interrupt(int irq, void *dev_instance)
>  	}
>  
>  	if (status & LinkChg)
> -		phy_mac_interrupt(tp->phydev);
> +		generic_handle_domain_irq(tp->phy_irq_domain, 0);

[Severity: High]
Does this actually work as intended without the PHY driver implementing the
interrupt callbacks?

Because the Realtek PHY drivers don't implement .config_intr and
.handle_interrupt, phy_drv_supports_irq() will return false, causing
phy_probe() to force phydev->irq to PHY_POLL.

Will this cause the virtual interrupt to be unhandled by phylib, meaning
link state detection degrades from an immediate event-driven notification to
1-second polling?

>  	rtl_irq_disable(tp);
>  	napi_schedule(&tp->napi);
[ ... ]
> @@ -5458,6 +5488,21 @@ static int r8169_mdio_register(struct rtl8169_private *tp)
[ ... ]
> +	virq = irq_create_mapping(tp->phy_irq_domain, 0);
> +	if (!virq)
> +		ret = -EINVAL;
> +
> +	new_bus->irq[0] = virq;
> +
>  	ret = devm_mdiobus_register(&pdev->dev, new_bus);

[Severity: Medium]
If irq_create_mapping() fails, ret is set to -EINVAL, but does this
assignment get immediately overwritten by the return value of
devm_mdiobus_register()?

This would cause the IRQ mapping failure to be silently ignored, leaving
the bus with an invalid IRQ of 0 without aborting the driver initialization.

>  	if (ret)
>  		return ret;