[PATCH 0/4] clk: mvebu: Fix some error handling paths + do some clean-up

[PATCH 0/4] clk: mvebu: Fix some error handling paths + do some clean-up

[Christophe JAILLET]

This serie fixes some (unlikely) error handlings paths.

The 4th patch is completely speculative. When I compile-tested the changes,
I had to remove this line in order for it to compile.
As it works fine (at least for me) without it, I wonder if it is needed at all.


Also, I wonder if the drivers in drivers/clk/mvebu are used by anyone.
In order to compile-test the changes, I also had to change the 'bool' in Kconfig
by 'bool "blah"'. Without this change, it was not possible to set
CONFIG_MVEBU_CLK_CPU required by Makefile.

I don't know if I did something wrong, if it is an issue only on my environment
or if something got broken at some time in the build chain but it looks
spurious.

If I'm right and that these drivers never compile and no-one noticed it,
maybe removing them is better than fixing some unlikely issues and style.
If these drivers should stay, Kconfig may need some love from someone.

Christophe JAILLET (4):
  clk: mvebu: Fix a memory leak in an error handling path
  clk: mvebu: Fix a another memory leak in an error handling path
  clk: mvebu: Fix a checkpatch warning
  clk: mvebu: Remove an unneeded include

 drivers/clk/mvebu/clk-cpu.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

-- 
2.27.0

[PATCH 1/4] clk: mvebu: Fix a memory leak in an error handling path

[Christophe JAILLET]

If an error occurs in the for_each loop, clk_name must be freed.

In order to do so, sightly rearrange the code:
   - move the allocation to simplify error handling
   - use kasprintf instead of kzalloc/sprintf to simplify code and avoid a
     magic number

Fixes: ab8ba01b3fe5 ("clk: mvebu: add armada-370-xp CPU specific clocks")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
The { } around the 1 line block after kasprintf is intentional and makes
sense with 2/2
---
 drivers/clk/mvebu/clk-cpu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
index c2af3395cf13..a11d7273fcc7 100644
--- a/drivers/clk/mvebu/clk-cpu.c
+++ b/drivers/clk/mvebu/clk-cpu.c
@@ -195,17 +195,17 @@ static void __init of_cpu_clk_setup(struct device_node *node)
 	for_each_of_cpu_node(dn) {
 		struct clk_init_data init;
 		struct clk *clk;
-		char *clk_name = kzalloc(5, GFP_KERNEL);
+		char *clk_name;
 		int cpu, err;
 
-		if (WARN_ON(!clk_name))
-			goto bail_out;
-
 		err = of_property_read_u32(dn, "reg", &cpu);
 		if (WARN_ON(err))
 			goto bail_out;
 
-		sprintf(clk_name, "cpu%d", cpu);
+		clk_name = kasprintf(GFP_KERNEL, "cpu%d", cpu);
+		if (WARN_ON(!clk_name)) {
+			goto bail_out;
+		}
 
 		cpuclk[cpu].parent_name = of_clk_get_parent_name(node, 0);
 		cpuclk[cpu].clk_name = clk_name;
-- 
2.27.0

AW: [PATCH 1/4] clk: mvebu: Fix a memory leak in an error handling path

[Walter Harms]

nitpicking:
 clk_name could be replaced with cpuclk[cpu].clk_name

and the commit msg is from the other patch (free  cpuclk[cpu].clk_name)

jm2c,

re,
 wh
________________________________________
Von: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Gesendet: Freitag, 23. April 2021 08:25:01
An: mturquette@baylibre.com; sboyd@kernel.org; gregory.clement@bootlin.com; thomas.petazzoni@free-electrons.com
Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org; Christophe JAILLET
Betreff: [PATCH 1/4] clk: mvebu: Fix a memory leak in an error handling path

WARNUNG: Diese E-Mail kam von außerhalb der Organisation. Klicken Sie nicht auf Links oder öffnen Sie keine Anhänge, es sei denn, Sie kennen den/die Absender*in und wissen, dass der Inhalt sicher ist.


If an error occurs in the for_each loop, clk_name must be freed.

In order to do so, sightly rearrange the code:
   - move the allocation to simplify error handling
   - use kasprintf instead of kzalloc/sprintf to simplify code and avoid a
     magic number

Fixes: ab8ba01b3fe5 ("clk: mvebu: add armada-370-xp CPU specific clocks")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
The { } around the 1 line block after kasprintf is intentional and makes
sense with 2/2
---
 drivers/clk/mvebu/clk-cpu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
index c2af3395cf13..a11d7273fcc7 100644
--- a/drivers/clk/mvebu/clk-cpu.c
+++ b/drivers/clk/mvebu/clk-cpu.c
@@ -195,17 +195,17 @@ static void __init of_cpu_clk_setup(struct device_node *node)
        for_each_of_cpu_node(dn) {
                struct clk_init_data init;
                struct clk *clk;
-               char *clk_name = kzalloc(5, GFP_KERNEL);
+               char *clk_name;
                int cpu, err;

-               if (WARN_ON(!clk_name))
-                       goto bail_out;
-
                err = of_property_read_u32(dn, "reg", &cpu);
                if (WARN_ON(err))
                        goto bail_out;

-               sprintf(clk_name, "cpu%d", cpu);
+               clk_name = kasprintf(GFP_KERNEL, "cpu%d", cpu);
+               if (WARN_ON(!clk_name)) {
+                       goto bail_out;
+               }

                cpuclk[cpu].parent_name = of_clk_get_parent_name(node, 0);
                cpuclk[cpu].clk_name = clk_name;
--
2.27.0

AW: [PATCH 1/4] clk: mvebu: Fix a memory leak in an error handling path

[Christophe JAILLET]

Le 23/04/2021 à 13:42, Walter Harms a écrit :
> nitpicking:
>   clk_name could be replaced with cpuclk[cpu].clk_name

Agreed, Thx.
I'll wait a few days to see if there are other comments before sending a 
v2. (especially if 4/4 is correct or not)
I'll also add "clk-cpu:" after "clk: mvebu:"

> and the commit msg is from the other patch (free  cpuclk[cpu].clk_name)
> 

But here, I don't follow you.
What do you mean? Which other patch?

Do you mean that the commit message has to be updated accordingly?
(ie: s/clk_name/cpuclk[cpu].clk_name/ must be freed)


> jm2c,
> 
> re,
>   wh
> ________________________________________
> Von: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Gesendet: Freitag, 23. April 2021 08:25:01
> An: mturquette@baylibre.com; sboyd@kernel.org; gregory.clement@bootlin.com; thomas.petazzoni@free-electrons.com
> Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org; Christophe JAILLET
> Betreff: [PATCH 1/4] clk: mvebu: Fix a memory leak in an error handling path
> 
> WARNUNG: Diese E-Mail kam von außerhalb der Organisation. Klicken Sie nicht auf Links oder öffnen Sie keine Anhänge, es sei denn, Sie kennen den/die Absender*in und wissen, dass der Inhalt sicher ist.
> 
> 
> If an error occurs in the for_each loop, clk_name must be freed.
> 
> In order to do so, sightly rearrange the code:
>     - move the allocation to simplify error handling
>     - use kasprintf instead of kzalloc/sprintf to simplify code and avoid a
>       magic number
> 
> Fixes: ab8ba01b3fe5 ("clk: mvebu: add armada-370-xp CPU specific clocks")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> The { } around the 1 line block after kasprintf is intentional and makes
> sense with 2/2
> ---
>   drivers/clk/mvebu/clk-cpu.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
> index c2af3395cf13..a11d7273fcc7 100644
> --- a/drivers/clk/mvebu/clk-cpu.c
> +++ b/drivers/clk/mvebu/clk-cpu.c
> @@ -195,17 +195,17 @@ static void __init of_cpu_clk_setup(struct device_node *node)
>          for_each_of_cpu_node(dn) {
>                  struct clk_init_data init;
>                  struct clk *clk;
> -               char *clk_name = kzalloc(5, GFP_KERNEL);
> +               char *clk_name;
>                  int cpu, err;
> 
> -               if (WARN_ON(!clk_name))
> -                       goto bail_out;
> -
>                  err = of_property_read_u32(dn, "reg", &cpu);
>                  if (WARN_ON(err))
>                          goto bail_out;
> 
> -               sprintf(clk_name, "cpu%d", cpu);
> +               clk_name = kasprintf(GFP_KERNEL, "cpu%d", cpu);
> +               if (WARN_ON(!clk_name)) {
> +                       goto bail_out;
> +               }
> 
>                  cpuclk[cpu].parent_name = of_clk_get_parent_name(node, 0);
>                  cpuclk[cpu].clk_name = clk_name;
> --
> 2.27.0
> 
> 

AW: [PATCH 1/4] clk: mvebu: Fix a memory leak in an error handling path

[Walter Harms]

yep,
there was a patch containing while()/free(), i guessed the comment
was accidentaly copied (or do i mixup something ?)

forget about it, the comment was confusing me. 
IMHO it is the wrong way around.
I would say:

 sightly rearrange the code:
    - use kasprintf instead of kzalloc/sprintf to simplify code and avoid a
      magic number
If an error occurs in the for_each loop, clk_name must be freed.
 goto bail_out;
 
as side effect more that 9 clk_cpu are now correctly shown.

hope that helps,

re,
 wh
________________________________________
Von: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Gesendet: Freitag, 23. April 2021 14:02:17
An: Walter Harms; mturquette@baylibre.com; sboyd@kernel.org; gregory.clement@bootlin.com; thomas.petazzoni@free-electrons.com
Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org
Betreff: AW: [PATCH 1/4] clk: mvebu: Fix a memory leak in an error handling path

WARNUNG: Diese E-Mail kam von außerhalb der Organisation. Klicken Sie nicht auf Links oder öffnen Sie keine Anhänge, es sei denn, Sie kennen den/die Absender*in und wissen, dass der Inhalt sicher ist.


Le 23/04/2021 à 13:42, Walter Harms a écrit :
> nitpicking:
>   clk_name could be replaced with cpuclk[cpu].clk_name

Agreed, Thx.
I'll wait a few days to see if there are other comments before sending a
v2. (especially if 4/4 is correct or not)
I'll also add "clk-cpu:" after "clk: mvebu:"

> and the commit msg is from the other patch (free  cpuclk[cpu].clk_name)
>

But here, I don't follow you.
What do you mean? Which other patch?

Do you mean that the commit message has to be updated accordingly?
(ie: s/clk_name/cpuclk[cpu].clk_name/ must be freed)


> jm2c,
>
> re,
>   wh
> ________________________________________
> Von: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Gesendet: Freitag, 23. April 2021 08:25:01
> An: mturquette@baylibre.com; sboyd@kernel.org; gregory.clement@bootlin.com; thomas.petazzoni@free-electrons.com
> Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org; Christophe JAILLET
> Betreff: [PATCH 1/4] clk: mvebu: Fix a memory leak in an error handling path
>
> WARNUNG: Diese E-Mail kam von außerhalb der Organisation. Klicken Sie nicht auf Links oder öffnen Sie keine Anhänge, es sei denn, Sie kennen den/die Absender*in und wissen, dass der Inhalt sicher ist.
>
>
> If an error occurs in the for_each loop, clk_name must be freed.
>
> In order to do so, sightly rearrange the code:
>     - move the allocation to simplify error handling
>     - use kasprintf instead of kzalloc/sprintf to simplify code and avoid a
>       magic number
>
> Fixes: ab8ba01b3fe5 ("clk: mvebu: add armada-370-xp CPU specific clocks")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> The { } around the 1 line block after kasprintf is intentional and makes
> sense with 2/2
> ---
>   drivers/clk/mvebu/clk-cpu.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
> index c2af3395cf13..a11d7273fcc7 100644
> --- a/drivers/clk/mvebu/clk-cpu.c
> +++ b/drivers/clk/mvebu/clk-cpu.c
> @@ -195,17 +195,17 @@ static void __init of_cpu_clk_setup(struct device_node *node)
>          for_each_of_cpu_node(dn) {
>                  struct clk_init_data init;
>                  struct clk *clk;
> -               char *clk_name = kzalloc(5, GFP_KERNEL);
> +               char *clk_name;
>                  int cpu, err;
>
> -               if (WARN_ON(!clk_name))
> -                       goto bail_out;
> -
>                  err = of_property_read_u32(dn, "reg", &cpu);
>                  if (WARN_ON(err))
>                          goto bail_out;
>
> -               sprintf(clk_name, "cpu%d", cpu);
> +               clk_name = kasprintf(GFP_KERNEL, "cpu%d", cpu);
> +               if (WARN_ON(!clk_name)) {
> +                       goto bail_out;
> +               }
>
>                  cpuclk[cpu].parent_name = of_clk_get_parent_name(node, 0);
>                  cpuclk[cpu].clk_name = clk_name;
> --
> 2.27.0
>
>

[PATCH 2/4] clk: mvebu: Fix a another memory leak in an error handling path

[Christophe JAILLET]

If we exit the for_each_of_cpu_node loop early, the reference on the
current node must be decremented, otherwise there is a leak.

Fixes: ab8ba01b3fe5 ("clk: mvebu: add armada-370-xp CPU specific clocks")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/clk/mvebu/clk-cpu.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
index a11d7273fcc7..9a5c2aec6ec2 100644
--- a/drivers/clk/mvebu/clk-cpu.c
+++ b/drivers/clk/mvebu/clk-cpu.c
@@ -199,11 +199,14 @@ static void __init of_cpu_clk_setup(struct device_node *node)
 		int cpu, err;
 
 		err = of_property_read_u32(dn, "reg", &cpu);
-		if (WARN_ON(err))
+		if (WARN_ON(err)) {
+			of_node_put(dn);
 			goto bail_out;
+		}
 
 		clk_name = kasprintf(GFP_KERNEL, "cpu%d", cpu);
 		if (WARN_ON(!clk_name)) {
+			of_node_put(dn);
 			goto bail_out;
 		}
 
@@ -222,8 +225,10 @@ static void __init of_cpu_clk_setup(struct device_node *node)
 		init.num_parents = 1;
 
 		clk = clk_register(NULL, &cpuclk[cpu].hw);
-		if (WARN_ON(IS_ERR(clk)))
+		if (WARN_ON(IS_ERR(clk))) {
+			of_node_put(dn);
 			goto bail_out;
+		}
 		clks[cpu] = clk;
 	}
 	clk_data.clk_num = MAX_CPU;
-- 
2.27.0

[PATCH 3/4] clk: mvebu: Add a missing space

[Christophe JAILLET]

Fix a checkpatch warning.
Add a missing space.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/clk/mvebu/clk-cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
index 9a5c2aec6ec2..55a8486f665f 100644
--- a/drivers/clk/mvebu/clk-cpu.c
+++ b/drivers/clk/mvebu/clk-cpu.c
@@ -238,7 +238,7 @@ static void __init of_cpu_clk_setup(struct device_node *node)
 	return;
 bail_out:
 	kfree(clks);
-	while(ncpus--)
+	while (ncpus--)
 		kfree(cpuclk[ncpus].clk_name);
 clks_out:
 	kfree(cpuclk);
-- 
2.27.0

[PATCH 4/4] clk: mvebu: Remove an unneeded include

[Christophe JAILLET]

Compiling without <asm/smp_plat.h> just works fine, so remove it

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Completely speculative, but compilation works for me without this include
---
 drivers/clk/mvebu/clk-cpu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
index 55a8486f665f..4879458c768a 100644
--- a/drivers/clk/mvebu/clk-cpu.c
+++ b/drivers/clk/mvebu/clk-cpu.c
@@ -16,7 +16,6 @@
 #include <linux/of.h>
 #include <linux/delay.h>
 #include <linux/mvebu-pmsu.h>
-#include <asm/smp_plat.h>
 
 #define SYS_CTRL_CLK_DIVIDER_CTRL_OFFSET               0x0
 #define   SYS_CTRL_CLK_DIVIDER_CTRL_RESET_ALL          0xff
-- 
2.27.0

Re: [PATCH 0/4] clk: mvebu: Fix some error handling paths + do some clean-up

[Thomas Petazzoni]

Hello,

On Fri, 23 Apr 2021 08:24:52 +0200
Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:

> Also, I wonder if the drivers in drivers/clk/mvebu are used by anyone.
> In order to compile-test the changes, I also had to change the 'bool' in Kconfig
> by 'bool "blah"'. Without this change, it was not possible to set
> CONFIG_MVEBU_CLK_CPU required by Makefile.

CONFIG_MVEBU_CLK_CPU is selected by ARMADA_370_CLK and ARMADA_XP_CLK,
which themselves are selected by MACH_ARMADA_370 and MACH_ARMADA_XP
respectively.

So unless I'm missing something, this code is definitely reachable and
compiled. You can use the mvebu_v7_defconfig of ARM32, and the code
will be built.

Best regards,

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 0/4] clk: mvebu: Fix some error handling paths + do some clean-up

[Christophe JAILLET]

Le 23/04/2021 à 09:27, Thomas Petazzoni a écrit :
> Hello,
> 
> On Fri, 23 Apr 2021 08:24:52 +0200
> Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> 
>> Also, I wonder if the drivers in drivers/clk/mvebu are used by anyone.
>> In order to compile-test the changes, I also had to change the 'bool' in Kconfig
>> by 'bool "blah"'. Without this change, it was not possible to set
>> CONFIG_MVEBU_CLK_CPU required by Makefile.
> 
> CONFIG_MVEBU_CLK_CPU is selected by ARMADA_370_CLK and ARMADA_XP_CLK,
> which themselves are selected by MACH_ARMADA_370 and MACH_ARMADA_XP
> respectively.

Hi, thanks for the clarification.

Usually, only slightly modifying dependencies in Kconfig is enough for 
me to trigger a built, even if I don't have the correct toolchain for 
this architecture. In this case, the tweak I had to do was "spurious" 
and Kconfig "looked strange" to me, hence my comment.

Glad to here that this code is still alive. So, my patches may make 
sense :).

CJ

> 
> So unless I'm missing something, this code is definitely reachable and
> compiled. You can use the mvebu_v7_defconfig of ARM32, and the code
> will be built.
> 
> Best regards,
> 
> Thomas
> 

[PATCH 0/4] clk: mvebu: Fix some error handling paths + do some clean-up

[Christophe JAILLET]

This serie fixes some (unlikely) error handlings paths.

The 4th patch is completely speculative. When I compile-tested the changes,
I had to remove this line in order for it to compile.
As it works fine (at least for me) without it, I wonder if it is needed at all.


Also, I wonder if the drivers in drivers/clk/mvebu are used by anyone.
In order to compile-test the changes, I also had to change the 'bool' in Kconfig
by 'bool "blah"'. Without this change, it was not possible to set
CONFIG_MVEBU_CLK_CPU required by Makefile.

I don't know if I did something wrong, if it is an issue only on my environment
or if something got broken at some time in the build chain but it looks
spurious.

If I'm right and that these drivers never compile and no-one noticed it,
maybe removing them is better than fixing some unlikely issues and style.
If these drivers should stay, Kconfig may need some love from someone.

Christophe JAILLET (4):
  clk: mvebu: Fix a memory leak in an error handling path
  clk: mvebu: Fix a another memory leak in an error handling path
  clk: mvebu: Fix a checkpatch warning
  clk: mvebu: Remove an unneeded include

 drivers/clk/mvebu/clk-cpu.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

-- 
2.27.0

Re: [PATCH 0/4] clk: mvebu: Fix some error handling paths + do some clean-up

[Marion & Christophe JAILLET]

Le 22/05/2021 à 08:51, Christophe JAILLET a écrit :
> This serie fixes some (unlikely) error handlings paths.
>
> The 4th patch is completely speculative. When I compile-tested the changes,
> I had to remove this line in order for it to compile.
> As it works fine (at least for me) without it, I wonder if it is needed at all.
>
>
> Also, I wonder if the drivers in drivers/clk/mvebu are used by anyone.
> In order to compile-test the changes, I also had to change the 'bool' in Kconfig
> by 'bool "blah"'. Without this change, it was not possible to set
> CONFIG_MVEBU_CLK_CPU required by Makefile.
>
> I don't know if I did something wrong, if it is an issue only on my environment
> or if something got broken at some time in the build chain but it looks
> spurious.
>
> If I'm right and that these drivers never compile and no-one noticed it,
> maybe removing them is better than fixing some unlikely issues and style.
> If these drivers should stay, Kconfig may need some love from someone.
>
> Christophe JAILLET (4):
>    clk: mvebu: Fix a memory leak in an error handling path
>    clk: mvebu: Fix a another memory leak in an error handling path
>    clk: mvebu: Fix a checkpatch warning
>    clk: mvebu: Remove an unneeded include
>
>   drivers/clk/mvebu/clk-cpu.c | 22 +++++++++++++---------
>   1 file changed, 13 insertions(+), 9 deletions(-)

NAK, wrong cover letter sent

Sorry for the noise.

CJ