[PATCH] PCI: exynos: Handle PHY initialization errors

[PATCH] PCI: exynos: Handle PHY initialization errors

[Sreeraj S Kurup]

Check the return values of phy_init() and phy_power_on() to
avoid continuing PCIe host initialization when PHY setup fails.

If phy_power_on() fails, call phy_exit() to release the PHY
resources initialized by phy_init() before returning the error.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
 drivers/pci/controller/dwc/pci-exynos.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
index e3c05ade381b..5ad07c0be8e5 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -225,13 +225,21 @@ static int exynos_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct exynos_pcie *ep = to_exynos_pcie(pci);
+	int ret;
 
 	pp->bridge->ops = &exynos_pci_ops;
 
 	exynos_pcie_assert_core_reset(ep);
 
-	phy_init(ep->phy);
-	phy_power_on(ep->phy);
+	ret = phy_init(ep->phy);
+	if (ret)
+		return ret;
+
+	ret = phy_power_on(ep->phy);
+	if (ret) {
+		phy_exit(ep->phy);
+		return ret;
+	}
 
 	exynos_pcie_deassert_core_reset(ep);
 	exynos_pcie_enable_irq_pulse(ep);
-- 
2.55.0

Re: [PATCH] PCI: exynos: Handle PHY initialization errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PCI: exynos: double phy_exit() on probe error after exynos_pcie_host_init()
- [High] PCI: exynos: unchecked exynos_pcie_host_init() in exynos_pcie_resume_noirq()
--

commit e723b41ff1e9114786c14ad5226542ae1f9e3a1b
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

PCI: exynos: Handle PHY initialization errors

This commit checks the return values of phy_init() and phy_power_on() in
exynos_pcie_host_init() and calls phy_exit() if phy_power_on() fails.

> diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
> index e3c05ade381bb..5ad07c0be8e51 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c
> @@ -225,13 +225,21 @@ static int exynos_pcie_host_init(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct exynos_pcie *ep = to_exynos_pcie(pci);
> +	int ret;
>  
>  	pp->bridge->ops = &exynos_pci_ops;
>  
>  	exynos_pcie_assert_core_reset(ep);
>  
> -	phy_init(ep->phy);
> -	phy_power_on(ep->phy);
> +	ret = phy_init(ep->phy);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_power_on(ep->phy);
> +	if (ret) {
> +		phy_exit(ep->phy);
> +		return ret;
> +	}

[Severity: High]
Does this cause an unbalanced phy_exit(ep->phy) call when probe fails in
exynos_pcie_probe() -> exynos_add_pcie_port() -> dw_pcie_host_init() ->
exynos_pcie_host_init()?

When phy_init(ep->phy) fails, ep->phy->init_count remains 0, and when
phy_power_on(ep->phy) fails, phy_exit(ep->phy) in exynos_pcie_host_init()
decrements ep->phy->init_count back to 0. Both dw_pcie_host_init() and
exynos_add_pcie_port() return that negative error code to
exynos_pcie_probe():

drivers/pci/controller/dwc/pci-exynos.c:exynos_pcie_probe() {
	...
	ret = exynos_add_pcie_port(ep, pdev);
	if (ret < 0)
		goto fail_probe;

	return 0;

fail_probe:
	phy_exit(ep->phy);
	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);

	return ret;
}

Because fail_probe unconditionally calls phy_exit(ep->phy), phy_exit() in
drivers/phy/phy-core.c decrements ep->phy->init_count from 0 to -1, which
prevents phy_init() from invoking phy->ops->init() on subsequent probe
attempts.

[Severity: High]
What happens during system resume in exynos_pcie_resume_noirq() if
exynos_pcie_host_init() returns early with an error here?

In exynos_pcie_host_init(), exynos_pcie_assert_core_reset(ep) is called
before phy_init(ep->phy) and phy_power_on(ep->phy). Returning early skips
exynos_pcie_deassert_core_reset(ep) and exynos_pcie_enable_irq_pulse(ep),
leaving the PCIe core held in reset and the PHY off, while
exynos_pcie_resume_noirq() does not check the return value of
exynos_pcie_host_init(pp):

drivers/pci/controller/dwc/pci-exynos.c:exynos_pcie_resume_noirq() {
	...
	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
	if (ret)
		return ret;

	/* exynos_pcie_host_init controls ep->phy */
	exynos_pcie_host_init(pp);
	dw_pcie_setup_rc(pp);
	exynos_pcie_start_link(pci);
	return dw_pcie_wait_for_link(pci);
}

Should exynos_pcie_resume_noirq() check the return value of
exynos_pcie_host_init(pp) and disable ep->supplies before returning the
error instead of calling dw_pcie_setup_rc(pp) and exynos_pcie_start_link(pci)
while the controller core is in reset?

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

[PATCH v2] PCI: exynos: Handle PCIe PHY initialization errors

[Sreeraj S Kurup]

Check the return values of "phy_init()" and "phy_power_on()" to
avoid continuing PCIe host initialization when PHY setup fails.

Track the PHY initialization and power state to ensure cleanup is
performed only when appropriate, avoiding double calls to
"phy_exit()" on probe failure.

Also check the return value of "exynos_pcie_host_init()" during
resume and abort the resume sequence if initialization fails,
disabling the regulators before returning the error.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
v2:
- Track PHY initialization and power state to prevent duplicate cleanup.
- Handle host initialization failures during resume.

 drivers/pci/controller/dwc/pci-exynos.c | 77 ++++++++++++++++++++++---
 1 file changed, 68 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
index e3c05ade381b..f6b87c26ab36 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -55,6 +55,8 @@ struct exynos_pcie {
 	struct clk_bulk_data		*clks;
 	struct phy			*phy;
 	struct regulator_bulk_data	supplies[2];
+	bool phy_initialized;
+	bool phy_powered;
 };
 
 static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)
@@ -225,13 +227,23 @@ static int exynos_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct exynos_pcie *ep = to_exynos_pcie(pci);
+	int ret;
 
 	pp->bridge->ops = &exynos_pci_ops;
 
 	exynos_pcie_assert_core_reset(ep);
 
-	phy_init(ep->phy);
-	phy_power_on(ep->phy);
+	ret = phy_init(ep->phy);
+	if (ret)
+		return ret;
+
+	ep->phy_initialized = true;
+
+	ret = phy_power_on(ep->phy);
+	if (ret)
+		return ret;
+
+	ep->phy_powered = true;
 
 	exynos_pcie_deassert_core_reset(ep);
 	exynos_pcie_enable_irq_pulse(ep);
@@ -323,7 +335,16 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	return 0;
 
 fail_probe:
-	phy_exit(ep->phy);
+	if (ep->phy_powered) {
+		phy_power_off(ep->phy);
+		ep->phy_powered = false;
+	}
+
+	if (ep->phy_initialized) {
+		phy_exit(ep->phy);
+		ep->phy_initialized = false;
+	}
+
 	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
 
 	return ret;
@@ -335,8 +356,17 @@ static void exynos_pcie_remove(struct platform_device *pdev)
 
 	dw_pcie_host_deinit(&ep->pci.pp);
 	exynos_pcie_assert_core_reset(ep);
-	phy_power_off(ep->phy);
-	phy_exit(ep->phy);
+
+	if (ep->phy_powered) {
+		phy_power_off(ep->phy);
+		ep->phy_powered = false;
+	}
+
+	if (ep->phy_initialized) {
+		phy_exit(ep->phy);
+		ep->phy_initialized = false;
+	}
+
 	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
 }
 
@@ -345,8 +375,17 @@ static int exynos_pcie_suspend_noirq(struct device *dev)
 	struct exynos_pcie *ep = dev_get_drvdata(dev);
 
 	exynos_pcie_assert_core_reset(ep);
-	phy_power_off(ep->phy);
-	phy_exit(ep->phy);
+
+	if (ep->phy_powered) {
+		phy_power_off(ep->phy);
+		ep->phy_powered = false;
+	}
+
+	if (ep->phy_initialized) {
+		phy_exit(ep->phy);
+		ep->phy_initialized = false;
+	}
+
 	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
 
 	return 0;
@@ -364,10 +403,30 @@ static int exynos_pcie_resume_noirq(struct device *dev)
 		return ret;
 
 	/* exynos_pcie_host_init controls ep->phy */
-	exynos_pcie_host_init(pp);
+	ret = exynos_pcie_host_init(pp);
+	if (ret)
+		goto err_cleanup;
+
 	dw_pcie_setup_rc(pp);
 	exynos_pcie_start_link(pci);
-	return dw_pcie_wait_for_link(pci);
+	ret = dw_pcie_wait_for_link(pci);
+	if (ret)
+		goto err_cleanup;
+
+	return 0;
+
+err_cleanup:
+	if (ep->phy_powered) {
+		phy_power_off(ep->phy);
+		ep->phy_powered = false;
+	}
+
+	if (ep->phy_initialized) {
+		phy_exit(ep->phy);
+		ep->phy_initialized = false;
+	}
+	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+	return ret;
 }
 
 static const struct dev_pm_ops exynos_pcie_pm_ops = {
-- 
2.55.0

Re: [PATCH v2] PCI: exynos: Handle PCIe PHY initialization errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PCI: exynos: unbalanced regulator disable and unpowered access on resume error
- [High] PCI: exynos: exynos_pcie_resume_noirq() powers down controller on -ENODEV
--

commit 7a2bc4e63574e4dca88761cb98d074446defd067
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

PCI: exynos: Handle PCIe PHY initialization errors

This commit checks return values from phy_init() and phy_power_on() in
exynos_pcie_host_init(), tracks PHY initialization and power state in struct
exynos_pcie, and adds error handling in exynos_pcie_resume_noirq() to clean up
the PHY and regulators on failure.

> diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
> index e3c05ade381bb..f6b87c26ab36c 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c

[ ... ]

> @@ -364,10 +403,30 @@ static int exynos_pcie_resume_noirq(struct device *dev)
>  		return ret;
>  
>  	/* exynos_pcie_host_init controls ep->phy */
> -	exynos_pcie_host_init(pp);
> +	ret = exynos_pcie_host_init(pp);
> +	if (ret)
> +		goto err_cleanup;
> +
>  	dw_pcie_setup_rc(pp);
>  	exynos_pcie_start_link(pci);
> -	return dw_pcie_wait_for_link(pci);
> +	ret = dw_pcie_wait_for_link(pci);
> +	if (ret)
> +		goto err_cleanup;

[Severity: High]
Should exynos_pcie_resume_noirq() power down the PHY and disable regulators
when dw_pcie_wait_for_link() returns a non-zero value such as -ENODEV?

In drivers/pci/controller/dwc/pcie-designware.c, dw_pcie_wait_for_link()
returns -ENODEV when no endpoint device is detected (LTSSM in
DW_PCIE_LTSSM_DETECT_QUIET or DW_PCIE_LTSSM_DETECT_ACT) and -EIO when a device
is in Poll.Active or Poll.Compliance. During initial probe,
dw_pcie_host_init() only aborts on -ETIMEDOUT, so probe succeeds and registers
the host bridge with the PHY and regulators active even when the slot is
empty.

With this change, resuming a system with no connected or active PCIe endpoint
causes dw_pcie_wait_for_link() to return -ENODEV or -EIO and jump to
err_cleanup, shutting down ep->phy and ep->supplies while the PCI root bus
remains registered (and without asserting core reset after
exynos_pcie_deassert_core_reset() and exynos_pcie_start_link()).

> +
> +	return 0;
> +
> +err_cleanup:
> +	if (ep->phy_powered) {
> +		phy_power_off(ep->phy);
> +		ep->phy_powered = false;
> +	}
> +
> +	if (ep->phy_initialized) {
> +		phy_exit(ep->phy);
> +		ep->phy_initialized = false;
> +	}
> +	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +	return ret;

[Severity: High]
What happens on subsequent PCI bus operations, the next system suspend, or
driver unbind if exynos_pcie_resume_noirq() fails and disables the PHY and
ep->supplies here?

During exynos_pcie_suspend_noirq(), the vdd18 and vdd10 supplies in
ep->supplies are disabled and the PHY is powered off and exited. If
exynos_pcie_resume_noirq() fails in regulator_bulk_enable(), or if
exynos_pcie_host_init() or dw_pcie_wait_for_link() fails and jumps to
err_cleanup, the PHY is powered off/exited and regulator_bulk_disable() brings
the regulator enable count back to zero.

However, returning an error from a resume_noirq callback in
device_resume_noirq() does not unbind the driver or unregister the PCI host
bridge, and the PM core continues resuming subordinate PCI devices on the root
bus.

Because the controller and PHY reference clock are now powered down while the
host bridge remains active, won't this lead to two issues:

1. Subsequent config space accesses via exynos_pcie_rd_own_conf() and
exynos_pcie_wr_own_conf(), shared IRQ handling, dw_pcie_host_deinit() in
exynos_pcie_remove(), and exynos_pcie_assert_core_reset() in
exynos_pcie_suspend_noirq() and exynos_pcie_remove() will perform MMIO
accesses to pci->dbi_base and pci->elbi_base while unpowered and unclocked.

2. Unlike ep->phy_powered and ep->phy_initialized, the regulator state of
ep->supplies is not tracked. On the next exynos_pcie_suspend_noirq() or
exynos_pcie_remove(), regulator_bulk_disable(ARRAY_SIZE(ep->supplies),
ep->supplies) will be called unconditionally on already-disabled regulators,
triggering an unbalanced regulator disable warning in _regulator_disable().

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

[PATCH v3] PCI: exynos: Handle PCIe PHY initialization errors

[Sreeraj S Kurup]

Check the return values of "phy_init()" and "phy_power_on()" to
avoid continuing PCIe host initialization when PHY setup fails.

Track the PHY initialization and power state to ensure cleanup is
performed only when appropriate, avoiding double calls to
"phy_exit()" on probe failure.

Also check the return value of "exynos_pcie_host_init()" during
resume and abort the resume sequence if PHY initialization fails.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
v3:
 - Address review feedback on PHY and regulator state tracking
 - Avoid powering down the controller when link detection fails

v2:
 - Track PHY initialization and power state to prevent duplicate cleanup
 - Handle host initialization failures during resume

 drivers/pci/controller/dwc/pci-exynos.c | 111 +++++++++++++++++++++---
 1 file changed, 100 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
index e3c05ade381b..d7504b0e5d96 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -11,6 +11,7 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/device.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
@@ -55,8 +56,34 @@ struct exynos_pcie {
 	struct clk_bulk_data		*clks;
 	struct phy			*phy;
 	struct regulator_bulk_data	supplies[2];
+	bool phy_initialized;
+	bool phy_powered;
+	bool supplies_enabled;
 };
 
+static int exynos_pcie_phy_exit(struct exynos_pcie *ep)
+{
+	int ret;
+
+	if (ep->phy_powered) {
+		ret = phy_power_off(ep->phy);
+		if (ret)
+			return ret;
+
+		ep->phy_powered = false;
+	}
+
+	if (ep->phy_initialized) {
+		ret = phy_exit(ep->phy);
+		if (ret)
+			return ret;
+
+		ep->phy_initialized = false;
+	}
+
+	return 0;
+}
+
 static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)
 {
 	writel(val, base + reg);
@@ -225,13 +252,23 @@ static int exynos_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct exynos_pcie *ep = to_exynos_pcie(pci);
+	int ret;
 
 	pp->bridge->ops = &exynos_pci_ops;
 
 	exynos_pcie_assert_core_reset(ep);
 
-	phy_init(ep->phy);
-	phy_power_on(ep->phy);
+	ret = phy_init(ep->phy);
+	if (ret)
+		return ret;
+
+	ep->phy_initialized = true;
+
+	ret = phy_power_on(ep->phy);
+	if (ret)
+		return ret;
+
+	ep->phy_powered = true;
 
 	exynos_pcie_deassert_core_reset(ep);
 	exynos_pcie_enable_irq_pulse(ep);
@@ -314,6 +351,7 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ep->supplies_enabled = true;
 	platform_set_drvdata(pdev, ep);
 
 	ret = exynos_add_pcie_port(ep, pdev);
@@ -323,8 +361,19 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	return 0;
 
 fail_probe:
-	phy_exit(ep->phy);
-	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+	if (exynos_pcie_phy_exit(ep))
+		dev_err(&pdev->dev, "failed to exit PCIe PHY\n");
+
+	if (ep->supplies_enabled) {
+		int err;
+
+		err = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+		if (err)
+			dev_err(&pdev->dev, "failed to disable regulators: %d\n", err);
+		else
+			ep->supplies_enabled = false;
+
+	}
 
 	return ret;
 }
@@ -332,22 +381,42 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 static void exynos_pcie_remove(struct platform_device *pdev)
 {
 	struct exynos_pcie *ep = platform_get_drvdata(pdev);
+	int ret;
 
 	dw_pcie_host_deinit(&ep->pci.pp);
 	exynos_pcie_assert_core_reset(ep);
-	phy_power_off(ep->phy);
-	phy_exit(ep->phy);
-	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+
+	ret = exynos_pcie_phy_exit(ep);
+	if (ret)
+		return ret;
+
+	if (ep->supplies_enabled) {
+		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+		if (ret)
+			dev_err(&pdev->dev, "failed to disable regulators: %d\n", ret);
+		else
+			ep->supplies_enabled = false;
+	}
 }
 
 static int exynos_pcie_suspend_noirq(struct device *dev)
 {
 	struct exynos_pcie *ep = dev_get_drvdata(dev);
+	int ret;
 
 	exynos_pcie_assert_core_reset(ep);
-	phy_power_off(ep->phy);
-	phy_exit(ep->phy);
-	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+
+	ret = exynos_pcie_phy_exit(ep);
+	if (ret)
+		return ret;
+
+	if (ep->supplies_enabled) {
+		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+		if (ret)
+			return ret;
+
+		ep->supplies_enabled = false;
+	}
 
 	return 0;
 }
@@ -358,13 +427,33 @@ static int exynos_pcie_resume_noirq(struct device *dev)
 	struct dw_pcie *pci = &ep->pci;
 	struct dw_pcie_rp *pp = &pci->pp;
 	int ret;
+	int err;
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
 	if (ret)
 		return ret;
 
+	ep->supplies_enabled = true;
+
 	/* exynos_pcie_host_init controls ep->phy */
-	exynos_pcie_host_init(pp);
+	ret = exynos_pcie_host_init(pp);
+	if (ret) {
+		err = exynos_pcie_phy_exit(ep);
+		if (err)
+			return err;
+
+		if (ep->supplies_enabled) {
+			err = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+			if (err)
+				return err;
+
+			ep->supplies_enabled = false;
+		}
+
+		return ret;
+
+	}
+
 	dw_pcie_setup_rc(pp);
 	exynos_pcie_start_link(pci);
 	return dw_pcie_wait_for_link(pci);
-- 
2.55.0

Re: [PATCH v3] PCI: exynos: Handle PCIe PHY initialization errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] PCI: exynos: return of int in void exynos_pcie_remove() and regulator leak
- [Medium] PCI: exynos: regulator leak in exynos_pcie_resume_noirq() on PHY exit error
- [Medium] PCI: exynos: unrecovered hardware state on exynos_pcie_suspend_noirq() error
--

commit 0c8cd2248f61d9e01add8f3a5e1b399c6f9c0fd4
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

PCI: exynos: Handle PCIe PHY initialization errors

This commit checks the return values of phy_init() and phy_power_on() in
exynos_pcie_host_init() and tracks PHY and regulator state across probe,
remove, suspend, and resume paths.

> diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
> index e3c05ade381bb..d7504b0e5d96c 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c

[ ... ]

> @@ -332,22 +381,42 @@ static int exynos_pcie_probe(struct platform_device *pdev)
>  static void exynos_pcie_remove(struct platform_device *pdev)
>  {
>  	struct exynos_pcie *ep = platform_get_drvdata(pdev);
> +	int ret;
>  
>  	dw_pcie_host_deinit(&ep->pci.pp);
>  	exynos_pcie_assert_core_reset(ep);
> -	phy_power_off(ep->phy);
> -	phy_exit(ep->phy);
> -	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +
> +	ret = exynos_pcie_phy_exit(ep);
> +	if (ret)
> +		return ret;
> +
> +	if (ep->supplies_enabled) {
> +		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +		if (ret)
> +			dev_err(&pdev->dev, "failed to disable regulators: %d\n", ret);
> +		else
> +			ep->supplies_enabled = false;
> +	}
>  }

[Severity: High]
In drivers/pci/controller/dwc/pci-exynos.c, exynos_pcie_remove() has a void
return type, so returning ret when exynos_pcie_phy_exit(ep) fails causes a
build error.

Also, if exynos_pcie_phy_exit(ep) returns a non-zero error during driver
removal, doesn't returning early here skip regulator_bulk_disable() and leave
the vdd18 and vdd10 supplies in ep->supplies enabled? Should this log an
error and continue to disable ep->supplies, similar to the fail_probe path in
exynos_pcie_probe()?

>  
>  static int exynos_pcie_suspend_noirq(struct device *dev)
>  {
>  	struct exynos_pcie *ep = dev_get_drvdata(dev);
> +	int ret;
>  
>  	exynos_pcie_assert_core_reset(ep);
> -	phy_power_off(ep->phy);
> -	phy_exit(ep->phy);
> -	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +
> +	ret = exynos_pcie_phy_exit(ep);
> +	if (ret)
> +		return ret;
> +
> +	if (ep->supplies_enabled) {
> +		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +		if (ret)
> +			return ret;
> +
> +		ep->supplies_enabled = false;
> +	}
>  
>  	return 0;
>  }

[Severity: Medium]
In exynos_pcie_suspend_noirq(), exynos_pcie_assert_core_reset(ep) is called
before exynos_pcie_phy_exit(ep) and regulator_bulk_disable().

If exynos_pcie_phy_exit(ep) or regulator_bulk_disable() returns an error,
device_suspend_noirq() in the PM core aborts before setting
dev->power.is_noirq_suspended, which means device_resume_noirq() will not
call exynos_pcie_resume_noirq() during suspend-abort recovery.

Would that leave the Exynos PCIe controller held in core reset (and with its
PHY powered off or regulators still enabled) when the system aborts suspend?

> @@ -358,13 +427,33 @@ static int exynos_pcie_resume_noirq(struct device *dev)
>  	struct dw_pcie *pci = &ep->pci;
>  	struct dw_pcie_rp *pp = &pci->pp;
>  	int ret;
> +	int err;
>  
>  	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
>  	if (ret)
>  		return ret;
>  
> +	ep->supplies_enabled = true;
> +
>  	/* exynos_pcie_host_init controls ep->phy */
> -	exynos_pcie_host_init(pp);
> +	ret = exynos_pcie_host_init(pp);
> +	if (ret) {
> +		err = exynos_pcie_phy_exit(ep);
> +		if (err)
> +			return err;
> +
> +		if (ep->supplies_enabled) {
> +			err = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +			if (err)
> +				return err;
> +
> +			ep->supplies_enabled = false;
> +		}
> +
> +		return ret;
> +
> +	}

[Severity: Medium]
In exynos_pcie_resume_noirq(), regulator_bulk_enable() enables ep->supplies
before calling exynos_pcie_host_init(pp).

If exynos_pcie_host_init(pp) fails (for instance, when phy_init() succeeds
and phy_power_on() fails) and the subsequent cleanup call to
exynos_pcie_phy_exit(ep) also returns an error, won't returning err early
bypass regulator_bulk_disable() and leak the regulator enable reference on
ep->supplies?

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

[PATCH v4] PCI: exynos: Handle PCIe PHY initialization errors

[Sreeraj S Kurup]

Check the return values of "phy_init()" and "phy_power_on()" to
avoid continuing PCIe host initialization when PHY setup fails.

Track the PHY initialization and power state to ensure cleanup is
performed only when appropriate, avoiding double calls to
"phy_exit()" on probe failure.

Also check the return value of "exynos_pcie_host_init()" during
resume and abort the resume sequence if PHY initialization fails.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
v4:
 - Fix invalid return in void exynos_pcie_remove() and ensure regulator cleanup
 - Add core reset deassertion on error in exynos_pcie_suspend_noirq()
 - Prevent regulator leaks in exynos_pcie_resume_noirq() error recovery path
v3:
 - Address review feedback on PHY and regulator state tracking
 - Avoid powering down the controller when link detection fails
v2:
 - Track PHY initialization and power state to prevent duplicate cleanup
 - Handle host initialization failures during resume

 drivers/pci/controller/dwc/pci-exynos.c | 115 +++++++++++++++++++++---
 1 file changed, 104 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
index e3c05ade381b..5b8d8f1ce8dc 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -11,6 +11,7 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/device.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
@@ -55,8 +56,34 @@ struct exynos_pcie {
 	struct clk_bulk_data		*clks;
 	struct phy			*phy;
 	struct regulator_bulk_data	supplies[2];
+	bool phy_initialized;
+	bool phy_powered;
+	bool supplies_enabled;
 };
 
+static int exynos_pcie_phy_exit(struct exynos_pcie *ep)
+{
+	int ret;
+
+	if (ep->phy_powered) {
+		ret = phy_power_off(ep->phy);
+		if (ret)
+			return ret;
+
+		ep->phy_powered = false;
+	}
+
+	if (ep->phy_initialized) {
+		ret = phy_exit(ep->phy);
+		if (ret)
+			return ret;
+
+		ep->phy_initialized = false;
+	}
+
+	return 0;
+}
+
 static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)
 {
 	writel(val, base + reg);
@@ -225,13 +252,23 @@ static int exynos_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct exynos_pcie *ep = to_exynos_pcie(pci);
+	int ret;
 
 	pp->bridge->ops = &exynos_pci_ops;
 
 	exynos_pcie_assert_core_reset(ep);
 
-	phy_init(ep->phy);
-	phy_power_on(ep->phy);
+	ret = phy_init(ep->phy);
+	if (ret)
+		return ret;
+
+	ep->phy_initialized = true;
+
+	ret = phy_power_on(ep->phy);
+	if (ret)
+		return ret;
+
+	ep->phy_powered = true;
 
 	exynos_pcie_deassert_core_reset(ep);
 	exynos_pcie_enable_irq_pulse(ep);
@@ -314,6 +351,7 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ep->supplies_enabled = true;
 	platform_set_drvdata(pdev, ep);
 
 	ret = exynos_add_pcie_port(ep, pdev);
@@ -323,8 +361,19 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	return 0;
 
 fail_probe:
-	phy_exit(ep->phy);
-	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+	if (exynos_pcie_phy_exit(ep))
+		dev_err(&pdev->dev, "failed to exit PCIe PHY\n");
+
+	if (ep->supplies_enabled) {
+		int err;
+
+		err = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+		if (err)
+			dev_err(&pdev->dev, "failed to disable regulators: %d\n", err);
+		else
+			ep->supplies_enabled = false;
+
+	}
 
 	return ret;
 }
@@ -332,24 +381,48 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 static void exynos_pcie_remove(struct platform_device *pdev)
 {
 	struct exynos_pcie *ep = platform_get_drvdata(pdev);
+	int ret;
 
 	dw_pcie_host_deinit(&ep->pci.pp);
 	exynos_pcie_assert_core_reset(ep);
-	phy_power_off(ep->phy);
-	phy_exit(ep->phy);
-	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+
+	ret = exynos_pcie_phy_exit(ep);
+	if (ret)
+		dev_err(&pdev->dev, "failed to exit PCIe PHY: %d\n", ret);
+
+	if (ep->supplies_enabled) {
+		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+		if (ret)
+			dev_err(&pdev->dev, "failed to disable regulators: %d\n", ret);
+		else
+			ep->supplies_enabled = false;
+	}
 }
 
 static int exynos_pcie_suspend_noirq(struct device *dev)
 {
 	struct exynos_pcie *ep = dev_get_drvdata(dev);
+	int ret;
 
 	exynos_pcie_assert_core_reset(ep);
-	phy_power_off(ep->phy);
-	phy_exit(ep->phy);
-	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+
+	ret = exynos_pcie_phy_exit(ep);
+	if (ret)
+		goto err_deassert_reset;
+
+	if (ep->supplies_enabled) {
+		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+		if (ret)
+			goto err_deassert_reset;
+
+		ep->supplies_enabled = false;
+	}
 
 	return 0;
+
+err_deassert_reset:
+	exynos_pcie_deassert_core_reset(ep);
+	return ret;
 }
 
 static int exynos_pcie_resume_noirq(struct device *dev)
@@ -358,13 +431,33 @@ static int exynos_pcie_resume_noirq(struct device *dev)
 	struct dw_pcie *pci = &ep->pci;
 	struct dw_pcie_rp *pp = &pci->pp;
 	int ret;
+	int err;
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
 	if (ret)
 		return ret;
 
+	ep->supplies_enabled = true;
+
 	/* exynos_pcie_host_init controls ep->phy */
-	exynos_pcie_host_init(pp);
+	ret = exynos_pcie_host_init(pp);
+	if (ret) {
+		err = exynos_pcie_phy_exit(ep);
+		if (err)
+			dev_err(dev, "failed to exit PCIe PHY during resume error: %d\n", err);
+
+		if (ep->supplies_enabled) {
+			err = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+			if (err)
+				dev_err(dev, "failed to disable regulators: %d\n", err);
+			else
+				ep->supplies_enabled = false;
+		}
+
+		return ret;
+
+	}
+
 	dw_pcie_setup_rc(pp);
 	exynos_pcie_start_link(pci);
 	return dw_pcie_wait_for_link(pci);
-- 
2.55.0

Re: [PATCH v4] PCI: exynos: Handle PCIe PHY initialization errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] PCI: exynos: unrecovered PHY and RC state in exynos_pcie_suspend_noirq()
--

commit 7a778ecdb6665562ea191dff854d89384184c33b
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

PCI: exynos: Handle PCIe PHY initialization errors

This commit adds error checking for phy_init() and phy_power_on() during host
initialization, tracks PHY and regulator state for cleanup, and handles host
initialization errors during resume.

> diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
> index e3c05ade381bb..5b8d8f1ce8dcd 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c

[ ... ]

> @@ -332,24 +381,48 @@ static int exynos_pcie_probe(struct platform_device *pdev)

[ ... ]

>  static int exynos_pcie_suspend_noirq(struct device *dev)
>  {
>  	struct exynos_pcie *ep = dev_get_drvdata(dev);
> +	int ret;
>  
>  	exynos_pcie_assert_core_reset(ep);
> -	phy_power_off(ep->phy);
> -	phy_exit(ep->phy);
> -	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +
> +	ret = exynos_pcie_phy_exit(ep);
> +	if (ret)
> +		goto err_deassert_reset;
> +
> +	if (ep->supplies_enabled) {
> +		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +		if (ret)
> +			goto err_deassert_reset;
> +
> +		ep->supplies_enabled = false;
> +	}
>  
>  	return 0;
> +
> +err_deassert_reset:
> +	exynos_pcie_deassert_core_reset(ep);
> +	return ret;
>  }

[Severity: Medium]
As reported by Sashiko on v3 [1], if exynos_pcie_phy_exit() or
regulator_bulk_disable() fails in exynos_pcie_suspend_noirq(), jumping to
err_deassert_reset only calls exynos_pcie_deassert_core_reset(ep) before
returning a negative error code.

By that point, exynos_pcie_assert_core_reset(ep) has already cleared
PCIE_CORE_RESET_ENABLE, PCIE_STICKY_RESET, and PCIE_NONSTICKY_RESET, and
exynos_pcie_phy_exit(ep) may have already powered off and exited the PHY
(which in exynos5433_pcie_phy_exit() gates the reference clock via
PCIE_REFCLK_GATING_EN and isolates the PHY in the PMU). Deasserting core
reset while the PHY is powered down and uninitialized violates the ordering in
exynos_pcie_host_init(), where phy_init() and phy_power_on() precede
exynos_pcie_deassert_core_reset().

Furthermore, in exynos_pcie_phy_exit():

	if (ep->phy_powered) {
		ret = phy_power_off(ep->phy);
		if (ret)
			return ret;

		ep->phy_powered = false;
	}

	if (ep->phy_initialized) {
		ret = phy_exit(ep->phy);
		if (ret)
			return ret;

		ep->phy_initialized = false;
	}

if phy_power_off() succeeds and phy_exit() fails, ep->phy_powered is false
while ep->phy_initialized remains true with phy->init_count == 1. Any
subsequent call to exynos_pcie_host_init() will call phy_init(ep->phy)
unconditionally, incrementing phy->init_count to 2 while ep->phy_initialized
only tracks a single boolean reference.

Finally, because exynos_pcie_suspend_noirq() returns a non-zero error to
device_suspend_noirq(), dev->power.is_noirq_suspended is not set and
exynos_pcie_resume_noirq() will not be called during suspend-abort recovery.

Would this leave the Root Complex active from the PM core's perspective while
its PHY remains powered down and uninitialized, and without restoring IRQ
pulse configuration, Root Complex setup, or the PCIe link?

[1] https://lore.kernel.org/r/20261030151701.3100-1-sreekuttan2156239@gmail.com

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

[PATCH v5] PCI: exynos: Handle PCIe PHY initialization errors

[Sreeraj S Kurup]

Check the return values of "phy_init()" and "phy_power_on()" to
avoid continuing PCIe host initialization when PHY setup fails.

Track the PHY initialization and power state to ensure cleanup is
performed only when appropriate, avoiding double calls to
"phy_exit()" on probe failure.

Also check the return value of "exynos_pcie_host_init()" during
resume and abort the resume sequence if PHY initialization fails.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
v5:
 - Avoid PM core state desynchronization in exynos_pcie_suspend_noirq()
 by logging teardown errors and returning 0 instead of aborting suspend
 on teardown failure
v4:
 - Fix invalid return in void exynos_pcie_remove() and ensure regulator cleanup
 - Add core reset deassertion on error in exynos_pcie_suspend_noirq()
 - Prevent regulator leaks in exynos_pcie_resume_noirq() error recovery path
v3:
 - Address review feedback on PHY and regulator state tracking
 - Avoid powering down the controller when link detection fails
v2:
 - Track PHY initialization and power state to prevent duplicate cleanup
 - Handle host initialization failures during resume

 drivers/pci/controller/dwc/pci-exynos.c | 111 +++++++++++++++++++++---
 1 file changed, 100 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
index e3c05ade381b..d82bdfea1f17 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -11,6 +11,7 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/device.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
@@ -55,8 +56,34 @@ struct exynos_pcie {
 	struct clk_bulk_data		*clks;
 	struct phy			*phy;
 	struct regulator_bulk_data	supplies[2];
+	bool phy_initialized;
+	bool phy_powered;
+	bool supplies_enabled;
 };
 
+static int exynos_pcie_phy_exit(struct exynos_pcie *ep)
+{
+	int ret;
+
+	if (ep->phy_powered) {
+		ret = phy_power_off(ep->phy);
+		if (ret)
+			return ret;
+
+		ep->phy_powered = false;
+	}
+
+	if (ep->phy_initialized) {
+		ret = phy_exit(ep->phy);
+		if (ret)
+			return ret;
+
+		ep->phy_initialized = false;
+	}
+
+	return 0;
+}
+
 static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)
 {
 	writel(val, base + reg);
@@ -225,13 +252,23 @@ static int exynos_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct exynos_pcie *ep = to_exynos_pcie(pci);
+	int ret;
 
 	pp->bridge->ops = &exynos_pci_ops;
 
 	exynos_pcie_assert_core_reset(ep);
 
-	phy_init(ep->phy);
-	phy_power_on(ep->phy);
+	ret = phy_init(ep->phy);
+	if (ret)
+		return ret;
+
+	ep->phy_initialized = true;
+
+	ret = phy_power_on(ep->phy);
+	if (ret)
+		return ret;
+
+	ep->phy_powered = true;
 
 	exynos_pcie_deassert_core_reset(ep);
 	exynos_pcie_enable_irq_pulse(ep);
@@ -314,6 +351,7 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ep->supplies_enabled = true;
 	platform_set_drvdata(pdev, ep);
 
 	ret = exynos_add_pcie_port(ep, pdev);
@@ -323,8 +361,19 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	return 0;
 
 fail_probe:
-	phy_exit(ep->phy);
-	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+	if (exynos_pcie_phy_exit(ep))
+		dev_err(&pdev->dev, "failed to exit PCIe PHY\n");
+
+	if (ep->supplies_enabled) {
+		int err;
+
+		err = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+		if (err)
+			dev_err(&pdev->dev, "failed to disable regulators: %d\n", err);
+		else
+			ep->supplies_enabled = false;
+
+	}
 
 	return ret;
 }
@@ -332,22 +381,42 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 static void exynos_pcie_remove(struct platform_device *pdev)
 {
 	struct exynos_pcie *ep = platform_get_drvdata(pdev);
+	int ret;
 
 	dw_pcie_host_deinit(&ep->pci.pp);
 	exynos_pcie_assert_core_reset(ep);
-	phy_power_off(ep->phy);
-	phy_exit(ep->phy);
-	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+
+	ret = exynos_pcie_phy_exit(ep);
+	if (ret)
+		dev_err(&pdev->dev, "failed to exit PCIe PHY: %d\n", ret);
+
+	if (ep->supplies_enabled) {
+		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+		if (ret)
+			dev_err(&pdev->dev, "failed to disable regulators: %d\n", ret);
+		else
+			ep->supplies_enabled = false;
+	}
 }
 
 static int exynos_pcie_suspend_noirq(struct device *dev)
 {
 	struct exynos_pcie *ep = dev_get_drvdata(dev);
+	int ret;
 
 	exynos_pcie_assert_core_reset(ep);
-	phy_power_off(ep->phy);
-	phy_exit(ep->phy);
-	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+
+	ret = exynos_pcie_phy_exit(ep);
+	if (ret)
+		dev_err(dev, "failed to exit PCIe PHY during suspend: %d\n", ret);
+
+	if (ep->supplies_enabled) {
+		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+		if (ret)
+			dev_err(dev, "failed to disable regulators during suspend: %d\n", ret);
+		else
+			ep->supplies_enabled = false;
+	}
 
 	return 0;
 }
@@ -358,13 +427,33 @@ static int exynos_pcie_resume_noirq(struct device *dev)
 	struct dw_pcie *pci = &ep->pci;
 	struct dw_pcie_rp *pp = &pci->pp;
 	int ret;
+	int err;
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
 	if (ret)
 		return ret;
 
+	ep->supplies_enabled = true;
+
 	/* exynos_pcie_host_init controls ep->phy */
-	exynos_pcie_host_init(pp);
+	ret = exynos_pcie_host_init(pp);
+	if (ret) {
+		err = exynos_pcie_phy_exit(ep);
+		if (err)
+			dev_err(dev, "failed to exit PCIe PHY during resume error: %d\n", err);
+
+		if (ep->supplies_enabled) {
+			err = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+			if (err)
+				dev_err(dev, "failed to disable regulators: %d\n", err);
+			else
+				ep->supplies_enabled = false;
+		}
+
+		return ret;
+
+	}
+
 	dw_pcie_setup_rc(pp);
 	exynos_pcie_start_link(pci);
 	return dw_pcie_wait_for_link(pci);
-- 
2.55.0

Re: [PATCH v5] PCI: exynos: Handle PCIe PHY initialization errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PCI: exynos: PHY and regulator ref leak in exynos_pcie_resume_noirq()
- [Medium] PCI: exynos: missing core reset assertion on post-init probe failure
--

commit e18d30d63d8279f6ed367a47b2c6e198b4bb8418
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

PCI: exynos: Handle PCIe PHY initialization errors

This commit checks return values from phy_init(), phy_power_on(), and
exynos_pcie_host_init(), and introduces boolean flags on struct exynos_pcie
to track PHY and regulator states across probe, remove, suspend, and resume.

> diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
> index e3c05ade381bb..d82bdfea1f17b 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c
> @@ -55,8 +56,34 @@ struct exynos_pcie {
>  	struct clk_bulk_data		*clks;
>  	struct phy			*phy;
>  	struct regulator_bulk_data	supplies[2];
> +	bool phy_initialized;
> +	bool phy_powered;
> +	bool supplies_enabled;
>  };
>  
> +static int exynos_pcie_phy_exit(struct exynos_pcie *ep)
> +{
> +	int ret;
> +
> +	if (ep->phy_powered) {
> +		ret = phy_power_off(ep->phy);
> +		if (ret)
> +			return ret;
> +
> +		ep->phy_powered = false;
> +	}
> +
> +	if (ep->phy_initialized) {
> +		ret = phy_exit(ep->phy);
> +		if (ret)
> +			return ret;
> +
> +		ep->phy_initialized = false;
> +	}
> +
> +	return 0;
> +}
> +
>  static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)
>  {
>  	writel(val, base + reg);
> @@ -225,13 +252,23 @@ static int exynos_pcie_host_init(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct exynos_pcie *ep = to_exynos_pcie(pci);
> +	int ret;
>  
>  	pp->bridge->ops = &exynos_pci_ops;
>  
>  	exynos_pcie_assert_core_reset(ep);
>  
> -	phy_init(ep->phy);
> -	phy_power_on(ep->phy);
> +	ret = phy_init(ep->phy);
> +	if (ret)
> +		return ret;
> +
> +	ep->phy_initialized = true;
> +
> +	ret = phy_power_on(ep->phy);
> +	if (ret)
> +		return ret;
> +
> +	ep->phy_powered = true;
>  
>  	exynos_pcie_deassert_core_reset(ep);
>  	exynos_pcie_enable_irq_pulse(ep);
> @@ -314,6 +351,7 @@ static int exynos_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ep->supplies_enabled = true;
>  	platform_set_drvdata(pdev, ep);
>  
>  	ret = exynos_add_pcie_port(ep, pdev);
> @@ -323,8 +361,19 @@ static int exynos_pcie_probe(struct platform_device *pdev)
>  	return 0;
>  
>  fail_probe:
> -	phy_exit(ep->phy);
> -	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +	if (exynos_pcie_phy_exit(ep))
> +		dev_err(&pdev->dev, "failed to exit PCIe PHY\n");
> +
> +	if (ep->supplies_enabled) {
> +		int err;
> +
> +		err = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +		if (err)
> +			dev_err(&pdev->dev, "failed to disable regulators: %d\n", err);
> +		else
> +			ep->supplies_enabled = false;
> +
> +	}

[Severity: Medium]
When dw_pcie_host_init() fails after exynos_pcie_host_init() has already
succeeded (for example, if dw_pcie_wait_for_link() returns -ETIMEDOUT or
pci_host_probe() fails), exynos_pcie_host_init() has already deasserted core
reset via exynos_pcie_deassert_core_reset(ep) and enabled pulse interrupts.

Because exynos_pcie_host_ops does not provide a .deinit callback for the
err_deinit_host path in dw_pcie_host_init(), control returns to fail_probe in
exynos_pcie_probe(), which powers off and exits ep->phy and disables
ep->supplies without calling exynos_pcie_assert_core_reset(ep) first (unlike
exynos_pcie_remove() and exynos_pcie_suspend_noirq()). Note that calling
exynos_pcie_assert_core_reset(ep) unconditionally in fail_probe would not be
safe either, since fail_probe is also reached when exynos_add_pcie_port()
fails before dw_pcie_get_resources() maps pci->elbi_base.

Would it be cleaner and safer to unwind phy_init(ep->phy) locally inside
exynos_pcie_host_init() when phy_power_on(ep->phy) fails, and implement
dw_pcie_host_ops.deinit to call exynos_pcie_assert_core_reset(ep),
phy_power_off(ep->phy), and phy_exit(ep->phy), thereby ensuring core reset is
asserted on post-init probe failures and avoiding manual boolean state
tracking across probe, remove, suspend, and resume?

>  	return ret;
>  }

[ ... ]

> @@ -332,22 +381,42 @@ static void exynos_pcie_remove(struct platform_device *pdev)

[ ... ]

>  static int exynos_pcie_suspend_noirq(struct device *dev)
>  {
>  	struct exynos_pcie *ep = dev_get_drvdata(dev);
> +	int ret;
>  
>  	exynos_pcie_assert_core_reset(ep);
> -	phy_power_off(ep->phy);
> -	phy_exit(ep->phy);
> -	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +
> +	ret = exynos_pcie_phy_exit(ep);
> +	if (ret)
> +		dev_err(dev, "failed to exit PCIe PHY during suspend: %d\n", ret);
> +
> +	if (ep->supplies_enabled) {
> +		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +		if (ret)
> +			dev_err(dev, "failed to disable regulators during suspend: %d\n", ret);
> +		else
> +			ep->supplies_enabled = false;
> +	}
>  
>  	return 0;
>  }
>  
>  static int exynos_pcie_resume_noirq(struct device *dev)
>  {
>  	struct exynos_pcie *ep = dev_get_drvdata(dev);
>  	struct dw_pcie *pci = &ep->pci;
>  	struct dw_pcie_rp *pp = &pci->pp;
>  	int ret;
> +	int err;
>  
>  	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
>  	if (ret)
>  		return ret;
>  
> +	ep->supplies_enabled = true;
> +
>  	/* exynos_pcie_host_init controls ep->phy */
> -	exynos_pcie_host_init(pp);
> +	ret = exynos_pcie_host_init(pp);

[Severity: High]
As reported by Sashiko on v4 [1], in exynos_pcie_suspend_noirq(), if
exynos_pcie_phy_exit(ep) or regulator_bulk_disable() fails, the error is
logged and 0 is returned so system suspend proceeds, while ep->phy_powered,
ep->phy_initialized, or ep->supplies_enabled remains true. Furthermore, if
phy_power_off(ep->phy) fails in exynos_pcie_phy_exit(), it returns early
without calling phy_exit(ep->phy) (which also leaks phy->init_count in
fail_probe and exynos_pcie_remove()), and exynos_pcie_suspend_noirq() still
proceeds to call regulator_bulk_disable() on the vdd18 and vdd10 supplies
while the PHY remains powered on.

When exynos_pcie_resume_noirq() subsequently runs, it unconditionally calls
regulator_bulk_enable() and exynos_pcie_host_init(pp) -> phy_init(ep->phy) /
phy_power_on(ep->phy) without checking whether ep->supplies_enabled,
ep->phy_initialized, or ep->phy_powered are already true.

Because regulator_bulk_disable(), phy_power_off(), and phy_exit() leave their
internal enable_count, power_count, and init_count at 1 on failure, calling
regulator_bulk_enable(), phy_init(), or phy_power_on() again increments those
counts to 2 while struct exynos_pcie only tracks state with single booleans.
In addition, phy_init() and phy_power_on() only invoke phy->ops->init and
phy->ops->power_on when init_count == 0 and power_count == 0, so hardware PHY
re-initialization is skipped on resume. Subsequent teardown in
exynos_pcie_remove() or the next suspend then decrements the counts only once
(from 2 to 1) before clearing the booleans to false.

Doesn't this permanently leak the PHY and regulator reference counts and skip
hardware PHY re-initialization on resume?

[1] https://lore.kernel.org/r/20261030163654.2733-1-sreekuttan2156239@gmail.com

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

[PATCH v6] PCI: exynos: Handle PCIe PHY initialization errors

[Sreeraj S Kurup]

Check the return values of "phy_init()" and "phy_power_on()" to
avoid continuing PCIe host initialization when PHY setup fails.

Track the PHY initialization and power state to ensure cleanup is
performed only when appropriate, avoiding double calls to
"phy_exit()" on probe failure.

Also check the return value of "exynos_pcie_host_init()" during
resume and abort the resume sequence if PHY initialization fails.

Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
v6:
 - Prevent state/reference count desynchronization in exynos_pcie_phy_exit()
 by ensuring both phy_power_off() and phy_exit() attempt cleanup even if one
 fails
 - Assert core reset on post-init probe failures in fail_probe if PHY/host
 initialization was reached
v5:
 - Avoid PM core state desynchronization in exynos_pcie_suspend_noirq()
 by logging teardown errors and returning 0 instead of aborting suspend
 on teardown failure
v4:
 - Fix invalid return in void exynos_pcie_remove() and ensure regulator cleanup
 - Add core reset deassertion on error in exynos_pcie_suspend_noirq()
 - Prevent regulator leaks in exynos_pcie_resume_noirq() error recovery path
v3:
 - Address review feedback on PHY and regulator state tracking
 - Avoid powering down the controller when link detection fails
v2:
 - Track PHY initialization and power state to prevent duplicate cleanup
 - Handle host initialization failures during resume

 drivers/pci/controller/dwc/pci-exynos.c | 115 +++++++++++++++++++++---
 1 file changed, 104 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
index e3c05ade381b..0085c3ed0c2f 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -11,6 +11,7 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/device.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
@@ -55,8 +56,35 @@ struct exynos_pcie {
 	struct clk_bulk_data		*clks;
 	struct phy			*phy;
 	struct regulator_bulk_data	supplies[2];
+	bool phy_initialized;
+	bool phy_powered;
+	bool supplies_enabled;
 };
 
+static int exynos_pcie_phy_exit(struct exynos_pcie *ep)
+{
+	int ret = 0;
+	int err;
+
+	if (ep->phy_powered) {
+		err = phy_power_off(ep->phy);
+		if (err)
+			ret = err;
+		else
+			ep->phy_powered = false;
+	}
+
+	if (ep->phy_initialized) {
+		err = phy_exit(ep->phy);
+		if (err)
+			ret = err;
+		else
+			ep->phy_initialized = false;
+	}
+
+	return ret;
+}
+
 static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)
 {
 	writel(val, base + reg);
@@ -225,13 +253,23 @@ static int exynos_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct exynos_pcie *ep = to_exynos_pcie(pci);
+	int ret;
 
 	pp->bridge->ops = &exynos_pci_ops;
 
 	exynos_pcie_assert_core_reset(ep);
 
-	phy_init(ep->phy);
-	phy_power_on(ep->phy);
+	ret = phy_init(ep->phy);
+	if (ret)
+		return ret;
+
+	ep->phy_initialized = true;
+
+	ret = phy_power_on(ep->phy);
+	if (ret)
+		return ret;
+
+	ep->phy_powered = true;
 
 	exynos_pcie_deassert_core_reset(ep);
 	exynos_pcie_enable_irq_pulse(ep);
@@ -314,6 +352,7 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ep->supplies_enabled = true;
 	platform_set_drvdata(pdev, ep);
 
 	ret = exynos_add_pcie_port(ep, pdev);
@@ -323,8 +362,22 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	return 0;
 
 fail_probe:
-	phy_exit(ep->phy);
-	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+	if (ep->phy_initialized || ep->phy_powered)
+		exynos_pcie_assert_core_reset(ep);
+
+	if (exynos_pcie_phy_exit(ep))
+		dev_err(&pdev->dev, "failed to exit PCIe PHY\n");
+
+	if (ep->supplies_enabled) {
+		int err;
+
+		err = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+		if (err)
+			dev_err(&pdev->dev, "failed to disable regulators: %d\n", err);
+		else
+			ep->supplies_enabled = false;
+
+	}
 
 	return ret;
 }
@@ -332,22 +385,42 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 static void exynos_pcie_remove(struct platform_device *pdev)
 {
 	struct exynos_pcie *ep = platform_get_drvdata(pdev);
+	int ret;
 
 	dw_pcie_host_deinit(&ep->pci.pp);
 	exynos_pcie_assert_core_reset(ep);
-	phy_power_off(ep->phy);
-	phy_exit(ep->phy);
-	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+
+	ret = exynos_pcie_phy_exit(ep);
+	if (ret)
+		dev_err(&pdev->dev, "failed to exit PCIe PHY: %d\n", ret);
+
+	if (ep->supplies_enabled) {
+		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+		if (ret)
+			dev_err(&pdev->dev, "failed to disable regulators: %d\n", ret);
+		else
+			ep->supplies_enabled = false;
+	}
 }
 
 static int exynos_pcie_suspend_noirq(struct device *dev)
 {
 	struct exynos_pcie *ep = dev_get_drvdata(dev);
+	int ret;
 
 	exynos_pcie_assert_core_reset(ep);
-	phy_power_off(ep->phy);
-	phy_exit(ep->phy);
-	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+
+	ret = exynos_pcie_phy_exit(ep);
+	if (ret)
+		dev_err(dev, "failed to exit PCIe PHY during suspend: %d\n", ret);
+
+	if (ep->supplies_enabled) {
+		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+		if (ret)
+			dev_err(dev, "failed to disable regulators during suspend: %d\n", ret);
+		else
+			ep->supplies_enabled = false;
+	}
 
 	return 0;
 }
@@ -358,13 +431,33 @@ static int exynos_pcie_resume_noirq(struct device *dev)
 	struct dw_pcie *pci = &ep->pci;
 	struct dw_pcie_rp *pp = &pci->pp;
 	int ret;
+	int err;
 
 	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
 	if (ret)
 		return ret;
 
+	ep->supplies_enabled = true;
+
 	/* exynos_pcie_host_init controls ep->phy */
-	exynos_pcie_host_init(pp);
+	ret = exynos_pcie_host_init(pp);
+	if (ret) {
+		err = exynos_pcie_phy_exit(ep);
+		if (err)
+			dev_err(dev, "failed to exit PCIe PHY during resume error: %d\n", err);
+
+		if (ep->supplies_enabled) {
+			err = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
+			if (err)
+				dev_err(dev, "failed to disable regulators: %d\n", err);
+			else
+				ep->supplies_enabled = false;
+		}
+
+		return ret;
+
+	}
+
 	dw_pcie_setup_rc(pp);
 	exynos_pcie_start_link(pci);
 	return dw_pcie_wait_for_link(pci);
-- 
2.55.0

Re: [PATCH v6] PCI: exynos: Handle PCIe PHY initialization errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PCI: exynos: PHY and regulator ref leak in exynos_pcie_resume_noirq()
- [Low] PCI: exynos: commit message omits regulator tracking and reset changes
--

commit fc4bd96cfce4c0daa59d215a173d06803b550297
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

PCI: exynos: Handle PCIe PHY initialization errors

This commit checks the return values of phy_init(), phy_power_on(), and
exynos_pcie_host_init(), tracks PHY and regulator state across probe, remove,
suspend, and resume, and updates teardown and error recovery paths in the
Exynos PCIe driver.

>     PCI: exynos: Handle PCIe PHY initialization errors
>     
>     Check the return values of "phy_init()" and "phy_power_on()" to
>     avoid continuing PCIe host initialization when PHY setup fails.
>     
>     Track the PHY initialization and power state to ensure cleanup is
>     performed only when appropriate, avoiding double calls to
>     "phy_exit()" on probe failure.
>     
>     Also check the return value of "exynos_pcie_host_init()" during
>     resume and abort the resume sequence if PHY initialization fails.
>     
>     Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

[Severity: Low]
This isn't a bug, but could the commit message also describe the regulator
state tracking and reset handling changes introduced by this patch?

In addition to PHY state tracking, the patch introduces ep->supplies_enabled
across exynos_pcie_probe(), exynos_pcie_remove(), exynos_pcie_suspend_noirq(),
and exynos_pcie_resume_noirq(), disables regulators when
exynos_pcie_host_init() fails during resume, asserts core reset via
exynos_pcie_assert_core_reset() in the probe failure path when PHY setup was
reached, and logs teardown errors in remove and suspend. Since the revision
notes below the tear line are stripped when the patch is applied, these
changes would otherwise be missing from the commit log.

> diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
> index e3c05ade381bb..0085c3ed0c2f5 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c

[ ... ]

> @@ -55,8 +56,35 @@ struct exynos_pcie {
>  	struct clk_bulk_data		*clks;
>  	struct phy			*phy;
>  	struct regulator_bulk_data	supplies[2];
> +	bool phy_initialized;
> +	bool phy_powered;
> +	bool supplies_enabled;
>  };
>  
> +static int exynos_pcie_phy_exit(struct exynos_pcie *ep)
> +{
> +	int ret = 0;
> +	int err;
> +
> +	if (ep->phy_powered) {
> +		err = phy_power_off(ep->phy);
> +		if (err)
> +			ret = err;
> +		else
> +			ep->phy_powered = false;
> +	}
> +
> +	if (ep->phy_initialized) {
> +		err = phy_exit(ep->phy);
> +		if (err)
> +			ret = err;
> +		else
> +			ep->phy_initialized = false;
> +	}
> +
> +	return ret;
> +}
> +
>  static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)
>  {
>  	writel(val, base + reg);
> @@ -225,13 +253,23 @@ static int exynos_pcie_host_init(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct exynos_pcie *ep = to_exynos_pcie(pci);
> +	int ret;
>  
>  	pp->bridge->ops = &exynos_pci_ops;
>  
>  	exynos_pcie_assert_core_reset(ep);
>  
> -	phy_init(ep->phy);
> -	phy_power_on(ep->phy);
> +	ret = phy_init(ep->phy);
> +	if (ret)
> +		return ret;
> +
> +	ep->phy_initialized = true;
> +
> +	ret = phy_power_on(ep->phy);
> +	if (ret)
> +		return ret;
> +
> +	ep->phy_powered = true;

[ ... ]

> @@ -332,22 +385,42 @@ static void exynos_pcie_remove(struct platform_device *pdev)

[ ... ]

>  static int exynos_pcie_suspend_noirq(struct device *dev)
>  {
>  	struct exynos_pcie *ep = dev_get_drvdata(dev);
> +	int ret;
>  
>  	exynos_pcie_assert_core_reset(ep);
> -	phy_power_off(ep->phy);
> -	phy_exit(ep->phy);
> -	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +
> +	ret = exynos_pcie_phy_exit(ep);
> +	if (ret)
> +		dev_err(dev, "failed to exit PCIe PHY during suspend: %d\n", ret);
> +
> +	if (ep->supplies_enabled) {
> +		ret = regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> +		if (ret)
> +			dev_err(dev, "failed to disable regulators during suspend: %d\n", ret);
> +		else
> +			ep->supplies_enabled = false;
> +	}
>  
>  	return 0;
>  }
>  
>  static int exynos_pcie_resume_noirq(struct device *dev)
>  {
>  	struct exynos_pcie *ep = dev_get_drvdata(dev);
>  	struct dw_pcie *pci = &ep->pci;
>  	struct dw_pcie_rp *pp = &pci->pp;
>  	int ret;
> +	int err;
>  
>  	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
>  	if (ret)
>  		return ret;
>  
> +	ep->supplies_enabled = true;
> +
>  	/* exynos_pcie_host_init controls ep->phy */
> -	exynos_pcie_host_init(pp);
> +	ret = exynos_pcie_host_init(pp);

[Severity: High]
As reported by Sashiko on v5 [1], can this sequence leak ep->supplies
regulator enable counts and ep->phy init_count/power_count on resume if
teardown fails during suspend?

In exynos_pcie_suspend_noirq(), if exynos_pcie_phy_exit() or
regulator_bulk_disable() fails, the error is logged, the corresponding state
flag (ep->phy_powered, ep->phy_initialized, or ep->supplies_enabled) remains
true, and the function returns 0 so suspend completes. At that point, the
underlying phy->power_count, phy->init_count, or regulator->enable_count is
still 1 (and in exynos_pcie_phy_exit(), phy_exit() is also called while the
PHY is still powered if phy_power_off() fails).

When exynos_pcie_resume_noirq() later runs, it unconditionally calls
regulator_bulk_enable() without checking !ep->supplies_enabled, and calls
exynos_pcie_host_init(), which unconditionally calls phy_init() and
phy_power_on() without checking !ep->phy_initialized and !ep->phy_powered.

Because the boolean flags only track a single reference, calling enable/init
again when a flag is already true increments regulator->enable_count,
phy->init_count, or phy->power_count to 2. In addition, phy_init() and
phy_power_on() only invoke phy->ops->init and phy->ops->power_on when the
counts transition from 0 to 1, so hardware PHY re-initialization is skipped
on resume even though core reset was asserted during suspend.

Later, when exynos_pcie_suspend_noirq(), the resume error path, or
exynos_pcie_remove() runs, regulator_bulk_disable(), phy_power_off(), and
phy_exit() are called at most once before clearing the booleans to false,
leaving the reference counts at 1 and triggering WARN_ON(regulator->enable_count)
on driver unbind.

Should exynos_pcie_resume_noirq() and exynos_pcie_host_init() guard
regulator_bulk_enable(), phy_init(), and phy_power_on() with checks on
ep->supplies_enabled, ep->phy_initialized, and ep->phy_powered, or otherwise
keep the state tracking in sync with the underlying reference counts?

[1] https://lore.kernel.org/r/20261030172350.2624-1-sreekuttan2156239@gmail.com

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