[PATCH v2 0/4] phy: freescale: fsl-samsung-hdmi: Fix runtime PM issues

[PATCH v2 0/4] phy: freescale: fsl-samsung-hdmi: Fix runtime PM issues

[Can Peng]

Hi,

v1 fixed runtime PM cleanup when PHY clock registration fails. During
review, additional runtime PM and clock lifetime issues were pointed out
in the same driver:

- the APB clock can be disabled twice on remove if the device is runtime
  suspended;
- the PHY clock .set_rate() path can access registers after the provider
  device has been runtime suspended;
- runtime resume can leak the APB clock enable count if PHY
  reconfiguration fails.

This v2 keeps the original cleanup fix and adds targeted fixes for the
other cases.

The PHY clock is registered after runtime PM is enabled, so the common
clock framework takes a runtime PM reference on the provider device
before invoking the rate callbacks. Patch 3 keeps the APB clock prepared
for the lifetime of the device and only gates it in runtime PM callbacks,
which makes the provider resume path safe while rate callbacks access
PHY registers.

Changes in v2:
- Expand the original single patch into a four-patch series.
- Add fixes for the remove-time APB clock underflow, CCF .set_rate()
  runtime PM path and runtime-resume error unwind reported during
  review.
- Link to v1: 
  https://lore.kernel.org/all/20260718071816.437047-1-pengcan@kylinos.cn/

Can Peng (4):
  phy: freescale: fsl-samsung-hdmi: Fix runtime PM cleanup
  phy: freescale: fsl-samsung-hdmi: Resume device on remove
  phy: freescale: fsl-samsung-hdmi: Only gate APB clock in runtime PM
  phy: freescale: fsl-samsung-hdmi: Disable APB clock on resume failure

 drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 32 +++++++++++++++-----
 1 file changed, 25 insertions(+), 7 deletions(-)

-- 
2.53.0

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

[PATCH v2 1/4] phy: freescale: fsl-samsung-hdmi: Fix runtime PM cleanup

[Can Peng]

fsl_samsung_hdmi_phy_probe() takes a runtime PM reference, marks the
device active and enables runtime PM before registering the PHY clock.
If phy_clk_register() fails, probe returns without dropping the runtime
PM reference or disabling runtime PM.

The remove callback only unregisters the clock provider, so runtime PM
is also left enabled after a successful probe followed by driver unbind.

Check pm_runtime_set_active(), use devm_pm_runtime_enable() so runtime
PM is disabled automatically, and drop the initial noresume reference
on PM setup and clock registration failures.

Fixes: 6ad082bee902 ("phy: freescale: add Samsung HDMI PHY")
Cc: stable@vger.kernel.org
Signed-off-by: Can Peng <pengcan@kylinos.cn>
---
 drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
index d010fec15671..e7af4f9c7b20 100644
--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
@@ -665,20 +665,26 @@ static int fsl_samsung_hdmi_phy_probe(struct platform_device *pdev)
 				     "failed to get ref clk\n");
 
 	pm_runtime_get_noresume(phy->dev);
-	pm_runtime_set_active(phy->dev);
-	pm_runtime_enable(phy->dev);
+	ret = pm_runtime_set_active(phy->dev);
+	if (ret)
+		goto pm_put_noidle;
+
+	ret = devm_pm_runtime_enable(phy->dev);
+	if (ret)
+		goto pm_put_noidle;
 
 	ret = phy_clk_register(phy);
 	if (ret) {
 		dev_err(&pdev->dev, "register clk failed\n");
-		goto register_clk_failed;
+		goto pm_put_noidle;
 	}
 
 	pm_runtime_put(phy->dev);
 
 	return 0;
 
-register_clk_failed:
+pm_put_noidle:
+	pm_runtime_put_noidle(phy->dev);
 	return ret;
 }
 
-- 
2.53.0


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

[PATCH v2 2/4] phy: freescale: fsl-samsung-hdmi: Resume device on remove

[Can Peng]

The APB clock is acquired with devm_clk_get_enabled(), so devres will
disable and unprepare it when the driver is unbound.

Runtime suspend also disables the APB clock. If the device is runtime
suspended when the driver is removed, devres will disable the already
disabled clock and the clock enable count can underflow.

Resume the device in the remove callback before devres runs so the final
devres disable is balanced. Drop the runtime PM reference with noidle so
the device is not suspended again before devres cleanup.

Fixes: 6ad082bee902 ("phy: freescale: add Samsung HDMI PHY")
Cc: stable@vger.kernel.org
Signed-off-by: Can Peng <pengcan@kylinos.cn>
---
 drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
index e7af4f9c7b20..5f41890bd134 100644
--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
@@ -690,7 +690,16 @@ static int fsl_samsung_hdmi_phy_probe(struct platform_device *pdev)
 
 static void fsl_samsung_hdmi_phy_remove(struct platform_device *pdev)
 {
+	int ret;
+
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret < 0)
+		dev_warn(&pdev->dev, "failed to resume on remove: %d\n", ret);
+
 	of_clk_del_provider(pdev->dev.of_node);
+
+	if (!ret)
+		pm_runtime_put_noidle(&pdev->dev);
 }
 
 static int __maybe_unused fsl_samsung_hdmi_phy_suspend(struct device *dev)
-- 
2.53.0


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

[PATCH v2 3/4] phy: freescale: fsl-samsung-hdmi: Only gate APB clock in runtime PM

[Can Peng]

The APB clock is acquired with devm_clk_get_enabled(), so it remains
prepared until devres cleanup and runtime PM only needs to gate or ungate
it.

Using clk_prepare_enable() from runtime resume is also problematic when
the common clock framework runtime-resumes the provider device before
calling the PHY clock rate callbacks, because clk_set_rate() is executed
while holding the clock prepare lock.

Use clk_enable() and clk_disable() in the runtime PM callbacks so the APB
clock remains prepared for the lifetime of the device and runtime resume
does not re-enter the prepare path.

Fixes: 6ad082bee902 ("phy: freescale: add Samsung HDMI PHY")
Cc: stable@vger.kernel.org
Signed-off-by: Can Peng <pengcan@kylinos.cn>
---
 drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
index 5f41890bd134..455d03220b72 100644
--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
@@ -706,7 +706,7 @@ static int __maybe_unused fsl_samsung_hdmi_phy_suspend(struct device *dev)
 {
 	struct fsl_samsung_hdmi_phy *phy = dev_get_drvdata(dev);
 
-	clk_disable_unprepare(phy->apbclk);
+	clk_disable(phy->apbclk);
 
 	return 0;
 }
@@ -716,7 +716,7 @@ static int __maybe_unused fsl_samsung_hdmi_phy_resume(struct device *dev)
 	struct fsl_samsung_hdmi_phy *phy = dev_get_drvdata(dev);
 	int ret = 0;
 
-	ret = clk_prepare_enable(phy->apbclk);
+	ret = clk_enable(phy->apbclk);
 	if (ret) {
 		dev_err(phy->dev, "failed to enable apbclk\n");
 		return ret;
-- 
2.53.0


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

[PATCH v2 4/4] phy: freescale: fsl-samsung-hdmi: Disable APB clock on resume failure

[Can Peng]

fsl_samsung_hdmi_phy_resume() enables apbclk and then reprograms the PHY
when a previous clock configuration is available. If the hardware
configuration fails, the function returns the error with apbclk still
enabled.

Disable apbclk on the failure path so the clock enable count remains
balanced.

Fixes: 6ad082bee902 ("phy: freescale: add Samsung HDMI PHY")
Cc: stable@vger.kernel.org
Signed-off-by: Can Peng <pengcan@kylinos.cn>
---
 drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
index 455d03220b72..bb1aa1977065 100644
--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
@@ -722,8 +722,11 @@ static int __maybe_unused fsl_samsung_hdmi_phy_resume(struct device *dev)
 		return ret;
 	}
 
-	if (phy->cur_cfg)
+	if (phy->cur_cfg) {
 		ret = fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
+		if (ret)
+			clk_disable(phy->apbclk);
+	}
 
 	return ret;
 
-- 
2.53.0


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

Re: [PATCH v2 4/4] phy: freescale: fsl-samsung-hdmi: Disable APB clock on resume failure

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The clock `set_rate` callback directly accesses hardware registers without ensuring the PHY device is runtime active, which could cause a synchronous external abort when called while the device is suspended.
- [High] The driver mutates a static global variable to store dynamically calculated, instance-specific PHY configurations, causing a race condition in multi-instance or concurrent scenarios.
- [Medium] If `pm_runtime_resume_and_get()` fails in `fsl_samsung_hdmi_phy_remove()`, devres cleanup will underflow the APB clock enable count.
--

--- Patch [4]: [PATCH v2 4/4] phy: freescale: fsl-samsung-hdmi: Disable APB clock on resume failure ---
commit f185b639eb37a8417c686059fd4e7a2399306055
Author: Can Peng <pengcan@kylinos.cn>

phy: freescale: fsl-samsung-hdmi: Disable APB clock on resume failure

This commit modifies the fsl_samsung_hdmi_phy_resume() function to disable
the apbclk on the failure path. This ensures the clock enable count remains
balanced if the hardware configuration fails after the clock was enabled.

Cc: stable@vger.kernel.org

> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> index 455d03220b72c..bb1aa19770650 100644
> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> @@ -722,8 +722,11 @@ static int __maybe_unused fsl_samsung_hdmi_phy_resume(struct device *dev)
>  		return ret;
>  	}
>  
> -	if (phy->cur_cfg)
> +	if (phy->cur_cfg) {
>  		ret = fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);

[Severity: High]
This is a pre-existing issue, but does reading phy->cur_cfg here race
with concurrent clock rate updates?

In fsl_samsung_hdmi_phy_find_settings(), the driver dynamically calculates
settings and writes them into a shared static global variable without
locking:

static struct phy_config calculated_phy_pll_cfg = { ... };

fsl_samsung_hdmi_phy_find_settings()
    ...
    fsl_samsung_hdmi_calculate_phy(&calculated_phy_pll_cfg, ...);

If a concurrent call to clk_round_rate() updates calculated_phy_pll_cfg
while this resume callback is reading from it via phy->cur_cfg, could it
write partially updated or invalid PLL values to the hardware, resulting in
corrupted output or a display lockup?

> +		if (ret)
> +			clk_disable(phy->apbclk);
> +	}
>  
>  	return ret;

[Severity: Critical]
This is a pre-existing issue, but does the clock set_rate callback risk a
synchronous external abort if called while the device is suspended?

The clk_set_rate callback directly accesses hardware registers without
calling pm_runtime_resume_and_get():

fsl_samsung_hdmi_phy_clk_set_rate()
    ...
    return fsl_samsung_hdmi_phy_configure(phy, target_settings);

fsl_samsung_hdmi_phy_configure()
    ...
    writeb(REG33_FIX_DA, phy->regs + PHY_REG(33));

Because the clock framework does not automatically resume provider devices
during clk_set_rate(), if a consumer calls clk_set_rate() while the PHY is
runtime suspended, would writing to these unclocked APB registers cause a
system crash?

[Severity: Medium]
This is a pre-existing issue, but does failing to resume in remove()
cause a clock enable count underflow?

If pm_runtime_resume_and_get() fails (for example, due to a PLL lock
timeout), the device remains suspended with an enable count of 0:

fsl_samsung_hdmi_phy_remove()
    ...
    ret = pm_runtime_resume_and_get(&pdev->dev);
    if (ret < 0)
        dev_warn(&pdev->dev, "failed to resume on remove: %d\n", ret);

When remove() returns, devres cleanup unconditionally calls
clk_disable_unprepare(). Since the driver mixes devm_clk_get_enabled()
with manual clk_enable() and clk_disable() PM runtime toggling, wouldn't
this invoke clk_disable() on a clock that is already at 0, triggering a
warning in the clock core?

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

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