[PATCH] clk: mvebu/clk-cpu.c: fix memory leakage

[PATCH] clk: mvebu/clk-cpu.c: fix memory leakage

[Cong Ding]

the variable cpuclk and clk_name should be properly freed.

Signed-off-by: Cong Ding <dinggnu@gmail.com>
---
 drivers/clk/mvebu/clk-cpu.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
index ff004578..1a0d84f 100644
--- a/drivers/clk/mvebu/clk-cpu.c
+++ b/drivers/clk/mvebu/clk-cpu.c
@@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
 
 	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
 	if (WARN_ON(!clks))
-		return;
+		goto clks_out;
 
 	for_each_node_by_type(dn, "cpu") {
 		struct clk_init_data init;
@@ -134,11 +134,11 @@ void __init of_cpu_clk_setup(struct device_node *node)
 		int cpu, err;
 
 		if (WARN_ON(!clk_name))
-			return;
+			goto clk_name_out;
 
 		err = of_property_read_u32(dn, "reg", &cpu);
 		if (WARN_ON(err))
-			return;
+			goto bail_out;
 
 		sprintf(clk_name, "cpu%d", cpu);
 		parent_clk = of_clk_get(node, 0);
@@ -166,7 +166,10 @@ void __init of_cpu_clk_setup(struct device_node *node)
 
 	return;
 bail_out:
+	kfree(clk_name);
+clk_name_out:
 	kfree(clks);
+clks_out:
 	kfree(cpuclk);
 }
 
-- 
1.7.9.5

Re: [PATCH] clk: mvebu/clk-cpu.c: fix memory leakage

[Gregory CLEMENT]

Dear Cong Ding,

On 01/14/2013 06:18 PM, Cong Ding wrote:
> the variable cpuclk and clk_name should be properly freed.
> 

Thanks for reporting this memory leak and for your patch but I think
we could do even better, see below:

> Signed-off-by: Cong Ding <dinggnu@gmail.com>
> ---
>  drivers/clk/mvebu/clk-cpu.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
> index ff004578..1a0d84f 100644
> --- a/drivers/clk/mvebu/clk-cpu.c
> +++ b/drivers/clk/mvebu/clk-cpu.c
> @@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
>  
>  	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
>  	if (WARN_ON(!clks))
> -		return;
> +		goto clks_out;
>  
>  	for_each_node_by_type(dn, "cpu") {
>  		struct clk_init_data init;
> @@ -134,11 +134,11 @@ void __init of_cpu_clk_setup(struct device_node *node)
>  		int cpu, err;
>  
>  		if (WARN_ON(!clk_name))
> -			return;
> +			goto clk_name_out;

I agree

>  
>  		err = of_property_read_u32(dn, "reg", &cpu);
>  		if (WARN_ON(err))
> -			return;
> +			goto bail_out;

I agree

>  
>  		sprintf(clk_name, "cpu%d", cpu);
>  		parent_clk = of_clk_get(node, 0);
> @@ -166,7 +166,10 @@ void __init of_cpu_clk_setup(struct device_node *node)
>  
>  	return;
>  bail_out:
> +	kfree(clk_name);

Here you free only one clk_name whereas we could have previous clk_name which have
been already allocated during the for_each_node_by_type loop.

A more annoying thing is that you use clk_name whereas it is only valid in the
for_each_node_by_type statement and then this patch breaks the compilation:

drivers/clk/mvebu/clk-cpu.c: In function ‘of_cpu_clk_setup’:
drivers/clk/mvebu/clk-cpu.c:169:8: error: ‘clk_name’ undeclared (first use in this function)
drivers/clk/mvebu/clk-cpu.c:169:8: note: each undeclared identifier is reported only once for each function it appears in

Thanks,

Gregory

Re: [PATCH] clk: mvebu/clk-cpu.c: fix memory leakage

[Cong Ding]

On Tue, Jan 15, 2013 at 03:13:00PM +0100, Gregory CLEMENT wrote:
> Dear Cong Ding,
> 
> On 01/14/2013 06:18 PM, Cong Ding wrote:
> > the variable cpuclk and clk_name should be properly freed.
> > 
> 
> Thanks for reporting this memory leak and for your patch but I think
> we could do even better, see below:
> 
> > Signed-off-by: Cong Ding <dinggnu@gmail.com>
> > ---
> >  drivers/clk/mvebu/clk-cpu.c |    9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
> > index ff004578..1a0d84f 100644
> > --- a/drivers/clk/mvebu/clk-cpu.c
> > +++ b/drivers/clk/mvebu/clk-cpu.c
> > @@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
> >  
> >  	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
> >  	if (WARN_ON(!clks))
> > -		return;
> > +		goto clks_out;
> >  
> >  	for_each_node_by_type(dn, "cpu") {
> >  		struct clk_init_data init;
> > @@ -134,11 +134,11 @@ void __init of_cpu_clk_setup(struct device_node *node)
> >  		int cpu, err;
> >  
> >  		if (WARN_ON(!clk_name))
> > -			return;
> > +			goto clk_name_out;
> 
> I agree
> 
> >  
> >  		err = of_property_read_u32(dn, "reg", &cpu);
> >  		if (WARN_ON(err))
> > -			return;
> > +			goto bail_out;
> 
> I agree
> 
> >  
> >  		sprintf(clk_name, "cpu%d", cpu);
> >  		parent_clk = of_clk_get(node, 0);
> > @@ -166,7 +166,10 @@ void __init of_cpu_clk_setup(struct device_node *node)
> >  
> >  	return;
> >  bail_out:
> > +	kfree(clk_name);
> 
> Here you free only one clk_name whereas we could have previous clk_name which have
> been already allocated during the for_each_node_by_type loop.
> 
> A more annoying thing is that you use clk_name whereas it is only valid in the
> for_each_node_by_type statement and then this patch breaks the compilation:
> 
> drivers/clk/mvebu/clk-cpu.c: In function ‘of_cpu_clk_setup’:
> drivers/clk/mvebu/clk-cpu.c:169:8: error: ‘clk_name’ undeclared (first use in this function)
> drivers/clk/mvebu/clk-cpu.c:169:8: note: each undeclared identifier is reported only once for each function it appears in
sorry for the mistake, I am sending version 2.

thanks,
- cong

[PATCH v2] clk: mvebu/clk-cpu.c: fix memory leakage

[Cong Ding]

>From 75c73077905b822be6e8a32a09d6b0cdb5e61763 Mon Sep 17 00:00:00 2001
From: Cong Ding <dinggnu@gmail.com>
Date: Mon, 14 Jan 2013 18:06:26 +0100
Subject: [PATCH v2] clk: mvebu/clk-cpu.c: fix memory leakage

the variable cpuclk and clk_name should be properly freed when error happens.

Signed-off-by: Cong Ding <dinggnu@gmail.com>
---
 drivers/clk/mvebu/clk-cpu.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
index ff004578..1066a43 100644
--- a/drivers/clk/mvebu/clk-cpu.c
+++ b/drivers/clk/mvebu/clk-cpu.c
@@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
 
 	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
 	if (WARN_ON(!clks))
-		return;
+		goto clks_out;
 
 	for_each_node_by_type(dn, "cpu") {
 		struct clk_init_data init;
@@ -134,11 +134,13 @@ void __init of_cpu_clk_setup(struct device_node *node)
 		int cpu, err;
 
 		if (WARN_ON(!clk_name))
-			return;
+			goto bail_out;
 
 		err = of_property_read_u32(dn, "reg", &cpu);
-		if (WARN_ON(err))
-			return;
+		if (WARN_ON(err)) {
+			kfree(clk_name);
+			goto bail_out;
+		}
 
 		sprintf(clk_name, "cpu%d", cpu);
 		parent_clk = of_clk_get(node, 0);
@@ -156,8 +158,10 @@ 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))) {
+			kfree(clk_name);
 			goto bail_out;
+		}
 		clks[cpu] = clk;
 	}
 	clk_data.clk_num = MAX_CPU;
@@ -167,6 +171,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
 	return;
 bail_out:
 	kfree(clks);
+clks_out:
 	kfree(cpuclk);
 }
 
-- 
1.7.9.5

Re: [PATCH v2] clk: mvebu/clk-cpu.c: fix memory leakage

[Jason Cooper]

Mike,

On Tue, Jan 15, 2013 at 03:23:08PM +0000, Cong Ding wrote:
> From 75c73077905b822be6e8a32a09d6b0cdb5e61763 Mon Sep 17 00:00:00 2001
> From: Cong Ding <dinggnu@gmail.com>
> Date: Mon, 14 Jan 2013 18:06:26 +0100
> Subject: [PATCH v2] clk: mvebu/clk-cpu.c: fix memory leakage
> 
> the variable cpuclk and clk_name should be properly freed when error happens.
> 
> Signed-off-by: Cong Ding <dinggnu@gmail.com>
> ---
>  drivers/clk/mvebu/clk-cpu.c |   15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)


Do you want to take this fix through the clock tree?  If so,

Acked-by: Jason Cooper <jason@lakedaemon.net>

Otherwise, just let me know.

thx,

Jason.

> diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
> index ff004578..1066a43 100644
> --- a/drivers/clk/mvebu/clk-cpu.c
> +++ b/drivers/clk/mvebu/clk-cpu.c
> @@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
>  
>  	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
>  	if (WARN_ON(!clks))
> -		return;
> +		goto clks_out;
>  
>  	for_each_node_by_type(dn, "cpu") {
>  		struct clk_init_data init;
> @@ -134,11 +134,13 @@ void __init of_cpu_clk_setup(struct device_node *node)
>  		int cpu, err;
>  
>  		if (WARN_ON(!clk_name))
> -			return;
> +			goto bail_out;
>  
>  		err = of_property_read_u32(dn, "reg", &cpu);
> -		if (WARN_ON(err))
> -			return;
> +		if (WARN_ON(err)) {
> +			kfree(clk_name);
> +			goto bail_out;
> +		}
>  
>  		sprintf(clk_name, "cpu%d", cpu);
>  		parent_clk = of_clk_get(node, 0);
> @@ -156,8 +158,10 @@ 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))) {
> +			kfree(clk_name);
>  			goto bail_out;
> +		}
>  		clks[cpu] = clk;
>  	}
>  	clk_data.clk_num = MAX_CPU;
> @@ -167,6 +171,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
>  	return;
>  bail_out:
>  	kfree(clks);
> +clks_out:
>  	kfree(cpuclk);
>  }
>  
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] clk: mvebu/clk-cpu.c: fix memory leakage

[Gregory CLEMENT]

On 01/15/2013 04:37 PM, Jason Cooper wrote:
> Mike,
> 
> On Tue, Jan 15, 2013 at 03:23:08PM +0000, Cong Ding wrote:
>> From 75c73077905b822be6e8a32a09d6b0cdb5e61763 Mon Sep 17 00:00:00 2001
>> From: Cong Ding <dinggnu@gmail.com>
>> Date: Mon, 14 Jan 2013 18:06:26 +0100
>> Subject: [PATCH v2] clk: mvebu/clk-cpu.c: fix memory leakage
>>
>> the variable cpuclk and clk_name should be properly freed when error happens.
>>
>> Signed-off-by: Cong Ding <dinggnu@gmail.com>
>> ---
>>  drivers/clk/mvebu/clk-cpu.c |   15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> 
> Do you want to take this fix through the clock tree?  If so,
> 
> Acked-by: Jason Cooper <jason@lakedaemon.net>
> 

I also think it should go through the clock tree but before this
I'd like we fix the last issue.

Cong Ding,

you didn't take in account the case when the allocation of the 1st clocks
when the 2nd cpu clock failed. In this case there is still a memory leak with
the clock_name of the first cpu clock. See below for my proposal:

> Otherwise, just let me know.
> 
> thx,
> 
> Jason.
> 
>> diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
>> index ff004578..1066a43 100644
>> --- a/drivers/clk/mvebu/clk-cpu.c
>> +++ b/drivers/clk/mvebu/clk-cpu.c
>> @@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
>>  
>>  	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
>>  	if (WARN_ON(!clks))
>> -		return;
>> +		goto clks_out;
>>  
>>  	for_each_node_by_type(dn, "cpu") {
>>  		struct clk_init_data init;
>> @@ -134,11 +134,13 @@ void __init of_cpu_clk_setup(struct device_node *node)
>>  		int cpu, err;
>>  
>>  		if (WARN_ON(!clk_name))
>> -			return;
>> +			goto bail_out;
>>  
>>  		err = of_property_read_u32(dn, "reg", &cpu);
>> -		if (WARN_ON(err))
>> -			return;
>> +		if (WARN_ON(err)) {

>> +			kfree(clk_name);
we can free it later

>> +			goto bail_out;
>> +		}
>>  
>>  		sprintf(clk_name, "cpu%d", cpu);
>>  		parent_clk = of_clk_get(node, 0);
>> @@ -156,8 +158,10 @@ 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))) {

>> +			kfree(clk_name);
we can free it later

>>  			goto bail_out;
>> +		}
>>  		clks[cpu] = clk;
>>  	}
>>  	clk_data.clk_num = MAX_CPU;
>> @@ -167,6 +171,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
>>  	return;
>>  bail_out:
>>  	kfree(clks);
>> +clks_out:

as cpuclk is allocated with all its member set to 0, and kfree(0) is a valid call.
We can add the following lines:

while(ncpus--)
	kfree(cpuclk[ncpus].clk_name);

>>  	kfree(cpuclk);
>>  }
>>  
>> -- 
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v2] clk: mvebu/clk-cpu.c: fix memory leakage

[Cong Ding]

On Tue, Jan 15, 2013 at 05:33:57PM +0100, Gregory CLEMENT wrote:
> On 01/15/2013 04:37 PM, Jason Cooper wrote:
> > Mike,
> > 
> > On Tue, Jan 15, 2013 at 03:23:08PM +0000, Cong Ding wrote:
> >> From 75c73077905b822be6e8a32a09d6b0cdb5e61763 Mon Sep 17 00:00:00 2001
> >> From: Cong Ding <dinggnu@gmail.com>
> >> Date: Mon, 14 Jan 2013 18:06:26 +0100
> >> Subject: [PATCH v2] clk: mvebu/clk-cpu.c: fix memory leakage
> >>
> >> the variable cpuclk and clk_name should be properly freed when error happens.
> >>
> >> Signed-off-by: Cong Ding <dinggnu@gmail.com>
> >> ---
> >>  drivers/clk/mvebu/clk-cpu.c |   15 ++++++++++-----
> >>  1 file changed, 10 insertions(+), 5 deletions(-)
> > 
> > 
> > Do you want to take this fix through the clock tree?  If so,
> > 
> > Acked-by: Jason Cooper <jason@lakedaemon.net>
> > 
> 
> I also think it should go through the clock tree but before this
> I'd like we fix the last issue.
> 
> Cong Ding,
> 
> you didn't take in account the case when the allocation of the 1st clocks
> when the 2nd cpu clock failed. In this case there is still a memory leak with
> the clock_name of the first cpu clock. See below for my proposal:
> 
> > Otherwise, just let me know.
> > 
> > thx,
> > 
> > Jason.
> > 
> >> diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
> >> index ff004578..1066a43 100644
> >> --- a/drivers/clk/mvebu/clk-cpu.c
> >> +++ b/drivers/clk/mvebu/clk-cpu.c
> >> @@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
> >>  
> >>  	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
> >>  	if (WARN_ON(!clks))
> >> -		return;
> >> +		goto clks_out;
> >>  
> >>  	for_each_node_by_type(dn, "cpu") {
> >>  		struct clk_init_data init;
> >> @@ -134,11 +134,13 @@ void __init of_cpu_clk_setup(struct device_node *node)
> >>  		int cpu, err;
> >>  
> >>  		if (WARN_ON(!clk_name))
> >> -			return;
> >> +			goto bail_out;
> >>  
> >>  		err = of_property_read_u32(dn, "reg", &cpu);
> >> -		if (WARN_ON(err))
> >> -			return;
> >> +		if (WARN_ON(err)) {
> 
> >> +			kfree(clk_name);
> we can free it later
> 
> >> +			goto bail_out;
> >> +		}
> >>  
> >>  		sprintf(clk_name, "cpu%d", cpu);
> >>  		parent_clk = of_clk_get(node, 0);
> >> @@ -156,8 +158,10 @@ 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))) {
> 
> >> +			kfree(clk_name);
> we can free it later
> 
> >>  			goto bail_out;
> >> +		}
> >>  		clks[cpu] = clk;
> >>  	}
> >>  	clk_data.clk_num = MAX_CPU;
> >> @@ -167,6 +171,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
> >>  	return;
> >>  bail_out:
> >>  	kfree(clks);
> >> +clks_out:
> 
> as cpuclk is allocated with all its member set to 0, and kfree(0) is a valid call.
> We can add the following lines:
> while(ncpus--)
> 	kfree(cpuclk[ncpus].clk_name);
> 
> >>  	kfree(cpuclk);
> >>  }
I agree the version 2 patch still includes memory leakage in terms of clk_name,
but I am wondering whether it is safe to call kfree(cpuclk[ncpus].clkname)
directly or not. It's true that kfree(0) is valid, but cpuclk[ncpus].clkname
might not be 0 when it is allocated by kzalloc. kzalloc just allocates the
memory while doesn't ensure the initial value in this memory area is 0. So I
am thinking we should call memset after the alloction or use a counter to
remember the number of clk_names allocated?

- cong

Re: [PATCH v2] clk: mvebu/clk-cpu.c: fix memory leakage

[Gregory CLEMENT]

On 01/15/2013 07:26 PM, Cong Ding wrote:
> On Tue, Jan 15, 2013 at 05:33:57PM +0100, Gregory CLEMENT wrote:
>> On 01/15/2013 04:37 PM, Jason Cooper wrote:
>>> Mike,
>>>
>>> On Tue, Jan 15, 2013 at 03:23:08PM +0000, Cong Ding wrote:
>>>> From 75c73077905b822be6e8a32a09d6b0cdb5e61763 Mon Sep 17 00:00:00 2001
>>>> From: Cong Ding <dinggnu@gmail.com>
>>>> Date: Mon, 14 Jan 2013 18:06:26 +0100
>>>> Subject: [PATCH v2] clk: mvebu/clk-cpu.c: fix memory leakage
>>>>
>>>> the variable cpuclk and clk_name should be properly freed when error happens.
>>>>
>>>> Signed-off-by: Cong Ding <dinggnu@gmail.com>
>>>> ---
>>>>  drivers/clk/mvebu/clk-cpu.c |   15 ++++++++++-----
>>>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>>
>>>
>>> Do you want to take this fix through the clock tree?  If so,
>>>
>>> Acked-by: Jason Cooper <jason@lakedaemon.net>
>>>
>>
>> I also think it should go through the clock tree but before this
>> I'd like we fix the last issue.
>>
>> Cong Ding,
>>
>> you didn't take in account the case when the allocation of the 1st clocks
>> when the 2nd cpu clock failed. In this case there is still a memory leak with
>> the clock_name of the first cpu clock. See below for my proposal:
>>
>>> Otherwise, just let me know.
>>>
>>> thx,
>>>
>>> Jason.
>>>
>>>> diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
>>>> index ff004578..1066a43 100644
>>>> --- a/drivers/clk/mvebu/clk-cpu.c
>>>> +++ b/drivers/clk/mvebu/clk-cpu.c
>>>> @@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
>>>>  
>>>>  	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
>>>>  	if (WARN_ON(!clks))
>>>> -		return;
>>>> +		goto clks_out;
>>>>  
>>>>  	for_each_node_by_type(dn, "cpu") {
>>>>  		struct clk_init_data init;
>>>> @@ -134,11 +134,13 @@ void __init of_cpu_clk_setup(struct device_node *node)
>>>>  		int cpu, err;
>>>>  
>>>>  		if (WARN_ON(!clk_name))
>>>> -			return;
>>>> +			goto bail_out;
>>>>  
>>>>  		err = of_property_read_u32(dn, "reg", &cpu);
>>>> -		if (WARN_ON(err))
>>>> -			return;
>>>> +		if (WARN_ON(err)) {
>>
>>>> +			kfree(clk_name);
>> we can free it later
>>
>>>> +			goto bail_out;
>>>> +		}
>>>>  
>>>>  		sprintf(clk_name, "cpu%d", cpu);
>>>>  		parent_clk = of_clk_get(node, 0);
>>>> @@ -156,8 +158,10 @@ 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))) {
>>
>>>> +			kfree(clk_name);
>> we can free it later
>>
>>>>  			goto bail_out;
>>>> +		}
>>>>  		clks[cpu] = clk;
>>>>  	}
>>>>  	clk_data.clk_num = MAX_CPU;
>>>> @@ -167,6 +171,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
>>>>  	return;
>>>>  bail_out:
>>>>  	kfree(clks);
>>>> +clks_out:
>>
>> as cpuclk is allocated with all its member set to 0, and kfree(0) is a valid call.
>> We can add the following lines:
>> while(ncpus--)
>> 	kfree(cpuclk[ncpus].clk_name);
>>
>>>>  	kfree(cpuclk);
>>>>  }
> I agree the version 2 patch still includes memory leakage in terms of clk_name,
> but I am wondering whether it is safe to call kfree(cpuclk[ncpus].clkname)
> directly or not. It's true that kfree(0) is valid, but cpuclk[ncpus].clkname
> might not be 0 when it is allocated by kzalloc. kzalloc just allocates the
> memory while doesn't ensure the initial value in this memory area is 0. So I

I think that you describe the behavior of kmalloc, but the main difference
with kmalloc and kzalloc, is that kzalloc allocates _zeroed_ paged, so I am
pretty sure that all the memory space allocated should be set to zero.

> am thinking we should call memset after the alloction or use a counter to
> remember the number of clk_names allocated?
> 
> - cong
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v3] clk: mvebu/clk-cpu.c: fix memory leakage

[Cong Ding]

the variable cpuclk and clk_name should be properly freed when error happens.

Signed-off-by: Cong Ding <dinggnu@gmail.com>
Acked-by: Jason Cooper <jason@lakedaemon.net>
---
 drivers/clk/mvebu/clk-cpu.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
index ff004578..9dd2551 100644
--- a/drivers/clk/mvebu/clk-cpu.c
+++ b/drivers/clk/mvebu/clk-cpu.c
@@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
 
 	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
 	if (WARN_ON(!clks))
-		return;
+		goto clks_out;
 
 	for_each_node_by_type(dn, "cpu") {
 		struct clk_init_data init;
@@ -134,11 +134,11 @@ void __init of_cpu_clk_setup(struct device_node *node)
 		int cpu, err;
 
 		if (WARN_ON(!clk_name))
-			return;
+			goto bail_out;
 
 		err = of_property_read_u32(dn, "reg", &cpu);
 		if (WARN_ON(err))
-			return;
+			goto bail_out;
 
 		sprintf(clk_name, "cpu%d", cpu);
 		parent_clk = of_clk_get(node, 0);
@@ -167,6 +167,9 @@ void __init of_cpu_clk_setup(struct device_node *node)
 	return;
 bail_out:
 	kfree(clks);
+	while(ncpus--)
+		kfree(cpuclk[ncpus].clk_name);
+clks_out:
 	kfree(cpuclk);
 }
 
-- 
1.7.10.4

Re: [PATCH v3] clk: mvebu/clk-cpu.c: fix memory leakage

[Gregory CLEMENT]

On 01/15/2013 07:44 PM, Cong Ding wrote:
> the variable cpuclk and clk_name should be properly freed when error happens.
Dear Cong Ding,

Thanks for you efforts!
I am happy with this patch and I tested it on the Armada XP DB board, so
you can now add my:

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Mike,

could you take this patch for 3.8-rc fixes?

If you prefer, Jason agrees to take it, but you probably didn't notice it,
because he uses your former(and no more valid) email when he wrote this.

Thanks,

Gregory

> 
> Signed-off-by: Cong Ding <dinggnu@gmail.com>
> Acked-by: Jason Cooper <jason@lakedaemon.net>
> ---
>  drivers/clk/mvebu/clk-cpu.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
> index ff004578..9dd2551 100644
> --- a/drivers/clk/mvebu/clk-cpu.c
> +++ b/drivers/clk/mvebu/clk-cpu.c
> @@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
>  
>  	clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
>  	if (WARN_ON(!clks))
> -		return;
> +		goto clks_out;
>  
>  	for_each_node_by_type(dn, "cpu") {
>  		struct clk_init_data init;
> @@ -134,11 +134,11 @@ void __init of_cpu_clk_setup(struct device_node *node)
>  		int cpu, err;
>  
>  		if (WARN_ON(!clk_name))
> -			return;
> +			goto bail_out;
>  
>  		err = of_property_read_u32(dn, "reg", &cpu);
>  		if (WARN_ON(err))
> -			return;
> +			goto bail_out;
>  
>  		sprintf(clk_name, "cpu%d", cpu);
>  		parent_clk = of_clk_get(node, 0);
> @@ -167,6 +167,9 @@ void __init of_cpu_clk_setup(struct device_node *node)
>  	return;
>  bail_out:
>  	kfree(clks);
> +	while(ncpus--)
> +		kfree(cpuclk[ncpus].clk_name);
> +clks_out:
>  	kfree(cpuclk);
>  }
>  
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH v3] clk: mvebu/clk-cpu.c: fix memory leakage

[Jason Cooper]

On Tue, Jan 15, 2013 at 09:46:03PM +0100, Gregory CLEMENT wrote:
> If you prefer, Jason agrees to take it, but you probably didn't notice it,
> because he uses your former(and no more valid) email when he wrote this.

Actually, I caught that after I hit send.  I resent (just to him), so he
should have it in his inbox.

I've also updated my email alias accordingly.

thx,

Jason.

Re: [PATCH v3] clk: mvebu/clk-cpu.c: fix memory leakage

[Mike Turquette]

Quoting Gregory CLEMENT (2013-01-15 12:46:03)
> On 01/15/2013 07:44 PM, Cong Ding wrote:
> > the variable cpuclk and clk_name should be properly freed when error happens.
> Dear Cong Ding,
> 
> Thanks for you efforts!
> I am happy with this patch and I tested it on the Armada XP DB board, so
> you can now add my:
> 
> Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> 
> Mike,
> 
> could you take this patch for 3.8-rc fixes?
> 
> If you prefer, Jason agrees to take it, but you probably didn't notice it,
> because he uses your former(and no more valid) email when he wrote this.
> 

Acked-by: Mike Turquette <mturquette@linaro.org>

I don't have an existing clk-fixes branch.  This patch is the first fix
I've reviewed for this cycle.  I'm happy if you want to take it and
submit as part of any other mvebu fixes you have.  Otherwise I can take
it.

Let me know what you decide.

Thanks,
Mike

> Thanks,
> 
> Gregory
> 
> > 
> > Signed-off-by: Cong Ding <dinggnu@gmail.com>
> > Acked-by: Jason Cooper <jason@lakedaemon.net>
> > ---
> >  drivers/clk/mvebu/clk-cpu.c |    9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/clk/mvebu/clk-cpu.c b/drivers/clk/mvebu/clk-cpu.c
> > index ff004578..9dd2551 100644
> > --- a/drivers/clk/mvebu/clk-cpu.c
> > +++ b/drivers/clk/mvebu/clk-cpu.c
> > @@ -124,7 +124,7 @@ void __init of_cpu_clk_setup(struct device_node *node)
> >  
> >       clks = kzalloc(ncpus * sizeof(*clks), GFP_KERNEL);
> >       if (WARN_ON(!clks))
> > -             return;
> > +             goto clks_out;
> >  
> >       for_each_node_by_type(dn, "cpu") {
> >               struct clk_init_data init;
> > @@ -134,11 +134,11 @@ void __init of_cpu_clk_setup(struct device_node *node)
> >               int cpu, err;
> >  
> >               if (WARN_ON(!clk_name))
> > -                     return;
> > +                     goto bail_out;
> >  
> >               err = of_property_read_u32(dn, "reg", &cpu);
> >               if (WARN_ON(err))
> > -                     return;
> > +                     goto bail_out;
> >  
> >               sprintf(clk_name, "cpu%d", cpu);
> >               parent_clk = of_clk_get(node, 0);
> > @@ -167,6 +167,9 @@ void __init of_cpu_clk_setup(struct device_node *node)
> >       return;
> >  bail_out:
> >       kfree(clks);
> > +     while(ncpus--)
> > +             kfree(cpuclk[ncpus].clk_name);
> > +clks_out:
> >       kfree(cpuclk);
> >  }
> >  
> > 
> 
> 
> -- 
> Gregory Clement, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com

Re: [PATCH v3] clk: mvebu/clk-cpu.c: fix memory leakage

[Jason Cooper]

On Tue, Jan 15, 2013 at 05:01:43PM -0800, Mike Turquette wrote:
> Quoting Gregory CLEMENT (2013-01-15 12:46:03)
> > On 01/15/2013 07:44 PM, Cong Ding wrote:
> > > the variable cpuclk and clk_name should be properly freed when error happens.
> > Dear Cong Ding,
> > 
> > Thanks for you efforts!
> > I am happy with this patch and I tested it on the Armada XP DB board, so
> > you can now add my:
> > 
> > Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > 
> > Mike,
> > 
> > could you take this patch for 3.8-rc fixes?
> > 
> > If you prefer, Jason agrees to take it, but you probably didn't notice it,
> > because he uses your former(and no more valid) email when he wrote this.
> > 
> 
> Acked-by: Mike Turquette <mturquette@linaro.org>
> 
> I don't have an existing clk-fixes branch.  This patch is the first fix
> I've reviewed for this cycle.  I'm happy if you want to take it and
> submit as part of any other mvebu fixes you have.  Otherwise I can take
> it.

I'll take it, no problem.  Thanks for the Ack Mike,

Jason.

Re: [PATCH v3] clk: mvebu/clk-cpu.c: fix memory leakage

[Jason Cooper]

On Tue, Jan 15, 2013 at 07:44:26PM +0100, Cong Ding wrote:
> the variable cpuclk and clk_name should be properly freed when error happens.
> 
> Signed-off-by: Cong Ding <dinggnu@gmail.com>
> Acked-by: Jason Cooper <jason@lakedaemon.net>
> ---
>  drivers/clk/mvebu/clk-cpu.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Applied to mvebu/fixes with MikeT's Ack.

thx,

Jason.