Re: [PATCH] cpufreq: Fix locking issues with governors

From: Quentin Perret
Date: Thu Jun 25 2020 - 07:15:01 EST


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@xxxxxxxxxx, '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