[PATCH v7 0/4] phy: ti-pipe3: Fix clock resource handling issues

[PATCH v7 0/4] phy: ti-pipe3: Fix clock resource handling issues

[Hongling Zeng]

This patch series fixes multiple clock resource handling issues in the
ti-pipe3 PHY driver.

Patch 1 fixes a critical issue where ti_pipe3_init() was ignoring the
return value of ti_pipe3_enable_clocks(), which could lead to unclocked
hardware access and unbalanced clock disables.

Patch 2 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 3 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.

Patch 4 fixes a clock leak in the init error path when regmap_update_bits()
fails in PCIe mode, adding proper clock cleanup consistent with other
error paths.

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

Hongling Zeng (4):
  phy: ti-pipe3: Fix ignored clock enable return value in init
  phy: ti: pipe3: Fix clock resource leak on probe errors
  phy: ti-pipe3: Fix EPROBE_DEFER handling for clock resources
  phy: ti-pipe3: Fix clock leak in init error path

 drivers/phy/ti/phy-ti-pipe3.c | 58 ++++++++++++++++++++++++++++--------
 1 file changed, 46 insertions(+), 12 deletions(-)

--
2.25.1

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

[PATCH v7 1/4] phy: ti-pipe3: Fix ignored clock enable return value in init

[Hongling Zeng]

ti_pipe3_init() ignores the return value of ti_pipe3_enable_clocks(),
which can lead to:

1. Unclocked hardware access if clock enable fails
2. Unbalanced clock disables in error paths

ti_pipe3_enable_clocks() returns an error code when clock enable fails
and rolls back any partially enabled clocks. If we ignore this error
and continue, we access hardware without proper clocking, which can
cause bus errors.

Additionally, if we reach error paths later in the function and call
ti_pipe3_disable_clocks(), we'll be disabling already-disabled clocks,
causing unbalanced disable warnings.

Fix this by checking the return value of ti_pipe3_enable_clocks()
and returning early if it fails.

Fixes: 234738ea3390 ("phy: ti-pipe3: move clk initialization to a separate function")
Reported-by: Sashiko AI <sashiko@kernel.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
Change in v7:
 -Remove the now-redundant initialization of ret, as suggested by Mani.
---
 drivers/phy/ti/phy-ti-pipe3.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
index b5543b5c674c..0e3bc9218733 100644
--- a/drivers/phy/ti/phy-ti-pipe3.c
+++ b/drivers/phy/ti/phy-ti-pipe3.c
@@ -498,9 +498,12 @@ static int ti_pipe3_init(struct phy *x)
 {
 	struct ti_pipe3 *phy = phy_get_drvdata(x);
 	u32 val;
-	int ret = 0;
+	int ret;
+
+	ret = ti_pipe3_enable_clocks(phy);
+	if (ret)
+		return ret;
 
-	ti_pipe3_enable_clocks(phy);
 	/*
 	 * Set pcie_pcs register to 0x96 for proper functioning of phy
 	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table
-- 
2.25.1


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

[PATCH v7 2/4] 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 v7:
 -Remove the redundant !IS_ERR(phy->refclk) check in the clock cleanup
  path, as suggested by Mani.
---
 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 0e3bc9218733..021657cbce48 100644
--- a/drivers/phy/ti/phy-ti-pipe3.c
+++ b/drivers/phy/ti/phy-ti-pipe3.c
@@ -834,21 +834,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)
+		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 v7 3/4] 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
- Use devm_clk_get_optional() for SATA refclk to handle optional
  clocks while propagating -EPROBE_DEFER and other errors

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 v7:
 -Use dev_err_probe() for refclk and sysclk acquisition failures.
---
 drivers/phy/ti/phy-ti-pipe3.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
index 021657cbce48..a67f5da43c83 100644
--- a/drivers/phy/ti/phy-ti-pipe3.c
+++ b/drivers/phy/ti/phy-ti-pipe3.c
@@ -608,14 +608,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 dev_err_probe(dev, PTR_ERR(phy->refclk),
+				"unable to get refclk\n");
 	}
 
 	if (phy->mode != PIPE3_MODE_SATA) {
@@ -631,8 +638,8 @@ static int ti_pipe3_get_clk(struct ti_pipe3 *phy)
 	if (phy->mode != PIPE3_MODE_PCIE || phy->phy_power_syscon) {
 		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 dev_err_probe(dev, PTR_ERR(phy->sys_clk),
+					"unable to get sysclk\n");
 		}
 	}
 
-- 
2.25.1


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

[PATCH v7 4/4] phy: ti-pipe3: Fix clock leak in init error path

[Hongling Zeng]

When regmap_update_bits() fails in ti_pipe3_init() for PCIe mode,
the function returns the error without calling ti_pipe3_disable_clocks().
This leaves the clocks permanently enabled since the PHY framework won't
invoke the .exit callback on init failure.

Fix this by adding proper clock cleanup in the PCIe error path, consistent
with how the DPLL program error path handles cleanup.

Fixes: 234738ea3390 ("phy: ti-pipe3: move clk initialization to a separate function")
Cc: stable@vger.kernel.org
Reported-by: Sashiko AI <sashiko@kernel.org>
Closes: https://lore.kernel.org/all/20260518023657.41852C2BCB0@smtp.kernel.org/
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
---
Change in v7:
 -Restore the original `return ret` in the SATA PLL-locked init path.
---
 drivers/phy/ti/phy-ti-pipe3.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
index a67f5da43c83..fea64db93da5 100644
--- a/drivers/phy/ti/phy-ti-pipe3.c
+++ b/drivers/phy/ti/phy-ti-pipe3.c
@@ -518,6 +518,8 @@ static int ti_pipe3_init(struct phy *x)
 		val = 0x96 << OMAP_CTRL_PCIE_PCS_DELAY_COUNT_SHIFT;
 		ret = regmap_update_bits(phy->pcs_syscon, phy->pcie_pcs_reg,
 					 PCIE_PCS_MASK, val);
+		if (ret)
+			ti_pipe3_disable_clocks(phy);
 		return ret;
 	}
 
@@ -555,8 +557,10 @@ static int ti_pipe3_exit(struct phy *x)
 	/* If dpll_reset_syscon is not present we wont power down SATA DPLL
 	 * due to Errata i783
 	 */
-	if (phy->mode == PIPE3_MODE_SATA && !phy->dpll_reset_syscon)
+	if (phy->mode == PIPE3_MODE_SATA && !phy->dpll_reset_syscon) {
+		ti_pipe3_disable_clocks(phy);
 		return 0;
+	}
 
 	/* PCIe doesn't have internal DPLL */
 	if (phy->mode != PIPE3_MODE_PCIE) {
-- 
2.25.1


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

Re: [PATCH v7 3/4] phy: ti-pipe3: Fix EPROBE_DEFER handling for clock resources

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] A redundant dev_err() call immediately before dev_err_probe() defeats the probe deferral suppression mechanism and causes double logging.
--

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

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

This commit fixes EPROBE_DEFER handling in ti_pipe3_get_clk() by propagating
the actual error code for the system clock instead of returning -EINVAL. It
also uses devm_clk_get_optional() for the SATA refclk to correctly handle
missing optional clocks while properly propagating probe deferrals.

> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> index 021657cbce481..a67f5da43c83b 100644
> --- a/drivers/phy/ti/phy-ti-pipe3.c
> +++ b/drivers/phy/ti/phy-ti-pipe3.c

[ ... ]

> @@ -608,14 +608,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 dev_err_probe(dev, PTR_ERR(phy->refclk),
> +				"unable to get refclk\n");

[Severity: Low]
Does keeping this dev_err() call defeat the purpose of using dev_err_probe()
below it?

Since dev_err_probe() suppresses logging during -EPROBE_DEFER, leaving the
unconditional dev_err() here will still output "unable to get refclk" to
the kernel log during normal probe deferrals. It will also cause the same
error message to be logged twice on actual failures.

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

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

Re: [PATCH v7 4/4] phy: ti-pipe3: Fix clock leak in init error path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch includes an undocumented fix for a clock leak in the SATA normal exit path.
--

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

phy: ti-pipe3: Fix clock leak in init error path

When regmap_update_bits() fails in ti_pipe3_init() for PCIe mode,
the function returns the error without calling ti_pipe3_disable_clocks().
This leaves the clocks permanently enabled since the PHY framework won't
invoke the .exit callback on init failure.

Link: https://lore.kernel.org/all/20260518023657.41852C2BCB0@smtp.kernel.org/

> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> index a67f5da43c83b..fea64db93da54 100644
> --- a/drivers/phy/ti/phy-ti-pipe3.c
> +++ b/drivers/phy/ti/phy-ti-pipe3.c
[ ... ]
> @@ -555,8 +557,10 @@ static int ti_pipe3_exit(struct phy *x)
>  	/* If dpll_reset_syscon is not present we wont power down SATA DPLL
>  	 * due to Errata i783
>  	 */
> -	if (phy->mode == PIPE3_MODE_SATA && !phy->dpll_reset_syscon)
> +	if (phy->mode == PIPE3_MODE_SATA && !phy->dpll_reset_syscon) {
> +		ti_pipe3_disable_clocks(phy);
>  		return 0;
> +	}
>  
>  	/* PCIe doesn't have internal DPLL */
>  	if (phy->mode != PIPE3_MODE_PCIE) {

[Severity: Medium]
Does the commit message need to document this fix in the SATA exit path?

The commit message only mentions fixing the clock leak in the ti_pipe3_init()
error path for PCIe mode. 

This addition of ti_pipe3_disable_clocks() in ti_pipe3_exit() for the SATA
mode seems like a meaningful functional change that isn't documented in the
commit log, which could cause confusion during review and hinder stable
backports.

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

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

Re: [PATCH v7 1/4] phy: ti-pipe3: Fix ignored clock enable return value in init

[sashiko-bot]

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

Pre-existing issues:
- [High] Clock enable count leak on race condition error path during SATA lock check in ti_pipe3_init()
--

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

phy: ti-pipe3: Fix ignored clock enable return value in init

ti_pipe3_init() ignores the return value of ti_pipe3_enable_clocks(),
which can lead to unclocked hardware access and unbalanced clock disables.
Fix this by checking the return value and returning early if it fails.

> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> index b5543b5c674c8..0e3bc92187338 100644
> --- a/drivers/phy/ti/phy-ti-pipe3.c
> +++ b/drivers/phy/ti/phy-ti-pipe3.c
> @@ -498,9 +498,12 @@ static int ti_pipe3_init(struct phy *x)
>  {
>  	struct ti_pipe3 *phy = phy_get_drvdata(x);
>  	u32 val;
> -	int ret = 0;
> +	int ret;
> +
> +	ret = ti_pipe3_enable_clocks(phy);
> +	if (ret)
> +		return ret;
>  
> -	ti_pipe3_enable_clocks(phy);
>  	/*
>  	 * Set pcie_pcs register to 0x96 for proper functioning of phy

[Severity: High]
This is a pre-existing issue, but further down in this function, does this
leak the clock enable count on a race condition error path?

If ti_pipe3_dpll_wait_lock() fails due to a timeout (returning -EBUSY), but
the DPLL locks immediately after, (val & PLL_LOCK) will evaluate to true.

For SATA mode, it then returns the stale -EBUSY error without disabling the
clocks:

drivers/phy/ti/phy-ti-pipe3.c:ti_pipe3_init() {
    ...
	if (val & PLL_IDLE) {
		val &= ~PLL_IDLE;
		ti_pipe3_writel(phy->pll_ctrl_base, PLL_CONFIGURATION2, val);
		ret = ti_pipe3_dpll_wait_lock(phy);
	}

	/* SATA has issues if re-programmed when locked */
	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_STATUS);
	if ((val & PLL_LOCK) && phy->mode == PIPE3_MODE_SATA)
		return ret;
    ...
}

While this requires specific hardware timings or delays (like high load or
preemption) allowing the timeout to expire just before the lock succeeds, it
remains a structural leak on this failure path.

Should this failure path also call ti_pipe3_disable_clocks() before returning
an error, similar to the other error paths in this function?

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

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