Re: [PATCH 1/2] phy: freescale: fsl-samsung-hdmi: Make sure clk_init_data is fully initialized

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

Pre-existing issues:
- [High] A global static variable is mutated during clock rate queries, causing data races and state corruption with PM resume operations.
--

--- Patch [1]: [PATCH 1/2] phy: freescale: fsl-samsung-hdmi: Make sure clk_init_data is fully initialized ---
commit eace776c1e0b617b09367e8f7566adff67211a9f
Author: Geert Uytterhoeven <[email protected]>

phy: freescale: fsl-samsung-hdmi: Make sure clk_init_data is fully initialized

The clk_init_data structure contains several mutually-exclusive members
for different methods to specify the possible parents of a clock,
prompting drivers to initialize only the members they need.  However,
not initializing all members may cause subtle issues, which are only
exposed when CONFIG_INIT_STACK_ALL_PATTERN or CONFIG_INIT_STACK_NONE is
enabled.

> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> index d010fec15671d..76c0d571e3155 100644
> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> @@ -610,7 +610,7 @@ static int phy_clk_register(struct fsl_samsung_hdmi_phy *phy)
>  {
>  	struct device *dev = phy->dev;
>  	struct device_node *np = dev->of_node;
> -	struct clk_init_data init;
> +	struct clk_init_data init = {};
>  	const char *parent_name;
>  	struct clk *phyclk;
>  	int ret;

[Severity: High]
This is a pre-existing issue, but does mutating the global static variable
calculated_phy_pll_cfg during side-effect-free clock queries cause state
corruption?

In fsl_samsung_hdmi_phy_find_settings(), the global variable is modified
unconditionally:

drivers/phy/freescale/phy-fsl-samsung-hdmi.c:fsl_samsung_hdmi_phy_find_settings() {
    ...
	/* Calculate the integer divider */
	int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate, &p, &m, &s);
	fsl_samsung_hdmi_calculate_phy(&calculated_phy_pll_cfg, int_div_clk, p, m, s);
    ...
}

Then in fsl_samsung_hdmi_phy_configure(), a pointer to this global is saved:

drivers/phy/freescale/phy-fsl-samsung-hdmi.c:fsl_samsung_hdmi_phy_configure() {
    ...
	phy->cur_cfg = cfg;
    ...
}

If a clock driver sets a rate (saving the pointer) and later performs a
clk_round_rate() query, the query will overwrite calculated_phy_pll_cfg.

When the device resumes, it restores the hardware using this potentially
corrupted pointer:

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

Can this lead to the hardware being restored to an incorrect frequency
upon PM resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy