Re: [PATCH 1/3] cpufreq: governor: register notifier from cs_init()

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

On 06/03/2015 03:57 PM, Viresh Kumar wrote:
> Notifiers are required only for conservative governor and the common
> governor code is unnecessarily polluted with that. Handle that from
> cs_init/exit() instead of cpufreq_governor_dbs().
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq_conservative.c | 26 +++++++++++++++-----------
>  drivers/cpufreq/cpufreq_governor.c     | 22 +++-------------------
>  drivers/cpufreq/cpufreq_governor.h     |  8 ++------
>  drivers/cpufreq/cpufreq_ondemand.c     |  4 ++--
>  4 files changed, 22 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index 25a70d06c5bf..75f875bb155e 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -148,6 +148,10 @@ static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
>  	return 0;
>  }
> 
> +static struct notifier_block cs_cpufreq_notifier_block = {
> +	.notifier_call = dbs_cpufreq_notifier,
> +};
> +
>  /************************** sysfs interface ************************/
>  static struct common_dbs_data cs_dbs_cdata;
> 
> @@ -317,7 +321,7 @@ static struct attribute_group cs_attr_group_gov_pol = {
> 
>  /************************** sysfs end ************************/
> 
> -static int cs_init(struct dbs_data *dbs_data)
> +static int cs_init(struct dbs_data *dbs_data, bool notify)
>  {
>  	struct cs_dbs_tuners *tuners;
> 
> @@ -336,25 +340,26 @@ static int cs_init(struct dbs_data *dbs_data)
>  	dbs_data->tuners = tuners;
>  	dbs_data->min_sampling_rate = MIN_SAMPLING_RATE_RATIO *
>  		jiffies_to_usecs(10);
> +
> +	if (notify)
> +		cpufreq_register_notifier(&cs_cpufreq_notifier_block,
> +					  CPUFREQ_TRANSITION_NOTIFIER);
> +
>  	mutex_init(&dbs_data->mutex);
>  	return 0;
>  }
> 
> -static void cs_exit(struct dbs_data *dbs_data)
> +static void cs_exit(struct dbs_data *dbs_data, bool notify)
>  {
> +	if (notify)
> +		cpufreq_unregister_notifier(&cs_cpufreq_notifier_block,
> +					    CPUFREQ_TRANSITION_NOTIFIER);
> +
>  	kfree(dbs_data->tuners);
>  }
> 
>  define_get_cpu_dbs_routines(cs_cpu_dbs_info);
> 
> -static struct notifier_block cs_cpufreq_notifier_block = {
> -	.notifier_call = dbs_cpufreq_notifier,
> -};
> -
> -static struct cs_ops cs_ops = {
> -	.notifier_block = &cs_cpufreq_notifier_block,
> -};
> -
>  static struct common_dbs_data cs_dbs_cdata = {
>  	.governor = GOV_CONSERVATIVE,
>  	.attr_group_gov_sys = &cs_attr_group_gov_sys,
> @@ -363,7 +368,6 @@ static struct common_dbs_data cs_dbs_cdata = {
>  	.get_cpu_dbs_info_s = get_cpu_dbs_info_s,
>  	.gov_dbs_timer = cs_dbs_timer,
>  	.gov_check_cpu = cs_check_cpu,
> -	.gov_ops = &cs_ops,
>  	.init = cs_init,
>  	.exit = cs_exit,
>  };
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 1b44496b2d2b..d64a82e6481a 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -278,7 +278,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
> 
>  		dbs_data->cdata = cdata;
>  		dbs_data->usage_count = 1;
> -		rc = cdata->init(dbs_data);
> +		rc = cdata->init(dbs_data, !policy->governor->initialized);
>  		if (rc) {
>  			pr_err("%s: POLICY_INIT: init() failed\n", __func__);
>  			kfree(dbs_data);
> @@ -291,7 +291,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>  		rc = sysfs_create_group(get_governor_parent_kobj(policy),
>  				get_sysfs_attr(dbs_data));
>  		if (rc) {
> -			cdata->exit(dbs_data);
> +			cdata->exit(dbs_data, !policy->governor->initialized);
>  			kfree(dbs_data);
>  			return rc;
>  		}
> @@ -309,14 +309,6 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>  		set_sampling_rate(dbs_data, max(dbs_data->min_sampling_rate,
>  					latency * LATENCY_MULTIPLIER));
> 
> -		if ((cdata->governor == GOV_CONSERVATIVE) &&
> -				(!policy->governor->initialized)) {
> -			struct cs_ops *cs_ops = dbs_data->cdata->gov_ops;
> -
> -			cpufreq_register_notifier(cs_ops->notifier_block,
> -					CPUFREQ_TRANSITION_NOTIFIER);
> -		}
> -
>  		if (!have_governor_per_policy())
>  			cdata->gdbs_data = dbs_data;
> 
> @@ -329,15 +321,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
>  			if (!have_governor_per_policy())
>  				cpufreq_put_global_kobject();
> 
> -			if ((dbs_data->cdata->governor == GOV_CONSERVATIVE) &&
> -				(policy->governor->initialized == 1)) {
> -				struct cs_ops *cs_ops = dbs_data->cdata->gov_ops;
> -
> -				cpufreq_unregister_notifier(cs_ops->notifier_block,
> -						CPUFREQ_TRANSITION_NOTIFIER);
> -			}
> -
> -			cdata->exit(dbs_data);
> +			cdata->exit(dbs_data, policy->governor->initialized == 1);
>  			kfree(dbs_data);
>  			cdata->gdbs_data = NULL;

I don't see why we need the check on policy->governor->initialized
because we call cdata->init() and cdata->exit(), *only* when the first
and last references to the governor are being made respectively
(filtered by dbs_data->usage_count), which is precisely what the
initialized flag checks. So passing policy->governor->initialized seems
to be redundant? And this is the case for both gov_per_policy and otherwise.

Regards
Preeti U Murthy