Hi Eduardo,
As you know I got into fixing cpu_cooling.c due to some cpufreq issues you and Lukasz were struggling with. I found some issues in cpu_cooling then and here are the fixes/cleanups.
Sorry for the long list. Haven't broken them into smaller sets as most of the patches are very small, easy to review and inter-dependent. Only few of them should take more time to review. If this doesn't work out, let me know and I will try to send separate inter-dependent sets.
Just apply whatever looks fine and I will update/resend the ones left in V2 if at required.
First few are updates to platform drivers. Exynos fails to register after few patches in this series as it doesn't handle -EPROBE_DEFER properly (reported that in reply to your patch as well). Others weren't setting clip_cpus properly and are fixed.
Next ones are cleanups of cpu_cooling.c to get things properly organized.
Let me know if I screwed it up completely.
Tested-on: Exynos5250 (Dual ARM Cortex A15). Rebased-over: v3.18-rc6 Pushed here: git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/linux.git thermal/cpu-cooling-fixes
Cc: Amit Daniel Kachhap amit.daniel@samsung.com Cc: Chanwoo Choi cw00.choi@samsung.com Cc: Hongbo Zhang hongbo.zhang@linaro.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Lukasz Majewski l.majewski@samsung.com Cc: Shawn Guo shawn.guo@linaro.org
Viresh Kumar (26): thermal: db8500: pass cpu_present_mask to cpufreq_cooling_register() thermal: imx: pass cpu_present_mask to cpufreq_cooling_register() thermal: exynos: pass cpu_present_mask to cpufreq_cooling_register() thermal: exynos: Handle -EPROBE_DEFER properly cpu_cooling: random comment fixups cpu_cooling: fix doc comment over struct cpufreq_cooling_device cpu_cooling: Add comment to clarify relation between cooling state and frequency cpu_cooling: Pass variable instead of its type to sizeof() cpu_cooling: no need to set cpufreq_state to zero cpu_cooling: no need to set cpufreq_dev to NULL cpu_cooling: propagate error returned by idr_alloc() cpu_cooling: Don't match min/max frequencies for all CPUs on cooling register cpu_cooling: don't iterate over all allowed_cpus to update cpufreq policy cpu_cooling: Don't check is_cpufreq_valid() cpu_cooling: do error handling at the bottom in __cpufreq_cooling_register() cpu_cooling: Drop useless locking around idr_alloc/idr_remove cpu_cooling: Merge cpufreq_apply_cooling() into cpufreq_set_cur_state() cpu_cooling: Merge get_cpu_frequency() into cpufreq_set_cur_state() cpu_cooling: find max level during device registration cpu_cooling: get_property() doesn't need to support GET_MAXL anymore cpu_cooling: create list of cpufreq_cooling_devices cpu_cooling: use cpufreq_dev_list instead of cpufreq_dev_count cpu_cooling: Pass 'cpufreq_dev' to get_property() cpu_cooling: Store frequencies in descending order cpu_cooling: Use cpufreq_dev->freq_table for finding level/freq cpu_cooling: update copyright tags
drivers/thermal/cpu_cooling.c | 405 +++++++++--------------- drivers/thermal/db8500_cpufreq_cooling.c | 5 +- drivers/thermal/imx_thermal.c | 4 +- drivers/thermal/samsung/exynos_thermal_common.c | 11 +- drivers/thermal/samsung/exynos_tmu.c | 4 +- 5 files changed, 153 insertions(+), 276 deletions(-)
cpufreq_cooling_register() expects mask of all the CPUs where frequency constraint is applicable.
This platform has more than one CPU to which these constraints will apply and so passing mask of only CPU0 wouldn't be sufficient. Also, this platform has a single cluster of CPUs and the constraint applies to all CPUs.
If CPU0 is hoplugged out then we may face strange BUGs as cpu_cooling framework isn't aware of any siblings sharing clock line.
Fix it by passing cpu_present_mask to cpufreq_cooling_register().
Cc: Hongbo Zhang hongbo.zhang@linaro.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/db8500_cpufreq_cooling.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/thermal/db8500_cpufreq_cooling.c b/drivers/thermal/db8500_cpufreq_cooling.c index 786d192..374ef1e 100644 --- a/drivers/thermal/db8500_cpufreq_cooling.c +++ b/drivers/thermal/db8500_cpufreq_cooling.c @@ -28,15 +28,12 @@ static int db8500_cpufreq_cooling_probe(struct platform_device *pdev) { struct thermal_cooling_device *cdev; - struct cpumask mask_val;
/* make sure cpufreq driver has been initialized */ if (!cpufreq_frequency_get_table(0)) return -EPROBE_DEFER;
- cpumask_set_cpu(0, &mask_val); - cdev = cpufreq_cooling_register(&mask_val); - + cdev = cpufreq_cooling_register(cpu_present_mask); if (IS_ERR(cdev)) { dev_err(&pdev->dev, "Failed to register cooling device\n"); return PTR_ERR(cdev);
cpufreq_cooling_register() expects mask of all the CPUs where frequency constraint is applicable.
This platform has more than one CPU to which these constraints will apply and so passing mask of only CPU0 wouldn't be sufficient. Also, this platform has a single cluster of CPUs and the constraint applies to all CPUs.
If CPU0 is hoplugged out then we may face strange BUGs as cpu_cooling framework isn't aware of any siblings sharing clock line.
Fix it by passing cpu_present_mask to cpufreq_cooling_register().
Cc: Shawn Guo shawn.guo@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/imx_thermal.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c index 5a1f107..1eefc6d 100644 --- a/drivers/thermal/imx_thermal.c +++ b/drivers/thermal/imx_thermal.c @@ -454,7 +454,6 @@ static int imx_thermal_probe(struct platform_device *pdev) const struct of_device_id *of_id = of_match_device(of_imx_thermal_match, &pdev->dev); struct imx_thermal_data *data; - struct cpumask clip_cpus; struct regmap *map; int measure_freq; int ret; @@ -516,8 +515,7 @@ static int imx_thermal_probe(struct platform_device *pdev) regmap_write(map, MISC0 + REG_SET, MISC0_REFTOP_SELBIASOFF); regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_POWER_DOWN);
- cpumask_set_cpu(0, &clip_cpus); - data->cdev = cpufreq_cooling_register(&clip_cpus); + data->cdev = cpufreq_cooling_register(cpu_present_mask); if (IS_ERR(data->cdev)) { ret = PTR_ERR(data->cdev); dev_err(&pdev->dev,
cpufreq_cooling_register() expects mask of all the CPUs where frequency constraint is applicable.
This platform has more than one CPU to which these constraints will apply and so passing mask of only CPU0 wouldn't be sufficient. Also, this platform has a single cluster of CPUs and the constraint applies to all CPUs.
If CPU0 is hoplugged out then we may face strange BUGs as cpu_cooling framework isn't aware of any siblings sharing clock line.
Fix it by passing cpu_present_mask to cpufreq_cooling_register().
Cc: Chanwoo Choi cw00.choi@samsung.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Amit Daniel Kachhap amit.daniel@samsung.com Cc: Lukasz Majewski l.majewski@samsung.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/samsung/exynos_thermal_common.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/thermal/samsung/exynos_thermal_common.c b/drivers/thermal/samsung/exynos_thermal_common.c index 3f5ad25..bf39212 100644 --- a/drivers/thermal/samsung/exynos_thermal_common.c +++ b/drivers/thermal/samsung/exynos_thermal_common.c @@ -347,7 +347,6 @@ void exynos_report_trigger(struct thermal_sensor_conf *conf) int exynos_register_thermal(struct thermal_sensor_conf *sensor_conf) { int ret; - struct cpumask mask_val; struct exynos_thermal_zone *th_zone;
if (!sensor_conf || !sensor_conf->read_temperature) { @@ -367,9 +366,8 @@ int exynos_register_thermal(struct thermal_sensor_conf *sensor_conf) * sensor */ if (sensor_conf->cooling_data.freq_clip_count > 0) { - cpumask_set_cpu(0, &mask_val); th_zone->cool_dev[th_zone->cool_dev_size] = - cpufreq_cooling_register(&mask_val); + cpufreq_cooling_register(cpu_present_mask); if (IS_ERR(th_zone->cool_dev[th_zone->cool_dev_size])) { dev_err(sensor_conf->dev, "Failed to register cpufreq cooling device\n");
cpufreq_cooling_register() can return -EPROBE_DEFER if cpufreq driver isn't ready yet and so the callers must defer their initialization.
Exynos thermal drivers weren't handling this well and were raising false error message when -EPROBE_DEFER is returned to them.
Fix them to handle -EPROBE_DEFER.
Cc: Chanwoo Choi cw00.choi@samsung.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Amit Daniel Kachhap amit.daniel@samsung.com Cc: Lukasz Majewski l.majewski@samsung.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/samsung/exynos_thermal_common.c | 7 ++++--- drivers/thermal/samsung/exynos_tmu.c | 4 +++- 2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/thermal/samsung/exynos_thermal_common.c b/drivers/thermal/samsung/exynos_thermal_common.c index bf39212..0be1d54 100644 --- a/drivers/thermal/samsung/exynos_thermal_common.c +++ b/drivers/thermal/samsung/exynos_thermal_common.c @@ -369,9 +369,10 @@ int exynos_register_thermal(struct thermal_sensor_conf *sensor_conf) th_zone->cool_dev[th_zone->cool_dev_size] = cpufreq_cooling_register(cpu_present_mask); if (IS_ERR(th_zone->cool_dev[th_zone->cool_dev_size])) { - dev_err(sensor_conf->dev, - "Failed to register cpufreq cooling device\n"); - ret = -EINVAL; + ret = PTR_ERR(th_zone->cool_dev[th_zone->cool_dev_size]); + if (ret != -EPROBE_DEFER) + dev_err(sensor_conf->dev, + "Failed to register cpufreq cooling device\n"); goto err_unregister; } th_zone->cool_dev_size++; diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c index 49c0924..cc3677f 100644 --- a/drivers/thermal/samsung/exynos_tmu.c +++ b/drivers/thermal/samsung/exynos_tmu.c @@ -683,7 +683,9 @@ static int exynos_tmu_probe(struct platform_device *pdev) /* Register the sensor with thermal management interface */ ret = exynos_register_thermal(sensor_conf); if (ret) { - dev_err(&pdev->dev, "Failed to register thermal interface\n"); + if (ret != -EPROBE_DEFER) + dev_err(&pdev->dev, + "Failed to register thermal interface\n"); goto err_clk; } data->reg_conf = sensor_conf;
On Fri, Nov 28, 2014 at 03:13:58PM +0530, Viresh Kumar wrote:
cpufreq_cooling_register() can return -EPROBE_DEFER if cpufreq driver isn't ready yet and so the callers must defer their initialization.
Exynos thermal drivers weren't handling this well and were raising false error message when -EPROBE_DEFER is returned to them.
Fix them to handle -EPROBE_DEFER.
As mentioned in patch 0, this one has been merged to another patch.
Cc: Chanwoo Choi cw00.choi@samsung.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Amit Daniel Kachhap amit.daniel@samsung.com Cc: Lukasz Majewski l.majewski@samsung.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/thermal/samsung/exynos_thermal_common.c | 7 ++++--- drivers/thermal/samsung/exynos_tmu.c | 4 +++- 2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/thermal/samsung/exynos_thermal_common.c b/drivers/thermal/samsung/exynos_thermal_common.c index bf39212..0be1d54 100644 --- a/drivers/thermal/samsung/exynos_thermal_common.c +++ b/drivers/thermal/samsung/exynos_thermal_common.c @@ -369,9 +369,10 @@ int exynos_register_thermal(struct thermal_sensor_conf *sensor_conf) th_zone->cool_dev[th_zone->cool_dev_size] = cpufreq_cooling_register(cpu_present_mask); if (IS_ERR(th_zone->cool_dev[th_zone->cool_dev_size])) {
dev_err(sensor_conf->dev,"Failed to register cpufreq cooling device\n");ret = -EINVAL;
ret = PTR_ERR(th_zone->cool_dev[th_zone->cool_dev_size]);if (ret != -EPROBE_DEFER)dev_err(sensor_conf->dev,"Failed to register cpufreq cooling device\n"); goto err_unregister;diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c index 49c0924..cc3677f 100644 --- a/drivers/thermal/samsung/exynos_tmu.c +++ b/drivers/thermal/samsung/exynos_tmu.c @@ -683,7 +683,9 @@ static int exynos_tmu_probe(struct platform_device *pdev) /* Register the sensor with thermal management interface */ ret = exynos_register_thermal(sensor_conf); if (ret) {
dev_err(&pdev->dev, "Failed to register thermal interface\n");
if (ret != -EPROBE_DEFER)dev_err(&pdev->dev,"Failed to register thermal interface\n");-- 2.0.3.693.g996b0fd
s/give/given
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 1ab0018..7c2ba2e 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -122,7 +122,7 @@ enum cpufreq_cooling_property { };
/** - * get_property - fetch a property of interest for a give cpu. + * get_property - fetch a property of interest for a given cpu. * @cpu: cpu for which the property is required * @input: query parameter * @output: query return @@ -132,6 +132,7 @@ enum cpufreq_cooling_property { * 1. get maximum cpu cooling states * 2. translate frequency to cooling state * 3. translate cooling state to frequency + * * Note that the code may be not in good shape * but it is written in this way in order to: * a) reduce duplicate code as most of the code can be shared. @@ -212,7 +213,7 @@ static int get_property(unsigned int cpu, unsigned long input, }
/** - * cpufreq_cooling_get_level - for a give cpu, return the cooling level. + * cpufreq_cooling_get_level - for a given cpu, return the cooling level. * @cpu: cpu for which the level is required * @freq: the frequency of interest *
On Fri, Nov 28, 2014 at 03:13:59PM +0530, Viresh Kumar wrote:
s/give/given
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Acked-by: Eduardo Valentin edubezval@gmail.com
drivers/thermal/cpu_cooling.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 1ab0018..7c2ba2e 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -122,7 +122,7 @@ enum cpufreq_cooling_property { }; /**
- get_property - fetch a property of interest for a give cpu.
- get_property - fetch a property of interest for a given cpu.
- @cpu: cpu for which the property is required
- @input: query parameter
- @output: query return
@@ -132,6 +132,7 @@ enum cpufreq_cooling_property {
- get maximum cpu cooling states
- translate frequency to cooling state
- translate cooling state to frequency
- Note that the code may be not in good shape
- but it is written in this way in order to:
- a) reduce duplicate code as most of the code can be shared.
@@ -212,7 +213,7 @@ static int get_property(unsigned int cpu, unsigned long input, } /**
- cpufreq_cooling_get_level - for a give cpu, return the cooling level.
- cpufreq_cooling_get_level - for a given cpu, return the cooling level.
- @cpu: cpu for which the level is required
- @freq: the frequency of interest
-- 2.0.3.693.g996b0fd
cooling_cpufreq_lock isn't used to protect this structure and so the comment over it is outdated. Fix it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 7c2ba2e..3ef9050 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -40,9 +40,8 @@ * frequency. * @allowed_cpus: all the cpus involved for this cpufreq_cooling_device. * - * This structure is required for keeping information of each - * cpufreq_cooling_device registered. In order to prevent corruption of this a - * mutex lock cooling_cpufreq_lock is used. + * This structure is required for keeping information of each registered + * cpufreq_cooling_device. */ struct cpufreq_cooling_device { int id;
This wasn't explained well anywhere and should be clearly specified. Lets add a top level comment for this.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 3ef9050..def0f21 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -28,6 +28,20 @@ #include <linux/cpu.h> #include <linux/cpu_cooling.h>
+/* + * Cooling state <-> CPUFreq frequency + * + * Cooling states are translated to frequencies throughout this driver and this + * is the relation between them. + * + * Highest cooling state corresponds to lowest possible frequency. + * + * i.e. + * level 0 --> 1st Max Freq + * level 1 --> 2nd Max Freq + * ... + */ + /** * struct cpufreq_cooling_device - data for cooling device with cpufreq * @id: unique integer value corresponding to each cpufreq_cooling_device
Just following coding guidelines here.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index def0f21..59725d8 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -468,8 +468,7 @@ __cpufreq_cooling_register(struct device_node *np, return ERR_PTR(-EINVAL); } } - cpufreq_dev = kzalloc(sizeof(struct cpufreq_cooling_device), - GFP_KERNEL); + cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL); if (!cpufreq_dev) return ERR_PTR(-ENOMEM);
Hi Viresh,
On Fri, Nov 28, 2014 at 09:44:02AM +0000, Viresh Kumar wrote:
Just following coding guidelines here.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/thermal/cpu_cooling.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index def0f21..59725d8 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -468,8 +468,7 @@ __cpufreq_cooling_register(struct device_node *np, return ERR_PTR(-EINVAL); } }
- cpufreq_dev = kzalloc(sizeof(struct cpufreq_cooling_device),
GFP_KERNEL);
- cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL);
This should be:
+ cpufreq_dev = kzalloc(sizeof(*cpufreq_dev), GFP_KERNEL);
Cheers, Javi
Hi,
On Tue, Dec 02, 2014 at 03:26:24PM +0000, Javi Merino wrote:
Hi Viresh,
On Fri, Nov 28, 2014 at 09:44:02AM +0000, Viresh Kumar wrote:
Just following coding guidelines here.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/thermal/cpu_cooling.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index def0f21..59725d8 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -468,8 +468,7 @@ __cpufreq_cooling_register(struct device_node *np, return ERR_PTR(-EINVAL); } }
- cpufreq_dev = kzalloc(sizeof(struct cpufreq_cooling_device),
GFP_KERNEL);
- cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL);
This should be:
- cpufreq_dev = kzalloc(sizeof(*cpufreq_dev), GFP_KERNEL);
Agreed.
Cheers, Javi
On 3 December 2014 at 04:37, Eduardo Valentin edubezval@gmail.com wrote:
This should be:
cpufreq_dev = kzalloc(sizeof(*cpufreq_dev), GFP_KERNEL);
This is insulting now. So stupid mistake..
Its already zero, we allocated cpufreq_dev with kzalloc.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 59725d8..cf9d1de 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -491,7 +491,7 @@ __cpufreq_cooling_register(struct device_node *np, return cool_dev; } cpufreq_dev->cool_dev = cool_dev; - cpufreq_dev->cpufreq_state = 0; + mutex_lock(&cooling_cpufreq_lock);
/* Register the notifier for first cpufreq cooling device */
It will be overwritten soon with return value of kzalloc().
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index cf9d1de..bb11dd4 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -448,7 +448,7 @@ __cpufreq_cooling_register(struct device_node *np, const struct cpumask *clip_cpus) { struct thermal_cooling_device *cool_dev; - struct cpufreq_cooling_device *cpufreq_dev = NULL; + struct cpufreq_cooling_device *cpufreq_dev; unsigned int min = 0, max = 0; char dev_name[THERMAL_NAME_LENGTH]; int ret = 0, i;
We aren't supposed to return our own error type here. Return what we got.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index bb11dd4..964586f 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -477,7 +477,7 @@ __cpufreq_cooling_register(struct device_node *np, ret = get_idr(&cpufreq_idr, &cpufreq_dev->id); if (ret) { kfree(cpufreq_dev); - return ERR_PTR(-EINVAL); + return ERR_PTR(cpufreq_dev->id); }
snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d",
On Fri, Nov 28, 2014 at 09:44:05AM +0000, Viresh Kumar wrote:
We aren't supposed to return our own error type here. Return what we got.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/thermal/cpu_cooling.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index bb11dd4..964586f 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -477,7 +477,7 @@ __cpufreq_cooling_register(struct device_node *np, ret = get_idr(&cpufreq_idr, &cpufreq_dev->id); if (ret) { kfree(cpufreq_dev);
return ERR_PTR(-EINVAL);
return ERR_PTR(cpufreq_dev->id);
The error is ret, not the id which is probably 0 if there was an error. So:
+ return ERR_PTR(ret);
Cheers, Javi
On 2 December 2014 at 21:05, Javi Merino javi.merino@arm.com wrote:
The error is ret, not the id which is probably 0 if there was an error. So:
return ERR_PTR(ret);
Where should I hide my face, shameful mistake :(
Hello Viresh,
On Fri, Nov 28, 2014 at 03:14:05PM +0530, Viresh Kumar wrote:
We aren't supposed to return our own error type here. Return what we got.
OK..
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/thermal/cpu_cooling.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index bb11dd4..964586f 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -477,7 +477,7 @@ __cpufreq_cooling_register(struct device_node *np, ret = get_idr(&cpufreq_idr, &cpufreq_dev->id); if (ret) { kfree(cpufreq_dev);
return ERR_PTR(-EINVAL);
return ERR_PTR(cpufreq_dev->id);
cpufreq_dev is an invalid object here. Maybe you want to use ret in your patch?
} snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d", -- 2.0.3.693.g996b0fd
In __cpufreq_cooling_register() we try to match min/max frequencies for all CPUs passed in 'clip_cpus' mask. This mask is the cpumask of cpus where the frequency constraints will be applied.
Same frequency constraint can be applied only to the CPUs belonging to the same cluster (i.e. CPUs sharing clock line). For all such CPUs we have a single 'struct cpufreq_policy' structure managing them and so getting policies for all CPUs wouldn't make any sense as they will all return the same pointer.
So, remove this useless check of checking min/max for all CPUs. Also update doc comment to make this more obvious that clip_cpus should be same as policy->related_cpus.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 964586f..c358ede 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -434,6 +434,7 @@ static struct notifier_block thermal_cpufreq_notifier_block = { * __cpufreq_cooling_register - helper function to create cpufreq cooling device * @np: a valid struct device_node to the cooling device device tree node * @clip_cpus: cpumask of cpus where the frequency constraints will happen. + * Normally this should be same as cpufreq policy->related_cpus. * * This interface function registers the cpufreq cooling device with the name * "thermal-cpufreq-%x". This api can support multiple instances of cpufreq @@ -449,25 +450,9 @@ __cpufreq_cooling_register(struct device_node *np, { struct thermal_cooling_device *cool_dev; struct cpufreq_cooling_device *cpufreq_dev; - unsigned int min = 0, max = 0; char dev_name[THERMAL_NAME_LENGTH]; - int ret = 0, i; - struct cpufreq_policy policy; + int ret = 0;
- /* Verify that all the clip cpus have same freq_min, freq_max limit */ - for_each_cpu(i, clip_cpus) { - /* continue if cpufreq policy not found and not return error */ - if (!cpufreq_get_policy(&policy, i)) - continue; - if (min == 0 && max == 0) { - min = policy.cpuinfo.min_freq; - max = policy.cpuinfo.max_freq; - } else { - if (min != policy.cpuinfo.min_freq || - max != policy.cpuinfo.max_freq) - return ERR_PTR(-EINVAL); - } - } cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL); if (!cpufreq_dev) return ERR_PTR(-ENOMEM);
All CPUs present in 'allowed_cpus' share the same 'struct cpufreq_policy' structure and so calling cpufreq_update_policy() for each of them doesn't make sense.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index c358ede..8ca0380 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -286,11 +286,10 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device, unsigned long cooling_state) { - unsigned int cpuid, clip_freq; + unsigned int clip_freq; struct cpumask *mask = &cpufreq_device->allowed_cpus; unsigned int cpu = cpumask_any(mask);
- /* Check if the old cooling action is same as new cooling action */ if (cpufreq_device->cpufreq_state == cooling_state) return 0; @@ -303,10 +302,8 @@ static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device, cpufreq_device->cpufreq_val = clip_freq; notify_device = cpufreq_device;
- for_each_cpu(cpuid, mask) { - if (is_cpufreq_valid(cpuid)) - cpufreq_update_policy(cpuid); - } + if (is_cpufreq_valid(cpu)) + cpufreq_update_policy(cpu);
notify_device = NOTIFY_INVALID;
Because get_cpu_frequency() has returned a valid frequency, it means that the cpufreq policy is surely valid and so no point checking that again with is_cpufreq_valid(). Get rid of the routine as well as there are no more users.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 8ca0380..41aa7d5 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -111,23 +111,6 @@ static void release_idr(struct idr *idr, int id)
/* Below code defines functions to be used for cpufreq as cooling device */
-/** - * is_cpufreq_valid - function to check frequency transitioning capability. - * @cpu: cpu for which check is needed. - * - * This function will check the current state of the system if - * it is capable of changing the frequency for a given @cpu. - * - * Return: 0 if the system is not currently capable of changing - * the frequency of given cpu. !0 in case the frequency is changeable. - */ -static int is_cpufreq_valid(int cpu) -{ - struct cpufreq_policy policy; - - return !cpufreq_get_policy(&policy, cpu); -} - enum cpufreq_cooling_property { GET_LEVEL, GET_FREQ, @@ -302,8 +285,7 @@ static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device, cpufreq_device->cpufreq_val = clip_freq; notify_device = cpufreq_device;
- if (is_cpufreq_valid(cpu)) - cpufreq_update_policy(cpu); + cpufreq_update_policy(cpu);
notify_device = NOTIFY_INVALID;
This makes life easy and bug free. And is scalable for future resource allocations.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 41aa7d5..032cba3 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -440,8 +440,8 @@ __cpufreq_cooling_register(struct device_node *np,
ret = get_idr(&cpufreq_idr, &cpufreq_dev->id); if (ret) { - kfree(cpufreq_dev); - return ERR_PTR(cpufreq_dev->id); + cool_dev = ERR_PTR(cpufreq_dev->id); + goto free_cdev; }
snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d", @@ -449,11 +449,9 @@ __cpufreq_cooling_register(struct device_node *np,
cool_dev = thermal_of_cooling_device_register(np, dev_name, cpufreq_dev, &cpufreq_cooling_ops); - if (IS_ERR(cool_dev)) { - release_idr(&cpufreq_idr, cpufreq_dev->id); - kfree(cpufreq_dev); - return cool_dev; - } + if (IS_ERR(cool_dev)) + goto remove_idr; + cpufreq_dev->cool_dev = cool_dev;
mutex_lock(&cooling_cpufreq_lock); @@ -467,6 +465,13 @@ __cpufreq_cooling_register(struct device_node *np, mutex_unlock(&cooling_cpufreq_lock);
return cool_dev; + +remove_idr: + release_idr(&cpufreq_idr, cpufreq_dev->id); +free_cdev: + kfree(cpufreq_dev); + + return cool_dev; }
/**
On Fri, Nov 28, 2014 at 09:44:09AM +0000, Viresh Kumar wrote:
This makes life easy and bug free. And is scalable for future resource allocations.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Yay! We have a similar fix as part of our patches for the power allocator governor (the only difference is that the instead of calling the label "remove_idr", we call it "release_idr"). So quite happy to see this going in. FWIW,
Acked-by: Javi Merino javi.merino@arm.com
Locking around idr_alloc/idr_remove looks rather pointless as there is no race it is trying to fix. Remove it.
get_idr() and release_idr() aren't much useful now, so get rid of them as well.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- @Eduardo: Same is true for thermal-core as well ? --- drivers/thermal/cpu_cooling.c | 45 ++++--------------------------------------- 1 file changed, 4 insertions(+), 41 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 032cba3..ca8f1bb 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -73,42 +73,6 @@ static unsigned int cpufreq_dev_count; #define NOTIFY_INVALID NULL static struct cpufreq_cooling_device *notify_device;
-/** - * get_idr - function to get a unique id. - * @idr: struct idr * handle used to create a id. - * @id: int * value generated by this function. - * - * This function will populate @id with an unique - * id, using the idr API. - * - * Return: 0 on success, an error code on failure. - */ -static int get_idr(struct idr *idr, int *id) -{ - int ret; - - mutex_lock(&cooling_cpufreq_lock); - ret = idr_alloc(idr, NULL, 0, 0, GFP_KERNEL); - mutex_unlock(&cooling_cpufreq_lock); - if (unlikely(ret < 0)) - return ret; - *id = ret; - - return 0; -} - -/** - * release_idr - function to free the unique id. - * @idr: struct idr * handle used for creating the id. - * @id: int value representing the unique id. - */ -static void release_idr(struct idr *idr, int id) -{ - mutex_lock(&cooling_cpufreq_lock); - idr_remove(idr, id); - mutex_unlock(&cooling_cpufreq_lock); -} - /* Below code defines functions to be used for cpufreq as cooling device */
enum cpufreq_cooling_property { @@ -430,7 +394,6 @@ __cpufreq_cooling_register(struct device_node *np, struct thermal_cooling_device *cool_dev; struct cpufreq_cooling_device *cpufreq_dev; char dev_name[THERMAL_NAME_LENGTH]; - int ret = 0;
cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL); if (!cpufreq_dev) @@ -438,8 +401,8 @@ __cpufreq_cooling_register(struct device_node *np,
cpumask_copy(&cpufreq_dev->allowed_cpus, clip_cpus);
- ret = get_idr(&cpufreq_idr, &cpufreq_dev->id); - if (ret) { + cpufreq_dev->id = idr_alloc(&cpufreq_idr, NULL, 0, 0, GFP_KERNEL); + if (unlikely(cpufreq_dev->id < 0)) { cool_dev = ERR_PTR(cpufreq_dev->id); goto free_cdev; } @@ -467,7 +430,7 @@ __cpufreq_cooling_register(struct device_node *np, return cool_dev;
remove_idr: - release_idr(&cpufreq_idr, cpufreq_dev->id); + idr_remove(&cpufreq_idr, cpufreq_dev->id); free_cdev: kfree(cpufreq_dev);
@@ -540,7 +503,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) mutex_unlock(&cooling_cpufreq_lock);
thermal_cooling_device_unregister(cpufreq_dev->cool_dev); - release_idr(&cpufreq_idr, cpufreq_dev->id); + idr_remove(&cpufreq_idr, cpufreq_dev->id); kfree(cpufreq_dev); } EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister);
Hi Viresh,
On Fri, Nov 28, 2014 at 09:44:10AM +0000, Viresh Kumar wrote:
Locking around idr_alloc/idr_remove looks rather pointless as there is no race it is trying to fix. Remove it.
You are assuming that all cpufreq cooling devices are registered sequentially, one after the other. That doesn't need be the case. I don't think the performance that you get from this patch justifies the possible races that could be introduced by not having the locking. Why should we remove this?
get_idr() and release_idr() aren't much useful now, so get rid of them as well.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
@Eduardo: Same is true for thermal-core as well ?
I think that my previous concern applies to thermal_core as well, thermal zones may not be initialised sequentially.
Cheers, Javi
On Fri, Nov 28, 2014 at 03:14:10PM +0530, Viresh Kumar wrote:
Locking around idr_alloc/idr_remove looks rather pointless as there is no race it is trying to fix. Remove it.
Can you please elaborate on how the idr's are going to be serialize without the lock?
get_idr() and release_idr() aren't much useful now, so get rid of them as well.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
@Eduardo: Same is true for thermal-core as well ?
Probably not ?
drivers/thermal/cpu_cooling.c | 45 ++++--------------------------------------- 1 file changed, 4 insertions(+), 41 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 032cba3..ca8f1bb 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -73,42 +73,6 @@ static unsigned int cpufreq_dev_count; #define NOTIFY_INVALID NULL static struct cpufreq_cooling_device *notify_device; -/**
- get_idr - function to get a unique id.
- @idr: struct idr * handle used to create a id.
- @id: int * value generated by this function.
- This function will populate @id with an unique
- id, using the idr API.
- Return: 0 on success, an error code on failure.
- */
-static int get_idr(struct idr *idr, int *id) -{
- int ret;
- mutex_lock(&cooling_cpufreq_lock);
- ret = idr_alloc(idr, NULL, 0, 0, GFP_KERNEL);
- mutex_unlock(&cooling_cpufreq_lock);
- if (unlikely(ret < 0))
return ret;- *id = ret;
- return 0;
-}
-/**
- release_idr - function to free the unique id.
- @idr: struct idr * handle used for creating the id.
- @id: int value representing the unique id.
- */
-static void release_idr(struct idr *idr, int id) -{
- mutex_lock(&cooling_cpufreq_lock);
- idr_remove(idr, id);
- mutex_unlock(&cooling_cpufreq_lock);
-}
/* Below code defines functions to be used for cpufreq as cooling device */ enum cpufreq_cooling_property { @@ -430,7 +394,6 @@ __cpufreq_cooling_register(struct device_node *np, struct thermal_cooling_device *cool_dev; struct cpufreq_cooling_device *cpufreq_dev; char dev_name[THERMAL_NAME_LENGTH];
- int ret = 0;
cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL); if (!cpufreq_dev) @@ -438,8 +401,8 @@ __cpufreq_cooling_register(struct device_node *np, cpumask_copy(&cpufreq_dev->allowed_cpus, clip_cpus);
- ret = get_idr(&cpufreq_idr, &cpufreq_dev->id);
- if (ret) {
- cpufreq_dev->id = idr_alloc(&cpufreq_idr, NULL, 0, 0, GFP_KERNEL);
- if (unlikely(cpufreq_dev->id < 0)) { cool_dev = ERR_PTR(cpufreq_dev->id); goto free_cdev; }
@@ -467,7 +430,7 @@ __cpufreq_cooling_register(struct device_node *np, return cool_dev; remove_idr:
- release_idr(&cpufreq_idr, cpufreq_dev->id);
- idr_remove(&cpufreq_idr, cpufreq_dev->id);
free_cdev: kfree(cpufreq_dev); @@ -540,7 +503,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) mutex_unlock(&cooling_cpufreq_lock); thermal_cooling_device_unregister(cpufreq_dev->cool_dev);
- release_idr(&cpufreq_idr, cpufreq_dev->id);
- idr_remove(&cpufreq_idr, cpufreq_dev->id); kfree(cpufreq_dev);
} EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister); -- 2.0.3.693.g996b0fd
On 3 December 2014 at 04:35, Eduardo Valentin edubezval@gmail.com wrote:
On Fri, Nov 28, 2014 at 03:14:10PM +0530, Viresh Kumar wrote:
Locking around idr_alloc/idr_remove looks rather pointless as there is no race it is trying to fix. Remove it.
Can you please elaborate on how the idr's are going to be serialize without the lock?
Dropped this patch, I was wrong ..
cpufreq_apply_cooling() has a single caller, cpufreq_set_cur_state() and cpufreq_set_cur_state() is an unnecessary wrapper over cpufreq_apply_cooling().
Get rid of it by merging both routines.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 58 ++++++++++++++----------------------------- 1 file changed, 19 insertions(+), 39 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index ca8f1bb..9673b48 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -219,44 +219,6 @@ static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) }
/** - * cpufreq_apply_cooling - function to apply frequency clipping. - * @cpufreq_device: cpufreq_cooling_device pointer containing frequency - * clipping data. - * @cooling_state: value of the cooling state. - * - * Function used to make sure the cpufreq layer is aware of current thermal - * limits. The limits are applied by updating the cpufreq policy. - * - * Return: 0 on success, an error code otherwise (-EINVAL in case wrong - * cooling state). - */ -static int cpufreq_apply_cooling(struct cpufreq_cooling_device *cpufreq_device, - unsigned long cooling_state) -{ - unsigned int clip_freq; - struct cpumask *mask = &cpufreq_device->allowed_cpus; - unsigned int cpu = cpumask_any(mask); - - /* Check if the old cooling action is same as new cooling action */ - if (cpufreq_device->cpufreq_state == cooling_state) - return 0; - - clip_freq = get_cpu_frequency(cpu, cooling_state); - if (!clip_freq) - return -EINVAL; - - cpufreq_device->cpufreq_state = cooling_state; - cpufreq_device->cpufreq_val = clip_freq; - notify_device = cpufreq_device; - - cpufreq_update_policy(cpu); - - notify_device = NOTIFY_INVALID; - - return 0; -} - -/** * cpufreq_thermal_notifier - notifier callback for cpufreq policy change. * @nb: struct notifier_block * with callback info. * @event: value showing cpufreq event for which this function invoked. @@ -357,8 +319,26 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state) { struct cpufreq_cooling_device *cpufreq_device = cdev->devdata; + unsigned int cpu = cpumask_any(&cpufreq_device->allowed_cpus); + unsigned int clip_freq;
- return cpufreq_apply_cooling(cpufreq_device, state); + /* Check if the old cooling action is same as new cooling action */ + if (cpufreq_device->cpufreq_state == state) + return 0; + + clip_freq = get_cpu_frequency(cpu, state); + if (!clip_freq) + return -EINVAL; + + cpufreq_device->cpufreq_state = state; + cpufreq_device->cpufreq_val = clip_freq; + notify_device = cpufreq_device; + + cpufreq_update_policy(cpu); + + notify_device = NOTIFY_INVALID; + + return 0; }
/* Bind cpufreq callbacks to thermal cooling device ops */
get_cpu_frequency() is used from only one place, i.e. cpufreq_set_cur_state(). And there is no need to have this extra level of function calls. Merge them.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 31 ++++--------------------------- 1 file changed, 4 insertions(+), 27 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 9673b48..5815abf 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -195,30 +195,6 @@ unsigned long cpufreq_cooling_get_level(unsigned int cpu, unsigned int freq) EXPORT_SYMBOL_GPL(cpufreq_cooling_get_level);
/** - * get_cpu_frequency - get the absolute value of frequency from level. - * @cpu: cpu for which frequency is fetched. - * @level: cooling level - * - * This function matches cooling level with frequency. Based on a cooling level - * of frequency, equals cooling state of cpu cooling device, it will return - * the corresponding frequency. - * e.g level=0 --> 1st MAX FREQ, level=1 ---> 2nd MAX FREQ, .... etc - * - * Return: 0 on error, the corresponding frequency otherwise. - */ -static unsigned int get_cpu_frequency(unsigned int cpu, unsigned long level) -{ - int ret = 0; - unsigned int freq; - - ret = get_property(cpu, level, &freq, GET_FREQ); - if (ret) - return 0; - - return freq; -} - -/** * cpufreq_thermal_notifier - notifier callback for cpufreq policy change. * @nb: struct notifier_block * with callback info. * @event: value showing cpufreq event for which this function invoked. @@ -321,14 +297,15 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev, struct cpufreq_cooling_device *cpufreq_device = cdev->devdata; unsigned int cpu = cpumask_any(&cpufreq_device->allowed_cpus); unsigned int clip_freq; + int ret = 0;
/* Check if the old cooling action is same as new cooling action */ if (cpufreq_device->cpufreq_state == state) return 0;
- clip_freq = get_cpu_frequency(cpu, state); - if (!clip_freq) - return -EINVAL; + ret = get_property(cpu, state, &clip_freq, GET_FREQ); + if (ret) + return ret;
cpufreq_device->cpufreq_state = state; cpufreq_device->cpufreq_val = clip_freq;
CPU frequency tables don't update after the driver is registered and so we don't need to iterate over them to find total number of states every time cpufreq_get_max_state() is called. Do it once at boot time.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 5815abf..05712d5 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -52,6 +52,8 @@ * cooling devices. * @cpufreq_val: integer value representing the absolute value of the clipped * frequency. + * @max_level: maximum cooling level. One less than total number of valid + * cpufreq frequencies. * @allowed_cpus: all the cpus involved for this cpufreq_cooling_device. * * This structure is required for keeping information of each registered @@ -62,6 +64,7 @@ struct cpufreq_cooling_device { struct thermal_cooling_device *cool_dev; unsigned int cpufreq_state; unsigned int cpufreq_val; + unsigned int max_level; struct cpumask allowed_cpus; }; static DEFINE_IDR(cpufreq_idr); @@ -246,19 +249,9 @@ static int cpufreq_get_max_state(struct thermal_cooling_device *cdev, unsigned long *state) { struct cpufreq_cooling_device *cpufreq_device = cdev->devdata; - struct cpumask *mask = &cpufreq_device->allowed_cpus; - unsigned int cpu; - unsigned int count = 0; - int ret;
- cpu = cpumask_any(mask); - - ret = get_property(cpu, 0, &count, GET_MAXL); - - if (count > 0) - *state = count; - - return ret; + *state = cpufreq_device->max_level; + return 0; }
/** @@ -351,11 +344,25 @@ __cpufreq_cooling_register(struct device_node *np, struct thermal_cooling_device *cool_dev; struct cpufreq_cooling_device *cpufreq_dev; char dev_name[THERMAL_NAME_LENGTH]; + struct cpufreq_frequency_table *pos, *table; + + table = cpufreq_frequency_get_table(cpumask_first(clip_cpus)); + if (!table) { + pr_debug("%s: CPUFreq table not found\n", __func__); + return ERR_PTR(-EPROBE_DEFER); + }
cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL); if (!cpufreq_dev) return ERR_PTR(-ENOMEM);
+ /* Find max levels */ + cpufreq_for_each_valid_entry(pos, table) + cpufreq_dev->max_level++; + + /* max_level is an index, not a counter */ + cpufreq_dev->max_level--; + cpumask_copy(&cpufreq_dev->allowed_cpus, clip_cpus);
cpufreq_dev->id = idr_alloc(&cpufreq_idr, NULL, 0, 0, GFP_KERNEL);
Viresh
On Fri, Nov 28, 2014 at 03:14:13PM +0530, Viresh Kumar wrote:
CPU frequency tables don't update after the driver is registered and so we don't need to iterate over them to find total number of states every time cpufreq_get_max_state() is called. Do it once at boot time.
Could you please update me regarding the story behind the opp del patch set?
http://permalink.gmane.org/gmane.linux.power-management.general/53348
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/thermal/cpu_cooling.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 5815abf..05712d5 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -52,6 +52,8 @@
- cooling devices.
- @cpufreq_val: integer value representing the absolute value of the clipped
- frequency.
- @max_level: maximum cooling level. One less than total number of valid
- cpufreq frequencies.
- @allowed_cpus: all the cpus involved for this cpufreq_cooling_device.
- This structure is required for keeping information of each registered
@@ -62,6 +64,7 @@ struct cpufreq_cooling_device { struct thermal_cooling_device *cool_dev; unsigned int cpufreq_state; unsigned int cpufreq_val;
- unsigned int max_level; struct cpumask allowed_cpus;
}; static DEFINE_IDR(cpufreq_idr); @@ -246,19 +249,9 @@ static int cpufreq_get_max_state(struct thermal_cooling_device *cdev, unsigned long *state) { struct cpufreq_cooling_device *cpufreq_device = cdev->devdata;
- struct cpumask *mask = &cpufreq_device->allowed_cpus;
- unsigned int cpu;
- unsigned int count = 0;
- int ret;
- cpu = cpumask_any(mask);
- ret = get_property(cpu, 0, &count, GET_MAXL);
- if (count > 0)
*state = count;- return ret;
- *state = cpufreq_device->max_level;
- return 0;
} /** @@ -351,11 +344,25 @@ __cpufreq_cooling_register(struct device_node *np, struct thermal_cooling_device *cool_dev; struct cpufreq_cooling_device *cpufreq_dev; char dev_name[THERMAL_NAME_LENGTH];
- struct cpufreq_frequency_table *pos, *table;
- table = cpufreq_frequency_get_table(cpumask_first(clip_cpus));
- if (!table) {
pr_debug("%s: CPUFreq table not found\n", __func__);return ERR_PTR(-EPROBE_DEFER);- }
cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL); if (!cpufreq_dev) return ERR_PTR(-ENOMEM);
- /* Find max levels */
- cpufreq_for_each_valid_entry(pos, table)
cpufreq_dev->max_level++;- /* max_level is an index, not a counter */
- cpufreq_dev->max_level--;
- cpumask_copy(&cpufreq_dev->allowed_cpus, clip_cpus);
cpufreq_dev->id = idr_alloc(&cpufreq_idr, NULL, 0, 0, GFP_KERNEL); -- 2.0.3.693.g996b0fd
On 3 December 2014 at 05:09, Eduardo Valentin edubezval@gmail.com wrote:
Could you please update me regarding the story behind the opp del patch set?
http://permalink.gmane.org/gmane.linux.power-management.general/53348
Its merged by Rafael, should be in next now.
On Wed, Dec 03, 2014 at 10:27:43AM +0530, Viresh Kumar wrote:
On 3 December 2014 at 05:09, Eduardo Valentin edubezval@gmail.com wrote:
Could you please update me regarding the story behind the opp del patch set?
http://permalink.gmane.org/gmane.linux.power-management.general/53348
Its merged by Rafael, should be in next now.
OK. Good. But does it mean we can remove OPPs after cpufreq registration? Meaning, is the cpufreq table changing over time?
BR,
Eduardo Valentin
On Wednesday, December 3, 2014, Eduardo Valentin edubezval@gmail.com wrote:
OK. Good. But does it mean we can remove OPPs after cpufreq registration? Meaning, is the cpufreq table changing over time?
Opps can be marked unavailable but not removed while they are used. And cpufreq table can't be changed over time.
We don't use get_property() to find max levels anymore as it is done at boot now. So, don't support GET_MAXL in get_property().
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 05712d5..ddb97aa 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -81,7 +81,6 @@ static struct cpufreq_cooling_device *notify_device; enum cpufreq_cooling_property { GET_LEVEL, GET_FREQ, - GET_MAXL, };
/** @@ -89,12 +88,11 @@ enum cpufreq_cooling_property { * @cpu: cpu for which the property is required * @input: query parameter * @output: query return - * @property: type of query (frequency, level, max level) + * @property: type of query (frequency, level) * * This is the common function to - * 1. get maximum cpu cooling states - * 2. translate frequency to cooling state - * 3. translate cooling state to frequency + * 1. translate frequency to cooling state + * 2. translate cooling state to frequency * * Note that the code may be not in good shape * but it is written in this way in order to: @@ -141,12 +139,6 @@ static int get_property(unsigned int cpu, unsigned long input, /* max_level is an index, not a counter */ max_level--;
- /* get max level */ - if (property == GET_MAXL) { - *output = (unsigned int)max_level; - return 0; - } - if (property == GET_FREQ) level = descend ? input : (max_level - input);
That will be used by later patches to iterate over all cpufreq cooling devices.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index ddb97aa..f76a665 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -66,8 +66,11 @@ struct cpufreq_cooling_device { unsigned int cpufreq_val; unsigned int max_level; struct cpumask allowed_cpus; + struct list_head head; }; + static DEFINE_IDR(cpufreq_idr); +static LIST_HEAD(cpufreq_dev_list); static DEFINE_MUTEX(cooling_cpufreq_lock);
static unsigned int cpufreq_dev_count; @@ -372,6 +375,7 @@ __cpufreq_cooling_register(struct device_node *np, goto remove_idr;
cpufreq_dev->cool_dev = cool_dev; + INIT_LIST_HEAD(&cpufreq_dev->head);
mutex_lock(&cooling_cpufreq_lock);
@@ -381,6 +385,7 @@ __cpufreq_cooling_register(struct device_node *np, CPUFREQ_POLICY_NOTIFIER); cpufreq_dev_count++;
+ list_add(&cpufreq_dev->head, &cpufreq_dev_list); mutex_unlock(&cooling_cpufreq_lock);
return cool_dev; @@ -451,6 +456,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) cpufreq_dev = cdev->devdata; mutex_lock(&cooling_cpufreq_lock); cpufreq_dev_count--; + list_del(&cpufreq_dev->head);
/* Unregister the notifier for the last cpufreq cooling device */ if (cpufreq_dev_count == 0)
Viresh,
On Fri, Nov 28, 2014 at 03:14:15PM +0530, Viresh Kumar wrote:
That will be used by later patches to iterate over all cpufreq cooling devices.
Can you please refresh the series on top of latest Linus tree (material for 3.18-rc7) ?
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/thermal/cpu_cooling.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index ddb97aa..f76a665 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -66,8 +66,11 @@ struct cpufreq_cooling_device { unsigned int cpufreq_val; unsigned int max_level; struct cpumask allowed_cpus;
- struct list_head head;
Yadwinder has already introduced a list of cpu cooling devs: commit 2dcd851fe4b4fe60c2f8520bf7668d7e9b2dd76b Author: Yadwinder Singh Brar yadi.brar@samsung.com Date: Fri Nov 7 19:12:29 2014 +0530
thermal: cpu_cooling: Update always cpufreq policy with thermal constraints
};
static DEFINE_IDR(cpufreq_idr); +static LIST_HEAD(cpufreq_dev_list); static DEFINE_MUTEX(cooling_cpufreq_lock); static unsigned int cpufreq_dev_count; @@ -372,6 +375,7 @@ __cpufreq_cooling_register(struct device_node *np, goto remove_idr; cpufreq_dev->cool_dev = cool_dev;
- INIT_LIST_HEAD(&cpufreq_dev->head);
mutex_lock(&cooling_cpufreq_lock); @@ -381,6 +385,7 @@ __cpufreq_cooling_register(struct device_node *np, CPUFREQ_POLICY_NOTIFIER); cpufreq_dev_count++;
- list_add(&cpufreq_dev->head, &cpufreq_dev_list); mutex_unlock(&cooling_cpufreq_lock);
return cool_dev; @@ -451,6 +456,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) cpufreq_dev = cdev->devdata; mutex_lock(&cooling_cpufreq_lock); cpufreq_dev_count--;
- list_del(&cpufreq_dev->head);
/* Unregister the notifier for the last cpufreq cooling device */ if (cpufreq_dev_count == 0) -- 2.0.3.693.g996b0fd
As we already have a list of cpufreq_cooling_devices now, lets use it instead of a local counter.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index f76a665..4965a55 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -73,8 +73,6 @@ static DEFINE_IDR(cpufreq_idr); static LIST_HEAD(cpufreq_dev_list); static DEFINE_MUTEX(cooling_cpufreq_lock);
-static unsigned int cpufreq_dev_count; - /* notify_table passes value to the CPUFREQ_ADJUST callback function. */ #define NOTIFY_INVALID NULL static struct cpufreq_cooling_device *notify_device; @@ -380,10 +378,9 @@ __cpufreq_cooling_register(struct device_node *np, mutex_lock(&cooling_cpufreq_lock);
/* Register the notifier for first cpufreq cooling device */ - if (cpufreq_dev_count == 0) + if (list_empty(&cpufreq_dev_list)) cpufreq_register_notifier(&thermal_cpufreq_notifier_block, CPUFREQ_POLICY_NOTIFIER); - cpufreq_dev_count++;
list_add(&cpufreq_dev->head, &cpufreq_dev_list); mutex_unlock(&cooling_cpufreq_lock); @@ -455,11 +452,10 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
cpufreq_dev = cdev->devdata; mutex_lock(&cooling_cpufreq_lock); - cpufreq_dev_count--; list_del(&cpufreq_dev->head);
/* Unregister the notifier for the last cpufreq cooling device */ - if (cpufreq_dev_count == 0) + if (list_empty(&cpufreq_dev_list)) cpufreq_unregister_notifier(&thermal_cpufreq_notifier_block, CPUFREQ_POLICY_NOTIFIER); mutex_unlock(&cooling_cpufreq_lock);
We already know the value of 'cpufreq_dev->max_level' and so there is no need calculating that once again. For this, we need to send 'cpufreq_dev' to get_property().
Make all necessary changes for this change. Because cpufreq_cooling_get_level() doesn't have access to 'cpufreq_dev', it is updated to iterate over the list of cpufreq_cooling_devices to get cooling device for the cpu number passed to it. This also makes it robust to return levels only for the CPU registered via a cooling device. We don't have to support anything that isn't registered yet.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 47 ++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 21 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 4965a55..07414c5 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -86,7 +86,7 @@ enum cpufreq_cooling_property {
/** * get_property - fetch a property of interest for a given cpu. - * @cpu: cpu for which the property is required + * @cpufreq_dev: cpufreq_dev for which the property is required * @input: query parameter * @output: query return * @property: type of query (frequency, level) @@ -103,20 +103,20 @@ enum cpufreq_cooling_property { * * Return: 0 on success, -EINVAL when invalid parameters are passed. */ -static int get_property(unsigned int cpu, unsigned long input, - unsigned int *output, +static int get_property(struct cpufreq_cooling_device *cpufreq_dev, + unsigned long input, unsigned int *output, enum cpufreq_cooling_property property) { int i; - unsigned long max_level = 0, level = 0; + unsigned long level = 0; unsigned int freq = CPUFREQ_ENTRY_INVALID; int descend = -1; - struct cpufreq_frequency_table *pos, *table = - cpufreq_frequency_get_table(cpu); + struct cpufreq_frequency_table *pos, *table;
if (!output) return -EINVAL;
+ table = cpufreq_frequency_get_table(cpumask_first(&cpufreq_dev->allowed_cpus)); if (!table) return -EINVAL;
@@ -130,18 +130,10 @@ static int get_property(unsigned int cpu, unsigned long input, descend = freq > pos->frequency;
freq = pos->frequency; - max_level++; }
- /* No valid cpu frequency entry */ - if (max_level == 0) - return -EINVAL; - - /* max_level is an index, not a counter */ - max_level--; - if (property == GET_FREQ) - level = descend ? input : (max_level - input); + level = descend ? input : (cpufreq_dev->max_level - input);
i = 0; cpufreq_for_each_valid_entry(pos, table) { @@ -154,7 +146,7 @@ static int get_property(unsigned int cpu, unsigned long input,
if (property == GET_LEVEL && (unsigned int)input == freq) { /* get level by frequency */ - *output = descend ? i : (max_level - i); + *output = descend ? i : (cpufreq_dev->max_level - i); return 0; } if (property == GET_FREQ && level == i) { @@ -181,12 +173,25 @@ static int get_property(unsigned int cpu, unsigned long input, */ unsigned long cpufreq_cooling_get_level(unsigned int cpu, unsigned int freq) { - unsigned int val; + struct cpufreq_cooling_device *cpufreq_dev;
- if (get_property(cpu, (unsigned long)freq, &val, GET_LEVEL)) - return THERMAL_CSTATE_INVALID; + mutex_lock(&cooling_cpufreq_lock); + list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, head) { + if (cpumask_test_cpu(cpu, &cpufreq_dev->allowed_cpus)) { + unsigned int val; + + mutex_unlock(&cooling_cpufreq_lock); + if (get_property(cpufreq_dev, (unsigned long)freq, &val, + GET_LEVEL)) + return THERMAL_CSTATE_INVALID; + + return (unsigned long)val; + } + } + mutex_unlock(&cooling_cpufreq_lock);
- return (unsigned long)val; + pr_err("%s: cpu:%d not part of any cooling device\n", __func__, cpu); + return THERMAL_CSTATE_INVALID; } EXPORT_SYMBOL_GPL(cpufreq_cooling_get_level);
@@ -289,7 +294,7 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev, if (cpufreq_device->cpufreq_state == state) return 0;
- ret = get_property(cpu, state, &clip_freq, GET_FREQ); + ret = get_property(cpufreq_device, state, &clip_freq, GET_FREQ); if (ret) return ret;
CPUFreq framework *doesn't* guarantee that frequencies present in cpufreq table will be in ascending or descending order. But cpu_cooling somehow assumes that.
Probably because most of current users are creating this list from DT, which is done with the help of OPP layer. And OPP layer creates the list in ascending order of frequencies.
But cpu_cooling can be used for other platforms too, which don't have frequencies arranged in any order.
This patch tries to fix this issue by creating another list of valid frequencies in descending order. Care is also taken to throw warnings for duplicate entries.
Later patches would use it to simplify code.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 07414c5..9a4a323 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -65,6 +65,7 @@ struct cpufreq_cooling_device { unsigned int cpufreq_state; unsigned int cpufreq_val; unsigned int max_level; + unsigned int *freq_table; /* In descending order */ struct cpumask allowed_cpus; struct list_head head; }; @@ -321,6 +322,20 @@ static struct notifier_block thermal_cpufreq_notifier_block = { .notifier_call = cpufreq_thermal_notifier, };
+static unsigned int find_next_max(struct cpufreq_frequency_table *table, + unsigned int prev_max) +{ + struct cpufreq_frequency_table *pos; + unsigned int max = 0; + + cpufreq_for_each_valid_entry(pos, table) { + if (pos->frequency > max && pos->frequency < prev_max) + max = pos->frequency; + } + + return max; +} + /** * __cpufreq_cooling_register - helper function to create cpufreq cooling device * @np: a valid struct device_node to the cooling device device tree node @@ -343,6 +358,7 @@ __cpufreq_cooling_register(struct device_node *np, struct cpufreq_cooling_device *cpufreq_dev; char dev_name[THERMAL_NAME_LENGTH]; struct cpufreq_frequency_table *pos, *table; + unsigned int freq, i;
table = cpufreq_frequency_get_table(cpumask_first(clip_cpus)); if (!table) { @@ -358,6 +374,14 @@ __cpufreq_cooling_register(struct device_node *np, cpufreq_for_each_valid_entry(pos, table) cpufreq_dev->max_level++;
+ cpufreq_dev->freq_table = kmalloc(sizeof(*cpufreq_dev->freq_table) * + cpufreq_dev->max_level, GFP_KERNEL); + if (!cpufreq_dev->freq_table) { + return ERR_PTR(-ENOMEM); + cool_dev = ERR_PTR(-ENOMEM); + goto free_cdev; + } + /* max_level is an index, not a counter */ cpufreq_dev->max_level--;
@@ -366,7 +390,7 @@ __cpufreq_cooling_register(struct device_node *np, cpufreq_dev->id = idr_alloc(&cpufreq_idr, NULL, 0, 0, GFP_KERNEL); if (unlikely(cpufreq_dev->id < 0)) { cool_dev = ERR_PTR(cpufreq_dev->id); - goto free_cdev; + goto free_table; }
snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d", @@ -377,6 +401,18 @@ __cpufreq_cooling_register(struct device_node *np, if (IS_ERR(cool_dev)) goto remove_idr;
+ /* Fill freq-table in descending order of frequencies */ + for (i = 0, freq = -1; i <= cpufreq_dev->max_level; i++) { + freq = find_next_max(table, freq); + cpufreq_dev->freq_table[i] = freq; + + /* Warn for duplicate entries */ + if (!freq) + pr_warn("%s: table has duplicate entries\n", __func__); + else + pr_debug("%s: freq:%u KHz\n", __func__, freq); + } + cpufreq_dev->cool_dev = cool_dev; INIT_LIST_HEAD(&cpufreq_dev->head);
@@ -394,6 +430,8 @@ __cpufreq_cooling_register(struct device_node *np,
remove_idr: idr_remove(&cpufreq_idr, cpufreq_dev->id); +free_table: + kfree(cpufreq_dev->freq_table); free_cdev: kfree(cpufreq_dev);
@@ -467,6 +505,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
thermal_cooling_device_unregister(cpufreq_dev->cool_dev); idr_remove(&cpufreq_idr, cpufreq_dev->id); + kfree(cpufreq_dev->freq_table); kfree(cpufreq_dev); } EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister);
Viresh,
On Fri, Nov 28, 2014 at 03:14:18PM +0530, Viresh Kumar wrote:
CPUFreq framework *doesn't* guarantee that frequencies present in cpufreq table will be in ascending or descending order. But cpu_cooling somehow assumes that.
Probably because most of current users are creating this list from DT, which is done with the help of OPP layer. And OPP layer creates the list in ascending order of frequencies.
But cpu_cooling can be used for other platforms too, which don't have frequencies arranged in any order.
This patch tries to fix this issue by creating another list of valid frequencies in descending order. Care is also taken to throw warnings for duplicate entries.
Later patches would use it to simplify code.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/thermal/cpu_cooling.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 07414c5..9a4a323 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -65,6 +65,7 @@ struct cpufreq_cooling_device { unsigned int cpufreq_state; unsigned int cpufreq_val; unsigned int max_level;
- unsigned int *freq_table; /* In descending order */ struct cpumask allowed_cpus; struct list_head head;
}; @@ -321,6 +322,20 @@ static struct notifier_block thermal_cpufreq_notifier_block = { .notifier_call = cpufreq_thermal_notifier, }; +static unsigned int find_next_max(struct cpufreq_frequency_table *table,
unsigned int prev_max)+{
- struct cpufreq_frequency_table *pos;
- unsigned int max = 0;
- cpufreq_for_each_valid_entry(pos, table) {
if (pos->frequency > max && pos->frequency < prev_max)
What happens if, for some random reason, the cpufreq table is in ascending order and this function is called with prev_max == (unsigned int) -1 ? What would be the returned max?
max = pos->frequency;- }
- return max;
+}
/**
- __cpufreq_cooling_register - helper function to create cpufreq cooling device
- @np: a valid struct device_node to the cooling device device tree node
@@ -343,6 +358,7 @@ __cpufreq_cooling_register(struct device_node *np, struct cpufreq_cooling_device *cpufreq_dev; char dev_name[THERMAL_NAME_LENGTH]; struct cpufreq_frequency_table *pos, *table;
- unsigned int freq, i;
table = cpufreq_frequency_get_table(cpumask_first(clip_cpus)); if (!table) { @@ -358,6 +374,14 @@ __cpufreq_cooling_register(struct device_node *np, cpufreq_for_each_valid_entry(pos, table) cpufreq_dev->max_level++;
- cpufreq_dev->freq_table = kmalloc(sizeof(*cpufreq_dev->freq_table) *
cpufreq_dev->max_level, GFP_KERNEL);- if (!cpufreq_dev->freq_table) {
return ERR_PTR(-ENOMEM);cool_dev = ERR_PTR(-ENOMEM);goto free_cdev;- }
- /* max_level is an index, not a counter */ cpufreq_dev->max_level--;
@@ -366,7 +390,7 @@ __cpufreq_cooling_register(struct device_node *np, cpufreq_dev->id = idr_alloc(&cpufreq_idr, NULL, 0, 0, GFP_KERNEL); if (unlikely(cpufreq_dev->id < 0)) { cool_dev = ERR_PTR(cpufreq_dev->id);
goto free_cdev;
goto free_table;snprintf(dev_name, sizeof(dev_name), "thermal-cpufreq-%d", @@ -377,6 +401,18 @@ __cpufreq_cooling_register(struct device_node *np, if (IS_ERR(cool_dev)) goto remove_idr;
- /* Fill freq-table in descending order of frequencies */
- for (i = 0, freq = -1; i <= cpufreq_dev->max_level; i++) {
freq = find_next_max(table, freq);cpufreq_dev->freq_table[i] = freq;/* Warn for duplicate entries */if (!freq)pr_warn("%s: table has duplicate entries\n", __func__);elsepr_debug("%s: freq:%u KHz\n", __func__, freq);- }
- cpufreq_dev->cool_dev = cool_dev; INIT_LIST_HEAD(&cpufreq_dev->head);
@@ -394,6 +430,8 @@ __cpufreq_cooling_register(struct device_node *np, remove_idr: idr_remove(&cpufreq_idr, cpufreq_dev->id); +free_table:
- kfree(cpufreq_dev->freq_table);
free_cdev: kfree(cpufreq_dev); @@ -467,6 +505,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) thermal_cooling_device_unregister(cpufreq_dev->cool_dev); idr_remove(&cpufreq_idr, cpufreq_dev->id);
- kfree(cpufreq_dev->freq_table); kfree(cpufreq_dev);
} EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister); -- 2.0.3.693.g996b0fd
On 3 December 2014 at 04:51, Eduardo Valentin edubezval@gmail.com wrote:
+static unsigned int find_next_max(struct cpufreq_frequency_table *table,
unsigned int prev_max)+{
struct cpufreq_frequency_table *pos;unsigned int max = 0;cpufreq_for_each_valid_entry(pos, table) {if (pos->frequency > max && pos->frequency < prev_max)What happens if, for some random reason, the cpufreq table is in ascending order and this function is called with prev_max == (unsigned int) -1 ? What would be the returned max?
The last frequency of the table, i.e. the max value. What bug did you catch, that I am not able to see..
On Wed, Dec 03, 2014 at 10:22:30AM +0530, Viresh Kumar wrote:
On 3 December 2014 at 04:51, Eduardo Valentin edubezval@gmail.com wrote:
+static unsigned int find_next_max(struct cpufreq_frequency_table *table,
unsigned int prev_max)+{
struct cpufreq_frequency_table *pos;unsigned int max = 0;cpufreq_for_each_valid_entry(pos, table) {if (pos->frequency > max && pos->frequency < prev_max)What happens if, for some random reason, the cpufreq table is in ascending order and this function is called with prev_max == (unsigned int) -1 ? What would be the returned max?
The last frequency of the table, i.e. the max value. What bug did you catch, that I am not able to see..
Well, passing -1 to unsigned int will make it max unsigned int, your function will return the first freq, which in the scenario I gave, it would be the min freq, not the first max, right?
On Wednesday, December 3, 2014, Eduardo Valentin edubezval@gmail.com wrote:
Well, passing -1 to unsigned int will make it max unsigned int, your
Yeah, that's what I want.
function will return the first freq, which in the scenario I gave, it would be the min freq, not the first max, right?
Are you sure you are reading the code correctly? Try one more time :)
On Wed, Dec 03, 2014 at 07:26:17PM +0530, Viresh Kumar wrote:
On Wednesday, December 3, 2014, Eduardo Valentin edubezval@gmail.com wrote:
Well, passing -1 to unsigned int will make it max unsigned int, your
Yeah, that's what I want.
function will return the first freq, which in the scenario I gave, it would be the min freq, not the first max, right?
Are you sure you are reading the code correctly? Try one more time :)
Yeah, the code is correct. I need more coffee probably. :-)
get_property() was an over complicated beast with BUGs. It used to believe that cpufreq table is present in ascending or descending order, which might not always be true.
Previous patch has created another freq table in descending order for us and we better use it now. With that get_property() simply goes away and another helper get_level() comes in.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/thermal/cpu_cooling.c | 96 +++++++------------------------------------ 1 file changed, 14 insertions(+), 82 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 9a4a323..db4c001 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -80,85 +80,27 @@ static struct cpufreq_cooling_device *notify_device;
/* Below code defines functions to be used for cpufreq as cooling device */
-enum cpufreq_cooling_property { - GET_LEVEL, - GET_FREQ, -}; - /** - * get_property - fetch a property of interest for a given cpu. + * get_level: Find the level for a particular frequency * @cpufreq_dev: cpufreq_dev for which the property is required - * @input: query parameter - * @output: query return - * @property: type of query (frequency, level) - * - * This is the common function to - * 1. translate frequency to cooling state - * 2. translate cooling state to frequency + * @freq: Frequency * - * Note that the code may be not in good shape - * but it is written in this way in order to: - * a) reduce duplicate code as most of the code can be shared. - * b) make sure the logic is consistent when translating between - * cooling states and frequencies. - * - * Return: 0 on success, -EINVAL when invalid parameters are passed. + * Returns: level on success, THERMAL_CSTATE_INVALID on error. */ -static int get_property(struct cpufreq_cooling_device *cpufreq_dev, - unsigned long input, unsigned int *output, - enum cpufreq_cooling_property property) +static unsigned long get_level(struct cpufreq_cooling_device *cpufreq_dev, + unsigned int freq) { - int i; - unsigned long level = 0; - unsigned int freq = CPUFREQ_ENTRY_INVALID; - int descend = -1; - struct cpufreq_frequency_table *pos, *table; - - if (!output) - return -EINVAL; + unsigned long level;
- table = cpufreq_frequency_get_table(cpumask_first(&cpufreq_dev->allowed_cpus)); - if (!table) - return -EINVAL; + for (level = 0; level <= cpufreq_dev->max_level; level++) { + if (freq == cpufreq_dev->freq_table[level]) + return level;
- cpufreq_for_each_valid_entry(pos, table) { - /* ignore duplicate entry */ - if (freq == pos->frequency) - continue; - - /* get the frequency order */ - if (freq != CPUFREQ_ENTRY_INVALID && descend == -1) - descend = freq > pos->frequency; - - freq = pos->frequency; + if (freq > cpufreq_dev->freq_table[level]) + break; }
- if (property == GET_FREQ) - level = descend ? input : (cpufreq_dev->max_level - input); - - i = 0; - cpufreq_for_each_valid_entry(pos, table) { - /* ignore duplicate entry */ - if (freq == pos->frequency) - continue; - - /* now we have a valid frequency entry */ - freq = pos->frequency; - - if (property == GET_LEVEL && (unsigned int)input == freq) { - /* get level by frequency */ - *output = descend ? i : (cpufreq_dev->max_level - i); - return 0; - } - if (property == GET_FREQ && level == i) { - /* get frequency by level */ - *output = freq; - return 0; - } - i++; - } - - return -EINVAL; + return THERMAL_CSTATE_INVALID; }
/** @@ -179,14 +121,8 @@ unsigned long cpufreq_cooling_get_level(unsigned int cpu, unsigned int freq) mutex_lock(&cooling_cpufreq_lock); list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, head) { if (cpumask_test_cpu(cpu, &cpufreq_dev->allowed_cpus)) { - unsigned int val; - mutex_unlock(&cooling_cpufreq_lock); - if (get_property(cpufreq_dev, (unsigned long)freq, &val, - GET_LEVEL)) - return THERMAL_CSTATE_INVALID; - - return (unsigned long)val; + return get_level(cpufreq_dev, freq); } } mutex_unlock(&cooling_cpufreq_lock); @@ -289,16 +225,12 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev, struct cpufreq_cooling_device *cpufreq_device = cdev->devdata; unsigned int cpu = cpumask_any(&cpufreq_device->allowed_cpus); unsigned int clip_freq; - int ret = 0;
/* Check if the old cooling action is same as new cooling action */ if (cpufreq_device->cpufreq_state == state) return 0;
- ret = get_property(cpufreq_device, state, &clip_freq, GET_FREQ); - if (ret) - return ret; - + clip_freq = cpufreq_device->freq_table[state]; cpufreq_device->cpufreq_state = state; cpufreq_device->cpufreq_val = clip_freq; notify_device = cpufreq_device;
On Fri, Nov 28, 2014 at 03:14:19PM +0530, Viresh Kumar wrote:
get_property() was an over complicated beast with BUGs. It used to believe that cpufreq table is present in ascending or descending order, which might not always be true.
Previous patch has created another freq table in descending order for us and we better use it now. With that get_property() simply goes away and another helper get_level() comes in.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/thermal/cpu_cooling.c | 96 +++++++------------------------------------ 1 file changed, 14 insertions(+), 82 deletions(-)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 9a4a323..db4c001 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -80,85 +80,27 @@ static struct cpufreq_cooling_device *notify_device; /* Below code defines functions to be used for cpufreq as cooling device */ -enum cpufreq_cooling_property {
- GET_LEVEL,
- GET_FREQ,
-};
/**
- get_property - fetch a property of interest for a given cpu.
- get_level: Find the level for a particular frequency
- @cpufreq_dev: cpufreq_dev for which the property is required
- @input: query parameter
- @output: query return
- @property: type of query (frequency, level)
- This is the common function to
- translate frequency to cooling state
- translate cooling state to frequency
- @freq: Frequency
- Note that the code may be not in good shape
- but it is written in this way in order to:
- a) reduce duplicate code as most of the code can be shared.
- b) make sure the logic is consistent when translating between
- cooling states and frequencies.
- Return: 0 on success, -EINVAL when invalid parameters are passed.
*/
- Returns: level on success, THERMAL_CSTATE_INVALID on error.
-static int get_property(struct cpufreq_cooling_device *cpufreq_dev,
unsigned long input, unsigned int *output,enum cpufreq_cooling_property property)+static unsigned long get_level(struct cpufreq_cooling_device *cpufreq_dev,
unsigned int freq){
- int i;
- unsigned long level = 0;
- unsigned int freq = CPUFREQ_ENTRY_INVALID;
- int descend = -1;
- struct cpufreq_frequency_table *pos, *table;
- if (!output)
return -EINVAL;
- unsigned long level;
- table = cpufreq_frequency_get_table(cpumask_first(&cpufreq_dev->allowed_cpus));
- if (!table)
return -EINVAL;
- for (level = 0; level <= cpufreq_dev->max_level; level++) {
if (freq == cpufreq_dev->freq_table[level])return level;
- cpufreq_for_each_valid_entry(pos, table) {
/* ignore duplicate entry */if (freq == pos->frequency)continue;/* get the frequency order */if (freq != CPUFREQ_ENTRY_INVALID && descend == -1)descend = freq > pos->frequency;freq = pos->frequency;
if (freq > cpufreq_dev->freq_table[level])break;
- if (property == GET_FREQ)
level = descend ? input : (cpufreq_dev->max_level - input);- i = 0;
- cpufreq_for_each_valid_entry(pos, table) {
/* ignore duplicate entry */if (freq == pos->frequency)continue;/* now we have a valid frequency entry */freq = pos->frequency;if (property == GET_LEVEL && (unsigned int)input == freq) {/* get level by frequency */*output = descend ? i : (cpufreq_dev->max_level - i);return 0;}if (property == GET_FREQ && level == i) {/* get frequency by level */*output = freq;return 0;}i++;- }
- return -EINVAL;
- return THERMAL_CSTATE_INVALID;
} /** @@ -179,14 +121,8 @@ unsigned long cpufreq_cooling_get_level(unsigned int cpu, unsigned int freq) mutex_lock(&cooling_cpufreq_lock); list_for_each_entry(cpufreq_dev, &cpufreq_dev_list, head) { if (cpumask_test_cpu(cpu, &cpufreq_dev->allowed_cpus)) {
unsigned int val;mutex_unlock(&cooling_cpufreq_lock);if (get_property(cpufreq_dev, (unsigned long)freq, &val,GET_LEVEL))return THERMAL_CSTATE_INVALID;return (unsigned long)val;
return get_level(cpufreq_dev, freq);@@ -289,16 +225,12 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev, struct cpufreq_cooling_device *cpufreq_device = cdev->devdata; unsigned int cpu = cpumask_any(&cpufreq_device->allowed_cpus); unsigned int clip_freq;
- int ret = 0;
/* Check if the old cooling action is same as new cooling action */ if (cpufreq_device->cpufreq_state == state) return 0;
- ret = get_property(cpufreq_device, state, &clip_freq, GET_FREQ);
- if (ret)
return ret;
- clip_freq = cpufreq_device->freq_table[state];
There should be some check for valid state here..
cpufreq_device->cpufreq_state = state; cpufreq_device->cpufreq_val = clip_freq; notify_device = cpufreq_device; -- 2.0.3.693.g996b0fd
On 3 December 2014 at 05:06, Eduardo Valentin edubezval@gmail.com wrote:
ret = get_property(cpufreq_device, state, &clip_freq, GET_FREQ);if (ret)return ret;
clip_freq = cpufreq_device->freq_table[state];There should be some check for valid state here..
What about this ??
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 6bd95bc..6e76417 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -228,6 +228,10 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev, unsigned int cpu = cpumask_any(&cpufreq_device->allowed_cpus); unsigned int clip_freq;
+ /* Request state should be less than max_level */ + if (WARN_ON(state > cpufreq_device->max_level)) + return -EINVAL; + /* Check if the old cooling action is same as new cooling action */ if (cpufreq_device->cpufreq_state == state) return 0;
On Wed, Dec 03, 2014 at 10:40:11AM +0530, Viresh Kumar wrote:
On 3 December 2014 at 05:06, Eduardo Valentin edubezval@gmail.com wrote:
ret = get_property(cpufreq_device, state, &clip_freq, GET_FREQ);if (ret)return ret;
clip_freq = cpufreq_device->freq_table[state];There should be some check for valid state here..
What about this ??
Sounds good to me.
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 6bd95bc..6e76417 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -228,6 +228,10 @@ static int cpufreq_set_cur_state(struct thermal_cooling_device *cdev, unsigned int cpu = cpumask_any(&cpufreq_device->allowed_cpus); unsigned int clip_freq;
/* Request state should be less than max_level */if (WARN_ON(state > cpufreq_device->max_level))return -EINVAL;/* Check if the old cooling action is same as new cooling action */ if (cpufreq_device->cpufreq_state == state) return 0;
Adding my copyright information for two purposes: - To get cc'd for future patches to review (Only if people read this header while sending mail) - Have done enough changes to earn a place here?
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- Drop this patch if you didn't like it Eduardo :) --- drivers/thermal/cpu_cooling.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index db4c001..4369963 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -4,6 +4,8 @@ * Copyright (C) 2012 Samsung Electronics Co., Ltd(http://www.samsung.com) * Copyright (C) 2012 Amit Daniel amit.kachhap@linaro.org * + * Copyright (C) 2014 Viresh Kumar viresh.kumar@linaro.org + * * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by
Hello Viresh K,
On Fri, Nov 28, 2014 at 03:14:20PM +0530, Viresh Kumar wrote:
Adding my copyright information for two purposes:
- To get cc'd for future patches to review (Only if people read this header while sending mail)
For this item, I get_maintainers might copy you due the amount of commit hits, no?
- Have done enough changes to earn a place here?
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Drop this patch if you didn't like it Eduardo :)
I am not against your patch, no.
But, I am probably not the correct person to judge this though. If you get a confirmation from the original copyright holder (amit/samsung), then I am glad to add this.
drivers/thermal/cpu_cooling.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index db4c001..4369963 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -4,6 +4,8 @@
- Copyright (C) 2012 Samsung Electronics Co., Ltd(http://www.samsung.com)
- Copyright (C) 2012 Amit Daniel amit.kachhap@linaro.org
- Copyright (C) 2014 Viresh Kumar viresh.kumar@linaro.org
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
-- 2.0.3.693.g996b0fd
On 3 December 2014 at 01:11, Eduardo Valentin edubezval@gmail.com wrote:
Hello Viresh K,
On Fri, Nov 28, 2014 at 03:14:20PM +0530, Viresh Kumar wrote:
Adding my copyright information for two purposes:
- To get cc'd for future patches to review (Only if people read this header while sending mail)
For this item, I get_maintainers might copy you due the amount of commit hits, no?
- Have done enough changes to earn a place here?
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Drop this patch if you didn't like it Eduardo :)
I am not against your patch, no.
But, I am probably not the correct person to judge this though. If you get a confirmation from the original copyright holder (amit/samsung), then I am glad to add this.
@Amit ?? :)
drivers/thermal/cpu_cooling.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index db4c001..4369963 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -4,6 +4,8 @@
- Copyright (C) 2012 Samsung Electronics Co., Ltd(http://www.samsung.com)
- Copyright (C) 2012 Amit Daniel amit.kachhap@linaro.org
- Copyright (C) 2014 Viresh Kumar viresh.kumar@linaro.org
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
-- 2.0.3.693.g996b0fd
On Wed, Dec 3, 2014 at 10:04 AM, Viresh Kumar viresh.kumar@linaro.org wrote:
On 3 December 2014 at 01:11, Eduardo Valentin edubezval@gmail.com wrote:
Hello Viresh K,
On Fri, Nov 28, 2014 at 03:14:20PM +0530, Viresh Kumar wrote:
Adding my copyright information for two purposes:
- To get cc'd for future patches to review (Only if people read this header while sending mail)
For this item, I get_maintainers might copy you due the amount of commit hits, no?
- Have done enough changes to earn a place here?
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Drop this patch if you didn't like it Eduardo :)
I am not against your patch, no.
But, I am probably not the correct person to judge this though. If you get a confirmation from the original copyright holder (amit/samsung), then I am glad to add this.
@Amit ?? :)
Yes its fine with me.
Regards, Amit
drivers/thermal/cpu_cooling.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index db4c001..4369963 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -4,6 +4,8 @@
- Copyright (C) 2012 Samsung Electronics Co., Ltd(http://www.samsung.com)
- Copyright (C) 2012 Amit Daniel amit.kachhap@linaro.org
- Copyright (C) 2014 Viresh Kumar viresh.kumar@linaro.org
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
-- 2.0.3.693.g996b0fd
-- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Viresh,
On Fri, Nov 28, 2014 at 03:13:54PM +0530, Viresh Kumar wrote:
Hi Eduardo,
As you know I got into fixing cpu_cooling.c due to some cpufreq issues you and Lukasz were struggling with. I found some issues in cpu_cooling then and here are the fixes/cleanups.
Ok. No problem. As I mentioned before, good to have a review in the code by someone with extra experience with cpufreq.
Well, I don't think there was an issue with cpufreq itself. The struggle is with sequencing, specially during booting. The Linux booting sequencing, when it comes to builtin module dependency and probing, does not help much. So, we need to do some tricks with the APIs.
Sorry for the long list. Haven't broken them into smaller sets as most of the patches are very small, easy to review and inter-dependent. Only few of them should take more time to review. If this doesn't work out, let me know and I will try to send separate inter-dependent sets.
I am fine with this approach, because now you are dealing with a single target: refactoring cpu cooling code.
Just apply whatever looks fine and I will update/resend the ones left in V2 if at required.
sure, I will have a look.
First few are updates to platform drivers. Exynos fails to register after few patches in this series as it doesn't handle -EPROBE_DEFER properly (reported that in reply to your patch as well). Others weren't setting clip_cpus properly and are fixed.
OK.
About Exynos, at which point/patch Exynos starts to fail?
As I mentioned in the thread about cpu cooling vs. cpufreq, I prefer to update all users of the updated [of_]cpufreq_cooling_register API, if you don't mind.
Can you please elaborate a little more about how this failure is happening?
Next ones are cleanups of cpu_cooling.c to get things properly organized.
Let me know if I screwed it up completely.
hehehe.. Ok. I will let you know.
Tested-on: Exynos5250 (Dual ARM Cortex A15). Rebased-over: v3.18-rc6 Pushed here: git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/linux.git thermal/cpu-cooling-fixes
Cc: Amit Daniel Kachhap amit.daniel@samsung.com Cc: Chanwoo Choi cw00.choi@samsung.com Cc: Hongbo Zhang hongbo.zhang@linaro.com Cc: Kyungmin Park kyungmin.park@samsung.com Cc: Lukasz Majewski l.majewski@samsung.com Cc: Shawn Guo shawn.guo@linaro.org
Viresh Kumar (26): thermal: db8500: pass cpu_present_mask to cpufreq_cooling_register() thermal: imx: pass cpu_present_mask to cpufreq_cooling_register() thermal: exynos: pass cpu_present_mask to cpufreq_cooling_register() thermal: exynos: Handle -EPROBE_DEFER properly cpu_cooling: random comment fixups cpu_cooling: fix doc comment over struct cpufreq_cooling_device cpu_cooling: Add comment to clarify relation between cooling state and frequency cpu_cooling: Pass variable instead of its type to sizeof() cpu_cooling: no need to set cpufreq_state to zero cpu_cooling: no need to set cpufreq_dev to NULL cpu_cooling: propagate error returned by idr_alloc() cpu_cooling: Don't match min/max frequencies for all CPUs on cooling register cpu_cooling: don't iterate over all allowed_cpus to update cpufreq policy cpu_cooling: Don't check is_cpufreq_valid() cpu_cooling: do error handling at the bottom in __cpufreq_cooling_register() cpu_cooling: Drop useless locking around idr_alloc/idr_remove cpu_cooling: Merge cpufreq_apply_cooling() into cpufreq_set_cur_state() cpu_cooling: Merge get_cpu_frequency() into cpufreq_set_cur_state() cpu_cooling: find max level during device registration cpu_cooling: get_property() doesn't need to support GET_MAXL anymore cpu_cooling: create list of cpufreq_cooling_devices cpu_cooling: use cpufreq_dev_list instead of cpufreq_dev_count cpu_cooling: Pass 'cpufreq_dev' to get_property() cpu_cooling: Store frequencies in descending order cpu_cooling: Use cpufreq_dev->freq_table for finding level/freq cpu_cooling: update copyright tags
drivers/thermal/cpu_cooling.c | 405 +++++++++--------------- drivers/thermal/db8500_cpufreq_cooling.c | 5 +- drivers/thermal/imx_thermal.c | 4 +- drivers/thermal/samsung/exynos_thermal_common.c | 11 +- drivers/thermal/samsung/exynos_tmu.c | 4 +- 5 files changed, 153 insertions(+), 276 deletions(-)
-- 2.0.3.693.g996b0fd
On 28 November 2014 at 18:56, Eduardo Valentin edubezval@gmail.com wrote:
About Exynos, at which point/patch Exynos starts to fail?
e2cb864 cpu_cooling: find max level during device registration
As I mentioned in the thread about cpu cooling vs. cpufreq, I prefer to update all users of the updated [of_]cpufreq_cooling_register API, if you don't mind.
Can you please elaborate a little more about how this failure is happening?
Its the same problem, cpufreq driver isn't ready and cpufreq_frequency_get_table() starts failing :)
linaro-kernel@lists.linaro.org
-
amit daniel kachhap -
Eduardo Valentin -
Javi Merino -
Viresh Kumar