| Message ID | 49c7d64460cdb39b006991e5251260eb0eea9f2a.1593082448.git.viresh.kumar@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cpufreq: Fix locking issues with governors | expand |
Hey Viresh On Thursday 25 Jun 2020 at 16:24:16 (+0530), Viresh Kumar wrote: > 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. Thanks for having a look at this! This solves the issue for the reference to policy->last_governor, but given that your patch is based on top of 20200623142138.209513-3-qperret@google.com, 'default_governor' needs a similar treatment I think. Perhaps something along the lines of the (completely untested) snippet below? diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index dad6b85f4c89..9d7cf2ce2768 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1062,6 +1062,17 @@ __weak struct cpufreq_governor *cpufreq_default_governor(void) return NULL; } +static bool get_default_governor(void) +{ + bool ret; + + mutex_lock(&cpufreq_governor_mutex); + ret = default_governor && !try_module_get(default_governor->owner); + mutex_unlock(&cpufreq_governor_mutex); + + return ret; +} + static int cpufreq_init_policy(struct cpufreq_policy *policy) { struct cpufreq_governor *gov = NULL; @@ -1073,20 +1084,21 @@ 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 (default_governor) { + } else if (get_default_governor()) { gov = default_governor; } else { return -ENODATA; } + put_governor = true; } else { /* Use the default policy if there is no last_policy. */ if (policy->last_policy) { pol = policy->last_policy; - } else if (default_governor) { + } else if (get_default_governor()) { pol = cpufreq_parse_policy(default_governor->name); + module_put(default_governor->owner); /* * In case the default governor is neiter "performance" * nor "powersave", fall back to the initial policy
On Thu, Jun 25, 2020 at 1:14 PM Quentin Perret <qperret@google.com> wrote: > > Hey Viresh > > On Thursday 25 Jun 2020 at 16:24:16 (+0530), Viresh Kumar wrote: > > 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. > > Thanks for having a look at this! > > This solves the issue for the reference to policy->last_governor, but > given that your patch is based on top of > 20200623142138.209513-3-qperret@google.com, 'default_governor' needs a > similar treatment I think. So I would prefer to rebase the $subject patch from Viresh on top of the current mainline, apply it first and rebase the "default governor" series on top of it - and include the changes needed for the default governor handling in there. Thanks!
On Thursday 25 Jun 2020 at 15:32:43 (+0200), Rafael J. Wysocki wrote: > On Thu, Jun 25, 2020 at 1:14 PM Quentin Perret <qperret@google.com> wrote: > > > > Hey Viresh > > > > On Thursday 25 Jun 2020 at 16:24:16 (+0530), Viresh Kumar wrote: > > > 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. > > > > Thanks for having a look at this! > > > > This solves the issue for the reference to policy->last_governor, but > > given that your patch is based on top of > > 20200623142138.209513-3-qperret@google.com, 'default_governor' needs a > > similar treatment I think. > > So I would prefer to rebase the $subject patch from Viresh on top of > the current mainline, apply it first and rebase the "default governor" > series on top of it - and include the changes needed for the default > governor handling in there. Right, and Viresh's patch might be -stable material too? In any case, making it standalone makes a lot of sense. Thanks, Quentin
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)
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 <qperret@google.com> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/cpufreq.c | 59 ++++++++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 23 deletions(-) -- 2.25.0.rc1.19.g042ed3e048af