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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Fix a checkpatch warning.
Add a missing space.
Signed-off-by: Christophe JAILLET <[email protected]>
---
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
Compiling without <asm/smp_plat.h> just works fine, so remove it
Signed-off-by: Christophe JAILLET <[email protected]>
---
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
Hello,
On Fri, 23 Apr 2021 08:24:52 +0200
Christophe JAILLET <[email protected]> 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
Le 23/04/2021 à 09:27, Thomas Petazzoni a écrit :
> Hello,
>
> On Fri, 23 Apr 2021 08:24:52 +0200
> Christophe JAILLET <[email protected]> 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
>
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 <[email protected]>
Gesendet: Freitag, 23. April 2021 08:25:01
An: [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; 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 <[email protected]>
---
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
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 <[email protected]>
> Gesendet: Freitag, 23. April 2021 08:25:01
> An: [email protected]; [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]; [email protected]; 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 <[email protected]>
> ---
> 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
>
>
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 <[email protected]>
Gesendet: Freitag, 23. April 2021 14:02:17
An: Walter Harms; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]
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 <[email protected]>
> Gesendet: Freitag, 23. April 2021 08:25:01
> An: [email protected]; [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]; [email protected]; 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 <[email protected]>
> ---
> 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
>
>
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