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

  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>
---
Changes in v2:
- Check the whole change in.
- Link to v1: https://lore.kernel.org/r/20240209-regmap-kunit-random-change-v1-1-ad2d76757583@kernel.org
---
 drivers/base/regmap/regmap-kunit.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)


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

Best regards,
  

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

On Fri, Feb 09, 2024 at 02:07:38PM -0800, Guenter Roeck wrote:

> 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.

Yeah, I realised after reading your earlier mail.  It's passing for me
somehow.

> 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).
Indeed, it was fine with the old code due to not caring about having
different values but we need to generate three values now.

>         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];

I don't understand how this interacts with the pre-sync check?
  

On 2/9/24 14:39, Mark Brown wrote:
> On Fri, Feb 09, 2024 at 02:07:38PM -0800, Guenter Roeck wrote:
> 
>> 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.
> 
> Yeah, I realised after reading your earlier mail.  It's passing for me
> somehow.
> 

It is passing because the probability for it to fail is really low.
It only fails reliably with the cheat below.

>> 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).
> Indeed, it was fine with the old code due to not caring about having
> different values but we need to generate three values now.
> 
>>          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];
> 
> I don't understand how this interacts with the pre-sync check?

That is because the test expects the memory in hw_buf[] and val[] to be
different (because hw_buf[] wasn't updated to the new values). The above
cheat forces hw_buf[2,3] and val[0,1] to be the same, so the pre-sync test
fails. The post-sync test passes because the values are expected to be
the same at that time.

Guenter
  

On Fri, Feb 09, 2024 at 03:20:40PM -0800, Guenter Roeck wrote:
> On 2/9/24 14:39, Mark Brown wrote:

> > > +       // 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];

> > I don't understand how this interacts with the pre-sync check?

> That is because the test expects the memory in hw_buf[] and val[] to be
> different (because hw_buf[] wasn't updated to the new values). The above
> cheat forces hw_buf[2,3] and val[0,1] to be the same, so the pre-sync test
> fails. The post-sync test passes because the values are expected to be
> the same at that time.

Ah, that was my read - I was thinking I was missing some way for both
tests to pass.