Re: [PATCH] clk: mediatek: pllfh: Fix IO remapping leak in register_pllfhs error path

[Brian Masney <bmasney@redhat.com>]

Hi Louis-Alexis,

Thanks for the patch.

On Wed, Jul 08, 2026 at 11:18:59AM +0200, Louis-Alexis Eyraud wrote:
> When mtk_clk_register_pllfhs function fails to register a PLL, it
> unregisters all PLLs and cleans up itself in its error path before
> returning, so the function callers don't need to do it.
> 
> But contrary to mtk_clk_unregister_pllfhs function, that does almost
> the same sequence, it does not free the IO memory mapped on fhctl node,
> leading to a leak.
> 
> Fix this leak by factorizing the cleanup sequence in a new private
> function and use it both mtk_clk_register_pllfhs and
> mtk_clk_unregister_pllfhs functions.
> 
> Also, change the loop index start value to avoid the -1 operation on
> index at each loop.
> 
> Fixes: d7964de8a8ea ("clk: mediatek: Add new clock driver to handle FHCTL hardware")
> Signed-off-by: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>
> ---
> This patch aims to fix a issue, detected by Sashiko
> during another series review ([1]), regarding the use of 
> mtk_clk_register_pllfhs function.
> 
> The patch is based on linux-next tree (tag: next-20260707).
> 
> [1]: https://sashiko.dev/#/patchset/20260701-mt8189-clocks-system-base-v1-0-2b048feea50a%40collabora.com?part=6
> ---
>  drivers/clk/mediatek/clk-pllfh.c | 87 ++++++++++++++++++----------------------
>  1 file changed, 39 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/clk/mediatek/clk-pllfh.c b/drivers/clk/mediatek/clk-pllfh.c
> index aa95cd9197b3..7463aa35d7e3 100644
> --- a/drivers/clk/mediatek/clk-pllfh.c
> +++ b/drivers/clk/mediatek/clk-pllfh.c
> @@ -197,6 +197,43 @@ static void mtk_clk_unregister_pllfh(struct clk_hw *hw)
>  	kfree(fh);
>  }
>  
> +static void mtk_clk_cleanup_pllfhs(const struct mtk_pll_data *plls, int num_plls,
> +				   struct mtk_pllfh_data *pllfhs, int num_fhs,
> +				   struct clk_hw_onecell_data *clk_data)
> +{
> +	void __iomem *base = NULL, *fhctl_base = NULL;
> +	int i;
> +
> +	for (i = num_plls - 1; i > 0; i--) {

It looks like this should be i >= 0

> +		const struct mtk_pll_data *pll = &plls[i];
> +		struct mtk_pllfh_data *pllfh;
> +		bool use_fhctl;
> +
> +		if (IS_ERR_OR_NULL(clk_data->hws[pll->id]))
> +			continue;
> +
> +		pllfh = get_pllfh_by_id(pllfhs, num_fhs, pll->id);
> +		use_fhctl = fhctl_is_supported_and_enabled(pllfh);
> +
> +		if (use_fhctl) {
> +			fhctl_base = pllfh->state.base;
> +			mtk_clk_unregister_pllfh(clk_data->hws[pll->id]);
> +		} else {
> +			base = mtk_clk_pll_get_base(clk_data->hws[pll->id],
> +						    pll);
> +			mtk_clk_unregister_pll(clk_data->hws[pll->id]);
> +		}
> +
> +		clk_data->hws[pll->id] = ERR_PTR(-ENOENT);
> +	}
> +
> +	if (fhctl_base)
> +		iounmap(fhctl_base);
> +
> +	iounmap(base);

When use_fhctl is enabled for all PLLs, base will be NULL.

Brian

> +}
> +
> +
>  int mtk_clk_register_pllfhs(struct device *dev,
>  			    const struct mtk_pll_data *plls, int num_plls,
>  			    struct mtk_pllfh_data *pllfhs, int num_fhs,
> @@ -238,24 +275,7 @@ int mtk_clk_register_pllfhs(struct device *dev,
>  	return 0;
>  
>  err:
> -	while (--i >= 0) {
> -		const struct mtk_pll_data *pll = &plls[i];
> -		struct mtk_pllfh_data *pllfh;
> -		bool use_fhctl;
> -
> -		pllfh = get_pllfh_by_id(pllfhs, num_fhs, pll->id);
> -		use_fhctl = fhctl_is_supported_and_enabled(pllfh);
> -
> -		if (use_fhctl)
> -			mtk_clk_unregister_pllfh(clk_data->hws[pll->id]);
> -		else
> -			mtk_clk_unregister_pll(clk_data->hws[pll->id]);
> -
> -		clk_data->hws[pll->id] = ERR_PTR(-ENOENT);
> -	}
> -
> -	iounmap(base);
> -
> +	mtk_clk_cleanup_pllfhs(plls, i, pllfhs, num_fhs, clk_data);
>  	return PTR_ERR(hw);
>  }
>  EXPORT_SYMBOL_GPL(mtk_clk_register_pllfhs);
> @@ -264,38 +284,9 @@ void mtk_clk_unregister_pllfhs(const struct mtk_pll_data *plls, int num_plls,
>  			       struct mtk_pllfh_data *pllfhs, int num_fhs,
>  			       struct clk_hw_onecell_data *clk_data)
>  {
> -	void __iomem *base = NULL, *fhctl_base = NULL;
> -	int i;
> -
>  	if (!clk_data)
>  		return;
>  
> -	for (i = num_plls; i > 0; i--) {
> -		const struct mtk_pll_data *pll = &plls[i - 1];
> -		struct mtk_pllfh_data *pllfh;
> -		bool use_fhctl;
> -
> -		if (IS_ERR_OR_NULL(clk_data->hws[pll->id]))
> -			continue;
> -
> -		pllfh = get_pllfh_by_id(pllfhs, num_fhs, pll->id);
> -		use_fhctl = fhctl_is_supported_and_enabled(pllfh);
> -
> -		if (use_fhctl) {
> -			fhctl_base = pllfh->state.base;
> -			mtk_clk_unregister_pllfh(clk_data->hws[pll->id]);
> -		} else {
> -			base = mtk_clk_pll_get_base(clk_data->hws[pll->id],
> -						    pll);
> -			mtk_clk_unregister_pll(clk_data->hws[pll->id]);
> -		}
> -
> -		clk_data->hws[pll->id] = ERR_PTR(-ENOENT);
> -	}
> -
> -	if (fhctl_base)
> -		iounmap(fhctl_base);
> -
> -	iounmap(base);
> +	mtk_clk_cleanup_pllfhs(plls, num_plls, pllfhs, num_fhs, clk_data);
>  }
>  EXPORT_SYMBOL_GPL(mtk_clk_unregister_pllfhs);
> 
> ---
> base-commit: b3a87fbe738f19cbe19411cf749f3d8c09c04de4
> change-id: 20260708-clk-mediatek-pllfh-fix-7e9ca35989e2
> 
> Best regards,
> -- 
> Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>
>