Re: [PATCH v3 1/4] PM / QoS: Initial kunit test

[Matthias Kaehlcke <mka@chromium.org>]

On Mon, Nov 25, 2019 at 06:42:16PM +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.

indeed, a unit test is useful in this case!

> 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 | 116 ++++++++++++++++++++++++++++++++++
>  3 files changed, 121 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..8267d91332a8
> --- /dev/null
> +++ b/drivers/base/power/qos-test.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 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);
> +
> +	freq_qos_remove_request(&req2);
> +	KUNIT_EXPECT_EQ(test, ret, 1);

This checks (again) the return value of the above freq_qos_add_request() call,
which I suppose is not intended. Remove?

> +	KUNIT_EXPECT_EQ(test, freq_qos_read_value(&qos, FREQ_QOS_MIN), 1000);
> +
> +	freq_qos_remove_request(&req1);
> +	KUNIT_EXPECT_EQ(test, ret, 1);

ditto

> +	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 2000, new max 1000 */

the code doesn't match the comment, max 1000 is removed

> +	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 readded after removal

nit: 're-added'. It took me a few secs to figure this is not a about
'read'ing something