Re: [PATCH v6 17/27] clk: mediatek: pllfh: clear state data in mtk_clk_cleanup_pllfhs
From: sashiko-bot@kernel.org
Date: 2026-09-04 16:18:31
Also in:
linux-clk
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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