[PATCH 0/2] Fixes for cpu cooling

[PATCH 0/2] Fixes for cpu cooling

[Javi Merino]

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

[PATCH 1/2] thermal: cpu_cooling: don't call kcalloc() under rcu_read_lock

[Javi Merino]

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 <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
 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

[PATCH 2/2] thermal: cpu_cooling: free power table on error or when unregistering

[Javi Merino]

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 <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Javi Merino <javi.merino@arm.com>
---
 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

Re: [PATCH 0/2] Fixes for cpu cooling

[Javi Merino]

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

Re: [PATCH 0/2] Fixes for cpu cooling

[Javi Merino]

On Tue, Aug 25, 2015 at 07:53:48PM +0100, Javi Merino wrote:
> 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

Another ping