Re: [PATCH 1/1] phy: freescale: fsl-samsung-hdmi: initialize default rate

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The clk_set_rate callback directly accesses PHY hardware registers without ensuring that PM runtime is active (apbclk) and the reference clock (refclk) is enabled.
- [High] Unsynchronized concurrent execution of fsl_samsung_hdmi_phy_configure() corrupts hardware state.
--

commit ca3b78db8bc5347322f2f22c6db7f47c219232b3
Author: Aristo Chen <aristo.chen@canonical.com>

    phy: freescale: fsl-samsung-hdmi: initialize default rate
    
    The HDMI PHY clock reports 74.25 MHz when cur_cfg is unset, but does
    not establish that rate in hardware. The common clock framework caches
    the reported rate when registering the clock and skips a first
    clk_set_rate(74250000) request because the rate appears unchanged.

> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> index 0f25d81de61bb..41addd4ad4149 100644
> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
[ ... ]
> @@ -710,11 +727,23 @@ static int __maybe_unused fsl_samsung_hdmi_phy_resume(struct device *dev)
>  		return ret;
>  	}
>  
> -	if (phy->cur_cfg)
> -		ret = fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
> +	if (!phy->cur_cfg)
> +		return 0;
>  
> -	return ret;
> +	ret = clk_prepare_enable(phy->refclk);
> +	if (ret) {
> +		dev_err(phy->dev, "failed to enable ref clk\n");
> +		goto disable_apbclk;
> +	}
> +
> +	ret = fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);

[Severity: High]
This isn't a bug introduced by this patch, but does the clk_set_rate callback
directly access PHY hardware registers without ensuring that PM runtime is
active (apbclk) and the reference clock (refclk) is enabled?

The clock framework invokes fsl_samsung_hdmi_phy_clk_set_rate() when a
consumer (like the imx8mp LCDIF driver) configures the pixel clock rate during
a display modeset:

drivers/phy/freescale/phy-fsl-samsung-hdmi.c:fsl_samsung_hdmi_phy_clk_set_rate() {
    ...
    return fsl_samsung_hdmi_phy_configure(phy, target_settings);
}

Because this driver does not implement a .prepare callback to manage its PM
runtime state, the PHY device can be auto-suspended (and its apbclk disabled)
before the modeset occurs.

Could this lead to a system crash (synchronous external abort) due to
accessing memory-mapped registers while the APB clock is disabled, or a
guaranteed PLL lock timeout because the reference clock is disabled?

[Severity: High]
This is a pre-existing issue, but can unsynchronized concurrent execution of
fsl_samsung_hdmi_phy_configure() corrupt hardware state?

fsl_samsung_hdmi_phy_configure() modifies shared state (phy->cur_cfg) and
writes to multiple registers sequentially. It is called here from the PM
runtime resume callback, and it is also called from the CCF clk_set_rate
callback:

drivers/phy/freescale/phy-fsl-samsung-hdmi.c:fsl_samsung_hdmi_phy_clk_set_rate() {
    ...
    return fsl_samsung_hdmi_phy_configure(phy, target_settings);
}

Since clk_set_rate does not hold a PM runtime reference, it can run
concurrently with a PM resume triggered by sysfs or another driver.

Could this race between a display modeset and a PM runtime resume request lead
to interleaved hardware register writes, undefined PHY hardware state, and
display corruption?

Fixing the PM runtime management in clk_set_rate would also inherently
serialize these paths.

> +	clk_disable_unprepare(phy->refclk);
> +	if (!ret)
> +		return 0;
>  
> +disable_apbclk:
> +	clk_disable_unprepare(phy->apbclk);
> +	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903054811.9720-1-aristo.chen@canonical.com?part=1

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