Re: [PATCH V3 3/3] cpufreq: Specify default governor on command line

[Viresh Kumar]

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