[PATCH 1/6] cpufreq: governor: Remove prints from allocation failures

[PATCH 1/6] cpufreq: governor: Remove prints from allocation failures

[Viresh Kumar]

These aren't required anymore as the allocation core already prints such
messages. Remove the redundant ones.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_conservative.c | 4 +---
 drivers/cpufreq/cpufreq_ondemand.c     | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 316df247e00d..5a15f649602c 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -273,10 +273,8 @@ static int cs_init(struct dbs_data *dbs_data, bool notify)
 	struct cs_dbs_tuners *tuners;
 
 	tuners = kzalloc(sizeof(*tuners), GFP_KERNEL);
-	if (!tuners) {
-		pr_err("%s: kzalloc failed\n", __func__);
+	if (!tuners)
 		return -ENOMEM;
-	}
 
 	tuners->down_threshold = DEF_FREQUENCY_DOWN_THRESHOLD;
 	tuners->freq_step = DEF_FREQUENCY_STEP;
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index 300163430516..309cfeb744b2 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -368,10 +368,8 @@ static int od_init(struct dbs_data *dbs_data, bool notify)
 	int cpu;
 
 	tuners = kzalloc(sizeof(*tuners), GFP_KERNEL);
-	if (!tuners) {
-		pr_err("%s: kzalloc failed\n", __func__);
+	if (!tuners)
 		return -ENOMEM;
-	}
 
 	cpu = get_cpu();
 	idle_time = get_cpu_idle_time_us(cpu, NULL);
-- 
2.7.1.410.g6faf27b

[PATCH 2/6] cpufreq: governor: Remove unnecessary bits from print message

[Viresh Kumar]

pr_*() helpers already prefix the print messages with
"cpufreq_governor:" and similar details aren't required in the actual
message.

For example, the print message getting fixed looks like this before this
patch:

cpufreq_governor: cpufreq: Governor initialization failed (dbs_data kobject init error 0)

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_governor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index be498d56dd69..45ca5aff88b2 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -458,7 +458,7 @@ static int cpufreq_governor_init(struct cpufreq_policy *policy)
 		goto out;
 
 	/* Failure, so roll back. */
-	pr_err("cpufreq: Governor initialization failed (dbs_data kobject init error %d)\n", ret);
+	pr_err("initialization failed (dbs_data kobject init error %d)\n", ret);
 
 	policy->governor_data = NULL;
 
-- 
2.7.1.410.g6faf27b

[PATCH 3/6] cpufreq: schedutil: Improve prints messages with pr_fmt

[Viresh Kumar]

Prefix print messages with KBUILD_MODNAME, i.e 'cpufreq_schedutil: '.
This helps to keep similar formatting for all the print messages
particular to a file and identify those easily in kernel logs.

Its already done this way for rest of the governors.

Along with that, remove the (now) redundant bits from a print message.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/sched/cpufreq_schedutil.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 154ae3a51e86..14c4aa25cc45 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -9,6 +9,8 @@
  * published by the Free Software Foundation.
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/cpufreq.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -388,7 +390,7 @@ static int sugov_init(struct cpufreq_policy *policy)
 	mutex_unlock(&global_tunables_lock);
 
 	sugov_policy_free(sg_policy);
-	pr_err("cpufreq: schedutil governor initialization failed (error %d)\n", ret);
+	pr_err("initialization failed (error %d)\n", ret);
 	return ret;
 }
 
-- 
2.7.1.410.g6faf27b

[PATCH 4/6] cpufreq: conservative: Remove declaration of cs_dbs_gov

[Viresh Kumar]

The code doesn't use 'cs_dbs_gov' anymore before it is defined and the
extra line for declaring it can be removed.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_conservative.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 5a15f649602c..904839171d94 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -127,8 +127,6 @@ static struct notifier_block cs_cpufreq_notifier_block = {
 };
 
 /************************** sysfs interface ************************/
-static struct dbs_governor cs_dbs_gov;
-
 static ssize_t store_sampling_down_factor(struct gov_attr_set *attr_set,
 					  const char *buf, size_t count)
 {
-- 
2.7.1.410.g6faf27b

[PATCH 5/6] cpufreq: Reuse gov_attr_* macros in schedutil governor

[Viresh Kumar]

These macros can be used by governors which don't use the common
governor code present in cpufreq_governor.c and should be moved to the
relevant header.

Now that they are getting moved to the right header file, reuse them in
schedutil governor as well (that required rename of show/store
routines).

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_governor.h | 8 --------
 include/linux/cpufreq.h            | 8 ++++++++
 kernel/sched/cpufreq_schedutil.c   | 8 ++++----
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h
index 34eb214b6d57..e2bc5281d3b7 100644
--- a/drivers/cpufreq/cpufreq_governor.h
+++ b/drivers/cpufreq/cpufreq_governor.h
@@ -70,14 +70,6 @@ static ssize_t show_##file_name						\
 	return sprintf(buf, "%u\n", dbs_data->file_name);		\
 }
 
-#define gov_attr_ro(_name)						\
-static struct governor_attr _name =					\
-__ATTR(_name, 0444, show_##_name, NULL)
-
-#define gov_attr_rw(_name)						\
-static struct governor_attr _name =					\
-__ATTR(_name, 0644, show_##_name, store_##_name)
-
 /* Common to all CPUs of a policy */
 struct policy_dbs_info {
 	struct cpufreq_policy *policy;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 4e81e08db752..fe4c48b0fa40 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -501,6 +501,14 @@ struct gov_attr_set {
 	int usage_count;
 };
 
+#define gov_attr_ro(_name)						\
+static struct governor_attr _name =					\
+__ATTR(_name, 0444, show_##_name, NULL)
+
+#define gov_attr_rw(_name)						\
+static struct governor_attr _name =					\
+__ATTR(_name, 0644, show_##_name, store_##_name)
+
 /* sysfs ops for cpufreq governors */
 extern const struct sysfs_ops governor_sysfs_ops;
 
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 14c4aa25cc45..4035765deafc 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -243,15 +243,15 @@ static inline struct sugov_tunables *to_sugov_tunables(struct gov_attr_set *attr
 	return container_of(attr_set, struct sugov_tunables, attr_set);
 }
 
-static ssize_t rate_limit_us_show(struct gov_attr_set *attr_set, char *buf)
+static ssize_t show_rate_limit_us(struct gov_attr_set *attr_set, char *buf)
 {
 	struct sugov_tunables *tunables = to_sugov_tunables(attr_set);
 
 	return sprintf(buf, "%u\n", tunables->rate_limit_us);
 }
 
-static ssize_t rate_limit_us_store(struct gov_attr_set *attr_set, const char *buf,
-				   size_t count)
+static ssize_t store_rate_limit_us(struct gov_attr_set *attr_set,
+				   const char *buf, size_t count)
 {
 	struct sugov_tunables *tunables = to_sugov_tunables(attr_set);
 	struct sugov_policy *sg_policy;
@@ -268,7 +268,7 @@ static ssize_t rate_limit_us_store(struct gov_attr_set *attr_set, const char *bu
 	return count;
 }
 
-static struct governor_attr rate_limit_us = __ATTR_RW(rate_limit_us);
+gov_attr_rw(rate_limit_us);
 
 static struct attribute *sugov_attributes[] = {
 	&rate_limit_us.attr,
-- 
2.7.1.410.g6faf27b

[PATCH 6/6] cpufreq: governor: Create cpufreq_policy_apply_limits()

[Viresh Kumar]

Create a new helper to avoid code duplication across governors.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq_governor.c | 7 +------
 include/linux/cpufreq.h            | 8 ++++++++
 kernel/sched/cpufreq_schedutil.c   | 9 +--------
 3 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 45ca5aff88b2..10cf91cadb97 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -551,12 +551,7 @@ static int cpufreq_governor_limits(struct cpufreq_policy *policy)
 	struct policy_dbs_info *policy_dbs = policy->governor_data;
 
 	mutex_lock(&policy_dbs->timer_mutex);
-
-	if (policy->max < policy->cur)
-		__cpufreq_driver_target(policy, policy->max, CPUFREQ_RELATION_H);
-	else if (policy->min > policy->cur)
-		__cpufreq_driver_target(policy, policy->min, CPUFREQ_RELATION_L);
-
+	cpufreq_policy_apply_limits(policy);
 	gov_update_sample_delay(policy_dbs, 0);
 
 	mutex_unlock(&policy_dbs->timer_mutex);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index fe4c48b0fa40..2c0c5177f3e1 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -493,6 +493,14 @@ void cpufreq_unregister_governor(struct cpufreq_governor *governor);
 struct cpufreq_governor *cpufreq_default_governor(void);
 struct cpufreq_governor *cpufreq_fallback_governor(void);
 
+static inline void cpufreq_policy_apply_limits(struct cpufreq_policy *policy)
+{
+	if (policy->max < policy->cur)
+		__cpufreq_driver_target(policy, policy->max, CPUFREQ_RELATION_H);
+	else if (policy->min > policy->cur)
+		__cpufreq_driver_target(policy, policy->min, CPUFREQ_RELATION_L);
+}
+
 /* Governor attribute set */
 struct gov_attr_set {
 	struct kobject kobj;
diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 4035765deafc..7699f88234ce 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -465,14 +465,7 @@ static int sugov_limits(struct cpufreq_policy *policy)
 
 	if (!policy->fast_switch_enabled) {
 		mutex_lock(&sg_policy->work_lock);
-
-		if (policy->max < policy->cur)
-			__cpufreq_driver_target(policy, policy->max,
-						CPUFREQ_RELATION_H);
-		else if (policy->min > policy->cur)
-			__cpufreq_driver_target(policy, policy->min,
-						CPUFREQ_RELATION_L);
-
+		cpufreq_policy_apply_limits(policy);
 		mutex_unlock(&sg_policy->work_lock);
 	}
 
-- 
2.7.1.410.g6faf27b

Re: [PATCH 5/6] cpufreq: Reuse gov_attr_* macros in schedutil governor

[Rafael J. Wysocki]

On Wed, May 18, 2016 at 2:25 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> These macros can be used by governors which don't use the common
> governor code present in cpufreq_governor.c and should be moved to the
> relevant header.
>
> Now that they are getting moved to the right header file, reuse them in
> schedutil governor as well (that required rename of show/store
> routines).

I'm not sure what the benefit is to be honest.

It adds one level of indirection to the definition of rate_limit_us,
but why is that better?

Re: [PATCH 4/6] cpufreq: conservative: Remove declaration of cs_dbs_gov

[Rafael J. Wysocki]

On Wed, May 18, 2016 at 2:25 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> The code doesn't use 'cs_dbs_gov' anymore before it is defined and the
> extra line for declaring it can be removed.

This sort of conflicts with a patch I've just posted
(https://patchwork.kernel.org/patch/9122121/) and I'd prefer to go
with that one if that's not a big deal.

Re: [PATCH 5/6] cpufreq: Reuse gov_attr_* macros in schedutil governor

[Viresh Kumar]

On 18-05-16, 23:01, Rafael J. Wysocki wrote:
> On Wed, May 18, 2016 at 2:25 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > These macros can be used by governors which don't use the common
> > governor code present in cpufreq_governor.c and should be moved to the
> > relevant header.
> >
> > Now that they are getting moved to the right header file, reuse them in
> > schedutil governor as well (that required rename of show/store
> > routines).
> 
> I'm not sure what the benefit is to be honest.
> 
> It adds one level of indirection to the definition of rate_limit_us,
> but why is that better?

I agree.

I did it because I am required to use these macros for the
interactive-governor and have to move them to cpufreq.h anyway.

Now that we have to move them out, I thought that we should perhaps
use them for schedutil as well. This would look more relevant to
schedutil once it has more tunables instead of just one.

-- 
viresh

Re: [PATCH 4/6] cpufreq: conservative: Remove declaration of cs_dbs_gov

[Viresh Kumar]

On 18-05-16, 23:03, Rafael J. Wysocki wrote:
> On Wed, May 18, 2016 at 2:25 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > The code doesn't use 'cs_dbs_gov' anymore before it is defined and the
> > extra line for declaring it can be removed.
> 
> This sort of conflicts with a patch I've just posted
> (https://patchwork.kernel.org/patch/9122121/) and I'd prefer to go
> with that one if that's not a big deal.

Yeah, drop it.

-- 
viresh

Re: [PATCH 5/6] cpufreq: Reuse gov_attr_* macros in schedutil governor

[Rafael J. Wysocki]

On Thu, May 19, 2016 at 4:13 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 18-05-16, 23:01, Rafael J. Wysocki wrote:
>> On Wed, May 18, 2016 at 2:25 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > These macros can be used by governors which don't use the common
>> > governor code present in cpufreq_governor.c and should be moved to the
>> > relevant header.
>> >
>> > Now that they are getting moved to the right header file, reuse them in
>> > schedutil governor as well (that required rename of show/store
>> > routines).
>>
>> I'm not sure what the benefit is to be honest.
>>
>> It adds one level of indirection to the definition of rate_limit_us,
>> but why is that better?
>
> I agree.
>
> I did it because I am required to use these macros for the
> interactive-governor and have to move them to cpufreq.h anyway.
>
> Now that we have to move them out, I thought that we should perhaps
> use them for schedutil as well. This would look more relevant to
> schedutil once it has more tunables instead of just one.

OK, then let's defer this change until we have interactive in the tree
(which still may or may not happen).