Re: [PATCH v6 18/27] clk: mediatek: Add MT8189 apmixedsys clock support
From: sashiko-bot@kernel.org
Date: 2026-09-04 16:17:50
Also in:
linux-clk
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: Pre-existing issues: - [High] Uninitialized variables cause potential hardware corruption due to ignored return values. - [High] Memory leak of the fhctl I/O mapping on early probe failure. -- commit 899c77c67da66015b28a154ffd13a738ef18913e Author: Louis-Alexis Eyraud [off-list ref] clk: mediatek: Add MT8189 apmixedsys clock support Add support for the MT8189 apmixedsys clock controller, which provides PLLs generated from SoC 26m.
quoted hunk ↗ jump to hunk
diff --git a/drivers/clk/mediatek/clk-mt8189-apmixedsys.c b/drivers/clk/mediatek/clk-mt8189-apmixedsys.c new file mode 100644 index 0000000000000..7399a444ec4d4 --- /dev/null +++ b/drivers/clk/mediatek/clk-mt8189-apmixedsys.c
[ ... ]
+static int clk_mt8189_apmixed_probe(struct platform_device *pdev)
+{
+ const u8 *fhctl_node = "mediatek,mt8189-fhctl";
+ struct device_node *node = pdev->dev.of_node;
+ struct clk_hw_onecell_data *clk_data;
+ int r;
+
+ clk_data = mtk_alloc_clk_data(ARRAY_SIZE(apmixed_plls));
+ if (!clk_data)
+ return -ENOMEM;
+
+ r = mtk_clk_register_pllfhs(&pdev->dev, fhctl_node, apmixed_plls,
+ ARRAY_SIZE(apmixed_plls), pllfhs,
+ ARRAY_SIZE(pllfhs), clk_data);
[Severity: High]
This is a pre-existing issue, but does this call expose a potential memory
leak of the fhctl I/O mapping on early probe failure?
If of_iomap() fails in mtk_clk_register_pllfhs(), it returns -EINVAL and
leaks the fhctl_base mapped earlier in fhctl_parse_dt():
drivers/clk/mediatek/clk-pllfh.c:mtk_clk_register_pllfhs() {
fhctl_parse_dt(fhctl_node, pllfhs, num_fhs);
base = of_iomap(dev->of_node, 0);
if (!base) {
pr_err("%s(): ioremap failed\n", __func__);
return -EINVAL;
}
...
}
Similarly, if the loop fails on the very first iteration, it branches to the
error path and calls mtk_clk_cleanup_pllfhs() with a loop counter of 0.
Because the cleanup loop relies on the loop counter to discover fhctl_base
from the state array, the loop is skipped, fhctl_base remains NULL, and the
reference is overwritten without calling iounmap().
Is there a way to ensure this mapping is correctly cleaned up if probe
fails during testing or device bind/unbind?
[Severity: High]
This is another pre-existing issue exposed by this call, but does
fhctl_parse_dt() ignore the return values of of_property_read_u32_index(),
leading to uninitialized stack variables being passed to get_pllfh_by_id()?
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);
...
}
If the device tree is missing the 'clocks' or 'mediatek,hopping-ssc-percent'
properties, of_property_read_u32_index() returns an error without initializing
pll_id and ssc_rate. These uninitialized stack variables are then used to look
up a PLL and configure its SSC rate, which could lead to hardware state
corruption.
Could we add error checking for these device tree reads?
+ if (r) + goto free_apmixed_data; + + r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data); + if (r) + goto unregister_plls; + + platform_set_drvdata(pdev, clk_data); + + return 0; + +unregister_plls: + mtk_clk_unregister_pllfhs(apmixed_plls, ARRAY_SIZE(apmixed_plls), pllfhs, + ARRAY_SIZE(pllfhs), clk_data); +free_apmixed_data: + mtk_free_clk_data(clk_data); + return r; +}
-- Sashiko AI review · https://sashiko.dev/#/patchset/20260904-mt8189-clocks-system-base-v6-0-5df247f56938@collabora.com?part=18