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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200626155750.GA540785@google.com>
Date:   Fri, 26 Jun 2020 16:57:50 +0100
From:   Quentin Perret <qperret@...gle.com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Rafael Wysocki <rjw@...ysocki.net>,
        Jonathan Corbet <corbet@....net>, linux-pm@...r.kernel.org,
        Vincent Guittot <vincent.guittot@...aro.org>,
        kernel-team@...roid.com, tkjos@...gle.com, adharmap@...eaurora.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V3 3/3] cpufreq: Specify default governor on command line

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.

> +
>  	} 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?

---8<---
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 93c6399c1a42..a0e90b567e1e 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -50,7 +50,6 @@ 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];
 
 /**
@@ -2814,13 +2813,11 @@ static int __init cpufreq_core_init(void)
        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);
+       if (!strlen(default_governor))
+               strncpy(default_governor, name, CPUFREQ_NAME_LEN);
 
        return 0;
 }
 module_param(off, int, 0444);
-module_param_string(default_governor, cpufreq_param_governor, CPUFREQ_NAME_LEN, 0444);
+module_param_string(default_governor, default_governor, CPUFREQ_NAME_LEN, 0444);
--->8---

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.

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 Viresh for the help!
Quentin