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

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

[Shawn Lin]

This short series fixes the INTx handling around the newly introduced
.reset_root_port() (b376b3ff9cb0):

Patch 1 stops .reset_root_port() from recreating the INTx irq domain
on every root port reset, which leaked the old domain and silently
broke INTx delivery afterwards, by moving 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(), per Niklas' suggestion. The irq domain is made
devm-managed and the chained handler is uninstalled through a devres
action, which also addresses the probe failure leak/use-after-free
flagged by the Sashiko review.

Patch 2 keeps the INTx IRQ masked while .reset_root_port() gates the
controller clocks, so the chained handler cannot read the unclocked
APB bus and raise a synchronous external abort.

This series supersedes the v1 series:
https://lore.kernel.org/linux-pci/1788403477-71491-1-git-send-email-shawn.lin@rock-chips.com/


Changes in v2:
- Moved the of_irq_get_byname() lookup, the INTx irq domain creation
  and the chained handler installation out of the host ops .init()
  callback into rockchip_pcie_configure_rc(), right after
  dw_pcie_host_init(), as suggested by Niklas Cassel. This supersedes
  v1 patch 1/2, as .init() no longer creates the irq domain, and
  removes the rockchip_pcie_host_hw_init() helper from v1.
- Made the INTx irq domain devm-managed with
  devm_irq_domain_instantiate() and uninstall the chained handler
  through a devres action, addressing the probe failure leak and
  use-after-free flagged by the Sashiko review.
- keep the INTx IRQ masked while .reset_root_port()
  gates the controller clocks, responding to the Sashiko review
  finding about accessing the unclocked APB bus.

Shawn Lin (2):
  PCI: dw-rockchip: Move the INTx irq setup to probe and make it
    devm-managed
  PCI: dw-rockchip: Mask the INTx IRQ while the controller clocks are
    gated

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 77 ++++++++++++++++++++-------
 1 file changed, 57 insertions(+), 20 deletions(-)

-- 
2.7.4


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

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

[Shawn Lin]

This short series fixes the INTx handling around the newly introduced
.reset_root_port() (b376b3ff9cb0):

Patch 1 stops .reset_root_port() from recreating the INTx irq domain
on every root port reset, which leaked the old domain and silently
broke INTx delivery afterwards, by moving 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(), per Niklas' suggestion. The irq domain is made
devm-managed and the chained handler is uninstalled through a devres
action, which also addresses the probe failure leak/use-after-free
flagged by the Sashiko review.

Patch 2 keeps the INTx IRQ masked while .reset_root_port() gates the
controller clocks, so the chained handler cannot read the unclocked
APB bus and raise a synchronous external abort.

This series supersedes the v1 series:
https://lore.kernel.org/linux-pci/1788403477-71491-1-git-send-email-shawn.lin@rock-chips.com/


Changes in v2:
- Moved the of_irq_get_byname() lookup, the INTx irq domain creation
  and the chained handler installation out of the host ops .init()
  callback into rockchip_pcie_configure_rc(), right after
  dw_pcie_host_init(), as suggested by Niklas Cassel. This supersedes
  v1 patch 1/2, as .init() no longer creates the irq domain, and
  removes the rockchip_pcie_host_hw_init() helper from v1.
- Made the INTx irq domain devm-managed with
  devm_irq_domain_instantiate() and uninstall the chained handler
  through a devres action, addressing the probe failure leak and
  use-after-free flagged by the Sashiko review.
- keep the INTx IRQ masked while .reset_root_port()
  gates the controller clocks, responding to the Sashiko review
  finding about accessing the unclocked APB bus.

Shawn Lin (2):
  PCI: dw-rockchip: Move the INTx irq setup to probe and make it
    devm-managed
  PCI: dw-rockchip: Mask the INTx IRQ while the controller clocks are
    gated

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 77 ++++++++++++++++++++-------
 1 file changed, 57 insertions(+), 20 deletions(-)

-- 
2.7.4

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

[Shawn Lin]

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>

---

Changes in v2:
- Moved the of_irq_get_byname() lookup, the INTx irq domain creation
  and the chained handler installation out of the host ops .init()
  callback into rockchip_pcie_configure_rc(), right after
  dw_pcie_host_init(), as suggested by Niklas Cassel. This supersedes
  v1 patch 1/2, as .init() no longer creates the irq domain, and
  removes the rockchip_pcie_host_hw_init() helper from v1.
- Made the INTx irq domain devm-managed with
  devm_irq_domain_instantiate() and uninstall the chained handler
  through a devres action, addressing the probe failure leak and
  use-after-free flagged by the Sashiko review.

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 65 +++++++++++++++++++--------
 1 file changed, 46 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index af26a07..ecf0d7e 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -113,6 +113,7 @@ struct rockchip_pcie {
 	struct reset_control *rst;
 	struct gpio_desc *rst_gpio;
 	struct irq_domain *irq_domain;
+	int intx_irq;
 	const struct rockchip_pcie_of_data *data;
 	bool supports_clkreq;
 	struct delayed_work trace_work;
@@ -187,9 +188,16 @@ static const struct irq_domain_ops intx_domain_ops = {
 	.map = rockchip_pcie_intx_map,
 };
 
-static int rockchip_pcie_init_irq_domain(struct rockchip_pcie *rockchip)
+static void rockchip_pcie_intx_chained_release(void *data)
+{
+	struct rockchip_pcie *rockchip = data;
+
+	irq_set_chained_handler_and_data(rockchip->intx_irq, NULL, NULL);
+}
+
+static int rockchip_pcie_init_irq_domain(struct device *dev,
+					 struct rockchip_pcie *rockchip)
 {
-	struct device *dev = rockchip->pci.dev;
 	struct device_node *intc;
 
 	intc = of_get_child_by_name(dev->of_node, "legacy-interrupt-controller");
@@ -198,12 +206,17 @@ static int rockchip_pcie_init_irq_domain(struct rockchip_pcie *rockchip)
 		return -EINVAL;
 	}
 
-	rockchip->irq_domain = irq_domain_create_linear(of_fwnode_handle(intc), PCI_NUM_INTX,
-							&intx_domain_ops, rockchip);
+	rockchip->irq_domain = devm_irq_domain_instantiate(dev,
+			&(struct irq_domain_info){
+				.fwnode = of_fwnode_handle(intc),
+				.size = PCI_NUM_INTX,
+				.ops = &intx_domain_ops,
+				.host_data = rockchip,
+			});
 	of_node_put(intc);
-	if (!rockchip->irq_domain) {
+	if (IS_ERR(rockchip->irq_domain)) {
 		dev_err(dev, "failed to get a INTx IRQ domain\n");
-		return -EINVAL;
+		return PTR_ERR(rockchip->irq_domain);
 	}
 
 	return 0;
@@ -422,22 +435,9 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
-	struct device *dev = rockchip->pci.dev;
-	int irq, ret;
-
-	irq = of_irq_get_byname(dev->of_node, "legacy");
-	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");
-
-	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;
@@ -739,6 +739,33 @@ static int rockchip_pcie_configure_rc(struct platform_device *pdev,
 		return ret;
 	}
 
+	/*
+	 * This is done here instead of in the host ops .init() callback,
+	 * which is also re-run by .reset_root_port(), so that the INTx irq
+	 * domain is only created once, at probe time.
+	 */
+	rockchip->intx_irq = of_irq_get_byname(dev->of_node, "legacy");
+	if (rockchip->intx_irq < 0)
+		return rockchip->intx_irq;
+
+	ret = rockchip_pcie_init_irq_domain(dev, rockchip);
+	if (ret < 0) {
+		dev_err(dev, "failed to init irq domain\n");
+		return ret;
+	}
+
+	irq_set_chained_handler_and_data(rockchip->intx_irq,
+					 rockchip_pcie_intx_handler, rockchip);
+
+	/*
+	 * Uninstall the chained handler on probe failure, so that it can
+	 * never run against the devm-freed rockchip structure.
+	 */
+	ret = devm_add_action_or_reset(dev, rockchip_pcie_intx_chained_release,
+				       rockchip);
+	if (ret)
+		return ret;
+
 	/* unmask hot reset/link-down reset */
 	val = FIELD_PREP_WM16(PCIE_LINK_REQ_RST_NOT_INT, 0);
 	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);
-- 
2.7.4


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

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

[Shawn Lin]

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>

---

Changes in v2:
- Moved the of_irq_get_byname() lookup, the INTx irq domain creation
  and the chained handler installation out of the host ops .init()
  callback into rockchip_pcie_configure_rc(), right after
  dw_pcie_host_init(), as suggested by Niklas Cassel. This supersedes
  v1 patch 1/2, as .init() no longer creates the irq domain, and
  removes the rockchip_pcie_host_hw_init() helper from v1.
- Made the INTx irq domain devm-managed with
  devm_irq_domain_instantiate() and uninstall the chained handler
  through a devres action, addressing the probe failure leak and
  use-after-free flagged by the Sashiko review.

 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 65 +++++++++++++++++++--------
 1 file changed, 46 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index af26a07..ecf0d7e 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -113,6 +113,7 @@ struct rockchip_pcie {
 	struct reset_control *rst;
 	struct gpio_desc *rst_gpio;
 	struct irq_domain *irq_domain;
+	int intx_irq;
 	const struct rockchip_pcie_of_data *data;
 	bool supports_clkreq;
 	struct delayed_work trace_work;
@@ -187,9 +188,16 @@ static const struct irq_domain_ops intx_domain_ops = {
 	.map = rockchip_pcie_intx_map,
 };
 
-static int rockchip_pcie_init_irq_domain(struct rockchip_pcie *rockchip)
+static void rockchip_pcie_intx_chained_release(void *data)
+{
+	struct rockchip_pcie *rockchip = data;
+
+	irq_set_chained_handler_and_data(rockchip->intx_irq, NULL, NULL);
+}
+
+static int rockchip_pcie_init_irq_domain(struct device *dev,
+					 struct rockchip_pcie *rockchip)
 {
-	struct device *dev = rockchip->pci.dev;
 	struct device_node *intc;
 
 	intc = of_get_child_by_name(dev->of_node, "legacy-interrupt-controller");
@@ -198,12 +206,17 @@ static int rockchip_pcie_init_irq_domain(struct rockchip_pcie *rockchip)
 		return -EINVAL;
 	}
 
-	rockchip->irq_domain = irq_domain_create_linear(of_fwnode_handle(intc), PCI_NUM_INTX,
-							&intx_domain_ops, rockchip);
+	rockchip->irq_domain = devm_irq_domain_instantiate(dev,
+			&(struct irq_domain_info){
+				.fwnode = of_fwnode_handle(intc),
+				.size = PCI_NUM_INTX,
+				.ops = &intx_domain_ops,
+				.host_data = rockchip,
+			});
 	of_node_put(intc);
-	if (!rockchip->irq_domain) {
+	if (IS_ERR(rockchip->irq_domain)) {
 		dev_err(dev, "failed to get a INTx IRQ domain\n");
-		return -EINVAL;
+		return PTR_ERR(rockchip->irq_domain);
 	}
 
 	return 0;
@@ -422,22 +435,9 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
-	struct device *dev = rockchip->pci.dev;
-	int irq, ret;
-
-	irq = of_irq_get_byname(dev->of_node, "legacy");
-	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");
-
-	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;
@@ -739,6 +739,33 @@ static int rockchip_pcie_configure_rc(struct platform_device *pdev,
 		return ret;
 	}
 
+	/*
+	 * This is done here instead of in the host ops .init() callback,
+	 * which is also re-run by .reset_root_port(), so that the INTx irq
+	 * domain is only created once, at probe time.
+	 */
+	rockchip->intx_irq = of_irq_get_byname(dev->of_node, "legacy");
+	if (rockchip->intx_irq < 0)
+		return rockchip->intx_irq;
+
+	ret = rockchip_pcie_init_irq_domain(dev, rockchip);
+	if (ret < 0) {
+		dev_err(dev, "failed to init irq domain\n");
+		return ret;
+	}
+
+	irq_set_chained_handler_and_data(rockchip->intx_irq,
+					 rockchip_pcie_intx_handler, rockchip);
+
+	/*
+	 * Uninstall the chained handler on probe failure, so that it can
+	 * never run against the devm-freed rockchip structure.
+	 */
+	ret = devm_add_action_or_reset(dev, rockchip_pcie_intx_chained_release,
+				       rockchip);
+	if (ret)
+		return ret;
+
 	/* unmask hot reset/link-down reset */
 	val = FIELD_PREP_WM16(PCIE_LINK_REQ_RST_NOT_INT, 0);
 	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);
-- 
2.7.4

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

[sashiko-bot]

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

New issues:
- [High] Probe failure after dw_pcie_host_init() skips required dw_pcie_host_deinit() cleanup, causing a severe use-after-free and resource leak.
- [High] PCI host enumerated before INTx IRQ domain is created, causing downstream legacy interrupts to unconditionally fail.

Pre-existing issues:
- [High] Early SYS IRQ firing causes NULL pointer dereference in rockchip_pcie_rc_sys_irq_thread.
--

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

PCI: dw-rockchip: Move the INTx irq setup to probe and make it devm-managed

This commit moves the INTx IRQ domain creation and chained handler setup
into the probe function, managing their lifetimes with devres. This prevents
IRQ domain leaks and use-after-free issues during root port resets.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index af26a07c0c9e5..ecf0d7e043b36 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c

[ ... ]

> @@ -739,6 +739,33 @@ static int rockchip_pcie_configure_rc(struct platform_device *pdev,
>  		return ret;
>  	}

[Severity: High]
This is a pre-existing issue, but does the current driver initialization
order risk a NULL pointer dereference in rockchip_pcie_rc_sys_irq_thread()?

The SYS IRQ is requested with devm_request_threaded_irq() earlier in
rockchip_pcie_configure_rc(), before dw_pcie_host_init() is called.
If the IRQ fires immediately (for example, if the bootloader leaves a
pending link-down reset status), rockchip_pcie_rc_sys_irq_thread() will
iterate over pp->bridge->bus. However, pp->bridge is not allocated until
dw_pcie_host_init() runs, which would cause a crash.

>  
> +	/*
> +	 * This is done here instead of in the host ops .init() callback,
> +	 * which is also re-run by .reset_root_port(), so that the INTx irq
> +	 * domain is only created once, at probe time.
> +	 */
> +	rockchip->intx_irq = of_irq_get_byname(dev->of_node, "legacy");
> +	if (rockchip->intx_irq < 0)
> +		return rockchip->intx_irq;

[Severity: High]
If of_irq_get_byname() fails here, the function returns directly. Since
dw_pcie_host_init() has already successfully executed and registered the
PCI host bridge, does this path need to call dw_pcie_host_deinit() before
returning? 

Without explicit cleanup, devres will free the rockchip_pcie structure on
probe failure, while the PCI subsystem might still retain active references
to the host controller. Can this cause a use-after-free?

> +
> +	ret = rockchip_pcie_init_irq_domain(dev, rockchip);

[Severity: High]
Since the INTx IRQ domain is now instantiated after dw_pcie_host_init()
has completed, will this cause legacy interrupts for downstream devices to
fail unconditionally?

The dw_pcie_host_init() call enumerates the PCI bus and probes downstream
devices. If those devices attempt to request legacy INTx interrupts during
their probe, the INTx IRQ domain will not exist yet.

> +	if (ret < 0) {
> +		dev_err(dev, "failed to init irq domain\n");
> +		return ret;
> +	}

[Severity: High]
Similarly to the above error path, returning here skips tearing down the
enumerated PCI host via dw_pcie_host_deinit(), which leaves dangling
references to the host structure.

> +
> +	irq_set_chained_handler_and_data(rockchip->intx_irq,
> +					 rockchip_pcie_intx_handler, rockchip);
> +
> +	/*
> +	 * Uninstall the chained handler on probe failure, so that it can
> +	 * never run against the devm-freed rockchip structure.
> +	 */
> +	ret = devm_add_action_or_reset(dev, rockchip_pcie_intx_chained_release,
> +				       rockchip);
> +	if (ret)
> +		return ret;

[Severity: High]
This error path also skips dw_pcie_host_deinit(). Should all of these new
error returns be updated to properly unregister the PCI host?

> +
>  	/* unmask hot reset/link-down reset */
>  	val = FIELD_PREP_WM16(PCIE_LINK_REQ_RST_NOT_INT, 0);
>  	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_INTR_MASK_MISC);

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

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

[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@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

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

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

[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@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

Re: [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@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.
> 

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

> 
> Kind regards,
> Niklas
> 


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

Re: [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@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.
> 

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

> 
> Kind regards,
> Niklas
> 

[PATCH v2 2/2] PCI: dw-rockchip: Mask the INTx IRQ while the controller clocks are gated

[Shawn Lin]

.reset_root_port() gates the controller clocks and PHY before
re-initializing the hardware. Devices on the bus may keep their INTx
line asserted across this window, so the chained INTx handler can run
and read the APB registers of the unclocked controller, raising a
synchronous external abort.

Mask the INTx IRQ before turning the clocks off and re-enable it once
the clocks are running again. On the error paths the clocks stay
gated, so the IRQ is deliberately left masked there.

Fixes: b376b3ff9cb0 ("PCI: dw-rockchip: Implement .reset_root_port() and use for link down")
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v2:
- keep the INTx IRQ masked while .reset_root_port()
  gates the controller clocks, responding to the Sashiko review
  finding about accessing the unclocked APB bus.

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

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index ecf0d7e..aee5ca8 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -434,7 +434,6 @@ static void rockchip_pcie_stop_link(struct dw_pcie *pci)
 static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
-	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
 
 	pci->dbi_base2 = pci->dbi_base + PCIE_TYPE0_HDR_DBI2_OFFSET;
 
@@ -925,6 +924,16 @@ static int rockchip_pcie_rc_reset_root_port(struct pci_host_bridge *bridge,
 	u32 val;
 	int ret;
 
+	/*
+	 * Devices may keep their INTx line asserted across the reset. Mask
+	 * the INTx IRQ so that the chained handler does not touch the
+	 * unclocked APB bus, which would raise a synchronous external abort.
+	 * The IRQ is re-enabled once the clocks are restored, and is
+	 * deliberately left masked on the error paths where the controller
+	 * remains unclocked.
+	 */
+	disable_irq(rockchip->intx_irq);
+
 	dw_pcie_stop_link(pci);
 	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
 	rockchip_pcie_phy_deinit(rockchip);
@@ -975,6 +984,7 @@ static int rockchip_pcie_rc_reset_root_port(struct pci_host_bridge *bridge,
 
 	/* Ignore errors, the link may come up later */
 	dw_pcie_wait_for_link(pci);
+	enable_irq(rockchip->intx_irq);
 	dev_dbg(dev, "Root Port reset completed\n");
 	return ret;
 
-- 
2.7.4


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

[PATCH v2 2/2] PCI: dw-rockchip: Mask the INTx IRQ while the controller clocks are gated

[Shawn Lin]

.reset_root_port() gates the controller clocks and PHY before
re-initializing the hardware. Devices on the bus may keep their INTx
line asserted across this window, so the chained INTx handler can run
and read the APB registers of the unclocked controller, raising a
synchronous external abort.

Mask the INTx IRQ before turning the clocks off and re-enable it once
the clocks are running again. On the error paths the clocks stay
gated, so the IRQ is deliberately left masked there.

Fixes: b376b3ff9cb0 ("PCI: dw-rockchip: Implement .reset_root_port() and use for link down")
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v2:
- keep the INTx IRQ masked while .reset_root_port()
  gates the controller clocks, responding to the Sashiko review
  finding about accessing the unclocked APB bus.

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

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index ecf0d7e..aee5ca8 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -434,7 +434,6 @@ static void rockchip_pcie_stop_link(struct dw_pcie *pci)
 static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
-	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
 
 	pci->dbi_base2 = pci->dbi_base + PCIE_TYPE0_HDR_DBI2_OFFSET;
 
@@ -925,6 +924,16 @@ static int rockchip_pcie_rc_reset_root_port(struct pci_host_bridge *bridge,
 	u32 val;
 	int ret;
 
+	/*
+	 * Devices may keep their INTx line asserted across the reset. Mask
+	 * the INTx IRQ so that the chained handler does not touch the
+	 * unclocked APB bus, which would raise a synchronous external abort.
+	 * The IRQ is re-enabled once the clocks are restored, and is
+	 * deliberately left masked on the error paths where the controller
+	 * remains unclocked.
+	 */
+	disable_irq(rockchip->intx_irq);
+
 	dw_pcie_stop_link(pci);
 	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
 	rockchip_pcie_phy_deinit(rockchip);
@@ -975,6 +984,7 @@ static int rockchip_pcie_rc_reset_root_port(struct pci_host_bridge *bridge,
 
 	/* Ignore errors, the link may come up later */
 	dw_pcie_wait_for_link(pci);
+	enable_irq(rockchip->intx_irq);
 	dev_dbg(dev, "Root Port reset completed\n");
 	return ret;
 
-- 
2.7.4

Re: [PATCH v2 2/2] PCI: dw-rockchip: Mask the INTx IRQ while the controller clocks are gated

[sashiko-bot]

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

New issues:
- [High] Unbalanced `disable_irq()` on error paths permanently masks the INTx IRQ if the root port reset is retried.

Pre-existing issues:
- [Critical] Retrying `.reset_root_port()` after a failure causes clock and PHY reference underflows and a synchronous external abort.
- [High] The threaded `sys` IRQ handler can race with `.reset_root_port()` and access unclocked APB registers, causing a synchronous external abort.
--

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

PCI: dw-rockchip: Mask the INTx IRQ while the controller clocks are gated

This patch masks the INTx IRQ during root port reset to prevent the chained
handler from reading APB registers while clocks are gated. The IRQ is then
re-enabled after clocks are restored.

> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index ecf0d7e043b36..aee5ca8a6ca84 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c

[ ... ]

> @@ -925,6 +924,16 @@ static int rockchip_pcie_rc_reset_root_port(struct pci_host_bridge *bridge,
>  	u32 val;
>  	int ret;
>  
> +	/*
> +	 * Devices may keep their INTx line asserted across the reset. Mask
> +	 * the INTx IRQ so that the chained handler does not touch the
> +	 * unclocked APB bus, which would raise a synchronous external abort.
> +	 * The IRQ is re-enabled once the clocks are restored, and is
> +	 * deliberately left masked on the error paths where the controller
> +	 * remains unclocked.
> +	 */
> +	disable_irq(rockchip->intx_irq);
> +

[Severity: High]
If rockchip_pcie_rc_reset_root_port() fails and returns early, it
intentionally skips calling enable_irq() to keep the IRQ masked while the
controller is unclocked.

If the reset is later retried (e.g., via AER recovery or sysfs), will the
second call to disable_irq() increment the IRQ descriptor's nesting depth
counter to 2?

If a subsequent reset attempt succeeds and calls enable_irq() exactly once,
could this regression leave the INTx IRQ permanently masked with a depth of 1,
even though the clocks have been successfully restored?

>  	dw_pcie_stop_link(pci);
>  	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);

[Severity: Critical]
This is a pre-existing issue, but retrying the reset after a failure appears
to be problematic here.

If a previous call to rockchip_pcie_rc_reset_root_port() failed after turning
off the APB clocks and returning early, the clocks and PHY remain
deinitialized.

On a retry, dw_pcie_stop_link() is called immediately while the APB clocks are
still gated. Since dw_pcie_stop_link() accesses the unclocked APB registers,
will this cause a fatal synchronous external abort?

Additionally, could the subsequent calls to clk_bulk_disable_unprepare() and
rockchip_pcie_phy_deinit() cause clock and PHY reference count underflows on
a retry?

[Severity: High]
This isn't a bug introduced by this patch, but does the threaded sys IRQ
handler have a similar clock gating race condition?

The threaded handler rockchip_pcie_rc_sys_irq_thread() reads the APB register
PCIE_CLIENT_INTR_STATUS_MISC without synchronizing against clock gating.

If dw_pcie_stop_link() triggers a link-down event, it could fire the sys IRQ
just before clk_bulk_disable_unprepare() disables the APB clocks. Can this
cause the concurrently running sys IRQ handler to access unclocked APB
registers and trigger a fatal synchronous external abort?

>  	rockchip_pcie_phy_deinit(rockchip);

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