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

[Ali Tariq <alitariq45892@gmail.com>]

On 7/12/26 11:04 PM, sashiko-bot@kernel.org wrote:
> 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?

This is fixed in a separate patch [1].

> [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;

I do not think we need to call starfive_pcie_disable_phy() here inside
the error handling. If starfive_pcie_enable_phy() fails, its internal 
error path already cleans up by calling phy_exit() via the 'err_phy_on' 
label. Calling starfive_pcie_disable_phy() here would cause a redundant, 
unbalanced phy_exit() call.

>      ...
> 	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?
> 

This is fixed in a separate patch [2].

> [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?
> 

This is also fixed in a separate patch [2].

[1] 
https://lore.kernel.org/linux-pci/20260713145014.740898-1-alitariq45892@gmail.com/T/#u
[2] 
https://lore.kernel.org/linux-pci/20260714113029.326207-1-alitariq45892@gmail.com/T/#u