thermal: cpu_cooling: Remove usage of devm functions
PATCH 1/1 thermal: cpu_cooling: Remove usage of devm functions
In the function cpufreq_get_requested_power, the memory allocated for load_cpu is live within the function only. And after the allocation it is immediately freed with devm_kfree. There is no need to allocate memory for load_cpu with devm function so replace devm_kcalloc with kcalloc and devm_kfree with kfree.
---
This patch is having one checkpatch.pl warning which suggests that kfree(NULL) is safe. But I think leaving code with if is nice because it reflects the fact that kcalloc was under an if. So, I have ignored checkpatch. If maintainer wants me to go for changing things, I am fine with it too.
--- drivers/thermal/cpu_cooling.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/thermal/cpu_cooling.c +3 -4
@@ -584,8 +584,7 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev,if (trace_thermal_power_cpu_get_power_enabled ()) {u32 ncpus =cpumask_weight (&cpufreq_device->allowed_cpus); - load_cpu =devm_kcalloc (&cdev->device, ncpus,sizeof (*load_cpu), -GFP_KERNEL ); + load_cpu =kcalloc (ncpus,sizeof (*load_cpu),GFP_KERNEL ); }for_each_cpu (cpu, &cpufreq_device->allowed_cpus) {
@@ -609,7 +608,7 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, ret =get_static_power (cpufreq_device, tz, freq, &static_power);if (ret) {if (load_cpu) -devm_kfree (&cdev->device, load_cpu); +kfree (load_cpu);return ret; }
@@ -618,7 +617,7 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, &cpufreq_device->allowed_cpus, freq, load_cpu, i, dynamic_power, static_power); -devm_kfree (&cdev->device, load_cpu); +kfree (load_cpu); } *power = static_power + dynamic_power;
Signature · 1 line
1.9.1
Quoted 6 lines · Vaishali Thakkar
In the function
cpufreq_get_requested_power, the memory allocated forload_cpuis live within the function only. And after the allocation it is immediately freed withdevm_kfree. There is no need to allocate memory forload_cpuwith devm function so replacedevm_kcallocwith kcalloc anddevm_kfreewith kfree.
Quoted 7 lines
Signed-off-by Vaishali Thakkar <[email protected]>---
This patch is having one checkpatch.pl warning which suggests that kfree(NULL) is safe. But I think leaving code with if is nice because it reflects the fact that kcalloc was under an if. So, I have ignored checkpatch. If maintainer wants me to go for changing things, I am fine with it too.
I will rather ask you to do this:
drivers/thermal/cpu_cooling.c +0 -0
@@ -584,8 +584,8 @@ static int cpufreq_get_requested_power(struct
thermal_cooling_device *cdev,
if (trace_thermal_power_cpu_get_power_enabled()) {
u32 ncpus = cpumask_weight(&cpufreq_device->allowed_cpus);- load_cpu = devm_kcalloc(&cdev->device, ncpus, sizeof(*load_cpu),
- GFP_KERNEL);
+ load_cpu = kcalloc(&cdev->device, ncpus, sizeof(*load_cpu),
+ GFP_KERNEL);
}
for_each_cpu(cpu, &cpufreq_device->allowed_cpus) {@@ -607,22 +607,20 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev,
dynamic_power = get_dynamic_power(cpufreq_device, freq);
ret = get_static_power(cpufreq_device, tz, freq, &static_power);- if (ret) {
- if (load_cpu)
- devm_kfree(&cdev->device, load_cpu);
- return ret;
- }
+ if (ret)
+ goto free;
- if (load_cpu) {
+ if (load_cpu)
trace_thermal_power_cpu_get_power(
&cpufreq_device->allowed_cpus,
freq, load_cpu, i, dynamic_power, static_power);- devm_kfree(&cdev->device, load_cpu);
- }
-
*power = static_power + dynamic_power;
- return 0;
+
+free:
+ kfree(&cdev->device, load_cpu);
+
+ return ret;
}
Quoted 16 lines · Viresh Kumar
Quoted 6 lines · Vaishali Thakkar
In the function
cpufreq_get_requested_power, the memory allocated forload_cpuis live within the function only. And after the allocation it is immediately freed withdevm_kfree. There is no need to allocate memory forload_cpuwith devm function so replacedevm_kcallocwith kcalloc anddevm_kfreewith kfree.Quoted 7 lines
Signed-off-by Vaishali Thakkar <[email protected]>---
This patch is having one checkpatch.pl warning which suggests that kfree(NULL) is safe. But I think leaving code with if is nice because it reflects the fact that kcalloc was under an if. So, I have ignored checkpatch. If maintainer wants me to go for changing things, I am fine with it too.
Quoted 1 line
I will rather ask you to do this:
Ok. I was not sure about introducing new label. Thanks.
I will send v2 with the changes you pointed.
Quoted 46 lines
drivers/thermal/cpu_cooling.c +2
-2@@ -584,8 +584,8 @@ static int cpufreq_get_requested_power(structthermal_cooling_device *cdev, if (trace_thermal_power_cpu_get_power_enabled()) { u32 ncpus = cpumask_weight(&cpufreq_device->allowed_cpus); - load_cpu =devm_kcalloc (&cdev->device, ncpus,sizeof (*load_cpu), -GFP_KERNEL ); + load_cpu =kcalloc (&cdev->device, ncpus,sizeof (*load_cpu), +GFP_KERNEL ); }for_each_cpu(cpu, &cpufreq_device->allowed_cpus) {drivers/thermal/cpu_cooling.c +8
-10@@ -607,22 +607,20 @@ static int cpufreq_get_requested_power(structthermal_cooling_device *cdev, dynamic_power =get_dynamic_power (cpufreq_device, freq); ret =get_static_power (cpufreq_device, tz, freq, &static_power); -if (ret) { -if (load_cpu) -devm_kfree (&cdev->device, load_cpu); -return ret; - } +if (ret) + goto free; -if (load_cpu) { +if (load_cpu)trace_thermal_power_cpu_get_power ( &cpufreq_device->allowed_cpus, freq, load_cpu, i, dynamic_power, static_power); -devm_kfree (&cdev->device, load_cpu); - } - *power = static_power + dynamic_power; -return 0 ; + +free: +kfree (&cdev->device, load_cpu); + +return ret; }
Signature · 1 line
Vaishali
Quoted 23 lines · Viresh Kumar
Quoted 6 lines · Vaishali Thakkar
In the function
cpufreq_get_requested_power, the memory allocated forload_cpuis live within the function only. And after the allocation it is immediately freed withdevm_kfree. There is no need to allocate memory forload_cpuwith devm function so replacedevm_kcallocwith kcalloc anddevm_kfreewith kfree.Quoted 7 lines
Signed-off-by Vaishali Thakkar <[email protected]>---
This patch is having one checkpatch.pl warning which suggests that kfree(NULL) is safe. But I think leaving code with if is nice because it reflects the fact that kcalloc was under an if. So, I have ignored checkpatch. If maintainer wants me to go for changing things, I am fine with it too.I will rather ask you to do this:
drivers/thermal/cpu_cooling.c +0
-0
drivers/thermal/cpu_cooling.c static int cpufreq_get_requested_power(struct · line 590
Quoted 9 lines
drivers/thermal/cpu_cooling.c +2
-2@@ -584,8 +584,8 @@ static int cpufreq_get_requested_power(structthermal_cooling_device *cdev, if (trace_thermal_power_cpu_get_power_enabled()) { u32 ncpus = cpumask_weight(&cpufreq_device->allowed_cpus); - load_cpu =devm_kcalloc (&cdev->device, ncpus,sizeof (*load_cpu), -GFP_KERNEL ); + load_cpu =kcalloc (&cdev->device, ncpus,sizeof (*load_cpu), +GFP_KERNEL ); }
Sorry. I forgot to ask you one more thing. devm_kcalloc takes one more argument then
kcalloc. So, the change I did is correct I guess. And I'm not sure why you suggested this
change? I assume you just want me to correct in my patch for last argument. But that line
also comes under 80 characters.
Quoted 31 lines
drivers/thermal/cpu_cooling.c +8
-10for_each_cpu (cpu, &cpufreq_device->allowed_cpus) {@@ -607,22 +607,20 @@ static int cpufreq_get_requested_power(structthermal_cooling_device *cdev, dynamic_power =get_dynamic_power (cpufreq_device, freq); ret =get_static_power (cpufreq_device, tz, freq, &static_power); -if (ret) { -if (load_cpu) -devm_kfree (&cdev->device, load_cpu); -return ret; - } +if (ret) + goto free; -if (load_cpu) { +if (load_cpu)trace_thermal_power_cpu_get_power ( &cpufreq_device->allowed_cpus, freq, load_cpu, i, dynamic_power, static_power); -devm_kfree (&cdev->device, load_cpu); - } - *power = static_power + dynamic_power; -return 0 ; + +free: +kfree (&cdev->device, load_cpu); + +return ret; }
Signature · 1 line
Vaishali
Quoted 11 lines · Vaishali Thakkar
Quoted 11 lines
I will rather ask you to do this:
drivers/thermal/cpu_cooling.c +0
-0@@ -584,8 +584,8 @@ static int cpufreq_get_requested_power(structthermal_cooling_device *cdev, if (trace_thermal_power_cpu_get_power_enabled()) { u32 ncpus = cpumask_weight(&cpufreq_device->allowed_cpus);
Quoted 9 lines
drivers/thermal/cpu_cooling.c static int cpufreq_get_requested_power(struct · line 590
Quoted 5 lines
drivers/thermal/cpu_cooling.c +2
-2- load_cpu =devm_kcalloc (&cdev->device, ncpus,sizeof (*load_cpu), -GFP_KERNEL ); + load_cpu =kcalloc (&cdev->device, ncpus,sizeof (*load_cpu), +GFP_KERNEL ); }Sorry. I forgot to ask you one more thing.
devm_kcalloctakes one more argument then
kcalloc. So, the change I did is correct I guess.
Right, there is an extra argument in my code.
Signature · 1 line
viresh
Quoted 41 lines · Vaishali Thakkar
In the function
cpufreq_get_requested_power, the memory allocated forload_cpuis live within the function only. And after the allocation it is immediately freed withdevm_kfree. There is no need to allocate memory forload_cpuwith devm function so replacedevm_kcallocwith kcalloc anddevm_kfreewith kfree.Signed-off-by Vaishali Thakkar <[email protected]>---
This patch is having one checkpatch.pl warning which suggests that kfree(NULL) is safe. But I think leaving code with if is nice because it reflects the fact that kcalloc was under an if. So, I have ignored checkpatch. If maintainer wants me to go for changing things, I am fine with it too.--- drivers/thermal/cpu_cooling.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)drivers/thermal/cpu_cooling.c +2
-3@@ -584,8 +584,7 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev,if (trace_thermal_power_cpu_get_power_enabled ()) {u32 ncpus =cpumask_weight (&cpufreq_device->allowed_cpus); - load_cpu =devm_kcalloc (&cdev->device, ncpus,sizeof (*load_cpu), -GFP_KERNEL ); + load_cpu =kcalloc (ncpus,sizeof (*load_cpu),GFP_KERNEL ); }for_each_cpu (cpu, &cpufreq_device->allowed_cpus) {@@ -609,7 +608,7 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, ret =get_static_power (cpufreq_device, tz, freq, &static_power);if (ret) {if (load_cpu) -devm_kfree (&cdev->device, load_cpu); +kfree (load_cpu);return ret; }
drivers/thermal/cpu_cooling.c static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, · line 623
Quoted 8 lines
drivers/thermal/cpu_cooling.c +1
-1@@ -618,7 +617,7 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, &cpufreq_device->allowed_cpus, freq, load_cpu, i, dynamic_power, static_power); -devm_kfree (&cdev->device, load_cpu); +kfree (load_cpu); } *power = static_power + dynamic_power;
@Eduardo: Ignore V2 and apply this one :)
Hi Eduardo, Rui,
Quoted 50 lines · Viresh Kumar
Quoted 48 lines · Vaishali Thakkar
In the function
cpufreq_get_requested_power, the memory allocated forload_cpuis live within the function only. And after the allocation it is immediately freed withdevm_kfree. There is no need to allocate memory forload_cpuwith devm function so replacedevm_kcallocwith kcalloc anddevm_kfreewith kfree.Signed-off-by Vaishali Thakkar <[email protected]>---
This patch is having one checkpatch.pl warning which suggests that kfree(NULL) is safe. But I think leaving code with if is nice because it reflects the fact that kcalloc was under an if. So, I have ignored checkpatch. If maintainer wants me to go for changing things, I am fine with it too.--- drivers/thermal/cpu_cooling.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)drivers/thermal/cpu_cooling.c +3
-4@@ -584,8 +584,7 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev,if (trace_thermal_power_cpu_get_power_enabled ()) {u32 ncpus =cpumask_weight (&cpufreq_device->allowed_cpus); - load_cpu =devm_kcalloc (&cdev->device, ncpus,sizeof (*load_cpu), -GFP_KERNEL ); + load_cpu =kcalloc (ncpus,sizeof (*load_cpu),GFP_KERNEL ); }for_each_cpu (cpu, &cpufreq_device->allowed_cpus) {@@ -609,7 +608,7 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, ret =get_static_power (cpufreq_device, tz, freq, &static_power);if (ret) {if (load_cpu) -devm_kfree (&cdev->device, load_cpu); +kfree (load_cpu);return ret; }@@ -618,7 +617,7 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, &cpufreq_device->allowed_cpus, freq, load_cpu, i, dynamic_power, static_power); -devm_kfree (&cdev->device, load_cpu); +kfree (load_cpu); }
Quoted 7 lines
drivers/thermal/cpu_cooling.c static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, · line 623
Quoted 1 line
drivers/thermal/cpu_cooling.c +0
-0*power = static_power + dynamic_power;Acked-by Javi Merino <[email protected]>Acked-by Viresh Kumar <[email protected]>@Eduardo: Ignore V2 and apply this one :)
Looks like this fell through the cracks. Can you pick it up?
Cheers,
Javi
Quoted 7 lines · Vaishali Thakkar
In the function
cpufreq_get_requested_power, the memory allocated forload_cpuis live within the function only. And after the allocation it is immediately freed withdevm_kfree. There is no need to allocate memory forload_cpuwith devm function so replacedevm_kcallocwith kcalloc anddevm_kfreewith kfree.Signed-off-by Vaishali Thakkar <[email protected]>
Quoted 6 lines
---
This patch is having one checkpatch.pl warning which suggests that kfree(NULL) is safe. But I think leaving code with if is nice because it reflects the fact that kcalloc was under an if. So, I have ignored checkpatch. If maintainer wants me to go for changing things, I am fine with it too.
I wouldn't have minded changing the
if (load_cpu)
kfree(load_cpu)
to just "kfree(load_cpu)" but I don't have a strong preference. I'm also happy with the patch as it is.
Cheers,
Javi
Quoted 36 lines
--- drivers/thermal/cpu_cooling.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)drivers/thermal/cpu_cooling.c +3
-4@@ -584,8 +584,7 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev,if (trace_thermal_power_cpu_get_power_enabled ()) {u32 ncpus =cpumask_weight (&cpufreq_device->allowed_cpus); - load_cpu =devm_kcalloc (&cdev->device, ncpus,sizeof (*load_cpu), -GFP_KERNEL ); + load_cpu =kcalloc (ncpus,sizeof (*load_cpu),GFP_KERNEL ); }for_each_cpu (cpu, &cpufreq_device->allowed_cpus) {@@ -609,7 +608,7 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, ret =get_static_power (cpufreq_device, tz, freq, &static_power);if (ret) {if (load_cpu) -devm_kfree (&cdev->device, load_cpu); +kfree (load_cpu);return ret; }@@ -618,7 +617,7 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, &cpufreq_device->allowed_cpus, freq, load_cpu, i, dynamic_power, static_power); -devm_kfree (&cdev->device, load_cpu); +kfree (load_cpu); } *power = static_power + dynamic_power;
Quoted 6 lines · Vaishali Thakkar
In the function
cpufreq_get_requested_power, the memory allocated forload_cpuis live within the function only. And after the allocation it is immediately freed withdevm_kfree. There is no need to allocate memory forload_cpuwith devm function so replacedevm_kcallocwith kcalloc anddevm_kfreewith kfree.
Quoted 7 lines
Signed-off-by Vaishali Thakkar <[email protected]>---
This patch is having one checkpatch.pl warning which suggests that kfree(NULL) is safe. But I think leaving code with if is nice because it reflects the fact that kcalloc was under an if. So, I have ignored checkpatch. If maintainer wants me to go for changing things, I am fine with it too.
Applied. And I've removed the "if (load_cpu)" for kfree() to avoid warning reports.
thanks,
rui
Quoted 36 lines
--- drivers/thermal/cpu_cooling.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)drivers/thermal/cpu_cooling.c +3
-4@@ -584,8 +584,7 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev,if (trace_thermal_power_cpu_get_power_enabled ()) {u32 ncpus =cpumask_weight (&cpufreq_device->allowed_cpus); - load_cpu =devm_kcalloc (&cdev->device, ncpus,sizeof (*load_cpu), -GFP_KERNEL ); + load_cpu =kcalloc (ncpus,sizeof (*load_cpu),GFP_KERNEL ); }for_each_cpu (cpu, &cpufreq_device->allowed_cpus) {@@ -609,7 +608,7 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, ret =get_static_power (cpufreq_device, tz, freq, &static_power);if (ret) {if (load_cpu) -devm_kfree (&cdev->device, load_cpu); +kfree (load_cpu);return ret; }@@ -618,7 +617,7 @@ static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, &cpufreq_device->allowed_cpus, freq, load_cpu, i, dynamic_power, static_power); -devm_kfree (&cdev->device, load_cpu); +kfree (load_cpu); } *power = static_power + dynamic_power;