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

[sashiko-bot@kernel.org]

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