[PATCH v2 0/7] clk: st: clkgen-pll: Add cleanup in clkgen_c32_pll_setup() and clkgen_odf_register()

[PATCH v2 0/7] clk: st: clkgen-pll: Add cleanup in clkgen_c32_pll_setup() and clkgen_odf_register()

[Haoxiang Li]

Patch 1 fixes a memory leak in clkgen_odf_register()
Patches 2 to 7 improve the cleanup process for function errors:
  - release the memory allocated by clkgen_get_register_base()
  - release the memory allocated by clkgen_pll_register()
  - release the memory allocated by clkgen_odf_register()
and removed unnecessary handling.

These works are carried out under the guidance of Brian Masney.
Thanks, Brian!

Changes in v2:
- Rename the patch title for Patch 2 and 3
- Add a Fixes tag in Patch 6

Haoxiang Li (7):
  clk: st: clkgen-pll: Fix a memory leak in clkgen_odf_register()
  clk: st: clkgen-pll: Rename clk variable in clkgen_c32_pll_setup()
  clk: st: clkgen-pll: Rename clk variable in clkgen_c32_pll_setup()
  clk: st: clkgen-pll: Add iounmap() in clkgen_c32_pll_setup()
  clk: st: clkgen-pll: Add clk_unregister for pll_clk in
    clkgen_c32_pll_setup()
  clk: st: clkgen-pll: Remove redundant kfree() in
    clkgen_c32_pll_setup()
  clk: st: clkgen-pll: Add clk_unregister for odf_clk in
    clkgen_c32_pll_setup()

 drivers/clk/st/clkgen-pll.c | 53 +++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 17 deletions(-)

-- 
2.25.1

[PATCH v2 1/7] clk: st: clkgen-pll: Fix a memory leak in clkgen_odf_register()

[Haoxiang Li]

If clk_register_composite() fails, call kfree() to release
div and gate.

Fixes: b9b8e614b580 ("clk: st: Support for PLLs inside ClockGenA(s)")
Cc: stable@vger.kernel.org
Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
Reviewed-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/st/clkgen-pll.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/st/clkgen-pll.c b/drivers/clk/st/clkgen-pll.c
index c258ff87a171..a7d605b52cf3 100644
--- a/drivers/clk/st/clkgen-pll.c
+++ b/drivers/clk/st/clkgen-pll.c
@@ -742,8 +742,11 @@ static struct clk * __init clkgen_odf_register(const char *parent_name,
 				     &div->hw, &clk_divider_ops,
 				     &gate->hw, &clk_gate_ops,
 				     flags);
-	if (IS_ERR(clk))
+	if (IS_ERR(clk)) {
+		kfree(div);
+		kfree(gate);
 		return clk;
+	}
 
 	pr_debug("%s: parent %s rate %lu\n",
 			__clk_get_name(clk),
-- 
2.25.1

Re: [PATCH v2 1/7] clk: st: clkgen-pll: Fix a memory leak in clkgen_odf_register()

[Markus Elfring]

> If clk_register_composite() fails, call kfree() to release
> div and gate.

You may put such information also into a single text line.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.19-rc6#n659

How do you think about to avoid a bit of duplicate source code here?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.19-rc6#n526

Regards,
Markus

[PATCH v2 2/7] clk: st: clkgen-pll: Rename clk variable in clkgen_c32_pll_setup()

[Haoxiang Li]

In clkgen_c32_pll_setup(), `clk` is used for two distint variables' name.
This patch rename the preceding `clk` to `pll_clk`.
The rename is in preparation for cleaning up some memory leaks.

Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
Reviewed-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/st/clkgen-pll.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/st/clkgen-pll.c b/drivers/clk/st/clkgen-pll.c
index a7d605b52cf3..24b9b66a83e1 100644
--- a/drivers/clk/st/clkgen-pll.c
+++ b/drivers/clk/st/clkgen-pll.c
@@ -755,11 +755,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;
@@ -777,12 +776,12 @@ 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))
+	if (IS_ERR(pll_clk))
 		return;
 
-	pll_name = __clk_get_name(clk);
+	pll_name = __clk_get_name(pll_clk);
 
 	num_odfs = datac->data->num_odfs;
 
@@ -831,6 +830,7 @@ static void __init clkgen_c32_pll_setup(struct device_node *np,
 	kfree(clk_data->clks);
 	kfree(clk_data);
 }
+
 static void __init clkgen_c32_pll0_setup(struct device_node *np)
 {
 	clkgen_c32_pll_setup(np,
-- 
2.25.1

Re: [PATCH v2 2/7] clk: st: clkgen-pll: Rename clk variable in clkgen_c32_pll_setup()

[Markus Elfring]

> In clkgen_c32_pll_setup(), `clk` is used for two distint variables' name.
> This patch rename …

* Did anything hinder you to avoid typos in such a change description?

* See also once more:
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.19-rc6#n94


Regards,
Markus

[PATCH v2 3/7] clk: st: clkgen-pll: Rename clk variable in clkgen_c32_pll_setup()

[Haoxiang Li]

In clkgen_c32_pll_setup(), `clk` is used for two distint variables' name.
This patch rename the later `clk` to `odf_clk`.
The rename is in preparation for cleaning up some memory leaks.

Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
---
 drivers/clk/st/clkgen-pll.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/st/clkgen-pll.c b/drivers/clk/st/clkgen-pll.c
index 24b9b66a83e1..566eb7e8a73c 100644
--- a/drivers/clk/st/clkgen-pll.c
+++ b/drivers/clk/st/clkgen-pll.c
@@ -797,7 +797,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;
 
@@ -813,13 +813,13 @@ static void __init clkgen_c32_pll_setup(struct device_node *np,
 			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))
+		if (IS_ERR(odf_clk))
 			goto err;
 
-		clk_data->clks[odf] = clk;
+		clk_data->clks[odf] = odf_clk;
 	}
 
 	of_clk_add_provider(np, of_clk_src_onecell_get, clk_data);
-- 
2.25.1

Re: [PATCH v2 3/7] clk: st: clkgen-pll: Rename clk variable in clkgen_c32_pll_setup()

[Markus Elfring]

> In clkgen_c32_pll_setup(), `clk` is used for two distint variables' name.
> This patch rename …

* Did anything hinder you to avoid typos in such a change description?

* See also once more:
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.19-rc6#n94


Regards,
Markus

Re: [PATCH v2 3/7] clk: st: clkgen-pll: Rename clk variable in clkgen_c32_pll_setup()

[Brian Masney]

On Wed, Jan 21, 2026 at 09:22:19AM +0800, Haoxiang Li wrote:
> In clkgen_c32_pll_setup(), `clk` is used for two distint variables' name.
> This patch rename the later `clk` to `odf_clk`.
> The rename is in preparation for cleaning up some memory leaks.
> 
> Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>

Reviewed-by: Brian Masney <bmasney@redhat.com>

[PATCH v2 4/7] clk: st: clkgen-pll: Add iounmap() in clkgen_c32_pll_setup()

[Haoxiang Li]

Add a iounmap() to release the memory allocated by
clkgen_get_register_base() in error path.

Fixes: b9b8e614b580 ("clk: st: Support for PLLs inside ClockGenA(s)")
Cc: stable@vger.kernel.org
Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
Reviewed-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/st/clkgen-pll.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/st/clkgen-pll.c b/drivers/clk/st/clkgen-pll.c
index 566eb7e8a73c..073180754a56 100644
--- a/drivers/clk/st/clkgen-pll.c
+++ b/drivers/clk/st/clkgen-pll.c
@@ -779,7 +779,7 @@ static void __init clkgen_c32_pll_setup(struct device_node *np,
 	pll_clk = clkgen_pll_register(parent_name, datac->data, pll_base, pll_flags,
 				  np->name, datac->data->lock);
 	if (IS_ERR(pll_clk))
-		return;
+		goto err_unmap;
 
 	pll_name = __clk_get_name(pll_clk);
 
@@ -829,6 +829,9 @@ static void __init clkgen_c32_pll_setup(struct device_node *np,
 	kfree(pll_name);
 	kfree(clk_data->clks);
 	kfree(clk_data);
+err_unmap:
+	if (pll_base)
+		iounmap(pll_base);
 }
 
 static void __init clkgen_c32_pll0_setup(struct device_node *np)
-- 
2.25.1

Re: [PATCH v2 4/7] clk: st: clkgen-pll: Add iounmap() in clkgen_c32_pll_setup()

[Markus Elfring]

> Add a iounmap() to release the memory allocated by

      an          call?


> clkgen_get_register_base() in error path.

Was such an improvable implementation detail detected by any known source code
analysis approaches?


…
> +++ b/drivers/clk/st/clkgen-pll.c
…
> @@ -829,6 +829,9 @@ static void __init clkgen_c32_pll_setup(struct device_node *np,
>  	kfree(pll_name);
>  	kfree(clk_data->clks);
>  	kfree(clk_data);
> +err_unmap:
> +	if (pll_base)
> +		iounmap(pll_base);

* I find this pointer check redundant because of a previous variable check.
  https://elixir.bootlin.com/linux/v6.19-rc5/source/drivers/clk/st/clkgen-pll.c#L771-L773

* I suggest to refine the goto chain a bit more.


>  }
>  
>  static void __init clkgen_c32_pll0_setup(struct device_node *np)

Regards,
Markus

[PATCH v2 5/7] clk: st: clkgen-pll: Add clk_unregister for pll_clk in clkgen_c32_pll_setup()

[Haoxiang Li]

In clkgen_c32_pll_setup(), clkgen_pll_register() allocated a
clkgen_pll memory and registered a clk. Add clk_unregister()
and kfree() to release the memory if error occurs.

Fixes: b9b8e614b580 ("clk: st: Support for PLLs inside ClockGenA(s)")
Cc: stable@vger.kernel.org
Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
Reviewed-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/st/clkgen-pll.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/st/clkgen-pll.c b/drivers/clk/st/clkgen-pll.c
index 073180754a56..ddf7673a52c5 100644
--- a/drivers/clk/st/clkgen-pll.c
+++ b/drivers/clk/st/clkgen-pll.c
@@ -764,7 +764,7 @@ static void __init clkgen_c32_pll_setup(struct device_node *np,
 	int num_odfs, odf;
 	struct clk_onecell_data *clk_data;
 	unsigned long pll_flags = 0;
-
+	struct clkgen_pll *pll;
 
 	parent_name = of_clk_get_parent_name(np, 0);
 	if (!parent_name)
@@ -787,7 +787,7 @@ static void __init clkgen_c32_pll_setup(struct device_node *np,
 
 	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
 	if (!clk_data)
-		return;
+		goto err_pll_unregister;
 
 	clk_data->clk_num = num_odfs;
 	clk_data->clks = kcalloc(clk_data->clk_num, sizeof(struct clk *),
@@ -829,6 +829,10 @@ static void __init clkgen_c32_pll_setup(struct device_node *np,
 	kfree(pll_name);
 	kfree(clk_data->clks);
 	kfree(clk_data);
+err_pll_unregister:
+	pll = to_clkgen_pll(__clk_get_hw(pll_clk));
+	clk_unregister(pll_clk);
+	kfree(pll);
 err_unmap:
 	if (pll_base)
 		iounmap(pll_base);
-- 
2.25.1

[PATCH v2 6/7] clk: st: clkgen-pll: Remove redundant kfree() in clkgen_c32_pll_setup()

[Haoxiang Li]

In clkgen_c32_pll_setup(), pll_name is actually pll_clk -> core->name.
This variable will be released in clk_unregister() -> __clk_release()
if some error occurs. Thus kfree(pll_name) will cause a double free.
Remove this redundant kfree().

Fixes: b9b8e614b580 ("clk: st: Support for PLLs inside ClockGenA(s)")
Cc: stable@vger.kernel.org
Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
Reviewed-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/st/clkgen-pll.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/clk/st/clkgen-pll.c b/drivers/clk/st/clkgen-pll.c
index ddf7673a52c5..b8d2b97a6b9b 100644
--- a/drivers/clk/st/clkgen-pll.c
+++ b/drivers/clk/st/clkgen-pll.c
@@ -826,7 +826,6 @@ static void __init clkgen_c32_pll_setup(struct device_node *np,
 	return;
 
 err:
-	kfree(pll_name);
 	kfree(clk_data->clks);
 	kfree(clk_data);
 err_pll_unregister:
-- 
2.25.1

[PATCH v2 7/7] clk: st: clkgen-pll: Add clk_unregister for odf_clk in clkgen_c32_pll_setup()

[Haoxiang Li]

In clkgen_c32_pll_setup(), clkgen_odf_register() allocated
clk_gate and clk_divider memory and registered a clk. Add
clk_unregister() and kfree() to release the memory if
error occurs. Initialize odf to zero for safe.

Fixes: b9b8e614b580 ("clk: st: Support for PLLs inside ClockGenA(s)")
Cc: stable@vger.kernel.org
Signed-off-by: Haoxiang Li <lihaoxiang@isrc.iscas.ac.cn>
Reviewed-by: Brian Masney <bmasney@redhat.com>
---
 drivers/clk/st/clkgen-pll.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/st/clkgen-pll.c b/drivers/clk/st/clkgen-pll.c
index b8d2b97a6b9b..3fc0af4b77c6 100644
--- a/drivers/clk/st/clkgen-pll.c
+++ b/drivers/clk/st/clkgen-pll.c
@@ -761,10 +761,12 @@ static void __init clkgen_c32_pll_setup(struct device_node *np,
 	struct clk *pll_clk;
 	const char *parent_name, *pll_name;
 	void __iomem *pll_base;
-	int num_odfs, odf;
+	int num_odfs, odf = 0;
 	struct clk_onecell_data *clk_data;
 	unsigned long pll_flags = 0;
 	struct clkgen_pll *pll;
+	struct clk_gate *gate;
+	struct clk_divider *div;
 
 	parent_name = of_clk_get_parent_name(np, 0);
 	if (!parent_name)
@@ -808,7 +810,7 @@ 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_odf_unregister;
 
 			of_clk_detect_critical(np, odf, &odf_flags);
 		}
@@ -817,7 +819,7 @@ static void __init clkgen_c32_pll_setup(struct device_node *np,
 				odf_flags, odf, &clkgena_c32_odf_lock,
 				clk_name);
 		if (IS_ERR(odf_clk))
-			goto err;
+			goto err_odf_unregister;
 
 		clk_data->clks[odf] = odf_clk;
 	}
@@ -825,6 +827,14 @@ static void __init clkgen_c32_pll_setup(struct device_node *np,
 	of_clk_add_provider(np, of_clk_src_onecell_get, clk_data);
 	return;
 
+err_odf_unregister:
+	while (--odf >= 0) {
+		gate = to_clk_gate(__clk_get_hw(clk_data->clks[odf]));
+		div = to_clk_divider(__clk_get_hw(clk_data->clks[odf]));
+		clk_unregister_composite(clk_data->clks[odf]);
+		kfree(div);
+		kfree(gate);
+	}
 err:
 	kfree(clk_data->clks);
 	kfree(clk_data);
-- 
2.25.1

Re: [PATCH v2 7/7] clk: st: clkgen-pll: Add clk_unregister for odf_clk in clkgen_c32_pll_setup()

[Markus Elfring]

> In clkgen_c32_pll_setup(), clkgen_odf_register() allocated
> clk_gate and clk_divider memory and registered a clk. Add
> clk_unregister() and kfree() to release the memory if

Would an other word wrapping become more desirable for an improved change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.19-rc6#n659


> error occurs. Initialize odf to zero for safe.

                                                data processing?

Regards,
Markus