On Wed, Nov 27, 2019 at 3:40 PM Leonard Crestez <leonard.crestez@xxxxxxx> 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@xxxxxxx>
Reviewed-by: Brendan Higgins <brendanhiggins@xxxxxxxxxx>
Tested-by: Brendan Higgins <brendanhiggins@xxxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxx>
Cheers!
