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

[Guenter Roeck <linux@roeck-us.net>]

Hi Mark,

On 2/9/24 13:33, 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.
> 
> 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>

This is actually worse than v1 because hw_buf[6] isn't used anywhere.
Making sure that the values in the val[] array don't match the values
in hw_buf[6..7] doesn't add any value.

With this change, and the patch below applied on top of it:

      # raw_sync: EXPECTATION FAILED at drivers/base/regmap/regmap-kunit.c:1329
      Expected &hw_buf[2] != val, but
          &hw_buf[2] ==
           fb  16  cf  93
          val ==
           fb  16  cf  93
      # raw_sync: EXPECTATION FAILED at drivers/base/regmap/regmap-kunit.c:1330
      Expected &hw_buf[4] != val, but
          &hw_buf[4] ==
           fb  16
          val ==
           fb  16
          not ok 1 flat-little
      # raw_sync: EXPECTATION FAILED at drivers/base/regmap/regmap-kunit.c:1329

[ lots of those ]

FWIW, I had struggled with the re-use of val[0] for two different tests
(on hw_buf[2] and hw_buf[4]) myself. The only solution other than making sure
that it neither matches hw_buf[2] nor hw_buf[4] I came up with was to use a
separate variable for the accesses to hw_buf[4] (or hw_buf[6] in the old code).
Something like the second patch below, applied on top of your v2.

Guenter

---
diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c
index 85e02f86b14c..67cc3239f478 100644
--- a/drivers/base/regmap/regmap-kunit.c
+++ b/drivers/base/regmap/regmap-kunit.c
@@ -1284,6 +1284,14 @@ static void raw_sync(struct kunit *test)
         hw_buf = (u16 *)data->vals;

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

         /* Do a regular write and a raw write in cache only mode */
         regcache_cache_only(map, true);

---
diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c
index 85e02f86b14c..d2cf510f86f0 100644
--- a/drivers/base/regmap/regmap-kunit.c
+++ b/drivers/base/regmap/regmap-kunit.c
@@ -1269,7 +1269,7 @@ static void raw_sync(struct kunit *test)
         struct regmap *map;
         struct regmap_config config;
         struct regmap_ram_data *data;
-       u16 val[2];
+       u16 val[3];
         u16 *hw_buf;
         unsigned int rval;
         int i;
@@ -1283,17 +1283,17 @@ 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[2], &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)));
+       KUNIT_EXPECT_EQ(test, 0, regmap_raw_write(map, 2, val, sizeof(u16) * 2));
         if (config.val_format_endian == REGMAP_ENDIAN_BIG)
                 KUNIT_EXPECT_EQ(test, 0, regmap_write(map, 4,
-                                                     be16_to_cpu(val[0])));
+                                                     be16_to_cpu(val[2])));
         else
                 KUNIT_EXPECT_EQ(test, 0, regmap_write(map, 4,
-                                                     le16_to_cpu(val[0])));
+                                                     le16_to_cpu(val[2])));

         /* We should read back the new values, and defaults for the rest */
         for (i = 0; i < config.max_register + 1; i++) {
@@ -1305,10 +1305,10 @@ static void raw_sync(struct kunit *test)
                 case 4:
                         if (config.val_format_endian == REGMAP_ENDIAN_BIG) {
                                 KUNIT_EXPECT_EQ(test, rval,
-                                               be16_to_cpu(val[i % 2]));
+                                               be16_to_cpu(val[i - 2]));
                         } else {
                                 KUNIT_EXPECT_EQ(test, rval,
-                                               le16_to_cpu(val[i % 2]));
+                                               le16_to_cpu(val[i - 2]));
                         }
                         break;
                 default:
@@ -1318,8 +1318,8 @@ 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[4], val, sizeof(u16));
+       KUNIT_EXPECT_MEMNEQ(test, &hw_buf[2], val, sizeof(u16) * 2);
+       KUNIT_EXPECT_MEMNEQ(test, &hw_buf[4], &val[2], sizeof(u16));

         for (i = 0; i < config.max_register + 1; i++)
                 data->written[i] = false;
@@ -1331,7 +1331,6 @@ 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[4], val, sizeof(u16));

         regmap_exit(map);
  }