[PATCH] regmap: kunit: Ensure that changed bytes are actually different

[PATCH] regmap: kunit: Ensure that changed bytes are actually different

[Mark Brown]

During the cache sync test we verify that values we expect to have been
written only to the cache do not appear in the hardware. This works most
of the time but since we randomly generate both the original and new values
there is a low probability that these values may actually be the same.
Wrap get_random_bytes() to ensure that the values are different, it is
likely we will want a similar pattern for other tests in the future.

We use random generation to try to avoid data dependencies in the tests.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/regmap/regmap-kunit.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c
index 026bdcb45127..59a672e2fb01 100644
--- a/drivers/base/regmap/regmap-kunit.c
+++ b/drivers/base/regmap/regmap-kunit.c
@@ -9,6 +9,23 @@
 
 #define BLOCK_TEST_SIZE 12
 
+static void get_changed_bytes(void *orig, void *new, size_t size)
+{
+	char *o = orig;
+	char *n = new;
+	int i;
+
+	get_random_bytes(new, size);
+
+	/*
+	 * This could be nicer and more efficient but we shouldn't
+	 * super care.
+	 */
+	for (i = 0; i < size; i++)
+		while (n[i] == o[i])
+			get_random_bytes(&n[i], 1);
+}
+
 static const struct regmap_config test_regmap_config = {
 	.max_register = BLOCK_TEST_SIZE,
 	.reg_stride = 1,
@@ -1265,16 +1282,16 @@ static void raw_sync(struct kunit *test)
 
 	hw_buf = (u16 *)data->vals;
 
-	get_random_bytes(&val, sizeof(val));
+	get_changed_bytes(&hw_buf[6], &val[0], sizeof(val));
 
 	/* Do a regular write and a raw write in cache only mode */
 	regcache_cache_only(map, true);
 	KUNIT_EXPECT_EQ(test, 0, regmap_raw_write(map, 2, val, sizeof(val)));
 	if (config.val_format_endian == REGMAP_ENDIAN_BIG)
-		KUNIT_EXPECT_EQ(test, 0, regmap_write(map, 6,
+		KUNIT_EXPECT_EQ(test, 0, regmap_write(map, 4,
 						      be16_to_cpu(val[0])));
 	else
-		KUNIT_EXPECT_EQ(test, 0, regmap_write(map, 6,
+		KUNIT_EXPECT_EQ(test, 0, regmap_write(map, 4,
 						      le16_to_cpu(val[0])));
 
 	/* We should read back the new values, and defaults for the rest */
@@ -1284,7 +1301,7 @@ static void raw_sync(struct kunit *test)
 		switch (i) {
 		case 2:
 		case 3:
-		case 6:
+		case 4:
 			if (config.val_format_endian == REGMAP_ENDIAN_BIG) {
 				KUNIT_EXPECT_EQ(test, rval,
 						be16_to_cpu(val[i % 2]));
@@ -1301,7 +1318,7 @@ static void raw_sync(struct kunit *test)
 	
 	/* The values should not appear in the "hardware" */
 	KUNIT_EXPECT_MEMNEQ(test, &hw_buf[2], val, sizeof(val));
-	KUNIT_EXPECT_MEMNEQ(test, &hw_buf[6], val, sizeof(u16));
+	KUNIT_EXPECT_MEMNEQ(test, &hw_buf[4], val, sizeof(u16));
 
 	for (i = 0; i < config.max_register + 1; i++)
 		data->written[i] = false;

---
base-commit: 3b201c9af7c0cad2e8311d96c0c1b399606c70fa
change-id: 20240209-regmap-kunit-random-change-178bd19b91b5

Best regards,
-- 
Mark Brown <broonie@kernel.org>

Re: [PATCH] regmap: kunit: Ensure that changed bytes are actually different

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 568 bytes --]

On Fri, Feb 09, 2024 at 08:02:27PM +0000, Mark Brown wrote:
> During the cache sync test we verify that values we expect to have been
> written only to the cache do not appear in the hardware. This works most
> of the time but since we randomly generate both the original and new values
> there is a low probability that these values may actually be the same.
> Wrap get_random_bytes() to ensure that the values are different, it is
> likely we will want a similar pattern for other tests in the future.

Sorry, works better if you actally check stuff in.  v2 coming.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regmap: kunit: Ensure that changed bytes are actually different

[Guenter Roeck]

Hi Mark,

On 2/9/24 12:05, Mark Brown wrote:
> On Fri, Feb 09, 2024 at 08:02:27PM +0000, Mark Brown wrote:
>> During the cache sync test we verify that values we expect to have been
>> written only to the cache do not appear in the hardware. This works most
>> of the time but since we randomly generate both the original and new values
>> there is a low probability that these values may actually be the same.
>> Wrap get_random_bytes() to ensure that the values are different, it is
>> likely we will want a similar pattern for other tests in the future.
> 
> Sorry, works better if you actally check stuff in.  v2 coming.


I don't know how v2 looks like (I think some of the hw_buf index values are
wrong), but have a look at the diff below.

Guenter

---
diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c
index 6fe259a2a73f..e73bdf01f118 100644
--- a/drivers/base/regmap/regmap-kunit.c
+++ b/drivers/base/regmap/regmap-kunit.c
@@ -1283,7 +1283,13 @@ static void raw_sync(struct kunit *test)

         hw_buf = (u16 *)data->vals;

-       get_changed_bytes(&hw_buf[6], &val[0], sizeof(val));
+       get_changed_bytes(&hw_buf[4], &val[0], sizeof(val));
+       // Let's cheat.
+       // Remember, the above code doesn't look into hw_buf[2..3],
+       // so anything might be in there, including the values from
+       // the val[] array.
+       hw_buf[2] = val[0];
+       hw_buf[3] = val[1];

         /* Do a regular write and a raw write in cache only mode */
         regcache_cache_only(map, true);
@@ -1331,7 +1337,7 @@ static void raw_sync(struct kunit *test)

         /* The values should now appear in the "hardware" */
         KUNIT_EXPECT_MEMEQ(test, &hw_buf[2], val, sizeof(val));
-       KUNIT_EXPECT_MEMEQ(test, &hw_buf[6], val, sizeof(u16));
+       KUNIT_EXPECT_MEMEQ(test, &hw_buf[4], val, sizeof(u16));

         regmap_exit(map);
  }

Re: [PATCH] regmap: kunit: Ensure that changed bytes are actually different

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 458 bytes --]

On Fri, Feb 09, 2024 at 01:25:36PM -0800, Guenter Roeck wrote:

> +       get_changed_bytes(&hw_buf[4], &val[0], sizeof(val));
> +       // Let's cheat.
> +       // Remember, the above code doesn't look into hw_buf[2..3],
> +       // so anything might be in there, including the values from
> +       // the val[] array.
> +       hw_buf[2] = val[0];
> +       hw_buf[3] = val[1];

Ugh, yeah - I'm paying attention to too many things at once today I
fear.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]