| Message ID | 6277a557b0102524ca317bf0aa94d7b479375d6e.1418902789.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 18 December 2014 at 17:30, Viresh Kumar <viresh.kumar@linaro.org> wrote: > Locking wasn't handled properly at places and this patch is an attempt to fix > it. Specially while creating/removing stat tables. Because we were required to allocated memory from within locks, we had to use mutex here. So, just consider a mutex instead of spinlock everywhere in this file. -- 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
On 23 December 2014 at 16:17, Prarit Bhargava <prarit@redhat.com> wrote: >> sysfs_remove_group(&policy->kobj, &stats_attr_group); > > ^^^ this line is not guaranteed to have removed the group files by the time it Strange .. Under what conditions can the above statement be true? > returns. That is important in this code IMO, as we are adding and removing the > stats entries rapidly. You may hit the dreaded "sysfs file already exists" > WARNING if this is done too fast, or if sysfs is locked for some other reason > and delays the removal of the group. In the worst case, it is possible you hit > a NULL pointer due to stale data access (because you kfree and set to a NULL > pointer below). Hmm, I have seen such stuff yet for sure. But anyway, that would be out of scope of this patch. As w or w/o locking, it is broken :) > I've never found a good method to block this sort of add/remove sysfs file race > from happening. Perhaps someone else may have a suggestion. In the past I've > thought about adding a usage count to the sysfs file handlers themselves but > that seems to lead to blocking on userspace access... Some more details and we can drag Greg-kh into this, but lets be sure of what we are asking.. >> kfree(stat->time_in_state); >> kfree(stat); >> policy->stats_data = NULL; >> + spin_unlock(&cpufreq_stats_lock); >> } > > There may not be an easy way around this ... You were just completing the earlier horror story or is this a new comment which I couldn't understand ? Which IRC do you hangout on? Just in case I need.. -- viresh -- 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
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c index 7701532b32c8..e18735816908 100644 --- a/drivers/cpufreq/cpufreq_stats.c +++ b/drivers/cpufreq/cpufreq_stats.c @@ -149,10 +149,12 @@ static void __cpufreq_stats_free_table(struct cpufreq_policy *policy) pr_debug("%s: Free stat table\n", __func__); + spin_lock(&cpufreq_stats_lock); sysfs_remove_group(&policy->kobj, &stats_attr_group); kfree(stat->time_in_state); kfree(stat); policy->stats_data = NULL; + spin_unlock(&cpufreq_stats_lock); } static void cpufreq_stats_free_table(unsigned int cpu) @@ -181,13 +183,17 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy) if (unlikely(!table)) return 0; + spin_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) @@ -218,21 +224,21 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy) stat->freq_table[i++] = pos->frequency; stat->state_num = i; - spin_lock(&cpufreq_stats_lock); stat->last_time = get_jiffies_64(); stat->last_index = freq_table_get_index(stat, policy->cur); - spin_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: + spin_unlock(&cpufreq_stats_lock); return ret; }
Locking wasn't handled properly at places and this patch is an attempt to fix it. Specially while creating/removing stat tables. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/cpufreq_stats.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)