Re: [PATCH v4 10/24] clk: mediatek: pll: Add BAR reset register offsets

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The newly added set and clear registers for the BAR reset (`rst_bar_set_addr`, `rst_bar_clr_addr`) are computed but never used in the prepare/unprepare callbacks.

Pre-existing issues:
- [High] The error handling rollback in `mtk_clk_register_plls()` blindly unregisters and frees clocks it previously skipped, leading to Use-After-Free and heap corruption.
--

--- Patch [10]: [PATCH v4 10/24] clk: mediatek: pll: Add BAR reset register offsets ---
commit 32dac933eac13088132963fe76e93c2f164b2862
Author: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

clk: mediatek: pll: Add BAR reset register offsets

Currently, the register offset to handle BAR reset, for PLLs that
support it, is hardcoded to CON0. On MT8189 SoC, there are separate
status, set and clr registers to handle this, so differents offsets
are needed.

> diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
> index ee478bc640851..4b0f7e2bfc5a5 100644
> --- a/drivers/clk/mediatek/clk-pll.c
> +++ b/drivers/clk/mediatek/clk-pll.c
> @@ -249,9 +249,9 @@ int mtk_pll_prepare(struct clk_hw *hw)
>  	udelay(20);
>  
>  	if (pll->data->flags & HAVE_RST_BAR) {
> -		r = readl(pll->base_addr + REG_CON0);
> +		r = readl(pll->rst_bar_addr);
>  		r |= pll->data->rst_bar_mask;
> -		writel(r, pll->base_addr + REG_CON0);
> +		writel(r, pll->rst_bar_addr);

[Severity: Medium]
Will this fail to correctly assert the hardware reset BAR on the MT8189 SoC?

The newly added rst_bar_set_addr is computed in mtk_clk_register_pll_ops(),
but it is never used here. Is it intended to write to the dedicated set
register when it exists, rather than performing a read-modify-write on
rst_bar_addr?

>  	}
>  
>  	return 0;
> @@ -263,9 +263,9 @@ void mtk_pll_unprepare(struct clk_hw *hw)
>  	u32 r;
>  
>  	if (pll->data->flags & HAVE_RST_BAR) {
> -		r = readl(pll->base_addr + REG_CON0);
> +		r = readl(pll->rst_bar_addr);
>  		r &= ~pll->data->rst_bar_mask;
> -		writel(r, pll->base_addr + REG_CON0);
> +		writel(r, pll->rst_bar_addr);

[Severity: Medium]
Similar to mtk_pll_prepare(), should this be writing to the newly added
rst_bar_clr_addr instead of read-modify-writing rst_bar_addr?

>  	}
>  
>  	__mtk_pll_tuner_disable(pll);

[ ... ]

> int mtk_clk_register_plls(struct device *dev,
> 			  const struct mtk_pll_data *plls, int num_plls,
> 			  struct clk_hw_onecell_data *clk_data)
> {

[ ... ]

> 	for (i = 0; i < num_plls; i++) {
> 		const struct mtk_pll_data *pll = &plls[i];
> 
> 		if (!IS_ERR_OR_NULL(clk_data->hws[pll->id])) {
> 			pr_warn("%pOF: Trying to register duplicate clock ID: %d\n",
> 				dev->of_node, pll->id);
> 			continue;
> 		}

[ ... ]

> 	return 0;
> 
> err:
> 	while (--i >= 0) {
> 		const struct mtk_pll_data *pll = &plls[i];
> 
> 		mtk_clk_unregister_pll(clk_data->hws[pll->id]);

[Severity: High]
This isn't a bug introduced by this patch, but does this error handling
rollback cause a use-after-free and memory corruption?

If the registration loop encounters a duplicate clock ID and skips it using
continue, a subsequent failure in the array will trigger this rollback loop.

The rollback loop unconditionally unregisters clocks for all indices less than
the failing index i.  This will cause kfree() to be called on a clock
object that was skipped by this function and passed in from elsewhere,
destroying an actively used resource.

Should the rollback loop verify that the clock was actually registered by
this function before unregistering it?

> 		clk_data->hws[pll->id] = ERR_PTR(-ENOENT);
> 	}
> 
> 	iounmap(base);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-mt8189-clocks-system-base-v4-0-e356b813a64c@collabora.com?part=10