Re: [PATCH 2/2] cpufreq: Specify default governor on command line

On Tuesday 16 Jun 2020 at 14:57:59 (+0530), Viresh Kumar wrote:

There is another problem here which we need to look at. Any governor
which is built as a module and isn't currently used, should be allowed
to unload. And this needs to be tested by you as well, should be easy
enough.

With the current implementation, you take a reference to the default
governor when the driver is registered and drop it only when the
driver goes away. Which means we won't be able to unload the module of
the governor even if it isn't used. Which is wrong. The solution I
proposed had the same issue as well.

You need to figure out a way where we don't need to keep holding the
module hostage even when it isn't used. I see two ways at least for
the same:

- Do that from the existing place: cpufreq_init_policy().

- And I think this can be done from governor-register/unregister as
  well.

Second one sounds good, if it is feasible to do that.

Good point.

I'm thinking something along the lines of:

---8<---

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 0f05caedc320..a9219404e07f 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c

@@ -2340,6 +2340,11 @@ int cpufreq_register_governor(struct cpufreq_governor *governor)
 		list_add(&governor->governor_list, &cpufreq_governor_list);
 	}
 
+	if (!strncasecmp(cpufreq_param_governor, governor->name, CPUFREQ_NAME_LEN))
+		default_governor = governor;
+	else if (!default_governor && cpufreq_default_governor() == governor)
+		default_governor = cpufreq_default_governor();
+
 	mutex_unlock(&cpufreq_governor_mutex);
 	return err;
 }

@@ -2368,6 +2373,8 @@ void cpufreq_unregister_governor(struct cpufreq_governor *governor)
 
 	mutex_lock(&cpufreq_governor_mutex);
 	list_del(&governor->governor_list);
+	if (governor == default_governor)
+		default_governor = cpufreq_default_governor();
 	mutex_unlock(&cpufreq_governor_mutex);
 }
 EXPORT_SYMBOL_GPL(cpufreq_unregister_governor);
--->8---

should do the trick. That removes the unnecessary reference count, and
feels like a good place to hook things -- that is how cpuidle does it
too IIRC.

I'll double check the locking/synchronization, but that shouldn't be too
bad (famous last words).

Cheers,
Quentin