Re: [PATCH net-next v2 2/6] r8169: create a virtual interrupt for linkchg
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-06-13 22:06:24
Also in:
lkml
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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;