thermal: cpu_cooling: Remove usage of devm functions

PATCH In review Vaishali Thakkar 3 participants 8 messages Aug 20, 2015 – Aug 27, 2015 Thermal

Share & tools

Vaishali Thakkar

author

2015-08-20 16:14 UTC #0 lore raw

PATCH v2 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.

Signed-off-by Vaishali Thakkar <[email protected]>

---
Changes since v1:
- Introduce new label based on Viresh Kumar's suggestion

---
 drivers/thermal/cpu_cooling.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

drivers/thermal/cpu_cooling.c +9 ~~-11~~

index 620dcd4..7027923 100644
@@ -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) {

@@ -607,22 +606,21 @@ 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(load_cpu);
+
+	return ret;
 }
 
 /**

Signature · 1 line

1.9.1

Viresh Kumar

Acked-by on v2 1/1

2015-08-20 16:17 UTC #1 lore raw

Quoted 6 lines · Vaishali Thakkar

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.

Quoted 7 lines

Signed-off-by Vaishali Thakkar <[email protected]>
---
Changes since v1:
- Introduce new label based on Viresh Kumar's suggestion
---
 drivers/thermal/cpu_cooling.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

Acked-by Viresh Kumar <[email protected]>

Signature · 1 line

viresh

Javi Merino

2015-08-26 12:47 UTC #2 lore raw

In reply to Vaishali Thakkar

I missed this because I wasn't CCed :( Thankfully, I'll be in MAINTAINERS for this soon.

Quoted 37 lines · Vaishali Thakkar

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.
Signed-off-by Vaishali Thakkar <[email protected]>
---
Changes since v1:
- Introduce new label based on Viresh Kumar's suggestion
---
 drivers/thermal/cpu_cooling.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)
drivers/thermal/cpu_cooling.c +1 -7
index 620dcd4..7027923 100644
@@ -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) {
@@ -607,22 +606,21 @@ 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;
-	}

drivers/thermal/cpu_cooling.c static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, · line 621

Quoted 10 lines

drivers/thermal/cpu_cooling.c +3 -2

+	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);

This introduces a memory leak. Keep the kfree() here, you can't drop it. Cheers,
Javi

Quoted 12 lines

drivers/thermal/cpu_cooling.c +5 -2

-	}
-
 	*power = static_power + dynamic_power;
 	return 0;
+
+free:
+	kfree(load_cpu);
+
+	return ret;
 }
 
 /**

Viresh Kumar

2015-08-26 12:52 UTC #3 lore raw

Quoted 2 lines · Javi Merino

I missed this because I wasn't CCed :( Thankfully, I'll be in MAINTAINERS for this soon.

Yeah, I need to resend that patch soon :)

Quoted 6 lines

Review
Quoted 1 line
(quoted diff) +0 -1
-		devm_kfree(&cdev->device, load_cpu);
This introduces a memory leak. Keep the kfree() here, you can't drop it. Cheers,
Javi

Quoted 7 lines

drivers/thermal/cpu_cooling.c static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, · line 621
Quoted 7 lines
drivers/thermal/cpu_cooling.c +3 -2
-	}
-
 	*power = static_power + dynamic_power;
 	return 0;
+
+free:
+	kfree(load_cpu);

Wouldn't this make that work ?

Signature · 1 line

viresh

Javi Merino

2015-08-26 13:09 UTC #4 lore raw

Quoted 12 lines · Viresh Kumar

Quoted 2 lines · Javi Merino
I missed this because I wasn't CCed :( Thankfully, I'll be in MAINTAINERS for this soon.
Yeah, I need to resend that patch soon :)
Quoted 6 lines
Review
Quoted 1 line
(quoted diff) +0 -1
-		devm_kfree(&cdev->device, load_cpu);
This introduces a memory leak. Keep the kfree() here, you can't drop it. Cheers,
Javi

Quoted 9 lines

Quoted 7 lines
drivers/thermal/cpu_cooling.c static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, · line 621
Quoted 7 lines
drivers/thermal/cpu_cooling.c +3 -2
-	}
-
 	*power = static_power + dynamic_power;
 	return 0;
+
+free:
+	kfree(load_cpu);
Wouldn't this make that work ?

Nope, you're not reaching that code path from there. Removing the "return 0" would work, but I don't like it, since we would be calling kfree() all the time, even when the trace is not enabled. I'd rather leave the kfree() where it is.

Cheers,
Javi

Viresh Kumar

2015-08-26 13:14 UTC #5 lore raw

Quoted 13 lines · Javi Merino

Quoted 12 lines · Viresh Kumar
Quoted 2 lines · Javi Merino
I missed this because I wasn't CCed :( Thankfully, I'll be in MAINTAINERS for this soon.
Yeah, I need to resend that patch soon :)
Quoted 6 lines
Review
Quoted 1 line
(quoted diff) +0 -1
-		devm_kfree(&cdev->device, load_cpu);
This introduces a memory leak. Keep the kfree() here, you can't drop it. Cheers,
Javi

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
drivers/thermal/cpu_cooling.c static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, · line 618
Quoted 4 lines
drivers/thermal/cpu_cooling.c +0 -2
-	}
-
 	*power = static_power + dynamic_power;
 	return 0;

So, the change I suggested on V1 removed this as well :) and Vaishali missed it completely.

Quoted 10 lines

Quoted 5 lines
Quoted 3 lines
drivers/thermal/cpu_cooling.c static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, · line 621
Quoted 3 lines
drivers/thermal/cpu_cooling.c +3 -0
+
+free:
+	kfree(load_cpu);
Wouldn't this make that work ?
Nope, you're not reaching that code path from there. Removing the "return 0" would work, but I don't like it, since we would be calling kfree() all the time, even when the trace is not enabled. I'd rather leave the kfree() where it is.

Hmm..

Signature · 1 line

viresh

Vaishali Thakkar

author

2015-08-27 01:31 UTC #6 lore raw

Quoted 14 lines · Viresh Kumar

Quoted 13 lines · Javi Merino
Quoted 12 lines · Viresh Kumar
Quoted 2 lines · Javi Merino
I missed this because I wasn't CCed :( Thankfully, I'll be in MAINTAINERS for this soon.
Yeah, I need to resend that patch soon :)
Quoted 6 lines
Quoted 1 line
- devm_kfree(&cdev->device, load_cpu);
This introduces a memory leak. Keep the kfree() here, you can't drop it. Cheers,
Javi

Quoted 7 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/thermal/cpu_cooling.c static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, · line 618
Quoted 4 lines
drivers/thermal/cpu_cooling.c +0 -2
-       }
-
        *power = static_power + dynamic_power;
        return 0;
So, the change I suggested on V1 removed this as well :) and Vaishali missed it completely.

Yes. I missed the point that kfree was called at 2 places previously. Would you like me to send v3 with changes having just new label with 'goto' at both of these places or you would like to apply v1 of the patch?

Quoted 15 lines

Quoted 10 lines
Quoted 5 lines
Quoted 3 lines
drivers/thermal/cpu_cooling.c static int cpufreq_get_requested_power(struct thermal_cooling_device *cdev, · line 621
Quoted 3 lines
drivers/thermal/cpu_cooling.c +3 -0
+
+free:
+       kfree(load_cpu);
Wouldn't this make that work ?
Nope, you're not reaching that code path from there. Removing the "return 0" would work, but I don't like it, since we would be calling kfree() all the time, even when the trace is not enabled. I'd rather leave the kfree() where it is.
Hmm..
--
viresh

Signature · 1 line

Vaishali

Javi Merino

2015-08-27 09:00 UTC #7 lore raw

Quoted 20 lines · Vaishali Thakkar

Quoted 19 lines · Viresh Kumar
Quoted 17 lines · Javi Merino
Quoted 16 lines · Viresh Kumar
Quoted 2 lines · Javi Merino
I missed this because I wasn't CCed :( Thankfully, I'll be in MAINTAINERS for this soon.
Yeah, I need to resend that patch soon :)
Quoted 10 lines
Quoted 1 line
- devm_kfree(&cdev->device, load_cpu);
This introduces a memory leak. Keep the kfree() here, you can't drop it. Cheers,
Javi
Quoted 4 lines
drivers/thermal/cpu_cooling.c +0 -2
-       }
-
        *power = static_power + dynamic_power;
        return 0;

Quoted 6 lines

Quoted 2 lines
So, the change I suggested on V1 removed this as well :) and Vaishali missed it completely.
Yes. I missed the point that kfree was called at 2 places previously. Would you like me to send v3 with changes having just new label with 'goto' at both of these places or you would like to apply v1 of the patch?

I vote for v1. I've acked it.

Cheers,
Javi