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

.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>

---

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 37 +++++++++++++++++----------
 1 file changed, 23 insertions(+), 14 deletions(-)

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Probe error path and driver unload fail to clean up INTx irq domain and chained handler, leading to a potential use-after-free and memory leak.
- [High] Disabling clocks during root port reset without masking IRQs can cause a synchronous external abort.
- [High] Double PHY de-initialization permanently bricks the PCI controller on subsequent resets.
- [High] DBI registers are accessed before the controller is set to RC mode during root port reset.
--

commit 2778c4d5819445db0e096281981b7df261e7bc31
Author: Shawn Lin <shawn.lin@rock-chips.com>

PCI: dw-rockchip: Do not recreate the INTx irq domain on root port reset

This patch extracts the programming of Root Complex registers out of
the .init() callback into a separate rockchip_pcie_host_hw_init() function.
It then calls this new function from .reset_root_port() instead, ensuring
the INTx IRQ domain and chained handler are set up only once at probe time.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 50824c8b1d83e..ff9239403cf22 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
[ ... ]
> @@ -440,13 +459,7 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
>  	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
>  					 rockchip);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the INTx
irq domain and chained handler to be leaked on probe failure or driver
unload?

If rockchip_pcie_host_init() or a later step in probe fails, devres will free
the rockchip structure. However, it appears the irq domain is created in
rockchip_pcie_init_irq_domain() without devm management and is never removed.
Additionally, the chained handler is assigned with the rockchip structure as
data but is never cleared on the error path.

If the legacy IRQ fires after probe failure, could the chained handler
(rockchip_pcie_intx_handler) dereference the freed rockchip pointer and cause a
use-after-free panic? Also, since the driver lacks a .deinit callback, does
this permanently leak the domain on module removal?

[ ... ]
> @@ -920,11 +933,7 @@ static int rockchip_pcie_rc_reset_root_port(struct pci_host_bridge *bridge,
>  	if (ret)
>  		goto deinit_phy;

[Severity: High]
This is a pre-existing issue, but does this jump lead to a double PHY
de-initialization that permanently bricks the PCI controller?

If the reset fails (e.g., dw_pcie_start_link() fails later in this function),
the code jumps to the deinit_phy label and calls rockchip_pcie_phy_deinit().
However, rockchip_pcie_rc_reset_root_port() already unconditionally calls
rockchip_pcie_phy_deinit() near the top of the function.

If the PCI core attempts another reset later, the unconditional deinit at the
top will execute again. Does this erroneously drop the PHY's power_count and
init_count from 0 to -1, causing subsequent rockchip_pcie_phy_init() calls to
silently fail to power on the PHY?

[Severity: High]
This is another pre-existing issue, but can disabling clocks during root port
reset cause a synchronous external abort (SError) if the chained IRQ is not
masked?

Earlier in rockchip_pcie_rc_reset_root_port(), the controller clocks are
disabled via clk_bulk_disable_unprepare() before resetting the hardware. If an
unhandled interrupt fires concurrently while the clocks are disabled, the CPU
will attempt to execute rockchip_pcie_intx_handler(), which directly reads
PCIE_CLIENT_INT_STATUS over the unclocked APB bus.

Should the chained IRQ be explicitly masked (or disable_irq() used) before
disabling the clocks to prevent accessing unclocked APB MMIO space?

> -	ret = pp->ops->init(pp);
> -	if (ret) {
> -		dev_err(dev, "Host init failed: %d\n", ret);
> -		goto deinit_clk;
> -	}
> +	rockchip_pcie_host_hw_init(pp);

[Severity: High]
This is a pre-existing issue, but does rockchip_pcie_host_hw_init() access DBI
registers before the controller is explicitly set to Root Complex mode?

During reset, rockchip_pcie_rc_reset_root_port() applies a hardware reset via
reset_control_assert(). It then calls rockchip_pcie_host_hw_init(), which writes
to DBI registers (like PCI_BASE_ADDRESS_0) and accesses PCIe capabilities.

Since the controller is instructed to act as a Root Complex
(PCIE_CLIENT_MODE_RC) only after rockchip_pcie_host_hw_init() returns, does
accessing the PCI configuration space prematurely violate hardware state
constraints and risk an SError on the AXI bus?

>  
>  	/* LTSSM enable control mode */
>  	val = FIELD_PREP_WM16(PCIE_LTSSM_ENABLE_ENHANCE, 1);

在 2026/09/03 星期四 11:13, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Probe error path and driver unload fail to clean up INTx irq domain and chained handler, leading to a potential use-after-free and memory leak.
> - [High] Disabling clocks during root port reset without masking IRQs can cause a synchronous external abort.
> - [High] Double PHY de-initialization permanently bricks the PCI controller on subsequent resets.
> - [High] DBI registers are accessed before the controller is set to RC mode during root port reset.
> --
> 
> commit 2778c4d5819445db0e096281981b7df261e7bc31
> Author: Shawn Lin <shawn.lin@rock-chips.com>
> 
> PCI: dw-rockchip: Do not recreate the INTx irq domain on root port reset
> 
> This patch extracts the programming of Root Complex registers out of
> the .init() callback into a separate rockchip_pcie_host_hw_init() function.
> It then calls this new function from .reset_root_port() instead, ensuring
> the INTx IRQ domain and chained handler are set up only once at probe time.
> 
>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> index 50824c8b1d83e..ff9239403cf22 100644
>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> [ ... ]
>> @@ -440,13 +459,7 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
>>   	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
>>   					 rockchip);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but is it possible for the INTx
> irq domain and chained handler to be leaked on probe failure or driver
> unload?
> 
> If rockchip_pcie_host_init() or a later step in probe fails, devres will free
> the rockchip structure. However, it appears the irq domain is created in
> rockchip_pcie_init_irq_domain() without devm management and is never removed.
> Additionally, the chained handler is assigned with the rockchip structure as
> data but is never cleared on the error path.
> 
> If the legacy IRQ fires after probe failure, could the chained handler
> (rockchip_pcie_intx_handler) dereference the freed rockchip pointer and cause a
> use-after-free panic? Also, since the driver lacks a .deinit callback, does
> this permanently leak the domain on module removal?
> 
> [ ... ]
>> @@ -920,11 +933,7 @@ static int rockchip_pcie_rc_reset_root_port(struct pci_host_bridge *bridge,
>>   	if (ret)
>>   		goto deinit_phy;
> 
> [Severity: High]
> This is a pre-existing issue, but does this jump lead to a double PHY
> de-initialization that permanently bricks the PCI controller?
> 
> If the reset fails (e.g., dw_pcie_start_link() fails later in this function),
> the code jumps to the deinit_phy label and calls rockchip_pcie_phy_deinit().
> However, rockchip_pcie_rc_reset_root_port() already unconditionally calls
> rockchip_pcie_phy_deinit() near the top of the function.
> 
> If the PCI core attempts another reset later, the unconditional deinit at the
> top will execute again. Does this erroneously drop the PHY's power_count and
> init_count from 0 to -1, causing subsequent rockchip_pcie_phy_init() calls to
> silently fail to power on the PHY?

I don't think there is an unbalanced deinit here. The unconditional
rockchip_pcie_phy_deinit() at the top of
rockchip_pcie_rc_reset_root_port() pairs with the
rockchip_pcie_phy_init() done at probe time, while the deinit_phy label
pairs with the rockchip_pcie_phy_init() performed inside this same
function. Every path keeps the pairs balanced: on success the PHY ends
up initialized exactly as it was before the reset, and on the error
paths the function unwinds precisely the initialization it performed
in this invocation. The only way to skew the refcounts would be
reset_control_assert() failing on two consecutive reset attempts, and
after the first such failure the controller is already dead.

> 
> [Severity: High]
> This is another pre-existing issue, but can disabling clocks during root port
> reset cause a synchronous external abort (SError) if the chained IRQ is not
> masked?
> 
> Earlier in rockchip_pcie_rc_reset_root_port(), the controller clocks are
> disabled via clk_bulk_disable_unprepare() before resetting the hardware. If an
> unhandled interrupt fires concurrently while the clocks are disabled, the CPU
> will attempt to execute rockchip_pcie_intx_handler(), which directly reads
> PCIE_CLIENT_INT_STATUS over the unclocked APB bus.
> 
> Should the chained IRQ be explicitly masked (or disable_irq() used) before
> disabling the clocks to prevent accessing unclocked APB MMIO space?


Ack, this is a real issue introduced by b376b3ff9cb0. Devices may keep
their INTx line asserted while the controller clocks and PHY are gated
during the root port reset, so the chained handler can run and read the
unclocked APB bus.

I will send a follow-up that masks the INTx IRQ before turning the
clocks off and re-enables it once the clocks are running again. On the
error paths the clocks stay gated, so the IRQ is deliberately left
masked there.


> 
>> -	ret = pp->ops->init(pp);
>> -	if (ret) {
>> -		dev_err(dev, "Host init failed: %d\n", ret);
>> -		goto deinit_clk;
>> -	}
>> +	rockchip_pcie_host_hw_init(pp);
> 
> [Severity: High]
> This is a pre-existing issue, but does rockchip_pcie_host_hw_init() access DBI
> registers before the controller is explicitly set to Root Complex mode?
> 
> During reset, rockchip_pcie_rc_reset_root_port() applies a hardware reset via
> reset_control_assert(). It then calls rockchip_pcie_host_hw_init(), which writes
> to DBI registers (like PCI_BASE_ADDRESS_0) and accesses PCIe capabilities.
> 
> Since the controller is instructed to act as a Root Complex
> (PCIE_CLIENT_MODE_RC) only after rockchip_pcie_host_hw_init() returns, does
> accessing the PCI configuration space prematurely violate hardware state
> constraints and risk an SError on the AXI bus?


This ordering is not introduced by this series: the original
.reset_root_port() implementation (b376b3ff9cb0) also called
pp->ops->init() — which performs the same DBI accesses — before writing 
PCIE_CLIENT_MODE_RC, so the order is unchanged. Also, the DBI
aperture is just memory-mapped configuration space behind the APB/AXI
bus; it does not depend on the PCIE_CLIENT_xxx mode bits, which select
the  link/LTSSM behaviour rather than gating config space access.

> 
>>   
>>   	/* LTSSM enable control mode */
>>   	val = FIELD_PREP_WM16(PCIE_LTSSM_ENABLE_ENHANCE, 1);
>

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().


Kind regards,
Niklas

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
>