* Re: [PATCH] PCI: starfive: Fix Runtime PM handling and teardown ordering
2026-07-13 14:50 [PATCH] PCI: starfive: Fix Runtime PM handling and teardown ordering Ali Tariq
@ 2026-07-13 15:03 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-07-13 15:03 UTC (permalink / raw)
To: Ali Tariq; +Cc: linux-pci
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
^ permalink raw reply [flat|nested] 2+ messages in thread