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

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