Re: [PATCH v2 1/2] PCI: dw-rockchip: Move the INTx irq setup to probe and make it devm-managed

[Niklas Cassel <cassel@kernel.org>]

On Fri, Sep 04, 2026 at 09:18:28AM +0800, Shawn Lin wrote:
> Since commit b376b3ff9cb0 ("PCI: dw-rockchip: Implement .reset_root_port()
> and use for link down"), .reset_root_port() re-runs the host ops
> .init() callback to reprogram the Root Complex after a controller
> reset. That works for the register programming, but .init() is not
> re-entrant: it also creates the INTx irq domain and installs the
> chained INTx handler. Every root port reset therefore ends up with a
> second irq domain registered for the same fwnode: the previous one is
> leaked, as it is never removed, and worse, the INTx virqs of the
> downstream PCI devices were allocated in the previous irq domain and
> are never re-mapped, while the chained handler now looks up virqs in
> the new, empty domain. After a link down recovery, INTx interrupts
> are silently lost.
> 
> Fix it by moving the of_irq_get_byname() lookup, the INTx irq domain
> creation and the chained handler installation out of .init() and into
> rockchip_pcie_configure_rc(), right after dw_pcie_host_init(). This
> mirrors how the qcom driver requests its global IRQ, and leaves
> .init() with nothing but idempotent register programming, so both
> .reset_root_port() and dw_pcie_resume_noirq() can safely re-run it.
> Re-running of_irq_get_byname() on every resume is also gone.
> 
> With the irq setup now living in probe, tie its lifetime to the device
> with devres: create the irq domain with devm_irq_domain_instantiate()
> and uninstall the chained handler through the
> rockchip_pcie_intx_chained_release() devres action. The driver is
> builtin and cannot be unbound (suppress_bind_attrs), so probe failure
> is the only path that ever needs this cleanup, and devres takes care
> of it without sprinkling it over every error path. Since the irq setup
> is the last step of rockchip_pcie_configure_rc(), the only failure
> point left after the chained handler is installed is
> devm_add_action_or_reset() itself, whose failure mode runs the action,
> so the handler can never run against the devm-freed rockchip
> structure. devres also unwinds in reverse registration order, so the
> handler is always uninstalled before the domain is removed. There is
> no devm API for chained handlers, hence the small devres action
> wrapper.
> 
> Fixes: b376b3ff9cb0 ("PCI: dw-rockchip: Implement .reset_root_port() and use for link down")
> Suggested-by: Niklas Cassel <cassel@kernel.org>
> Cc: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

I would have written it in two commits:
1/3 that moves the of_irq_get_byname() lookup, the INTx irq domain creation,
    and chained handler installation.
2/3 that makes the irq doman device managed, to not leak the resources, as
    flagged by Sashiko
(3/3 that masks the INTx IRQ during .reset_root_port())


Since fixing the resource leak seems like a separate issue.

The commit message for this patch does seem quite long, mostly because
it is fixing two separate issues.


Sorry that we did not detect this INTx issue when sending the
.reset_root_port() patch. Since RK3588 does not support INTx in EP mode,
it is unfortunately not tested when running the pci_endpoint_test selftest.


Kind regards,
Niklas