Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH V3 0/5] cpufreq: Use QoS layer to manage freq-constraints
@ 2019-06-10 10:51 Viresh Kumar
  2019-06-10 10:51 ` [PATCH V3 1/5] PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier() Viresh Kumar
                   ` (4 more replies)
  0 siblings, 5 replies; 27+ messages in thread
From: Viresh Kumar @ 2019-06-10 10:51 UTC (permalink / raw)
  To: Rafael Wysocki, Daniel Lezcano, Kevin Hilman, Len Brown,
	Pavel Machek, Ulf Hansson
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Qais.Yousef, mka,
	juri.lelli, linux-kernel

Hello,

This patchset attempts to manage CPU frequency constraints using the PM
QoS framework. It only does the basic stuff right now and moves the
userspace constraints to use the QoS infrastructure.

Todo:
- Migrate all users to the QoS framework and get rid of cpufreq specific
  notifiers.
- Make PM QoS learn about the relation of CPUs in a policy, so a single
  list of constraints is managed for all of them instead of per-cpu
  constraints.

V2->V3:
- Add a comment in cpufreq.c as suggested by Qais.
- Rebased on latest pm/linux-next.

V1->V2:
- The previous version introduced a completely new framework, this one
  moves to PM QoS instead.
- Lots of changes because of this.

--
viresh

Viresh Kumar (5):
  PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier()
  PM / QOS: Pass request type to dev_pm_qos_read_value()
  PM / QoS: Add support for MIN/MAX frequency constraints
  cpufreq: Register notifiers with the PM QoS framework
  cpufreq: Add QoS requests for userspace constraints

 Documentation/power/pm_qos_interface.txt |  12 +-
 drivers/base/power/domain.c              |   8 +-
 drivers/base/power/domain_governor.c     |   4 +-
 drivers/base/power/qos.c                 | 115 +++++++++++--
 drivers/base/power/runtime.c             |   2 +-
 drivers/cpufreq/cpufreq.c                | 203 ++++++++++++++++-------
 drivers/cpuidle/governor.c               |   2 +-
 include/linux/cpufreq.h                  |  12 +-
 include/linux/pm_qos.h                   |  71 ++++++--
 9 files changed, 325 insertions(+), 104 deletions(-)

-- 
2.21.0.rc0.269.g1a574e7a288b


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH V3 1/5] PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier()
  2019-06-10 10:51 [PATCH V3 0/5] cpufreq: Use QoS layer to manage freq-constraints Viresh Kumar
@ 2019-06-10 10:51 ` Viresh Kumar
  2019-06-11 23:45   ` Matthias Kaehlcke
                     ` (2 more replies)
  2019-06-10 10:51 ` [PATCH V3 2/5] PM / QOS: Pass request type to dev_pm_qos_read_value() Viresh Kumar
                   ` (3 subsequent siblings)
  4 siblings, 3 replies; 27+ messages in thread
From: Viresh Kumar @ 2019-06-10 10:51 UTC (permalink / raw)
  To: Rafael Wysocki, Len Brown, Pavel Machek, Kevin Hilman, Ulf Hansson
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Qais.Yousef, mka,
	juri.lelli, linux-kernel

In order to use the same set of routines to register notifiers for
different request types, update the existing
dev_pm_qos_{add|remove}_notifier() routines with an additional
parameter: request-type.

For now, it only supports resume-latency request type.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/power/pm_qos_interface.txt | 10 ++++++----
 drivers/base/power/domain.c              |  8 +++++---
 drivers/base/power/qos.c                 | 14 ++++++++++++--
 include/linux/pm_qos.h                   | 12 ++++++++----
 4 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt
index 19c5f7b1a7ba..ec7d662d1707 100644
--- a/Documentation/power/pm_qos_interface.txt
+++ b/Documentation/power/pm_qos_interface.txt
@@ -164,12 +164,14 @@ directory.
 Notification mechanisms:
 The per-device PM QoS framework has a per-device notification tree.
 
-int dev_pm_qos_add_notifier(device, notifier):
-Adds a notification callback function for the device.
+int dev_pm_qos_add_notifier(device, notifier, type):
+Adds a notification callback function for the device for a particular request
+type.
+
 The callback is called when the aggregated value of the device constraints list
-is changed (for resume latency device PM QoS only).
+is changed.
 
-int dev_pm_qos_remove_notifier(device, notifier):
+int dev_pm_qos_remove_notifier(device, notifier, type):
 Removes the notification callback function for the device.
 
 
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 33c30c1e6a30..b063bc41b0a9 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1536,7 +1536,8 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	if (ret)
 		genpd_free_dev_data(dev, gpd_data);
 	else
-		dev_pm_qos_add_notifier(dev, &gpd_data->nb);
+		dev_pm_qos_add_notifier(dev, &gpd_data->nb,
+					DEV_PM_QOS_RESUME_LATENCY);
 
 	return ret;
 }
@@ -1569,7 +1570,8 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
 
 	pdd = dev->power.subsys_data->domain_data;
 	gpd_data = to_gpd_data(pdd);
-	dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
+	dev_pm_qos_remove_notifier(dev, &gpd_data->nb,
+				   DEV_PM_QOS_RESUME_LATENCY);
 
 	genpd_lock(genpd);
 
@@ -1597,7 +1599,7 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
 
  out:
 	genpd_unlock(genpd);
-	dev_pm_qos_add_notifier(dev, &gpd_data->nb);
+	dev_pm_qos_add_notifier(dev, &gpd_data->nb, DEV_PM_QOS_RESUME_LATENCY);
 
 	return ret;
 }
diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index 6c91f8df1d59..cfd463212513 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -467,6 +467,7 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request);
  *
  * @dev: target device for the constraint
  * @notifier: notifier block managed by caller.
+ * @type: request type.
  *
  * Will register the notifier into a notification chain that gets called
  * upon changes to the target value for the device.
@@ -474,10 +475,14 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request);
  * If the device's constraints object doesn't exist when this routine is called,
  * it will be created (or error code will be returned if that fails).
  */
-int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier)
+int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier,
+			    enum dev_pm_qos_req_type type)
 {
 	int ret = 0;
 
+	if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
+		return -EINVAL;
+
 	mutex_lock(&dev_pm_qos_mtx);
 
 	if (IS_ERR(dev->power.qos))
@@ -500,15 +505,20 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_add_notifier);
  *
  * @dev: target device for the constraint
  * @notifier: notifier block to be removed.
+ * @type: request type.
  *
  * Will remove the notifier from the notification chain that gets called
  * upon changes to the target value.
  */
 int dev_pm_qos_remove_notifier(struct device *dev,
-			       struct notifier_block *notifier)
+			       struct notifier_block *notifier,
+			       enum dev_pm_qos_req_type type)
 {
 	int retval = 0;
 
+	if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
+		return -EINVAL;
+
 	mutex_lock(&dev_pm_qos_mtx);
 
 	/* Silently return if the constraints object is not present. */
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index 6ea1ae373d77..1f4d456e8fff 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -146,9 +146,11 @@ int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
 int dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value);
 int dev_pm_qos_remove_request(struct dev_pm_qos_request *req);
 int dev_pm_qos_add_notifier(struct device *dev,
-			    struct notifier_block *notifier);
+			    struct notifier_block *notifier,
+			    enum dev_pm_qos_req_type type);
 int dev_pm_qos_remove_notifier(struct device *dev,
-			       struct notifier_block *notifier);
+			       struct notifier_block *notifier,
+			       enum dev_pm_qos_req_type type);
 void dev_pm_qos_constraints_init(struct device *dev);
 void dev_pm_qos_constraints_destroy(struct device *dev);
 int dev_pm_qos_add_ancestor_request(struct device *dev,
@@ -202,10 +204,12 @@ static inline int dev_pm_qos_update_request(struct dev_pm_qos_request *req,
 static inline int dev_pm_qos_remove_request(struct dev_pm_qos_request *req)
 			{ return 0; }
 static inline int dev_pm_qos_add_notifier(struct device *dev,
-					  struct notifier_block *notifier)
+					  struct notifier_block *notifier,
+					  enum dev_pm_qos_req_type type);
 			{ return 0; }
 static inline int dev_pm_qos_remove_notifier(struct device *dev,
-					     struct notifier_block *notifier)
+					     struct notifier_block *notifier,
+					     enum dev_pm_qos_req_type type)
 			{ return 0; }
 static inline void dev_pm_qos_constraints_init(struct device *dev)
 {
-- 
2.21.0.rc0.269.g1a574e7a288b


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH V3 2/5] PM / QOS: Pass request type to dev_pm_qos_read_value()
  2019-06-10 10:51 [PATCH V3 0/5] cpufreq: Use QoS layer to manage freq-constraints Viresh Kumar
  2019-06-10 10:51 ` [PATCH V3 1/5] PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier() Viresh Kumar
@ 2019-06-10 10:51 ` Viresh Kumar
  2019-06-12  0:08   ` Matthias Kaehlcke
                     ` (2 more replies)
  2019-06-10 10:51 ` [PATCH V3 3/5] PM / QoS: Add support for MIN/MAX frequency constraints Viresh Kumar
                   ` (2 subsequent siblings)
  4 siblings, 3 replies; 27+ messages in thread
From: Viresh Kumar @ 2019-06-10 10:51 UTC (permalink / raw)
  To: Rafael Wysocki, Len Brown, Pavel Machek, Kevin Hilman,
	Ulf Hansson, Daniel Lezcano
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Qais.Yousef, mka,
	juri.lelli, linux-kernel

In order to use dev_pm_qos_read_value(), and other internal routines to
it, to read values for different QoS requests, pass request type as a
parameter to these routines.

For now, it only supports resume-latency request type.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 Documentation/power/pm_qos_interface.txt |  2 +-
 drivers/base/power/domain_governor.c     |  4 +--
 drivers/base/power/qos.c                 | 10 +++---
 drivers/base/power/runtime.c             |  2 +-
 drivers/cpuidle/governor.c               |  2 +-
 include/linux/pm_qos.h                   | 41 +++++++++++++++++-------
 6 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt
index ec7d662d1707..cfcb1df39799 100644
--- a/Documentation/power/pm_qos_interface.txt
+++ b/Documentation/power/pm_qos_interface.txt
@@ -123,7 +123,7 @@ Will remove the element.  After removal it will update the aggregate target and
 call the notification trees if the target was changed as a result of removing
 the request.
 
-s32 dev_pm_qos_read_value(device):
+s32 dev_pm_qos_read_value(device, type):
 Returns the aggregated value for a given device's constraints list.
 
 enum pm_qos_flags_status dev_pm_qos_flags(device, mask)
diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
index 3838045c9277..f66ac46d07d0 100644
--- a/drivers/base/power/domain_governor.c
+++ b/drivers/base/power/domain_governor.c
@@ -33,7 +33,7 @@ static int dev_update_qos_constraint(struct device *dev, void *data)
 		 * take its current PM QoS constraint (that's the only thing
 		 * known at this point anyway).
 		 */
-		constraint_ns = dev_pm_qos_read_value(dev);
+		constraint_ns = dev_pm_qos_read_value(dev, DEV_PM_QOS_RESUME_LATENCY);
 		constraint_ns *= NSEC_PER_USEC;
 	}
 
@@ -66,7 +66,7 @@ static bool default_suspend_ok(struct device *dev)
 	td->constraint_changed = false;
 	td->cached_suspend_ok = false;
 	td->effective_constraint_ns = 0;
-	constraint_ns = __dev_pm_qos_read_value(dev);
+	constraint_ns = __dev_pm_qos_read_value(dev, DEV_PM_QOS_RESUME_LATENCY);
 
 	spin_unlock_irqrestore(&dev->power.lock, flags);
 
diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index cfd463212513..7bb88174371f 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -92,27 +92,29 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_flags);
 /**
  * __dev_pm_qos_read_value - Get PM QoS constraint for a given device.
  * @dev: Device to get the PM QoS constraint value for.
+ * @type: QoS request type.
  *
  * This routine must be called with dev->power.lock held.
  */
-s32 __dev_pm_qos_read_value(struct device *dev)
+s32 __dev_pm_qos_read_value(struct device *dev, enum dev_pm_qos_req_type type)
 {
 	lockdep_assert_held(&dev->power.lock);
 
-	return dev_pm_qos_raw_read_value(dev);
+	return dev_pm_qos_raw_read_value(dev, type);
 }
 
 /**
  * dev_pm_qos_read_value - Get PM QoS constraint for a given device (locked).
  * @dev: Device to get the PM QoS constraint value for.
+ * @type: QoS request type.
  */
-s32 dev_pm_qos_read_value(struct device *dev)
+s32 dev_pm_qos_read_value(struct device *dev, enum dev_pm_qos_req_type type)
 {
 	unsigned long flags;
 	s32 ret;
 
 	spin_lock_irqsave(&dev->power.lock, flags);
-	ret = __dev_pm_qos_read_value(dev);
+	ret = __dev_pm_qos_read_value(dev, type);
 	spin_unlock_irqrestore(&dev->power.lock, flags);
 
 	return ret;
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 952a1e7057c7..92ffc2a0151d 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -275,7 +275,7 @@ static int rpm_check_suspend_allowed(struct device *dev)
 	    || (dev->power.request_pending
 			&& dev->power.request == RPM_REQ_RESUME))
 		retval = -EAGAIN;
-	else if (__dev_pm_qos_read_value(dev) == 0)
+	else if (__dev_pm_qos_read_value(dev, DEV_PM_QOS_RESUME_LATENCY) == 0)
 		retval = -EPERM;
 	else if (dev->power.runtime_status == RPM_SUSPENDED)
 		retval = 1;
diff --git a/drivers/cpuidle/governor.c b/drivers/cpuidle/governor.c
index 9fddf828a76f..fb5659eb2009 100644
--- a/drivers/cpuidle/governor.c
+++ b/drivers/cpuidle/governor.c
@@ -110,7 +110,7 @@ int cpuidle_governor_latency_req(unsigned int cpu)
 {
 	int global_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
 	struct device *device = get_cpu_device(cpu);
-	int device_req = dev_pm_qos_raw_read_value(device);
+	int device_req = dev_pm_qos_raw_read_value(device, DEV_PM_QOS_RESUME_LATENCY);
 
 	return device_req < global_req ? device_req : global_req;
 }
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index 1f4d456e8fff..55814d48c39c 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -139,8 +139,8 @@ s32 pm_qos_read_value(struct pm_qos_constraints *c);
 #ifdef CONFIG_PM
 enum pm_qos_flags_status __dev_pm_qos_flags(struct device *dev, s32 mask);
 enum pm_qos_flags_status dev_pm_qos_flags(struct device *dev, s32 mask);
-s32 __dev_pm_qos_read_value(struct device *dev);
-s32 dev_pm_qos_read_value(struct device *dev);
+s32 __dev_pm_qos_read_value(struct device *dev, enum dev_pm_qos_req_type type);
+s32 dev_pm_qos_read_value(struct device *dev, enum dev_pm_qos_req_type type);
 int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
 			   enum dev_pm_qos_req_type type, s32 value);
 int dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value);
@@ -176,11 +176,19 @@ static inline s32 dev_pm_qos_requested_flags(struct device *dev)
 	return dev->power.qos->flags_req->data.flr.flags;
 }
 
-static inline s32 dev_pm_qos_raw_read_value(struct device *dev)
+static inline s32 dev_pm_qos_raw_read_value(struct device *dev,
+					    enum dev_pm_qos_req_type type)
 {
-	return IS_ERR_OR_NULL(dev->power.qos) ?
-		PM_QOS_RESUME_LATENCY_NO_CONSTRAINT :
-		pm_qos_read_value(&dev->power.qos->resume_latency);
+	struct dev_pm_qos *qos = dev->power.qos;
+
+	switch (type) {
+	case DEV_PM_QOS_RESUME_LATENCY:
+		return IS_ERR_OR_NULL(qos) ? PM_QOS_RESUME_LATENCY_NO_CONSTRAINT
+			: pm_qos_read_value(&qos->resume_latency);
+	default:
+		WARN_ON(1);
+		return 0;
+	}
 }
 #else
 static inline enum pm_qos_flags_status __dev_pm_qos_flags(struct device *dev,
@@ -189,10 +197,6 @@ static inline enum pm_qos_flags_status __dev_pm_qos_flags(struct device *dev,
 static inline enum pm_qos_flags_status dev_pm_qos_flags(struct device *dev,
 							s32 mask)
 			{ return PM_QOS_FLAGS_UNDEFINED; }
-static inline s32 __dev_pm_qos_read_value(struct device *dev)
-			{ return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT; }
-static inline s32 dev_pm_qos_read_value(struct device *dev)
-			{ return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT; }
 static inline int dev_pm_qos_add_request(struct device *dev,
 					 struct dev_pm_qos_request *req,
 					 enum dev_pm_qos_req_type type,
@@ -245,10 +249,23 @@ static inline s32 dev_pm_qos_requested_resume_latency(struct device *dev)
 	return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
 }
 static inline s32 dev_pm_qos_requested_flags(struct device *dev) { return 0; }
-static inline s32 dev_pm_qos_raw_read_value(struct device *dev)
+
+static inline s32 dev_pm_qos_raw_read_value(struct device *dev,
+					    enum dev_pm_qos_req_type type)
 {
-	return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
+	switch type {
+	case DEV_PM_QOS_RESUME_LATENCY:
+		return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
+	default:
+		WARN_ON(1);
+		return 0;
+	}
 }
+
+static inline s32 __dev_pm_qos_read_value(struct device *dev, enum dev_pm_qos_req_type type)
+			{ return dev_pm_qos_raw_read_value(dev, type); }
+static inline s32 dev_pm_qos_read_value(struct device *dev, enum dev_pm_qos_req_type type)
+			{ return dev_pm_qos_raw_read_value(dev, type); }
 #endif
 
 #endif
-- 
2.21.0.rc0.269.g1a574e7a288b


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH V3 3/5] PM / QoS: Add support for MIN/MAX frequency constraints
  2019-06-10 10:51 [PATCH V3 0/5] cpufreq: Use QoS layer to manage freq-constraints Viresh Kumar
  2019-06-10 10:51 ` [PATCH V3 1/5] PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier() Viresh Kumar
  2019-06-10 10:51 ` [PATCH V3 2/5] PM / QOS: Pass request type to dev_pm_qos_read_value() Viresh Kumar
@ 2019-06-10 10:51 ` Viresh Kumar
  2019-06-13  0:04   ` Matthias Kaehlcke
  2019-06-17  9:23   ` Ulf Hansson
  2019-06-10 10:51 ` [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework Viresh Kumar
  2019-06-10 10:51 ` [PATCH V3 5/5] cpufreq: Add QoS requests for userspace constraints Viresh Kumar
  4 siblings, 2 replies; 27+ messages in thread
From: Viresh Kumar @ 2019-06-10 10:51 UTC (permalink / raw)
  To: Rafael Wysocki, Pavel Machek, Len Brown
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Qais.Yousef, mka,
	juri.lelli, linux-kernel

This patch introduces the min-frequency and max-frequency device
constraints, which will be used by the cpufreq core to begin with.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/power/qos.c | 103 +++++++++++++++++++++++++++++++++------
 include/linux/pm_qos.h   |  18 +++++++
 2 files changed, 107 insertions(+), 14 deletions(-)

diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index 7bb88174371f..e977aef437a5 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -151,6 +151,14 @@ static int apply_constraint(struct dev_pm_qos_request *req,
 			req->dev->power.set_latency_tolerance(req->dev, value);
 		}
 		break;
+	case DEV_PM_QOS_MIN_FREQUENCY:
+		ret = pm_qos_update_target(&qos->min_frequency,
+					   &req->data.pnode, action, value);
+		break;
+	case DEV_PM_QOS_MAX_FREQUENCY:
+		ret = pm_qos_update_target(&qos->max_frequency,
+					   &req->data.pnode, action, value);
+		break;
 	case DEV_PM_QOS_FLAGS:
 		ret = pm_qos_update_flags(&qos->flags, &req->data.flr,
 					  action, value);
@@ -179,12 +187,11 @@ static int dev_pm_qos_constraints_allocate(struct device *dev)
 	if (!qos)
 		return -ENOMEM;
 
-	n = kzalloc(sizeof(*n), GFP_KERNEL);
+	n = kzalloc(3 * sizeof(*n), GFP_KERNEL);
 	if (!n) {
 		kfree(qos);
 		return -ENOMEM;
 	}
-	BLOCKING_INIT_NOTIFIER_HEAD(n);
 
 	c = &qos->resume_latency;
 	plist_head_init(&c->list);
@@ -193,6 +200,7 @@ static int dev_pm_qos_constraints_allocate(struct device *dev)
 	c->no_constraint_value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
 	c->type = PM_QOS_MIN;
 	c->notifiers = n;
+	BLOCKING_INIT_NOTIFIER_HEAD(n);
 
 	c = &qos->latency_tolerance;
 	plist_head_init(&c->list);
@@ -201,6 +209,24 @@ 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;
 
+	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 = &qos->max_frequency;
+	plist_head_init(&c->list);
+	c->target_value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
+	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);
+
 	INIT_LIST_HEAD(&qos->flags.list);
 
 	spin_lock_irq(&dev->power.lock);
@@ -254,11 +280,25 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
 		apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
 		memset(req, 0, sizeof(*req));
 	}
+
 	c = &qos->latency_tolerance;
 	plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) {
 		apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
 		memset(req, 0, sizeof(*req));
 	}
+
+	c = &qos->min_frequency;
+	plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) {
+		apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE);
+		memset(req, 0, sizeof(*req));
+	}
+
+	c = &qos->max_frequency;
+	plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) {
+		apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
+		memset(req, 0, sizeof(*req));
+	}
+
 	f = &qos->flags;
 	list_for_each_entry_safe(req, tmp, &f->list, data.flr.node) {
 		apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
@@ -370,6 +410,8 @@ static int __dev_pm_qos_update_request(struct dev_pm_qos_request *req,
 	switch(req->type) {
 	case DEV_PM_QOS_RESUME_LATENCY:
 	case DEV_PM_QOS_LATENCY_TOLERANCE:
+	case DEV_PM_QOS_MIN_FREQUENCY:
+	case DEV_PM_QOS_MAX_FREQUENCY:
 		curr_value = req->data.pnode.prio;
 		break;
 	case DEV_PM_QOS_FLAGS:
@@ -482,9 +524,6 @@ int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier,
 {
 	int ret = 0;
 
-	if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
-		return -EINVAL;
-
 	mutex_lock(&dev_pm_qos_mtx);
 
 	if (IS_ERR(dev->power.qos))
@@ -492,10 +531,28 @@ int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier,
 	else if (!dev->power.qos)
 		ret = dev_pm_qos_constraints_allocate(dev);
 
-	if (!ret)
+	if (ret)
+		goto unlock;
+
+	switch (type) {
+	case DEV_PM_QOS_RESUME_LATENCY:
 		ret = blocking_notifier_chain_register(dev->power.qos->resume_latency.notifiers,
 						       notifier);
+		break;
+	case DEV_PM_QOS_MIN_FREQUENCY:
+		ret = blocking_notifier_chain_register(dev->power.qos->min_frequency.notifiers,
+						       notifier);
+		break;
+	case DEV_PM_QOS_MAX_FREQUENCY:
+		ret = blocking_notifier_chain_register(dev->power.qos->max_frequency.notifiers,
+						       notifier);
+		break;
+	default:
+		WARN_ON(1);
+		ret = -EINVAL;
+	}
 
+unlock:
 	mutex_unlock(&dev_pm_qos_mtx);
 	return ret;
 }
@@ -516,20 +573,35 @@ int dev_pm_qos_remove_notifier(struct device *dev,
 			       struct notifier_block *notifier,
 			       enum dev_pm_qos_req_type type)
 {
-	int retval = 0;
-
-	if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
-		return -EINVAL;
+	int ret = 0;
 
 	mutex_lock(&dev_pm_qos_mtx);
 
 	/* Silently return if the constraints object is not present. */
-	if (!IS_ERR_OR_NULL(dev->power.qos))
-		retval = blocking_notifier_chain_unregister(dev->power.qos->resume_latency.notifiers,
-							    notifier);
+	if (IS_ERR_OR_NULL(dev->power.qos))
+		goto unlock;
 
+	switch (type) {
+	case DEV_PM_QOS_RESUME_LATENCY:
+		ret = blocking_notifier_chain_unregister(dev->power.qos->resume_latency.notifiers,
+							 notifier);
+		break;
+	case DEV_PM_QOS_MIN_FREQUENCY:
+		ret = blocking_notifier_chain_unregister(dev->power.qos->min_frequency.notifiers,
+							 notifier);
+		break;
+	case DEV_PM_QOS_MAX_FREQUENCY:
+		ret = blocking_notifier_chain_unregister(dev->power.qos->max_frequency.notifiers,
+							 notifier);
+		break;
+	default:
+		WARN_ON(1);
+		ret = -EINVAL;
+	}
+
+unlock:
 	mutex_unlock(&dev_pm_qos_mtx);
-	return retval;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_qos_remove_notifier);
 
@@ -589,6 +661,9 @@ static void __dev_pm_qos_drop_user_request(struct device *dev,
 		req = dev->power.qos->flags_req;
 		dev->power.qos->flags_req = NULL;
 		break;
+	default:
+		WARN_ON(1);
+		return;
 	}
 	__dev_pm_qos_remove_request(req);
 	kfree(req);
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index 55814d48c39c..3f994283a5ae 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -40,6 +40,8 @@ enum pm_qos_flags_status {
 #define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT	PM_QOS_LATENCY_ANY
 #define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS	PM_QOS_LATENCY_ANY_NS
 #define PM_QOS_LATENCY_TOLERANCE_DEFAULT_VALUE	0
+#define PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE	0
+#define PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE	(-1)
 #define PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT	(-1)
 
 #define PM_QOS_FLAG_NO_POWER_OFF	(1 << 0)
@@ -58,6 +60,8 @@ struct pm_qos_flags_request {
 enum dev_pm_qos_req_type {
 	DEV_PM_QOS_RESUME_LATENCY = 1,
 	DEV_PM_QOS_LATENCY_TOLERANCE,
+	DEV_PM_QOS_MIN_FREQUENCY,
+	DEV_PM_QOS_MAX_FREQUENCY,
 	DEV_PM_QOS_FLAGS,
 };
 
@@ -99,10 +103,14 @@ struct pm_qos_flags {
 struct dev_pm_qos {
 	struct pm_qos_constraints resume_latency;
 	struct pm_qos_constraints latency_tolerance;
+	struct pm_qos_constraints min_frequency;
+	struct pm_qos_constraints max_frequency;
 	struct pm_qos_flags flags;
 	struct dev_pm_qos_request *resume_latency_req;
 	struct dev_pm_qos_request *latency_tolerance_req;
 	struct dev_pm_qos_request *flags_req;
+	struct dev_pm_qos_request *min_frequency_req;
+	struct dev_pm_qos_request *max_frequency_req;
 };
 
 /* Action requested to pm_qos_update_target */
@@ -185,6 +193,12 @@ static inline s32 dev_pm_qos_raw_read_value(struct device *dev,
 	case DEV_PM_QOS_RESUME_LATENCY:
 		return IS_ERR_OR_NULL(qos) ? PM_QOS_RESUME_LATENCY_NO_CONSTRAINT
 			: pm_qos_read_value(&qos->resume_latency);
+	case DEV_PM_QOS_MIN_FREQUENCY:
+		return IS_ERR_OR_NULL(qos) ? PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE
+			: pm_qos_read_value(&qos->min_frequency);
+	case DEV_PM_QOS_MAX_FREQUENCY:
+		return IS_ERR_OR_NULL(qos) ? PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE
+			: pm_qos_read_value(&qos->max_frequency);
 	default:
 		WARN_ON(1);
 		return 0;
@@ -256,6 +270,10 @@ static inline s32 dev_pm_qos_raw_read_value(struct device *dev,
 	switch type {
 	case DEV_PM_QOS_RESUME_LATENCY:
 		return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
+	case DEV_PM_QOS_MIN_FREQUENCY:
+		return PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE;
+	case DEV_PM_QOS_MAX_FREQUENCY:
+		return PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
 	default:
 		WARN_ON(1);
 		return 0;
-- 
2.21.0.rc0.269.g1a574e7a288b


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework
  2019-06-10 10:51 [PATCH V3 0/5] cpufreq: Use QoS layer to manage freq-constraints Viresh Kumar
                   ` (2 preceding siblings ...)
  2019-06-10 10:51 ` [PATCH V3 3/5] PM / QoS: Add support for MIN/MAX frequency constraints Viresh Kumar
@ 2019-06-10 10:51 ` Viresh Kumar
  2019-06-14 16:46   ` Matthias Kaehlcke
                     ` (3 more replies)
  2019-06-10 10:51 ` [PATCH V3 5/5] cpufreq: Add QoS requests for userspace constraints Viresh Kumar
  4 siblings, 4 replies; 27+ messages in thread
From: Viresh Kumar @ 2019-06-10 10:51 UTC (permalink / raw)
  To: Rafael Wysocki
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Qais.Yousef, mka,
	juri.lelli, linux-kernel

This registers the notifiers for min/max frequency constraints with the
PM QoS framework. The constraints are also taken into consideration in
cpufreq_set_policy().

This also relocates cpufreq_policy_put_kobj() as it is required to be
called from cpufreq_policy_alloc() now.

No constraints are added until now though.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 139 +++++++++++++++++++++++++++++++-------
 include/linux/cpufreq.h   |   4 ++
 2 files changed, 120 insertions(+), 23 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 85ff958e01f1..547d221b2ff2 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -26,6 +26,7 @@
 #include <linux/kernel_stat.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/pm_qos.h>
 #include <linux/slab.h>
 #include <linux/suspend.h>
 #include <linux/syscore_ops.h>
@@ -1126,11 +1127,77 @@ static void handle_update(struct work_struct *work)
 	cpufreq_update_policy(cpu);
 }
 
+static void cpufreq_update_freq_work(struct work_struct *work)
+{
+	struct cpufreq_policy *policy =
+		container_of(work, struct cpufreq_policy, req_work);
+	struct cpufreq_policy new_policy = *policy;
+
+	/* We should read constraint values from QoS layer */
+	new_policy.min = 0;
+	new_policy.max = UINT_MAX;
+
+	down_write(&policy->rwsem);
+
+	if (!policy_is_inactive(policy))
+		cpufreq_set_policy(policy, &new_policy);
+
+	up_write(&policy->rwsem);
+}
+
+static int cpufreq_update_freq(struct cpufreq_policy *policy)
+{
+	schedule_work(&policy->req_work);
+	return 0;
+}
+
+static int cpufreq_notifier_min(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);
+
+	return cpufreq_update_freq(policy);
+}
+
+static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy)
+{
+	struct kobject *kobj;
+	struct completion *cmp;
+
+	down_write(&policy->rwsem);
+	cpufreq_stats_free_table(policy);
+	kobj = &policy->kobj;
+	cmp = &policy->kobj_unregister;
+	up_write(&policy->rwsem);
+	kobject_put(kobj);
+
+	/*
+	 * We need to make sure that the underlying kobj is
+	 * actually not referenced anymore by anybody before we
+	 * proceed with unloading.
+	 */
+	pr_debug("waiting for dropping of refcount\n");
+	wait_for_completion(cmp);
+	pr_debug("wait complete\n");
+}
+
 static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 {
 	struct cpufreq_policy *policy;
+	struct device *dev = get_cpu_device(cpu);
 	int ret;
 
+	if (!dev)
+		return NULL;
+
 	policy = kzalloc(sizeof(*policy), GFP_KERNEL);
 	if (!policy)
 		return NULL;
@@ -1147,7 +1214,7 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 	ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
 				   cpufreq_global_kobject, "policy%u", cpu);
 	if (ret) {
-		pr_err("%s: failed to init policy->kobj: %d\n", __func__, ret);
+		dev_err(dev, "%s: failed to init policy->kobj: %d\n", __func__, ret);
 		/*
 		 * The entire policy object will be freed below, but the extra
 		 * memory allocated for the kobject name needs to be freed by
@@ -1157,16 +1224,41 @@ 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;
+
+	ret = dev_pm_qos_add_notifier(dev, &policy->nb_min,
+				      DEV_PM_QOS_MIN_FREQUENCY);
+	if (ret) {
+		dev_err(dev, "Failed to register MIN QoS notifier: %d (%*pbl)\n",
+			ret, cpumask_pr_args(policy->cpus));
+		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;
+	}
+
 	INIT_LIST_HEAD(&policy->policy_list);
 	init_rwsem(&policy->rwsem);
 	spin_lock_init(&policy->transition_lock);
 	init_waitqueue_head(&policy->transition_wait);
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update);
+	INIT_WORK(&policy->req_work, cpufreq_update_freq_work);
 
 	policy->cpu = cpu;
 	return policy;
 
+err_min_qos_notifier:
+	dev_pm_qos_remove_notifier(dev, &policy->nb_min,
+				   DEV_PM_QOS_MIN_FREQUENCY);
+err_kobj_remove:
+	cpufreq_policy_put_kobj(policy);
 err_free_real_cpus:
 	free_cpumask_var(policy->real_cpus);
 err_free_rcpumask:
@@ -1179,30 +1271,9 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 	return NULL;
 }
 
-static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy)
-{
-	struct kobject *kobj;
-	struct completion *cmp;
-
-	down_write(&policy->rwsem);
-	cpufreq_stats_free_table(policy);
-	kobj = &policy->kobj;
-	cmp = &policy->kobj_unregister;
-	up_write(&policy->rwsem);
-	kobject_put(kobj);
-
-	/*
-	 * We need to make sure that the underlying kobj is
-	 * actually not referenced anymore by anybody before we
-	 * proceed with unloading.
-	 */
-	pr_debug("waiting for dropping of refcount\n");
-	wait_for_completion(cmp);
-	pr_debug("wait complete\n");
-}
-
 static void cpufreq_policy_free(struct cpufreq_policy *policy)
 {
+	struct device *dev = get_cpu_device(policy->cpu);
 	unsigned long flags;
 	int cpu;
 
@@ -1214,6 +1285,10 @@ 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_MIN_FREQUENCY);
 	cpufreq_policy_put_kobj(policy);
 	free_cpumask_var(policy->real_cpus);
 	free_cpumask_var(policy->related_cpus);
@@ -2290,6 +2365,8 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
 		       struct cpufreq_policy *new_policy)
 {
 	struct cpufreq_governor *old_gov;
+	struct device *cpu_dev = get_cpu_device(policy->cpu);
+	unsigned long min, max;
 	int ret;
 
 	pr_debug("setting new policy for CPU %u: %u - %u kHz\n",
@@ -2304,11 +2381,27 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
 	if (new_policy->min > new_policy->max)
 		return -EINVAL;
 
+	/*
+	 * PM QoS framework collects all the requests from users and provide us
+	 * the final aggregated value here.
+	 */
+	min = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MIN_FREQUENCY);
+	max = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MAX_FREQUENCY);
+
+	if (min > new_policy->min)
+		new_policy->min = min;
+	if (max < new_policy->max)
+		new_policy->max = max;
+
 	/* verify the cpu speed can be set within this limit */
 	ret = cpufreq_driver->verify(new_policy);
 	if (ret)
 		return ret;
 
+	/*
+	 * The notifier-chain shall be removed once all the users of
+	 * CPUFREQ_ADJUST are moved to use the QoS framework.
+	 */
 	/* adjust if necessary - all reasons */
 	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
 			CPUFREQ_ADJUST, new_policy);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d01a74fbc4db..0fe7678da9c2 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -83,6 +83,7 @@ struct cpufreq_policy {
 
 	struct work_struct	update; /* if update_policy() needs to be
 					 * called, but you're in IRQ context */
+	struct work_struct	req_work;
 
 	struct cpufreq_user_policy user_policy;
 	struct cpufreq_frequency_table	*freq_table;
@@ -147,6 +148,9 @@ 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 cpufreq_freqs {
-- 
2.21.0.rc0.269.g1a574e7a288b


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH V3 5/5] cpufreq: Add QoS requests for userspace constraints
  2019-06-10 10:51 [PATCH V3 0/5] cpufreq: Use QoS layer to manage freq-constraints Viresh Kumar
                   ` (3 preceding siblings ...)
  2019-06-10 10:51 ` [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework Viresh Kumar
@ 2019-06-10 10:51 ` Viresh Kumar
  2019-06-14 17:14   ` Matthias Kaehlcke
  2019-06-17  9:23   ` Ulf Hansson
  4 siblings, 2 replies; 27+ messages in thread
From: Viresh Kumar @ 2019-06-10 10:51 UTC (permalink / raw)
  To: Rafael Wysocki
  Cc: Viresh Kumar, linux-pm, Vincent Guittot, Qais.Yousef, mka,
	juri.lelli, linux-kernel

This implements QoS requests to manage userspace configuration of min
and max frequency.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 92 +++++++++++++++++++--------------------
 include/linux/cpufreq.h   |  8 +---
 2 files changed, 47 insertions(+), 53 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 547d221b2ff2..ff754981fcb4 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -720,23 +720,15 @@ static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf)
 static ssize_t store_##file_name					\
 (struct cpufreq_policy *policy, const char *buf, size_t count)		\
 {									\
-	int ret, temp;							\
-	struct cpufreq_policy new_policy;				\
+	unsigned long val;						\
+	int ret;							\
 									\
-	memcpy(&new_policy, policy, sizeof(*policy));			\
-	new_policy.min = policy->user_policy.min;			\
-	new_policy.max = policy->user_policy.max;			\
-									\
-	ret = sscanf(buf, "%u", &new_policy.object);			\
+	ret = sscanf(buf, "%lu", &val);					\
 	if (ret != 1)							\
 		return -EINVAL;						\
 									\
-	temp = new_policy.object;					\
-	ret = cpufreq_set_policy(policy, &new_policy);		\
-	if (!ret)							\
-		policy->user_policy.object = temp;			\
-									\
-	return ret ? ret : count;					\
+	ret = dev_pm_qos_update_request(policy->object##_freq_req, val);\
+	return ret && ret != 1 ? ret : count;				\
 }
 
 store_one(scaling_min_freq, min);
@@ -1133,10 +1125,6 @@ static void cpufreq_update_freq_work(struct work_struct *work)
 		container_of(work, struct cpufreq_policy, req_work);
 	struct cpufreq_policy new_policy = *policy;
 
-	/* We should read constraint values from QoS layer */
-	new_policy.min = 0;
-	new_policy.max = UINT_MAX;
-
 	down_write(&policy->rwsem);
 
 	if (!policy_is_inactive(policy))
@@ -1243,6 +1231,12 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 		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;
+
+	policy->max_freq_req = policy->min_freq_req + 1;
 	INIT_LIST_HEAD(&policy->policy_list);
 	init_rwsem(&policy->rwsem);
 	spin_lock_init(&policy->transition_lock);
@@ -1254,6 +1248,9 @@ 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_MIN_FREQUENCY);
@@ -1289,6 +1286,10 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
 				   DEV_PM_QOS_MAX_FREQUENCY);
 	dev_pm_qos_remove_notifier(dev, &policy->nb_min,
 				   DEV_PM_QOS_MIN_FREQUENCY);
+	dev_pm_qos_remove_request(policy->max_freq_req);
+	dev_pm_qos_remove_request(policy->min_freq_req);
+	kfree(policy->min_freq_req);
+
 	cpufreq_policy_put_kobj(policy);
 	free_cpumask_var(policy->real_cpus);
 	free_cpumask_var(policy->related_cpus);
@@ -1366,16 +1367,30 @@ static int cpufreq_online(unsigned int cpu)
 	cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
 
 	if (new_policy) {
-		policy->user_policy.min = policy->min;
-		policy->user_policy.max = policy->max;
+		struct device *dev = get_cpu_device(cpu);
 
 		for_each_cpu(j, policy->related_cpus) {
 			per_cpu(cpufreq_cpu_data, j) = policy;
 			add_cpu_dev_symlink(policy, j);
 		}
-	} else {
-		policy->min = policy->user_policy.min;
-		policy->max = policy->user_policy.max;
+
+		ret = dev_pm_qos_add_request(dev, policy->min_freq_req,
+					     DEV_PM_QOS_MIN_FREQUENCY,
+					     policy->min);
+		if (ret < 0) {
+			dev_err(dev, "Failed to add min-freq constraint (%d)\n",
+				ret);
+			goto out_destroy_policy;
+		}
+
+		ret = dev_pm_qos_add_request(dev, policy->max_freq_req,
+					     DEV_PM_QOS_MAX_FREQUENCY,
+					     policy->max);
+		if (ret < 0) {
+			dev_err(dev, "Failed to add max-freq constraint (%d)\n",
+				ret);
+			goto out_destroy_policy;
+		}
 	}
 
 	if (cpufreq_driver->get && !cpufreq_driver->setpolicy) {
@@ -2366,7 +2381,6 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
 {
 	struct cpufreq_governor *old_gov;
 	struct device *cpu_dev = get_cpu_device(policy->cpu);
-	unsigned long min, max;
 	int ret;
 
 	pr_debug("setting new policy for CPU %u: %u - %u kHz\n",
@@ -2374,24 +2388,12 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
 
 	memcpy(&new_policy->cpuinfo, &policy->cpuinfo, sizeof(policy->cpuinfo));
 
-	/*
-	* This check works well when we store new min/max freq attributes,
-	* because new_policy is a copy of policy with one field updated.
-	*/
-	if (new_policy->min > new_policy->max)
-		return -EINVAL;
-
 	/*
 	 * PM QoS framework collects all the requests from users and provide us
 	 * the final aggregated value here.
 	 */
-	min = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MIN_FREQUENCY);
-	max = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MAX_FREQUENCY);
-
-	if (min > new_policy->min)
-		new_policy->min = min;
-	if (max < new_policy->max)
-		new_policy->max = max;
+	new_policy->min = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MIN_FREQUENCY);
+	new_policy->max = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MAX_FREQUENCY);
 
 	/* verify the cpu speed can be set within this limit */
 	ret = cpufreq_driver->verify(new_policy);
@@ -2480,10 +2482,9 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
  * @cpu: CPU to re-evaluate the policy for.
  *
  * Update the current frequency for the cpufreq policy of @cpu and use
- * cpufreq_set_policy() to re-apply the min and max limits saved in the
- * user_policy sub-structure of that policy, which triggers the evaluation
- * of policy notifiers and the cpufreq driver's ->verify() callback for the
- * policy in question, among other things.
+ * cpufreq_set_policy() to re-apply the min and max limits, which triggers the
+ * evaluation of policy notifiers and the cpufreq driver's ->verify() callback
+ * for the policy in question, among other things.
  */
 void cpufreq_update_policy(unsigned int cpu)
 {
@@ -2503,8 +2504,6 @@ void cpufreq_update_policy(unsigned int cpu)
 
 	pr_debug("updating policy for CPU %u\n", cpu);
 	memcpy(&new_policy, policy, sizeof(*policy));
-	new_policy.min = policy->user_policy.min;
-	new_policy.max = policy->user_policy.max;
 
 	cpufreq_set_policy(policy, &new_policy);
 
@@ -2549,10 +2548,9 @@ static int cpufreq_boost_set_sw(int state)
 			break;
 		}
 
-		down_write(&policy->rwsem);
-		policy->user_policy.max = policy->max;
-		cpufreq_governor_limits(policy);
-		up_write(&policy->rwsem);
+		ret = dev_pm_qos_update_request(policy->max_freq_req, policy->max);
+		if (ret)
+			break;
 	}
 
 	return ret;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 0fe7678da9c2..6bbed9af4fd2 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -50,11 +50,6 @@ struct cpufreq_cpuinfo {
 	unsigned int		transition_latency;
 };
 
-struct cpufreq_user_policy {
-	unsigned int		min;    /* in kHz */
-	unsigned int		max;    /* in kHz */
-};
-
 struct cpufreq_policy {
 	/* CPUs sharing clock, require sw coordination */
 	cpumask_var_t		cpus;	/* Online CPUs only */
@@ -85,7 +80,8 @@ struct cpufreq_policy {
 					 * called, but you're in IRQ context */
 	struct work_struct	req_work;
 
-	struct cpufreq_user_policy user_policy;
+	struct dev_pm_qos_request *min_freq_req;
+	struct dev_pm_qos_request *max_freq_req;
 	struct cpufreq_frequency_table	*freq_table;
 	enum cpufreq_table_sorting freq_table_sorted;
 
-- 
2.21.0.rc0.269.g1a574e7a288b


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 1/5] PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier()
  2019-06-10 10:51 ` [PATCH V3 1/5] PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier() Viresh Kumar
@ 2019-06-11 23:45   ` Matthias Kaehlcke
  2019-06-17  9:23   ` Ulf Hansson
  2019-06-17 22:52   ` Rafael J. Wysocki
  2 siblings, 0 replies; 27+ messages in thread
From: Matthias Kaehlcke @ 2019-06-11 23:45 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael Wysocki, Len Brown, Pavel Machek, Kevin Hilman,
	Ulf Hansson, linux-pm, Vincent Guittot, Qais.Yousef, juri.lelli,
	linux-kernel

On Mon, Jun 10, 2019 at 04:21:32PM +0530, Viresh Kumar wrote:
> In order to use the same set of routines to register notifiers for
> different request types, update the existing
> dev_pm_qos_{add|remove}_notifier() routines with an additional
> parameter: request-type.
> 
> For now, it only supports resume-latency request type.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  Documentation/power/pm_qos_interface.txt | 10 ++++++----
>  drivers/base/power/domain.c              |  8 +++++---
>  drivers/base/power/qos.c                 | 14 ++++++++++++--
>  include/linux/pm_qos.h                   | 12 ++++++++----
>  4 files changed, 31 insertions(+), 13 deletions(-)

My QoS background is nil, but this looks reasonable to me:

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 2/5] PM / QOS: Pass request type to dev_pm_qos_read_value()
  2019-06-10 10:51 ` [PATCH V3 2/5] PM / QOS: Pass request type to dev_pm_qos_read_value() Viresh Kumar
@ 2019-06-12  0:08   ` Matthias Kaehlcke
  2019-06-17  9:23   ` Ulf Hansson
  2019-06-17 23:14   ` Rafael J. Wysocki
  2 siblings, 0 replies; 27+ messages in thread
From: Matthias Kaehlcke @ 2019-06-12  0:08 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael Wysocki, Len Brown, Pavel Machek, Kevin Hilman,
	Ulf Hansson, Daniel Lezcano, linux-pm, Vincent Guittot,
	Qais.Yousef, juri.lelli, linux-kernel

On Mon, Jun 10, 2019 at 04:21:33PM +0530, Viresh Kumar wrote:
> In order to use dev_pm_qos_read_value(), and other internal routines to
> it, to read values for different QoS requests, pass request type as a
> parameter to these routines.

nit: I find that somewhat hard to parse, a possible improvement:

"In order to allow dev_pm_qos_read_value() and other {related,similar}
 internal routines to read values for different QoS requests, pass
 request type as a parameter to these routines."

(I'm not a native English speaker, so I don't claim this to be
correct ;-)

> For now, it only supports resume-latency request type.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  Documentation/power/pm_qos_interface.txt |  2 +-
>  drivers/base/power/domain_governor.c     |  4 +--
>  drivers/base/power/qos.c                 | 10 +++---
>  drivers/base/power/runtime.c             |  2 +-
>  drivers/cpuidle/governor.c               |  2 +-
>  include/linux/pm_qos.h                   | 41 +++++++++++++++++-------
>  6 files changed, 40 insertions(+), 21 deletions(-)

looks good to me, however I'm just a QoS n00b:

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 3/5] PM / QoS: Add support for MIN/MAX frequency constraints
  2019-06-10 10:51 ` [PATCH V3 3/5] PM / QoS: Add support for MIN/MAX frequency constraints Viresh Kumar
@ 2019-06-13  0:04   ` Matthias Kaehlcke
  2019-06-17  9:23   ` Ulf Hansson
  1 sibling, 0 replies; 27+ messages in thread
From: Matthias Kaehlcke @ 2019-06-13  0:04 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael Wysocki, Pavel Machek, Len Brown, linux-pm,
	Vincent Guittot, Qais.Yousef, juri.lelli, linux-kernel

On Mon, Jun 10, 2019 at 04:21:34PM +0530, Viresh Kumar wrote:
> This patch introduces the min-frequency and max-frequency device
> constraints, which will be used by the cpufreq core to begin with.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Again, I'm mostly ignorant about QoS, in the context of the
existing code (particularly looking at DEV_PM_QOS_RESUME_LATENCY)
this looks reasonable to me. FWIW:

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework
  2019-06-10 10:51 ` [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework Viresh Kumar
@ 2019-06-14 16:46   ` Matthias Kaehlcke
  2019-06-17  3:02     ` Viresh Kumar
  2019-06-17  9:23   ` Ulf Hansson
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 27+ messages in thread
From: Matthias Kaehlcke @ 2019-06-14 16:46 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael Wysocki, linux-pm, Vincent Guittot, Qais.Yousef,
	juri.lelli, linux-kernel

Hi Viresh,

On Mon, Jun 10, 2019 at 04:21:35PM +0530, Viresh Kumar wrote:
> This registers the notifiers for min/max frequency constraints with the
> PM QoS framework. The constraints are also taken into consideration in
> cpufreq_set_policy().
> 
> This also relocates cpufreq_policy_put_kobj() as it is required to be
> called from cpufreq_policy_alloc() now.
> 
> No constraints are added until now though.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 139 +++++++++++++++++++++++++++++++-------
>  include/linux/cpufreq.h   |   4 ++
>  2 files changed, 120 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 85ff958e01f1..547d221b2ff2 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -26,6 +26,7 @@
>  #include <linux/kernel_stat.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/pm_qos.h>
>  #include <linux/slab.h>
>  #include <linux/suspend.h>
>  #include <linux/syscore_ops.h>
> @@ -1126,11 +1127,77 @@ static void handle_update(struct work_struct *work)
>  	cpufreq_update_policy(cpu);
>  }
>  
> +static void cpufreq_update_freq_work(struct work_struct *work)
> +{
> +	struct cpufreq_policy *policy =
> +		container_of(work, struct cpufreq_policy, req_work);
> +	struct cpufreq_policy new_policy = *policy;
> +
> +	/* We should read constraint values from QoS layer */
> +	new_policy.min = 0;
> +	new_policy.max = UINT_MAX;
> +
> +	down_write(&policy->rwsem);
> +
> +	if (!policy_is_inactive(policy))
> +		cpufreq_set_policy(policy, &new_policy);
> +
> +	up_write(&policy->rwsem);
> +}
> +
> +static int cpufreq_update_freq(struct cpufreq_policy *policy)
> +{
> +	schedule_work(&policy->req_work);

I think you need to add a cancel_work_sync() in cpufreq_policy_free()
to make sure the work doesn't run after the policy has been freed.

Otherwise it looks good to me.

Cheers

Matthias

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 5/5] cpufreq: Add QoS requests for userspace constraints
  2019-06-10 10:51 ` [PATCH V3 5/5] cpufreq: Add QoS requests for userspace constraints Viresh Kumar
@ 2019-06-14 17:14   ` Matthias Kaehlcke
  2019-06-17  3:07     ` Viresh Kumar
  2019-06-17  9:23   ` Ulf Hansson
  1 sibling, 1 reply; 27+ messages in thread
From: Matthias Kaehlcke @ 2019-06-14 17:14 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael Wysocki, linux-pm, Vincent Guittot, Qais.Yousef,
	juri.lelli, linux-kernel

Hi Viresh,

On Mon, Jun 10, 2019 at 04:21:36PM +0530, Viresh Kumar wrote:
> This implements QoS requests to manage userspace configuration of min
> and max frequency.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 92 +++++++++++++++++++--------------------
>  include/linux/cpufreq.h   |  8 +---
>  2 files changed, 47 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 547d221b2ff2..ff754981fcb4 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -720,23 +720,15 @@ static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf)
>  static ssize_t store_##file_name					\
>  (struct cpufreq_policy *policy, const char *buf, size_t count)		\
>  {									\
> -	int ret, temp;							\
> -	struct cpufreq_policy new_policy;				\
> +	unsigned long val;						\
> +	int ret;							\
>  									\
> -	memcpy(&new_policy, policy, sizeof(*policy));			\
> -	new_policy.min = policy->user_policy.min;			\
> -	new_policy.max = policy->user_policy.max;			\
> -									\
> -	ret = sscanf(buf, "%u", &new_policy.object);			\
> +	ret = sscanf(buf, "%lu", &val);					\
>  	if (ret != 1)							\
>  		return -EINVAL;						\
>  									\
> -	temp = new_policy.object;					\
> -	ret = cpufreq_set_policy(policy, &new_policy);		\
> -	if (!ret)							\
> -		policy->user_policy.object = temp;			\
> -									\
> -	return ret ? ret : count;					\
> +	ret = dev_pm_qos_update_request(policy->object##_freq_req, val);\
> +	return ret && ret != 1 ? ret : count;				\

nit: I wonder if

  return (ret >= 0) ? count : ret;

would be clearer.

Other than that:

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework
  2019-06-14 16:46   ` Matthias Kaehlcke
@ 2019-06-17  3:02     ` Viresh Kumar
  0 siblings, 0 replies; 27+ messages in thread
From: Viresh Kumar @ 2019-06-17  3:02 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Rafael Wysocki, linux-pm, Vincent Guittot, Qais.Yousef,
	juri.lelli, linux-kernel

On 14-06-19, 09:46, Matthias Kaehlcke wrote:
> Hi Viresh,
> 
> On Mon, Jun 10, 2019 at 04:21:35PM +0530, Viresh Kumar wrote:
> > This registers the notifiers for min/max frequency constraints with the
> > PM QoS framework. The constraints are also taken into consideration in
> > cpufreq_set_policy().
> > 
> > This also relocates cpufreq_policy_put_kobj() as it is required to be
> > called from cpufreq_policy_alloc() now.
> > 
> > No constraints are added until now though.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/cpufreq/cpufreq.c | 139 +++++++++++++++++++++++++++++++-------
> >  include/linux/cpufreq.h   |   4 ++
> >  2 files changed, 120 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 85ff958e01f1..547d221b2ff2 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/kernel_stat.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> > +#include <linux/pm_qos.h>
> >  #include <linux/slab.h>
> >  #include <linux/suspend.h>
> >  #include <linux/syscore_ops.h>
> > @@ -1126,11 +1127,77 @@ static void handle_update(struct work_struct *work)
> >  	cpufreq_update_policy(cpu);
> >  }
> >  
> > +static void cpufreq_update_freq_work(struct work_struct *work)
> > +{
> > +	struct cpufreq_policy *policy =
> > +		container_of(work, struct cpufreq_policy, req_work);
> > +	struct cpufreq_policy new_policy = *policy;
> > +
> > +	/* We should read constraint values from QoS layer */
> > +	new_policy.min = 0;
> > +	new_policy.max = UINT_MAX;
> > +
> > +	down_write(&policy->rwsem);
> > +
> > +	if (!policy_is_inactive(policy))
> > +		cpufreq_set_policy(policy, &new_policy);
> > +
> > +	up_write(&policy->rwsem);
> > +}
> > +
> > +static int cpufreq_update_freq(struct cpufreq_policy *policy)
> > +{
> > +	schedule_work(&policy->req_work);
> 
> I think you need to add a cancel_work_sync() in cpufreq_policy_free()
> to make sure the work doesn't run after the policy has been freed.

Right, added this to the commit.

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 547d221b2ff2..878add2cac3c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1289,6 +1289,8 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
                                   DEV_PM_QOS_MAX_FREQUENCY);
        dev_pm_qos_remove_notifier(dev, &policy->nb_min,
                                   DEV_PM_QOS_MIN_FREQUENCY);
+       cancel_work_sync(&policy->req_work);
+
        cpufreq_policy_put_kobj(policy);
        free_cpumask_var(policy->real_cpus);
        free_cpumask_var(policy->related_cpus);

-- 
viresh

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 5/5] cpufreq: Add QoS requests for userspace constraints
  2019-06-14 17:14   ` Matthias Kaehlcke
@ 2019-06-17  3:07     ` Viresh Kumar
  0 siblings, 0 replies; 27+ messages in thread
From: Viresh Kumar @ 2019-06-17  3:07 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: Rafael Wysocki, linux-pm, Vincent Guittot, Qais.Yousef,
	juri.lelli, linux-kernel

On 14-06-19, 10:14, Matthias Kaehlcke wrote:
> Hi Viresh,
> 
> On Mon, Jun 10, 2019 at 04:21:36PM +0530, Viresh Kumar wrote:
> > This implements QoS requests to manage userspace configuration of min
> > and max frequency.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/cpufreq/cpufreq.c | 92 +++++++++++++++++++--------------------
> >  include/linux/cpufreq.h   |  8 +---
> >  2 files changed, 47 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 547d221b2ff2..ff754981fcb4 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -720,23 +720,15 @@ static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf)
> >  static ssize_t store_##file_name					\
> >  (struct cpufreq_policy *policy, const char *buf, size_t count)		\
> >  {									\
> > -	int ret, temp;							\
> > -	struct cpufreq_policy new_policy;				\
> > +	unsigned long val;						\
> > +	int ret;							\
> >  									\
> > -	memcpy(&new_policy, policy, sizeof(*policy));			\
> > -	new_policy.min = policy->user_policy.min;			\
> > -	new_policy.max = policy->user_policy.max;			\
> > -									\
> > -	ret = sscanf(buf, "%u", &new_policy.object);			\
> > +	ret = sscanf(buf, "%lu", &val);					\
> >  	if (ret != 1)							\
> >  		return -EINVAL;						\
> >  									\
> > -	temp = new_policy.object;					\
> > -	ret = cpufreq_set_policy(policy, &new_policy);		\
> > -	if (!ret)							\
> > -		policy->user_policy.object = temp;			\
> > -									\
> > -	return ret ? ret : count;					\
> > +	ret = dev_pm_qos_update_request(policy->object##_freq_req, val);\
> > +	return ret && ret != 1 ? ret : count;				\
> 
> nit: I wonder if
> 
>   return (ret >= 0) ? count : ret;
> 
> would be clearer.

Done. Thanks.

> Other than that:
> 
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

-- 
viresh

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 1/5] PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier()
  2019-06-10 10:51 ` [PATCH V3 1/5] PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier() Viresh Kumar
  2019-06-11 23:45   ` Matthias Kaehlcke
@ 2019-06-17  9:23   ` Ulf Hansson
  2019-06-17 22:52   ` Rafael J. Wysocki
  2 siblings, 0 replies; 27+ messages in thread
From: Ulf Hansson @ 2019-06-17  9:23 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael Wysocki, Len Brown, Pavel Machek, Kevin Hilman, Linux PM,
	Vincent Guittot, Qais.Yousef, Matthias Kaehlcke, juri.lelli,
	Linux Kernel Mailing List

On Mon, 10 Jun 2019 at 12:51, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> In order to use the same set of routines to register notifiers for
> different request types, update the existing
> dev_pm_qos_{add|remove}_notifier() routines with an additional
> parameter: request-type.
>
> For now, it only supports resume-latency request type.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe


> ---
>  Documentation/power/pm_qos_interface.txt | 10 ++++++----
>  drivers/base/power/domain.c              |  8 +++++---
>  drivers/base/power/qos.c                 | 14 ++++++++++++--
>  include/linux/pm_qos.h                   | 12 ++++++++----
>  4 files changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt
> index 19c5f7b1a7ba..ec7d662d1707 100644
> --- a/Documentation/power/pm_qos_interface.txt
> +++ b/Documentation/power/pm_qos_interface.txt
> @@ -164,12 +164,14 @@ directory.
>  Notification mechanisms:
>  The per-device PM QoS framework has a per-device notification tree.
>
> -int dev_pm_qos_add_notifier(device, notifier):
> -Adds a notification callback function for the device.
> +int dev_pm_qos_add_notifier(device, notifier, type):
> +Adds a notification callback function for the device for a particular request
> +type.
> +
>  The callback is called when the aggregated value of the device constraints list
> -is changed (for resume latency device PM QoS only).
> +is changed.
>
> -int dev_pm_qos_remove_notifier(device, notifier):
> +int dev_pm_qos_remove_notifier(device, notifier, type):
>  Removes the notification callback function for the device.
>
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 33c30c1e6a30..b063bc41b0a9 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1536,7 +1536,8 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>         if (ret)
>                 genpd_free_dev_data(dev, gpd_data);
>         else
> -               dev_pm_qos_add_notifier(dev, &gpd_data->nb);
> +               dev_pm_qos_add_notifier(dev, &gpd_data->nb,
> +                                       DEV_PM_QOS_RESUME_LATENCY);
>
>         return ret;
>  }
> @@ -1569,7 +1570,8 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
>
>         pdd = dev->power.subsys_data->domain_data;
>         gpd_data = to_gpd_data(pdd);
> -       dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
> +       dev_pm_qos_remove_notifier(dev, &gpd_data->nb,
> +                                  DEV_PM_QOS_RESUME_LATENCY);
>
>         genpd_lock(genpd);
>
> @@ -1597,7 +1599,7 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
>
>   out:
>         genpd_unlock(genpd);
> -       dev_pm_qos_add_notifier(dev, &gpd_data->nb);
> +       dev_pm_qos_add_notifier(dev, &gpd_data->nb, DEV_PM_QOS_RESUME_LATENCY);
>
>         return ret;
>  }
> diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
> index 6c91f8df1d59..cfd463212513 100644
> --- a/drivers/base/power/qos.c
> +++ b/drivers/base/power/qos.c
> @@ -467,6 +467,7 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request);
>   *
>   * @dev: target device for the constraint
>   * @notifier: notifier block managed by caller.
> + * @type: request type.
>   *
>   * Will register the notifier into a notification chain that gets called
>   * upon changes to the target value for the device.
> @@ -474,10 +475,14 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_remove_request);
>   * If the device's constraints object doesn't exist when this routine is called,
>   * it will be created (or error code will be returned if that fails).
>   */
> -int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier)
> +int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier,
> +                           enum dev_pm_qos_req_type type)
>  {
>         int ret = 0;
>
> +       if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
> +               return -EINVAL;
> +
>         mutex_lock(&dev_pm_qos_mtx);
>
>         if (IS_ERR(dev->power.qos))
> @@ -500,15 +505,20 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_add_notifier);
>   *
>   * @dev: target device for the constraint
>   * @notifier: notifier block to be removed.
> + * @type: request type.
>   *
>   * Will remove the notifier from the notification chain that gets called
>   * upon changes to the target value.
>   */
>  int dev_pm_qos_remove_notifier(struct device *dev,
> -                              struct notifier_block *notifier)
> +                              struct notifier_block *notifier,
> +                              enum dev_pm_qos_req_type type)
>  {
>         int retval = 0;
>
> +       if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
> +               return -EINVAL;
> +
>         mutex_lock(&dev_pm_qos_mtx);
>
>         /* Silently return if the constraints object is not present. */
> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> index 6ea1ae373d77..1f4d456e8fff 100644
> --- a/include/linux/pm_qos.h
> +++ b/include/linux/pm_qos.h
> @@ -146,9 +146,11 @@ int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
>  int dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value);
>  int dev_pm_qos_remove_request(struct dev_pm_qos_request *req);
>  int dev_pm_qos_add_notifier(struct device *dev,
> -                           struct notifier_block *notifier);
> +                           struct notifier_block *notifier,
> +                           enum dev_pm_qos_req_type type);
>  int dev_pm_qos_remove_notifier(struct device *dev,
> -                              struct notifier_block *notifier);
> +                              struct notifier_block *notifier,
> +                              enum dev_pm_qos_req_type type);
>  void dev_pm_qos_constraints_init(struct device *dev);
>  void dev_pm_qos_constraints_destroy(struct device *dev);
>  int dev_pm_qos_add_ancestor_request(struct device *dev,
> @@ -202,10 +204,12 @@ static inline int dev_pm_qos_update_request(struct dev_pm_qos_request *req,
>  static inline int dev_pm_qos_remove_request(struct dev_pm_qos_request *req)
>                         { return 0; }
>  static inline int dev_pm_qos_add_notifier(struct device *dev,
> -                                         struct notifier_block *notifier)
> +                                         struct notifier_block *notifier,
> +                                         enum dev_pm_qos_req_type type);
>                         { return 0; }
>  static inline int dev_pm_qos_remove_notifier(struct device *dev,
> -                                            struct notifier_block *notifier)
> +                                            struct notifier_block *notifier,
> +                                            enum dev_pm_qos_req_type type)
>                         { return 0; }
>  static inline void dev_pm_qos_constraints_init(struct device *dev)
>  {
> --
> 2.21.0.rc0.269.g1a574e7a288b
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 2/5] PM / QOS: Pass request type to dev_pm_qos_read_value()
  2019-06-10 10:51 ` [PATCH V3 2/5] PM / QOS: Pass request type to dev_pm_qos_read_value() Viresh Kumar
  2019-06-12  0:08   ` Matthias Kaehlcke
@ 2019-06-17  9:23   ` Ulf Hansson
  2019-06-17 23:14   ` Rafael J. Wysocki
  2 siblings, 0 replies; 27+ messages in thread
From: Ulf Hansson @ 2019-06-17  9:23 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael Wysocki, Len Brown, Pavel Machek, Kevin Hilman,
	Daniel Lezcano, Linux PM, Vincent Guittot, Qais.Yousef,
	Matthias Kaehlcke, juri.lelli, Linux Kernel Mailing List

On Mon, 10 Jun 2019 at 12:51, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> In order to use dev_pm_qos_read_value(), and other internal routines to
> it, to read values for different QoS requests, pass request type as a
> parameter to these routines.
>
> For now, it only supports resume-latency request type.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe


> ---
>  Documentation/power/pm_qos_interface.txt |  2 +-
>  drivers/base/power/domain_governor.c     |  4 +--
>  drivers/base/power/qos.c                 | 10 +++---
>  drivers/base/power/runtime.c             |  2 +-
>  drivers/cpuidle/governor.c               |  2 +-
>  include/linux/pm_qos.h                   | 41 +++++++++++++++++-------
>  6 files changed, 40 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/power/pm_qos_interface.txt b/Documentation/power/pm_qos_interface.txt
> index ec7d662d1707..cfcb1df39799 100644
> --- a/Documentation/power/pm_qos_interface.txt
> +++ b/Documentation/power/pm_qos_interface.txt
> @@ -123,7 +123,7 @@ Will remove the element.  After removal it will update the aggregate target and
>  call the notification trees if the target was changed as a result of removing
>  the request.
>
> -s32 dev_pm_qos_read_value(device):
> +s32 dev_pm_qos_read_value(device, type):
>  Returns the aggregated value for a given device's constraints list.
>
>  enum pm_qos_flags_status dev_pm_qos_flags(device, mask)
> diff --git a/drivers/base/power/domain_governor.c b/drivers/base/power/domain_governor.c
> index 3838045c9277..f66ac46d07d0 100644
> --- a/drivers/base/power/domain_governor.c
> +++ b/drivers/base/power/domain_governor.c
> @@ -33,7 +33,7 @@ static int dev_update_qos_constraint(struct device *dev, void *data)
>                  * take its current PM QoS constraint (that's the only thing
>                  * known at this point anyway).
>                  */
> -               constraint_ns = dev_pm_qos_read_value(dev);
> +               constraint_ns = dev_pm_qos_read_value(dev, DEV_PM_QOS_RESUME_LATENCY);
>                 constraint_ns *= NSEC_PER_USEC;
>         }
>
> @@ -66,7 +66,7 @@ static bool default_suspend_ok(struct device *dev)
>         td->constraint_changed = false;
>         td->cached_suspend_ok = false;
>         td->effective_constraint_ns = 0;
> -       constraint_ns = __dev_pm_qos_read_value(dev);
> +       constraint_ns = __dev_pm_qos_read_value(dev, DEV_PM_QOS_RESUME_LATENCY);
>
>         spin_unlock_irqrestore(&dev->power.lock, flags);
>
> diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
> index cfd463212513..7bb88174371f 100644
> --- a/drivers/base/power/qos.c
> +++ b/drivers/base/power/qos.c
> @@ -92,27 +92,29 @@ EXPORT_SYMBOL_GPL(dev_pm_qos_flags);
>  /**
>   * __dev_pm_qos_read_value - Get PM QoS constraint for a given device.
>   * @dev: Device to get the PM QoS constraint value for.
> + * @type: QoS request type.
>   *
>   * This routine must be called with dev->power.lock held.
>   */
> -s32 __dev_pm_qos_read_value(struct device *dev)
> +s32 __dev_pm_qos_read_value(struct device *dev, enum dev_pm_qos_req_type type)
>  {
>         lockdep_assert_held(&dev->power.lock);
>
> -       return dev_pm_qos_raw_read_value(dev);
> +       return dev_pm_qos_raw_read_value(dev, type);
>  }
>
>  /**
>   * dev_pm_qos_read_value - Get PM QoS constraint for a given device (locked).
>   * @dev: Device to get the PM QoS constraint value for.
> + * @type: QoS request type.
>   */
> -s32 dev_pm_qos_read_value(struct device *dev)
> +s32 dev_pm_qos_read_value(struct device *dev, enum dev_pm_qos_req_type type)
>  {
>         unsigned long flags;
>         s32 ret;
>
>         spin_lock_irqsave(&dev->power.lock, flags);
> -       ret = __dev_pm_qos_read_value(dev);
> +       ret = __dev_pm_qos_read_value(dev, type);
>         spin_unlock_irqrestore(&dev->power.lock, flags);
>
>         return ret;
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 952a1e7057c7..92ffc2a0151d 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -275,7 +275,7 @@ static int rpm_check_suspend_allowed(struct device *dev)
>             || (dev->power.request_pending
>                         && dev->power.request == RPM_REQ_RESUME))
>                 retval = -EAGAIN;
> -       else if (__dev_pm_qos_read_value(dev) == 0)
> +       else if (__dev_pm_qos_read_value(dev, DEV_PM_QOS_RESUME_LATENCY) == 0)
>                 retval = -EPERM;
>         else if (dev->power.runtime_status == RPM_SUSPENDED)
>                 retval = 1;
> diff --git a/drivers/cpuidle/governor.c b/drivers/cpuidle/governor.c
> index 9fddf828a76f..fb5659eb2009 100644
> --- a/drivers/cpuidle/governor.c
> +++ b/drivers/cpuidle/governor.c
> @@ -110,7 +110,7 @@ int cpuidle_governor_latency_req(unsigned int cpu)
>  {
>         int global_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
>         struct device *device = get_cpu_device(cpu);
> -       int device_req = dev_pm_qos_raw_read_value(device);
> +       int device_req = dev_pm_qos_raw_read_value(device, DEV_PM_QOS_RESUME_LATENCY);
>
>         return device_req < global_req ? device_req : global_req;
>  }
> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> index 1f4d456e8fff..55814d48c39c 100644
> --- a/include/linux/pm_qos.h
> +++ b/include/linux/pm_qos.h
> @@ -139,8 +139,8 @@ s32 pm_qos_read_value(struct pm_qos_constraints *c);
>  #ifdef CONFIG_PM
>  enum pm_qos_flags_status __dev_pm_qos_flags(struct device *dev, s32 mask);
>  enum pm_qos_flags_status dev_pm_qos_flags(struct device *dev, s32 mask);
> -s32 __dev_pm_qos_read_value(struct device *dev);
> -s32 dev_pm_qos_read_value(struct device *dev);
> +s32 __dev_pm_qos_read_value(struct device *dev, enum dev_pm_qos_req_type type);
> +s32 dev_pm_qos_read_value(struct device *dev, enum dev_pm_qos_req_type type);
>  int dev_pm_qos_add_request(struct device *dev, struct dev_pm_qos_request *req,
>                            enum dev_pm_qos_req_type type, s32 value);
>  int dev_pm_qos_update_request(struct dev_pm_qos_request *req, s32 new_value);
> @@ -176,11 +176,19 @@ static inline s32 dev_pm_qos_requested_flags(struct device *dev)
>         return dev->power.qos->flags_req->data.flr.flags;
>  }
>
> -static inline s32 dev_pm_qos_raw_read_value(struct device *dev)
> +static inline s32 dev_pm_qos_raw_read_value(struct device *dev,
> +                                           enum dev_pm_qos_req_type type)
>  {
> -       return IS_ERR_OR_NULL(dev->power.qos) ?
> -               PM_QOS_RESUME_LATENCY_NO_CONSTRAINT :
> -               pm_qos_read_value(&dev->power.qos->resume_latency);
> +       struct dev_pm_qos *qos = dev->power.qos;
> +
> +       switch (type) {
> +       case DEV_PM_QOS_RESUME_LATENCY:
> +               return IS_ERR_OR_NULL(qos) ? PM_QOS_RESUME_LATENCY_NO_CONSTRAINT
> +                       : pm_qos_read_value(&qos->resume_latency);
> +       default:
> +               WARN_ON(1);
> +               return 0;
> +       }
>  }
>  #else
>  static inline enum pm_qos_flags_status __dev_pm_qos_flags(struct device *dev,
> @@ -189,10 +197,6 @@ static inline enum pm_qos_flags_status __dev_pm_qos_flags(struct device *dev,
>  static inline enum pm_qos_flags_status dev_pm_qos_flags(struct device *dev,
>                                                         s32 mask)
>                         { return PM_QOS_FLAGS_UNDEFINED; }
> -static inline s32 __dev_pm_qos_read_value(struct device *dev)
> -                       { return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT; }
> -static inline s32 dev_pm_qos_read_value(struct device *dev)
> -                       { return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT; }
>  static inline int dev_pm_qos_add_request(struct device *dev,
>                                          struct dev_pm_qos_request *req,
>                                          enum dev_pm_qos_req_type type,
> @@ -245,10 +249,23 @@ static inline s32 dev_pm_qos_requested_resume_latency(struct device *dev)
>         return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
>  }
>  static inline s32 dev_pm_qos_requested_flags(struct device *dev) { return 0; }
> -static inline s32 dev_pm_qos_raw_read_value(struct device *dev)
> +
> +static inline s32 dev_pm_qos_raw_read_value(struct device *dev,
> +                                           enum dev_pm_qos_req_type type)
>  {
> -       return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
> +       switch type {
> +       case DEV_PM_QOS_RESUME_LATENCY:
> +               return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
> +       default:
> +               WARN_ON(1);
> +               return 0;
> +       }
>  }
> +
> +static inline s32 __dev_pm_qos_read_value(struct device *dev, enum dev_pm_qos_req_type type)
> +                       { return dev_pm_qos_raw_read_value(dev, type); }
> +static inline s32 dev_pm_qos_read_value(struct device *dev, enum dev_pm_qos_req_type type)
> +                       { return dev_pm_qos_raw_read_value(dev, type); }
>  #endif
>
>  #endif
> --
> 2.21.0.rc0.269.g1a574e7a288b
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 3/5] PM / QoS: Add support for MIN/MAX frequency constraints
  2019-06-10 10:51 ` [PATCH V3 3/5] PM / QoS: Add support for MIN/MAX frequency constraints Viresh Kumar
  2019-06-13  0:04   ` Matthias Kaehlcke
@ 2019-06-17  9:23   ` Ulf Hansson
  1 sibling, 0 replies; 27+ messages in thread
From: Ulf Hansson @ 2019-06-17  9:23 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael Wysocki, Pavel Machek, Len Brown, Linux PM,
	Vincent Guittot, Qais.Yousef, Matthias Kaehlcke, juri.lelli,
	Linux Kernel Mailing List

On Mon, 10 Jun 2019 at 12:52, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> This patch introduces the min-frequency and max-frequency device
> constraints, which will be used by the cpufreq core to begin with.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe


> ---
>  drivers/base/power/qos.c | 103 +++++++++++++++++++++++++++++++++------
>  include/linux/pm_qos.h   |  18 +++++++
>  2 files changed, 107 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
> index 7bb88174371f..e977aef437a5 100644
> --- a/drivers/base/power/qos.c
> +++ b/drivers/base/power/qos.c
> @@ -151,6 +151,14 @@ static int apply_constraint(struct dev_pm_qos_request *req,
>                         req->dev->power.set_latency_tolerance(req->dev, value);
>                 }
>                 break;
> +       case DEV_PM_QOS_MIN_FREQUENCY:
> +               ret = pm_qos_update_target(&qos->min_frequency,
> +                                          &req->data.pnode, action, value);
> +               break;
> +       case DEV_PM_QOS_MAX_FREQUENCY:
> +               ret = pm_qos_update_target(&qos->max_frequency,
> +                                          &req->data.pnode, action, value);
> +               break;
>         case DEV_PM_QOS_FLAGS:
>                 ret = pm_qos_update_flags(&qos->flags, &req->data.flr,
>                                           action, value);
> @@ -179,12 +187,11 @@ static int dev_pm_qos_constraints_allocate(struct device *dev)
>         if (!qos)
>                 return -ENOMEM;
>
> -       n = kzalloc(sizeof(*n), GFP_KERNEL);
> +       n = kzalloc(3 * sizeof(*n), GFP_KERNEL);
>         if (!n) {
>                 kfree(qos);
>                 return -ENOMEM;
>         }
> -       BLOCKING_INIT_NOTIFIER_HEAD(n);
>
>         c = &qos->resume_latency;
>         plist_head_init(&c->list);
> @@ -193,6 +200,7 @@ static int dev_pm_qos_constraints_allocate(struct device *dev)
>         c->no_constraint_value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
>         c->type = PM_QOS_MIN;
>         c->notifiers = n;
> +       BLOCKING_INIT_NOTIFIER_HEAD(n);
>
>         c = &qos->latency_tolerance;
>         plist_head_init(&c->list);
> @@ -201,6 +209,24 @@ 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;
>
> +       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 = &qos->max_frequency;
> +       plist_head_init(&c->list);
> +       c->target_value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
> +       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);
> +
>         INIT_LIST_HEAD(&qos->flags.list);
>
>         spin_lock_irq(&dev->power.lock);
> @@ -254,11 +280,25 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
>                 apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
>                 memset(req, 0, sizeof(*req));
>         }
> +
>         c = &qos->latency_tolerance;
>         plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) {
>                 apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
>                 memset(req, 0, sizeof(*req));
>         }
> +
> +       c = &qos->min_frequency;
> +       plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) {
> +               apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE);
> +               memset(req, 0, sizeof(*req));
> +       }
> +
> +       c = &qos->max_frequency;
> +       plist_for_each_entry_safe(req, tmp, &c->list, data.pnode) {
> +               apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
> +               memset(req, 0, sizeof(*req));
> +       }
> +
>         f = &qos->flags;
>         list_for_each_entry_safe(req, tmp, &f->list, data.flr.node) {
>                 apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
> @@ -370,6 +410,8 @@ static int __dev_pm_qos_update_request(struct dev_pm_qos_request *req,
>         switch(req->type) {
>         case DEV_PM_QOS_RESUME_LATENCY:
>         case DEV_PM_QOS_LATENCY_TOLERANCE:
> +       case DEV_PM_QOS_MIN_FREQUENCY:
> +       case DEV_PM_QOS_MAX_FREQUENCY:
>                 curr_value = req->data.pnode.prio;
>                 break;
>         case DEV_PM_QOS_FLAGS:
> @@ -482,9 +524,6 @@ int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier,
>  {
>         int ret = 0;
>
> -       if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
> -               return -EINVAL;
> -
>         mutex_lock(&dev_pm_qos_mtx);
>
>         if (IS_ERR(dev->power.qos))
> @@ -492,10 +531,28 @@ int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier,
>         else if (!dev->power.qos)
>                 ret = dev_pm_qos_constraints_allocate(dev);
>
> -       if (!ret)
> +       if (ret)
> +               goto unlock;
> +
> +       switch (type) {
> +       case DEV_PM_QOS_RESUME_LATENCY:
>                 ret = blocking_notifier_chain_register(dev->power.qos->resume_latency.notifiers,
>                                                        notifier);
> +               break;
> +       case DEV_PM_QOS_MIN_FREQUENCY:
> +               ret = blocking_notifier_chain_register(dev->power.qos->min_frequency.notifiers,
> +                                                      notifier);
> +               break;
> +       case DEV_PM_QOS_MAX_FREQUENCY:
> +               ret = blocking_notifier_chain_register(dev->power.qos->max_frequency.notifiers,
> +                                                      notifier);
> +               break;
> +       default:
> +               WARN_ON(1);
> +               ret = -EINVAL;
> +       }
>
> +unlock:
>         mutex_unlock(&dev_pm_qos_mtx);
>         return ret;
>  }
> @@ -516,20 +573,35 @@ int dev_pm_qos_remove_notifier(struct device *dev,
>                                struct notifier_block *notifier,
>                                enum dev_pm_qos_req_type type)
>  {
> -       int retval = 0;
> -
> -       if (WARN_ON(type != DEV_PM_QOS_RESUME_LATENCY))
> -               return -EINVAL;
> +       int ret = 0;
>
>         mutex_lock(&dev_pm_qos_mtx);
>
>         /* Silently return if the constraints object is not present. */
> -       if (!IS_ERR_OR_NULL(dev->power.qos))
> -               retval = blocking_notifier_chain_unregister(dev->power.qos->resume_latency.notifiers,
> -                                                           notifier);
> +       if (IS_ERR_OR_NULL(dev->power.qos))
> +               goto unlock;
>
> +       switch (type) {
> +       case DEV_PM_QOS_RESUME_LATENCY:
> +               ret = blocking_notifier_chain_unregister(dev->power.qos->resume_latency.notifiers,
> +                                                        notifier);
> +               break;
> +       case DEV_PM_QOS_MIN_FREQUENCY:
> +               ret = blocking_notifier_chain_unregister(dev->power.qos->min_frequency.notifiers,
> +                                                        notifier);
> +               break;
> +       case DEV_PM_QOS_MAX_FREQUENCY:
> +               ret = blocking_notifier_chain_unregister(dev->power.qos->max_frequency.notifiers,
> +                                                        notifier);
> +               break;
> +       default:
> +               WARN_ON(1);
> +               ret = -EINVAL;
> +       }
> +
> +unlock:
>         mutex_unlock(&dev_pm_qos_mtx);
> -       return retval;
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_qos_remove_notifier);
>
> @@ -589,6 +661,9 @@ static void __dev_pm_qos_drop_user_request(struct device *dev,
>                 req = dev->power.qos->flags_req;
>                 dev->power.qos->flags_req = NULL;
>                 break;
> +       default:
> +               WARN_ON(1);
> +               return;
>         }
>         __dev_pm_qos_remove_request(req);
>         kfree(req);
> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> index 55814d48c39c..3f994283a5ae 100644
> --- a/include/linux/pm_qos.h
> +++ b/include/linux/pm_qos.h
> @@ -40,6 +40,8 @@ enum pm_qos_flags_status {
>  #define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT    PM_QOS_LATENCY_ANY
>  #define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS PM_QOS_LATENCY_ANY_NS
>  #define PM_QOS_LATENCY_TOLERANCE_DEFAULT_VALUE 0
> +#define PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE     0
> +#define PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE     (-1)
>  #define PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT (-1)
>
>  #define PM_QOS_FLAG_NO_POWER_OFF       (1 << 0)
> @@ -58,6 +60,8 @@ struct pm_qos_flags_request {
>  enum dev_pm_qos_req_type {
>         DEV_PM_QOS_RESUME_LATENCY = 1,
>         DEV_PM_QOS_LATENCY_TOLERANCE,
> +       DEV_PM_QOS_MIN_FREQUENCY,
> +       DEV_PM_QOS_MAX_FREQUENCY,
>         DEV_PM_QOS_FLAGS,
>  };
>
> @@ -99,10 +103,14 @@ struct pm_qos_flags {
>  struct dev_pm_qos {
>         struct pm_qos_constraints resume_latency;
>         struct pm_qos_constraints latency_tolerance;
> +       struct pm_qos_constraints min_frequency;
> +       struct pm_qos_constraints max_frequency;
>         struct pm_qos_flags flags;
>         struct dev_pm_qos_request *resume_latency_req;
>         struct dev_pm_qos_request *latency_tolerance_req;
>         struct dev_pm_qos_request *flags_req;
> +       struct dev_pm_qos_request *min_frequency_req;
> +       struct dev_pm_qos_request *max_frequency_req;
>  };
>
>  /* Action requested to pm_qos_update_target */
> @@ -185,6 +193,12 @@ static inline s32 dev_pm_qos_raw_read_value(struct device *dev,
>         case DEV_PM_QOS_RESUME_LATENCY:
>                 return IS_ERR_OR_NULL(qos) ? PM_QOS_RESUME_LATENCY_NO_CONSTRAINT
>                         : pm_qos_read_value(&qos->resume_latency);
> +       case DEV_PM_QOS_MIN_FREQUENCY:
> +               return IS_ERR_OR_NULL(qos) ? PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE
> +                       : pm_qos_read_value(&qos->min_frequency);
> +       case DEV_PM_QOS_MAX_FREQUENCY:
> +               return IS_ERR_OR_NULL(qos) ? PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE
> +                       : pm_qos_read_value(&qos->max_frequency);
>         default:
>                 WARN_ON(1);
>                 return 0;
> @@ -256,6 +270,10 @@ static inline s32 dev_pm_qos_raw_read_value(struct device *dev,
>         switch type {
>         case DEV_PM_QOS_RESUME_LATENCY:
>                 return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
> +       case DEV_PM_QOS_MIN_FREQUENCY:
> +               return PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE;
> +       case DEV_PM_QOS_MAX_FREQUENCY:
> +               return PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
>         default:
>                 WARN_ON(1);
>                 return 0;
> --
> 2.21.0.rc0.269.g1a574e7a288b
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework
  2019-06-10 10:51 ` [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework Viresh Kumar
  2019-06-14 16:46   ` Matthias Kaehlcke
@ 2019-06-17  9:23   ` Ulf Hansson
  2019-06-17 23:26   ` Rafael J. Wysocki
  2019-06-17 23:37   ` Rafael J. Wysocki
  3 siblings, 0 replies; 27+ messages in thread
From: Ulf Hansson @ 2019-06-17  9:23 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael Wysocki, Linux PM, Vincent Guittot, Qais.Yousef,
	Matthias Kaehlcke, juri.lelli, Linux Kernel Mailing List

On Mon, 10 Jun 2019 at 12:52, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> This registers the notifiers for min/max frequency constraints with the
> PM QoS framework. The constraints are also taken into consideration in
> cpufreq_set_policy().
>
> This also relocates cpufreq_policy_put_kobj() as it is required to be
> called from cpufreq_policy_alloc() now.
>
> No constraints are added until now though.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe


> ---
>  drivers/cpufreq/cpufreq.c | 139 +++++++++++++++++++++++++++++++-------
>  include/linux/cpufreq.h   |   4 ++
>  2 files changed, 120 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 85ff958e01f1..547d221b2ff2 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -26,6 +26,7 @@
>  #include <linux/kernel_stat.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/pm_qos.h>
>  #include <linux/slab.h>
>  #include <linux/suspend.h>
>  #include <linux/syscore_ops.h>
> @@ -1126,11 +1127,77 @@ static void handle_update(struct work_struct *work)
>         cpufreq_update_policy(cpu);
>  }
>
> +static void cpufreq_update_freq_work(struct work_struct *work)
> +{
> +       struct cpufreq_policy *policy =
> +               container_of(work, struct cpufreq_policy, req_work);
> +       struct cpufreq_policy new_policy = *policy;
> +
> +       /* We should read constraint values from QoS layer */
> +       new_policy.min = 0;
> +       new_policy.max = UINT_MAX;
> +
> +       down_write(&policy->rwsem);
> +
> +       if (!policy_is_inactive(policy))
> +               cpufreq_set_policy(policy, &new_policy);
> +
> +       up_write(&policy->rwsem);
> +}
> +
> +static int cpufreq_update_freq(struct cpufreq_policy *policy)
> +{
> +       schedule_work(&policy->req_work);
> +       return 0;
> +}
> +
> +static int cpufreq_notifier_min(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);
> +
> +       return cpufreq_update_freq(policy);
> +}
> +
> +static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy)
> +{
> +       struct kobject *kobj;
> +       struct completion *cmp;
> +
> +       down_write(&policy->rwsem);
> +       cpufreq_stats_free_table(policy);
> +       kobj = &policy->kobj;
> +       cmp = &policy->kobj_unregister;
> +       up_write(&policy->rwsem);
> +       kobject_put(kobj);
> +
> +       /*
> +        * We need to make sure that the underlying kobj is
> +        * actually not referenced anymore by anybody before we
> +        * proceed with unloading.
> +        */
> +       pr_debug("waiting for dropping of refcount\n");
> +       wait_for_completion(cmp);
> +       pr_debug("wait complete\n");
> +}
> +
>  static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>  {
>         struct cpufreq_policy *policy;
> +       struct device *dev = get_cpu_device(cpu);
>         int ret;
>
> +       if (!dev)
> +               return NULL;
> +
>         policy = kzalloc(sizeof(*policy), GFP_KERNEL);
>         if (!policy)
>                 return NULL;
> @@ -1147,7 +1214,7 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>         ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
>                                    cpufreq_global_kobject, "policy%u", cpu);
>         if (ret) {
> -               pr_err("%s: failed to init policy->kobj: %d\n", __func__, ret);
> +               dev_err(dev, "%s: failed to init policy->kobj: %d\n", __func__, ret);
>                 /*
>                  * The entire policy object will be freed below, but the extra
>                  * memory allocated for the kobject name needs to be freed by
> @@ -1157,16 +1224,41 @@ 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;
> +
> +       ret = dev_pm_qos_add_notifier(dev, &policy->nb_min,
> +                                     DEV_PM_QOS_MIN_FREQUENCY);
> +       if (ret) {
> +               dev_err(dev, "Failed to register MIN QoS notifier: %d (%*pbl)\n",
> +                       ret, cpumask_pr_args(policy->cpus));
> +               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;
> +       }
> +
>         INIT_LIST_HEAD(&policy->policy_list);
>         init_rwsem(&policy->rwsem);
>         spin_lock_init(&policy->transition_lock);
>         init_waitqueue_head(&policy->transition_wait);
>         init_completion(&policy->kobj_unregister);
>         INIT_WORK(&policy->update, handle_update);
> +       INIT_WORK(&policy->req_work, cpufreq_update_freq_work);
>
>         policy->cpu = cpu;
>         return policy;
>
> +err_min_qos_notifier:
> +       dev_pm_qos_remove_notifier(dev, &policy->nb_min,
> +                                  DEV_PM_QOS_MIN_FREQUENCY);
> +err_kobj_remove:
> +       cpufreq_policy_put_kobj(policy);
>  err_free_real_cpus:
>         free_cpumask_var(policy->real_cpus);
>  err_free_rcpumask:
> @@ -1179,30 +1271,9 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>         return NULL;
>  }
>
> -static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy)
> -{
> -       struct kobject *kobj;
> -       struct completion *cmp;
> -
> -       down_write(&policy->rwsem);
> -       cpufreq_stats_free_table(policy);
> -       kobj = &policy->kobj;
> -       cmp = &policy->kobj_unregister;
> -       up_write(&policy->rwsem);
> -       kobject_put(kobj);
> -
> -       /*
> -        * We need to make sure that the underlying kobj is
> -        * actually not referenced anymore by anybody before we
> -        * proceed with unloading.
> -        */
> -       pr_debug("waiting for dropping of refcount\n");
> -       wait_for_completion(cmp);
> -       pr_debug("wait complete\n");
> -}
> -
>  static void cpufreq_policy_free(struct cpufreq_policy *policy)
>  {
> +       struct device *dev = get_cpu_device(policy->cpu);
>         unsigned long flags;
>         int cpu;
>
> @@ -1214,6 +1285,10 @@ 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_MIN_FREQUENCY);
>         cpufreq_policy_put_kobj(policy);
>         free_cpumask_var(policy->real_cpus);
>         free_cpumask_var(policy->related_cpus);
> @@ -2290,6 +2365,8 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
>                        struct cpufreq_policy *new_policy)
>  {
>         struct cpufreq_governor *old_gov;
> +       struct device *cpu_dev = get_cpu_device(policy->cpu);
> +       unsigned long min, max;
>         int ret;
>
>         pr_debug("setting new policy for CPU %u: %u - %u kHz\n",
> @@ -2304,11 +2381,27 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
>         if (new_policy->min > new_policy->max)
>                 return -EINVAL;
>
> +       /*
> +        * PM QoS framework collects all the requests from users and provide us
> +        * the final aggregated value here.
> +        */
> +       min = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MIN_FREQUENCY);
> +       max = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MAX_FREQUENCY);
> +
> +       if (min > new_policy->min)
> +               new_policy->min = min;
> +       if (max < new_policy->max)
> +               new_policy->max = max;
> +
>         /* verify the cpu speed can be set within this limit */
>         ret = cpufreq_driver->verify(new_policy);
>         if (ret)
>                 return ret;
>
> +       /*
> +        * The notifier-chain shall be removed once all the users of
> +        * CPUFREQ_ADJUST are moved to use the QoS framework.
> +        */
>         /* adjust if necessary - all reasons */
>         blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
>                         CPUFREQ_ADJUST, new_policy);
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index d01a74fbc4db..0fe7678da9c2 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -83,6 +83,7 @@ struct cpufreq_policy {
>
>         struct work_struct      update; /* if update_policy() needs to be
>                                          * called, but you're in IRQ context */
> +       struct work_struct      req_work;
>
>         struct cpufreq_user_policy user_policy;
>         struct cpufreq_frequency_table  *freq_table;
> @@ -147,6 +148,9 @@ 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 cpufreq_freqs {
> --
> 2.21.0.rc0.269.g1a574e7a288b
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 5/5] cpufreq: Add QoS requests for userspace constraints
  2019-06-10 10:51 ` [PATCH V3 5/5] cpufreq: Add QoS requests for userspace constraints Viresh Kumar
  2019-06-14 17:14   ` Matthias Kaehlcke
@ 2019-06-17  9:23   ` Ulf Hansson
  1 sibling, 0 replies; 27+ messages in thread
From: Ulf Hansson @ 2019-06-17  9:23 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael Wysocki, Linux PM, Vincent Guittot, Qais.Yousef,
	Matthias Kaehlcke, juri.lelli, Linux Kernel Mailing List

On Mon, 10 Jun 2019 at 12:52, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> This implements QoS requests to manage userspace configuration of min
> and max frequency.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe


> ---
>  drivers/cpufreq/cpufreq.c | 92 +++++++++++++++++++--------------------
>  include/linux/cpufreq.h   |  8 +---
>  2 files changed, 47 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 547d221b2ff2..ff754981fcb4 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -720,23 +720,15 @@ static ssize_t show_scaling_cur_freq(struct cpufreq_policy *policy, char *buf)
>  static ssize_t store_##file_name                                       \
>  (struct cpufreq_policy *policy, const char *buf, size_t count)         \
>  {                                                                      \
> -       int ret, temp;                                                  \
> -       struct cpufreq_policy new_policy;                               \
> +       unsigned long val;                                              \
> +       int ret;                                                        \
>                                                                         \
> -       memcpy(&new_policy, policy, sizeof(*policy));                   \
> -       new_policy.min = policy->user_policy.min;                       \
> -       new_policy.max = policy->user_policy.max;                       \
> -                                                                       \
> -       ret = sscanf(buf, "%u", &new_policy.object);                    \
> +       ret = sscanf(buf, "%lu", &val);                                 \
>         if (ret != 1)                                                   \
>                 return -EINVAL;                                         \
>                                                                         \
> -       temp = new_policy.object;                                       \
> -       ret = cpufreq_set_policy(policy, &new_policy);          \
> -       if (!ret)                                                       \
> -               policy->user_policy.object = temp;                      \
> -                                                                       \
> -       return ret ? ret : count;                                       \
> +       ret = dev_pm_qos_update_request(policy->object##_freq_req, val);\
> +       return ret && ret != 1 ? ret : count;                           \
>  }
>
>  store_one(scaling_min_freq, min);
> @@ -1133,10 +1125,6 @@ static void cpufreq_update_freq_work(struct work_struct *work)
>                 container_of(work, struct cpufreq_policy, req_work);
>         struct cpufreq_policy new_policy = *policy;
>
> -       /* We should read constraint values from QoS layer */
> -       new_policy.min = 0;
> -       new_policy.max = UINT_MAX;
> -
>         down_write(&policy->rwsem);
>
>         if (!policy_is_inactive(policy))
> @@ -1243,6 +1231,12 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>                 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;
> +
> +       policy->max_freq_req = policy->min_freq_req + 1;
>         INIT_LIST_HEAD(&policy->policy_list);
>         init_rwsem(&policy->rwsem);
>         spin_lock_init(&policy->transition_lock);
> @@ -1254,6 +1248,9 @@ 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_MIN_FREQUENCY);
> @@ -1289,6 +1286,10 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
>                                    DEV_PM_QOS_MAX_FREQUENCY);
>         dev_pm_qos_remove_notifier(dev, &policy->nb_min,
>                                    DEV_PM_QOS_MIN_FREQUENCY);
> +       dev_pm_qos_remove_request(policy->max_freq_req);
> +       dev_pm_qos_remove_request(policy->min_freq_req);
> +       kfree(policy->min_freq_req);
> +
>         cpufreq_policy_put_kobj(policy);
>         free_cpumask_var(policy->real_cpus);
>         free_cpumask_var(policy->related_cpus);
> @@ -1366,16 +1367,30 @@ static int cpufreq_online(unsigned int cpu)
>         cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
>
>         if (new_policy) {
> -               policy->user_policy.min = policy->min;
> -               policy->user_policy.max = policy->max;
> +               struct device *dev = get_cpu_device(cpu);
>
>                 for_each_cpu(j, policy->related_cpus) {
>                         per_cpu(cpufreq_cpu_data, j) = policy;
>                         add_cpu_dev_symlink(policy, j);
>                 }
> -       } else {
> -               policy->min = policy->user_policy.min;
> -               policy->max = policy->user_policy.max;
> +
> +               ret = dev_pm_qos_add_request(dev, policy->min_freq_req,
> +                                            DEV_PM_QOS_MIN_FREQUENCY,
> +                                            policy->min);
> +               if (ret < 0) {
> +                       dev_err(dev, "Failed to add min-freq constraint (%d)\n",
> +                               ret);
> +                       goto out_destroy_policy;
> +               }
> +
> +               ret = dev_pm_qos_add_request(dev, policy->max_freq_req,
> +                                            DEV_PM_QOS_MAX_FREQUENCY,
> +                                            policy->max);
> +               if (ret < 0) {
> +                       dev_err(dev, "Failed to add max-freq constraint (%d)\n",
> +                               ret);
> +                       goto out_destroy_policy;
> +               }
>         }
>
>         if (cpufreq_driver->get && !cpufreq_driver->setpolicy) {
> @@ -2366,7 +2381,6 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
>  {
>         struct cpufreq_governor *old_gov;
>         struct device *cpu_dev = get_cpu_device(policy->cpu);
> -       unsigned long min, max;
>         int ret;
>
>         pr_debug("setting new policy for CPU %u: %u - %u kHz\n",
> @@ -2374,24 +2388,12 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
>
>         memcpy(&new_policy->cpuinfo, &policy->cpuinfo, sizeof(policy->cpuinfo));
>
> -       /*
> -       * This check works well when we store new min/max freq attributes,
> -       * because new_policy is a copy of policy with one field updated.
> -       */
> -       if (new_policy->min > new_policy->max)
> -               return -EINVAL;
> -
>         /*
>          * PM QoS framework collects all the requests from users and provide us
>          * the final aggregated value here.
>          */
> -       min = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MIN_FREQUENCY);
> -       max = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MAX_FREQUENCY);
> -
> -       if (min > new_policy->min)
> -               new_policy->min = min;
> -       if (max < new_policy->max)
> -               new_policy->max = max;
> +       new_policy->min = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MIN_FREQUENCY);
> +       new_policy->max = dev_pm_qos_read_value(cpu_dev, DEV_PM_QOS_MAX_FREQUENCY);
>
>         /* verify the cpu speed can be set within this limit */
>         ret = cpufreq_driver->verify(new_policy);
> @@ -2480,10 +2482,9 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
>   * @cpu: CPU to re-evaluate the policy for.
>   *
>   * Update the current frequency for the cpufreq policy of @cpu and use
> - * cpufreq_set_policy() to re-apply the min and max limits saved in the
> - * user_policy sub-structure of that policy, which triggers the evaluation
> - * of policy notifiers and the cpufreq driver's ->verify() callback for the
> - * policy in question, among other things.
> + * cpufreq_set_policy() to re-apply the min and max limits, which triggers the
> + * evaluation of policy notifiers and the cpufreq driver's ->verify() callback
> + * for the policy in question, among other things.
>   */
>  void cpufreq_update_policy(unsigned int cpu)
>  {
> @@ -2503,8 +2504,6 @@ void cpufreq_update_policy(unsigned int cpu)
>
>         pr_debug("updating policy for CPU %u\n", cpu);
>         memcpy(&new_policy, policy, sizeof(*policy));
> -       new_policy.min = policy->user_policy.min;
> -       new_policy.max = policy->user_policy.max;
>
>         cpufreq_set_policy(policy, &new_policy);
>
> @@ -2549,10 +2548,9 @@ static int cpufreq_boost_set_sw(int state)
>                         break;
>                 }
>
> -               down_write(&policy->rwsem);
> -               policy->user_policy.max = policy->max;
> -               cpufreq_governor_limits(policy);
> -               up_write(&policy->rwsem);
> +               ret = dev_pm_qos_update_request(policy->max_freq_req, policy->max);
> +               if (ret)
> +                       break;
>         }
>
>         return ret;
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 0fe7678da9c2..6bbed9af4fd2 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -50,11 +50,6 @@ struct cpufreq_cpuinfo {
>         unsigned int            transition_latency;
>  };
>
> -struct cpufreq_user_policy {
> -       unsigned int            min;    /* in kHz */
> -       unsigned int            max;    /* in kHz */
> -};
> -
>  struct cpufreq_policy {
>         /* CPUs sharing clock, require sw coordination */
>         cpumask_var_t           cpus;   /* Online CPUs only */
> @@ -85,7 +80,8 @@ struct cpufreq_policy {
>                                          * called, but you're in IRQ context */
>         struct work_struct      req_work;
>
> -       struct cpufreq_user_policy user_policy;
> +       struct dev_pm_qos_request *min_freq_req;
> +       struct dev_pm_qos_request *max_freq_req;
>         struct cpufreq_frequency_table  *freq_table;
>         enum cpufreq_table_sorting freq_table_sorted;
>
> --
> 2.21.0.rc0.269.g1a574e7a288b
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 1/5] PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier()
  2019-06-10 10:51 ` [PATCH V3 1/5] PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier() Viresh Kumar
  2019-06-11 23:45   ` Matthias Kaehlcke
  2019-06-17  9:23   ` Ulf Hansson
@ 2019-06-17 22:52   ` Rafael J. Wysocki
  2 siblings, 0 replies; 27+ messages in thread
From: Rafael J. Wysocki @ 2019-06-17 22:52 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Len Brown, Pavel Machek, Kevin Hilman, Ulf Hansson, linux-pm,
	Vincent Guittot, Qais.Yousef, mka, juri.lelli, linux-kernel

On Monday, June 10, 2019 12:51:32 PM CEST Viresh Kumar wrote:
> In order to use the same set of routines to register notifiers for
> different request types, update the existing
> dev_pm_qos_{add|remove}_notifier() routines with an additional
> parameter: request-type.
> 
> For now, it only supports resume-latency request type.

It would be good to mention the broader rationale of this change in its changelog
(that is, the frequency limits use case).




^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 2/5] PM / QOS: Pass request type to dev_pm_qos_read_value()
  2019-06-10 10:51 ` [PATCH V3 2/5] PM / QOS: Pass request type to dev_pm_qos_read_value() Viresh Kumar
  2019-06-12  0:08   ` Matthias Kaehlcke
  2019-06-17  9:23   ` Ulf Hansson
@ 2019-06-17 23:14   ` Rafael J. Wysocki
  2 siblings, 0 replies; 27+ messages in thread
From: Rafael J. Wysocki @ 2019-06-17 23:14 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Len Brown, Pavel Machek, Kevin Hilman, Ulf Hansson,
	Daniel Lezcano, linux-pm, Vincent Guittot, Qais.Yousef, mka,
	juri.lelli, linux-kernel

On Monday, June 10, 2019 12:51:33 PM CEST Viresh Kumar wrote:
> In order to use dev_pm_qos_read_value(), and other internal routines to
> it, to read values for different QoS requests, pass request type as a
> parameter to these routines.
> 
> For now, it only supports resume-latency request type.

I don't quite like the structure by which the type arg is passed through the
entire call chain until the switch in dep_pm_qos_raw_read_value().

There is only one direct user of dev_pm_qos_raw_read_value() AFAICS which
is cpuidle. It shouldn't need to suffer the general case overhead, so I would
rename that function to dev_pm_qos_raw_resume_latency() and update cpuidle
accordingly.

Moreover, the callers of __dev_pm_qos_read_value() are interested in the
resume latency value too, so it might make sense to rename this as
__dev_pm_qos_resume_latency(), update its callers and put the switch
into dev_pm_qos_read_value().

Plus the changelog should explain the broader rationale of this change like
for the first patch IMO.




^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework
  2019-06-10 10:51 ` [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework Viresh Kumar
  2019-06-14 16:46   ` Matthias Kaehlcke
  2019-06-17  9:23   ` Ulf Hansson
@ 2019-06-17 23:26   ` Rafael J. Wysocki
  2019-06-18 11:25     ` Viresh Kumar
  2019-06-17 23:37   ` Rafael J. Wysocki
  3 siblings, 1 reply; 27+ messages in thread
From: Rafael J. Wysocki @ 2019-06-17 23:26 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linux-pm, Vincent Guittot, Qais.Yousef, mka, juri.lelli, linux-kernel

On Monday, June 10, 2019 12:51:35 PM CEST Viresh Kumar wrote:
> This registers the notifiers for min/max frequency constraints with the
> PM QoS framework. The constraints are also taken into consideration in
> cpufreq_set_policy().
> 
> This also relocates cpufreq_policy_put_kobj() as it is required to be
> called from cpufreq_policy_alloc() now.
> 
> No constraints are added until now though.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 139 +++++++++++++++++++++++++++++++-------
>  include/linux/cpufreq.h   |   4 ++
>  2 files changed, 120 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 85ff958e01f1..547d221b2ff2 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -26,6 +26,7 @@
>  #include <linux/kernel_stat.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/pm_qos.h>
>  #include <linux/slab.h>
>  #include <linux/suspend.h>
>  #include <linux/syscore_ops.h>
> @@ -1126,11 +1127,77 @@ static void handle_update(struct work_struct *work)
>  	cpufreq_update_policy(cpu);
>  }
>  
> +static void cpufreq_update_freq_work(struct work_struct *work)
> +{
> +	struct cpufreq_policy *policy =
> +		container_of(work, struct cpufreq_policy, req_work);
> +	struct cpufreq_policy new_policy = *policy;
> +
> +	/* We should read constraint values from QoS layer */
> +	new_policy.min = 0;
> +	new_policy.max = UINT_MAX;
> +
> +	down_write(&policy->rwsem);
> +
> +	if (!policy_is_inactive(policy))
> +		cpufreq_set_policy(policy, &new_policy);
> +
> +	up_write(&policy->rwsem);
> +}
> +
> +static int cpufreq_update_freq(struct cpufreq_policy *policy)
> +{
> +	schedule_work(&policy->req_work);
> +	return 0;
> +}
> +
> +static int cpufreq_notifier_min(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);
> +
> +	return cpufreq_update_freq(policy);
> +}

This is a bit convoluted.

Two different notifiers are registered basically for the same thing.

Any chance to use just one?




^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework
  2019-06-10 10:51 ` [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework Viresh Kumar
                     ` (2 preceding siblings ...)
  2019-06-17 23:26   ` Rafael J. Wysocki
@ 2019-06-17 23:37   ` Rafael J. Wysocki
  2019-06-18 11:34     ` Viresh Kumar
  3 siblings, 1 reply; 27+ messages in thread
From: Rafael J. Wysocki @ 2019-06-17 23:37 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linux-pm, Vincent Guittot, Qais.Yousef, mka, juri.lelli, linux-kernel

On Monday, June 10, 2019 12:51:35 PM CEST Viresh Kumar wrote:
> This registers the notifiers for min/max frequency constraints with the
> PM QoS framework. The constraints are also taken into consideration in
> cpufreq_set_policy().
> 
> This also relocates cpufreq_policy_put_kobj() as it is required to be
> called from cpufreq_policy_alloc() now.
> 
> No constraints are added until now though.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 139 +++++++++++++++++++++++++++++++-------
>  include/linux/cpufreq.h   |   4 ++
>  2 files changed, 120 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 85ff958e01f1..547d221b2ff2 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -26,6 +26,7 @@
>  #include <linux/kernel_stat.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/pm_qos.h>
>  #include <linux/slab.h>
>  #include <linux/suspend.h>
>  #include <linux/syscore_ops.h>
> @@ -1126,11 +1127,77 @@ static void handle_update(struct work_struct *work)
>  	cpufreq_update_policy(cpu);
>  }
>  
> +static void cpufreq_update_freq_work(struct work_struct *work)
> +{
> +	struct cpufreq_policy *policy =
> +		container_of(work, struct cpufreq_policy, req_work);
> +	struct cpufreq_policy new_policy = *policy;
> +
> +	/* We should read constraint values from QoS layer */
> +	new_policy.min = 0;
> +	new_policy.max = UINT_MAX;
> +
> +	down_write(&policy->rwsem);
> +
> +	if (!policy_is_inactive(policy))
> +		cpufreq_set_policy(policy, &new_policy);
> +
> +	up_write(&policy->rwsem);
> +}
> +
> +static int cpufreq_update_freq(struct cpufreq_policy *policy)
> +{
> +	schedule_work(&policy->req_work);
> +	return 0;
> +}
> +
> +static int cpufreq_notifier_min(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);
> +
> +	return cpufreq_update_freq(policy);
> +}
> +
> +static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy)
> +{
> +	struct kobject *kobj;
> +	struct completion *cmp;
> +
> +	down_write(&policy->rwsem);
> +	cpufreq_stats_free_table(policy);
> +	kobj = &policy->kobj;
> +	cmp = &policy->kobj_unregister;
> +	up_write(&policy->rwsem);
> +	kobject_put(kobj);
> +
> +	/*
> +	 * We need to make sure that the underlying kobj is
> +	 * actually not referenced anymore by anybody before we
> +	 * proceed with unloading.
> +	 */
> +	pr_debug("waiting for dropping of refcount\n");
> +	wait_for_completion(cmp);
> +	pr_debug("wait complete\n");
> +}
> +
>  static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>  {
>  	struct cpufreq_policy *policy;
> +	struct device *dev = get_cpu_device(cpu);
>  	int ret;
>  
> +	if (!dev)
> +		return NULL;
> +
>  	policy = kzalloc(sizeof(*policy), GFP_KERNEL);
>  	if (!policy)
>  		return NULL;
> @@ -1147,7 +1214,7 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
>  	ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
>  				   cpufreq_global_kobject, "policy%u", cpu);
>  	if (ret) {
> -		pr_err("%s: failed to init policy->kobj: %d\n", __func__, ret);
> +		dev_err(dev, "%s: failed to init policy->kobj: %d\n", __func__, ret);
>  		/*
>  		 * The entire policy object will be freed below, but the extra
>  		 * memory allocated for the kobject name needs to be freed by
> @@ -1157,16 +1224,41 @@ 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;
> +
> +	ret = dev_pm_qos_add_notifier(dev, &policy->nb_min,
> +				      DEV_PM_QOS_MIN_FREQUENCY);
> +	if (ret) {
> +		dev_err(dev, "Failed to register MIN QoS notifier: %d (%*pbl)\n",
> +			ret, cpumask_pr_args(policy->cpus));
> +		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;
> +	}
> +
>  	INIT_LIST_HEAD(&policy->policy_list);
>  	init_rwsem(&policy->rwsem);
>  	spin_lock_init(&policy->transition_lock);
>  	init_waitqueue_head(&policy->transition_wait);
>  	init_completion(&policy->kobj_unregister);
>  	INIT_WORK(&policy->update, handle_update);
> +	INIT_WORK(&policy->req_work, cpufreq_update_freq_work);

One more thing.

handle_update() is very similar to cpufreq_update_freq_work().

Why are both of them needed?




^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework
  2019-06-17 23:26   ` Rafael J. Wysocki
@ 2019-06-18 11:25     ` Viresh Kumar
  2019-06-18 22:23       ` Rafael J. Wysocki
  0 siblings, 1 reply; 27+ messages in thread
From: Viresh Kumar @ 2019-06-18 11:25 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-pm, Vincent Guittot, Qais.Yousef, mka, juri.lelli, linux-kernel

On 18-06-19, 01:26, Rafael J. Wysocki wrote:
> On Monday, June 10, 2019 12:51:35 PM CEST Viresh Kumar wrote:
> > +static int cpufreq_notifier_min(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);
> > +
> > +	return cpufreq_update_freq(policy);
> > +}
> 
> This is a bit convoluted.
> 
> Two different notifiers are registered basically for the same thing.
> 
> Any chance to use just one?

The way QoS is designed, it handles one value only at a time and we need two,
min/max. I thought a lot about it earlier and this is what I came up with :(

You have any suggestions here ?

-- 
viresh

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework
  2019-06-17 23:37   ` Rafael J. Wysocki
@ 2019-06-18 11:34     ` Viresh Kumar
  0 siblings, 0 replies; 27+ messages in thread
From: Viresh Kumar @ 2019-06-18 11:34 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-pm, Vincent Guittot, Qais.Yousef, mka, juri.lelli, linux-kernel

On 18-06-19, 01:37, Rafael J. Wysocki wrote:
> One more thing.
> 
> handle_update() is very similar to cpufreq_update_freq_work().
> 
> Why are both of them needed?

I probably did that because of locking required in cpufreq_update_freq_work()
but maybe I can do better. Lemme try and come back on it.

-- 
viresh

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework
  2019-06-18 11:25     ` Viresh Kumar
@ 2019-06-18 22:23       ` Rafael J. Wysocki
  2019-06-19  6:39         ` Viresh Kumar
  0 siblings, 1 reply; 27+ messages in thread
From: Rafael J. Wysocki @ 2019-06-18 22:23 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: linux-pm, Vincent Guittot, Qais.Yousef, mka, juri.lelli, linux-kernel

On Tuesday, June 18, 2019 1:25:22 PM CEST Viresh Kumar wrote:
> On 18-06-19, 01:26, Rafael J. Wysocki wrote:
> > On Monday, June 10, 2019 12:51:35 PM CEST Viresh Kumar wrote:
> > > +static int cpufreq_notifier_min(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);
> > > +
> > > +	return cpufreq_update_freq(policy);
> > > +}
> > 
> > This is a bit convoluted.
> > 
> > Two different notifiers are registered basically for the same thing.
> > 
> > Any chance to use just one?
> 
> The way QoS is designed, it handles one value only at a time and we need two,
> min/max. I thought a lot about it earlier and this is what I came up with :(
> 
> You have any suggestions here ?

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.




^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework
  2019-06-18 22:23       ` Rafael J. Wysocki
@ 2019-06-19  6:39         ` Viresh Kumar
  2019-06-19  9:15           ` Rafael J. Wysocki
  0 siblings, 1 reply; 27+ messages in thread
From: Viresh Kumar @ 2019-06-19  6:39 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-pm, Vincent Guittot, Qais.Yousef, mka, juri.lelli, linux-kernel

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 {

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework
  2019-06-19  6:39         ` Viresh Kumar
@ 2019-06-19  9:15           ` Rafael J. Wysocki
  0 siblings, 0 replies; 27+ messages in thread
From: Rafael J. Wysocki @ 2019-06-19  9:15 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael J. Wysocki, Linux PM, Vincent Guittot, Qais.Yousef,
	Matthias Kaehlcke, Juri Lelli, Linux Kernel Mailing List

On Wed, Jun 19, 2019 at 8:39 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> 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.

OK, leave it as is.  That's not a big deal.

It is slightly awkward, but oh well.

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, back to index

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-10 10:51 [PATCH V3 0/5] cpufreq: Use QoS layer to manage freq-constraints Viresh Kumar
2019-06-10 10:51 ` [PATCH V3 1/5] PM / QOS: Pass request type to dev_pm_qos_{add|remove}_notifier() Viresh Kumar
2019-06-11 23:45   ` Matthias Kaehlcke
2019-06-17  9:23   ` Ulf Hansson
2019-06-17 22:52   ` Rafael J. Wysocki
2019-06-10 10:51 ` [PATCH V3 2/5] PM / QOS: Pass request type to dev_pm_qos_read_value() Viresh Kumar
2019-06-12  0:08   ` Matthias Kaehlcke
2019-06-17  9:23   ` Ulf Hansson
2019-06-17 23:14   ` Rafael J. Wysocki
2019-06-10 10:51 ` [PATCH V3 3/5] PM / QoS: Add support for MIN/MAX frequency constraints Viresh Kumar
2019-06-13  0:04   ` Matthias Kaehlcke
2019-06-17  9:23   ` Ulf Hansson
2019-06-10 10:51 ` [PATCH V3 4/5] cpufreq: Register notifiers with the PM QoS framework Viresh Kumar
2019-06-14 16:46   ` Matthias Kaehlcke
2019-06-17  3:02     ` Viresh Kumar
2019-06-17  9:23   ` Ulf Hansson
2019-06-17 23:26   ` Rafael J. Wysocki
2019-06-18 11:25     ` Viresh Kumar
2019-06-18 22:23       ` Rafael J. Wysocki
2019-06-19  6:39         ` Viresh Kumar
2019-06-19  9:15           ` Rafael J. Wysocki
2019-06-17 23:37   ` Rafael J. Wysocki
2019-06-18 11:34     ` Viresh Kumar
2019-06-10 10:51 ` [PATCH V3 5/5] cpufreq: Add QoS requests for userspace constraints Viresh Kumar
2019-06-14 17:14   ` Matthias Kaehlcke
2019-06-17  3:07     ` Viresh Kumar
2019-06-17  9:23   ` Ulf Hansson

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org linux-pm@archiver.kernel.org
	public-inbox-index linux-pm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox