[PATCH 0/2] Two small INTx fixes for Rockchip's dwc based PCIe controller driver

[PATCH 0/2] Two small INTx fixes for Rockchip's dwc based PCIe controller driver

[Shawn Lin]

This small series fixes two INTx related issues in the Rockchip
DesignWare PCIe controller driver.

Note that patch 2 depends on b376b3ff9cb0 which is not yet in Linus'
tree, so this series is based on linux-next.



Shawn Lin (2):
  PCI: dw-rockchip: Bail out if the INTx irq domain creation fails
  PCI: dw-rockchip: Do not recreate the INTx irq domain on root port
    reset

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 41 +++++++++++++++++----------
 1 file changed, 26 insertions(+), 15 deletions(-)

-- 
2.7.4

[PATCH 1/2] PCI: dw-rockchip: Bail out if the INTx irq domain creation fails

[Shawn Lin]

rockchip_pcie_init_irq_domain() can fail, e.g. when the DT node is
missing its "legacy-interrupt-controller" child node. In that case,
rockchip_pcie_host_init() only prints an error and continues, arming the
chained INTx handler with a NULL irq_domain. The first INTx interrupt
then dereferences the NULL irq_domain in rockchip_pcie_intx_handler(),
which panics the machine.

Propagate the error to abort probing, keeping the error message for
easier debugging.

Fixes: e8aae154df61 ("PCI: rockchip-dwc: Add legacy interrupt support")
Cc: Peter Geis <pgwipeout@gmail.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index af26a07..50824c8 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -432,8 +432,10 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
 	pci->dbi_base2 = pci->dbi_base + PCIE_TYPE0_HDR_DBI2_OFFSET;
 
 	ret = rockchip_pcie_init_irq_domain(rockchip);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(dev, "failed to init irq domain\n");
+		return ret;
+	}
 
 	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
 					 rockchip);
-- 
2.7.4

Re: [PATCH 1/2] PCI: dw-rockchip: Bail out if the INTx irq domain creation fails

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a missing IRQ domain and chained handler cleanup on initialization failure paths, leading to a memory leak and potential Use-After-Free.
- [High] This problem wasn't introduced by this patch, but there is an unprotected lockless iteration over the PCI bus devices list in the threaded IRQ handler.
--

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

PCI: dw-rockchip: Bail out if the INTx irq domain creation fails

rockchip_pcie_init_irq_domain() can fail, e.g. when the DT node is
missing its "legacy-interrupt-controller" child node. This commit
propagates the error to abort probing instead of continuing with a
NULL irq_domain, which previously caused a panic on the first INTx.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index af26a07c0c9e5..50824c8b1d83e 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -432,8 +432,10 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
>  	pci->dbi_base2 = pci->dbi_base + PCIE_TYPE0_HDR_DBI2_OFFSET;
>  
>  	ret = rockchip_pcie_init_irq_domain(rockchip);
> -	if (ret < 0)
> +	if (ret < 0) {
>  		dev_err(dev, "failed to init irq domain\n");
> +		return ret;
> +	}
>  
>  	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
>  					 rockchip);

[Severity: High]
This is a pre-existing regression, but there is a missing IRQ
domain and chained handler cleanup on initialization failure paths,
which can cause a memory leak and use-after-free.

If rockchip_pcie_host_init() succeeds but a subsequent step in
dw_pcie_host_init() fails (e.g., link training), the probe aborts and
devres frees the rockchip struct. Because the driver lacks a .deinit
callback in rockchip_pcie_host_ops, the IRQ domain allocated via
irq_domain_create_linear() is leaked.

Additionally, the chained handler mapped via
irq_set_chained_handler_and_data() is never removed. Can a subsequent
legacy interrupt or spurious trigger invoke rockchip_pcie_intx_handler()
with the devm-freed rockchip pointer, causing a kernel panic?

Does rockchip_pcie_host_ops need a .deinit callback to properly unwind
these resources on failure paths?


While reviewing this file, another pre-existing regression was noticed in
rockchip_pcie_rc_sys_irq_thread():

drivers/pci/controller/dwc/pcie-dw-rockchip.c:rockchip_pcie_rc_sys_irq_thread() {
[ ... ]
	if (reg & PCIE_LINK_REQ_RST_NOT_INT) {
		dev_dbg(dev, "hot reset or link-down reset\n");
		for_each_pci_bridge(port, pp->bridge->bus) {
			if (pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
				pci_host_handle_link_down(port);
		}
	}
[ ... ]
}

[Severity: High]
This is a pre-existing regression, but there is an unprotected lockless
iteration over the PCI bus devices list in this threaded IRQ handler.

When a hot-reset or link-down event occurs, the handler traverses
pp->bridge->bus->devices using for_each_pci_bridge(). This traversal is
performed locklessly without acquiring the required down_read(&pci_bus_sem).

If a concurrent PCI device removal operation modifies the list (e.g., via
sysfs or hotplug), could this lockless traversal access poisoned pointers
or freed device structures, potentially causing list corruption or a panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1788403477-71491-1-git-send-email-shawn.lin@rock-chips.com?part=1

Re: [PATCH 1/2] PCI: dw-rockchip: Bail out if the INTx irq domain creation fails

[Shawn Lin]

在 2026/09/03 星期四 11:12, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but there is a missing IRQ domain and chained handler cleanup on initialization failure paths, leading to a memory leak and potential Use-After-Free.
> - [High] This problem wasn't introduced by this patch, but there is an unprotected lockless iteration over the PCI bus devices list in the threaded IRQ handler.
> --
> 
> commit e7543706c2d1058d78b54feb3252321efa4a390b
> Author: Shawn Lin <shawn.lin@rock-chips.com>
> 
> PCI: dw-rockchip: Bail out if the INTx irq domain creation fails
> 
> rockchip_pcie_init_irq_domain() can fail, e.g. when the DT node is
> missing its "legacy-interrupt-controller" child node. This commit
> propagates the error to abort probing instead of continuing with a
> NULL irq_domain, which previously caused a panic on the first INTx.
> 
>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> index af26a07c0c9e5..50824c8b1d83e 100644
>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>> @@ -432,8 +432,10 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
>>   	pci->dbi_base2 = pci->dbi_base + PCIE_TYPE0_HDR_DBI2_OFFSET;
>>   
>>   	ret = rockchip_pcie_init_irq_domain(rockchip);
>> -	if (ret < 0)
>> +	if (ret < 0) {
>>   		dev_err(dev, "failed to init irq domain\n");
>> +		return ret;
>> +	}
>>   
>>   	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
>>   					 rockchip);
> 
> [Severity: High]
> This is a pre-existing regression, but there is a missing IRQ
> domain and chained handler cleanup on initialization failure paths,
> which can cause a memory leak and use-after-free.
> 
> If rockchip_pcie_host_init() succeeds but a subsequent step in
> dw_pcie_host_init() fails (e.g., link training), the probe aborts and
> devres frees the rockchip struct. Because the driver lacks a .deinit
> callback in rockchip_pcie_host_ops, the IRQ domain allocated via
> irq_domain_create_linear() is leaked.
> 
> Additionally, the chained handler mapped via
> irq_set_chained_handler_and_data() is never removed. Can a subsequent
> legacy interrupt or spurious trigger invoke rockchip_pcie_intx_handler()
> with the devm-freed rockchip pointer, causing a kernel panic?
> 
> Does rockchip_pcie_host_ops need a .deinit callback to properly unwind
> these resources on failure paths?
> 

Ack, good catch. One correction though: the "driver unload" part does
not apply here. The driver is builtin (builtin_platform_driver()) and
sets suppress_bind_attrs, so it can never be unbound and there is no
module removal path. The probe failure window remains real though: if
dw_pcie_host_init() fails after the host ops .init() callback succeeded,
devres frees the rockchip structure while the chained handler still
points to it, and the INTx irq domain is leaked.

I will send a follow-up series that makes the INTx irq domain devm-
managed via devm_irq_domain_instantiate() and uninstalls the chained
handler through a devres action, so that both are released automatically
when probe fails.


> 
> While reviewing this file, another pre-existing regression was noticed in
> rockchip_pcie_rc_sys_irq_thread():
> 
> drivers/pci/controller/dwc/pcie-dw-rockchip.c:rockchip_pcie_rc_sys_irq_thread() {
> [ ... ]
> 	if (reg & PCIE_LINK_REQ_RST_NOT_INT) {
> 		dev_dbg(dev, "hot reset or link-down reset\n");
> 		for_each_pci_bridge(port, pp->bridge->bus) {
> 			if (pci_pcie_type(port) == PCI_EXP_TYPE_ROOT_PORT)
> 				pci_host_handle_link_down(port);
> 		}
> 	}
> [ ... ]
> }
> 
> [Severity: High]
> This is a pre-existing regression, but there is an unprotected lockless
> iteration over the PCI bus devices list in this threaded IRQ handler.
> 
> When a hot-reset or link-down event occurs, the handler traverses
> pp->bridge->bus->devices using for_each_pci_bridge(). This traversal is
> performed locklessly without acquiring the required down_read(&pci_bus_sem).
> 
> If a concurrent PCI device removal operation modifies the list (e.g., via
> sysfs or hotplug), could this lockless traversal access poisoned pointers
> or freed device structures, potentially causing list corruption or a panic?

Note that this is exactly what qcom does in its threaded IRQ handler:
qcom_pcie_global_irq_thread() walks pp->bridge->bus with
for_each_pci_bridge() and calls pci_host_handle_link_down() the same way
(4d88cb82a6d9 "PCI: qcom: Implement .reset_root_port() and use for link
down"), which is what the rockchip code was modelled on.

If the PCI core considers this walk racy, I think it should be fixed at
the pci_host_handle_link_down() level so that both users (qcom and
rockchip) are covered, rather than driver by driver.

> 

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

[Shawn Lin]

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

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 50824c8..ff92394 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -418,6 +418,27 @@ static void rockchip_pcie_stop_link(struct dw_pcie *pci)
 	rockchip_pcie_ltssm_trace(rockchip, false);
 }
 
+/*
+ * (Re)program the Root Complex registers that are cleared by the controller
+ * reset. Called from .init() at probe time and from .reset_root_port().
+ * The INTx irq domain must not be touched here, as downstream devices hold
+ * virqs mapped in it.
+ */
+static void rockchip_pcie_host_hw_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+	pci->dbi_base2 = pci->dbi_base + PCIE_TYPE0_HDR_DBI2_OFFSET;
+
+	rockchip_pcie_configure_l1ss(pci);
+	rockchip_pcie_enable_l0s(pci);
+	pp->bridge->reset_root_port = rockchip_pcie_rc_reset_root_port;
+
+	/* Disable Root Ports BAR0 and BAR1 as they report bogus size */
+	dw_pcie_writel_dbi2(pci, PCI_BASE_ADDRESS_0, 0x0);
+	dw_pcie_writel_dbi2(pci, PCI_BASE_ADDRESS_1, 0x0);
+}
+
 static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -429,8 +450,6 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
 	if (irq < 0)
 		return irq;
 
-	pci->dbi_base2 = pci->dbi_base + PCIE_TYPE0_HDR_DBI2_OFFSET;
-
 	ret = rockchip_pcie_init_irq_domain(rockchip);
 	if (ret < 0) {
 		dev_err(dev, "failed to init irq domain\n");
@@ -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);
 
-	rockchip_pcie_configure_l1ss(pci);
-	rockchip_pcie_enable_l0s(pci);
-	pp->bridge->reset_root_port = rockchip_pcie_rc_reset_root_port;
-
-	/* Disable Root Ports BAR0 and BAR1 as they report bogus size */
-	dw_pcie_writel_dbi2(pci, PCI_BASE_ADDRESS_0, 0x0);
-	dw_pcie_writel_dbi2(pci, PCI_BASE_ADDRESS_1, 0x0);
+	rockchip_pcie_host_hw_init(pp);
 
 	return 0;
 }
@@ -920,11 +933,7 @@ static int rockchip_pcie_rc_reset_root_port(struct pci_host_bridge *bridge,
 	if (ret)
 		goto deinit_phy;
 
-	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);
 
 	/* LTSSM enable control mode */
 	val = FIELD_PREP_WM16(PCIE_LTSSM_ENABLE_ENHANCE, 1);
-- 
2.7.4

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

[sashiko-bot]

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);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1788403477-71491-1-git-send-email-shawn.lin@rock-chips.com?part=2

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

[Shawn Lin]

在 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);
> 

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

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


Kind regards,
Niklas

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

[Shawn Lin]

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
> 

Re: [PATCH 0/2] Two small INTx fixes for Rockchip's dwc based PCIe controller driver

[Niklas Cassel]

Hello Shawn,

On Thu, Sep 03, 2026 at 10:44:35AM +0800, Shawn Lin wrote:
> 
> This small series fixes two INTx related issues in the Rockchip
> DesignWare PCIe controller driver.
> 
> Note that patch 2 depends on b376b3ff9cb0 which is not yet in Linus'
> tree, so this series is based on linux-next.

$ git tag --contains b376b3ff9cb0
v7.3-rc1

FWIW, the commit you are referencing is in Linus' tree already.


Kind regards,
Niklas

Re: [PATCH 0/2] Two small INTx fixes for Rockchip's dwc based PCIe controller driver

[Shawn Lin]

在 2026/09/03 星期四 16:20, Niklas Cassel 写道:
> Hello Shawn,
> 
> On Thu, Sep 03, 2026 at 10:44:35AM +0800, Shawn Lin wrote:
>>
>> This small series fixes two INTx related issues in the Rockchip
>> DesignWare PCIe controller driver.
>>
>> Note that patch 2 depends on b376b3ff9cb0 which is not yet in Linus'
>> tree, so this series is based on linux-next.
> 
> $ git tag --contains b376b3ff9cb0
> v7.3-rc1
> 
> FWIW, the commit you are referencing is in Linus' tree already.

Ah, a few days ago I didn't send out the patch in time, but now it is 
indeed on Linus' tree.

Thanks.

> 
> 
> Kind regards,
> Niklas
>