| Message ID | 96b6e6ca02b664194ff3e57e1ec768fbc597bf38.1593418662.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: Allow default governor on cmdline and fix locking issues | expand |
On 29-06-20, 10:44, Quentin Perret wrote: > On Monday 29 Jun 2020 at 13:55:00 (+0530), Viresh Kumar wrote: > > static int __init cpufreq_core_init(void) > > { > > + struct cpufreq_governor *gov = cpufreq_default_governor(); > > + > > 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(default_governor)) > > Should we test '!strlen(default_governor) && gov' here actually? > We check the return value of cpufreq_default_governor() in > cpufreq_init_policy(), so I'm guessing we should do the same here to be > on the safe side. With the current setup (the Kconfig option being a choice which selects one governor at least), it is not possible for gov to be NULL here. And so I didn't worry about it :) -- viresh
On Monday 29 Jun 2020 at 15:16:27 (+0530), Viresh Kumar wrote: > On 29-06-20, 10:44, Quentin Perret wrote: > > On Monday 29 Jun 2020 at 13:55:00 (+0530), Viresh Kumar wrote: > > > static int __init cpufreq_core_init(void) > > > { > > > + struct cpufreq_governor *gov = cpufreq_default_governor(); > > > + > > > 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(default_governor)) > > > > Should we test '!strlen(default_governor) && gov' here actually? > > We check the return value of cpufreq_default_governor() in > > cpufreq_init_policy(), so I'm guessing we should do the same here to be > > on the safe side. > > With the current setup (the Kconfig option being a choice which > selects one governor at least), it is not possible for gov to be NULL > here. And so I didn't worry about it :) Right, so should we remove the check in cpufreq_init_policy() then? I don't mind either way as long as we are consitent :) Thanks, Quentin
On 29-06-20, 10:48, Quentin Perret wrote: > On Monday 29 Jun 2020 at 15:16:27 (+0530), Viresh Kumar wrote: > > On 29-06-20, 10:44, Quentin Perret wrote: > > > On Monday 29 Jun 2020 at 13:55:00 (+0530), Viresh Kumar wrote: > > > > static int __init cpufreq_core_init(void) > > > > { > > > > + struct cpufreq_governor *gov = cpufreq_default_governor(); > > > > + > > > > 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(default_governor)) > > > > > > Should we test '!strlen(default_governor) && gov' here actually? > > > We check the return value of cpufreq_default_governor() in > > > cpufreq_init_policy(), so I'm guessing we should do the same here to be > > > on the safe side. > > > > With the current setup (the Kconfig option being a choice which > > selects one governor at least), it is not possible for gov to be NULL > > here. And so I didn't worry about it :) > > Right, so should we remove the check in cpufreq_init_policy() then? > I don't mind either way as long as we are consitent :) We can get rid of that as well. -- viresh
On Monday 29 Jun 2020 at 10:48:25 (+0100), Quentin Perret wrote: > On Monday 29 Jun 2020 at 15:16:27 (+0530), Viresh Kumar wrote: > > On 29-06-20, 10:44, Quentin Perret wrote: > > > On Monday 29 Jun 2020 at 13:55:00 (+0530), Viresh Kumar wrote: > > > > static int __init cpufreq_core_init(void) > > > > { > > > > + struct cpufreq_governor *gov = cpufreq_default_governor(); > > > > + > > > > 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(default_governor)) > > > > > > Should we test '!strlen(default_governor) && gov' here actually? > > > We check the return value of cpufreq_default_governor() in > > > cpufreq_init_policy(), so I'm guessing we should do the same here to be > > > on the safe side. > > > > With the current setup (the Kconfig option being a choice which > > selects one governor at least), it is not possible for gov to be NULL > > here. And so I didn't worry about it :) > > Right, so should we remove the check in cpufreq_init_policy() then? > I don't mind either way as long as we are consitent :) And actually maybe we should remove the weakly defined cpufreq_default_governor() implementation too? That'd make sure we get a link-time error if for some reason things change in the Kconfig options. Thanks, Quentin
On 29-06-20, 10:50, Quentin Perret wrote: > On Monday 29 Jun 2020 at 10:48:25 (+0100), Quentin Perret wrote: > > On Monday 29 Jun 2020 at 15:16:27 (+0530), Viresh Kumar wrote: > > > On 29-06-20, 10:44, Quentin Perret wrote: > > > > On Monday 29 Jun 2020 at 13:55:00 (+0530), Viresh Kumar wrote: > > > > > static int __init cpufreq_core_init(void) > > > > > { > > > > > + struct cpufreq_governor *gov = cpufreq_default_governor(); > > > > > + > > > > > 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(default_governor)) > > > > > > > > Should we test '!strlen(default_governor) && gov' here actually? > > > > We check the return value of cpufreq_default_governor() in > > > > cpufreq_init_policy(), so I'm guessing we should do the same here to be > > > > on the safe side. > > > > > > With the current setup (the Kconfig option being a choice which > > > selects one governor at least), it is not possible for gov to be NULL > > > here. And so I didn't worry about it :) > > > > Right, so should we remove the check in cpufreq_init_policy() then? > > I don't mind either way as long as we are consitent :) > > And actually maybe we should remove the weakly defined > cpufreq_default_governor() implementation too? That'd make sure we get a > link-time error if for some reason things change in the Kconfig options. That would be fine I believe. I will do all that in a separate patch then and let this series go through with no more changes :) -- viresh
On Monday 29 Jun 2020 at 15:24:23 (+0530), Viresh Kumar wrote: > On 29-06-20, 10:50, Quentin Perret wrote: > > On Monday 29 Jun 2020 at 10:48:25 (+0100), Quentin Perret wrote: > > > On Monday 29 Jun 2020 at 15:16:27 (+0530), Viresh Kumar wrote: > > > > On 29-06-20, 10:44, Quentin Perret wrote: > > > > > On Monday 29 Jun 2020 at 13:55:00 (+0530), Viresh Kumar wrote: > > > > > > static int __init cpufreq_core_init(void) > > > > > > { > > > > > > + struct cpufreq_governor *gov = cpufreq_default_governor(); > > > > > > + > > > > > > 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(default_governor)) > > > > > > > > > > Should we test '!strlen(default_governor) && gov' here actually? > > > > > We check the return value of cpufreq_default_governor() in > > > > > cpufreq_init_policy(), so I'm guessing we should do the same here to be > > > > > on the safe side. > > > > > > > > With the current setup (the Kconfig option being a choice which > > > > selects one governor at least), it is not possible for gov to be NULL > > > > here. And so I didn't worry about it :) > > > > > > Right, so should we remove the check in cpufreq_init_policy() then? > > > I don't mind either way as long as we are consitent :) > > > > And actually maybe we should remove the weakly defined > > cpufreq_default_governor() implementation too? That'd make sure we get a > > link-time error if for some reason things change in the Kconfig options. > > That would be fine I believe. I will do all that in a separate patch > then and let this series go through with no more changes :) OK, that works for me. Thanks! Quentin
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index fb95fad81c79..8deb5a89328a 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -703,6 +703,11 @@ cpufreq.off=1 [CPU_FREQ] disable the cpufreq sub-system + cpufreq.default_governor= + [CPU_FREQ] Name of the default cpufreq governor or + policy to use. This governor must be registered in the + kernel before the cpufreq driver probes. + cpu_init_udelay=N [X86] Delay for N microsec between assert and de-assert of APIC INIT to start processors. This delay occurs diff --git a/Documentation/admin-guide/pm/cpufreq.rst b/Documentation/admin-guide/pm/cpufreq.rst index 0c74a7784964..368e612145d2 100644 --- a/Documentation/admin-guide/pm/cpufreq.rst +++ b/Documentation/admin-guide/pm/cpufreq.rst @@ -147,9 +147,9 @@ CPUs in it. The next major initialization step for a new policy object is to attach a scaling governor to it (to begin with, that is the default scaling governor -determined by the kernel configuration, but it may be changed later -via ``sysfs``). First, a pointer to the new policy object is passed to the -governor's ``->init()`` callback which is expected to initialize all of the +determined by the kernel command line or configuration, but it may be changed +later via ``sysfs``). First, a pointer to the new policy object is passed to +the governor's ``->init()`` callback which is expected to initialize all of the data structures necessary to handle the given policy and, possibly, to add a governor ``sysfs`` interface to it. Next, the governor is started by invoking its ``->start()`` callback. diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index e9e8200a0211..ad94b1d47ddb 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -50,6 +50,8 @@ static LIST_HEAD(cpufreq_governor_list); #define for_each_governor(__governor) \ list_for_each_entry(__governor, &cpufreq_governor_list, governor_list) +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 +1063,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; int ret; @@ -1071,21 +1072,27 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy) gov = get_governor(policy->last_governor); if (gov) { pr_debug("Restoring governor %s for cpu %d\n", - policy->governor->name, policy->cpu); - } else if (def_gov) { - gov = def_gov; - __module_get(gov->owner); + gov->name, policy->cpu); } else { - return -ENODATA; + gov = get_governor(default_governor); + } + + if (!gov) { + gov = cpufreq_default_governor(); + if (!gov) + return -ENODATA; + __module_get(gov->owner); } + } 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. */ @@ -2795,13 +2802,19 @@ EXPORT_SYMBOL_GPL(cpufreq_unregister_driver); static int __init cpufreq_core_init(void) { + struct cpufreq_governor *gov = cpufreq_default_governor(); + 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(default_governor)) + strncpy(default_governor, gov->name, CPUFREQ_NAME_LEN); + return 0; } module_param(off, int, 0444); +module_param_string(default_governor, default_governor, CPUFREQ_NAME_LEN, 0444); core_initcall(cpufreq_core_init);