Re: [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework

[Viresh Kumar <viresh.kumar@linaro.org>]

On 19-06-19, 00:23, Rafael J. Wysocki wrote:
> In patch [3/5] you could point notifiers for both min and max freq to the same
> notifier head.   Both of your notifiers end up calling cpufreq_update_policy()
> anyway.

I tried it and the changes in qos.c file look fine. But I don't like at all how
cpufreq.c looks now. We only register for min-freq notifier now and that takes
care of max as well. What could have been better is if we could have registered
a freq-notifier instead of min/max, which isn't possible as well because of how
qos framework works.

Honestly, the cpufreq changes look hacky to me :(

What do you say.

-- 
viresh

---
 drivers/base/power/qos.c  | 15 ++++++++-------
 drivers/cpufreq/cpufreq.c | 38 ++++++++------------------------------
 include/linux/cpufreq.h   |  3 +--
 3 files changed, 17 insertions(+), 39 deletions(-)

diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index cde2692b97f9..9bbf2d2a3376 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -202,20 +202,20 @@ static int dev_pm_qos_constraints_allocate(struct device *dev)
 	if (!qos)
 		return -ENOMEM;
 
-	n = kzalloc(3 * sizeof(*n), GFP_KERNEL);
+	n = kzalloc(2 * sizeof(*n), GFP_KERNEL);
 	if (!n) {
 		kfree(qos);
 		return -ENOMEM;
 	}
 
+	BLOCKING_INIT_NOTIFIER_HEAD(n);
 	c = &qos->resume_latency;
 	plist_head_init(&c->list);
 	c->target_value = PM_QOS_RESUME_LATENCY_DEFAULT_VALUE;
 	c->default_value = PM_QOS_RESUME_LATENCY_DEFAULT_VALUE;
 	c->no_constraint_value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
 	c->type = PM_QOS_MIN;
-	c->notifiers = n;
-	BLOCKING_INIT_NOTIFIER_HEAD(n);
+	c->notifiers = n++;
 
 	c = &qos->latency_tolerance;
 	plist_head_init(&c->list);
@@ -224,14 +224,16 @@ static int dev_pm_qos_constraints_allocate(struct device *dev)
 	c->no_constraint_value = PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT;
 	c->type = PM_QOS_MIN;
 
+	/* Same notifier head is used for both min/max frequency */
+	BLOCKING_INIT_NOTIFIER_HEAD(n);
+
 	c = &qos->min_frequency;
 	plist_head_init(&c->list);
 	c->target_value = PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE;
 	c->default_value = PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE;
 	c->no_constraint_value = PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE;
 	c->type = PM_QOS_MAX;
-	c->notifiers = ++n;
-	BLOCKING_INIT_NOTIFIER_HEAD(n);
+	c->notifiers = n;
 
 	c = &qos->max_frequency;
 	plist_head_init(&c->list);
@@ -239,8 +241,7 @@ static int dev_pm_qos_constraints_allocate(struct device *dev)
 	c->default_value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
 	c->no_constraint_value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
 	c->type = PM_QOS_MIN;
-	c->notifiers = ++n;
-	BLOCKING_INIT_NOTIFIER_HEAD(n);
+	c->notifiers = n;
 
 	INIT_LIST_HEAD(&qos->flags.list);
 
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 1344e1b1307f..1605dba1327e 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1139,19 +1139,10 @@ static int cpufreq_update_freq(struct cpufreq_policy *policy)
 	return 0;
 }
 
-static int cpufreq_notifier_min(struct notifier_block *nb, unsigned long freq,
+static int cpufreq_notifier_qos(struct notifier_block *nb, unsigned long freq,
 				void *data)
 {
-	struct cpufreq_policy *policy = container_of(nb, struct cpufreq_policy, nb_min);
-
-	return cpufreq_update_freq(policy);
-}
-
-static int cpufreq_notifier_max(struct notifier_block *nb, unsigned long freq,
-				void *data)
-{
-	struct cpufreq_policy *policy = container_of(nb, struct cpufreq_policy, nb_max);
+	struct cpufreq_policy *policy = container_of(nb, struct cpufreq_policy, nb_qos);
 
 	return cpufreq_update_freq(policy);
@@ -1214,10 +1205,10 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 		goto err_free_real_cpus;
 	}
 
-	policy->nb_min.notifier_call = cpufreq_notifier_min;
-	policy->nb_max.notifier_call = cpufreq_notifier_max;
+	policy->nb_qos.notifier_call = cpufreq_notifier_qos;
 
-	ret = dev_pm_qos_add_notifier(dev, &policy->nb_min,
+	/* Notifier for min frequency also takes care of max frequency notifier */
+	ret = dev_pm_qos_add_notifier(dev, &policy->nb_qos,
 				      DEV_PM_QOS_MIN_FREQUENCY);
 	if (ret) {
 		dev_err(dev, "Failed to register MIN QoS notifier: %d (%*pbl)\n",
@@ -1225,18 +1216,10 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 		goto err_kobj_remove;
 	}
 
-	ret = dev_pm_qos_add_notifier(dev, &policy->nb_max,
-				      DEV_PM_QOS_MAX_FREQUENCY);
-	if (ret) {
-		dev_err(dev, "Failed to register MAX QoS notifier: %d (%*pbl)\n",
-			ret, cpumask_pr_args(policy->cpus));
-		goto err_min_qos_notifier;
-	}
-
 	policy->min_freq_req = kzalloc(2 * sizeof(*policy->min_freq_req),
 				       GFP_KERNEL);
 	if (!policy->min_freq_req)
-		goto err_max_qos_notifier;
+		goto err_min_qos_notifier;
 
 	policy->max_freq_req = policy->min_freq_req + 1;
 	INIT_LIST_HEAD(&policy->policy_list);
@@ -1250,11 +1233,8 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 	policy->cpu = cpu;
 	return policy;
 
-err_max_qos_notifier:
-	dev_pm_qos_remove_notifier(dev, &policy->nb_max,
-				   DEV_PM_QOS_MAX_FREQUENCY);
 err_min_qos_notifier:
-	dev_pm_qos_remove_notifier(dev, &policy->nb_min,
+	dev_pm_qos_remove_notifier(dev, &policy->nb_qos,
 				   DEV_PM_QOS_MIN_FREQUENCY);
 err_kobj_remove:
 	cpufreq_policy_put_kobj(policy);
@@ -1284,9 +1264,7 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
 		per_cpu(cpufreq_cpu_data, cpu) = NULL;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
-	dev_pm_qos_remove_notifier(dev, &policy->nb_max,
-				   DEV_PM_QOS_MAX_FREQUENCY);
-	dev_pm_qos_remove_notifier(dev, &policy->nb_min,
+	dev_pm_qos_remove_notifier(dev, &policy->nb_qos,
 				   DEV_PM_QOS_MIN_FREQUENCY);
 	cancel_work_sync(&policy->req_work);
 	dev_pm_qos_remove_request(policy->max_freq_req);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 6bbed9af4fd2..2080d6490ed1 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -145,8 +145,7 @@ struct cpufreq_policy {
 	/* Pointer to the cooling device if used for thermal mitigation */
 	struct thermal_cooling_device *cdev;
 
-	struct notifier_block nb_min;
-	struct notifier_block nb_max;
+	struct notifier_block nb_qos;
 };
 
 struct cpufreq_freqs {