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

[Leonard Crestez <leonard.crestez@nxp.com>]

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>