Re: [PATCH 2/3] cpufreq: governor: split cpufreq_governor_dbs()

[Viresh Kumar <viresh.kumar@linaro.org>]

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);

-- 
viresh