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

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

[Ali Tariq]

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

[PATCH v2] 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.

Fixes: 6168efbebace ("PCI: starfive: Enable controller runtime PM before probing host bridge")
Signed-off-by: Ali Tariq <alitariq45892@gmail.com>
---
Changes in v2:
- Added Fixes tag to commit message
---
 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 v2] PCI: starfive: Fix unchecked pm_runtime_get_sync() in probe

[sashiko-bot]

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

Pre-existing issues:
- [High] Runtime PM is disabled asynchronously and prematurely during driver removal, leading to an incorrect power state and violating PM guidelines.
- [High] IRQ domains are destroyed during host deinit while their associated interrupts are still mapped and active.
--

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

    PCI: starfive: Fix unchecked pm_runtime_get_sync() in probe

    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.

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

While reviewing this patch, I noticed a couple of pre-existing issues in
related code:

> 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);
> 	platform_set_drvdata(pdev, NULL);
> }

[Severity: High]
This isn't a bug introduced by this patch, but should this use 
pm_runtime_put_sync() instead?

In starfive_pcie_remove(), pm_runtime_put() queues an asynchronous idle
request, but pm_runtime_disable() is called immediately after. This cancels
the pending request, leaving the device in an active power state indefinitely.

Additionally, this teardown happens before plda_pcie_host_deinit(). If
pm_runtime_put_sync() were used, the hardware would drop power immediately,
causing plda_pcie_host_deinit() to access unpowered registers and potentially
crash the system.

Should the host bridge be torn down first, and only then synchronously power
off the device?


Also in drivers/pci/controller/plda/pcie-plda-host.c:

> static void plda_pcie_irq_domain_deinit(struct plda_pcie_rp *pcie)
> {
> 	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);
> 
> 	irq_domain_remove(pcie->intx_domain);
> 	irq_domain_remove(pcie->event_domain);
> }

[Severity: High]
This is a pre-existing issue, but are we destroying the IRQ domains while
their associated interrupts are still mapped and active?

During driver removal, starfive_pcie_remove() calls plda_pcie_host_deinit(),
which synchronously removes the event and INTx IRQ domains via
plda_pcie_irq_domain_deinit().

However, the interrupts mapped from these domains are never unmapped via
irq_dispose_mapping(). The actual interrupt handlers requested via
devm_request_irq() in plda_init_interrupts() haven't been freed yet, because
devm teardown occurs strictly after the driver remove() callback returns.

Is it safe to destroy these domains with active mappings and bound interrupts,
or could this lead to issues during subsequent devm teardown?

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

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

[Ali Tariq]

Gentle ping.

Did anyone get a chance to review this patch?

Happy to send v3 with new changes if required.

Regards,
Ali

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

[Manivannan Sadhasivam]

On Sat, 18 Jul 2026 20:33:51 +0500, Ali Tariq wrote:
> 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.
> 
> [...]

Applied, thanks!

[1/1] PCI: starfive: Fix unchecked pm_runtime_get_sync() in probe
      commit: aaae917990623a6ca6b638557056606a1ae4a8d6

Best regards,
-- 
மணிவண்ணன் சதாசிவம்