* [PATCH v4 0/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY
@ 2019-11-26 15:17 Leonard Crestez
2019-11-26 15:17 ` [PATCH v4 1/4] PM / QoS: Initial kunit test Leonard Crestez
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Leonard Crestez @ 2019-11-26 15:17 UTC (permalink / raw)
To: Rafael J. Wysocki, Matthias Kaehlcke, Chanwoo Choi
Cc: Viresh Kumar, MyungJoo Ham, Kyungmin Park, Artur Świgoń,
Angus Ainslie, Brendan Higgins, linux-kselftest, kunit-dev,
linux-pm, linux-imx
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 on top of freq_qos.
Discussion about removal is 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. Cpufreq could only use dev_pm_qos by
implementing an additional layer of aggregation anyway.
However in the devfreq subsystem scaling is always performed on a per-device
basis so dev_pm_qos is a very good match. Support for dev_pm_qos in devfreq
core is here (latest version, no dependencies outside this series):
https://patchwork.kernel.org/cover/11252409/
That series is RFC mostly because it needs these PM core patches.
Earlier versions got entangled in some locking cleanups but those are
not strictly necessary to get dev_pm_qos functionality.
In theory if freq_qos is extended to handle conflicting min/max values then
this sharing would be valuable. Right now freq_qos just ties two unrelated
pm_qos aggregations for min and 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 and the whole approach of "removing requests" is somewhat dubios:
request objects should be owned by consumers and the list of qos requests
will most likely be empty when the target device is deleted. Series follows
current pattern for dev_pm_qos.
First two patches can be applied separately.
Changes since v3:
* Fix s/QOS/QoS in patch 2 title
* Improves comments in kunit test
* Fix assertions after freq_qos_remove_request
* Remove (c) from NXP copyright header
* Wrap long lines in qos.c to be under 80 chars. This fixes checkpatch but the
rule is already broken by code in the files.
* Collect reviews
Link to v3: https://patchwork.kernel.org/cover/11260627/
Changes since v2:
* #define PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE FREQ_QOS_MAX_DEFAULT_VALUE
* #define FREQ_QOS_MAX_DEFAULT_VALUE S32_MAX (in new patch)
* Add initial kunit test for freq_qos, validating the MAX_DEFAULT_VALUE found
by Matthias and another recent fix. Testing this should be easier!
Link to v2: https://patchwork.kernel.org/cover/11250413/
Changes since v1:
* Don't rename or EXPORT_SYMBOL_GPL the freq_qos_apply function; just
drop the static marker.
Link to v1: https://patchwork.kernel.org/cover/11212887/
Leonard Crestez (4):
PM / QoS: Initial kunit test
PM / QoS: Redefine FREQ_QOS_MAX_DEFAULT_VALUE to S32_MAX
PM / QoS: Reorder pm_qos/freq_qos/dev_pm_qos structs
PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY
drivers/base/Kconfig | 4 ++
drivers/base/power/Makefile | 1 +
drivers/base/power/qos-test.c | 117 ++++++++++++++++++++++++++++++++++
drivers/base/power/qos.c | 73 +++++++++++++++++++--
include/linux/pm_qos.h | 86 ++++++++++++++-----------
kernel/power/qos.c | 4 +-
6 files changed, 242 insertions(+), 43 deletions(-)
create mode 100644 drivers/base/power/qos-test.c
--
2.17.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 1/4] PM / QoS: Initial kunit test
2019-11-26 15:17 [PATCH v4 0/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY Leonard Crestez
@ 2019-11-26 15:17 ` Leonard Crestez
2019-11-26 20:04 ` Matthias Kaehlcke
2019-11-26 15:17 ` [PATCH v4 2/4] PM / QoS: Redefine FREQ_QOS_MAX_DEFAULT_VALUE to S32_MAX Leonard Crestez
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Leonard Crestez @ 2019-11-26 15:17 UTC (permalink / raw)
To: Rafael J. Wysocki, Matthias Kaehlcke, Chanwoo Choi
Cc: Viresh Kumar, MyungJoo Ham, Kyungmin Park, Artur Świgoń,
Angus Ainslie, Brendan Higgins, linux-kselftest, kunit-dev,
linux-pm, linux-imx
The pm_qos family of APIs are used in relatively difficult to reproduce
scenarios such as thermal throttling so they benefit from unit testing.
Start by adding basic tests from the the freq_qos APIs. It includes
tests for issues that were brought up on mailing lists:
https://patchwork.kernel.org/patch/11252425/#23017005
https://patchwork.kernel.org/patch/11253421/
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
drivers/base/Kconfig | 4 ++
drivers/base/power/Makefile | 1 +
drivers/base/power/qos-test.c | 117 ++++++++++++++++++++++++++++++++++
3 files changed, 122 insertions(+)
create mode 100644 drivers/base/power/qos-test.c
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index e37d37684132..d4ae1c1adf69 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -155,10 +155,14 @@ config DEBUG_TEST_DRIVER_REMOVE
This option is expected to find errors and may render your system
unusable. You should say N here unless you are explicitly looking to
test this functionality.
+config PM_QOS_KUNIT_TEST
+ bool "KUnit Test for PM QoS features"
+ depends on KUNIT
+
config HMEM_REPORTING
bool
default n
depends on NUMA
help
diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
index ec5bb190b9d0..8fdd0073eeeb 100644
--- a/drivers/base/power/Makefile
+++ b/drivers/base/power/Makefile
@@ -2,7 +2,8 @@
obj-$(CONFIG_PM) += sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o wakeup_stats.o
obj-$(CONFIG_PM_TRACE_RTC) += trace.o
obj-$(CONFIG_PM_GENERIC_DOMAINS) += domain.o domain_governor.o
obj-$(CONFIG_HAVE_CLK) += clock_ops.o
+obj-$(CONFIG_PM_QOS_KUNIT_TEST) += qos-test.o
ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
diff --git a/drivers/base/power/qos-test.c b/drivers/base/power/qos-test.c
new file mode 100644
index 000000000000..3115db08d56b
--- /dev/null
+++ b/drivers/base/power/qos-test.c
@@ -0,0 +1,117 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 NXP
+ */
+#include <kunit/test.h>
+#include <linux/pm_qos.h>
+
+/* Basic test for aggregating two "min" requests */
+static void freq_qos_test_min(struct kunit *test)
+{
+ struct freq_constraints qos;
+ struct freq_qos_request req1, req2;
+ int ret;
+
+ freq_constraints_init(&qos);
+ memset(&req1, 0, sizeof(req1));
+ memset(&req2, 0, sizeof(req2));
+
+ ret = freq_qos_add_request(&qos, &req1, FREQ_QOS_MIN, 1000);
+ KUNIT_EXPECT_EQ(test, ret, 1);
+ ret = freq_qos_add_request(&qos, &req2, FREQ_QOS_MIN, 2000);
+ KUNIT_EXPECT_EQ(test, ret, 1);
+
+ KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN), 2000);
+
+ ret = freq_qos_remove_request(&req2);
+ KUNIT_EXPECT_EQ(test, ret, 1);
+ KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN), 1000);
+
+ ret = freq_qos_remove_request(&req1);
+ KUNIT_EXPECT_EQ(test, ret, 1);
+ KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN),
+ FREQ_QOS_MIN_DEFAULT_VALUE);
+}
+
+/* Test that requests for MAX_DEFAULT_VALUE have no effect */
+static void freq_qos_test_maxdef(struct kunit *test)
+{
+ struct freq_constraints qos;
+ struct freq_qos_request req1, req2;
+ int ret;
+
+ freq_constraints_init(&qos);
+ memset(&req1, 0, sizeof(req1));
+ memset(&req2, 0, sizeof(req2));
+ KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MAX),
+ FREQ_QOS_MAX_DEFAULT_VALUE);
+
+ ret = freq_qos_add_request(&qos, &req1, FREQ_QOS_MAX,
+ FREQ_QOS_MAX_DEFAULT_VALUE);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+ ret = freq_qos_add_request(&qos, &req2, FREQ_QOS_MAX,
+ FREQ_QOS_MAX_DEFAULT_VALUE);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+
+ /* Add max 1000 */
+ ret = freq_qos_update_request(&req1, 1000);
+ KUNIT_EXPECT_EQ(test, ret, 1);
+ KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MAX), 1000);
+
+ /* Add max 2000, no impact */
+ ret = freq_qos_update_request(&req2, 2000);
+ KUNIT_EXPECT_EQ(test, ret, 0);
+ KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MAX), 1000);
+
+ /* Remove max 1000, new max 2000 */
+ ret = freq_qos_remove_request(&req1);
+ KUNIT_EXPECT_EQ(test, ret, 1);
+ KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MAX), 2000);
+}
+
+/*
+ * Test that a freq_qos_request can be added again after removal
+ *
+ * This issue was solved by commit 05ff1ba412fd ("PM: QoS: Invalidate frequency
+ * QoS requests after removal")
+ */
+static void freq_qos_test_readd(struct kunit *test)
+{
+ struct freq_constraints qos;
+ struct freq_qos_request req;
+ int ret;
+
+ freq_constraints_init(&qos);
+ memset(&req, 0, sizeof(req));
+ KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN),
+ FREQ_QOS_MIN_DEFAULT_VALUE);
+
+ /* Add */
+ ret = freq_qos_add_request(&qos, &req, FREQ_QOS_MIN, 1000);
+ KUNIT_EXPECT_EQ(test, ret, 1);
+ KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN), 1000);
+
+ /* Remove */
+ ret = freq_qos_remove_request(&req);
+ KUNIT_EXPECT_EQ(test, ret, 1);
+ KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN),
+ FREQ_QOS_MIN_DEFAULT_VALUE);
+
+ /* Add again */
+ ret = freq_qos_add_request(&qos, &req, FREQ_QOS_MIN, 2000);
+ KUNIT_EXPECT_EQ(test, ret, 1);
+ KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN), 2000);
+}
+
+static struct kunit_case pm_qos_test_cases[] = {
+ KUNIT_CASE(freq_qos_test_min),
+ KUNIT_CASE(freq_qos_test_maxdef),
+ KUNIT_CASE(freq_qos_test_readd),
+ {},
+};
+
+static struct kunit_suite pm_qos_test_module = {
+ .name = "qos-kunit-test",
+ .test_cases = pm_qos_test_cases,
+};
+kunit_test_suite(pm_qos_test_module);
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 2/4] PM / QoS: Redefine FREQ_QOS_MAX_DEFAULT_VALUE to S32_MAX
2019-11-26 15:17 [PATCH v4 0/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY Leonard Crestez
2019-11-26 15:17 ` [PATCH v4 1/4] PM / QoS: Initial kunit test Leonard Crestez
@ 2019-11-26 15:17 ` Leonard Crestez
2019-11-26 15:17 ` [PATCH v4 3/4] PM / QoS: Reorder pm_qos/freq_qos/dev_pm_qos structs Leonard Crestez
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Leonard Crestez @ 2019-11-26 15:17 UTC (permalink / raw)
To: Rafael J. Wysocki, Matthias Kaehlcke, Chanwoo Choi
Cc: Viresh Kumar, MyungJoo Ham, Kyungmin Park, Artur Świgoń,
Angus Ainslie, Brendan Higgins, linux-kselftest, kunit-dev,
linux-pm, linux-imx
QOS requests for DEFAULT_VALUE are supposed to be ignored but this is
not the case for FREQ_QOS_MAX. Adding one request for MAX_DEFAULT_VALUE
and one for a real value will cause freq_qos_read_value to unexpectedly
return MAX_DEFAULT_VALUE (-1).
This happens because freq_qos max value is aggregated with PM_QOS_MIN
but FREQ_QOS_MAX_DEFAULT_VALUE is (-1) so it's smaller than other
values.
Fix this by redefining FREQ_QOS_MAX_DEFAULT_VALUE to S32_MAX.
Looking at current users for freq_qos it seems that none of them create
requests for FREQ_QOS_MAX_DEFAULT_VALUE.
Fixes: 77751a466ebd ("PM: QoS: Introduce frequency QoS")
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reported-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
include/linux/pm_qos.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/pm_qos.h b/include/linux/pm_qos.h
index ebf5ef17cc2a..24a6263c9931 100644
--- a/include/linux/pm_qos.h
+++ b/include/linux/pm_qos.h
@@ -254,11 +254,11 @@ 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)
+#define FREQ_QOS_MAX_DEFAULT_VALUE S32_MAX
enum freq_qos_req_type {
FREQ_QOS_MIN = 1,
FREQ_QOS_MAX,
};
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 3/4] PM / QoS: Reorder pm_qos/freq_qos/dev_pm_qos structs
2019-11-26 15:17 [PATCH v4 0/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY Leonard Crestez
2019-11-26 15:17 ` [PATCH v4 1/4] PM / QoS: Initial kunit test Leonard Crestez
2019-11-26 15:17 ` [PATCH v4 2/4] PM / QoS: Redefine FREQ_QOS_MAX_DEFAULT_VALUE to S32_MAX Leonard Crestez
@ 2019-11-26 15:17 ` Leonard Crestez
2019-11-26 15:17 ` [PATCH v4 4/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY Leonard Crestez
2019-11-29 11:34 ` [PATCH v4 0/4] " Rafael J. Wysocki
4 siblings, 0 replies; 13+ messages in thread
From: Leonard Crestez @ 2019-11-26 15:17 UTC (permalink / raw)
To: Rafael J. Wysocki, Matthias Kaehlcke, Chanwoo Choi
Cc: Viresh Kumar, MyungJoo Ham, Kyungmin Park, Artur Świgoń,
Angus Ainslie, Brendan Higgins, linux-kselftest, kunit-dev,
linux-pm, linux-imx
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>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
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 24a6263c9931..678fec6da5b9 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 S32_MAX
+
+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 S32_MAX
-
-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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v4 4/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY
2019-11-26 15:17 [PATCH v4 0/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY Leonard Crestez
` (2 preceding siblings ...)
2019-11-26 15:17 ` [PATCH v4 3/4] PM / QoS: Reorder pm_qos/freq_qos/dev_pm_qos structs Leonard Crestez
@ 2019-11-26 15:17 ` Leonard Crestez
2019-11-26 21:01 ` Matthias Kaehlcke
2019-11-29 11:34 ` [PATCH v4 0/4] " Rafael J. Wysocki
4 siblings, 1 reply; 13+ messages in thread
From: Leonard Crestez @ 2019-11-26 15:17 UTC (permalink / raw)
To: Rafael J. Wysocki, Matthias Kaehlcke, Chanwoo Choi
Cc: Viresh Kumar, MyungJoo Ham, Kyungmin Park, Artur Świgoń,
Angus Ainslie, Brendan Higgins, linux-kselftest, kunit-dev,
linux-pm, linux-imx
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.
This removes the "static" marking on freq_qos_apply but does not export
it for modules.
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
drivers/base/power/qos.c | 73 ++++++++++++++++++++++++++++++++++++----
include/linux/pm_qos.h | 12 +++++++
kernel/power/qos.c | 4 ++-
3 files changed, 82 insertions(+), 7 deletions(-)
diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
index 350dcafd751f..8e93167f1783 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,24 @@ 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 +342,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 +421,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 +550,18 @@ 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 +597,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 678fec6da5b9..19eafca5680e 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 FREQ_QOS_MAX_DEFAULT_VALUE
#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;
}
}
@@ -291,10 +301,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 a45cba7df0ae..83edf8698118 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -712,12 +712,14 @@ s32 freq_qos_read_value(struct freq_constraints *qos,
/**
* 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.
+ *
+ * This is only meant to be called from inside pm_qos, not drivers.
*/
-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) {
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/4] PM / QoS: Initial kunit test
2019-11-26 15:17 ` [PATCH v4 1/4] PM / QoS: Initial kunit test Leonard Crestez
@ 2019-11-26 20:04 ` Matthias Kaehlcke
2019-11-27 23:40 ` Leonard Crestez
0 siblings, 1 reply; 13+ messages in thread
From: Matthias Kaehlcke @ 2019-11-26 20:04 UTC (permalink / raw)
To: Leonard Crestez
Cc: Rafael J. Wysocki, Chanwoo Choi, Viresh Kumar, MyungJoo Ham,
Kyungmin Park, Artur Świgoń,
Angus Ainslie, Brendan Higgins, linux-kselftest, kunit-dev,
linux-pm, linux-imx
On Tue, Nov 26, 2019 at 05:17:10PM +0200, Leonard Crestez wrote:
> The pm_qos family of APIs are used in relatively difficult to reproduce
> scenarios such as thermal throttling so they benefit from unit testing.
>
> Start by adding basic tests from the the freq_qos APIs. It includes
> tests for issues that were brought up on mailing lists:
>
> https://patchwork.kernel.org/patch/11252425/#23017005
> https://patchwork.kernel.org/patch/11253421/
>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
> drivers/base/Kconfig | 4 ++
> drivers/base/power/Makefile | 1 +
> drivers/base/power/qos-test.c | 117 ++++++++++++++++++++++++++++++++++
> 3 files changed, 122 insertions(+)
> create mode 100644 drivers/base/power/qos-test.c
>
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index e37d37684132..d4ae1c1adf69 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -155,10 +155,14 @@ config DEBUG_TEST_DRIVER_REMOVE
>
> This option is expected to find errors and may render your system
> unusable. You should say N here unless you are explicitly looking to
> test this functionality.
>
> +config PM_QOS_KUNIT_TEST
> + bool "KUnit Test for PM QoS features"
> + depends on KUNIT
> +
> config HMEM_REPORTING
> bool
> default n
> depends on NUMA
> help
> diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
> index ec5bb190b9d0..8fdd0073eeeb 100644
> --- a/drivers/base/power/Makefile
> +++ b/drivers/base/power/Makefile
> @@ -2,7 +2,8 @@
> obj-$(CONFIG_PM) += sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
> obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o wakeup_stats.o
> obj-$(CONFIG_PM_TRACE_RTC) += trace.o
> obj-$(CONFIG_PM_GENERIC_DOMAINS) += domain.o domain_governor.o
> obj-$(CONFIG_HAVE_CLK) += clock_ops.o
> +obj-$(CONFIG_PM_QOS_KUNIT_TEST) += qos-test.o
>
> ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
> diff --git a/drivers/base/power/qos-test.c b/drivers/base/power/qos-test.c
> new file mode 100644
> index 000000000000..3115db08d56b
> --- /dev/null
> +++ b/drivers/base/power/qos-test.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019 NXP
> + */
> +#include <kunit/test.h>
> +#include <linux/pm_qos.h>
> +
> +/* Basic test for aggregating two "min" requests */
> +static void freq_qos_test_min(struct kunit *test)
> +{
> + struct freq_constraints qos;
> + struct freq_qos_request req1, req2;
> + int ret;
> +
> + freq_constraints_init(&qos);
> + memset(&req1, 0, sizeof(req1));
> + memset(&req2, 0, sizeof(req2));
> +
> + ret = freq_qos_add_request(&qos, &req1, FREQ_QOS_MIN, 1000);
> + KUNIT_EXPECT_EQ(test, ret, 1);
> + ret = freq_qos_add_request(&qos, &req2, FREQ_QOS_MIN, 2000);
> + KUNIT_EXPECT_EQ(test, ret, 1);
> +
> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN), 2000);
> +
> + ret = freq_qos_remove_request(&req2);
> + KUNIT_EXPECT_EQ(test, ret, 1);
> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN), 1000);
> +
> + ret = freq_qos_remove_request(&req1);
> + KUNIT_EXPECT_EQ(test, ret, 1);
> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN),
> + FREQ_QOS_MIN_DEFAULT_VALUE);
> +}
> +
> +/* Test that requests for MAX_DEFAULT_VALUE have no effect */
> +static void freq_qos_test_maxdef(struct kunit *test)
> +{
> + struct freq_constraints qos;
> + struct freq_qos_request req1, req2;
> + int ret;
> +
> + freq_constraints_init(&qos);
> + memset(&req1, 0, sizeof(req1));
> + memset(&req2, 0, sizeof(req2));
> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MAX),
> + FREQ_QOS_MAX_DEFAULT_VALUE);
> +
> + ret = freq_qos_add_request(&qos, &req1, FREQ_QOS_MAX,
> + FREQ_QOS_MAX_DEFAULT_VALUE);
> + KUNIT_EXPECT_EQ(test, ret, 0);
> + ret = freq_qos_add_request(&qos, &req2, FREQ_QOS_MAX,
> + FREQ_QOS_MAX_DEFAULT_VALUE);
> + KUNIT_EXPECT_EQ(test, ret, 0);
> +
> + /* Add max 1000 */
> + ret = freq_qos_update_request(&req1, 1000);
> + KUNIT_EXPECT_EQ(test, ret, 1);
> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MAX), 1000);
> +
> + /* Add max 2000, no impact */
> + ret = freq_qos_update_request(&req2, 2000);
> + KUNIT_EXPECT_EQ(test, ret, 0);
> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MAX), 1000);
> +
> + /* Remove max 1000, new max 2000 */
> + ret = freq_qos_remove_request(&req1);
> + KUNIT_EXPECT_EQ(test, ret, 1);
> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MAX), 2000);
nit: this last part isn't really related with MAX_DEFAULT_VALUE. It's a
worthwhile test, but not necessarily in this test case. It might make more sense
to set one of the constraints to FREQ_QOS_MAX_DEFAULT_VALUE again, and verify it
doesn't have an impact.
Just a comment, there's nothing really wrong with how it is.
> +}
> +
> +/*
> + * Test that a freq_qos_request can be added again after removal
> + *
> + * This issue was solved by commit 05ff1ba412fd ("PM: QoS: Invalidate frequency
> + * QoS requests after removal")
> + */
> +static void freq_qos_test_readd(struct kunit *test)
> +{
> + struct freq_constraints qos;
> + struct freq_qos_request req;
> + int ret;
> +
> + freq_constraints_init(&qos);
> + memset(&req, 0, sizeof(req));
> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN),
> + FREQ_QOS_MIN_DEFAULT_VALUE);
nit: you could do this check once in a dedicated test and omit it
in other tests to de-clutter
> +
> + /* Add */
> + ret = freq_qos_add_request(&qos, &req, FREQ_QOS_MIN, 1000);
> + KUNIT_EXPECT_EQ(test, ret, 1);
> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN), 1000);
similar here, this test validates re-adding, another dedicated test
could verify once that the aggregate value is correct after adding a single
request. Checking the return value still is sensible, just in case.
I guess it can be argued either way, checking the values every time is
extra-safe, omitting the checks reduces clutter and might help to make it
clearer what the test really intends to verify.
> +
> + /* Remove */
> + ret = freq_qos_remove_request(&req);
> + KUNIT_EXPECT_EQ(test, ret, 1);
> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN),
> + FREQ_QOS_MIN_DEFAULT_VALUE);
ditto
> + /* Add again */
> + ret = freq_qos_add_request(&qos, &req, FREQ_QOS_MIN, 2000);
> + KUNIT_EXPECT_EQ(test, ret, 1);
> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN), 2000);
Here the explicit check makes sense, since we verify re-adding.
Anyway, my comments are just about possible improvements, it's also fine
as is:
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 4/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY
2019-11-26 15:17 ` [PATCH v4 4/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY Leonard Crestez
@ 2019-11-26 21:01 ` Matthias Kaehlcke
2019-11-29 5:55 ` Leonard Crestez
0 siblings, 1 reply; 13+ messages in thread
From: Matthias Kaehlcke @ 2019-11-26 21:01 UTC (permalink / raw)
To: Leonard Crestez
Cc: Rafael J. Wysocki, Chanwoo Choi, Viresh Kumar, MyungJoo Ham,
Kyungmin Park, Artur Świgoń,
Angus Ainslie, Brendan Higgins, linux-kselftest, kunit-dev,
linux-pm, linux-imx
On Tue, Nov 26, 2019 at 05:17:13PM +0200, Leonard Crestez wrote:
> 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.
>
> This removes the "static" marking on freq_qos_apply but does not export
> it for modules.
>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
> drivers/base/power/qos.c | 73 ++++++++++++++++++++++++++++++++++++----
> include/linux/pm_qos.h | 12 +++++++
> kernel/power/qos.c | 4 ++-
> 3 files changed, 82 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
> index 350dcafd751f..8e93167f1783 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,24 @@ 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));
> + }
> +
nit: these loops look a bit redundant, might be worth to encapsulate them in a
macro.
> 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 +342,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 +421,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 +550,18 @@ 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 +597,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 678fec6da5b9..19eafca5680e 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 FREQ_QOS_MAX_DEFAULT_VALUE
> #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;
> }
> }
> @@ -291,10 +301,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 a45cba7df0ae..83edf8698118 100644
> --- a/kernel/power/qos.c
> +++ b/kernel/power/qos.c
> @@ -712,12 +712,14 @@ s32 freq_qos_read_value(struct freq_constraints *qos,
> /**
> * 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.
> + *
> + * This is only meant to be called from inside pm_qos, not drivers.
> */
> -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) {
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Tested-by: Matthias Kaehlcke <mka@chromium.org>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/4] PM / QoS: Initial kunit test
2019-11-26 20:04 ` Matthias Kaehlcke
@ 2019-11-27 23:40 ` Leonard Crestez
2019-12-02 16:49 ` Brendan Higgins
0 siblings, 1 reply; 13+ messages in thread
From: Leonard Crestez @ 2019-11-27 23:40 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Rafael J. Wysocki, Chanwoo Choi, Viresh Kumar, MyungJoo Ham,
Kyungmin Park, Artur Świgoń,
Angus Ainslie, Brendan Higgins, linux-kselftest, kunit-dev,
linux-pm, dl-linux-imx
On 26.11.2019 22:04, Matthias Kaehlcke wrote:
> On Tue, Nov 26, 2019 at 05:17:10PM +0200, Leonard Crestez wrote:
>> The pm_qos family of APIs are used in relatively difficult to reproduce
>> scenarios such as thermal throttling so they benefit from unit testing.
>>
>> Start by adding basic tests from the the freq_qos APIs. It includes
>> tests for issues that were brought up on mailing lists:
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>> drivers/base/Kconfig | 4 ++
>> drivers/base/power/Makefile | 1 +
>> drivers/base/power/qos-test.c | 117 ++++++++++++++++++++++++++++++++++
>> 3 files changed, 122 insertions(+)
>> create mode 100644 drivers/base/power/qos-test.c
>>
>> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
>> index e37d37684132..d4ae1c1adf69 100644
>> --- a/drivers/base/Kconfig
>> +++ b/drivers/base/Kconfig
>> @@ -155,10 +155,14 @@ config DEBUG_TEST_DRIVER_REMOVE
>>
>> This option is expected to find errors and may render your system
>> unusable. You should say N here unless you are explicitly looking to
>> test this functionality.
>>
>> +config PM_QOS_KUNIT_TEST
>> + bool "KUnit Test for PM QoS features"
>> + depends on KUNIT
>> +
>> config HMEM_REPORTING
>> bool
>> default n
>> depends on NUMA
>> help
>> diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
>> index ec5bb190b9d0..8fdd0073eeeb 100644
>> --- a/drivers/base/power/Makefile
>> +++ b/drivers/base/power/Makefile
>> @@ -2,7 +2,8 @@
>> obj-$(CONFIG_PM) += sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
>> obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o wakeup_stats.o
>> obj-$(CONFIG_PM_TRACE_RTC) += trace.o
>> obj-$(CONFIG_PM_GENERIC_DOMAINS) += domain.o domain_governor.o
>> obj-$(CONFIG_HAVE_CLK) += clock_ops.o
>> +obj-$(CONFIG_PM_QOS_KUNIT_TEST) += qos-test.o
>>
>> ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
>> diff --git a/drivers/base/power/qos-test.c b/drivers/base/power/qos-test.c
>> new file mode 100644
>> index 000000000000..3115db08d56b
>> --- /dev/null
>> +++ b/drivers/base/power/qos-test.c
>> @@ -0,0 +1,117 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright 2019 NXP
>> + */
>> +#include <kunit/test.h>
>> +#include <linux/pm_qos.h>
>> +
>> +/* Basic test for aggregating two "min" requests */
>> +static void freq_qos_test_min(struct kunit *test)
>> +{
>> + struct freq_constraints qos;
>> + struct freq_qos_request req1, req2;
>> + int ret;
>> +
>> + freq_constraints_init(&qos);
>> + memset(&req1, 0, sizeof(req1));
>> + memset(&req2, 0, sizeof(req2));
>> +
>> + ret = freq_qos_add_request(&qos, &req1, FREQ_QOS_MIN, 1000);
>> + KUNIT_EXPECT_EQ(test, ret, 1);
>> + ret = freq_qos_add_request(&qos, &req2, FREQ_QOS_MIN, 2000);
>> + KUNIT_EXPECT_EQ(test, ret, 1);
>> +
>> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN), 2000);
>> +
>> + ret = freq_qos_remove_request(&req2);
>> + KUNIT_EXPECT_EQ(test, ret, 1);
>> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN), 1000);
>> +
>> + ret = freq_qos_remove_request(&req1);
>> + KUNIT_EXPECT_EQ(test, ret, 1);
>> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN),
>> + FREQ_QOS_MIN_DEFAULT_VALUE);
>> +}
>> +
>> +/* Test that requests for MAX_DEFAULT_VALUE have no effect */
>> +static void freq_qos_test_maxdef(struct kunit *test)
>> +{
>> + struct freq_constraints qos;
>> + struct freq_qos_request req1, req2;
>> + int ret;
>> +
>> + freq_constraints_init(&qos);
>> + memset(&req1, 0, sizeof(req1));
>> + memset(&req2, 0, sizeof(req2));
>> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MAX),
>> + FREQ_QOS_MAX_DEFAULT_VALUE);
>> +
>> + ret = freq_qos_add_request(&qos, &req1, FREQ_QOS_MAX,
>> + FREQ_QOS_MAX_DEFAULT_VALUE);
>> + KUNIT_EXPECT_EQ(test, ret, 0);
>> + ret = freq_qos_add_request(&qos, &req2, FREQ_QOS_MAX,
>> + FREQ_QOS_MAX_DEFAULT_VALUE);
>> + KUNIT_EXPECT_EQ(test, ret, 0);
>> +
>> + /* Add max 1000 */
>> + ret = freq_qos_update_request(&req1, 1000);
>> + KUNIT_EXPECT_EQ(test, ret, 1);
>> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MAX), 1000);
>> +
>> + /* Add max 2000, no impact */
>> + ret = freq_qos_update_request(&req2, 2000);
>> + KUNIT_EXPECT_EQ(test, ret, 0);
>> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MAX), 1000);
>> +
>> + /* Remove max 1000, new max 2000 */
>> + ret = freq_qos_remove_request(&req1);
>> + KUNIT_EXPECT_EQ(test, ret, 1);
>> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MAX), 2000);
>
> nit: this last part isn't really related with MAX_DEFAULT_VALUE. It's a
> worthwhile test, but not necessarily in this test case. It might make more sense
> to set one of the constraints to FREQ_QOS_MAX_DEFAULT_VALUE again, and verify it
> doesn't have an impact.
>
> Just a comment, there's nothing really wrong with how it is.
>
>> +}
>> +
>> +/*
>> + * Test that a freq_qos_request can be added again after removal
>> + *
>> + * This issue was solved by commit 05ff1ba412fd ("PM: QoS: Invalidate frequency
>> + * QoS requests after removal")
>> + */
>> +static void freq_qos_test_readd(struct kunit *test)
>> +{
>> + struct freq_constraints qos;
>> + struct freq_qos_request req;
>> + int ret;
>> +
>> + freq_constraints_init(&qos);
>> + memset(&req, 0, sizeof(req));
>> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN),
>> + FREQ_QOS_MIN_DEFAULT_VALUE);
>
> nit: you could do this check once in a dedicated test and omit it
> in other tests to de-clutter
>
>> +
>> + /* Add */
>> + ret = freq_qos_add_request(&qos, &req, FREQ_QOS_MIN, 1000);
>> + KUNIT_EXPECT_EQ(test, ret, 1);
>> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN), 1000);
>
> similar here, this test validates re-adding, another dedicated test
> could verify once that the aggregate value is correct after adding a single
> request. Checking the return value still is sensible, just in case.
>
> I guess it can be argued either way, checking the values every time is
> extra-safe, omitting the checks reduces clutter and might help to make it
> clearer what the test really intends to verify.
The complaint of "too many assertions" is odd for an unit test; I just
wrote enough code to validate corectness without relying on a pile of
external shell scripts and DTS hacks.
If we had more tests then the constant checking of every single return
value might get tedious, but right now there are only 3 and their logic
is reasonably easy to follow.
> Anyway, my comments are just about possible improvements, it's also fine
> as is:
>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 4/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY
2019-11-26 21:01 ` Matthias Kaehlcke
@ 2019-11-29 5:55 ` Leonard Crestez
0 siblings, 0 replies; 13+ messages in thread
From: Leonard Crestez @ 2019-11-29 5:55 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Rafael J. Wysocki, Chanwoo Choi, Viresh Kumar, MyungJoo Ham,
Kyungmin Park, Artur Świgoń,
Angus Ainslie, Brendan Higgins, linux-kselftest, kunit-dev,
linux-pm, dl-linux-imx
On 26.11.2019 23:01, Matthias Kaehlcke wrote:
> On Tue, Nov 26, 2019 at 05:17:13PM +0200, Leonard Crestez wrote:
>> 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.
>>
>> This removes the "static" marking on freq_qos_apply but does not export
>> it for modules.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> ---
>> drivers/base/power/qos.c | 73 ++++++++++++++++++++++++++++++++++++----
>> include/linux/pm_qos.h | 12 +++++++
>> kernel/power/qos.c | 4 ++-
>> 3 files changed, 82 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/base/power/qos.c b/drivers/base/power/qos.c
>> index 350dcafd751f..8e93167f1783 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,24 @@ 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));
>> + }
>> +
>
> nit: these loops look a bit redundant, might be worth to encapsulate them in a
> macro.
There are only 2 identical loops though? Other requests to be cleared
are enumerated in slightly different ways.
A pm_qos_for_each_request macro could be added but it would only be used
in a handful of places.
>> 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 +342,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 +421,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 +550,18 @@ 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 +597,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 678fec6da5b9..19eafca5680e 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 FREQ_QOS_MAX_DEFAULT_VALUE
>> #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;
>> }
>> }
>> @@ -291,10 +301,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 a45cba7df0ae..83edf8698118 100644
>> --- a/kernel/power/qos.c
>> +++ b/kernel/power/qos.c
>> @@ -712,12 +712,14 @@ s32 freq_qos_read_value(struct freq_constraints *qos,
>> /**
>> * 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.
>> + *
>> + * This is only meant to be called from inside pm_qos, not drivers.
>> */
>> -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) {
>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Tested-by: Matthias Kaehlcke <mka@chromium.org>
Thanks for taking the time to look into this.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 0/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY
2019-11-26 15:17 [PATCH v4 0/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY Leonard Crestez
` (3 preceding siblings ...)
2019-11-26 15:17 ` [PATCH v4 4/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY Leonard Crestez
@ 2019-11-29 11:34 ` Rafael J. Wysocki
2019-11-29 14:20 ` Leonard Crestez
4 siblings, 1 reply; 13+ messages in thread
From: Rafael J. Wysocki @ 2019-11-29 11:34 UTC (permalink / raw)
To: Leonard Crestez
Cc: Matthias Kaehlcke, Chanwoo Choi, Viresh Kumar, MyungJoo Ham,
Kyungmin Park, Artur Świgoń,
Angus Ainslie, Brendan Higgins, linux-kselftest, kunit-dev,
linux-pm, linux-imx
On Tuesday, November 26, 2019 4:17:09 PM CET 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 on top of freq_qos.
>
> Discussion about removal is 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. Cpufreq could only use dev_pm_qos by
> implementing an additional layer of aggregation anyway.
>
> However in the devfreq subsystem scaling is always performed on a per-device
> basis so dev_pm_qos is a very good match. Support for dev_pm_qos in devfreq
> core is here (latest version, no dependencies outside this series):
>
> https://patchwork.kernel.org/cover/11252409/
>
> That series is RFC mostly because it needs these PM core patches.
> Earlier versions got entangled in some locking cleanups but those are
> not strictly necessary to get dev_pm_qos functionality.
>
> In theory if freq_qos is extended to handle conflicting min/max values then
> this sharing would be valuable. Right now freq_qos just ties two unrelated
> pm_qos aggregations for min and 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 and the whole approach of "removing requests" is somewhat dubios:
> request objects should be owned by consumers and the list of qos requests
> will most likely be empty when the target device is deleted. Series follows
> current pattern for dev_pm_qos.
>
> First two patches can be applied separately.
>
> Changes since v3:
> * Fix s/QOS/QoS in patch 2 title
> * Improves comments in kunit test
> * Fix assertions after freq_qos_remove_request
> * Remove (c) from NXP copyright header
> * Wrap long lines in qos.c to be under 80 chars. This fixes checkpatch but the
> rule is already broken by code in the files.
> * Collect reviews
> Link to v3: https://patchwork.kernel.org/cover/11260627/
>
> Changes since v2:
> * #define PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE FREQ_QOS_MAX_DEFAULT_VALUE
> * #define FREQ_QOS_MAX_DEFAULT_VALUE S32_MAX (in new patch)
> * Add initial kunit test for freq_qos, validating the MAX_DEFAULT_VALUE found
> by Matthias and another recent fix. Testing this should be easier!
> Link to v2: https://patchwork.kernel.org/cover/11250413/
>
> Changes since v1:
> * Don't rename or EXPORT_SYMBOL_GPL the freq_qos_apply function; just
> drop the static marker.
> Link to v1: https://patchwork.kernel.org/cover/11212887/
>
> Leonard Crestez (4):
> PM / QoS: Initial kunit test
> PM / QoS: Redefine FREQ_QOS_MAX_DEFAULT_VALUE to S32_MAX
> PM / QoS: Reorder pm_qos/freq_qos/dev_pm_qos structs
> PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY
>
> drivers/base/Kconfig | 4 ++
> drivers/base/power/Makefile | 1 +
> drivers/base/power/qos-test.c | 117 ++++++++++++++++++++++++++++++++++
> drivers/base/power/qos.c | 73 +++++++++++++++++++--
> include/linux/pm_qos.h | 86 ++++++++++++++-----------
> kernel/power/qos.c | 4 +-
> 6 files changed, 242 insertions(+), 43 deletions(-)
> create mode 100644 drivers/base/power/qos-test.c
>
>
I have applied the whole series as 5.5 material, but I have reordered the fix
(patch [2/4]) before the rest of it and marked it for -stable.
Thanks!
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 0/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY
2019-11-29 11:34 ` [PATCH v4 0/4] " Rafael J. Wysocki
@ 2019-11-29 14:20 ` Leonard Crestez
2019-12-02 1:24 ` Chanwoo Choi
0 siblings, 1 reply; 13+ messages in thread
From: Leonard Crestez @ 2019-11-29 14:20 UTC (permalink / raw)
To: Rafael J. Wysocki, Chanwoo Choi, MyungJoo Ham, Kyungmin Park
Cc: Matthias Kaehlcke, Viresh Kumar, Artur Świgoń,
Angus Ainslie, Brendan Higgins, linux-kselftest, kunit-dev,
linux-pm, dl-linux-imx
On 2019-11-29 1:34 PM, Rafael J. Wysocki wrote:
> On Tuesday, November 26, 2019 4:17:09 PM CET 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 on top of freq_qos.
>>
>> Discussion about removal is 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%7C4531c54354d54442d71808d774c02866%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637106240968746397&sdata=LYIRtXYe8qPz1G%2F0ADYpPhbhviv1pkk7%2B%2BQ1dX1DQR8%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. Cpufreq could only use dev_pm_qos by
>> implementing an additional layer of aggregation anyway.
>>
>> However in the devfreq subsystem scaling is always performed on a per-device
>> basis so dev_pm_qos is a very good match. Support for dev_pm_qos in devfreq
>> core is here (latest version, no dependencies outside this series):
>>
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11252409%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7C4531c54354d54442d71808d774c02866%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637106240968746397&sdata=YodRx0IRVsjQXYA5X7UEosn%2FatO%2BWREfotwWrley2DQ%3D&reserved=0
>>
>> That series is RFC mostly because it needs these PM core patches.
>> Earlier versions got entangled in some locking cleanups but those are
>> not strictly necessary to get dev_pm_qos functionality.
>>
>> In theory if freq_qos is extended to handle conflicting min/max values then
>> this sharing would be valuable. Right now freq_qos just ties two unrelated
>> pm_qos aggregations for min and 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 and the whole approach of "removing requests" is somewhat dubios:
>> request objects should be owned by consumers and the list of qos requests
>> will most likely be empty when the target device is deleted. Series follows
>> current pattern for dev_pm_qos.
>>
>> First two patches can be applied separately.
>>
>> Changes since v3:
>> * Fix s/QOS/QoS in patch 2 title
>> * Improves comments in kunit test
>> * Fix assertions after freq_qos_remove_request
>> * Remove (c) from NXP copyright header
>> * Wrap long lines in qos.c to be under 80 chars. This fixes checkpatch but the
>> rule is already broken by code in the files.
>> * Collect reviews
>> Link to v3: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11260627%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7C4531c54354d54442d71808d774c02866%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637106240968746397&sdata=e%2B5SGU%2Bx4UjxlY292ejMO1kDewc3MmFvCpf0SDB0K4U%3D&reserved=0
>>
>> Changes since v2:
>> * #define PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE FREQ_QOS_MAX_DEFAULT_VALUE
>> * #define FREQ_QOS_MAX_DEFAULT_VALUE S32_MAX (in new patch)
>> * Add initial kunit test for freq_qos, validating the MAX_DEFAULT_VALUE found
>> by Matthias and another recent fix. Testing this should be easier!
>> Link to v2: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11250413%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7C4531c54354d54442d71808d774c02866%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637106240968746397&sdata=vyz%2FN2x7OZPCSx4Md8yQkOSjPtfNUvW6%2FHtG0bTG1xU%3D&reserved=0
>>
>> Changes since v1:
>> * Don't rename or EXPORT_SYMBOL_GPL the freq_qos_apply function; just
>> drop the static marker.
>> Link to v1: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11212887%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7C4531c54354d54442d71808d774c02866%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637106240968746397&sdata=SjcSQmMRZu0z3ATWygBpD8mRToCZT%2FBgQ7U3IRpMUB0%3D&reserved=0
>>
>> Leonard Crestez (4):
>> PM / QoS: Initial kunit test
>> PM / QoS: Redefine FREQ_QOS_MAX_DEFAULT_VALUE to S32_MAX
>> PM / QoS: Reorder pm_qos/freq_qos/dev_pm_qos structs
>> PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY
>>
>> drivers/base/Kconfig | 4 ++
>> drivers/base/power/Makefile | 1 +
>> drivers/base/power/qos-test.c | 117 ++++++++++++++++++++++++++++++++++
>> drivers/base/power/qos.c | 73 +++++++++++++++++++--
>> include/linux/pm_qos.h | 86 ++++++++++++++-----------
>> kernel/power/qos.c | 4 +-
>> 6 files changed, 242 insertions(+), 43 deletions(-)
>> create mode 100644 drivers/base/power/qos-test.c
>>
>>
>
> I have applied the whole series as 5.5 material, but I have reordered the fix
> (patch [2/4]) before the rest of it and marked it for -stable.
Thanks!
Devfreq maintainers, do you think the devfreq parts could be queued for
5.5 as well? I'm not sure about the mechanics involved in this since
devfreq qos depends at build time on this dev_pm_qos series.
Latest version is here: https://patchwork.kernel.org/cover/11252415/
It's RFC because it didn't compile against unpatched linux-next but it's
otherwise a reduced version of a series that went through review and
testing.
--
Regards,
Leonard
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 0/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY
2019-11-29 14:20 ` Leonard Crestez
@ 2019-12-02 1:24 ` Chanwoo Choi
0 siblings, 0 replies; 13+ messages in thread
From: Chanwoo Choi @ 2019-12-02 1:24 UTC (permalink / raw)
To: Leonard Crestez, Rafael J. Wysocki, MyungJoo Ham, Kyungmin Park
Cc: Matthias Kaehlcke, Viresh Kumar, Artur Świgoń,
Angus Ainslie, Brendan Higgins, linux-kselftest, kunit-dev,
linux-pm, dl-linux-imx
On 11/29/19 11:20 PM, Leonard Crestez wrote:
> On 2019-11-29 1:34 PM, Rafael J. Wysocki wrote:
>> On Tuesday, November 26, 2019 4:17:09 PM CET 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 on top of freq_qos.
>>>
>>> Discussion about removal is 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%7C4531c54354d54442d71808d774c02866%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637106240968746397&sdata=LYIRtXYe8qPz1G%2F0ADYpPhbhviv1pkk7%2B%2BQ1dX1DQR8%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. Cpufreq could only use dev_pm_qos by
>>> implementing an additional layer of aggregation anyway.
>>>
>>> However in the devfreq subsystem scaling is always performed on a per-device
>>> basis so dev_pm_qos is a very good match. Support for dev_pm_qos in devfreq
>>> core is here (latest version, no dependencies outside this series):
>>>
>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11252409%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7C4531c54354d54442d71808d774c02866%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637106240968746397&sdata=YodRx0IRVsjQXYA5X7UEosn%2FatO%2BWREfotwWrley2DQ%3D&reserved=0
>>>
>>> That series is RFC mostly because it needs these PM core patches.
>>> Earlier versions got entangled in some locking cleanups but those are
>>> not strictly necessary to get dev_pm_qos functionality.
>>>
>>> In theory if freq_qos is extended to handle conflicting min/max values then
>>> this sharing would be valuable. Right now freq_qos just ties two unrelated
>>> pm_qos aggregations for min and 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 and the whole approach of "removing requests" is somewhat dubios:
>>> request objects should be owned by consumers and the list of qos requests
>>> will most likely be empty when the target device is deleted. Series follows
>>> current pattern for dev_pm_qos.
>>>
>>> First two patches can be applied separately.
>>>
>>> Changes since v3:
>>> * Fix s/QOS/QoS in patch 2 title
>>> * Improves comments in kunit test
>>> * Fix assertions after freq_qos_remove_request
>>> * Remove (c) from NXP copyright header
>>> * Wrap long lines in qos.c to be under 80 chars. This fixes checkpatch but the
>>> rule is already broken by code in the files.
>>> * Collect reviews
>>> Link to v3: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11260627%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7C4531c54354d54442d71808d774c02866%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637106240968746397&sdata=e%2B5SGU%2Bx4UjxlY292ejMO1kDewc3MmFvCpf0SDB0K4U%3D&reserved=0
>>>
>>> Changes since v2:
>>> * #define PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE FREQ_QOS_MAX_DEFAULT_VALUE
>>> * #define FREQ_QOS_MAX_DEFAULT_VALUE S32_MAX (in new patch)
>>> * Add initial kunit test for freq_qos, validating the MAX_DEFAULT_VALUE found
>>> by Matthias and another recent fix. Testing this should be easier!
>>> Link to v2: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11250413%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7C4531c54354d54442d71808d774c02866%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637106240968746397&sdata=vyz%2FN2x7OZPCSx4Md8yQkOSjPtfNUvW6%2FHtG0bTG1xU%3D&reserved=0
>>>
>>> Changes since v1:
>>> * Don't rename or EXPORT_SYMBOL_GPL the freq_qos_apply function; just
>>> drop the static marker.
>>> Link to v1: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fcover%2F11212887%2F&data=02%7C01%7Cleonard.crestez%40nxp.com%7C4531c54354d54442d71808d774c02866%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637106240968746397&sdata=SjcSQmMRZu0z3ATWygBpD8mRToCZT%2FBgQ7U3IRpMUB0%3D&reserved=0
>>>
>>> Leonard Crestez (4):
>>> PM / QoS: Initial kunit test
>>> PM / QoS: Redefine FREQ_QOS_MAX_DEFAULT_VALUE to S32_MAX
>>> PM / QoS: Reorder pm_qos/freq_qos/dev_pm_qos structs
>>> PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY
>>>
>>> drivers/base/Kconfig | 4 ++
>>> drivers/base/power/Makefile | 1 +
>>> drivers/base/power/qos-test.c | 117 ++++++++++++++++++++++++++++++++++
>>> drivers/base/power/qos.c | 73 +++++++++++++++++++--
>>> include/linux/pm_qos.h | 86 ++++++++++++++-----------
>>> kernel/power/qos.c | 4 +-
>>> 6 files changed, 242 insertions(+), 43 deletions(-)
>>> create mode 100644 drivers/base/power/qos-test.c
>>>
>>>
>>
>> I have applied the whole series as 5.5 material, but I have reordered the fix
>> (patch [2/4]) before the rest of it and marked it for -stable.
>
> Thanks!
>
> Devfreq maintainers, do you think the devfreq parts could be queued for
> 5.5 as well? I'm not sure about the mechanics involved in this since
> devfreq qos depends at build time on this dev_pm_qos series.
>
> Latest version is here: https://patchwork.kernel.org/cover/11252415/
Hi Leonard,
I agree devfreq's pm-qos patch.
[1] https://patchwork.kernel.org/cover/11252415/
But, I think need to discuss about this series[2].
Acutally, I don't want to split out the device_register.
[2]https://patchwork.kernel.org/cover/11242865/
>
> It's RFC because it didn't compile against unpatched linux-next but it's
> otherwise a reduced version of a series that went through review and
> testing.
>
> --
> Regards,
> Leonard
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/4] PM / QoS: Initial kunit test
2019-11-27 23:40 ` Leonard Crestez
@ 2019-12-02 16:49 ` Brendan Higgins
0 siblings, 0 replies; 13+ messages in thread
From: Brendan Higgins @ 2019-12-02 16:49 UTC (permalink / raw)
To: Leonard Crestez
Cc: Matthias Kaehlcke, Rafael J. Wysocki, Chanwoo Choi, Viresh Kumar,
MyungJoo Ham, Kyungmin Park, Artur Świgoń,
Angus Ainslie, linux-kselftest, kunit-dev, linux-pm,
dl-linux-imx
On Wed, Nov 27, 2019 at 3:40 PM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>
> On 26.11.2019 22:04, Matthias Kaehlcke wrote:
> > On Tue, Nov 26, 2019 at 05:17:10PM +0200, Leonard Crestez wrote:
> >> The pm_qos family of APIs are used in relatively difficult to reproduce
> >> scenarios such as thermal throttling so they benefit from unit testing.
> >>
> >> Start by adding basic tests from the the freq_qos APIs. It includes
> >> tests for issues that were brought up on mailing lists:
> >>
> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
Tested-by: Brendan Higgins <brendanhiggins@google.com>
> >> ---
> >> drivers/base/Kconfig | 4 ++
> >> drivers/base/power/Makefile | 1 +
> >> drivers/base/power/qos-test.c | 117 ++++++++++++++++++++++++++++++++++
> >> 3 files changed, 122 insertions(+)
> >> create mode 100644 drivers/base/power/qos-test.c
> >>
> >> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> >> index e37d37684132..d4ae1c1adf69 100644
> >> --- a/drivers/base/Kconfig
> >> +++ b/drivers/base/Kconfig
> >> @@ -155,10 +155,14 @@ config DEBUG_TEST_DRIVER_REMOVE
> >>
> >> This option is expected to find errors and may render your system
> >> unusable. You should say N here unless you are explicitly looking to
> >> test this functionality.
> >>
> >> +config PM_QOS_KUNIT_TEST
> >> + bool "KUnit Test for PM QoS features"
> >> + depends on KUNIT
> >> +
> >> config HMEM_REPORTING
> >> bool
> >> default n
> >> depends on NUMA
> >> help
> >> diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
> >> index ec5bb190b9d0..8fdd0073eeeb 100644
> >> --- a/drivers/base/power/Makefile
> >> +++ b/drivers/base/power/Makefile
> >> @@ -2,7 +2,8 @@
> >> obj-$(CONFIG_PM) += sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
> >> obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o wakeup_stats.o
> >> obj-$(CONFIG_PM_TRACE_RTC) += trace.o
> >> obj-$(CONFIG_PM_GENERIC_DOMAINS) += domain.o domain_governor.o
> >> obj-$(CONFIG_HAVE_CLK) += clock_ops.o
> >> +obj-$(CONFIG_PM_QOS_KUNIT_TEST) += qos-test.o
> >>
> >> ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
> >> diff --git a/drivers/base/power/qos-test.c b/drivers/base/power/qos-test.c
> >> new file mode 100644
> >> index 000000000000..3115db08d56b
> >> --- /dev/null
> >> +++ b/drivers/base/power/qos-test.c
> >> @@ -0,0 +1,117 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright 2019 NXP
> >> + */
> >> +#include <kunit/test.h>
> >> +#include <linux/pm_qos.h>
> >> +
> >> +/* Basic test for aggregating two "min" requests */
> >> +static void freq_qos_test_min(struct kunit *test)
> >> +{
> >> + struct freq_constraints qos;
> >> + struct freq_qos_request req1, req2;
> >> + int ret;
> >> +
> >> + freq_constraints_init(&qos);
> >> + memset(&req1, 0, sizeof(req1));
> >> + memset(&req2, 0, sizeof(req2));
> >> +
> >> + ret = freq_qos_add_request(&qos, &req1, FREQ_QOS_MIN, 1000);
> >> + KUNIT_EXPECT_EQ(test, ret, 1);
> >> + ret = freq_qos_add_request(&qos, &req2, FREQ_QOS_MIN, 2000);
> >> + KUNIT_EXPECT_EQ(test, ret, 1);
> >> +
> >> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN), 2000);
> >> +
> >> + ret = freq_qos_remove_request(&req2);
> >> + KUNIT_EXPECT_EQ(test, ret, 1);
> >> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN), 1000);
> >> +
> >> + ret = freq_qos_remove_request(&req1);
> >> + KUNIT_EXPECT_EQ(test, ret, 1);
> >> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN),
> >> + FREQ_QOS_MIN_DEFAULT_VALUE);
> >> +}
> >> +
> >> +/* Test that requests for MAX_DEFAULT_VALUE have no effect */
> >> +static void freq_qos_test_maxdef(struct kunit *test)
> >> +{
> >> + struct freq_constraints qos;
> >> + struct freq_qos_request req1, req2;
> >> + int ret;
> >> +
> >> + freq_constraints_init(&qos);
> >> + memset(&req1, 0, sizeof(req1));
> >> + memset(&req2, 0, sizeof(req2));
> >> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MAX),
> >> + FREQ_QOS_MAX_DEFAULT_VALUE);
> >> +
> >> + ret = freq_qos_add_request(&qos, &req1, FREQ_QOS_MAX,
> >> + FREQ_QOS_MAX_DEFAULT_VALUE);
> >> + KUNIT_EXPECT_EQ(test, ret, 0);
> >> + ret = freq_qos_add_request(&qos, &req2, FREQ_QOS_MAX,
> >> + FREQ_QOS_MAX_DEFAULT_VALUE);
> >> + KUNIT_EXPECT_EQ(test, ret, 0);
> >> +
> >> + /* Add max 1000 */
> >> + ret = freq_qos_update_request(&req1, 1000);
> >> + KUNIT_EXPECT_EQ(test, ret, 1);
> >> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MAX), 1000);
> >> +
> >> + /* Add max 2000, no impact */
> >> + ret = freq_qos_update_request(&req2, 2000);
> >> + KUNIT_EXPECT_EQ(test, ret, 0);
> >> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MAX), 1000);
> >> +
> >> + /* Remove max 1000, new max 2000 */
> >> + ret = freq_qos_remove_request(&req1);
> >> + KUNIT_EXPECT_EQ(test, ret, 1);
> >> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MAX), 2000);
> >
> > nit: this last part isn't really related with MAX_DEFAULT_VALUE. It's a
> > worthwhile test, but not necessarily in this test case. It might make more sense
> > to set one of the constraints to FREQ_QOS_MAX_DEFAULT_VALUE again, and verify it
> > doesn't have an impact.
> >
> > Just a comment, there's nothing really wrong with how it is.
> >
> >> +}
> >> +
> >> +/*
> >> + * Test that a freq_qos_request can be added again after removal
> >> + *
> >> + * This issue was solved by commit 05ff1ba412fd ("PM: QoS: Invalidate frequency
> >> + * QoS requests after removal")
> >> + */
> >> +static void freq_qos_test_readd(struct kunit *test)
> >> +{
> >> + struct freq_constraints qos;
> >> + struct freq_qos_request req;
> >> + int ret;
> >> +
> >> + freq_constraints_init(&qos);
> >> + memset(&req, 0, sizeof(req));
> >> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN),
> >> + FREQ_QOS_MIN_DEFAULT_VALUE);
> >
> > nit: you could do this check once in a dedicated test and omit it
> > in other tests to de-clutter
> >
> >> +
> >> + /* Add */
> >> + ret = freq_qos_add_request(&qos, &req, FREQ_QOS_MIN, 1000);
> >> + KUNIT_EXPECT_EQ(test, ret, 1);
> >> + KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN), 1000);
> >
> > similar here, this test validates re-adding, another dedicated test
> > could verify once that the aggregate value is correct after adding a single
> > request. Checking the return value still is sensible, just in case.
> >
> > I guess it can be argued either way, checking the values every time is
> > extra-safe, omitting the checks reduces clutter and might help to make it
> > clearer what the test really intends to verify.
>
> The complaint of "too many assertions" is odd for an unit test; I just
> wrote enough code to validate corectness without relying on a pile of
> external shell scripts and DTS hacks.
I think Matthias was just trying to say that it might be a little
cleaner if each test case only had expectations corresponding to the
particular property that the test case is asserting, which I agree
with.
I created the KUNIT_ASSERT_* variants just for this case; the idea is
that you ASSERT the preconditions for the test case and you EXPECT the
result you want. Hopefully this should make it immediately obvious
when examining a test case what assertions/expectations correspond to
the properties that the test is trying to prove and which are
prerequisite.
> If we had more tests then the constant checking of every single return
> value might get tedious, but right now there are only 3 and their logic
> is reasonably easy to follow.
I didn't have any trouble following your test. I agree with Matthias
that these are potential minor improvements, but I also think it is
fine as is.
> > Anyway, my comments are just about possible improvements, it's also fine
> > as is:
> >
> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Cheers!
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-12-02 16:49 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-26 15:17 [PATCH v4 0/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY Leonard Crestez
2019-11-26 15:17 ` [PATCH v4 1/4] PM / QoS: Initial kunit test Leonard Crestez
2019-11-26 20:04 ` Matthias Kaehlcke
2019-11-27 23:40 ` Leonard Crestez
2019-12-02 16:49 ` Brendan Higgins
2019-11-26 15:17 ` [PATCH v4 2/4] PM / QoS: Redefine FREQ_QOS_MAX_DEFAULT_VALUE to S32_MAX Leonard Crestez
2019-11-26 15:17 ` [PATCH v4 3/4] PM / QoS: Reorder pm_qos/freq_qos/dev_pm_qos structs Leonard Crestez
2019-11-26 15:17 ` [PATCH v4 4/4] PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY Leonard Crestez
2019-11-26 21:01 ` Matthias Kaehlcke
2019-11-29 5:55 ` Leonard Crestez
2019-11-29 11:34 ` [PATCH v4 0/4] " Rafael J. Wysocki
2019-11-29 14:20 ` Leonard Crestez
2019-12-02 1:24 ` Chanwoo Choi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).