On Thu, Sep 03, 2020 at 02:45:08PM +0100, Ionela Voinescu wrote:

Hi Sudeep,

Thank you for your review here and for the other patches.

On Wednesday 02 Sep 2020 at 14:28:38 (+0100), Sudeep Holla wrote:
[..]

quoted

quoted

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 4d5fe777184a..570bf2ebe9d4 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c

@@ -61,6 +61,12 @@ static struct cpufreq_driver *cpufreq_driver;
 static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
 static DEFINE_RWLOCK(cpufreq_driver_lock);
 
+static DEFINE_STATIC_KEY_FALSE(cpufreq_freq_invariance);
+bool cpufreq_supports_freq_invariance(void)
+{
+	return static_branch_likely(&cpufreq_freq_invariance);
+}
+
 /* Flag to suspend/resume CPUFreq governors */
 static bool cpufreq_suspended;
 

@@ -2720,6 +2726,15 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
 	cpufreq_driver = driver_data;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
+	/*
+	 * Mark support for the scheduler's frequency invariance engine for
+	 * drivers that implement target(), target_index() or fast_switch().
+	 */
+	if (!cpufreq_driver->setpolicy) {
+		static_branch_enable_cpuslocked(&cpufreq_freq_invariance);
+		pr_debug("supports frequency invariance");
+	}
+
 	if (driver_data->setpolicy)

[super nit] while I understand cpufreq_driver = driver_data, it looks odd
if 2 consecutive statements refer it with different variables. Or am I
confusing myself hugely.

No, you are right. If you look at the rest of the register function,
after cpufreq_driver = driver_data, both driver_data and cpufreq_driver
are used. For me using cpufreq_driver seemed more natural as after being
assigned driver_data, it will continue to be used after registration.

Ah OK, I haven't seen the whole file/function, just looked at the patch.

If it's alright with you I won't make this change for now. It's possible
that a better solution is to change the other occurrences of either
cpufreq_driver or driver_data in a separate patch, to make things
consistent across the function.

I am fine to keep it as is, hence I mentioned it as super nit. If there
are other occurrences, then better to take it up separately.

-- 
Regards,
Sudeep