[PATCH] phy: freescale: fsl-samsung-hdmi: Fix runtime PM cleanup

[PATCH] 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

Re: [PATCH] phy: freescale: fsl-samsung-hdmi: Fix runtime PM cleanup

[sashiko-bot]

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

Pre-existing issues:
- [High] Double-disable of `apbclk` on driver unbind causing a clock enable count underflow.
- [Medium] Hardware registers are accessed in `clk_set_rate` while the device may be runtime suspended, causing a bus error.
- [Medium] Clock enable count leak when hardware configuration fails during runtime resume.
--

--- Patch [1]: [PATCH] phy: freescale: fsl-samsung-hdmi: Fix runtime PM cleanup ---
commit c64d89c2a04d8191edbe1e45c553f7627c043e56
Author: Can Peng <pengcan@kylinos.cn>

phy: freescale: fsl-samsung-hdmi: Fix runtime PM cleanup

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.

Fixes: 6ad082bee902 ("phy: freescale: add Samsung HDMI PHY")
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 d010fec15671d..e7af4f9c7b20b 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;

[Severity: High]
This isn't a bug introduced by this patch, but doesn't this driver double-disable
apbclk on unbind if the device is runtime suspended?

In fsl_samsung_hdmi_phy_probe(), apbclk is acquired via devm_clk_get_enabled(),
which registers a devres action to disable it on teardown:

	phy->apbclk = devm_clk_get_enabled(phy->dev, "apb");

However, the driver explicitly disables this clock during runtime suspend.
When the driver is unbound, fsl_samsung_hdmi_phy_remove() completes without
calling pm_runtime_get_sync() to resume the device.

If the device was already in runtime suspend, the clock is disabled, and then
devres executes and disables it a second time, causing a clock enable count
underflow warning.

Could the remove callback be updated to ensure the device is resumed?

> +
> +	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;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but can the clock framework trigger
a bus error by accessing hardware registers while the device is runtime suspended?

The clock framework calls fsl_samsung_hdmi_phy_clk_set_rate(), which calls
fsl_samsung_hdmi_phy_configure():

drivers/phy/freescale/phy-fsl-samsung-hdmi.c:fsl_samsung_hdmi_phy_configure() {
	...
	writeb(REG33_FIX_DA, phy->regs + PHY_REG(33));
	...
}

The driver does not set the CLK_SET_RATE_GATE flag, so a consumer can set the
rate while the clock is unprepared.

Since there are no PM runtime calls in the clock operations, the device might
be runtime suspended (and apbclk disabled) when this happens, causing a
synchronous external abort when accessing PHY registers.

Should the clock operations ensure the device is resumed before accessing registers?

[Severity: Medium]
This is a pre-existing issue, but does fsl_samsung_hdmi_phy_resume() leak the
clock enable count if hardware configuration fails?

During fsl_samsung_hdmi_phy_resume(), clk_prepare_enable() is called on apbclk.
If the subsequent call to fsl_samsung_hdmi_phy_configure() fails, it returns
the error without disabling the clock:

drivers/phy/freescale/phy-fsl-samsung-hdmi.c:fsl_samsung_hdmi_phy_resume() {
	...
	ret = clk_prepare_enable(phy->apbclk);
	...
		ret = fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);

	return ret;
}

Should this error path unwind the clock enable?

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

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