Re: [PATCH v6 17/27] clk: mediatek: pllfh: clear state data in mtk_clk_cleanup_pllfhs

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Variables `pll_id` and `ssc_rate` are used uninitialized if device tree property reads fail.
--

commit d96970ce40bb3dd0da1358d2ea7d97e33c5495ae
Author: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

clk: mediatek: pllfh: clear state data in mtk_clk_cleanup_pllfhs

Currently, fhctl_parse_dt function sets some state variables that are
never cleared in the unregister nominal path or register error one.
As they are stored in a static array variable, given as parameter from
the caller function, they retain their values until they are initialized
again by another fhctl_parse_dt call.

> diff --git a/drivers/clk/mediatek/clk-pllfh.c b/drivers/clk/mediatek/clk-pllfh.c
> index 398edad2a69e6..ae56d4ad66dc8 100644
> --- a/drivers/clk/mediatek/clk-pllfh.c
> +++ b/drivers/clk/mediatek/clk-pllfh.c
> @@ -244,6 +244,14 @@ static void mtk_clk_cleanup_pllfhs(void __iomem *iomem_base,
>  		clk_data->hws[pll->id] = ERR_PTR(-ENOENT);
>  	}
>  
> +	for (i = 0; i < num_fhs; i++) {
> +		struct mtk_pllfh_data *pllfh = &pllfhs[i];
> +
> +		pllfh->state.fh_enable = 0;
> +		pllfh->state.ssc_rate = 0;

[Severity: High]
This isn't a bug introduced by this patch, but while we are clearing this
state data here, there appears to be an initialization issue in
fhctl_parse_dt() where pll_id and ssc_rate are populated:

drivers/clk/mediatek/clk-pllfh.c:fhctl_parse_dt() {
    ...
		of_property_read_u32_index(node, "clocks", offset + 1, &pll_id);
		of_property_read_u32_index(node,
					   "mediatek,hopping-ssc-percent",
					   i, &ssc_rate);

		pllfh = get_pllfh_by_id(pllfhs, num_fhs, pll_id);
    ...
}

Are these variables used uninitialized if the device tree property reads
fail?

If the clocks or mediatek,hopping-ssc-percent properties are missing or
malformed in the device tree, of_property_read_u32_index() will fail
silently and leave pll_id and ssc_rate unmodified.

On the first iteration, this could result in stack garbage being used to
look up a PLL ID and set its spread-spectrum clocking rate. On subsequent
iterations, the variables might incorrectly reuse values from the previous
clock, applying wrong configurations to the hardware.

Should we be checking the return values of of_property_read_u32_index()
to prevent unexpected PLL state configurations?

> +		pllfh->state.base = NULL;
> +	}
> +
>  	if (fhctl_base)
>  		iounmap(fhctl_base);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-mt8189-clocks-system-base-v6-0-5df247f56938@collabora.com?part=17