Hi Viresh, On 02/02/16 16:27, Viresh Kumar wrote: > Until now, governors (ondemand/conservative) were using the > 'global-attr' or 'freq-attr', depending on the sysfs location where we > want to create governor's directory. > > The problem is that, in case of 'freq-attr', we are forced to use > show()/store() present in cpufreq.c, which always take policy->rwsem. > > And because of that we were facing some ABBA lockups during governor > callback event CPUFREQ_GOV_POLICY_EXIT. And so we were dropping the > rwsem right before calling governor callback for CPUFREQ_GOV_POLICY_EXIT > event. > > That caused further problems and it never worked perfectly. > > This patch attempts to fix that by creating separate sysfs-ops for > cpufreq governors. > > Because things got much simplified now, we don't need separate > show/store callbacks for governor-for-system and governor-per-policy > cases. > > Signed-off-by: Viresh Kumar <viresh.ku...@linaro.org>
This patch cleans things up a lot, that's good. One thing I'm still concerned about, though: don't we need some locking in place for some of the store operations on governors attributes? Are store_{ignore_nice_load, sampling_down_fact, etc} safe without locking? It seems that we can call them from different cpus concurrently. Best, - Juri > --- > drivers/cpufreq/cpufreq_conservative.c | 71 > +++++++++++++--------------------- > drivers/cpufreq/cpufreq_governor.c | 50 +++++++++++++++++++----- > drivers/cpufreq/cpufreq_governor.h | 31 +++++++++++++-- > drivers/cpufreq/cpufreq_ondemand.c | 71 > +++++++++++++--------------------- > 4 files changed, 122 insertions(+), 101 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq_conservative.c > b/drivers/cpufreq/cpufreq_conservative.c > index 57750367bd26..980145da796a 100644 > --- a/drivers/cpufreq/cpufreq_conservative.c > +++ b/drivers/cpufreq/cpufreq_conservative.c > @@ -275,51 +275,35 @@ static ssize_t store_freq_step(struct dbs_data > *dbs_data, const char *buf, > return count; > } > > -show_store_one(cs, sampling_rate); > -show_store_one(cs, sampling_down_factor); > -show_store_one(cs, up_threshold); > -show_store_one(cs, down_threshold); > -show_store_one(cs, ignore_nice_load); > -show_store_one(cs, freq_step); > -show_one(cs, min_sampling_rate); > - > -gov_sys_pol_attr_rw(sampling_rate); > -gov_sys_pol_attr_rw(sampling_down_factor); > -gov_sys_pol_attr_rw(up_threshold); > -gov_sys_pol_attr_rw(down_threshold); > -gov_sys_pol_attr_rw(ignore_nice_load); > -gov_sys_pol_attr_rw(freq_step); > -gov_sys_pol_attr_ro(min_sampling_rate); > - > -static struct attribute *dbs_attributes_gov_sys[] = { > - &min_sampling_rate_gov_sys.attr, > - &sampling_rate_gov_sys.attr, > - &sampling_down_factor_gov_sys.attr, > - &up_threshold_gov_sys.attr, > - &down_threshold_gov_sys.attr, > - &ignore_nice_load_gov_sys.attr, > - &freq_step_gov_sys.attr, > +gov_show_one(cs, sampling_rate); > +gov_show_one(cs, sampling_down_factor); > +gov_show_one(cs, up_threshold); > +gov_show_one(cs, down_threshold); > +gov_show_one(cs, ignore_nice_load); > +gov_show_one(cs, freq_step); > +gov_show_one(cs, min_sampling_rate); > + > +gov_attr_rw(sampling_rate); > +gov_attr_rw(sampling_down_factor); > +gov_attr_rw(up_threshold); > +gov_attr_rw(down_threshold); > +gov_attr_rw(ignore_nice_load); > +gov_attr_rw(freq_step); > +gov_attr_ro(min_sampling_rate); > + > +static struct attribute *dbs_attributes[] = { > + &min_sampling_rate.attr, > + &sampling_rate.attr, > + &sampling_down_factor.attr, > + &up_threshold.attr, > + &down_threshold.attr, > + &ignore_nice_load.attr, > + &freq_step.attr, > NULL > }; > > -static struct attribute_group cs_attr_group_gov_sys = { > - .attrs = dbs_attributes_gov_sys, > - .name = "conservative", > -}; > - > -static struct attribute *dbs_attributes_gov_pol[] = { > - &min_sampling_rate_gov_pol.attr, > - &sampling_rate_gov_pol.attr, > - &sampling_down_factor_gov_pol.attr, > - &up_threshold_gov_pol.attr, > - &down_threshold_gov_pol.attr, > - &ignore_nice_load_gov_pol.attr, > - &freq_step_gov_pol.attr, > - NULL > -}; > - > -static struct attribute_group cs_attr_group_gov_pol = { > - .attrs = dbs_attributes_gov_pol, > +static struct attribute_group cs_attr_group = { > + .attrs = dbs_attributes, > .name = "conservative", > }; > > @@ -365,8 +349,7 @@ define_get_cpu_dbs_routines(cs_cpu_dbs_info); > > static struct common_dbs_data cs_dbs_cdata = { > .governor = GOV_CONSERVATIVE, > - .attr_group_gov_sys = &cs_attr_group_gov_sys, > - .attr_group_gov_pol = &cs_attr_group_gov_pol, > + .attr_group = &cs_attr_group, > .get_cpu_cdbs = get_cpu_cdbs, > .get_cpu_dbs_info_s = get_cpu_dbs_info_s, > .gov_dbs_timer = cs_dbs_timer, > diff --git a/drivers/cpufreq/cpufreq_governor.c > b/drivers/cpufreq/cpufreq_governor.c > index 9a7edc91ad57..e785a118cbdc 100644 > --- a/drivers/cpufreq/cpufreq_governor.c > +++ b/drivers/cpufreq/cpufreq_governor.c > @@ -22,14 +22,37 @@ > > #include "cpufreq_governor.h" > > -static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data) > +#define to_dbs_data(k) container_of(k, struct dbs_data, kobj) > +#define to_attr(a) container_of(a, struct governor_attr, attr) > + > +static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf) > { > - if (have_governor_per_policy()) > - return dbs_data->cdata->attr_group_gov_pol; > - else > - return dbs_data->cdata->attr_group_gov_sys; > + struct dbs_data *dbs_data = to_dbs_data(kobj); > + struct governor_attr *gattr = to_attr(attr); > + > + if (gattr->show) > + return gattr->show(dbs_data, buf); > + > + return -EIO; > +} > + > +static ssize_t store(struct kobject *kobj, struct attribute *attr, > + const char *buf, size_t count) > +{ > + struct dbs_data *dbs_data = to_dbs_data(kobj); > + struct governor_attr *gattr = to_attr(attr); > + > + if (gattr->store) > + return gattr->store(dbs_data, buf, count); > + > + return -EIO; > } > > +static const struct sysfs_ops sysfs_ops = { > + .show = show, > + .store = store, > +}; > + > void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) > { > struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu); > @@ -354,6 +377,7 @@ static int cpufreq_governor_init(struct cpufreq_policy > *policy, > struct dbs_data *dbs_data, > struct common_dbs_data *cdata) > { > + struct attribute_group *attr_group; > int ret; > > /* State should be equivalent to EXIT */ > @@ -395,10 +419,17 @@ static int cpufreq_governor_init(struct cpufreq_policy > *policy, > > policy->governor_data = dbs_data; > > - ret = sysfs_create_group(get_governor_parent_kobj(policy), > - get_sysfs_attr(dbs_data)); > - if (ret) > + attr_group = dbs_data->cdata->attr_group; > + dbs_data->kobj_type.sysfs_ops = &sysfs_ops; > + dbs_data->kobj_type.default_attrs = attr_group->attrs; > + > + ret = kobject_init_and_add(&dbs_data->kobj, &dbs_data->kobj_type, > + get_governor_parent_kobj(policy), > + attr_group->name); > + if (ret) { > + pr_err("%s: failed to init dbs_data kobj: %d\n", __func__, ret); > goto reset_gdbs_data; > + } > > return 0; > > @@ -426,8 +457,7 @@ static int cpufreq_governor_exit(struct cpufreq_policy > *policy, > return -EBUSY; > > if (!--dbs_data->usage_count) { > - sysfs_remove_group(get_governor_parent_kobj(policy), > - get_sysfs_attr(dbs_data)); > + kobject_put(&dbs_data->kobj); > > policy->governor_data = NULL; > > diff --git a/drivers/cpufreq/cpufreq_governor.h > b/drivers/cpufreq/cpufreq_governor.h > index ad44a8546a3a..59b28133dd68 100644 > --- a/drivers/cpufreq/cpufreq_governor.h > +++ b/drivers/cpufreq/cpufreq_governor.h > @@ -108,6 +108,31 @@ static ssize_t store_##file_name##_gov_pol > \ > show_one(_gov, file_name); \ > store_one(_gov, file_name) > > +/* Governor's specific attributes */ > +struct dbs_data; > +struct governor_attr { > + struct attribute attr; > + ssize_t (*show)(struct dbs_data *dbs_data, char *buf); > + ssize_t (*store)(struct dbs_data *dbs_data, const char *buf, > + size_t count); > +}; > + > +#define gov_show_one(_gov, file_name) > \ > +static ssize_t show_##file_name > \ > +(struct dbs_data *dbs_data, char *buf) > \ > +{ \ > + struct _gov##_dbs_tuners *tuners = dbs_data->tuners; \ > + return sprintf(buf, "%u\n", tuners->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) > + > /* create helper routines */ > #define define_get_cpu_dbs_routines(_dbs_info) > \ > static struct cpu_dbs_info *get_cpu_cdbs(int cpu) \ > @@ -197,14 +222,12 @@ struct cs_dbs_tuners { > }; > > /* Common Governor data across policies */ > -struct dbs_data; > struct common_dbs_data { > /* Common across governors */ > #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 */ > + struct attribute_group *attr_group; /* one governor - system */ > > /* > * Common data for platforms that don't set > @@ -234,6 +257,8 @@ struct dbs_data { > struct common_dbs_data *cdata; > int usage_count; > void *tuners; > + struct kobject kobj; > + struct kobj_type kobj_type; > }; > > /* Governor specific ops, will be passed to dbs_data->gov_ops */ > diff --git a/drivers/cpufreq/cpufreq_ondemand.c > b/drivers/cpufreq/cpufreq_ondemand.c > index b31f64745232..b7983dd02e24 100644 > --- a/drivers/cpufreq/cpufreq_ondemand.c > +++ b/drivers/cpufreq/cpufreq_ondemand.c > @@ -436,51 +436,35 @@ static ssize_t store_powersave_bias(struct dbs_data > *dbs_data, const char *buf, > return count; > } > > -show_store_one(od, sampling_rate); > -show_store_one(od, io_is_busy); > -show_store_one(od, up_threshold); > -show_store_one(od, sampling_down_factor); > -show_store_one(od, ignore_nice_load); > -show_store_one(od, powersave_bias); > -show_one(od, min_sampling_rate); > - > -gov_sys_pol_attr_rw(sampling_rate); > -gov_sys_pol_attr_rw(io_is_busy); > -gov_sys_pol_attr_rw(up_threshold); > -gov_sys_pol_attr_rw(sampling_down_factor); > -gov_sys_pol_attr_rw(ignore_nice_load); > -gov_sys_pol_attr_rw(powersave_bias); > -gov_sys_pol_attr_ro(min_sampling_rate); > - > -static struct attribute *dbs_attributes_gov_sys[] = { > - &min_sampling_rate_gov_sys.attr, > - &sampling_rate_gov_sys.attr, > - &up_threshold_gov_sys.attr, > - &sampling_down_factor_gov_sys.attr, > - &ignore_nice_load_gov_sys.attr, > - &powersave_bias_gov_sys.attr, > - &io_is_busy_gov_sys.attr, > +gov_show_one(od, sampling_rate); > +gov_show_one(od, io_is_busy); > +gov_show_one(od, up_threshold); > +gov_show_one(od, sampling_down_factor); > +gov_show_one(od, ignore_nice_load); > +gov_show_one(od, powersave_bias); > +gov_show_one(od, min_sampling_rate); > + > +gov_attr_rw(sampling_rate); > +gov_attr_rw(io_is_busy); > +gov_attr_rw(up_threshold); > +gov_attr_rw(sampling_down_factor); > +gov_attr_rw(ignore_nice_load); > +gov_attr_rw(powersave_bias); > +gov_attr_ro(min_sampling_rate); > + > +static struct attribute *dbs_attributes[] = { > + &min_sampling_rate.attr, > + &sampling_rate.attr, > + &up_threshold.attr, > + &sampling_down_factor.attr, > + &ignore_nice_load.attr, > + &powersave_bias.attr, > + &io_is_busy.attr, > NULL > }; > > -static struct attribute_group od_attr_group_gov_sys = { > - .attrs = dbs_attributes_gov_sys, > - .name = "ondemand", > -}; > - > -static struct attribute *dbs_attributes_gov_pol[] = { > - &min_sampling_rate_gov_pol.attr, > - &sampling_rate_gov_pol.attr, > - &up_threshold_gov_pol.attr, > - &sampling_down_factor_gov_pol.attr, > - &ignore_nice_load_gov_pol.attr, > - &powersave_bias_gov_pol.attr, > - &io_is_busy_gov_pol.attr, > - NULL > -}; > - > -static struct attribute_group od_attr_group_gov_pol = { > - .attrs = dbs_attributes_gov_pol, > +static struct attribute_group od_attr_group = { > + .attrs = dbs_attributes, > .name = "ondemand", > }; > > @@ -542,8 +526,7 @@ static struct od_ops od_ops = { > > static struct common_dbs_data od_dbs_cdata = { > .governor = GOV_ONDEMAND, > - .attr_group_gov_sys = &od_attr_group_gov_sys, > - .attr_group_gov_pol = &od_attr_group_gov_pol, > + .attr_group = &od_attr_group, > .get_cpu_cdbs = get_cpu_cdbs, > .get_cpu_dbs_info_s = get_cpu_dbs_info_s, > .gov_dbs_timer = od_dbs_timer, > -- > 2.7.0.79.gdc08a19 >