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

[Preeti U Murthy <preeti@linux.vnet.ibm.com>]

On 06/03/2015 03:57 PM, Viresh Kumar wrote:
> cpufreq_governor_dbs() is hardly readable, it is just too big and
> complicated. Lets make it more readable by splitting out event specific
> routines.
> 
> Order of statements is changed at few places, but that shouldn't bring
> any functional change.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Best way to verify the changes here is to keep both copies of code side
> by side and comparing it event wise.
> 
>  drivers/cpufreq/cpufreq_governor.c | 326 +++++++++++++++++++++----------------
>  1 file changed, 185 insertions(+), 141 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index d64a82e6481a..dc382a5a2158 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -239,195 +239,239 @@ static void set_sampling_rate(struct dbs_data *dbs_data,
>  	}
>  }
> 
> -int cpufreq_governor_dbs(struct cpufreq_policy *policy,
> -		struct common_dbs_data *cdata, unsigned int event)
> +static int cpufreq_governor_init(struct cpufreq_policy *policy,
> +				 struct dbs_data *dbs_data,
> +				 struct common_dbs_data *cdata)
>  {
> -	struct dbs_data *dbs_data;
> -	struct od_cpu_dbs_info_s *od_dbs_info = NULL;
> -	struct cs_cpu_dbs_info_s *cs_dbs_info = NULL;
> -	struct od_ops *od_ops = NULL;
> -	struct od_dbs_tuners *od_tuners = NULL;
> -	struct cs_dbs_tuners *cs_tuners = NULL;
> -	struct cpu_dbs_common_info *cpu_cdbs;
> -	unsigned int sampling_rate, latency, ignore_nice, j, cpu = policy->cpu;
> -	int io_busy = 0;
> -	int rc;
> +	unsigned int latency;
> +	int ret;
> 
> -	if (have_governor_per_policy())
> -		dbs_data = policy->governor_data;
> -	else
> -		dbs_data = cdata->gdbs_data;
> +	if (dbs_data) {
> +		WARN_ON(have_governor_per_policy());

Shouldn't this be outside this loop ? We warn here and allocate dbs_dta
freshly in the current code for the case where governor is per policy.

> +		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.

> +		policy->governor_data = dbs_data;
> +		return 0;
> +	}

Regards
Preeti U Murthy