Re: [PATCH v2 2/2] cpufreq: Specify default governor on command line
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: 2020-06-25 14:08:32
Also in:
linux-pm, lkml
On Thu, Jun 25, 2020 at 3:50 PM Quentin Perret [off-list ref] wrote:
On Thursday 25 Jun 2020 at 15:28:43 (+0200), Rafael J. Wysocki wrote:quoted
On Thu, Jun 25, 2020 at 1:53 PM Quentin Perret [off-list ref] wrote:quoted
On Thursday 25 Jun 2020 at 13:44:34 (+0200), Rafael J. Wysocki wrote:quoted
On Thu, Jun 25, 2020 at 1:36 PM Viresh Kumar [off-list ref] wrote:quoted
This change is not right IMO. This part handles the set-policy case, where there are no governors. Right now this code, for some reasons unknown to me, forcefully uses the default governor set to indicate the policy, which is not a great idea in my opinion TBH. This doesn't and shouldn't care about governor modules and should only be looking at strings instead of governor pointer.Sounds right.quoted
Rafael, I even think we should remove this code completely and just rely on what the driver has sent to us. Using the selected governor for set policy drivers is very confusing and also we shouldn't be forced to compiling any governor for the set-policy case.Well, AFAICS the idea was to use the default governor as a kind of default policy proxy, but I agree that strings should be sufficient for that.I agree with all the above. I'd much rather not rely on the default governor name to populate the default policy, too, so +1 from me.So before this series the default governor was selected at the kernel configuration time (pre-build) and was always built-in. Because it could not go away, its name could be used to indicate the default policy for the "setpolicy" drivers. After this series, however, it cannot be used this way reliably, but you can still pass cpufreq_param_governor to cpufreq_parse_policy() instead of def_gov->name in cpufreq_init_policy(), can't you?Good point. I also need to fallback to the default builtin governor if the command line parameter isn't valid (or non-existent), so perhaps something like so?
Yes, that should work if I haven't missed anything.
quoted hunk ↗ jump to hunk
iff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index dad6b85f4c89..20a2020abf88 100644--- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c@@ -653,6 +653,23 @@ static unsigned int cpufreq_parse_policy(char *str_governor) return CPUFREQ_POLICY_UNKNOWN; } +static unsigned int cpufreq_default_policy(void) +{ + unsigned int pol; + + pol = cpufreq_parse_policy(cpufreq_param_governor); + if (pol != CPUFREQ_POLICY_UNKNOWN) + return pol; + + if (IS_BUILTIN(CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE)) + return CPUFREQ_POLICY_PERFORMANCE; + + if (IS_BUILTIN(CONFIG_CPU_FREQ_DEFAULT_GOV_POWERSAVE)) + return CPUFREQ_POLICY_POWERSAVE; + + return CPUFREQ_POLICY_UNKNOWN; +} + /** * cpufreq_parse_governor - parse a governor string only for has_target() * @str_governor: Governor name.@@ -1085,8 +1102,8 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy) /* Use the default policy if there is no last_policy. */ if (policy->last_policy) { pol = policy->last_policy; - } else if (default_governor) { - pol = cpufreq_parse_policy(default_governor->name); + } else { + pol = cpufreq_default_policy(); /* * In case the default governor is neiter "performance" * nor "powersave", fall back to the initial policy