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

[PATCH v5 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.

Changes in v5:
- Add patch to fix ignored clock enable return value in ti_pipe3_init()
- Improve error handling consistency across all paths

Changes in v4:
- Use devm_clk_get_optional() for SATA refclk
- Drop manual -ENOENT 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 v5 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.

Changes in v5:
- Add patch to fix ignored clock enable return value in ti_pipe3_init()
- Improve error handling consistency across all paths

Changes in v4:
- Use devm_clk_get_optional() for SATA refclk
- Drop manual -ENOENT 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

[PATCH v5 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>
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>

---
Change in v5:
  -Add Fix ignored clock enable return value in init patch
---
 drivers/phy/ti/phy-ti-pipe3.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
index b5543b5c674c..b8c055893742 100644
--- a/drivers/phy/ti/phy-ti-pipe3.c
+++ b/drivers/phy/ti/phy-ti-pipe3.c
@@ -500,7 +500,10 @@ static int ti_pipe3_init(struct phy *x)
 	u32 val;
 	int ret = 0;
 
-	ti_pipe3_enable_clocks(phy);
+	ret = ti_pipe3_enable_clocks(phy);
+	if (ret)
+		return ret;
+
 	/*
 	 * 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 v5 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>
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>

---
Change in v5:
  -Add Fix ignored clock enable return value in init patch
---
 drivers/phy/ti/phy-ti-pipe3.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
index b5543b5c674c..b8c055893742 100644
--- a/drivers/phy/ti/phy-ti-pipe3.c
+++ b/drivers/phy/ti/phy-ti-pipe3.c
@@ -500,7 +500,10 @@ static int ti_pipe3_init(struct phy *x)
 	u32 val;
 	int ret = 0;
 
-	ti_pipe3_enable_clocks(phy);
+	ret = ti_pipe3_enable_clocks(phy);
+	if (ret)
+		return ret;
+
 	/*
 	 * 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

[PATCH v5 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 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
---
Change in v5:
  -Add Fix ignored clock enable return value in init patch
---
 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 b8c055893742..2d36fe4c4218 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 && !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 v5 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 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
---
Change in v5:
  -Add Fix ignored clock enable return value in init patch
---
 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 b8c055893742..2d36fe4c4218 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 && !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

[PATCH v5 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 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
---
  Change in v5:
   -Add Fix ignored clock enable return value in init patch
---
 drivers/phy/ti/phy-ti-pipe3.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
index 2d36fe4c4218..9ec228c2a940 100644
--- a/drivers/phy/ti/phy-ti-pipe3.c
+++ b/drivers/phy/ti/phy-ti-pipe3.c
@@ -608,14 +608,20 @@ 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) {
@@ -632,7 +638,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

[PATCH v5 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 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
---
  Change in v5:
   -Add Fix ignored clock enable return value in init patch
---
 drivers/phy/ti/phy-ti-pipe3.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
index 2d36fe4c4218..9ec228c2a940 100644
--- a/drivers/phy/ti/phy-ti-pipe3.c
+++ b/drivers/phy/ti/phy-ti-pipe3.c
@@ -608,14 +608,20 @@ 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) {
@@ -632,7 +638,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

Re: [PATCH v5 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:
- [Medium] The patch correctly fixes -EPROBE_DEFER propagation for `sysclk` and `refclk` but retains `dev_err()` for logging. This introduces dmesg spam because the probe will now be repeatedly deferred and retried, printing the error on every attempt.
--

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

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

This commit fixes probe deferral handling by propagating the actual error
codes for sysclk and refclk instead of returning -EINVAL or ignoring errors
completely.

> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> index 2d36fe4c4218c..9ec228c2a940c 100644
> --- a/drivers/phy/ti/phy-ti-pipe3.c
> +++ b/drivers/phy/ti/phy-ti-pipe3.c
> @@ -608,14 +608,20 @@ 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");

Now that the code correctly propagates -EPROBE_DEFER, will this dev_err()
call cause dmesg spam on systems where the clock provider is deferred and
retried multiple times?

Would it be better to replace dev_err() with dev_err_probe() here to cleanly
suppress the error message on deferrals and record the deferral reason?

> -		/* 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);
>  	}

[ ... ]

> @@ -632,7 +638,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");

Does this sysclk error handling also have the same logging issue when
returning -EPROBE_DEFER?

> -			return -EINVAL;
> +			return PTR_ERR(phy->sys_clk);
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518062938.48114-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 v5 3/4] phy: ti-pipe3: Fix EPROBE_DEFER handling for clock resources

[Hongling Zeng]

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

   Hi,
   Resend to fix threading / delivery issues.
   Thank you for the review. You raise a valid point about using dev_err_probe()
   to avoid dmesg spam during probe deferral.

   I've kept dev_err() in this patch to maintain consistency with the existing
   code style and focus on the core EPROBE_DEFER fixes. However, I agree that
   dev_err_probe() would be better and can submit a follow-up cleanup patch to
   address the logging improvement.
    
   Does this approach work for you, or would you prefer I include the dev_err_probe()
   change in this series?

   Best regards,
   Hongling

在 2026年05月18日 16:09, Hongling Zeng 写道:
> -- 
> Sashiko AI review ·https://sashiko.dev/#/patchset/20260518062938.48114-1-zenghongling@kylinos.cn?part=3
>
> -- 
>    Hi,
>
>    Thank you for the review. You raise a valid point about using dev_err_probe()
>    to avoid dmesg spam during probe deferral.
>
>    I've kept dev_err() in this patch to maintain consistency with the existing
>    code style and focus on the core EPROBE_DEFER fixes. However, I agree that
>    dev_err_probe() would be better and can submit a follow-up cleanup patch to
>    address the logging improvement.
>     
>    Does this approach work for you, or would you prefer I include the dev_err_probe()
>    change in this series?
>
>    Best regards,
>    Hongling
>
>
> 在 2026年05月18日 14:29, 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 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
>> ---
>>    Change in v5:
>>     -Add Fix ignored clock enable return value in init patch
>> ---
>>   drivers/phy/ti/phy-ti-pipe3.c | 20 +++++++++++++-------
>>   1 file changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
>> index 2d36fe4c4218..9ec228c2a940 100644
>> --- a/drivers/phy/ti/phy-ti-pipe3.c
>> +++ b/drivers/phy/ti/phy-ti-pipe3.c
>> @@ -608,14 +608,20 @@ 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) {
>> @@ -632,7 +638,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);
>>   		}
>>   	}
>>   
>


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

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

[Hongling Zeng]

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

   Hi,
   Resend to fix threading / delivery issues.
   Thank you for the review. You raise a valid point about using dev_err_probe()
   to avoid dmesg spam during probe deferral.

   I've kept dev_err() in this patch to maintain consistency with the existing
   code style and focus on the core EPROBE_DEFER fixes. However, I agree that
   dev_err_probe() would be better and can submit a follow-up cleanup patch to
   address the logging improvement.
    
   Does this approach work for you, or would you prefer I include the dev_err_probe()
   change in this series?

   Best regards,
   Hongling

在 2026年05月18日 16:09, Hongling Zeng 写道:
> -- 
> Sashiko AI review ·https://sashiko.dev/#/patchset/20260518062938.48114-1-zenghongling@kylinos.cn?part=3
>
> -- 
>    Hi,
>
>    Thank you for the review. You raise a valid point about using dev_err_probe()
>    to avoid dmesg spam during probe deferral.
>
>    I've kept dev_err() in this patch to maintain consistency with the existing
>    code style and focus on the core EPROBE_DEFER fixes. However, I agree that
>    dev_err_probe() would be better and can submit a follow-up cleanup patch to
>    address the logging improvement.
>     
>    Does this approach work for you, or would you prefer I include the dev_err_probe()
>    change in this series?
>
>    Best regards,
>    Hongling
>
>
> 在 2026年05月18日 14:29, 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 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
>> ---
>>    Change in v5:
>>     -Add Fix ignored clock enable return value in init patch
>> ---
>>   drivers/phy/ti/phy-ti-pipe3.c | 20 +++++++++++++-------
>>   1 file changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
>> index 2d36fe4c4218..9ec228c2a940 100644
>> --- a/drivers/phy/ti/phy-ti-pipe3.c
>> +++ b/drivers/phy/ti/phy-ti-pipe3.c
>> @@ -608,14 +608,20 @@ 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) {
>> @@ -632,7 +638,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);
>>   		}
>>   	}
>>   
>

[PATCH v5 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")
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>
Cc: stable@vger.kernel.org

---
Change in v5:
  -Add Fix ignored clock enable return value in init patch
---
 drivers/phy/ti/phy-ti-pipe3.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
index 9ec228c2a940..4897e4ba2d7d 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;
 	}
 
-- 
2.25.1


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

[PATCH v5 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")
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>
Cc: stable@vger.kernel.org

---
Change in v5:
  -Add Fix ignored clock enable return value in init patch
---
 drivers/phy/ti/phy-ti-pipe3.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
index 9ec228c2a940..4897e4ba2d7d 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;
 	}
 
-- 
2.25.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Resource leak in ti_pipe3_init() for SATA mode late-lock race.
- [High] Unbalanced clock lifecycle operations for SATA mode due to early return in ti_pipe3_exit().
--

commit d2822879f79b4be953a465847a8621060caa36ac
Author: Hongling Zeng <zenghongling@kylinos.cn>
Subject: phy: ti-pipe3: Fix clock leak in init error path

This commit fixes a clock reference count leak in the PCIe error path of
ti_pipe3_init(). It explicitly calls ti_pipe3_disable_clocks() when
regmap_update_bits() fails, ensuring proper cleanup 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 9ec228c2a940c..4897e4ba2d7db 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;
>  	}
>  

This problem wasn't introduced by this patch, but a similar regression
exists in the error handling path of ti_pipe3_init() just below this block.

When bringing the DPLL out of IDLE, if ti_pipe3_dpll_wait_lock() times out,
it sets ret to -EBUSY. Immediately after, PLL_STATUS is read:

	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
	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;

If the DPLL locked just after the timeout, the PLL_LOCK bit will be set.
For SATA mode, the function returns -EBUSY early without calling
ti_pipe3_disable_clocks().

Since the PHY framework won't invoke the exit callback on an init failure,
will this leak the clock reference counts?

Should the function either return 0 here to treat the late lock as a success,
or call ti_pipe3_disable_clocks() before returning the error?

Another regression that wasn't introduced by this patch exists in the driver's
lifecycle management for SATA mode.

In ti_pipe3_exit(), if the mode is SATA and dpll_reset_syscon is not present,
the function returns early to avoid powering down the SATA DPLL due to
Errata i783:

static int ti_pipe3_exit(struct phy *x)
{
	struct ti_pipe3 *phy = phy_get_drvdata(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)
		return 0;

However, it returns without calling ti_pipe3_disable_clocks().

Because ti_pipe3_init() unconditionally calls ti_pipe3_enable_clocks(),
could repeated initialization and exit cycles, such as during suspend and
resume or module reload, invoke clk_prepare_enable() without matching
clk_disable_unprepare() calls?

If the errata strictly requires keeping the clocks enabled, does the driver
need to track the initialization state to avoid double-enabling? Alternatively,
should it properly disable the clocks before returning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518062938.48114-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 v5 4/4] phy: ti-pipe3: Fix clock leak in init error path

[Hongling Zeng]

-- 
Sashiko AI review ·https://sashiko.dev/#/patchset/20260518062938.48114-1-zenghongling@kylinos.cn?part=4
--
Hi:
  Resend to fix threading / delivery issues.
   Thank you for identifying these SATA mode issues. Both are pre-existing bugs in
   the current codebase and were not introduced by this series.

   This series focuses on specific EPROBE_DEFER and resource leak fixes. The SATA
   mode issues you've found are valid but affect different code paths and should be
   addressed separately to keep the patch series focused.

   I can submit follow-up patches to fix these SATA mode issues. Would you prefer
   them in v6 or as separate patches after this series?

   Best regards,
   Hongling
  

在 2026年05月18日 14:29, 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")
> 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>
> Cc: stable@vger.kernel.org
>
> ---
> Change in v5:
>    -Add Fix ignored clock enable return value in init patch
> ---
>   drivers/phy/ti/phy-ti-pipe3.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> index 9ec228c2a940..4897e4ba2d7d 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;
>   	}
>   


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

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

[Hongling Zeng]

-- 
Sashiko AI review ·https://sashiko.dev/#/patchset/20260518062938.48114-1-zenghongling@kylinos.cn?part=4
--
Hi:
  Resend to fix threading / delivery issues.
   Thank you for identifying these SATA mode issues. Both are pre-existing bugs in
   the current codebase and were not introduced by this series.

   This series focuses on specific EPROBE_DEFER and resource leak fixes. The SATA
   mode issues you've found are valid but affect different code paths and should be
   addressed separately to keep the patch series focused.

   I can submit follow-up patches to fix these SATA mode issues. Would you prefer
   them in v6 or as separate patches after this series?

   Best regards,
   Hongling
  

在 2026年05月18日 14:29, 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")
> 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>
> Cc: stable@vger.kernel.org
>
> ---
> Change in v5:
>    -Add Fix ignored clock enable return value in init patch
> ---
>   drivers/phy/ti/phy-ti-pipe3.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/phy/ti/phy-ti-pipe3.c b/drivers/phy/ti/phy-ti-pipe3.c
> index 9ec228c2a940..4897e4ba2d7d 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;
>   	}
>