| Message ID | be196b656bb5fbf2c59a179e6453aa963b862109.1574781196.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | PM / QoS: Restore DEV_PM_QOS_MIN/MAX_FREQUENCY | expand |
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>
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>
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!
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);
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