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

[Shawn Lin]

在 2026/09/04 星期五 21:07, Niklas Cassel 写道:
> 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 at kernel.org>
>> Cc: Wilfred Mallawa <wilfred.mallawa at wdc.com>
>> Signed-off-by: Shawn Lin <shawn.lin at 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.
> 

Don't worry, and thanks for these suggestion. I'll spin a v3 later.

> 
> Kind regards,
> Niklas
>