[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