[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