Message-ID: <CAKohpo=A4BBecmepsYSqHYrJg5n9WE-QB3ssvb_dOBZYXuB-eg@mail.gmail.com>
Date:	Wed, 27 Mar 2013 15:34:08 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	Jacob Shin <jacob.shin@....com>
Cc:	"Rafael J. Wysocki" <rjw@...k.pl>, cpufreq@...r.kernel.org,
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
	linaro-kernel@...ts.linaro.org, robin.randhawa@....com,
	Steve.Bannister@....com, Liviu.Dudau@....com,
	charles.garcia-tobin@....com,
	Arvind Chauhan <Arvind.Chauhan@....com>
Subject: Re: [PATCH V3 2/4] cpufreq: governor: Implement per policy instances
 of governors

On 27 March 2013 09:59, Viresh Kumar <viresh.kumar@...aro.org> wrote:
> On 27 March 2013 01:18, Jacob Shin <jacob.shin@....com> wrote:

>> Hmm .. I don't think this works for both ondemand and conservative
>> governors running at the same time .

Hi Jacob,

This must be early morning for you and you must be looking for something
to start with.

I am still stuck at a issue, which i am not able to fix.
- cat of */cpufreq/ondemand/** isn't showing anything on console, but all
  pointers are correctly set and i can see the right values with printk()
- I am able to see all prints for have_multiple_policies enabled.
- Due to this, i haven't tested any corner cases for now.

@Rafael: I will post all this as a separate patch later which you can then
fold into original commit. I am just waiting for a complete fix.

Following is the work i have done until now (attached too), leave earlier
fixups:

--------x-------------x------------------

From: Viresh Kumar <viresh.kumar@...aro.org>
Date: Tue, 26 Mar 2013 23:20:18 +0530
Subject: [PATCH] fixup! cpufreq: governor: Implement per policy instances of
 governors

---
 drivers/cpufreq/cpufreq.c              | 13 +++++++
 drivers/cpufreq/cpufreq_conservative.c | 24 ++++++------
 drivers/cpufreq/cpufreq_governor.c     | 71 ++++++++++++++++++++++++----------
 drivers/cpufreq/cpufreq_governor.h     | 28 ++++++++++++--
 drivers/cpufreq/cpufreq_ondemand.c     | 25 ++++++------
 include/linux/cpufreq.h                |  2 +
 6 files changed, 113 insertions(+), 50 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 3d83b02..3d990b4 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -136,6 +136,11 @@ struct kobject *get_governor_parent_kobj(struct
cpufreq_policy *policy)
  return cpufreq_global_kobject;
 }

+bool have_multiple_policies(void)
+{
+ return cpufreq_driver->have_multiple_policies;
+}
+
 static struct cpufreq_policy *__cpufreq_cpu_get(unsigned int cpu, bool sysfs)
 {
  struct cpufreq_policy *data;
@@ -1554,6 +1559,13 @@ static int __cpufreq_governor(struct
cpufreq_policy *policy,
  policy->cpu, event);
  ret = policy->governor->governor(policy, event);

+ if (!ret) {
+ if (event == CPUFREQ_GOV_POLICY_INIT)
+ policy->governor->initialized++;
+ else if (event == CPUFREQ_GOV_POLICY_EXIT)
+ policy->governor->initialized--;
+ }
+
  /* we keep one module reference alive for
  each CPU governed by this CPU */
  if ((event != CPUFREQ_GOV_START) || ret)
@@ -1577,6 +1589,7 @@ int cpufreq_register_governor(struct
cpufreq_governor *governor)

  mutex_lock(&cpufreq_governor_mutex);

+ governor->initialized = 0;
  err = -EBUSY;
  if (__find_governor(governor->name) == NULL) {
  err = 0;
diff --git a/drivers/cpufreq/cpufreq_conservative.c
b/drivers/cpufreq/cpufreq_conservative.c
index 63499c8..c09d7c8 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -157,11 +157,13 @@ static int dbs_cpufreq_notifier(struct
notifier_block *nb, unsigned long val,
 }

 /************************** sysfs interface ************************/
+static struct common_dbs_data cs_dbs_cdata;
+declare_get_tuners(cs);
+
 static ssize_t store_sampling_down_factor(struct cpufreq_policy *policy,
  const char *buf, size_t count)
 {
- struct dbs_data *dbs_data = policy->governor_data;
- struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
+ struct cs_dbs_tuners *cs_tuners = cs_get_tuners(policy, NULL);
  unsigned int input;
  int ret;
  ret = sscanf(buf, "%u", &input);
@@ -176,8 +178,8 @@ static ssize_t store_sampling_down_factor(struct
cpufreq_policy *policy,
 static ssize_t store_sampling_rate(struct cpufreq_policy *policy,
  const char *buf, size_t count)
 {
- struct dbs_data *dbs_data = policy->governor_data;
- struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
+ struct dbs_data *dbs_data = NULL;
+ struct cs_dbs_tuners *cs_tuners = cs_get_tuners(policy, &dbs_data);
  unsigned int input;
  int ret;
  ret = sscanf(buf, "%u", &input);
@@ -192,8 +194,7 @@ static ssize_t store_sampling_rate(struct
cpufreq_policy *policy,
 static ssize_t store_up_threshold(struct cpufreq_policy *policy,
  const char *buf, size_t count)
 {
- struct dbs_data *dbs_data = policy->governor_data;
- struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
+ struct cs_dbs_tuners *cs_tuners = cs_get_tuners(policy, NULL);
  unsigned int input;
  int ret;
  ret = sscanf(buf, "%u", &input);
@@ -208,8 +209,7 @@ static ssize_t store_up_threshold(struct
cpufreq_policy *policy,
 static ssize_t store_down_threshold(struct cpufreq_policy *policy,
  const char *buf, size_t count)
 {
- struct dbs_data *dbs_data = policy->governor_data;
- struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
+ struct cs_dbs_tuners *cs_tuners = cs_get_tuners(policy, NULL);
  unsigned int input;
  int ret;
  ret = sscanf(buf, "%u", &input);
@@ -226,8 +226,7 @@ static ssize_t store_down_threshold(struct
cpufreq_policy *policy,
 static ssize_t store_ignore_nice(struct cpufreq_policy *policy,
  const char *buf, size_t count)
 {
- struct dbs_data *dbs_data = policy->governor_data;
- struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
+ struct cs_dbs_tuners *cs_tuners = cs_get_tuners(policy, NULL);
  unsigned int input, j;
  int ret;

@@ -259,8 +258,7 @@ static ssize_t store_ignore_nice(struct
cpufreq_policy *policy,
 static ssize_t store_freq_step(struct cpufreq_policy *policy,
  const char *buf, size_t count)
 {
- struct dbs_data *dbs_data = policy->governor_data;
- struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;
+ struct cs_dbs_tuners *cs_tuners = cs_get_tuners(policy, NULL);
  unsigned int input;
  int ret;
  ret = sscanf(buf, "%u", &input);
@@ -285,7 +283,7 @@ show_one(cs, up_threshold, up_threshold);
 show_one(cs, down_threshold, down_threshold);
 show_one(cs, ignore_nice, ignore_nice);
 show_one(cs, freq_step, freq_step);
-declare_show_sampling_rate_min();
+declare_show_sampling_rate_min(cs);

 cpufreq_freq_attr_rw(sampling_rate);
 cpufreq_freq_attr_rw(sampling_down_factor);
diff --git a/drivers/cpufreq/cpufreq_governor.c
b/drivers/cpufreq/cpufreq_governor.c
index 41e5e56..54ca5fc 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -216,10 +216,9 @@ 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)
 {
- struct dbs_data *dbs_data = policy->governor_data;
+ 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 cs_ops *cs_ops = NULL;
  struct od_ops *od_ops = NULL;
  struct od_dbs_tuners *od_tuners = NULL;
  struct cs_dbs_tuners *cs_tuners = NULL;
@@ -228,11 +227,22 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
  int io_busy = 0;
  int rc;

+ if (have_multiple_policies())
+ dbs_data = policy->governor_data;
+ else
+ dbs_data = cdata->gdbs_data;
+
  WARN_ON(!dbs_data && (event != CPUFREQ_GOV_POLICY_INIT));

  switch (event) {
  case CPUFREQ_GOV_POLICY_INIT:
- WARN_ON(dbs_data);
+ if (have_multiple_policies()) {
+ WARN_ON(dbs_data);
+ } else if (dbs_data) {
+ policy->governor_data = dbs_data;
+ return 0;
+ }
+
  dbs_data = kzalloc(sizeof(*dbs_data), GFP_KERNEL);
  if (!dbs_data) {
  pr_err("%s: POLICY_INIT: kzalloc failed\n", __func__);
@@ -246,6 +256,15 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
  kfree(dbs_data);
  return rc;
  }
+
+ rc = sysfs_create_group(get_governor_parent_kobj(policy),
+ dbs_data->cdata->attr_group);
+ if (rc) {
+ cdata->exit(dbs_data);
+ kfree(dbs_data);
+ return rc;
+ }
+
  policy->governor_data = dbs_data;

  /* policy latency is in nS. Convert it to uS first */
@@ -258,10 +277,36 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
  MIN_LATENCY_MULTIPLIER * latency);
  set_sampling_rate(dbs_data, max(dbs_data->min_sampling_rate,
  latency * LATENCY_MULTIPLIER));
+
+ if (dbs_data->cdata->governor == GOV_CONSERVATIVE) {
+ struct cs_ops *cs_ops = dbs_data->cdata->gov_ops;
+
+ cpufreq_register_notifier(cs_ops->notifier_block,
+ CPUFREQ_TRANSITION_NOTIFIER);
+ }
+
+ if (!have_multiple_policies())
+ cdata->gdbs_data = dbs_data;
+
  return 0;
  case CPUFREQ_GOV_POLICY_EXIT:
- cdata->exit(dbs_data);
- kfree(dbs_data);
+ if ((policy->governor->initialized == 1) ||
+ have_multiple_policies()) {
+ sysfs_remove_group(get_governor_parent_kobj(policy),
+ dbs_data->cdata->attr_group);
+
+ if (dbs_data->cdata->governor == GOV_CONSERVATIVE) {
+ struct cs_ops *cs_ops = dbs_data->cdata->gov_ops;
+
+ cpufreq_register_notifier(cs_ops->notifier_block,
+ CPUFREQ_TRANSITION_NOTIFIER);
+ }
+
+ cdata->exit(dbs_data);
+ kfree(dbs_data);
+ cdata->gdbs_data = NULL;
+ }
+
  policy->governor_data = NULL;
  return 0;
  }
@@ -273,7 +318,6 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
  cs_dbs_info = dbs_data->cdata->get_cpu_dbs_info_s(cpu);
  sampling_rate = cs_tuners->sampling_rate;
  ignore_nice = cs_tuners->ignore_nice;
- cs_ops = dbs_data->cdata->gov_ops;
  } else {
  od_tuners = dbs_data->tuners;
  od_dbs_info = dbs_data->cdata->get_cpu_dbs_info_s(cpu);
@@ -307,13 +351,6 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
      dbs_data->cdata->gov_dbs_timer);
  }

- rc = sysfs_create_group(get_governor_parent_kobj(policy),
- dbs_data->cdata->attr_group);
- if (rc) {
- mutex_unlock(&dbs_data->mutex);
- return rc;
- }
-
  /*
  * conservative does not implement micro like ondemand
  * governor, thus we are bound to jiffes/HZ
@@ -322,9 +359,6 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
  cs_dbs_info->down_skip = 0;
  cs_dbs_info->enable = 1;
  cs_dbs_info->requested_freq = policy->cur;
-
- cpufreq_register_notifier(cs_ops->notifier_block,
- CPUFREQ_TRANSITION_NOTIFIER);
  } else {
  od_dbs_info->rate_mult = 1;
  od_dbs_info->sample_type = OD_NORMAL_SAMPLE;
@@ -349,11 +383,6 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
  mutex_lock(&dbs_data->mutex);
  mutex_destroy(&cpu_cdbs->timer_mutex);

- sysfs_remove_group(get_governor_parent_kobj(policy),
- dbs_data->cdata->attr_group);
- if (dbs_data->cdata->governor == GOV_CONSERVATIVE)
- cpufreq_unregister_notifier(cs_ops->notifier_block,
- CPUFREQ_TRANSITION_NOTIFIER);
  mutex_unlock(&dbs_data->mutex);

  break;
diff --git a/drivers/cpufreq/cpufreq_governor.h
b/drivers/cpufreq/cpufreq_governor.h
index 8b18d25..fcf10d9 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -40,13 +40,29 @@
 /* Ondemand Sampling types */
 enum {OD_NORMAL_SAMPLE, OD_SUB_SAMPLE};

+
+#define declare_get_tuners(_gov) \
+static struct _gov##_dbs_tuners *_gov##_get_tuners \
+(struct cpufreq_policy *policy, struct dbs_data **ldbs_data) \
+{ \
+ if (have_multiple_policies()) { \
+ struct dbs_data *dbs_data = policy->governor_data; \
+ if (ldbs_data) \
+ *ldbs_data = dbs_data; \
+ return dbs_data->tuners; \
+ } else { \
+ if (ldbs_data) \
+ *ldbs_data = _gov##_dbs_cdata.gdbs_data; \
+ return _gov##_dbs_cdata.gdbs_data->tuners; \
+ } \
+}
+
 /* Macro creating sysfs show routines */
 #define show_one(_gov, file_name, object) \
 static ssize_t show_##file_name \
 (struct cpufreq_policy *policy, char *buf) \
 { \
- struct dbs_data *dbs_data = policy->governor_data; \
- struct _gov##_dbs_tuners *tuners = dbs_data->tuners; \
+ struct _gov##_dbs_tuners *tuners = _gov##_get_tuners(policy, NULL); \
  return sprintf(buf, "%u\n", tuners->file_name); \
 }

@@ -133,6 +149,9 @@ struct common_dbs_data {
  int governor;
  struct attribute_group *attr_group;

+ /* Common data for platforms that don't set have_multiple_policies */
+ struct dbs_data *gdbs_data;
+
  struct cpu_dbs_common_info *(*get_cpu_cdbs)(int cpu);
  void *(*get_cpu_dbs_info_s)(int cpu);
  void (*gov_dbs_timer)(struct work_struct *work);
@@ -177,11 +196,12 @@ static inline int
delay_for_sampling_rate(unsigned int sampling_rate)
  return delay;
 }

-#define declare_show_sampling_rate_min() \
+#define declare_show_sampling_rate_min(_gov) \
 static ssize_t show_sampling_rate_min(struct cpufreq_policy *policy, \
  char *buf) \
 { \
- struct dbs_data *dbs_data = policy->governor_data; \
+ struct dbs_data *dbs_data = NULL; \
+ _gov##_get_tuners(policy, &dbs_data); \
  return sprintf(buf, "%u\n", dbs_data->min_sampling_rate); \
 }

diff --git a/drivers/cpufreq/cpufreq_ondemand.c
b/drivers/cpufreq/cpufreq_ondemand.c
index 29ed48a..4860137 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -257,6 +257,9 @@ max_delay:
 }

 /************************** sysfs interface ************************/
+static struct common_dbs_data od_dbs_cdata;
+declare_get_tuners(od);
+
 /**
  * update_sampling_rate - update sampling rate effective immediately if needed.
  * @new_rate: new sampling rate
@@ -321,12 +324,14 @@ static void update_sampling_rate(struct dbs_data
*dbs_data,
 static ssize_t store_sampling_rate(struct cpufreq_policy *policy,
  const char *buf, size_t count)
 {
- struct dbs_data *dbs_data = policy->governor_data;
+ struct dbs_data *dbs_data = NULL;
  unsigned int input;
  int ret;
  ret = sscanf(buf, "%u", &input);
  if (ret != 1)
  return -EINVAL;
+
+ od_get_tuners(policy, &dbs_data);
  update_sampling_rate(dbs_data, input);
  return count;
 }
@@ -334,8 +339,7 @@ static ssize_t store_sampling_rate(struct
cpufreq_policy *policy,
 static ssize_t store_io_is_busy(struct cpufreq_policy *policy, const char *buf,
  size_t count)
 {
- struct dbs_data *dbs_data = policy->governor_data;
- struct od_dbs_tuners *od_tuners = dbs_data->tuners;
+ struct od_dbs_tuners *od_tuners = od_get_tuners(policy, NULL);
  unsigned int input;
  int ret;
  unsigned int j;
@@ -358,8 +362,7 @@ static ssize_t store_io_is_busy(struct
cpufreq_policy *policy, const char *buf,
 static ssize_t store_up_threshold(struct cpufreq_policy *policy,
  const char *buf, size_t count)
 {
- struct dbs_data *dbs_data = policy->governor_data;
- struct od_dbs_tuners *od_tuners = dbs_data->tuners;
+ struct od_dbs_tuners *od_tuners = od_get_tuners(policy, NULL);
  unsigned int input;
  int ret;
  ret = sscanf(buf, "%u", &input);
@@ -379,8 +382,7 @@ static ssize_t store_up_threshold(struct
cpufreq_policy *policy,
 static ssize_t store_sampling_down_factor(struct cpufreq_policy *policy,
  const char *buf, size_t count)
 {
- struct dbs_data *dbs_data = policy->governor_data;
- struct od_dbs_tuners *od_tuners = dbs_data->tuners;
+ struct od_dbs_tuners *od_tuners = od_get_tuners(policy, NULL);
  unsigned int input, j;
  int ret;
  ret = sscanf(buf, "%u", &input);
@@ -401,8 +403,7 @@ static ssize_t store_sampling_down_factor(struct
cpufreq_policy *policy,
 static ssize_t store_ignore_nice(struct cpufreq_policy *policy, const
char *buf,
  size_t count)
 {
- struct dbs_data *dbs_data = policy->governor_data;
- struct od_dbs_tuners *od_tuners = dbs_data->tuners;
+ struct od_dbs_tuners *od_tuners = od_get_tuners(policy, NULL);
  unsigned int input;
  int ret;

@@ -437,8 +438,7 @@ static ssize_t store_ignore_nice(struct
cpufreq_policy *policy, const char *buf,
 static ssize_t store_powersave_bias(struct cpufreq_policy *policy,
  const char *buf, size_t count)
 {
- struct dbs_data *dbs_data = policy->governor_data;
- struct od_dbs_tuners *od_tuners = dbs_data->tuners;
+ struct od_dbs_tuners *od_tuners = od_get_tuners(policy, NULL);
  unsigned int input;
  int ret;
  ret = sscanf(buf, "%u", &input);
@@ -460,7 +460,7 @@ show_one(od, up_threshold, up_threshold);
 show_one(od, sampling_down_factor, sampling_down_factor);
 show_one(od, ignore_nice, ignore_nice);
 show_one(od, powersave_bias, powersave_bias);
-declare_show_sampling_rate_min();
+declare_show_sampling_rate_min(od);

 cpufreq_freq_attr_rw(sampling_rate);
 cpufreq_freq_attr_rw(io_is_busy);
@@ -530,6 +530,7 @@ static int od_init(struct dbs_data *dbs_data)
  tuners->io_is_busy = should_io_be_busy();

  dbs_data->tuners = tuners;
+ pr_info("%s: tuners %p\n", __func__, tuners);
  mutex_init(&dbs_data->mutex);
  return 0;
 }
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index dd53cea..394ea0d 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -187,6 +187,7 @@ static inline unsigned long cpufreq_scale(unsigned
long old, u_int div, u_int mu

 struct cpufreq_governor {
  char name[CPUFREQ_NAME_LEN];
+ int initialized;
  int (*governor) (struct cpufreq_policy *policy,
  unsigned int event);
  ssize_t (*show_setspeed) (struct cpufreq_policy *policy,
@@ -323,6 +324,7 @@ const char *cpufreq_get_current_driver(void);
 int cpufreq_get_policy(struct cpufreq_policy *policy, unsigned int cpu);
 int cpufreq_update_policy(unsigned int cpu);
 struct kobject *get_governor_parent_kobj(struct cpufreq_policy *policy);
+bool have_multiple_policies(void);

 #ifdef CONFIG_CPU_FREQ
 /* query the current CPU frequency (in kHz). If zero, cpufreq
couldn't detect it */

Download attachment "0001-fixup-cpufreq-governor-Implement-per-policy-instance.patch" of type "application/octet-stream" (16433 bytes)

Powered by blists - more mailing lists