[PATCH] PCI: starfive: Fix Runtime PM handling and teardown ordering

[PATCH] PCI: starfive: Fix Runtime PM handling and teardown ordering

[Ali Tariq]

The starfive_pcie_remove() path incorrectly disabled runtime PM
before executing plda_pcie_host_deinit(),
which can cause unmanaged hardware register access
while power domains or clocks are disabled.

Fix this by restructuring starfive_pcie_remove() to deinitialize
the host hardware first while runtime PM is active,
followed by a synchronous pm_runtime_put_sync() and pm_runtime_disable().

Signed-off-by: Ali Tariq <alitariq45892@gmail.com>
---
 drivers/pci/controller/plda/pcie-starfive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c
index e9a472985b8a..ba8ef7842e35 100644
--- a/drivers/pci/controller/plda/pcie-starfive.c
+++ b/drivers/pci/controller/plda/pcie-starfive.c
@@ -440,9 +440,9 @@ static void starfive_pcie_remove(struct platform_device *pdev)
 {
 	struct starfive_jh7110_pcie *pcie = platform_get_drvdata(pdev);
 
-	pm_runtime_put(&pdev->dev);
-	pm_runtime_disable(&pdev->dev);
 	plda_pcie_host_deinit(&pcie->plda);
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	platform_set_drvdata(pdev, NULL);
 }
 
-- 
2.34.1

Re: [PATCH] PCI: starfive: Fix Runtime PM handling and teardown ordering

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver fails to check the return value of `pm_runtime_get_sync()` during probe, risking a kernel panic.
- [High] `starfive_pcie_host_init()` leaks PHY resources and completely ignores `regulator_enable()` failures.
- [High] Unsynchronized teardown of chained IRQ handlers risks a race condition and kernel panic.
- [Medium] IRQ domains created in `plda_pcie_init_irq_domains()` are permanently leaked on probe failure.
- [Medium] IRQ domains are removed without first disposing of their active IRQ mappings, triggering a kernel warning.
- [Medium] `starfive_pcie_hide_rc_bar()` incorrectly relies on exact equality checks, allowing its protection to be bypassed via unaligned accesses.
--

--- Patch [1]: [PATCH] PCI: starfive: Fix Runtime PM handling and teardown ordering ---
commit 179d6c49f89a16ca3a07a0fadbdedce7703762df
Author: Ali Tariq <alitariq45892@gmail.com>

PCI: starfive: Fix Runtime PM handling and teardown ordering

The starfive_pcie_remove() path incorrectly disabled runtime PM
before executing plda_pcie_host_deinit(),
which can cause unmanaged hardware register access
while power domains or clocks are disabled.

> diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c
> index 298036c3e7f94..6b439378e82eb 100644
> --- a/drivers/pci/controller/plda/pcie-starfive.c
> +++ b/drivers/pci/controller/plda/pcie-starfive.c
> @@ -436,9 +436,9 @@ static void starfive_pcie_remove(struct platform_device *pdev)
>  {
>  	struct starfive_jh7110_pcie *pcie = platform_get_drvdata(pdev);
>  
> -	pm_runtime_put(&pdev->dev);
> -	pm_runtime_disable(&pdev->dev);
>  	plda_pcie_host_deinit(&pcie->plda);
> +	pm_runtime_put_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
>  	platform_set_drvdata(pdev, NULL);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but doesn't starfive_pcie_probe()
ignore the return value of pm_runtime_get_sync()?

drivers/pci/controller/plda/pcie-starfive.c:starfive_pcie_probe() {
    ...
	pm_runtime_enable(&pdev->dev);
	pm_runtime_get_sync(&pdev->dev);

	plda->host_ops = &sf_host_ops;
    ...
}

If pm_runtime_get_sync() fails, won't it return a negative error code but leave
the device suspended?  Since the usage counter is still incremented on failure,
does the lack of error handling cause a PM usage count leak and risk hardware
access on powered-off devices during the subsequent plda_pcie_host_init()?

[Severity: High]
This is a pre-existing issue, but does starfive_pcie_host_init() leak PHY
resources and ignore regulator enable failures?

drivers/pci/controller/plda/pcie-starfive.c:starfive_pcie_host_init() {
    ...
	ret = starfive_pcie_clk_rst_init(pcie);
	if (ret)
		return ret;

	if (pcie->vpcie3v3) {
		ret = regulator_enable(pcie->vpcie3v3);
		if (ret)
			dev_err_probe(dev, ret, "failed to enable vpcie3v3 regulator\n");
	}
    ...
}

If starfive_pcie_clk_rst_init() fails, doesn't it return immediately without
calling starfive_pcie_disable_phy(), permanently leaving the PHY powered on?

Also, if regulator_enable() fails, shouldn't there be a return statement after
the dev_err_probe() call to prevent initializing the controller without power?

[Severity: Medium]
This isn't a bug introduced by this patch, but are IRQ domains permanently
leaked on probe failure?

drivers/pci/controller/plda/pcie-plda-host.c:plda_init_interrupts() {
    ...
	port->irq = platform_get_irq(pdev, 0);
	if (port->irq < 0)
		return -ENODEV;
    ...
}

If this returns an error, its caller plda_pcie_host_init() handles it here:

drivers/pci/controller/plda/pcie-plda-host.c:plda_pcie_host_init() {
    ...
err_host:
	if (port->host_ops && port->host_ops->host_deinit)
		port->host_ops->host_deinit(port);

	return ret;
}

Doesn't this err_host cleanup path omit the required call to
plda_pcie_irq_domain_deinit(), permanently leaking the IRQ domains and their
memory?

[Severity: High]
This is a pre-existing issue, but is there an unsynchronized teardown of
chained IRQ handlers here?

drivers/pci/controller/plda/pcie-plda-host.c:plda_pcie_irq_domain_deinit() {
    ...
	irq_set_chained_handler_and_data(pcie->irq, NULL, NULL);
	irq_set_chained_handler_and_data(pcie->msi_irq, NULL, NULL);
	irq_set_chained_handler_and_data(pcie->intx_irq, NULL, NULL);

	irq_domain_remove(pcie->msi.dev_domain);
    ...
}

Since irq_set_chained_handler_and_data() doesn't wait for executing handlers
to finish, could an interrupt fire on another CPU and execute concurrently
with the teardown process?

If the hardware clocks are disabled in host_deinit(), could this trigger a
fatal bus fault when the executing handler reads un-clocked PCIe registers?
Should synchronize_irq() be called here?

[Severity: Medium]
This isn't a bug introduced by this patch, but are IRQ domains removed without
disposing of their active mappings?

drivers/pci/controller/plda/pcie-plda-host.c:plda_pcie_irq_domain_deinit() {
    ...
	irq_domain_remove(pcie->msi.dev_domain);

	irq_domain_remove(pcie->intx_domain);
	irq_domain_remove(pcie->event_domain);
    ...
}

Since hardware interrupts were mapped using irq_create_mapping() during
initialization, doesn't directly removing the domains without calling
irq_dispose_mapping() trigger a warning in the core IRQ subsystem?

[Severity: Medium]
This is a pre-existing issue, but can the protection in
starfive_pcie_hide_rc_bar() be bypassed?

drivers/pci/controller/plda/pcie-starfive.c:starfive_pcie_hide_rc_bar() {
    ...
	if (pci_is_root_bus(bus) && !devfn &&
	    (offset == PCI_BASE_ADDRESS_0 || offset == PCI_BASE_ADDRESS_1))
		return true;

	return false;
}

Because this uses an exact equality check, what happens if there is a 1-byte
or 2-byte access at an unaligned offset (e.g., offset = 0x11)?

Does this evaluate to false and pass the access through, bypassing the
hardware protection boundary?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713145014.740898-1-alitariq45892@gmail.com?part=1

[PATCH v2] PCI: starfive: Fix Runtime PM handling and teardown ordering

[Ali Tariq]

The starfive_pcie_remove() path incorrectly disabled runtime PM
before executing plda_pcie_host_deinit(),
which can cause unmanaged hardware register access
while power domains or clocks are disabled.

Fix this by restructuring starfive_pcie_remove() to deinitialize
the host hardware first while runtime PM is active,
followed by a synchronous pm_runtime_put_sync() and pm_runtime_disable().

This bug was found in automated AI review by sashiko-bot.

Fixes: 39b91eb40c6a ("PCI: starfive: Add JH7110 PCIe controller")
Link: https://lore.kernel.org/linux-pci/20260712180440.423421F000E9@smtp.kernel.org/
Signed-off-by: Ali Tariq <alitariq45892@gmail.com>
---
Changes in v2:
- Added Fixes tag to commit message
- Added reference link to the bug found by sashiko-bot
---
 drivers/pci/controller/plda/pcie-starfive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c
index e9a472985b8a..ba8ef7842e35 100644
--- a/drivers/pci/controller/plda/pcie-starfive.c
+++ b/drivers/pci/controller/plda/pcie-starfive.c
@@ -440,9 +440,9 @@ static void starfive_pcie_remove(struct platform_device *pdev)
 {
 	struct starfive_jh7110_pcie *pcie = platform_get_drvdata(pdev);
 
-	pm_runtime_put(&pdev->dev);
-	pm_runtime_disable(&pdev->dev);
 	plda_pcie_host_deinit(&pcie->plda);
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
 	platform_set_drvdata(pdev, NULL);
 }
 
-- 
2.34.1

Re: [PATCH v2] PCI: starfive: Fix Runtime PM handling and teardown ordering

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver ignores the return value of `pm_runtime_get_sync()` during probe, which can lead to unmanaged hardware register accesses.
- [High] `starfive_pcie_host_init()` leaks resources on error paths and silently ignores `regulator_enable()` failures.
- [High] IRQ domains are permanently leaked on error paths during initialization.
- [High] Use-After-Free of `irq_domain` and access to disabled hardware due to improper `devres` interrupt teardown.
- [High] Leaked virtual IRQ mappings (virqs) because `irq_dispose_mapping()` is never called on teardown.
- [Medium] Incorrect wrapper macro `pm_sleep_ptr()` used for the `dev_pm_ops` structure pointer.
--

--- Patch [1]: [PATCH v2] PCI: starfive: Fix Runtime PM handling and teardown ordering ---
commit 8242552d46d79f7bcd9a83c741e3ab1492a12e4c
Author: Ali Tariq <alitariq45892@gmail.com>

PCI: starfive: Fix Runtime PM handling and teardown ordering

The starfive_pcie_remove() path incorrectly disabled runtime PM before
executing plda_pcie_host_deinit(), which can cause unmanaged hardware register
access while power domains or clocks are disabled. Fix this by restructuring
starfive_pcie_remove() to deinitialize the host hardware first while runtime
PM is active, followed by a synchronous pm_runtime_put_sync() and
pm_runtime_disable().

Link: https://lore.kernel.org/linux-pci/20260712180440.423421F000E9@smtp.kernel.org/

> diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c
> index 298036c3e7f94..6b439378e82eb 100644
> --- a/drivers/pci/controller/plda/pcie-starfive.c
> +++ b/drivers/pci/controller/plda/pcie-starfive.c
> @@ -436,9 +436,9 @@ static void starfive_pcie_remove(struct platform_device *pdev)
>  {
>  	struct starfive_jh7110_pcie *pcie = platform_get_drvdata(pdev);
>  
> -	pm_runtime_put(&pdev->dev);
> -	pm_runtime_disable(&pdev->dev);
>  	plda_pcie_host_deinit(&pcie->plda);

[Severity: High]
This isn't a bug introduced by this patch, but does this deinitialize the IRQ
domains and hardware clocks before devres has freed the interrupts requested
with devm_request_irq()?

The driver allocates event interrupts using devm_request_irq(), which delays
free_irq() until after starfive_pcie_remove() completes. However, calling
plda_pcie_host_deinit() manually removes the irq_domain structures and fully
disables the hardware clocks and PHY.

When devres eventually invokes free_irq() on device unbind, the kernel will
dereference the freed IRQ domain and invoke the masking callback:

drivers/pci/controller/plda/pcie-plda-host.c:plda_mask_event_irq() {
	...
	val = readl_relaxed(port->bridge_addr + IMASK_LOCAL);
	val &= ~mask;
	writel_relaxed(val, port->bridge_addr + IMASK_LOCAL);
	...
}

Can this access hardware registers when they have already been unclocked,
resulting in a Use-After-Free and a synchronous external abort?

> +	pm_runtime_put_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
>  	platform_set_drvdata(pdev, NULL);
>  }

While reviewing this code, I noticed a few other pre-existing issues in
related areas:

drivers/pci/controller/plda/pcie-starfive.c:starfive_pcie_probe() {
	...
	pm_runtime_enable(&pdev->dev);
	pm_runtime_get_sync(&pdev->dev);

[Severity: High]
This is a pre-existing issue, but is the return value of pm_runtime_get_sync()
ignored here during probe?

If pm_runtime_get_sync() fails to enable the PM domain or clocks, the error
will go undetected. Will the driver then proceed to access unpowered hardware
registers, potentially causing a synchronous external abort?


drivers/pci/controller/plda/pcie-starfive.c:starfive_pcie_driver {
	...
	.driver = {
		.name = "pcie-starfive",
		.of_match_table = of_match_ptr(starfive_pcie_of_match),
		.pm = pm_sleep_ptr(&starfive_pcie_pm_ops),

[Severity: Medium]
This isn't a bug introduced by this patch, but shouldn't pm_ptr() be used here
instead of pm_sleep_ptr() for the dev_pm_ops structure pointer?

The Power Management Subsystem guidelines mandate using pm_ptr() for the
structure pointer when PM callbacks are involved. Using pm_sleep_ptr()
inappropriately ties the structure's inclusion directly to CONFIG_PM_SLEEP
rather than CONFIG_PM.


drivers/pci/controller/plda/pcie-starfive.c:starfive_pcie_host_init() {
	...
		ret = regulator_enable(pcie->vpcie3v3);
		if (ret)
			dev_err_probe(dev, ret, "failed to enable vpcie3v3 regulator\n");

[Severity: High]
This is a pre-existing issue, but is there a missing return statement after
the regulator_enable() failure?

If regulator_enable() fails, it prints an error via dev_err_probe() but
execution proceeds to touch hardware without the required 3.3V supply.

Also, if starfive_pcie_clk_rst_init() fails earlier in this function, it
returns the error immediately without disabling the previously enabled PHY.
Does this permanently leak the PHY resources?


drivers/pci/controller/plda/pcie-plda-host.c:plda_pcie_init_irq_domains() {
	...
	port->intx_domain = irq_domain_create_linear(of_fwnode_handle(pcie_intc_node), PCI_NUM_INTX,
						     &intx_domain_ops, port);
	if (!port->intx_domain) {
		dev_err(dev, "failed to get an INTx IRQ domain\n");
		of_node_put(pcie_intc_node);
		return -ENOMEM;

[Severity: High]
This isn't a bug introduced by this patch, but does this error path leak the
event_domain created immediately prior?


drivers/pci/controller/plda/pcie-plda-host.c:plda_pcie_host_init() {
	...
	ret = plda_init_interrupts(pdev, port, plda_event);
	if (ret)
		goto err_host;

[Severity: High]
This is a pre-existing issue, but does this error jump bypass the err_probe
label where plda_pcie_irq_domain_deinit() would normally clean up the IRQ
domains?

If plda_init_interrupts() fails during probe, jumping directly to err_host
completely skips the irq_domain cleanup, causing a resource leak.


drivers/pci/controller/plda/pcie-plda-host.c:plda_pcie_irq_domain_deinit() {
	...
	irq_domain_remove(pcie->msi.dev_domain);

	irq_domain_remove(pcie->intx_domain);
	irq_domain_remove(pcie->event_domain);

[Severity: High]
This isn't a bug introduced by this patch, but are these domains removed
without first disposing of their associated IRQ mappings?

The driver uses irq_create_mapping() to allocate virtual IRQs for event_irq,
intx_irq, and msi_irq. Removing the domains without disposing of the mapped
virqs via irq_dispose_mapping() leaks the IRQ descriptors and mapping entries.
Could this trigger a WARN_ON within irq_domain_remove() because the domain's
radix tree still contains active mappings on teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718133825.445041-1-alitariq45892@gmail.com?part=1

Re: [PATCH v2] PCI: starfive: Fix Runtime PM handling and teardown ordering

[Ali Tariq]

Politely pinging.

Did anyone get a chance to review this patch?

I am happy to send changes if required.

Regards,
Ali

Re: [PATCH v2] PCI: starfive: Fix Runtime PM handling and teardown ordering

[Manivannan Sadhasivam]

On Sat, 18 Jul 2026 18:38:25 +0500, Ali Tariq wrote:
> The starfive_pcie_remove() path incorrectly disabled runtime PM
> before executing plda_pcie_host_deinit(),
> which can cause unmanaged hardware register access
> while power domains or clocks are disabled.
> 
> Fix this by restructuring starfive_pcie_remove() to deinitialize
> the host hardware first while runtime PM is active,
> followed by a synchronous pm_runtime_put_sync() and pm_runtime_disable().
> 
> [...]

Applied, thanks!

[1/1] PCI: starfive: Fix Runtime PM handling and teardown ordering
      commit: fb9f7973473fc30d62e0f5f90d59df8ef5223777

Best regards,
-- 
மணிவண்ணன் சதாசிவம்