[PATCH 0/3] PM: QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY

[PATCH 0/3] PM: QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY

[Leonard Crestez]

Support for frequency limits in dev_pm_qos was removed when cpufreq was
switched to freq_qos, this series attempts to restore it by
reimplementing top of freq_qos.

Previous discussion here:

https://lore.kernel.org/linux-pm/VI1PR04MB7023DF47D046AEADB4E051EBEE680@VI1PR04MB7023.eurprd04.prod.outlook.com/T/#u

The cpufreq core switched away because it needs contraints at the level
of a "cpufreq_policy" which cover multiple cpus so dev_pm_qos coupling
to struct device was not useful (and was incorrectly handling). Cpufreq
could only use dev_pm_qos by implementing an additional layer of
aggregation from CPU to policy.

However the devfreq subsystem scaling is always performed for each
device so dev_pm_qos is a very good match. Support for dev_pm_qos
inside devfreq is implemented by this series:

	https://patchwork.kernel.org/cover/11171807/

Rafael: If this makes sense to you I could incorporate the restoration
of DEV_PM_QOS_MIN/MAX_FREQUENCY in v10 of the devfreq qos series.

In theory if freq_qos is extended to handle conflicting min/max values then
this sharing would be useful. Right now freq_qos just ties two unrelated
pm_qos aggregations for min/max freq.

---
This is implemented by embeding a freq_qos_request inside dev_pm_qos_request:
the data field was already an union in order to deal with flag requests.

The internal _freq_qos_apply is exported so that it can be called from
dev_pm_qos apply_constraints.

The dev_pm_qos_constraints_destroy function has no obvious equivalent in
freq_qos but really the whole approach of "removing requests" is somewhat dubios:
request objects should be owned by consumers and the list of qos requests
should be empty when the target device is deleted. Clearing the request
list and would likely result in a WARN next time "update_request" is
called by the requestor.

Leonard Crestez (3):
  PM: QoS: Reorder pm_qos/freq_qos/dev_pm_qos structs
  PM: QoS: Export _freq_qos_apply
  PM: QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY

 drivers/base/power/qos.c | 69 +++++++++++++++++++++++++++++---
 include/linux/pm_qos.h   | 86 +++++++++++++++++++++++-----------------
 kernel/power/qos.c       | 11 ++---
 3 files changed, 119 insertions(+), 47 deletions(-)

-- 
2.17.1

[PATCH 1/3] PM: QoS: Reorder pm_qos/freq_qos/dev_pm_qos structs

[Leonard Crestez]

This allows dev_pm_qos to embed freq_qos structs, which is done in the
next patch. Separate commit to make it easier to review.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 include/linux/pm_qos.h | 74 ++++++++++++++++++++++--------------------
 1 file changed, 38 insertions(+), 36 deletions(-)

diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index ebf5ef17cc2a..9105f47f5195 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -47,25 +47,10 @@ struct pm_qos_request {
 struct pm_qos_flags_request {
 	struct list_head node;
 	s32 flags;	/* Do not change to 64 bit */
 };
 
-enum dev_pm_qos_req_type {
-	DEV_PM_QOS_RESUME_LATENCY = 1,
-	DEV_PM_QOS_LATENCY_TOLERANCE,
-	DEV_PM_QOS_FLAGS,
-};
-
-struct dev_pm_qos_request {
-	enum dev_pm_qos_req_type type;
-	union {
-		struct plist_node pnode;
-		struct pm_qos_flags_request flr;
-	} data;
-	struct device *dev;
-};
-
 enum pm_qos_type {
 	PM_QOS_UNITIALIZED,
 	PM_QOS_MAX,		/* return the largest value */
 	PM_QOS_MIN,		/* return the smallest value */
 	PM_QOS_SUM		/* return the sum */
@@ -88,10 +73,48 @@ struct pm_qos_constraints {
 struct pm_qos_flags {
 	struct list_head list;
 	s32 effective_flags;	/* Do not change to 64 bit */
 };
 
+
+#define FREQ_QOS_MIN_DEFAULT_VALUE	0
+#define FREQ_QOS_MAX_DEFAULT_VALUE	(-1)
+
+enum freq_qos_req_type {
+	FREQ_QOS_MIN = 1,
+	FREQ_QOS_MAX,
+};
+
+struct freq_constraints {
+	struct pm_qos_constraints min_freq;
+	struct blocking_notifier_head min_freq_notifiers;
+	struct pm_qos_constraints max_freq;
+	struct blocking_notifier_head max_freq_notifiers;
+};
+
+struct freq_qos_request {
+	enum freq_qos_req_type type;
+	struct plist_node pnode;
+	struct freq_constraints *qos;
+};
+
+
+enum dev_pm_qos_req_type {
+	DEV_PM_QOS_RESUME_LATENCY = 1,
+	DEV_PM_QOS_LATENCY_TOLERANCE,
+	DEV_PM_QOS_FLAGS,
+};
+
+struct dev_pm_qos_request {
+	enum dev_pm_qos_req_type type;
+	union {
+		struct plist_node pnode;
+		struct pm_qos_flags_request flr;
+	} data;
+	struct device *dev;
+};
+
 struct dev_pm_qos {
 	struct pm_qos_constraints resume_latency;
 	struct pm_qos_constraints latency_tolerance;
 	struct pm_qos_flags flags;
 	struct dev_pm_qos_request *resume_latency_req;
@@ -253,31 +276,10 @@ static inline s32 dev_pm_qos_raw_resume_latency(struct device *dev)
 {
 	return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
 }
 #endif
 
-#define FREQ_QOS_MIN_DEFAULT_VALUE	0
-#define FREQ_QOS_MAX_DEFAULT_VALUE	(-1)
-
-enum freq_qos_req_type {
-	FREQ_QOS_MIN = 1,
-	FREQ_QOS_MAX,
-};
-
-struct freq_constraints {
-	struct pm_qos_constraints min_freq;
-	struct blocking_notifier_head min_freq_notifiers;
-	struct pm_qos_constraints max_freq;
-	struct blocking_notifier_head max_freq_notifiers;
-};
-
-struct freq_qos_request {
-	enum freq_qos_req_type type;
-	struct plist_node pnode;
-	struct freq_constraints *qos;
-};
-
 static inline int freq_qos_request_active(struct freq_qos_request *req)
 {
 	return !IS_ERR_OR_NULL(req->qos);
 }
 
-- 
2.17.1

[PATCH 2/3] PM: QoS: Export _freq_qos_apply

[Leonard Crestez]

This is exported only for dev_pm_qos to use in order to implement
per-device freq constraints.

Export with a leading underscore because this is an implementation
detail, it's not meant to be used by drivers making QoS requests.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 include/linux/pm_qos.h |  2 ++
 kernel/power/qos.c     | 11 ++++++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index 9105f47f5195..e90dae0b8de9 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -291,10 +291,12 @@ s32 freq_qos_read_value(struct freq_constraints *qos,
 int freq_qos_add_request(struct freq_constraints *qos,
 			 struct freq_qos_request *req,
 			 enum freq_qos_req_type type, s32 value);
 int freq_qos_update_request(struct freq_qos_request *req, s32 new_value);
 int freq_qos_remove_request(struct freq_qos_request *req);
+int _freq_qos_apply(struct freq_qos_request *req,
+		    enum pm_qos_req_action action, s32 value);
 
 int freq_qos_add_notifier(struct freq_constraints *qos,
 			  enum freq_qos_req_type type,
 			  struct notifier_block *notifier);
 int freq_qos_remove_notifier(struct freq_constraints *qos,
diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index 04e83fdfbe80..ea38ae86bd66 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -708,16 +708,16 @@ s32 freq_qos_read_value(struct freq_constraints *qos,
 
 	return ret;
 }
 
 /**
- * freq_qos_apply - Add/modify/remove frequency QoS request.
+ * _freq_qos_apply - Add/modify/remove frequency QoS request.
  * @req: Constraint request to apply.
  * @action: Action to perform (add/update/remove).
  * @value: Value to assign to the QoS request.
  */
-static int freq_qos_apply(struct freq_qos_request *req,
+int _freq_qos_apply(struct freq_qos_request *req,
 			  enum pm_qos_req_action action, s32 value)
 {
 	int ret;
 
 	switch(req->type) {
@@ -733,10 +733,11 @@ static int freq_qos_apply(struct freq_qos_request *req,
 		ret = -EINVAL;
 	}
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(_freq_qos_apply);
 
 /**
  * freq_qos_add_request - Insert new frequency QoS request into a given list.
  * @qos: Constraints to update.
  * @req: Preallocated request object.
@@ -763,11 +764,11 @@ int freq_qos_add_request(struct freq_constraints *qos,
 		 "%s() called for active request\n", __func__))
 		return -EINVAL;
 
 	req->qos = qos;
 	req->type = type;
-	ret = freq_qos_apply(req, PM_QOS_ADD_REQ, value);
+	ret = _freq_qos_apply(req, PM_QOS_ADD_REQ, value);
 	if (ret < 0) {
 		req->qos = NULL;
 		req->type = 0;
 	}
 
@@ -796,11 +797,11 @@ int freq_qos_update_request(struct freq_qos_request *req, s32 new_value)
 		return -EINVAL;
 
 	if (req->pnode.prio == new_value)
 		return 0;
 
-	return freq_qos_apply(req, PM_QOS_UPDATE_REQ, new_value);
+	return _freq_qos_apply(req, PM_QOS_UPDATE_REQ, new_value);
 }
 EXPORT_SYMBOL_GPL(freq_qos_update_request);
 
 /**
  * freq_qos_remove_request - Remove frequency QoS request from its list.
@@ -819,11 +820,11 @@ int freq_qos_remove_request(struct freq_qos_request *req)
 
 	if (WARN(!freq_qos_request_active(req),
 		 "%s() called for unknown object\n", __func__))
 		return -EINVAL;
 
-	return freq_qos_apply(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
+	return _freq_qos_apply(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
 }
 EXPORT_SYMBOL_GPL(freq_qos_remove_request);
 
 /**
  * freq_qos_add_notifier - Add frequency QoS change notifier.
-- 
2.17.1

[PATCH 3/3] PM: QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY

[Leonard Crestez]

Support for adding per-device frequency limits was removed in commit
2aac8bdf7a0f ("PM: QoS: Drop frequency QoS types from device PM QoS")
after cpufreq switched to use a new "freq_constraints" construct.

Restore support for per-device freq limits but base this upon
freq_constraints. This is primarily meant to be used by the devfreq
subsystem.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/base/power/qos.c | 69 ++++++++++++++++++++++++++++++++++++----
 include/linux/pm_qos.h   | 10 ++++++
 2 files changed, 73 insertions(+), 6 deletions(-)

diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index 350dcafd751f..52f74edee548 100644
--- a/drivers/base/power/qos.c
+++ b/drivers/base/power/qos.c
@@ -113,14 +113,24 @@ 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);
 
-	if (type == DEV_PM_QOS_RESUME_LATENCY) {
+	switch (type) {
+	case DEV_PM_QOS_RESUME_LATENCY:
 		ret = IS_ERR_OR_NULL(qos) ? PM_QOS_RESUME_LATENCY_NO_CONSTRAINT
 			: pm_qos_read_value(&qos->resume_latency);
-	} else {
+		break;
+	case DEV_PM_QOS_MIN_FREQUENCY:
+		ret = IS_ERR_OR_NULL(qos) ? PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE
+			: freq_qos_read_value(&qos->freq, FREQ_QOS_MIN);
+		break;
+	case DEV_PM_QOS_MAX_FREQUENCY:
+		ret = IS_ERR_OR_NULL(qos) ? PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE
+			: freq_qos_read_value(&qos->freq, FREQ_QOS_MAX);
+		break;
+	default:
 		WARN_ON(1);
 		ret = 0;
 	}
 
 	spin_unlock_irqrestore(&dev->power.lock, flags);
@@ -157,10 +167,14 @@ static int apply_constraint(struct dev_pm_qos_request *req,
 		if (ret) {
 			value = pm_qos_read_value(&qos->latency_tolerance);
 			req->dev->power.set_latency_tolerance(req->dev, value);
 		}
 		break;
+	case DEV_PM_QOS_MIN_FREQUENCY:
+	case DEV_PM_QOS_MAX_FREQUENCY:
+		ret = _freq_qos_apply(&req->data.freq, action, value);
+		break;
 	case DEV_PM_QOS_FLAGS:
 		ret = pm_qos_update_flags(&qos->flags, &req->data.flr,
 					  action, value);
 		break;
 	default:
@@ -207,10 +221,12 @@ static int dev_pm_qos_constraints_allocate(struct device *dev)
 	c->target_value = PM_QOS_LATENCY_TOLERANCE_DEFAULT_VALUE;
 	c->default_value = PM_QOS_LATENCY_TOLERANCE_DEFAULT_VALUE;
 	c->no_constraint_value = PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT;
 	c->type = PM_QOS_MIN;
 
+	freq_constraints_init(&qos->freq);
+
 	INIT_LIST_HEAD(&qos->flags.list);
 
 	spin_lock_irq(&dev->power.lock);
 	dev->power.qos = qos;
 	spin_unlock_irq(&dev->power.lock);
@@ -267,10 +283,22 @@ void dev_pm_qos_constraints_destroy(struct device *dev)
 	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->freq.min_freq;
+	plist_for_each_entry_safe(req, tmp, &c->list, data.freq.pnode) {
+		apply_constraint(req, PM_QOS_REMOVE_REQ, PM_QOS_MIN_FREQUENCY_DEFAULT_VALUE);
+		memset(req, 0, sizeof(*req));
+	}
+
+	c = &qos->freq.max_freq;
+	plist_for_each_entry_safe(req, tmp, &c->list, data.freq.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);
 		memset(req, 0, sizeof(*req));
 	}
@@ -312,15 +340,26 @@ static int __dev_pm_qos_add_request(struct device *dev,
 		ret = -ENODEV;
 	else if (!dev->power.qos)
 		ret = dev_pm_qos_constraints_allocate(dev);
 
 	trace_dev_pm_qos_add_request(dev_name(dev), type, value);
-	if (!ret) {
-		req->dev = dev;
-		req->type = type;
+	if (ret)
+		return ret;
+
+	req->dev = dev;
+	req->type = type;
+	if (req->type == DEV_PM_QOS_MIN_FREQUENCY)
+		ret = freq_qos_add_request(&dev->power.qos->freq,
+					   &req->data.freq,
+					   FREQ_QOS_MIN, value);
+	else if (req->type == DEV_PM_QOS_MAX_FREQUENCY)
+		ret = freq_qos_add_request(&dev->power.qos->freq,
+					   &req->data.freq,
+					   FREQ_QOS_MAX, value);
+	else
 		ret = apply_constraint(req, PM_QOS_ADD_REQ, value);
-	}
+
 	return ret;
 }
 
 /**
  * dev_pm_qos_add_request - inserts new qos request into the list
@@ -380,10 +419,14 @@ 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:
 		curr_value = req->data.pnode.prio;
 		break;
+	case DEV_PM_QOS_MIN_FREQUENCY:
+	case DEV_PM_QOS_MAX_FREQUENCY:
+		curr_value = req->data.freq.pnode.prio;
+		break;
 	case DEV_PM_QOS_FLAGS:
 		curr_value = req->data.flr.flags;
 		break;
 	default:
 		return -EINVAL;
@@ -505,10 +548,16 @@ int dev_pm_qos_add_notifier(struct device *dev, struct notifier_block *notifier,
 	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 = freq_qos_add_notifier(&dev->power.qos->freq, FREQ_QOS_MIN, notifier);
+		break;
+	case DEV_PM_QOS_MAX_FREQUENCY:
+		ret = freq_qos_add_notifier(&dev->power.qos->freq, FREQ_QOS_MAX, notifier);
+		break;
 	default:
 		WARN_ON(1);
 		ret = -EINVAL;
 	}
 
@@ -544,10 +593,18 @@ int dev_pm_qos_remove_notifier(struct device *dev,
 	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 = freq_qos_remove_notifier(&dev->power.qos->freq,
+					       FREQ_QOS_MIN, notifier);
+		break;
+	case DEV_PM_QOS_MAX_FREQUENCY:
+		ret = freq_qos_remove_notifier(&dev->power.qos->freq,
+					       FREQ_QOS_MAX, notifier);
+		break;
 	default:
 		WARN_ON(1);
 		ret = -EINVAL;
 	}
 
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index e90dae0b8de9..e97c2e376889 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -32,10 +32,12 @@ enum pm_qos_flags_status {
 #define PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE	(2000 * USEC_PER_SEC)
 #define PM_QOS_RESUME_LATENCY_DEFAULT_VALUE	PM_QOS_LATENCY_ANY
 #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)
 
 struct pm_qos_request {
@@ -99,25 +101,29 @@ struct freq_qos_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,
 };
 
 struct dev_pm_qos_request {
 	enum dev_pm_qos_req_type type;
 	union {
 		struct plist_node pnode;
 		struct pm_qos_flags_request flr;
+		struct freq_qos_request freq;
 	} data;
 	struct device *dev;
 };
 
 struct dev_pm_qos {
 	struct pm_qos_constraints resume_latency;
 	struct pm_qos_constraints latency_tolerance;
+	struct freq_constraints freq;
 	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;
 };
@@ -212,10 +218,14 @@ static inline s32 dev_pm_qos_read_value(struct device *dev,
 					enum dev_pm_qos_req_type type)
 {
 	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.17.1

Re: [PATCH 0/3] PM: QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY

[Leonard Crestez]

On 25.10.2019 21:00, Leonard Crestez wrote:
> Support for frequency limits in dev_pm_qos was removed when cpufreq was
> switched to freq_qos, this series attempts to restore it by
> reimplementing top of freq_qos.
> 
> Previous discussion here:
> 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-pm%2FVI1PR04MB7023DF47D046AEADB4E051EBEE680%40VI1PR04MB7023.eurprd04.prod.outlook.com%2FT%2F%23u&data=02%7C01%7Cleonard.crestez%40nxp.com%7C56eca0f61d714ccda20d08d75975467d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637076232541620636&sdata=X7yVr362%2F2LyvYLX%2FPlmadiHIR2Q1whFOXGdqecG6s4%3D&reserved=0
> 
> The cpufreq core switched away because it needs contraints at the level
> of a "cpufreq_policy" which cover multiple cpus so dev_pm_qos coupling
> to struct device was not useful (and was incorrectly handling). Cpufreq
> could only use dev_pm_qos by implementing an additional layer of
> aggregation from CPU to policy.
> 
> However the devfreq subsystem scaling is always performed for each
> device so dev_pm_qos is a very good match. Support for dev_pm_qos
> inside devfreq is implemented by this series:
> 
> 	https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11171807%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7C56eca0f61d714ccda20d08d75975467d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637076232541630633&sdata=REK2U%2B7xDg3wmXO1JM5aO%2BdjvpQkKh9%2BVrFz4ULxjnE%3D&reserved=0
> 
> Rafael: If this makes sense to you I could incorporate the restoration
> of DEV_PM_QOS_MIN/MAX_FREQUENCY in v10 of the devfreq qos series.
> 
> In theory if freq_qos is extended to handle conflicting min/max values then
> this sharing would be useful. Right now freq_qos just ties two unrelated
> pm_qos aggregations for min/max freq.
> 
> ---
> This is implemented by embeding a freq_qos_request inside dev_pm_qos_request:
> the data field was already an union in order to deal with flag requests.
> 
> The internal _freq_qos_apply is exported so that it can be called from
> dev_pm_qos apply_constraints.
> 
> The dev_pm_qos_constraints_destroy function has no obvious equivalent in
> freq_qos but really the whole approach of "removing requests" is somewhat dubios:
> request objects should be owned by consumers and the list of qos requests
> should be empty when the target device is deleted. Clearing the request
> list and would likely result in a WARN next time "update_request" is
> called by the requestor.
> 
> Leonard Crestez (3):
>    PM: QoS: Reorder pm_qos/freq_qos/dev_pm_qos structs
>    PM: QoS: Export _freq_qos_apply
>    PM: QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY
> 
>   drivers/base/power/qos.c | 69 +++++++++++++++++++++++++++++---
>   include/linux/pm_qos.h   | 86 +++++++++++++++++++++++-----------------
>   kernel/power/qos.c       | 11 ++---
>   3 files changed, 119 insertions(+), 47 deletions(-)

Any feedback?

The DEV_PM_QOS_MIN/MAX_FREQUENCY constraints are very useful for devfreq 
but devfreq maintainers seem unwilling to take such core changes. See:

     https://patchwork.kernel.org/patch/11221877/#22974093

This makes a lot of sense, PM QoS core changes should be reviewed by 
core PM maintainers.

What would it take to restore DEV_PM_QOS_MIN/MAX_FREQUENCY?

Cpufreq itself could still use DEV_PM_QOS if it performed two-stage 
aggregation. This would require a bit of additional effort in 
cpufreq_policy code but simplify consumers and expose fewer internals. 
This sounds like a worthwhile tradeoff.

--
Regards,
Leonard

Re: [PATCH 2/3] PM: QoS: Export _freq_qos_apply

[Rafael J. Wysocki]

On Friday, October 25, 2019 8:00:48 PM CET Leonard Crestez wrote:
> This is exported only for dev_pm_qos to use in order to implement
> per-device freq constraints.
> 
> Export with a leading underscore because this is an implementation
> detail, it's not meant to be used by drivers making QoS requests.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  include/linux/pm_qos.h |  2 ++
>  kernel/power/qos.c     | 11 ++++++-----
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
> index 9105f47f5195..e90dae0b8de9 100644
> --- a/include/linux/pm_qos.h
> +++ b/include/linux/pm_qos.h
> @@ -291,10 +291,12 @@ s32 freq_qos_read_value(struct freq_constraints *qos,
>  int freq_qos_add_request(struct freq_constraints *qos,
>  			 struct freq_qos_request *req,
>  			 enum freq_qos_req_type type, s32 value);
>  int freq_qos_update_request(struct freq_qos_request *req, s32 new_value);
>  int freq_qos_remove_request(struct freq_qos_request *req);
> +int _freq_qos_apply(struct freq_qos_request *req,
> +		    enum pm_qos_req_action action, s32 value);
>  
>  int freq_qos_add_notifier(struct freq_constraints *qos,
>  			  enum freq_qos_req_type type,
>  			  struct notifier_block *notifier);
>  int freq_qos_remove_notifier(struct freq_constraints *qos,
> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
> index 04e83fdfbe80..ea38ae86bd66 100644
> --- a/kernel/power/qos.c
> +++ b/kernel/power/qos.c
> @@ -708,16 +708,16 @@ s32 freq_qos_read_value(struct freq_constraints *qos,
>  
>  	return ret;
>  }
>  
>  /**
> - * freq_qos_apply - Add/modify/remove frequency QoS request.
> + * _freq_qos_apply - Add/modify/remove frequency QoS request.
>   * @req: Constraint request to apply.
>   * @action: Action to perform (add/update/remove).
>   * @value: Value to assign to the QoS request.
>   */
> -static int freq_qos_apply(struct freq_qos_request *req,
> +int _freq_qos_apply(struct freq_qos_request *req,
>  			  enum pm_qos_req_action action, s32 value)
>  {
>  	int ret;
>  
>  	switch(req->type) {
> @@ -733,10 +733,11 @@ static int freq_qos_apply(struct freq_qos_request *req,
>  		ret = -EINVAL;
>  	}
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(_freq_qos_apply);

The devuce PM QoS code is not modular, so this is not necessary.

And so I wouldn't change the name of the function, just make it non-static
and add its header to pm_qos.h.

>  
>  /**
>   * freq_qos_add_request - Insert new frequency QoS request into a given list.
>   * @qos: Constraints to update.
>   * @req: Preallocated request object.
> @@ -763,11 +764,11 @@ int freq_qos_add_request(struct freq_constraints *qos,
>  		 "%s() called for active request\n", __func__))
>  		return -EINVAL;
>  
>  	req->qos = qos;
>  	req->type = type;
> -	ret = freq_qos_apply(req, PM_QOS_ADD_REQ, value);
> +	ret = _freq_qos_apply(req, PM_QOS_ADD_REQ, value);
>  	if (ret < 0) {
>  		req->qos = NULL;
>  		req->type = 0;
>  	}
>  
> @@ -796,11 +797,11 @@ int freq_qos_update_request(struct freq_qos_request *req, s32 new_value)
>  		return -EINVAL;
>  
>  	if (req->pnode.prio == new_value)
>  		return 0;
>  
> -	return freq_qos_apply(req, PM_QOS_UPDATE_REQ, new_value);
> +	return _freq_qos_apply(req, PM_QOS_UPDATE_REQ, new_value);
>  }
>  EXPORT_SYMBOL_GPL(freq_qos_update_request);
>  
>  /**
>   * freq_qos_remove_request - Remove frequency QoS request from its list.
> @@ -819,11 +820,11 @@ int freq_qos_remove_request(struct freq_qos_request *req)
>  
>  	if (WARN(!freq_qos_request_active(req),
>  		 "%s() called for unknown object\n", __func__))
>  		return -EINVAL;
>  
> -	return freq_qos_apply(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
> +	return _freq_qos_apply(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
>  }
>  EXPORT_SYMBOL_GPL(freq_qos_remove_request);
>  
>  /**
>   * freq_qos_add_notifier - Add frequency QoS change notifier.
> 

Re: [PATCH 2/3] PM: QoS: Export _freq_qos_apply

[Leonard Crestez]

On 14.11.2019 00:24, Rafael J. Wysocki wrote:
> On Friday, October 25, 2019 8:00:48 PM CET Leonard Crestez wrote:
>> This is exported only for dev_pm_qos to use in order to implement
>> per-device freq constraints.
>>
>> Export with a leading underscore because this is an implementation
>> detail, it's not meant to be used by drivers making QoS requests.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>>   include/linux/pm_qos.h |  2 ++
>>   kernel/power/qos.c     | 11 ++++++-----
>>   2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
>> index 9105f47f5195..e90dae0b8de9 100644
>> --- a/include/linux/pm_qos.h
>> +++ b/include/linux/pm_qos.h
>> @@ -291,10 +291,12 @@ s32 freq_qos_read_value(struct freq_constraints *qos,
>>   int freq_qos_add_request(struct freq_constraints *qos,
>>   			 struct freq_qos_request *req,
>>   			 enum freq_qos_req_type type, s32 value);
>>   int freq_qos_update_request(struct freq_qos_request *req, s32 new_value);
>>   int freq_qos_remove_request(struct freq_qos_request *req);
>> +int _freq_qos_apply(struct freq_qos_request *req,
>> +		    enum pm_qos_req_action action, s32 value);
>>   
>>   int freq_qos_add_notifier(struct freq_constraints *qos,
>>   			  enum freq_qos_req_type type,
>>   			  struct notifier_block *notifier);
>>   int freq_qos_remove_notifier(struct freq_constraints *qos,
>> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
>> index 04e83fdfbe80..ea38ae86bd66 100644
>> --- a/kernel/power/qos.c
>> +++ b/kernel/power/qos.c
>> @@ -708,16 +708,16 @@ s32 freq_qos_read_value(struct freq_constraints *qos,
>>   
>>   	return ret;
>>   }
>>   
>>   /**
>> - * freq_qos_apply - Add/modify/remove frequency QoS request.
>> + * _freq_qos_apply - Add/modify/remove frequency QoS request.
>>    * @req: Constraint request to apply.
>>    * @action: Action to perform (add/update/remove).
>>    * @value: Value to assign to the QoS request.
>>    */
>> -static int freq_qos_apply(struct freq_qos_request *req,
>> +int _freq_qos_apply(struct freq_qos_request *req,
>>   			  enum pm_qos_req_action action, s32 value)
>>   {
>>   	int ret;
>>   
>>   	switch(req->type) {
>> @@ -733,10 +733,11 @@ static int freq_qos_apply(struct freq_qos_request *req,
>>   		ret = -EINVAL;
>>   	}
>>   
>>   	return ret;
>>   }
>> +EXPORT_SYMBOL_GPL(_freq_qos_apply);
> 
> The devuce PM QoS code is not modular, so this is not necessary.
> 
> And so I wouldn't change the name of the function, just make it non-static
> and add its header to pm_qos.h.

OK, and since the changes for exporting freq_qos_apply are so small I'll 
just I'll just squash into the third patch.

> 
>>   
>>   /**
>>    * freq_qos_add_request - Insert new frequency QoS request into a given list.
>>    * @qos: Constraints to update.
>>    * @req: Preallocated request object.
>> @@ -763,11 +764,11 @@ int freq_qos_add_request(struct freq_constraints *qos,
>>   		 "%s() called for active request\n", __func__))
>>   		return -EINVAL;
>>   
>>   	req->qos = qos;
>>   	req->type = type;
>> -	ret = freq_qos_apply(req, PM_QOS_ADD_REQ, value);
>> +	ret = _freq_qos_apply(req, PM_QOS_ADD_REQ, value);
>>   	if (ret < 0) {
>>   		req->qos = NULL;
>>   		req->type = 0;
>>   	}
>>   
>> @@ -796,11 +797,11 @@ int freq_qos_update_request(struct freq_qos_request *req, s32 new_value)
>>   		return -EINVAL;
>>   
>>   	if (req->pnode.prio == new_value)
>>   		return 0;
>>   
>> -	return freq_qos_apply(req, PM_QOS_UPDATE_REQ, new_value);
>> +	return _freq_qos_apply(req, PM_QOS_UPDATE_REQ, new_value);
>>   }
>>   EXPORT_SYMBOL_GPL(freq_qos_update_request);
>>   
>>   /**
>>    * freq_qos_remove_request - Remove frequency QoS request from its list.
>> @@ -819,11 +820,11 @@ int freq_qos_remove_request(struct freq_qos_request *req)
>>   
>>   	if (WARN(!freq_qos_request_active(req),
>>   		 "%s() called for unknown object\n", __func__))
>>   		return -EINVAL;
>>   
>> -	return freq_qos_apply(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
>> +	return _freq_qos_apply(req, PM_QOS_REMOVE_REQ, PM_QOS_DEFAULT_VALUE);
>>   }
>>   EXPORT_SYMBOL_GPL(freq_qos_remove_request);
>>   
>>   /**
>>    * freq_qos_add_notifier - Add frequency QoS change notifier.
>>
> 
> 
> 
> 
>