linux-kernel - Re: [PATCH v2] clk: st: clkgen-pll: Add cleaup in clkgen_c32_pll

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aWjeyNfrfY5dQLg3@redhat.com>
Date: Thu, 15 Jan 2026 07:34:16 -0500
From: Brian Masney <bmasney@...hat.com>
To: Haoxiang Li <lihaoxiang@...c.iscas.ac.cn>
Cc: mturquette@...libre.com, sboyd@...nel.org, linux-clk@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] clk: st: clkgen-pll: Add cleaup in
 clkgen_c32_pll_setup()

Hi Haoxiang,

Thanks for the patch!

On Thu, Jan 15, 2026 at 12:44:39PM +0800, Haoxiang Li wrote:
> In clkgen_c32_pll_setup(), there exists several leaks if errors ouccers.
> Add iounmap() to free the memory allocated by clkgen_get_register_base().
> Add clk_unregister() and kfree to do the cleaup for clkgen_pll_register().
> Add a while to do the cleaup for clkgen_odf_register().
> Use distinct variable names for two register calls' return values.
> 
> Signed-off-by: Haoxiang Li <lihaoxiang@...c.iscas.ac.cn>

There's a lot going on in this patch, and it's hard for someone else to
review. Can you break this up into several patches? For example:

- The kfree(pll_name) should be in it's own patch, with it's own
  explanation.

- It would help if the rename of 'clk' to 'pll_clk' was in it's own
  patch. Along with the rename of 'clk' to 'odf_clk' in a separate
  patch. You can say in these two commit messages that the renames
  are in preparation for cleaning up some memory leaks.

I'll pick up below with more suggestions.

> ---
> Changes in v2:
> - Add several cleanups. Thanks, Brian!
> - Modify the changelog.
> ---
>  drivers/clk/st/clkgen-pll.c | 44 +++++++++++++++++++++++++------------
>  1 file changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/clk/st/clkgen-pll.c b/drivers/clk/st/clkgen-pll.c
> index c258ff87a171..100172f9fdf8 100644
> --- a/drivers/clk/st/clkgen-pll.c
> +++ b/drivers/clk/st/clkgen-pll.c
> @@ -752,11 +752,10 @@ static struct clk * __init clkgen_odf_register(const char *parent_name,
>  	return clk;
>  }
>  
> -
>  static void __init clkgen_c32_pll_setup(struct device_node *np,
>  		struct clkgen_pll_data_clks *datac)
>  {
> -	struct clk *clk;
> +	struct clk *pll_clk;
>  	const char *parent_name, *pll_name;
>  	void __iomem *pll_base;
>  	int num_odfs, odf;
> @@ -774,18 +773,18 @@ static void __init clkgen_c32_pll_setup(struct device_node *np,
>  
>  	of_clk_detect_critical(np, 0, &pll_flags);
>  
> -	clk = clkgen_pll_register(parent_name, datac->data, pll_base, pll_flags,
> +	pll_clk = clkgen_pll_register(parent_name, datac->data, pll_base, pll_flags,
>  				  np->name, datac->data->lock);
> -	if (IS_ERR(clk))
> -		return;
> +	if (IS_ERR(pll_clk))
> +		goto err_unmap;
>  
> -	pll_name = __clk_get_name(clk);
> +	pll_name = __clk_get_name(pll_clk);
>  
>  	num_odfs = datac->data->num_odfs;
>  
>  	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
>  	if (!clk_data)
> -		return;
> +		goto err_pll_unregister;

This could also be it's own cleanup patch.

>  
>  	clk_data->clk_num = num_odfs;
>  	clk_data->clks = kcalloc(clk_data->clk_num, sizeof(struct clk *),
> @@ -795,7 +794,7 @@ static void __init clkgen_c32_pll_setup(struct device_node *np,
>  		goto err;
>  
>  	for (odf = 0; odf < num_odfs; odf++) {
> -		struct clk *clk;
> +		struct clk *odf_clk;
>  		const char *clk_name;
>  		unsigned long odf_flags = 0;
>  
> @@ -806,28 +805,45 @@ static void __init clkgen_c32_pll_setup(struct device_node *np,
>  			if (of_property_read_string_index(np,
>  							  "clock-output-names",
>  							  odf, &clk_name))
> -				return;
> +				goto err;
>  
>  			of_clk_detect_critical(np, odf, &odf_flags);
>  		}
>  
> -		clk = clkgen_odf_register(pll_name, pll_base, datac->data,
> +		odf_clk = clkgen_odf_register(pll_name, pll_base, datac->data,
>  				odf_flags, odf, &clkgena_c32_odf_lock,
>  				clk_name);
> -		if (IS_ERR(clk))
> -			goto err;
> +		if (IS_ERR(odf_clk))
> +			goto err_odf_unregister;
>  
> -		clk_data->clks[odf] = clk;
> +		clk_data->clks[odf] = odf_clk;
>  	}
>  
>  	of_clk_add_provider(np, of_clk_src_onecell_get, clk_data);
>  	return;
>  
> +err_odf_unregister:
> +	while (odf--) {

odf is not initialized at the top of the function. It would be good to
default it to 0. I know it is in the for loop, but just to be safe.

> +		struct clk_gate *gate = to_clk_gate(__clk_get_hw(clk_data->clks[odf]));
> +		struct clk_divider *div = to_clk_divider(__clk_get_hw(clk_data->clks[odf]));
> +
> +		clk_unregister_composite(clk_data->clks[odf]);
> +		kfree(div);
> +		kfree(gate);
> +	}
>  err:
> -	kfree(pll_name);
>  	kfree(clk_data->clks);
>  	kfree(clk_data);
> +err_pll_unregister:
> +	struct clkgen_pll *pll = to_clkgen_pll(__clk_get_hw(pll_clk));

Generally declarations go at the top of the function, or the top of an
if / while.

Since you are making changes to the top of the function, it would be
good to add another patch to put the variables in reverse Christmas tree
order (longest to shortest by name).

> +
> +       clk_unregister(pll_clk);

The clk_unregister() can also be it's own cleanup patch.

> +       kfree(pll);
> +err_unmap:
> +       if (pll_base)
> +               iounmap(pll_base);

The iounmap() can also be in it's own patch like you had in v1.

You may end up with 6-8 patches, but each one will be small, boring, and
it will make it easier for others review.

Thanks!

Brian