On 26-06-20, 16:57, Quentin Perret wrote:
> On Friday 26 Jun 2020 at 09:21:44 (+0530), Viresh Kumar wrote:
> > index e798a1193bdf..93c6399c1a42 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -50,6 +50,9 @@ static LIST_HEAD(cpufreq_governor_list);
> > #define for_each_governor(__governor) \
> > list_for_each_entry(__governor, &cpufreq_governor_list, governor_list)
> >
> > +static char cpufreq_param_governor[CPUFREQ_NAME_LEN];
> > +static char default_governor[CPUFREQ_NAME_LEN];
> > +
> > /**
> > * The "cpufreq driver" - the arch- or hardware-dependent low
> > * level driver of CPUFreq support, and its spinlock. This lock
> > @@ -1061,7 +1064,6 @@ __weak struct cpufreq_governor
> > *cpufreq_default_governor(void)
> >
> > static int cpufreq_init_policy(struct cpufreq_policy *policy)
> > {
> > - struct cpufreq_governor *def_gov = cpufreq_default_governor();
> > struct cpufreq_governor *gov = NULL;
> > unsigned int pol = CPUFREQ_POLICY_UNKNOWN;
> > bool put_governor = false;
> > @@ -1071,22 +1073,29 @@ static int cpufreq_init_policy(struct
> > cpufreq_policy *policy)
> > /* Update policy governor to the one used before hotplug. */
> > gov = get_governor(policy->last_governor);
> > if (gov) {
> > - put_governor = true;
> > pr_debug("Restoring governor %s for cpu %d\n",
> > - policy->governor->name, policy->cpu);
> > - } else if (def_gov) {
> > - gov = def_gov;
> > + gov->name, policy->cpu);
> > } else {
> > - return -ENODATA;
> > + gov = get_governor(default_governor);
> > + }
> > +
> > + if (gov) {
> > + put_governor = true;
> > + } else {
> > + gov = cpufreq_default_governor();
> > + if (!gov)
> > + return -ENODATA;
> > }
>
> As mentioned on patch 01, doing put_module() below if gov != NULL would
> avoid this dance with put_governor, but this works too, so no strong
> opinion.
I did it this way because the code looks buggy otherwise, even though
it isn't as put_module() handles it just fine. And so I would like to
keep it this way, unless there are two votes against mine :)
> > +
> > } else {
> > +
> > /* Use the default policy if there is no last_policy. */
> > if (policy->last_policy) {
> > pol = policy->last_policy;
> > - } else if (def_gov) {
> > - pol = cpufreq_parse_policy(def_gov->name);
> > + } else {
> > + pol = cpufreq_parse_policy(default_governor);
> > /*
> > - * In case the default governor is neiter "performance"
> > + * In case the default governor is neither "performance"
> > * nor "powersave", fall back to the initial policy
> > * value set by the driver.
> > */
> > @@ -2796,13 +2805,22 @@ EXPORT_SYMBOL_GPL(cpufreq_unregister_driver);
> >
> > static int __init cpufreq_core_init(void)
> > {
> > + struct cpufreq_governor *gov = cpufreq_default_governor();
> > + char *name = gov->name;
> > +
> > if (cpufreq_disabled())
> > return -ENODEV;
> >
> > cpufreq_global_kobject = kobject_create_and_add("cpufreq",
> > &cpu_subsys.dev_root->kobj);
> > BUG_ON(!cpufreq_global_kobject);
> >
> > + if (strlen(cpufreq_param_governor))
> > + name = cpufreq_param_governor;
> > +
> > + strncpy(default_governor, name, CPUFREQ_NAME_LEN);
>
> Do we need both cpufreq_param_governor and default_governor?
> Could we move everything to only one of them? Something a little bit
> like that maybe?
No because we want to fallback to the default governor when the
governor shown by the cpufreq_param_governor is valid but missing.
> Also, one thing to keep in mind with this version (or the one you
> suggested) is that if the command line parameter is not valid, we will
> not fallback on the builtin default for the ->setpolicy() case. But I
> suppose one might argue this is a reasonable behaviour, so no objection
> from me.
Right, I did that on purpose.
> Otherwise, apart from these nits, I gave this a go on my setup, with
> builtin and modular governors & drivers, and everything worked exactly
> as expected.
Thanks for testing it out Quentin.
--
viresh
