[PATCH] PCI: starfive: Fix unchecked pm_runtime_get_sync() in probe

[PATCH] PCI: starfive: Fix unchecked pm_runtime_get_sync() in probe

[Ali Tariq]

pm_runtime_get_sync() is called in starfive_pcie_probe() without
checking its return value. If runtime resume fails, the driver
proceeds to configure PCIe hardware through regmap_update_bits(),
enable clocks and resets, and power on the PHY, even though the
device may not actually be powered.

pm_runtime_get_sync() also increments the usage counter even when
resume fails, which would leave the counter unbalanced if this
error path were later handled without additional cleanup.

Switch to pm_runtime_resume_and_get(), which balances the usage
counter internally on failure, and bail out of probe before any
hardware is touched if resume does not succeed.

Tested on StarFive VisionFive 2 v1.2A board.

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

diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c
index 298036c3e7f9..e9a472985b8a 100644
--- a/drivers/pci/controller/plda/pcie-starfive.c
+++ b/drivers/pci/controller/plda/pcie-starfive.c
@@ -410,7 +410,11 @@ static int starfive_pcie_probe(struct platform_device *pdev)
 		return ret;
 
 	pm_runtime_enable(&pdev->dev);
-	pm_runtime_get_sync(&pdev->dev);
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret < 0) {
+		pm_runtime_disable(&pdev->dev);
+		return dev_err_probe(dev, ret, "failed to resume device\n");
+	}
 
 	plda->host_ops = &sf_host_ops;
 	plda->num_events = PLDA_MAX_EVENT_NUM;
-- 
2.34.1

Re: [PATCH] PCI: starfive: Fix unchecked pm_runtime_get_sync() in probe

[sashiko-bot]

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

Pre-existing issues:
- [High] The driver's removal path incorrectly calls asynchronous pm_runtime_put() before pm_runtime_disable(), and tears down PM references before disabling the hardware.
- [High] Failure in starfive_pcie_clk_rst_init() returns directly without powering off the PHY, causing a resource leak.
- [High] Missing return statement after regulator_enable() failure causes a silent fallthrough, later leading to an unbalanced regulator_disable().
--

--- Patch [1]: [PATCH] PCI: starfive: Fix unchecked pm_runtime_get_sync() in probe ---
commit 83a329f8aee98c4aec071468fc96ecc89497124e
Author: Ali Tariq <alitariq45892@gmail.com>

PCI: starfive: Fix unchecked pm_runtime_get_sync() in probe

This patch replaces pm_runtime_get_sync() with pm_runtime_resume_and_get()
in starfive_pcie_probe() to properly handle runtime resume failures. It
adds error handling to disable runtime PM and bail out of the probe
function if the device fails to resume, preventing unbalanced usage
counters and hardware configuration on an unpowered device.

> diff --git a/drivers/pci/controller/plda/pcie-starfive.c b/drivers/pci/controller/plda/pcie-starfive.c
> index 298036c3e7f94..e9a472985b8ac 100644
> --- a/drivers/pci/controller/plda/pcie-starfive.c
> +++ b/drivers/pci/controller/plda/pcie-starfive.c
> @@ -410,7 +410,11 @@ static int starfive_pcie_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	pm_runtime_enable(&pdev->dev);
> -	pm_runtime_get_sync(&pdev->dev);
> +	ret = pm_runtime_resume_and_get(&pdev->dev);
> +	if (ret < 0) {
> +		pm_runtime_disable(&pdev->dev);
> +		return dev_err_probe(dev, ret, "failed to resume device\n");
> +	}
>  
>  	plda->host_ops = &sf_host_ops;
>  	plda->num_events = PLDA_MAX_EVENT_NUM;

[Severity: High]
This isn't a bug introduced by this patch, but looking at starfive_pcie_remove():

drivers/pci/controller/plda/pcie-starfive.c:starfive_pcie_remove() {
    ...
	pm_runtime_put(&pdev->dev);
	pm_runtime_disable(&pdev->dev);
	plda_pcie_host_deinit(&pcie->plda);
    ...
}

Does this leave the hardware in an active power state upon module unload?

Since pm_runtime_put() is asynchronous, wouldn't calling pm_runtime_disable()
immediately afterward risk canceling the idle/suspend work?

Also, does tearing down the PM reference before stopping the PCIe host
controller via plda_pcie_host_deinit() reverse the proper teardown order?
Could this cause the device to power off while still being actively accessed
by the core?

[Severity: High]
This is a pre-existing issue, but I noticed a potential resource leak in
starfive_pcie_host_init():

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

If starfive_pcie_clk_rst_init() fails, it returns the error code directly.
Does this fail to clean up the successfully initialized PHY?

Since plda_pcie_host_init() detects this initialization failure and returns
early without invoking host_deinit, does this result in permanently leaked
PHY resources and an unbalanced power state?

[Severity: High]
This is a pre-existing issue, but does this code swallow the regulator error?

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

Because there is no return statement after dev_err_probe(), the function
proceeds and eventually returns success at the end. Could this cause the
driver to continue operating with unpowered hardware?

Later on, starfive_pcie_host_deinit() unconditionally calls
regulator_disable(). Since the regulator was not successfully enabled,
would this trigger an unbalanced disable warning in the regulator core and
corrupt regulator use counts?

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

Re: [PATCH] PCI: starfive: Fix unchecked pm_runtime_get_sync() in probe

[Ali Tariq]

Thanks for the review. These are all pre-existing issues separate from
this patch, but worth fixing. I'll send a follow-up series addressing
each of them separately.