Re: [PATCH 2/2] PCI: dw-rockchip: Do not recreate the INTx irq domain on root port reset

[Shawn Lin <shawn.lin@rock-chips.com>]

Hi Niklas

在 2026/09/03 星期四 16:37, Niklas Cassel 写道:
> On Thu, Sep 03, 2026 at 10:44:37AM +0800, Shawn Lin wrote:
>> .reset_root_port() re-runs the host ops .init() callback to reprogram
>> the Root Complex after the controller reset. However, .init() also
>> creates a new INTx irq domain on every root port reset, so that:
>>
>>    - the previous irq domain is leaked, as it is never removed, and two
>>      irq domains end up registered for the same fwnode;
>>
>>    - 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. Hence, after a
>>      link down recovery, INTx interrupts are silently lost.
>>
>> Split the (re)programming of the Root Complex registers out of .init()
>> into rockchip_pcie_host_hw_init() and call that from .reset_root_port()
>> instead. The INTx irq domain and the chained handler are now only set up
>> once, at probe time, which keeps the already mapped virqs valid across
>> root port resets.
>>
>> Fixes: b376b3ff9cb0 ("PCI: dw-rockchip: Implement .reset_root_port() and use for link down")
>> Cc: Niklas Cassel <cassel@kernel.org>
>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
>>
>> ---
> 
> If I compare to pcie-qcom.c, the difference is that they do
> e.g.:
> irq = platform_get_irq_byname_optional(pdev, "global");
> 
> after calling dw_pcie_host_init() in qcom_pcie_probe().
> 
> 
> I guess pcie-dw-rockchip.c could do the same:
> Call of_irq_get_byname() and rockchip_pcie_init_irq_domain() in
> rockchip_pcie_configure_rc(), after calling dw_pcie_host_init().
> 
> That way, you don't need to introduce another rockchip_pcie_host_hw_init().
> 
> pci->pp.ops->init() is called by both dw_pcie_host_init() and
> dw_pcie_resume_noirq(). So calling of_irq_get_byname() in .init()
> does seem slightly wrong, as we would get the irq on each resume.
> 
> Perhaps pcie-dw-rockchip.c does not have support for resume, so it
> does not matter right now, but still seems a bit weird to call
> of_irq_get_byname() in init().
> 
> I did not look if rockchip_pcie_init_irq_domain() should be called
> on each resume, but I since we don't tear down the irq_domain in
> pci->pp.ops->deinit(), in fact we don't even have a ->deinit(),
> so calling rockchip_pcie_init_irq_domain() in ->init() does seem
> wrong as well.
> 
> So my vote is to move both to rockchip_pcie_configure_rc(), after
> calling dw_pcie_host_init().
> 

Thanks for the review!

I agree with your reasoning. Will moves the of_irq_get_byname() lookup,
the INTx irq domain creation and the chained handler installation into
rockchip_pcie_configure_rc(), right after dw_pcie_host_init(), matching
the qcom pattern you pointed out. The rockchip_pcie_host_hw_init()
helper from will be gone in v2, and the host ops .init() callback is
back to doing only idempotent register programming, which
.reset_root_port() can safely re-run.

Since there is still no ->deinit() to pair with .init(), and the
Sashiko review also flagged the missing cleanup on probe failure, I
made the irq domain devm-managed (devm_irq_domain_instantiate()) and
uninstall the chained handler through a devres action while moving the
code, so everything is released automatically if probe fails.

Will send out v2 for review.

> 
> Kind regards,
> Niklas
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip