Re: [PATCH 3/11] cpufreq: governor: Use common global_dbs_data pointer
From: Saravana Kannan
Date: Wed Feb 03 2016 - 20:40:34 EST
On 02/03/2016 05:25 PM, Rafael J. Wysocki wrote:
On Thu, Feb 4, 2016 at 2:11 AM, Saravana Kannan <skannan@xxxxxxxxxxxxxx> wrote:
On 02/03/2016 03:22 PM, Rafael J. Wysocki wrote:
From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
If the ondemand and conservative governors cannot use per-policy
tunables (CPUFREQ_HAVE_GOVERNOR_PER_POLICY is not set in the cpufreq
driver), all policy objects point to the same single dbs_data object.
Additionally, that object is pointed to by a global pointer hidden in
the governor's data structures.
There is no reason for that pointer to be buried in those
data structures, though, so make it explicitly global.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
---
drivers/cpufreq/cpufreq_governor.c | 20 ++++++++++----------
drivers/cpufreq/cpufreq_governor.h | 20 ++++++++++----------
2 files changed, 20 insertions(+), 20 deletions(-)
Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
+++ linux-pm/drivers/cpufreq/cpufreq_governor.h
@@ -78,7 +78,7 @@ __ATTR(_name, 0644, show_##_name##_gov_p
static ssize_t show_##file_name##_gov_sys \
(struct kobject *kobj, struct attribute *attr, char *buf) \
{ \
- struct _gov##_dbs_tuners *tuners =
_gov##_dbs_cdata.gdbs_data->tuners; \
+ struct _gov##_dbs_tuners *tuners = global_dbs_data->tuners; \
return sprintf(buf, "%u\n", tuners->file_name); \
} \
\
@@ -94,7 +94,7 @@ static ssize_t show_##file_name##_gov_po
static ssize_t store_##file_name##_gov_sys \
(struct kobject *kobj, struct attribute *attr, const char *buf, size_t
count) \
{ \
- struct dbs_data *dbs_data = _gov##_dbs_cdata.gdbs_data; \
+ struct dbs_data *dbs_data = global_dbs_data; \
return store_##file_name(dbs_data, buf, count); \
} \
\
@@ -201,19 +201,14 @@ struct cs_dbs_tuners {
/* Common Governor data across policies */
struct dbs_data;
struct common_dbs_data {
- /* Common across governors */
+ struct cpufreq_governor gov;
+
#define GOV_ONDEMAND 0
#define GOV_CONSERVATIVE 1
int governor;
struct attribute_group *attr_group_gov_sys; /* one governor -
system */
struct attribute_group *attr_group_gov_pol; /* one governor -
policy */
- /*
- * Common data for platforms that don't set
- * CPUFREQ_HAVE_GOVERNOR_PER_POLICY
- */
- struct dbs_data *gdbs_data;
-
struct cpu_dbs_info *(*get_cpu_cdbs)(int cpu);
void *(*get_cpu_dbs_info_s)(int cpu);
unsigned int (*gov_dbs_timer)(struct cpufreq_policy *policy);
@@ -233,6 +228,11 @@ struct dbs_data {
void *tuners;
};
+/*
+ * Common governor data for platforms without
CPUFREQ_HAVE_GOVERNOR_PER_POLICY.
+ */
+extern struct dbs_data *global_dbs_data;
+
/* Governor specific ops, will be passed to dbs_data->gov_ops */
struct od_ops {
void (*powersave_bias_init_cpu)(int cpu);
@@ -256,7 +256,7 @@ static inline int delay_for_sampling_rat
static ssize_t show_sampling_rate_min_gov_sys \
(struct kobject *kobj, struct attribute *attr, char *buf) \
{ \
- struct dbs_data *dbs_data = _gov##_dbs_cdata.gdbs_data; \
+ struct dbs_data *dbs_data = global_dbs_data; \
return sprintf(buf, "%u\n", dbs_data->min_sampling_rate); \
} \
\
Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -22,6 +22,9 @@
#include "cpufreq_governor.h"
+struct dbs_data *global_dbs_data;
+EXPORT_SYMBOL_GPL(global_dbs_data);
+
DEFINE_MUTEX(dbs_data_mutex);
EXPORT_SYMBOL_GPL(dbs_data_mutex);
@@ -377,22 +380,19 @@ static int cpufreq_governor_init(struct
latency * LATENCY_MULTIPLIER));
if (!have_governor_per_policy())
- cdata->gdbs_data = dbs_data;
+ global_dbs_data = dbs_data;
policy->governor_data = dbs_data;
ret = sysfs_create_group(get_governor_parent_kobj(policy),
get_sysfs_attr(dbs_data));
- if (ret)
- goto reset_gdbs_data;
-
- return 0;
+ if (!ret)
+ return 0;
I think the previous method of a handling the error is easier to read and
more in line with the typical kernel coding style. The successful path ends
in an unconditional return statement and the error paths are handled with a
goto.
You are talking about something like this now:
if (condition)
goto label;
return 0;
label:
do stuff
I'm sorry, but I fail to see how this is easier to read than
if (!condition)
return 0;
do stuff
The return statement is not unconditional in either case, but in the
first one it is just obfuscated by using the label and goto which are
completely unnecessary.
It's more readable because someone new is quickly scanning the code to
understand what's going on, once you hit an unconditional return (as in,
return without any ifs around it) you can just assume the rest of the
code is error handling and skip reading/mentally processing them.
That, and it's more inline with how most of the kernel handles error
conditions.
Anyway, you are removing it since it's not related to the patch. So, not
planning to debate this fairly subjective opinion further.
-Saravana
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project