[v3] regmap: kunit: Ensure that changed bytes are actually different
Commit Message
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, there
are other tests which should have similar verification that we actually
changed something.
While we're at it refactor the test to use three changed values rather
than attempting to use one of them twice, that just complicates checking
that our new values are actually new.
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 v3:
- Use 3 separate test values rather than attempting to reuse one of them
for raw and regular writes.
- Link to v2: https://lore.kernel.org/r/20240209-regmap-kunit-random-change-v2-1-be0a447c2891@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 | 54 +++++++++++++++++++++++++++-----------
1 file changed, 38 insertions(+), 16 deletions(-)
---
base-commit: 3b201c9af7c0cad2e8311d96c0c1b399606c70fa
change-id: 20240209-regmap-kunit-random-change-178bd19b91b5
Best regards,
Comments
On Sun, Feb 11, 2024 at 04:58:17PM +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, there
> are other tests which should have similar verification that we actually
> changed something.
>
> While we're at it refactor the test to use three changed values rather
> than attempting to use one of them twice, that just complicates checking
> that our new values are actually new.
>
> 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>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Minor comment below.
> +
> + /*
> + * The value written via _write() was translated by the core,
> + * translate the original copy for comparison purposes.
> + */
> + if (config.val_format_endian == REGMAP_ENDIAN_BIG)
> + val[2] = cpu_to_be16(val[2]);
> + else
> + val[2] = cpu_to_le16(val[2]);
>
> /* 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[2], &val[0], sizeof(val));
I kept those two checks separate on purpose because a non-equal check
will "succeed" if a single byte is different. That means the above check
will "pass" if one of regmap_raw_write() or regmap_write() works but
the other is broken.
Thanks,
Guenter
On Sun, Feb 11, 2024 at 10:54:25AM -0800, Guenter Roeck wrote:
> On Sun, Feb 11, 2024 at 04:58:17PM +0000, Mark Brown wrote:
> > - KUNIT_EXPECT_MEMNEQ(test, &hw_buf[6], val, sizeof(u16));
> > + KUNIT_EXPECT_MEMNEQ(test, &hw_buf[2], &val[0], sizeof(val));
> I kept those two checks separate on purpose because a non-equal check
> will "succeed" if a single byte is different. That means the above check
> will "pass" if one of regmap_raw_write() or regmap_write() works but
> the other is broken.
Good point, I'll do an incremental change for that though since that's
more of a belt and braces thing for something that should be checked
elsewhere than a core part of the test here.
On Sun, 11 Feb 2024 16:58:17 +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, there
> are other tests which should have similar verification that we actually
> changed something.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next
Thanks!
[1/1] regmap: kunit: Ensure that changed bytes are actually different
commit: 2f0dbb24f78a333433a2b875c0b76bf55c119cd4
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
@@ -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,
@@ -1251,7 +1268,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;
@@ -1265,17 +1282,13 @@ static void raw_sync(struct kunit *test)
hw_buf = (u16 *)data->vals;
- get_random_bytes(&val, 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)));
- if (config.val_format_endian == REGMAP_ENDIAN_BIG)
- KUNIT_EXPECT_EQ(test, 0, regmap_write(map, 6,
- be16_to_cpu(val[0])));
- else
- KUNIT_EXPECT_EQ(test, 0, regmap_write(map, 6,
- le16_to_cpu(val[0])));
+ KUNIT_EXPECT_EQ(test, 0, regmap_raw_write(map, 2, val,
+ sizeof(u16) * 2));
+ KUNIT_EXPECT_EQ(test, 0, regmap_write(map, 4, val[2]));
/* We should read back the new values, and defaults for the rest */
for (i = 0; i < config.max_register + 1; i++) {
@@ -1284,24 +1297,34 @@ static void raw_sync(struct kunit *test)
switch (i) {
case 2:
case 3:
- case 6:
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;
+ case 4:
+ KUNIT_EXPECT_EQ(test, rval, val[i - 2]);
+ break;
default:
KUNIT_EXPECT_EQ(test, config.reg_defaults[i].def, rval);
break;
}
}
+
+ /*
+ * The value written via _write() was translated by the core,
+ * translate the original copy for comparison purposes.
+ */
+ if (config.val_format_endian == REGMAP_ENDIAN_BIG)
+ val[2] = cpu_to_be16(val[2]);
+ else
+ val[2] = cpu_to_le16(val[2]);
/* 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[2], &val[0], sizeof(val));
for (i = 0; i < config.max_register + 1; i++)
data->written[i] = false;
@@ -1312,8 +1335,7 @@ static void raw_sync(struct kunit *test)
KUNIT_EXPECT_EQ(test, 0, regcache_sync(map));
/* 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[2], &val[0], sizeof(val));
regmap_exit(map);
}