Re: [PATCH] phy: freescale: fsl-samsung-hdmi: Balance runtime PM operations

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Double disable of the `apbclk` clock during device removal if the device is runtime suspended, leading to an enable count underflow.
- [High] The clock provider is incorrectly registered with a `struct clk *` instead of a `struct clk_hw *`, leading to type confusion when consumers fetch the clock.
- [High] The clock `set_rate` callback accesses hardware registers without ensuring the PHY is powered on via runtime PM.
- [High] Calling `clk_round_rate()` corrupts the active PHY configuration, leading to an incorrect hardware state on the next resume.
- [Medium] Clock enable count leak in the runtime resume error path if hardware configuration fails.
--

--- Patch [1]: [PATCH] phy: freescale: fsl-samsung-hdmi: Balance runtime PM operations ---
commit b4a831e48597d7a10cd9677a3f27a806afe1d380
Author: Fabio Estevam <festevam@nabladev.com>

phy: freescale: fsl-samsung-hdmi: Balance runtime PM operations

fsl_samsung_hdmi_phy_probe() enables runtime PM, but the remove callback does
not disable it, resulting in unbalanced pm_runtime_enable warnings. This
commit disables runtime PM after removing the clock provider, and undoes the
runtime PM operations when phy_clk_register() fails.

> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> @@ -679,12 +679,15 @@ static int fsl_samsung_hdmi_phy_probe(struct platform_device *pdev)
>  	return 0;
>  
>  register_clk_failed:
> +	pm_runtime_disable(phy->dev);
> +	pm_runtime_put_noidle(phy->dev);
>  	return ret;
>  }
>  
>  static void fsl_samsung_hdmi_phy_remove(struct platform_device *pdev)
>  {
>  	of_clk_del_provider(pdev->dev.of_node);
> +	pm_runtime_disable(&pdev->dev);

[Severity: High]
This is a pre-existing issue, but does disabling runtime PM here without first
resuming the device lead to a double disable of the apbclk?

The apbclk is initialized via devm_clk_get_enabled() in
fsl_samsung_hdmi_phy_probe():

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

This means the driver core will automatically call clk_disable_unprepare() on
driver removal.

If the device happens to be idle and runtime suspended, the driver will have
already manually disabled this clock in fsl_samsung_hdmi_phy_suspend().
Calling pm_runtime_disable() here freezes the PM state as suspended.

When the devres cleanup action invokes clk_disable_unprepare() again on the
already disabled clock, could this underflow the clock enable count and
generate a core warning?

>  }

[ ... ]

While reviewing this file, I also noticed a few other pre-existing issues:

[Severity: High]
This is a pre-existing issue, but does the clock registration pass the wrong
pointer type to the hardware provider?

In drivers/phy/freescale/phy-fsl-samsung-hdmi.c:phy_clk_register():

	ret = of_clk_add_hw_provider(np, of_clk_hw_simple_get, phyclk);

The phy_clk_register() function passes the struct clk *phyclk as the private
data. However, the driver specifies of_clk_hw_simple_get() as the provider
callback. This callback strictly assumes the private data is a struct clk_hw *
and simply returns it.

Will consumers be given a struct clk * when the clock framework expects a
struct clk_hw *, potentially leading to undefined behavior or kernel panics
when they attempt to use the returned pointer?


[Severity: High]
This is a pre-existing issue, but does the set_rate callback access hardware
registers without ensuring the PHY is powered on?

In drivers/phy/freescale/phy-fsl-samsung-hdmi.c:fsl_samsung_hdmi_phy_clk_set_rate():

	return fsl_samsung_hdmi_phy_configure(phy, target_settings);

This triggers immediate MMIO writes to phy->regs inside
fsl_samsung_hdmi_phy_configure(). Because this PHY acts as a clock provider,
the common clock framework can invoke the set_rate callback regardless of
whether the clock is prepared or enabled.

If the PHY device is runtime suspended and its apbclk is gated off, could
writing to the hardware in this state trigger a synchronous external abort?

Would it be safer to call pm_runtime_resume_and_get() to ensure the hardware
is accessible before modifying the configuration?


[Severity: Medium]
This is a pre-existing issue, but is there a clock enable count leak if
hardware configuration fails during a runtime resume?

In drivers/phy/freescale/phy-fsl-samsung-hdmi.c:fsl_samsung_hdmi_phy_resume():

	ret = fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
	if (ret)
		return ret;

At the top of this function, the apbclk is successfully enabled via
clk_prepare_enable(phy->apbclk). If fsl_samsung_hdmi_phy_configure() returns
an error (for example, if the PLL fails to lock), the function propagates the
error without disabling the clock.

Since the PM core treats the failed resume as leaving the device suspended,
the matching suspend callback will not run later. Will this permanently
increment the clock's enable count on each failed resume, eventually
preventing it from being disabled system-wide?


[Severity: High]
This is a pre-existing issue, but does speculative rate determination corrupt
the active PHY configuration?

In drivers/phy/freescale/phy-fsl-samsung-hdmi.c:fsl_samsung_hdmi_phy_find_settings():

	fsl_samsung_hdmi_calculate_phy(&calculated_phy_pll_cfg, int_div_clk, p, m, s);

This mutates the global static configuration structure calculated_phy_pll_cfg
during speculative rate determination. When a rate is successfully set,
phy->cur_cfg can point directly to this global variable.

If a consumer (such as a display controller testing different resolutions)
later calls clk_round_rate(), it overwrites the contents of the global
calculated_phy_pll_cfg. Because phy->cur_cfg references the same memory, the
active configuration is silently modified.

When the PHY runtime resumes and flushes the configuration:

	ret = fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);

Will the PHY program itself with these uncommitted test settings instead of
the previously committed rate, potentially breaking HDMI output?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729192830.487150-1-festevam@gmail.com?part=1

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