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

[Guenter Roeck]

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@xxxxxxxxxxxx>
> Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>

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);
 }