[PATCH 0/3] PCI: rzg3s: Fix issues in cleanup paths

[PATCH 0/3] PCI: rzg3s: Fix issues in cleanup paths

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi all,

This series fixes a few issues in the cleanup paths of the Renesas RZ/G3S
PCIe host controller driver. The fixes include:
- Disabling the PCIe reference clock on probe failure.
- Propagating the original error code from platform_get_irq_byname() to
allow probe deferral.
- Unwinding chained handler registration on error in
rzg3s_pcie_init_irqdomain() and clearing chained handlers in
rzg3s_pcie_teardown_irqdomain() to avoid leaving stale handler and data
pointers attached to the parent interrupts.

Note:
- This patch applies on top of pci-next (commit - 5d9f1dd3b3e7).
- There is a patch [0] inflight for this driver which doesn't apply
  on pci-next. As there is a conflict between the two, the patch [0]
  will be rebased on top of this series and v3 will be sent out for review.

[0] https://lore.kernel.org/all/20260630141720.3938514-1-john.madieu.xa@bp.renesas.com/

Cheers,
Prabhakar

Lad Prabhakar (3):
  PCI: rzg3s: Disable refclk on probe failure
  PCI: rzg3s: Propagate platform_get_irq_byname() errors
  PCI: rzg3s: Fix IRQ domain initialization error handling

 drivers/pci/controller/pcie-rzg3s-host.c | 38 +++++++++++++++++-------
 1 file changed, 28 insertions(+), 10 deletions(-)

-- 
2.54.0

[PATCH 1/3] PCI: rzg3s: Disable refclk on probe failure

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

rzg3s_pcie_host_init_port() enables the PCIe reference clock using
clk_prepare_enable(), but if rzg3s_pcie_probe() subsequently fails, for
example because pci_host_probe() returns an error, the probe error path
only releases the clock handle with clk_put().

Balance clk_prepare_enable() by calling clk_disable_unprepare() in the
probe cleanup path before releasing the clock, ensuring the reference
clock is properly disabled on probe failure.

Fixes: 7ef502fb35b28 ("PCI: Add Renesas RZ/G3S host controller driver")
Cc: stable@vger.kernel.org
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/pci/controller/pcie-rzg3s-host.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index a1a78bdb7af1..01488660ab08 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1897,6 +1897,7 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
 	return 0;
 
 host_probe_teardown:
+	clk_disable_unprepare(host->port.refclk);
 	rzg3s_pcie_teardown_irqdomain(host);
 	host->data->config_deinit(host);
 rpm_put:
-- 
2.54.0

Re: [PATCH 1/3] PCI: rzg3s: Disable refclk on probe failure

[sashiko-bot]

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

New issues:
- [Medium] The patch violates hardware teardown order by disabling the PCIe reference clock before asserting controller resets.

Pre-existing issues:
- [Critical] Missing IRQ cleanup for chained INTx handlers on probe failure leads to use-after-free.
- [High] MMIO register access after PM runtime clock gating in suspend path causes system hang.
--

--- Patch [1]: [PATCH 1/3] PCI: rzg3s: Disable refclk on probe failure ---
commit 0d4ef239e6084c21b4710c1db03e3a6365363533
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Subject: PCI: rzg3s: Disable refclk on probe failure

This commit disables the PCIe reference clock when the probe function fails.
It adds a call to clk_disable_unprepare in the error path, ensuring the
clock is balanced if initialization fails.

Cc: stable@vger.kernel.org

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index 66f687304c1cb..bd6471a7fb7a1 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
> @@ -1762,6 +1762,7 @@ static int rzg3s_pcie_probe(struct platform_device *pdev)
>  	return 0;
>  
>  host_probe_teardown:
> +	clk_disable_unprepare(host->port.refclk);

[Severity: Medium]
Does this new addition violate the hardware teardown order by disabling the
reference clock before asserting controller resets?

During setup, the reference clock is enabled prior to calling
config_post_init() which deasserts resets. In this new error path,
clk_disable_unprepare() is called before host->data->config_deinit(host).

Could disabling the clock while the core is out of reset leave the state
machine hanging or undefined? Should the clock disable be moved after the
config_deinit() call to maintain strict LIFO initialization order?

>  	rzg3s_pcie_teardown_irqdomain(host);

[Severity: Critical]
This is a pre-existing issue, but does the teardown function fail to clean
up chained handlers on probe failure?

Looking at rzg3s_pcie_teardown_irqdomain(), it removes the IRQ domain but
does not unregister the chained INTx hardware handlers:

drivers/pci/controller/pcie-rzg3s-host.c:rzg3s_pcie_teardown_irqdomain() {
        if (IS_ENABLED(CONFIG_PCI_MSI))
                rzg3s_pcie_teardown_msi(host);

        irq_domain_remove(host->intx_domain);
}

If pci_host_probe() fails and the devres mechanism frees the host structure,
could a shared or spurious INTx interrupt firing later cause
rzg3s_pcie_intx_irq_handler() to dereference the freed host pointer? Should
this call irq_set_chained_handler_and_data(irq, NULL, NULL) to prevent a
use-after-free?

>  	host->data->config_deinit(host);
>  rpm_put:

[Severity: High]
This is a pre-existing issue, but can the suspend path cause a system hang
due to MMIO access after PM runtime clock gating?

In rzg3s_pcie_suspend_noirq(), the driver disables the device PM domain
clocks before calling the hardware de-initialization routine:

drivers/pci/controller/pcie-rzg3s-host.c:rzg3s_pcie_suspend_noirq() {
        ...
        ret = pm_runtime_put_sync(dev);
        if (ret)
                return ret;

        clk_disable_unprepare(port->refclk);

        /* SoC-specific de-initialization */
        ret = data->config_deinit(host);
        ...
}

For the RZ/G3E SoC, the de-initialization callback performs an MMIO write:

drivers/pci/controller/pcie-rzg3s-host.c:rzg3e_pcie_config_deinit() {
        writel_relaxed(0, host->axi + RZG3S_PCI_RESET);
        return 0;
}

Will accessing this MMIO region while the bus clock is gated trigger a fatal
bus fault on ARM64 during system suspend? Should the de-initialization
happen before pm_runtime_put_sync() is called?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715223019.3002237-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=1

[PATCH 2/3] PCI: rzg3s: Propagate platform_get_irq_byname() errors

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

platform_get_irq_byname() may return specific error codes such as
-EPROBE_DEFER, but rzg3s_pcie_init_irqdomain() replaces all failures
with -EINVAL.

This breaks probe deferral by preventing the driver core from retrying
probe once the interrupt provider becomes available.

Propagate the original error code instead.

Fixes: 7ef502fb35b28 ("PCI: Add Renesas RZ/G3S host controller driver")
Cc: stable@vger.kernel.org
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/pci/controller/pcie-rzg3s-host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 01488660ab08..f51b5d7c1998 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1019,7 +1019,7 @@ static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
 
 		irq = platform_get_irq_byname(pdev, irq_name);
 		if (irq < 0)
-			return dev_err_probe(dev, -EINVAL,
+			return dev_err_probe(dev, irq,
 					     "Failed to parse and map INT%c IRQ\n",
 					     'A' + i);
 
-- 
2.54.0

Re: [PATCH 2/3] PCI: rzg3s: Propagate platform_get_irq_byname() errors

[sashiko-bot]

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

Pre-existing issues:
- [High] Failure to clear chained INTx IRQ handlers on error paths and during teardown leads to a use-after-free on probe failure.
- [High] System suspend path accesses AXI registers after disabling the PM domain, causing an asynchronous external abort.
- [High] INTx handler calculates hardware IRQ index assuming virtual IRQs are contiguous, which can lead to IRQ storms and lockups.
--

--- Patch [2]: [PATCH 2/3] PCI: rzg3s: Propagate platform_get_irq_byname() errors ---
commit 24fb1c19eb92c4744be36d3ec0690bbefc19cef4
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

PCI: rzg3s: Propagate platform_get_irq_byname() errors

This commit modifies rzg3s_pcie_init_irqdomain() to propagate the specific
error code returned by platform_get_irq_byname() instead of overriding it
with -EINVAL. This ensures probe deferral works correctly if the interrupt
provider is not yet available.

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> index bd6471a7fb7a1..92371310d313b 100644
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
> @@ -975,7 +975,7 @@ static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
>  
>  		irq = platform_get_irq_byname(pdev, irq_name);
>  		if (irq < 0)
> -			return dev_err_probe(dev, -EINVAL,
> +			return dev_err_probe(dev, irq,
>  					     "Failed to parse and map INT%c IRQ\n",
>  					     'A' + i);
>  

[Severity: High]
This is a pre-existing issue, but returning directly from this loop on
error without cleaning up chained INTx handlers leads to a use-after-free.

If platform_get_irq_byname() fails for i > 0 (for example, by returning
-EPROBE_DEFER), does the driver leave chained handlers registered for the
previous iterations?

When the probe fails, the host structure is freed by devm.

If the parent interrupt fires during the probe deferral waiting period,
could the kernel execute rzg3s_pcie_intx_irq_handler() with a dangling
pointer?

Similarly, should rzg3s_pcie_teardown_irqdomain() clear these handlers?

It currently removes the IRQ domain but completely omits cleaning up the
INTx chained handlers, which might leave them dangling on unbind.


[Severity: High]
This is a pre-existing issue, but does the system suspend path access AXI
registers after disabling the PM domain?

In drivers/pci/controller/pcie-rzg3s-host.c:rzg3s_pcie_suspend_noirq():

	ret = pm_runtime_put_sync(dev);
	if (ret)
		return ret;
	
	clk_disable_unprepare(port->refclk);
	
	/* SoC-specific de-initialization */
	ret = data->config_deinit(host);

This disables the power domain before executing SoC-specific
de-initialization.

For RZ/G3E, rzg3e_pcie_config_deinit() performs a writel_relaxed() to
host->axi + RZG3S_PCI_RESET.

Can accessing the AXI register space without clocks trigger a hardware fault?


[Severity: High]
This is a pre-existing issue, but does the INTx handler incorrectly assume
virtual IRQs are strictly contiguous?

In drivers/pci/controller/pcie-rzg3s-host.c:rzg3s_pcie_intx_irq_handler():

	unsigned int irq = irq_desc_get_irq(desc);
	u32 intx = irq - host->intx_irqs[0];

This calculates the hardware interrupt index assuming dynamically allocated
virtual IRQ numbers from platform_get_irq_byname() are contiguous.

Because this driver opts into PROBE_PREFER_ASYNCHRONOUS, concurrent IRQ
allocations by other drivers could mean virqs are not sequential.

If intx evaluates to an out-of-bounds index, generic_handle_domain_irq()
will fail to find the mapping.

Could this leave the level-triggered INTx line asserted but unserviced,
resulting in an IRQ storm?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715223019.3002237-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=2

[PATCH 3/3] PCI: rzg3s: Fix IRQ domain initialization error handling

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

rzg3s_pcie_init_irqdomain() installs chained handlers for the INTx
parent interrupts before creating the INTx irqdomain and initializing
MSI support.

If any subsequent step fails, such as obtaining an INTx IRQ,
creating the irqdomain, or initializing MSI, the error path returns
without removing any chained handlers that were already installed.
This leaves stale handler and data pointers attached to the parent
interrupts.

Fix the error path by unwinding the chained handler registration before
returning. Also clear the chained handlers during
rzg3s_pcie_teardown_irqdomain() so that teardown fully mirrors the
initialization sequence.

Fixes: 7ef502fb35b28 ("PCI: Add Renesas RZ/G3S host controller driver")
Cc: stable@vger.kernel.org
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/pci/controller/pcie-rzg3s-host.c | 37 +++++++++++++++++-------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index f51b5d7c1998..5aeb030d43c8 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -1006,22 +1006,30 @@ static const struct irq_domain_ops rzg3s_pcie_intx_domain_ops = {
 	.xlate = irq_domain_xlate_onetwocell,
 };
 
+static void rzg3s_pcie_clear_intx_handlers(struct rzg3s_pcie_host *host, int count)
+{
+	while (--count >= 0)
+		irq_set_chained_handler_and_data(host->intx_irqs[count], NULL, NULL);
+}
+
 static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
 {
 	struct device *dev = host->dev;
 	struct platform_device *pdev = to_platform_device(dev);
+	int i, ret;
 
-	for (int i = 0; i < PCI_NUM_INTX; i++) {
+	for (i = 0; i < PCI_NUM_INTX; i++) {
 		char irq_name[5] = {0};
 		int irq;
 
 		scnprintf(irq_name, ARRAY_SIZE(irq_name), "int%c", 'a' + i);
 
 		irq = platform_get_irq_byname(pdev, irq_name);
-		if (irq < 0)
-			return dev_err_probe(dev, irq,
-					     "Failed to parse and map INT%c IRQ\n",
-					     'A' + i);
+		if (irq < 0) {
+			ret = irq;
+			dev_err_probe(dev, ret, "Failed to parse and map INT%c IRQ\n", 'A' + i);
+			goto err_cleanup_intx;
+		}
 
 		host->intx_irqs[i] = irq;
 		irq_set_chained_handler_and_data(irq,
@@ -1033,21 +1041,28 @@ static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
 						     PCI_NUM_INTX,
 						     &rzg3s_pcie_intx_domain_ops,
 						     host);
-	if (!host->intx_domain)
-		return dev_err_probe(dev, -EINVAL,
-				     "Failed to add irq domain for INTx IRQs\n");
+	if (!host->intx_domain) {
+		ret = -EINVAL;
+		dev_err_probe(dev, ret, "Failed to add irq domain for INTx IRQs\n");
+		goto err_cleanup_intx;
+	}
 	irq_domain_update_bus_token(host->intx_domain, DOMAIN_BUS_WIRED);
 
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
-		int ret = rzg3s_pcie_init_msi(host);
+		ret = rzg3s_pcie_init_msi(host);
 
 		if (ret) {
 			irq_domain_remove(host->intx_domain);
-			return ret;
+			goto err_cleanup_intx;
 		}
 	}
 
 	return 0;
+
+err_cleanup_intx:
+	rzg3s_pcie_clear_intx_handlers(host, i);
+
+	return ret;
 }
 
 static void rzg3s_pcie_teardown_irqdomain(struct rzg3s_pcie_host *host)
@@ -1055,6 +1070,8 @@ static void rzg3s_pcie_teardown_irqdomain(struct rzg3s_pcie_host *host)
 	if (IS_ENABLED(CONFIG_PCI_MSI))
 		rzg3s_pcie_teardown_msi(host);
 
+	rzg3s_pcie_clear_intx_handlers(host, PCI_NUM_INTX);
+
 	irq_domain_remove(host->intx_domain);
 }
 
-- 
2.54.0

Re: [PATCH 3/3] PCI: rzg3s: Fix IRQ domain initialization error handling

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-After-Free window in INTx IRQ domain error handling path.
- [High] Chained INTx handlers are registered before their dependent IRQ domain is created.
- [High] Unsafe contiguous virtual IRQ assumption leads to incorrect hardware IRQ index calculation.
- [High] Read-Modify-Write operations corrupt Write-1-to-Clear (W1C) interrupt status registers.
--

--- Patch [3]: [PATCH 3/3] PCI: rzg3s: Fix IRQ domain initialization error handling ---
commit 7daa41537d847b29b2790ef6fee098277445112b
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
PCI: rzg3s: Fix IRQ domain initialization error handling

This commit attempts to fix the error path during IRQ domain initialization
by ensuring chained handlers are unregistered if subsequent steps fail. It
also updates the teardown path to clear INTx handlers.

> diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
> --- a/drivers/pci/controller/pcie-rzg3s-host.c
> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
[ ... ]
>  static int rzg3s_pcie_init_irqdomain(struct rzg3s_pcie_host *host)
>  {
[ ... ]
>  	for (i = 0; i < PCI_NUM_INTX; i++) {
[ ... ]
>  		host->intx_irqs[i] = irq;
>  		irq_set_chained_handler_and_data(irq,
>  						 rzg3s_pcie_intx_irq_handler,
>  						 host);
>  	}
>  
>  	host->intx_domain = irq_domain_create_linear(dev_fwnode(dev),
>  						     PCI_NUM_INTX,
>  						     &rzg3s_pcie_intx_domain_ops,
>  						     host);

[Severity: High]
This is a pre-existing issue, but does this sequence leave a window where
interrupts can be misrouted?

If a hardware INTx interrupt fires immediately after unmasking via
irq_set_chained_handler_and_data(), rzg3s_pcie_intx_irq_handler() will execute
while host->intx_domain is still NULL.

Passing a NULL domain to generic_handle_domain_irq() causes the IRQ core to
fall back to the system's irq_default_domain, which might route spurious
interrupts to unrelated devices.

>  	if (IS_ENABLED(CONFIG_PCI_MSI)) {
> -		int ret = rzg3s_pcie_init_msi(host);
> +		ret = rzg3s_pcie_init_msi(host);
>  
>  		if (ret) {
>  			irq_domain_remove(host->intx_domain);
> -			return ret;
> +			goto err_cleanup_intx;
>  		}
>  	}
>  
>  	return 0;
> +
> +err_cleanup_intx:
> +	rzg3s_pcie_clear_intx_handlers(host, i);
> +
> +	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the error cleanup order
create a use-after-free window?

If rzg3s_pcie_init_msi() fails, the code calls
irq_domain_remove(host->intx_domain) and then jumps to err_cleanup_intx to
clear the handlers.

If a hardware INTx interrupt fires between the domain removal and unregistering
the handler, rzg3s_pcie_intx_irq_handler() could dereference the already freed
host->intx_domain pointer.

[Severity: High]
This is a pre-existing issue in the handler itself. Is it safe to assume
platform IRQs are strictly contiguous?

rzg3s_pcie_intx_irq_handler() calculates the hardware index by subtracting
virtual Linux IRQ numbers:

rzg3s_pcie_intx_irq_handler() {
        ...
        unsigned int irq = irq_desc_get_irq(desc);
        u32 intx = irq - host->intx_irqs[0];
        ...
}

Since platform_get_irq_byname() dynamically requests platform IRQs, they are
not guaranteed to be contiguous. If they are disjoint, this subtraction would
produce an out-of-bounds index for generic_handle_domain_irq().

[Severity: High]
This is another pre-existing issue regarding interrupt acknowledgment. Will
using a read-modify-write operation on a Write-1-to-Clear (W1C) register
silently drop concurrently pending interrupts?

For example, in rzg3s_pcie_msi_irq():

rzg3s_pcie_msi_irq() {
        ...
        /* Clear the MSI */
        rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PINTRCVIS,
                               RZG3S_PCI_PINTRCVIS_MSI,
                               RZG3S_PCI_PINTRCVIS_MSI);
        ...
}

If RZG3S_PCI_PINTRCVIS is a W1C register, reading the current state and
writing it back means any concurrently pending interrupts (read as 1) will be
written back as 1, inadvertently clearing them before they can be handled.

A similar operation happens in rzg3s_pcie_intx_irq_ack().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715223019.3002237-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=3