[PATCH 0/3] regmap: Add KUnit tests for read/write checking

[PATCH 0/3] regmap: Add KUnit tests for read/write checking

[Mark Brown]

Since Takashi found an issue with maple tree syncing registers it
shouldn't do add some test cases that catch that case and some more
potential issues, ideally we'd run through the combination of
readability with all possible I/O calls but that's lifting for another
day.  We did find one issue with missing readability checks which will 
be fixed separately.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
Mark Brown (3):
      regmap: Add test that writes to write only registers are prevented
      regmap: Add a test case for write only registers
      regmap: Add test to make sure we don't sync to read only registers

 drivers/base/regmap/regmap-kunit.c | 115 +++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)
---
base-commit: ecfb8ce26d02ec136b4e1ae8c5a77d22d335057b
change-id: 20230613-regmap-kunit-read-write-024393cdbef0

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

[PATCH 1/3] regmap: Add test that writes to write only registers are prevented

[Mark Brown]

We should have error checking that verifies that writes to write only
registers are suppressed, verify that this happens as it should.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/regmap/regmap-kunit.c | 41 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c
index 4ec5cbfc0ca0..f5c175022a47 100644
--- a/drivers/base/regmap/regmap-kunit.c
+++ b/drivers/base/regmap/regmap-kunit.c
@@ -92,6 +92,11 @@ static struct regmap *gen_regmap(struct regmap_config *config,
 	return ret;
 }
 
+static bool reg_5_false(struct device *context, unsigned int reg)
+{
+	return reg != 5;
+}
+
 static void basic_read_write(struct kunit *test)
 {
 	struct regcache_types *t = (struct regcache_types *)test->param_value;
@@ -191,6 +196,41 @@ static void bulk_read(struct kunit *test)
 	regmap_exit(map);
 }
 
+static void write_readonly(struct kunit *test)
+{
+	struct regcache_types *t = (struct regcache_types *)test->param_value;
+	struct regmap *map;
+	struct regmap_config config;
+	struct regmap_ram_data *data;
+	unsigned int val;
+	int i;
+
+	config = test_regmap_config;
+	config.cache_type = t->type;
+	config.num_reg_defaults = BLOCK_TEST_SIZE;
+	config.writeable_reg = reg_5_false;
+
+	map = gen_regmap(&config, &data);
+	KUNIT_ASSERT_FALSE(test, IS_ERR(map));
+	if (IS_ERR(map))
+		return;
+
+	get_random_bytes(&val, sizeof(val));
+
+	for (i = 0; i < BLOCK_TEST_SIZE; i++)
+		data->written[i] = false;
+
+	/* Change the value of all registers, readonly should fail */
+	for (i = 0; i < BLOCK_TEST_SIZE; i++)
+		KUNIT_EXPECT_EQ(test, i != 5, regmap_write(map, i, val) == 0);
+
+	/* Did that match what we see on the device? */
+	for (i = 0; i < BLOCK_TEST_SIZE; i++)
+		KUNIT_EXPECT_EQ(test, i != 5, data->written[i]);
+
+	regmap_exit(map);
+}
+
 static void reg_defaults(struct kunit *test)
 {
 	struct regcache_types *t = (struct regcache_types *)test->param_value;
@@ -1037,6 +1077,7 @@ static struct kunit_case regmap_test_cases[] = {
 	KUNIT_CASE_PARAM(basic_read_write, regcache_types_gen_params),
 	KUNIT_CASE_PARAM(bulk_write, regcache_types_gen_params),
 	KUNIT_CASE_PARAM(bulk_read, regcache_types_gen_params),
+	KUNIT_CASE_PARAM(write_readonly, regcache_types_gen_params),
 	KUNIT_CASE_PARAM(reg_defaults, regcache_types_gen_params),
 	KUNIT_CASE_PARAM(reg_defaults_read_dev, regcache_types_gen_params),
 	KUNIT_CASE_PARAM(register_patch, regcache_types_gen_params),

-- 
2.30.2

[PATCH 2/3] regmap: Add a test case for write only registers

[Mark Brown]

Validate that attempts to read from write only registers fail and don't
somehow trigger spurious hardware accesses.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/regmap/regmap-kunit.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c
index f5c175022a47..a1a1a9ce2e13 100644
--- a/drivers/base/regmap/regmap-kunit.c
+++ b/drivers/base/regmap/regmap-kunit.c
@@ -231,6 +231,37 @@ static void write_readonly(struct kunit *test)
 	regmap_exit(map);
 }
 
+static void read_writeonly(struct kunit *test)
+{
+	struct regcache_types *t = (struct regcache_types *)test->param_value;
+	struct regmap *map;
+	struct regmap_config config;
+	struct regmap_ram_data *data;
+	unsigned int val;
+	int i;
+
+	config = test_regmap_config;
+	config.cache_type = t->type;
+	config.readable_reg = reg_5_false;
+
+	map = gen_regmap(&config, &data);
+	KUNIT_ASSERT_FALSE(test, IS_ERR(map));
+	if (IS_ERR(map))
+		return;
+
+	for (i = 0; i < BLOCK_TEST_SIZE; i++)
+		data->read[i] = false;
+
+	/* Try to read all the registers, the writeonly one should fail */
+	for (i = 0; i < BLOCK_TEST_SIZE; i++)
+		KUNIT_EXPECT_EQ(test, i != 5, regmap_read(map, i, &val) == 0);
+
+	/* Did we trigger a hardware access? */
+	KUNIT_EXPECT_FALSE(test, data->read[5]);
+
+	regmap_exit(map);
+}
+
 static void reg_defaults(struct kunit *test)
 {
 	struct regcache_types *t = (struct regcache_types *)test->param_value;
@@ -1078,6 +1109,7 @@ static struct kunit_case regmap_test_cases[] = {
 	KUNIT_CASE_PARAM(bulk_write, regcache_types_gen_params),
 	KUNIT_CASE_PARAM(bulk_read, regcache_types_gen_params),
 	KUNIT_CASE_PARAM(write_readonly, regcache_types_gen_params),
+	KUNIT_CASE_PARAM(read_writeonly, regcache_types_gen_params),
 	KUNIT_CASE_PARAM(reg_defaults, regcache_types_gen_params),
 	KUNIT_CASE_PARAM(reg_defaults_read_dev, regcache_types_gen_params),
 	KUNIT_CASE_PARAM(register_patch, regcache_types_gen_params),

-- 
2.30.2

[PATCH 3/3] regmap: Add test to make sure we don't sync to read only registers

[Mark Brown]

Ensure that a read only value in the register cache does not result in a
write during regcache_sync().

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/regmap/regmap-kunit.c | 42 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c
index a1a1a9ce2e13..6f444ac0ec49 100644
--- a/drivers/base/regmap/regmap-kunit.c
+++ b/drivers/base/regmap/regmap-kunit.c
@@ -680,6 +680,47 @@ static void cache_sync_defaults(struct kunit *test)
 	regmap_exit(map);
 }
 
+static void cache_sync_readonly(struct kunit *test)
+{
+	struct regcache_types *t = (struct regcache_types *)test->param_value;
+	struct regmap *map;
+	struct regmap_config config;
+	struct regmap_ram_data *data;
+	unsigned int val;
+	int i;
+
+	config = test_regmap_config;
+	config.cache_type = t->type;
+	config.writeable_reg = reg_5_false;
+
+	map = gen_regmap(&config, &data);
+	KUNIT_ASSERT_FALSE(test, IS_ERR(map));
+	if (IS_ERR(map))
+		return;
+
+	/* Read all registers to fill the cache */
+	for (i = 0; i < BLOCK_TEST_SIZE; i++)
+		KUNIT_EXPECT_EQ(test, 0, regmap_read(map, i, &val));
+
+	/* Change the value of all registers, readonly should fail */
+	get_random_bytes(&val, sizeof(val));
+	regcache_cache_only(map, true);
+	for (i = 0; i < BLOCK_TEST_SIZE; i++)
+		KUNIT_EXPECT_EQ(test, i != 5, regmap_write(map, i, val) == 0);
+	regcache_cache_only(map, false);
+
+	/* Resync */
+	for (i = 0; i < BLOCK_TEST_SIZE; i++)
+		data->written[i] = false;
+	KUNIT_EXPECT_EQ(test, 0, regcache_sync(map));
+
+	/* Did that match what we see on the device? */
+	for (i = 0; i < BLOCK_TEST_SIZE; i++)
+		KUNIT_EXPECT_EQ(test, i != 5, data->written[i]);
+
+	regmap_exit(map);
+}
+
 static void cache_sync_patch(struct kunit *test)
 {
 	struct regcache_types *t = (struct regcache_types *)test->param_value;
@@ -1119,6 +1160,7 @@ static struct kunit_case regmap_test_cases[] = {
 	KUNIT_CASE_PARAM(cache_bypass, real_cache_types_gen_params),
 	KUNIT_CASE_PARAM(cache_sync, real_cache_types_gen_params),
 	KUNIT_CASE_PARAM(cache_sync_defaults, real_cache_types_gen_params),
+	KUNIT_CASE_PARAM(cache_sync_readonly, real_cache_types_gen_params),
 	KUNIT_CASE_PARAM(cache_sync_patch, real_cache_types_gen_params),
 	KUNIT_CASE_PARAM(cache_drop, sparse_cache_types_gen_params),
 

-- 
2.30.2

Re: [PATCH 0/3] regmap: Add KUnit tests for read/write checking

[Mark Brown]

On Tue, 13 Jun 2023 21:17:41 +0100, Mark Brown wrote:
> Since Takashi found an issue with maple tree syncing registers it
> shouldn't do add some test cases that catch that case and some more
> potential issues, ideally we'd run through the combination of
> readability with all possible I/O calls but that's lifting for another
> day.  We did find one issue with missing readability checks which will
> be fixed separately.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Thanks!

[1/3] regmap: Add test that writes to write only registers are prevented
      commit: 180033061e203a7c5510e7c38bccd885657de517
[2/3] regmap: Add a test case for write only registers
      commit: a07bff4054c9e2b3a5c5790312a4a45aaeca7afe
[3/3] regmap: Add test to make sure we don't sync to read only registers
      commit: 357a1ebd0c012f5421252547ebf4ee619b45733d

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