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

[sashiko-bot@kernel.org]

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