Re: [PATCH v2 2/2] cpufreq: Specify default governor on command line
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: 2020-06-25 10:53:01
Also in:
linux-pm, lkml
On Thu, Jun 25, 2020 at 10:50 AM Viresh Kumar [off-list ref] wrote:
On 24-06-20, 16:32, Quentin Perret wrote:quoted
Right, but I must admit that, looking at this more, I'm getting a bit confused with the overall locking for governors :/ When in cpufreq_init_policy() we find a governor using find_governor(policy->last_governor), what guarantees this governor is not concurrently unregistered? That is, what guarantees this governor doesn't go away between that find_governor() call, and the subsequent call to try_module_get() in cpufreq_set_policy() down the line? Can we somewhat assume that whatever governor is referred to by policy->last_governor will have a non-null refcount? Or are the cpufreq_online() and cpufreq_unregister_governor() path mutually exclusive? Or is there something else?This should be sufficient to fix pending issues I believe. Based over your patches.
LGTM, but can you post it in a new thread to let Patchwork pick it up?
quoted hunk ↗ jump to hunk
-------------------------8<------------------------- From: Viresh Kumar <viresh.kumar@linaro.org> Date: Thu, 25 Jun 2020 13:15:23 +0530 Subject: [PATCH] cpufreq: Fix locking issues with governors The locking around governors handling isn't adequate currently. The list of governors should never be traversed without locking in place. Also we must make sure the governor isn't removed while it is still referenced by code. Reported-by: Quentin Perret <redacted> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/cpufreq.c | 59 ++++++++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 23 deletions(-)diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 4b1a5c0173cf..dad6b85f4c89 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c@@ -624,6 +624,24 @@ static struct cpufreq_governor *find_governor(const char *str_governor) return NULL; } +static struct cpufreq_governor *get_governor(const char *str_governor) +{ + struct cpufreq_governor *t; + + mutex_lock(&cpufreq_governor_mutex); + t = find_governor(str_governor); + if (!t) + goto unlock; + + if (!try_module_get(t->owner)) + t = NULL; + +unlock: + mutex_unlock(&cpufreq_governor_mutex); + + return t; +} + static unsigned int cpufreq_parse_policy(char *str_governor) { if (!strncasecmp(str_governor, "performance", CPUFREQ_NAME_LEN))@@ -643,28 +661,14 @@ static struct cpufreq_governor *cpufreq_parse_governor(char *str_governor) { struct cpufreq_governor *t; - mutex_lock(&cpufreq_governor_mutex); - - t = find_governor(str_governor); - if (!t) { - int ret; - - mutex_unlock(&cpufreq_governor_mutex); - - ret = request_module("cpufreq_%s", str_governor); - if (ret) - return NULL; - - mutex_lock(&cpufreq_governor_mutex); + t = get_governor(str_governor); + if (t) + return t; - t = find_governor(str_governor); - } - if (t && !try_module_get(t->owner)) - t = NULL; - - mutex_unlock(&cpufreq_governor_mutex); + if (request_module("cpufreq_%s", str_governor)) + return NULL; - return t; + return get_governor(str_governor); } /**@@ -818,12 +822,14 @@ static ssize_t show_scaling_available_governors(struct cpufreq_policy *policy, goto out; } + mutex_lock(&cpufreq_governor_mutex); for_each_governor(t) { if (i >= (ssize_t) ((PAGE_SIZE / sizeof(char)) - (CPUFREQ_NAME_LEN + 2))) - goto out; + break; i += scnprintf(&buf[i], CPUFREQ_NAME_PLEN, "%s ", t->name); } + mutex_unlock(&cpufreq_governor_mutex); out: i += sprintf(&buf[i], "\n"); return i;@@ -1060,11 +1066,14 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy) { struct cpufreq_governor *gov = NULL; unsigned int pol = CPUFREQ_POLICY_UNKNOWN; + bool put_governor = false; + int ret; if (has_target()) { /* Update policy governor to the one used before hotplug. */ - gov = find_governor(policy->last_governor); + 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 (default_governor) {@@ -1091,7 +1100,11 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy) return -ENODATA; } - return cpufreq_set_policy(policy, gov, pol); + ret = cpufreq_set_policy(policy, gov, pol); + if (put_governor) + module_put(gov->owner); + + return ret; } static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)