Commit c36cf0717631 ("thermal: cpu_cooling: implement the power
cooling device API") introduced two bugs: a call to kcalloc() (that
might sleep) under RCU and not freeing the allocation when it's no
longer needed. This series fixes both issues.
Javi Merino (2):
thermal: cpu_cooling: don't call kcalloc() under rcu_read_lock
thermal: cpu_cooling: free power table on error or when unregistering
drivers/thermal/cpu_cooling.c | 52 ++++++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 21 deletions(-)
--
1.9.1
build_dyn_power_table() allocates the power table while holding
rcu_read_lock. kcalloc using GFP_KERNEL may sleep, so it can't be
called in an RCU read-side path.
Move the rcu protection to the part of the function that really needs
it: the part that handles the dev_pm_opp pointer received from
dev_pm_opp_find_freq_ceil(). In the unlikely case that there is an OPP
added to the cpu while this function is running, return -EAGAIN.
Fixes: c36cf0717631 ("thermal: cpu_cooling: implement the power cooling device API")
Cc: Zhang Rui <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Signed-off-by: Javi Merino <[email protected]>
---
drivers/thermal/cpu_cooling.c | 47 +++++++++++++++++++++----------------------
1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index 6509c61b9648..b6c0f93ea5c2 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -253,7 +253,9 @@ static int cpufreq_thermal_notifier(struct notifier_block *nb,
* efficiently. Power is stored in mW, frequency in KHz. The
* resulting table is in ascending order.
*
- * Return: 0 on success, -E* on error.
+ * Return: 0 on success, -EINVAL if there are no OPPs for any CPUs,
+ * -ENOMEM if we run out of memory or -EAGAIN if an OPP was
+ * added/enabled while the function was executing.
*/
static int build_dyn_power_table(struct cpufreq_cooling_device *cpufreq_device,
u32 capacitance)
@@ -261,11 +263,9 @@ static int build_dyn_power_table(struct cpufreq_cooling_device *cpufreq_device,
struct power_table *power_table;
struct dev_pm_opp *opp;
struct device *dev = NULL;
- int num_opps = 0, cpu, i, ret = 0;
+ int num_opps = 0, cpu, i;
unsigned long freq;
- rcu_read_lock();
-
for_each_cpu(cpu, &cpufreq_device->allowed_cpus) {
dev = get_cpu_device(cpu);
if (!dev) {
@@ -275,24 +275,20 @@ static int build_dyn_power_table(struct cpufreq_cooling_device *cpufreq_device,
}
num_opps = dev_pm_opp_get_opp_count(dev);
- if (num_opps > 0) {
+ if (num_opps > 0)
break;
- } else if (num_opps < 0) {
- ret = num_opps;
- goto unlock;
- }
+ else if (num_opps < 0)
+ return num_opps;
}
- if (num_opps == 0) {
- ret = -EINVAL;
- goto unlock;
- }
+ if (num_opps == 0)
+ return -EINVAL;
power_table = kcalloc(num_opps, sizeof(*power_table), GFP_KERNEL);
- if (!power_table) {
- ret = -ENOMEM;
- goto unlock;
- }
+ if (!power_table)
+ return -ENOMEM;
+
+ rcu_read_lock();
for (freq = 0, i = 0;
opp = dev_pm_opp_find_freq_ceil(dev, &freq), !IS_ERR(opp);
@@ -300,6 +296,11 @@ static int build_dyn_power_table(struct cpufreq_cooling_device *cpufreq_device,
u32 freq_mhz, voltage_mv;
u64 power;
+ if (i >= num_opps) {
+ rcu_read_unlock();
+ return -EAGAIN;
+ }
+
freq_mhz = freq / 1000000;
voltage_mv = dev_pm_opp_get_voltage(opp) / 1000;
@@ -317,18 +318,16 @@ static int build_dyn_power_table(struct cpufreq_cooling_device *cpufreq_device,
power_table[i].power = power;
}
- if (i == 0) {
- ret = PTR_ERR(opp);
- goto unlock;
- }
+ rcu_read_unlock();
+
+ if (i != num_opps)
+ return PTR_ERR(opp);
cpufreq_device->cpu_dev = dev;
cpufreq_device->dyn_power_table = power_table;
cpufreq_device->dyn_power_table_entries = i;
-unlock:
- rcu_read_unlock();
- return ret;
+ return 0;
}
static u32 cpu_freq_to_power(struct cpufreq_cooling_device *cpufreq_device,
--
1.9.1
The power table is not being freed on error from cpufreq_cooling
register or when unregistering. Free it.
Fixes: c36cf0717631 ("thermal: cpu_cooling: implement the power cooling device API")
Cc: Zhang Rui <[email protected]>
Cc: Eduardo Valentin <[email protected]>
Signed-off-by: Javi Merino <[email protected]>
---
drivers/thermal/cpu_cooling.c | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index b6c0f93ea5c2..4fecc34545c3 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -263,7 +263,7 @@ static int build_dyn_power_table(struct cpufreq_cooling_device *cpufreq_device,
struct power_table *power_table;
struct dev_pm_opp *opp;
struct device *dev = NULL;
- int num_opps = 0, cpu, i;
+ int num_opps = 0, cpu, i, ret = 0;
unsigned long freq;
for_each_cpu(cpu, &cpufreq_device->allowed_cpus) {
@@ -298,7 +298,8 @@ static int build_dyn_power_table(struct cpufreq_cooling_device *cpufreq_device,
if (i >= num_opps) {
rcu_read_unlock();
- return -EAGAIN;
+ ret = -EAGAIN;
+ goto free_power_table;
}
freq_mhz = freq / 1000000;
@@ -320,14 +321,21 @@ static int build_dyn_power_table(struct cpufreq_cooling_device *cpufreq_device,
rcu_read_unlock();
- if (i != num_opps)
- return PTR_ERR(opp);
+ if (i != num_opps) {
+ ret = PTR_ERR(opp);
+ goto free_power_table;
+ }
cpufreq_device->cpu_dev = dev;
cpufreq_device->dyn_power_table = power_table;
cpufreq_device->dyn_power_table_entries = i;
return 0;
+
+free_power_table:
+ kfree(power_table);
+
+ return ret;
}
static u32 cpu_freq_to_power(struct cpufreq_cooling_device *cpufreq_device,
@@ -837,7 +845,7 @@ __cpufreq_cooling_register(struct device_node *np,
ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);
if (ret) {
cool_dev = ERR_PTR(ret);
- goto free_table;
+ goto free_power_table;
}
snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d",
@@ -877,6 +885,8 @@ __cpufreq_cooling_register(struct device_node *np,
remove_idr:
release_idr(&cpufreq_idr, cpufreq_dev->id);
+free_power_table:
+ kfree(cpufreq_dev->dyn_power_table);
free_table:
kfree(cpufreq_dev->freq_table);
free_time_in_idle_timestamp:
@@ -1023,6 +1033,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
thermal_cooling_device_unregister(cpufreq_dev->cool_dev);
release_idr(&cpufreq_idr, cpufreq_dev->id);
+ kfree(cpufreq_dev->dyn_power_table);
kfree(cpufreq_dev->time_in_idle_timestamp);
kfree(cpufreq_dev->time_in_idle);
kfree(cpufreq_dev->freq_table);
--
1.9.1
On Mon, Aug 17, 2015 at 07:21:41PM +0100, Javi Merino wrote:
> Commit c36cf0717631 ("thermal: cpu_cooling: implement the power
> cooling device API") introduced two bugs: a call to kcalloc() (that
> might sleep) under RCU and not freeing the allocation when it's no
> longer needed. This series fixes both issues.
>
> Javi Merino (2):
> thermal: cpu_cooling: don't call kcalloc() under rcu_read_lock
> thermal: cpu_cooling: free power table on error or when unregistering
Gentle ping