PCI: starfive: Fix resource leaks on error paths in host_init()

PATCH Superseded Ali Tariq 4 participants 11 messages Jul 14 – Aug 9

Subsystems (4)

Stable branch PCI PCI native host bridge and endpoint drivers PCIe driver for StarFive JH71x0

lore

Share & tools

lore

Ali Tariq

author

2026-07-14 11:32 UTC #0 lore raw

PATCH 1/1 PCI: starfive: Fix resource leaks on error paths in host_init()

starfive_pcie_host_init() acquires the PHY, clocks/resets, and an optional regulator in sequence, but does not correctly unwind these resources when a later step fails.

If starfive_pcie_clk_rst_init() fails after the PHY has already been successfully enabled, the function returns directly without disabling the PHY, leaking it and leaving it powered.

If regulator_enable() fails for the optional vpcie3v3 regulator, the failure is only logged; the function falls through and returns success, leaving the driver believing the regulator is enabled while continuing to configure PCIe hardware that may be unpowered. This also leaves the clocks and PHY enabled with nothing to clean them up.

Disable the PHY on the clk/reset failure path, and disable the clocks/resets and PHY, then return the error, if the regulator fails to enable.

Build-tested and boot-tested on StarFive VisionFive 2 v1.2A

Signed-off-by Ali Tariq <[email protected]>

---
 drivers/pci/controller/plda/pcie-starfive.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/pci/controller/plda/pcie-starfive.c +8 -2

index ba8ef7842e35..156b31eef868 100644
@@ -303,13 +303,19 @@ static int starfive_pcie_host_init(struct plda_pcie_rp *plda)
 			   STG_SYSCON_CLKREQ, STG_SYSCON_CLKREQ);
 
 	ret = starfive_pcie_clk_rst_init(pcie);
-	if (ret)
+	if (ret) {
+		starfive_pcie_disable_phy(pcie);
 		return ret;
+	}
 
 	if (pcie->vpcie3v3) {
 		ret = regulator_enable(pcie->vpcie3v3);
-		if (ret)
+		if (ret) {
 			dev_err_probe(dev, ret, "failed to enable vpcie3v3 regulator\n");
+			starfive_pcie_clk_rst_deinit(pcie);
+			starfive_pcie_disable_phy(pcie);
+			return ret;
+		}
 	}
 
 	if (pcie->reset_gpio)

Signature · 1 line

2.34.1

[email protected]

AI review

2026-07-14 11:53 UTC #1 lore raw

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

Pre-existing issues:
- [High] Use-after-free of IRQ domains during driver unbind or when probe fails after interrupt initialization. --

--- Patch [1]: [PATCH] PCI: starfive: Fix resource leaks on error paths in host_init() --- commit 64603196409722c49e238cd53d9d6a6b7efd0b22
Author: Ali Tariq <[email protected]>

PCI: starfive: Fix resource leaks on error paths in host_init()

This commit unwinds previously acquired PHY and clock/reset resources in starfive_pcie_host_init() when subsequent initialization steps or regulator enablement fails.

Quoted 4 lines

drivers/pci/controller/plda/pcie-starfive.c +0 -0
index 298036c3e7f94..86b248a1ea758 100644

[ ... ]

Quoted 10 lines

drivers/pci/controller/plda/pcie-starfive.c +3 -1

@@ -303,13 +303,19 @@ static int starfive_pcie_host_init(struct plda_pcie_rp *plda)
 			   STG_SYSCON_CLKREQ, STG_SYSCON_CLKREQ);
 
 	ret = starfive_pcie_clk_rst_init(pcie);
-	if (ret)
+	if (ret) {
+		starfive_pcie_disable_phy(pcie);
 		return ret;
+	}

drivers/pci/controller/plda/pcie-starfive.c static int starfive_pcie_host_init(struct plda_pcie_rp *plda) · line 319

Quoted 10 lines

drivers/pci/controller/plda/pcie-starfive.c +5 -1

 	if (pcie->vpcie3v3) {
 		ret = regulator_enable(pcie->vpcie3v3);
-		if (ret)
+		if (ret) {
 			dev_err_probe(dev, ret, "failed to enable vpcie3v3 regulator\n");
+			starfive_pcie_clk_rst_deinit(pcie);
+			starfive_pcie_disable_phy(pcie);
+			return ret;
+		}
 	}

[Severity: High]
This is a pre-existing issue, but since we are fixing error paths in the host initialization, I noticed a potential use-after-free in the related PLDA host initialization code.

In drivers/pci/controller/plda/pcie-plda-host.c, plda_init_interrupts() allocates the interrupt using devm_request_irq():

plda_init_interrupts() {
    ...
    ret = devm_request_irq(dev, event_irq,
                           plda_event_handler,
                           0, NULL, port);
    ...
}

However, on device removal or if probe fails, plda_pcie_host_init() calls plda_pcie_irq_domain_deinit(), which explicitly removes and frees the event domain:

plda_pcie_host_init() {
    ...
err_probe:
    plda_pcie_irq_domain_deinit(port);
    ...
}

plda_pcie_irq_domain_deinit() {
    ...
    irq_domain_remove(pcie->event_domain);
    ...
}

Because the IRQs are devm-managed, their cleanup is deferred until after the driver's probe or remove function returns.

When devres eventually calls free_irq(), will the IRQ core attempt to deactivate the interrupt using the now-freed irq_data->domain pointer?

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Ali Tariq

author

2026-07-14 15:49 UTC #2 lore raw

This is a separate, unrelated issue from the submitted patch. I will work on this and will submit a separate patch.

Quoted 81 lines · On 7/14/26 4:53 PM, [email protected] wrote:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Use-after-free of IRQ domains during driver unbind or when probe fails after interrupt initialization. --
--- Patch [1]: [PATCH] PCI: starfive: Fix resource leaks on error paths in host_init() --- commit 64603196409722c49e238cd53d9d6a6b7efd0b22
Author: Ali Tariq <[email protected]>
PCI: starfive: Fix resource leaks on error paths in host_init()
This commit unwinds previously acquired PHY and clock/reset resources in starfive_pcie_host_init() when subsequent initialization steps or regulator enablement fails.
Quoted 4 lines
drivers/pci/controller/plda/pcie-starfive.c +0 -0
index 298036c3e7f94..86b248a1ea758 100644
[ ... ]
Quoted 10 lines
drivers/pci/controller/plda/pcie-starfive.c +3 -1
@@ -303,13 +303,19 @@ static int starfive_pcie_host_init(struct plda_pcie_rp *plda)
  			   STG_SYSCON_CLKREQ, STG_SYSCON_CLKREQ);
 
  	ret = starfive_pcie_clk_rst_init(pcie);
-	if (ret)
+	if (ret) {
+		starfive_pcie_disable_phy(pcie);
  		return ret;
+	}
 
drivers/pci/controller/plda/pcie-starfive.c static int starfive_pcie_host_init(struct plda_pcie_rp *plda) · line 319
Quoted 10 lines
drivers/pci/controller/plda/pcie-starfive.c +5 -1
  	if (pcie->vpcie3v3) {
  		ret = regulator_enable(pcie->vpcie3v3);
-		if (ret)
+		if (ret) {
  			dev_err_probe(dev, ret, "failed to enable vpcie3v3 regulator\n");
+			starfive_pcie_clk_rst_deinit(pcie);
+			starfive_pcie_disable_phy(pcie);
+			return ret;
+		}
  	}
[Severity: High]
This is a pre-existing issue, but since we are fixing error paths in the host initialization, I noticed a potential use-after-free in the related PLDA host initialization code.
In drivers/pci/controller/plda/pcie-plda-host.c, plda_init_interrupts() allocates the interrupt using devm_request_irq():
plda_init_interrupts() {
     ...
     ret = devm_request_irq(dev, event_irq,
                            plda_event_handler,
                            0, NULL, port);
     ...
}
However, on device removal or if probe fails, plda_pcie_host_init() calls plda_pcie_irq_domain_deinit(), which explicitly removes and frees the event domain:
plda_pcie_host_init() {
     ...
err_probe:
     plda_pcie_irq_domain_deinit(port);
     ...
}
plda_pcie_irq_domain_deinit() {
     ...
     irq_domain_remove(pcie->event_domain);
     ...
}
Because the IRQs are devm-managed, their cleanup is deferred until after the driver's probe or remove function returns.
When devres eventually calls free_irq(), will the IRQ core attempt to deactivate the interrupt using the now-freed irq_data->domain pointer?

Markus Elfring

2026-07-15 10:26 UTC #3 lore raw

In reply to Ali Tariq

…

Quoted 3 lines

Disable the PHY on the clk/reset failure path, and disable the clocks/resets and PHY, then return the error, if the regulator fails to enable.

…

Please avoid a bit of duplicate source code by using an additional label.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v7.2-rc3#n526

Regards,
Markus

Markus Elfring

2026-08-09 10:27 UTC #4 lore raw

RRRRRRR

Am 15.07.26 um 12:26 schrieb Markus Elfring:

Quoted 12 lines

…
Quoted 3 lines
Disable the PHY on the clk/reset failure path, and disable the clocks/resets and PHY, then return the error, if the regulator fails to enable.
…
Please avoid a bit of duplicate source code by using an additional label. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/
linux.git/tree/Documentation/process/coding-style.rst?h=v7.2- rc3#n526
Regards, Markus

Ali Tariq

author

2026-07-15 13:47 UTC #5 lore raw

PATCH v2 1/1 PCI: starfive: Fix resource leaks on error paths in host_init()

In reply to Ali Tariq

starfive_pcie_host_init() acquires the PHY, clocks/resets, and an optional regulator in sequence, but does not correctly unwind these resources when a later step fails.

If starfive_pcie_clk_rst_init() fails after the PHY has already been successfully enabled, the function returns directly without disabling the PHY, leaking it and leaving it powered.

Disable the PHY on the clk/reset failure path, and disable the clocks/resets and PHY, then return the error, if the regulator fails to enable.

Build-tested and boot-tested on StarFive VisionFive 2 v1.2A

Signed-off-by Ali Tariq <[email protected]>

---
Changes in v2:
- Use a shared error-handling label instead of duplicating cleanup code

---
 drivers/pci/controller/plda/pcie-starfive.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

drivers/pci/controller/plda/pcie-starfive.c +11 -2

index ba8ef7842e35..fab44054a5de 100644
@@ -304,12 +304,14 @@ static int starfive_pcie_host_init(struct plda_pcie_rp *plda)
 
 	ret = starfive_pcie_clk_rst_init(pcie);
 	if (ret)
-		return ret;
+		goto err_disable_phy;
 
 	if (pcie->vpcie3v3) {
 		ret = regulator_enable(pcie->vpcie3v3);
-		if (ret)
+		if (ret) {
 			dev_err_probe(dev, ret, "failed to enable vpcie3v3 regulator\n");
+			goto err_clk_rst;
+		}
 	}
 
 	if (pcie->reset_gpio)

@@ -379,6 +381,13 @@ static int starfive_pcie_host_init(struct plda_pcie_rp *plda)
 		dev_info(dev, "port link down\n");
 
 	return 0;
+
+err_clk_rst:
+	starfive_pcie_clk_rst_deinit(pcie);
+err_disable_phy:
+	starfive_pcie_disable_phy(pcie);
+
+	return ret;
 }
 
 static const struct plda_pcie_host_ops sf_host_ops = {

Signature · 1 line

2.34.1

[email protected]

AI review

2026-07-15 13:53 UTC #6 lore raw

Sashiko has reviewed this patch and found no issues. It looks great!

Quoted 14 lines

starfive_pcie_host_init() acquires the PHY, clocks/resets, and an optional regulator in sequence, but does not correctly unwind these resources when a later step fails.
If starfive_pcie_clk_rst_init() fails after the PHY has already been successfully enabled, the function returns directly without disabling the PHY, leaking it and leaving it powered.
If regulator_enable() fails for the optional vpcie3v3 regulator, the failure is only logged; the function falls through and returns success, leaving the driver believing the regulator is enabled while continuing to configure PCIe hardware that may be unpowered. This also leaves the clocks and PHY enabled with nothing to clean them up.

Quoted 7 lines

Disable the PHY on the clk/reset failure path, and disable the clocks/resets and PHY, then return the error, if the regulator fails to enable.
Build-tested and boot-tested on StarFive VisionFive 2 v1.2A
Signed-off-by Ali Tariq <[email protected]>

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Markus Elfring

2026-07-15 18:22 UTC #7 lore raw

In reply to Ali Tariq

…

Quoted 3 lines

Disable the PHY on the clk/reset failure path, and disable the clocks/resets and PHY, then return the error, if the regulator fails to enable.…

How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?

Regards,
Markus

Ali Tariq

author

2026-07-16 10:21 UTC #8 lore raw

PATCH v3 1/1 PCI: starfive: Fix resource leaks on error paths in host_init()

In reply to Ali Tariq

starfive_pcie_host_init() acquires the PHY, clocks/resets, and an optional regulator in sequence, but does not correctly unwind these resources when a later step fails.

If starfive_pcie_clk_rst_init() fails after the PHY has already been successfully enabled, the function returns directly without disabling the PHY, leaking it and leaving it powered.

Disable the PHY on the clk/reset failure path, and disable the clocks/resets and PHY, then return the error, if the regulator fails to enable.

Build-tested and boot-tested on StarFive VisionFive 2 v1.2A

Fixes 05a75df4182e ("PCI: starfive: Use regulator APIs to control the 3v3 power supply of PCIe slots")

Fixes 39b91eb40c6a ("PCI: starfive: Add JH7110 PCIe controller")

Signed-off-by Ali Tariq <[email protected]>

Cc [email protected]

---
Changes in v3:
- Add Fixes: tags for both introducing commits
- Add Cc: [email protected]

Changes in v2:
- Use a shared error-handling label instead of duplicating cleanup code

---
 drivers/pci/controller/plda/pcie-starfive.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

drivers/pci/controller/plda/pcie-starfive.c +11 -2

index ba8ef7842e35..fab44054a5de 100644
@@ -304,12 +304,14 @@ static int starfive_pcie_host_init(struct plda_pcie_rp *plda)
 
 	ret = starfive_pcie_clk_rst_init(pcie);
 	if (ret)
-		return ret;
+		goto err_disable_phy;
 
 	if (pcie->vpcie3v3) {
 		ret = regulator_enable(pcie->vpcie3v3);
-		if (ret)
+		if (ret) {
 			dev_err_probe(dev, ret, "failed to enable vpcie3v3 regulator\n");
+			goto err_clk_rst;
+		}
 	}
 
 	if (pcie->reset_gpio)

@@ -379,6 +381,13 @@ static int starfive_pcie_host_init(struct plda_pcie_rp *plda)
 		dev_info(dev, "port link down\n");
 
 	return 0;
+
+err_clk_rst:
+	starfive_pcie_clk_rst_deinit(pcie);
+err_disable_phy:
+	starfive_pcie_disable_phy(pcie);
+
+	return ret;
 }
 
 static const struct plda_pcie_host_ops sf_host_ops = {

Signature · 1 line

2.34.1

[email protected]

AI review

2026-07-16 10:36 UTC #9 lore raw

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

Pre-existing issues:
- [High] pm_runtime_get_sync() return value is not checked in probe, which can lead to a system crash due to hardware access failures. --

--- Patch [1]: [PATCH v3] PCI: starfive: Fix resource leaks on error paths in host_init() --- commit 312c978532200464dd10047fcee7a0fbb43cf2dc
Author: Ali Tariq <[email protected]>

PCI: starfive: Fix resource leaks on error paths in host_init()

This commit addresses resource unwind paths in starfive_pcie_host_init(). It ensures the PHY is properly disabled if clock/reset initialization fails, and that clocks, resets, and the PHY are correctly deactivated if the regulator fails to enable.

drivers/pci/controller/plda/pcie-starfive.c

Quoted 4 lines

drivers/pci/controller/plda/pcie-starfive.c +0 -0
index 298036c3e7f94..628f8c8d67471 100644

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential issue nearby in starfive_pcie_probe() regarding pm_runtime_get_sync():

drivers/pci/controller/plda/pcie-starfive.c:starfive_pcie_probe() {
	...
	pm_runtime_enable(&pdev->dev);
	pm_runtime_get_sync(&pdev->dev);

	plda->host_ops = &sf_host_ops;
	...
	ret = plda_pcie_host_init(&pcie->plda, &starfive_pcie_ops,
				  &stf_pcie_event);
	...
}

Is it safe to ignore the return value of pm_runtime_get_sync() here?

If it fails to enable the device power domain or clock, it will return an error, but the function proceeds to call plda_pcie_host_init() which will access memory-mapped hardware registers.

Accessing unpowered hardware registers can trigger an asynchronous external abort or synchronous bus fault on RISC-V systems, causing a kernel crash.

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Manivannan Sadhasivam

maintainer Applied

2026-07-16 16:53 UTC #10 lore raw

In reply to Ali Tariq

Quoted 9 lines · Ali Tariq

starfive_pcie_host_init() acquires the PHY, clocks/resets, and an optional regulator in sequence, but does not correctly unwind these resources when a later step fails.
If starfive_pcie_clk_rst_init() fails after the PHY has already been successfully enabled, the function returns directly without disabling the PHY, leaking it and leaving it powered.
[...]

Applied, thanks!

[1/1] PCI: starfive: Fix resource leaks on error paths in host_init()

      commit: 22877a061f81c5d58041e384b3131684bec636b9

Best regards,

Signature · 1 line

மணிவண்ணன் சதாசிவம்