On Friday, January 02, 2015 11:16:50 AM Viresh Kumar wrote:
Locking wasn't handled properly at places and this patch is an attempt to fix it. Specially while creating/removing stat tables.
This is pants, sorry.
Whenever you change locking, you need to know exactly (a) what is wrong with the way it is currently done and (b) how you are going to improve it. All of that needs to be described here.
For example, you seem to be thinking that locking is missing from __cpufreq_stats_free_table(), so you are adding it in there. Thus you need to describe (a) why it is missing (eg. what races are possible because of that) and (b) why adding it the way you do is going to improve the situation.
Reviewed-by: Prarit Bhargava prarit@redhat.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/cpufreq_stats.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c index de55ca86b6f1..95867985ea02 100644 --- a/drivers/cpufreq/cpufreq_stats.c +++ b/drivers/cpufreq/cpufreq_stats.c @@ -150,10 +150,12 @@ static void __cpufreq_stats_free_table(struct cpufreq_policy *policy) pr_debug("%s: Free stat table\n", __func__);
- mutex_lock(&cpufreq_stats_lock); sysfs_remove_group(&policy->kobj, &stats_attr_group); kfree(stat->time_in_state); kfree(stat); policy->stats_data = NULL;
- mutex_unlock(&cpufreq_stats_lock);
} static void cpufreq_stats_free_table(unsigned int cpu) @@ -182,13 +184,17 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy) if (unlikely(!table)) return 0;
- mutex_lock(&cpufreq_stats_lock);
- /* stats already initialized */
- if (policy->stats_data)
return -EEXIST;
- if (policy->stats_data) {
ret = -EEXIST;goto unlock;- }
stat = kzalloc(sizeof(*stat), GFP_KERNEL); if (!stat)
return -ENOMEM;
goto unlock;/* Find total allocation size */ cpufreq_for_each_valid_entry(pos, table) @@ -219,21 +225,21 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy) stat->freq_table[i++] = pos->frequency; stat->state_num = i;
- mutex_lock(&cpufreq_stats_lock); stat->last_time = get_jiffies_64(); stat->last_index = freq_table_get_index(stat, policy->cur);
- mutex_unlock(&cpufreq_stats_lock);
policy->stats_data = stat; ret = sysfs_create_group(&policy->kobj, &stats_attr_group); if (!ret)
return 0;
goto unlock;/* We failed, release resources */ policy->stats_data = NULL; kfree(stat->time_in_state); free_stat: kfree(stat); +unlock:
- mutex_unlock(&cpufreq_stats_lock);
return ret; }