On 04-06-15, 15:34, Preeti U Murthy wrote:
- if (dbs_data) {
WARN_ON(have_governor_per_policy());Shouldn't this be outside this loop ? We warn here and allocate dbs_dta
Loop ? Its just an 'if' block :)
freshly in the current code for the case where governor is per policy.
So what we are doing in the current code is equally disgusting. We already have a pointer and we overwrite it.
dbs_data->usage_count++;Besides, in the case where a governor exists per policy, we will end up incrementing the usage_count to more than 1 under this condition, which does not make sense.
So, the only sane option here is to return an error immediately I think.
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index ed849a8777dd..57a39f8a92b7 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -247,7 +247,8 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy, int ret;
if (dbs_data) { - WARN_ON(have_governor_per_policy()); + if (WARN_ON(have_governor_per_policy())) + return -EINVAL; dbs_data->usage_count++; policy->governor_data = dbs_data; return 0; @@ -276,24 +277,28 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy, latency * LATENCY_MULTIPLIER));
if (!have_governor_per_policy()) { - WARN_ON(cpufreq_get_global_kobject()); + if (WARN_ON(cpufreq_get_global_kobject())) { + ret = -EINVAL; + goto cdata_exit; + } cdata->gdbs_data = dbs_data; }
ret = sysfs_create_group(get_governor_parent_kobj(policy), get_sysfs_attr(dbs_data)); if (ret) - goto cdata_exit; + goto put_kobj;
policy->governor_data = dbs_data;
return 0;
-cdata_exit: +put_kobj: if (!have_governor_per_policy()) { cdata->gdbs_data = NULL; cpufreq_put_global_kobject(); } +cdata_exit: cdata->exit(dbs_data, !policy->governor->initialized); free_dbs_data: kfree(dbs_data);