Re: [PATCH v2 17/18] clk: mediatek: Add MT8189 scp clock support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incorrect `sta_ofs` in `scp_cg_regs` causes clock state tracking failures.

Pre-existing issues:
- [High] Missing `of_clk_del_provider()` in the error path of `__mtk_clk_simple_probe()` leads to a dangling clock provider and subsequent Use-After-Free.
- [Medium] Memory leak of the `struct mtk_clk_gate` allocation in `mtk_clk_register_gate()`.
--

--- Patch [17]: [PATCH v2 17/18] clk: mediatek: Add MT8189 scp clock support ---
commit 622c7d611c5de5d2297fc568f0bb3702456cb016
Author: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

clk: mediatek: Add MT8189 scp clock support

This commit adds support for the MT8189 scp clock controller. It provides
clock gate control for the System Control Processor by defining the necessary
gate clocks and registering them with the common clock framework.

> diff --git a/drivers/clk/mediatek/clk-mt8189-scp.c b/drivers/clk/mediatek/clk-mt8189-scp.c
> index 0000000000000..75197cd98b521
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8189-scp.c
> @@ -0,0 +1,77 @@
[ ... ]
> +static const struct mtk_gate_regs scp_cg_regs = {
> +	.set_ofs = 0x4,
> +	.clr_ofs = 0x8,
> +	.sta_ofs = 0x4,
> +};

[Severity: High]
Is this sta_ofs correct? Most other MediaTek clock drivers with differing set
and clr registers use 0x0 for the sta register. If reading from offset 0x4
returns garbage or write-only trigger values, could this break the common clock
framework's is_enabled checks and cause incorrect reference counting?

[ ... ]


This is a pre-existing issue, but there appears to be a dangling clock provider
in the error path of __mtk_clk_simple_probe():

drivers/clk/mediatek/clk-mtk.c:__mtk_clk_simple_probe() {
    ...
	r = of_clk_add_hw_provider(node, of_clk_hw_onecell_get, clk_data);
	if (r)
		goto unregister_clks;

	platform_set_drvdata(pdev, clk_data);

	if (mcd->rst_desc) {
		r = mtk_register_reset_controller_with_dev(&pdev->dev,
							   mcd->rst_desc);
		if (r)
			goto unregister_clks;
    ...
unregister_clks:
	if (mcd->clks)
		mtk_clk_unregister_gates(mcd->clks, mcd->num_clks, clk_data);
    ...
}

[Severity: High]
If mtk_register_reset_controller_with_dev() fails, we jump to unregister_clks,
which unregisters the clocks and frees clk_data. Does this leave the device
node registered as a clock provider pointing to freed memory, since
of_clk_del_provider(node) is never called in this error path? Could this lead
to a use-after-free when another driver attempts to get a clock?


This is another pre-existing issue, but there appears to be a memory leak in
mtk_clk_register_gate():

drivers/clk/mediatek/clk-gate.c:mtk_clk_register_gate() {
    ...
	cg = kzalloc_obj(*cg);
	if (!cg)
		return ERR_PTR(-ENOMEM);

	init.name = gate->name;
	init.flags = gate->flags | CLK_SET_RATE_PARENT;
	init.parent_names = gate->parent_name ? &gate->parent_name : NULL;
	init.num_parents = gate->parent_name ? 1 : 0;
	init.ops = gate->ops;
	if (mtk_cg_uses_hwv(init.ops) && !regmap_hwv)
		return dev_err_ptr_probe(
			dev, -ENXIO,
			"regmap not found for hardware voter clocks\n");
    ...
}

[Severity: Medium]
If a clock gate requires a hardware voter regmap but none was found, the
function early-returns an error. Are we leaking the newly allocated cg 
structure here by returning directly without freeing it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-mt8189-clocks-system-base-v2-0-2926da3db6cf@collabora.com?part=17