Re: [PATCH v3 10/18] clk: mediatek: Add MT8189 topckgen clock support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Teardown asymmetry between manually managed clocks and the devm-managed clock notifier leads to a Use-After-Free of the clock struct on probe failure or driver removal.
- [Medium] Integer post-dividers (e.g., _d16, _d4_d8) are incorrectly defined with arbitrary fractional factors (e.g., 61/973, 92/1473, 43/1375) instead of exact integer division ratios (1/16, 1/32).
--

--- Patch [10]: [PATCH v3 10/18] clk: mediatek: Add MT8189 topckgen clock support ---
commit c5b259302a4ef0f8df16da71e98a2848bcf8992d
Author: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>
Subject: clk: mediatek: Add MT8189 topckgen clock support

Add support for the MT8189 topckgen clock controller, which provides
muxes and dividers for clock selection in other IP blocks.

> diff --git a/drivers/clk/mediatek/clk-mt8189-topckgen.c b/drivers/clk/mediatek/clk-mt8189-topckgen.c
> new file mode 100644
> index 0000000000000..2473b617e9d62
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8189-topckgen.c
> @@ -0,0 +1,1025 @@
[ ... ]
> +static const struct mtk_fixed_factor top_divs[] = {
> +	FACTOR(CLK_TOP_MAINPLL_D3, "mainpll_d3", "mainpll", 1, 3),
> +	FACTOR(CLK_TOP_MAINPLL_D4, "mainpll_d4", "mainpll", 1, 4),
> +	FACTOR(CLK_TOP_MAINPLL_D4_D2, "mainpll_d4_d2", "mainpll", 1, 8),
> +	FACTOR(CLK_TOP_MAINPLL_D4_D4, "mainpll_d4_d4", "mainpll", 1, 16),
> +	FACTOR(CLK_TOP_MAINPLL_D4_D8, "mainpll_d4_d8", "mainpll", 43, 1375),

[Severity: Medium]
Does this intentionally use an arbitrary fractional factor (43/1375) instead
of the standard integer post-divider ratio (1/32)?

While this might arrive at a specific exact frequency for a static parent rate,
could this break dynamic frequency scaling? If the mainpll frequency changes,
applying this fraction instead of an exact 1/32 digital division ratio could
cause the clock framework to calculate incorrect output rates.

I also notice this pattern on other dividers below, such as
CLK_TOP_TVDPLL1_D16 (92/1473) and CLK_TOP_OSC_D16 (61/973).

> +	FACTOR(CLK_TOP_MAINPLL_D5, "mainpll_d5", "mainpll", 1, 5),
[ ... ]
> +/* Register mux notifier for MFG mux */
> +static int clk_mt8189_reg_mfg_mux_notifier(struct device *dev,
> +					   struct clk *clk)
> +{
> +	struct mtk_mux_nb *mfg_mux_nb;
> +
> +	mfg_mux_nb = devm_kzalloc(dev, sizeof(*mfg_mux_nb), GFP_KERNEL);
> +	if (!mfg_mux_nb)
> +		return -ENOMEM;
> +
> +	mfg_mux_nb->ops = &mtk_mux_clr_set_upd_ops;
> +	mfg_mux_nb->bypass_index = 0; /* Bypass to CLK_TOP_MFG_REF_SEL */
> +
> +	return devm_mtk_clk_mux_notifier_register(dev, clk, mfg_mux_nb);

[Severity: High]
Can this teardown asymmetry cause a use-after-free of the clk struct on
probe failure or driver removal?

This registers the notifier using devm_mtk_clk_mux_notifier_register(), which
queues up a devres cleanup action to automatically call
clk_notifier_unregister(). 

However, if probe fails or the driver is removed, mtk_clk_simple_probe()
and mtk_clk_simple_remove() explicitly unregister the clocks. This calls
mtk_clk_unregister_muxes() -> clk_hw_unregister() -> clk_unregister(),
which frees the struct clk pointer.

By the time the devres actions are executed and clk_notifier_unregister() is
called, the struct clk pointer is already freed, leading to a dereference of
the freed pointer when modifying its notifier_count.

> +}
> +
> +static const struct mtk_clk_desc topck_desc = {
> +	.factor_clks = top_divs,
> +	.num_factor_clks = ARRAY_SIZE(top_divs),
> +	.mux_clks = top_muxes,
> +	.num_mux_clks = ARRAY_SIZE(top_muxes),
> +	.composite_clks = top_composites,
> +	.num_composite_clks = ARRAY_SIZE(top_composites),
> +	.clks = top_clks,
> +	.num_clks = ARRAY_SIZE(top_clks),
> +	.clk_notifier_func = clk_mt8189_reg_mfg_mux_notifier,
> +	.mfg_clk_idx = CLK_TOP_MFG_SEL_MFGPLL,
> +	.clk_lock = &mt8189_clk_lock,
> +};
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-mt8189-clocks-system-base-v3-0-8e6d99ab3ad2@collabora.com?part=10