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

This commit creates a virtual interrupt domain in r8169_mdio_register() so
the MAC link change interrupt can be dispatched into phylib via
generic_handle_domain_irq() instead of calling phy_mac_interrupt() on
tp->phydev directly.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index c60710f9bd21..560f987437b6 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c

[ ... ]

> @@ -5458,6 +5488,21 @@ static int r8169_mdio_register(struct rtl8169_private *tp)
>  		new_bus->write_c45 = r8169_mdio_write_reg_c45;
>  	}
>
> +	tp->phy_irq_domain = irq_domain_add_linear(NULL, 1,
> +						   &rtl_phy_irq_domain_ops, tp);
> +	if (!tp->phy_irq_domain)
> +		return -ENOMEM;
> +
> +	ret = devm_add_action_or_reset(&pdev->dev, rtl_phy_irq_cleanup, tp);
> +	if (ret)
> +		return ret;
> +
> +	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);
>  	if (ret)
>  		return ret;

[High]
Should the failure of irq_create_mapping() actually abort registration
here?

	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);
	if (ret)
		return ret;

When irq_create_mapping() returns 0, ret is set to -EINVAL but the
function continues on. The next assignment to ret from
devm_mdiobus_register() overwrites that -EINVAL, so the mapping failure
is silently dropped.

A couple of follow-on consequences seem possible:

The mdiobus is registered with new_bus->irq[0] == 0. phy_interrupt_is_valid()
only special-cases PHY_POLL (-1) and PHY_MAC_INTERRUPT (-2), so 0 looks
like a normal IRQ number and phylib will try to request_threaded_irq(0,
...) when the PHY starts.

In rtl8169_interrupt(), generic_handle_domain_irq(tp->phy_irq_domain, 0)
will be called against a domain with no hwirq 0 mapping, so LinkChg
events would not reach the PHY state machine.

Would something like this be more appropriate?

	virq = irq_create_mapping(tp->phy_irq_domain, 0);
	if (!virq)
		return -EINVAL;

	new_bus->irq[0] = virq;