Re: [PATCH v6 18/27] clk: mediatek: Add MT8189 apmixedsys clock support

[sashiko-bot@kernel.org]

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 <louisalexis.eyraud@collabora.com>

clk: mediatek: Add MT8189 apmixedsys clock support

Add support for the MT8189 apmixedsys clock controller, which provides
PLLs generated from SoC 26m.

> 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