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

[Quentin Perret]

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