[PATCH v4 0/2] phy: ti-pipe3: Fix clock resource handling issues

[PATCH v4 0/2] phy: ti-pipe3: Fix clock resource handling issues

[Hongling Zeng]

This patch series fixes two clock resource handling issues in the
ti-pipe3 PHY driver:

Patch 1 fixes a clock resource leak when probe fails after enabling
the SATA refclk. The error path now properly disables the clock and
cleans up runtime PM resources.

Patch 2 fixes EPROBE_DEFER handling to prevent masking probe deferral
errors. It uses devm_clk_get_optional() for SATA refclk to properly
handle optional clocks while still propagating -EPROBE_DEFER and other
error codes.

These fixes ensure proper resource cleanup on probe failures and prevent
permanent driver initialization failures due to incorrect error handling.

Changes in v4:
- Fix patch version numbering (was incorrectly labeled as v2/v3)
- Ensure patches are in the same series to avoid breaking Patchwork

Hongling Zeng (2):
  phy: ti: pipe3: Fix clock resource leak on probe errors
  phy: ti-pipe3: Fix EPROBE_DEFER handling for clock resources

 drivers/phy/ti/phy-ti-pipe3.c | 47 +++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 11 deletions(-)

--
2.25.1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v4 1/2] phy: ti: pipe3: Fix clock resource leak on probe errors

[Hongling Zeng]

When devm_phy_create() or devm_of_phy_provider_register() fails,
the refclk that was enabled earlier is not disabled, causing a
resource leak.

Fix this by adding an error handling path to disable the clock
when these functions fail.

Fixes: 234738ea3390 ("phy: ti-pipe3: move clk initialization to a separate function")
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>

---
 Change in v2:
  -Add pm_runtime_disable() in error path (reported by Sashiko AI).
---
 change in v3:
  -Fix unbalanced clock disable by checking clk_prepare_enable()return value and
setting sata_refclk_enabledonly on success.
  -Fix error path teardown order to match ti_pipe3_remove()(disable clock before
disabling runtime PM).
---
 change in v4:
  -Fix the patch version
---
 drivers/phy/ti/phy-ti-pipe3.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
index b5543b5c674c..58fbc3b27813 100644
--- a/drivers/phy/ti/phy-ti-pipe3.c
+++ b/drivers/phy/ti/phy-ti-pipe3.c
@@ -831,21 +831,39 @@ static int ti_pipe3_probe(struct platform_device *pdev)
 	 */
 	if (phy->mode == PIPE3_MODE_SATA) {
 		if (!IS_ERR(phy->refclk)) {
-			clk_prepare_enable(phy->refclk);
+			ret = clk_prepare_enable(phy->refclk);
+			if (ret) {
+				dev_err(dev, "Failed to enable refclk %d\n", ret);
+				goto err_pm_disable;
+			}
 			phy->sata_refclk_enabled = true;
 		}
 	}
 
 	generic_phy = devm_phy_create(dev, NULL, &ops);
-	if (IS_ERR(generic_phy))
-		return PTR_ERR(generic_phy);
+	if (IS_ERR(generic_phy)) {
+		ret = PTR_ERR(generic_phy);
+		goto err_clk_disable;
+	}
 
 	phy_set_drvdata(generic_phy, phy);
 
 	ti_pipe3_power_off(generic_phy);
 
 	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
-	return PTR_ERR_OR_ZERO(phy_provider);
+	if (IS_ERR(phy_provider)) {
+		ret = PTR_ERR(phy_provider);
+		goto err_clk_disable;
+	}
+
+	return 0;
+
+err_clk_disable:
+	if (phy->sata_refclk_enabled && !IS_ERR(phy->refclk))
+		clk_disable_unprepare(phy->refclk);
+err_pm_disable:
+	pm_runtime_disable(dev);
+	return ret;
 }
 
 static void ti_pipe3_remove(struct platform_device *pdev)
-- 
2.25.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v4 2/2] phy: ti-pipe3: Fix EPROBE_DEFER handling for clock resources

[Hongling Zeng]

ti_pipe3_get_clk() has two issues with -EPROBE_DEFER error handling:

1. When devm_clk_get() for sysclk fails, the function returns -EINVAL
   instead of propagating the actual error code. This masks -EPROBE_DEFER
   to -EINVAL, breaking the probe deferral mechanism and causing permanent
   driver initialization failure on systems with non-deterministic probe
   ordering.

2. For SATA PHY refclk, the function ignores all errors to support older
   DTBs missing the refclk property. However, this incorrectly ignores
   -EPROBE_DEFER as well, causing the driver to proceed without waiting
   for the clock provider to become available.

Fix both issues:
- Return PTR_ERR(phy->sys_clk) instead of -EINVAL to propagate all
  error codes including -EPROBE_DEFER
- For SATA refclk, only ignore -ENOENT (clock not found in old DTBs)
  but propagate other errors like -EPROBE_DEFER

Fixes: a70143bbef6b ("drivers: phy: usb3/pipe3: Adapt pipe3 driver to Generic PHY Framework")
Fixes: 7f33912d2978 ("phy: ti-pipe3: Fix SATA across suspend/resume")
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>

---
  Change in v4:
   - Merge refclk leak fix and EPROBE_DEFER fix into a single patch
   - Use devm_clk_get_optional() for SATA refclk
   - Drop manual -ENOENT handling
   - Ensure error paths are fully symmetric
---
 drivers/phy/ti/phy-ti-pipe3.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
index 58fbc3b27813..e0ab7d21e99c 100644
--- a/drivers/phy/ti/phy-ti-pipe3.c
+++ b/drivers/phy/ti/phy-ti-pipe3.c
@@ -605,14 +605,21 @@ static int ti_pipe3_get_clk(struct ti_pipe3 *phy)
 	struct clk *clk;
 	struct device *dev = phy->dev;
 
-	phy->refclk = devm_clk_get(dev, "refclk");
+	/*
+	 * refclk is optional for SATA PHY to support older DTBs, but
+	 * required for other modes. Use devm_clk_get_optional() for SATA
+	 * which returns NULL for -ENOENT, allowing us to propagate all
+	 * other errors including -EPROBE_DEFER.
+	 */
+	if (phy->mode == PIPE3_MODE_SATA) {
+		phy->refclk = devm_clk_get_optional(dev, "refclk");
+	} else {
+		phy->refclk = devm_clk_get(dev, "refclk");
+	}
+
 	if (IS_ERR(phy->refclk)) {
 		dev_err(dev, "unable to get refclk\n");
-		/* older DTBs have missing refclk in SATA PHY
-		 * so don't bail out in case of SATA PHY.
-		 */
-		if (phy->mode != PIPE3_MODE_SATA)
-			return PTR_ERR(phy->refclk);
+		return PTR_ERR(phy->refclk);
 	}
 
 	if (phy->mode != PIPE3_MODE_SATA) {
@@ -629,7 +636,7 @@ static int ti_pipe3_get_clk(struct ti_pipe3 *phy)
 		phy->sys_clk = devm_clk_get(dev, "sysclk");
 		if (IS_ERR(phy->sys_clk)) {
 			dev_err(dev, "unable to get sysclk\n");
-			return -EINVAL;
+			return PTR_ERR(phy->sys_clk);
 		}
 	}
 
-- 
2.25.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 1/2] phy: ti: pipe3: Fix clock resource leak on probe errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Clock reference leak in ti_pipe3_init() on PCIe error path.
--

commit 26f79663d08d9c0cec90795a1b32d3e5e37b7841
Author: Hongling Zeng <zenghongling@kylinos.cn>

phy: ti: pipe3: Fix clock resource leak on probe errors

When devm_phy_create() or devm_of_phy_provider_register() fails,
the refclk that was enabled earlier is not disabled, causing a
resource leak.

Fix this by adding an error handling path to disable the clock
when these functions fail.

Fixes: 234738ea3390 ("phy: ti-pipe3: move clk initialization to a separate function")
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>

> Fix this by adding an error handling path to disable the clock
> when these functions fail.

This isn't a bug in this patch, but while fixing clock leaks in this driver,
does ti_pipe3_init() also leak the clock references on the PCIe error path?

If regmap_update_bits() fails in ti_pipe3_init(), the function returns the
error without calling ti_pipe3_disable_clocks():

drivers/phy/ti/phy-ti-pipe3.c:ti_pipe3_init() {
	ti_pipe3_enable_clocks(phy);
	...
	if (phy->mode == PIPE3_MODE_PCIE) {
		if (!phy->pcs_syscon) {
			omap_control_pcie_pcs(phy->control_dev, 0x96);
			return 0;
		}

		val = 0x96 << OMAP_CTRL_PCIE_PCS_DELAY_COUNT_SHIFT;
		ret = regmap_update_bits(phy->pcs_syscon, phy->pcie_pcs_reg,
					 PCIE_PCS_MASK, val);
		return ret;
	}
	...
}

Because the initialization fails, the core PHY framework will not invoke the
.exit callback, potentially leaving the clocks permanently enabled.

Unlike the ti_pipe3_dpll_program() error path later in the same function
which correctly calls ti_pipe3_disable_clocks(phy), this PCIe error path
appears to miss the cleanup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518021539.14890-1-zenghongling@kylinos.cn?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/2] phy: ti-pipe3: Fix EPROBE_DEFER handling for clock resources

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Log spam during boot due to lack of dev_err_probe() for deferred clocks.
- [Low] Unnecessary braces used for single-statement if/else branches.
--

commit e5fb8e810f7245e72a8e8c129ec34b40381c5a33
Author: Hongling Zeng <zenghongling@kylinos.cn>

phy: ti-pipe3: Fix EPROBE_DEFER handling for clock resources

This commit fixes probe deferral handling in the ti-pipe3 driver by returning
the actual error code for sysclk failures instead of masking it. It also
propagates non-ENOENT errors for the SATA refclk.

> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> --- a/drivers/phy/ti/phy-ti-pipe3.c
> +++ b/drivers/phy/ti/phy-ti-pipe3.c
> @@ -605,14 +605,21 @@ static int ti_pipe3_get_clk(struct ti_pipe3 *phy)
>  	struct clk *clk;
>  	struct device *dev = phy->dev;
>  
> -	phy->refclk = devm_clk_get(dev, "refclk");
> +	/*
> +	 * refclk is optional for SATA PHY to support older DTBs, but
> +	 * required for other modes. Use devm_clk_get_optional() for SATA
> +	 * which returns NULL for -ENOENT, allowing us to propagate all
> +	 * other errors including -EPROBE_DEFER.
> +	 */
> +	if (phy->mode == PIPE3_MODE_SATA) {
> +		phy->refclk = devm_clk_get_optional(dev, "refclk");
> +	} else {
> +		phy->refclk = devm_clk_get(dev, "refclk");
> +	}

This isn't a bug, but is it necessary to use braces here? The Linux kernel
coding style typically recommends omitting braces for single-statement
if/else branches.

> +
>  	if (IS_ERR(phy->refclk)) {
>  		dev_err(dev, "unable to get refclk\n");
> -		/* older DTBs have missing refclk in SATA PHY
> -		 * so don't bail out in case of SATA PHY.
> -		 */
> -		if (phy->mode != PIPE3_MODE_SATA)
> -			return PTR_ERR(phy->refclk);
> +		return PTR_ERR(phy->refclk);
>  	}

Since PTR_ERR() now correctly propagates -EPROBE_DEFER when the clock is not
yet available, will this dev_err() call cause repetitive log spam regressions
during normal boot sequence deferrals?

Could this be replaced with dev_err_probe() to properly suppress the error
message when the clock provider is simply deferred?

[ ... ]

> @@ -629,7 +636,7 @@ static int ti_pipe3_get_clk(struct ti_pipe3 *phy)
>  		phy->sys_clk = devm_clk_get(dev, "sysclk");
>  		if (IS_ERR(phy->sys_clk)) {
>  			dev_err(dev, "unable to get sysclk\n");
> -			return -EINVAL;
> +			return PTR_ERR(phy->sys_clk);
>  		}
>  	}

Does the same log spam regression apply here now that sysclk errors,
including -EPROBE_DEFER, are correctly propagated?

Using dev_err_probe() here might also help keep the kernel log clean during
probe deferral.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518021539.14890-1-zenghongling@kylinos.cn?part=2

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy