The flat regcache will always assume the data in the cache is valid.
Since the cache takes priority over hardware values, this may shadow the
actual state of the device. This is not the case with REGCACHE_RBTREE
for example, which makes these implementation behave differently.

Add a new containing cache structure with the flat data table and a
bitmap indicating cache validity. Use this to provide an implementation
for regcache_ops.drop.

Since this makes the flat cache behave more like other caches, a special
case in the read_writeonly unit test can also be dropped. This reverts
commit d0c99ffe2126 ("regmap: Allow reads from write only registers with
the flat cache"). Furthermore we can now add REGCACHE_FLAG to the table
of sparse_cache tests.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
Changes since v2:
Link: https://lore.kernel.org/all/20250109151106.38645-1-sander@svanheule.net/
- Complete renaming of index variables so regcache-flat.c compiles again

Changes since v1:
Link: https://lore.kernel.org/all/20241231100256.194753-1-sander@svanheule.net/
- Fix off-by-one in length for bitmap_clear()
- Add REGCACHE_FLAT to the list of sparse cache tests
---
 drivers/base/regmap/regcache-flat.c | 70 ++++++++++++++++++++++++-----
 drivers/base/regmap/regmap-kunit.c  | 21 ++++-----
 2 files changed, 69 insertions(+), 22 deletions(-)

diff --git a/drivers/base/regmap/regcache-flat.c b/drivers/base/regmap/regcache-flat.c
index f36d3618b67c..a5b9b2b15d3d 100644
--- a/drivers/base/regmap/regcache-flat.c
+++ b/drivers/base/regmap/regcache-flat.c
@@ -6,6 +6,8 @@
 //
 // Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
 
+#include <linux/bitmap.h>
+#include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
@@ -18,34 +20,65 @@ static inline unsigned int regcache_flat_get_index(const struct regmap *map,
 	return regcache_get_index_by_order(map, reg);
 }
 
+struct regcache_flat_data {
+	unsigned int *data;
+	unsigned long *valid;
+};
+
 static int regcache_flat_init(struct regmap *map)
 {
 	int i;
-	unsigned int *cache;
+	unsigned int cache_size;
+	struct regcache_flat_data *cache = NULL;
+	unsigned long *cache_valid = NULL;
+	unsigned int *cache_data = NULL;
 
 	if (!map || map->reg_stride_order < 0 || !map->max_register_is_set)
 		return -EINVAL;
 
-	map->cache = kcalloc(regcache_flat_get_index(map, map->max_register)
-			     + 1, sizeof(unsigned int), map->alloc_flags);
-	if (!map->cache)
+	cache_size = regcache_flat_get_index(map, map->max_register) + 1;
+	cache_data = kcalloc(cache_size, sizeof(unsigned int), map->alloc_flags);
+	if (!cache_data)
 		return -ENOMEM;
 
+	cache_valid = bitmap_zalloc(cache_size, map->alloc_flags);
+	if (!cache_valid)
+		goto err_free_valid;
+
+	map->cache = kmalloc(sizeof(*cache), map->alloc_flags);
+	if (!map->cache)
+		goto err_free;
+
 	cache = map->cache;
+	cache->valid = cache_valid;
+	cache->data = cache_data;
 
 	for (i = 0; i < map->num_reg_defaults; i++) {
 		unsigned int reg = map->reg_defaults[i].reg;
 		unsigned int index = regcache_flat_get_index(map, reg);
 
-		cache[index] = map->reg_defaults[i].def;
+		cache->data[index] = map->reg_defaults[i].def;
+		__set_bit(index, cache->valid);
 	}
 
 	return 0;
+
+err_free:
+	kfree(cache_data);
+err_free_valid:
+	bitmap_free(cache_valid);
+	return -ENOMEM;
 }
 
 static int regcache_flat_exit(struct regmap *map)
 {
-	kfree(map->cache);
+	struct regcache_flat_data *cache = map->cache;
+
+	if (cache) {
+		bitmap_free(cache->valid);
+		kfree(cache->data);
+	}
+	kfree(cache);
 	map->cache = NULL;
 
 	return 0;
@@ -54,10 +87,13 @@ static int regcache_flat_exit(struct regmap *map)
 static int regcache_flat_read(struct regmap *map,
 			      unsigned int reg, unsigned int *value)
 {
-	unsigned int *cache = map->cache;
+	struct regcache_flat_data *cache = map->cache;
 	unsigned int index = regcache_flat_get_index(map, reg);
 
-	*value = cache[index];
+	if (!test_bit(index, cache->valid))
+		return -ENOENT;
+
+	*value = cache->data[index];
 
 	return 0;
 }
@@ -65,10 +101,23 @@ static int regcache_flat_read(struct regmap *map,
 static int regcache_flat_write(struct regmap *map, unsigned int reg,
 			       unsigned int value)
 {
-	unsigned int *cache = map->cache;
+	struct regcache_flat_data *cache = map->cache;
 	unsigned int index = regcache_flat_get_index(map, reg);
 
-	cache[index] = value;
+	cache->data[index] = value;
+	__set_bit(index, cache->valid);
+
+	return 0;
+}
+
+static int regcache_flat_drop(struct regmap *map, unsigned int min,
+			      unsigned int max)
+{
+	struct regcache_flat_data *cache = map->cache;
+	unsigned int bitmap_min = regcache_flat_get_index(map, min);
+	unsigned int bitmap_max = regcache_flat_get_index(map, max);
+
+	bitmap_clear(cache->valid, bitmap_min, bitmap_max + 1 - bitmap_min);
 
 	return 0;
 }
@@ -80,4 +129,5 @@ struct regcache_ops regcache_flat_ops = {
 	.exit = regcache_flat_exit,
 	.read = regcache_flat_read,
 	.write = regcache_flat_write,
+	.drop = regcache_flat_drop,
 };
diff --git a/drivers/base/regmap/regmap-kunit.c b/drivers/base/regmap/regmap-kunit.c
index 64ea340950b6..c9ab1e767505 100644
--- a/drivers/base/regmap/regmap-kunit.c
+++ b/drivers/base/regmap/regmap-kunit.c
@@ -136,6 +136,12 @@ static const struct regmap_test_param real_cache_types_list[] = {
 KUNIT_ARRAY_PARAM(real_cache_types, real_cache_types_list, param_to_desc);
 
 static const struct regmap_test_param sparse_cache_types_list[] = {
+	{ .cache = REGCACHE_FLAT,   .from_reg = 0 },
+	{ .cache = REGCACHE_FLAT,   .from_reg = 0, .fast_io = true },
+	{ .cache = REGCACHE_FLAT,   .from_reg = 0x2001 },
+	{ .cache = REGCACHE_FLAT,   .from_reg = 0x2002 },
+	{ .cache = REGCACHE_FLAT,   .from_reg = 0x2003 },
+	{ .cache = REGCACHE_FLAT,   .from_reg = 0x2004 },
 	{ .cache = REGCACHE_RBTREE, .from_reg = 0 },
 	{ .cache = REGCACHE_RBTREE, .from_reg = 0, .fast_io = true },
 	{ .cache = REGCACHE_RBTREE, .from_reg = 0x2001 },
@@ -567,18 +573,9 @@ static void read_writeonly(struct kunit *test)
 	for (i = 0; i < BLOCK_TEST_SIZE; i++)
 		data->read[i] = false;
 
-	/*
-	 * Try to read all the registers, the writeonly one should
-	 * fail if we aren't using the flat cache.
-	 */
-	for (i = 0; i < BLOCK_TEST_SIZE; i++) {
-		if (config.cache_type != REGCACHE_FLAT) {
-			KUNIT_EXPECT_EQ(test, i != 5,
-					regmap_read(map, i, &val) == 0);
-		} else {
-			KUNIT_EXPECT_EQ(test, 0, regmap_read(map, i, &val));
-		}
-	}
+	/* 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]);
-- 
2.47.1

On Thu, Jan 09, 2025 at 07:02:55PM +0100, Sander Vanheule wrote:
> The flat regcache will always assume the data in the cache is valid.
> Since the cache takes priority over hardware values, this may shadow the
> actual state of the device. This is not the case with REGCACHE_RBTREE
> for example, which makes these implementation behave differently.

This is causing spurious events to be generated by the audio drivers on
Pine64 Plus, the audio drivers for that do use the flat cache (see
sound/soc/sunxi):

   https://lava.sirena.org.uk/scheduler/job/1056879#L1897

I haven't investigated yet but I suspect this is going to have been
triggered by the change from assuming all registers default to 0 if not
otherwise specified to reading back from the hardware if the first
access is a read.  That does seem a bit like a driver bug (I'm not clear
on the precise mechanism apart from anything else) but I worry that
it'll be a widespread one where things do read/modify/write cycles.
There may also be initialisation from suspend issues where we stop
resetting values after resume.  We do need a plan for this, possibly we
should default to the old behaviour.

It's definitely too late in the release cycle to apply the change in any
case.

Hi Mark,

On Thu, 2025-01-09 at 20:11 +0000, Mark Brown wrote:
> On Thu, Jan 09, 2025 at 07:02:55PM +0100, Sander Vanheule wrote:
> > The flat regcache will always assume the data in the cache is valid.
> > Since the cache takes priority over hardware values, this may shadow the
> > actual state of the device. This is not the case with REGCACHE_RBTREE
> > for example, which makes these implementation behave differently.
> 
> This is causing spurious events to be generated by the audio drivers on
> Pine64 Plus, the audio drivers for that do use the flat cache (see
> sound/soc/sunxi):
> 
>    https://lava.sirena.org.uk/scheduler/job/1056879#L1897
> 
> I haven't investigated yet but I suspect this is going to have been
> triggered by the change from assuming all registers default to 0 if not
> otherwise specified to reading back from the hardware if the first
> access is a read.  That does seem a bit like a driver bug (I'm not clear
> on the precise mechanism apart from anything else) but I worry that
> it'll be a widespread one where things do read/modify/write cycles.
> There may also be initialisation from suspend issues where we stop
> resetting values after resume.  We do need a plan for this, possibly we
> should default to the old behaviour.
> 
> It's definitely too late in the release cycle to apply the change in any
> case.

Apologies for the delay, it took me some time to get back to this properly.

Would you be open to providing a new type of flat cache with sparse
initalisation (e.g. REGCACHE_FLAT_SPARSE) to provide the behavior provided by
this patches? Most of the code could be shared with REGCACHE_FLAT.

REGCACHE_FLAT and REGCACHE_FLAT_SPARSE would both maintain the cache validity as
in this patch. On a read, _FLAT_SPARSE would return -ENOENT on an invalid cache
entry. _FLAT would then always return 0 and the (zero-initialized) value,
ignoring the cache validity and maintaining the behavior drivers may currently
rely on.

If you would like existing drivers to transition to REGCACHE_FLAT_SPARSE, I
could also add a deprecation warning for users of REGCACHE_FLAT.

I can also just send a patch series, if that helps.

Best,
Sander

On Thu, Oct 16, 2025 at 10:46:33AM +0200, Sander Vanheule wrote:

> Would you be open to providing a new type of flat cache with sparse
> initalisation (e.g. REGCACHE_FLAT_SPARSE) to provide the behavior provided by
> this patches? Most of the code could be shared with REGCACHE_FLAT.

Taking a step back for a minute, what's the actual problem you're trying
to solve here?  Why use a flat cache rather than a maple tree cache for
your application?

On Thu, 2025-10-16 at 12:53 +0100, Mark Brown wrote:
> On Thu, Oct 16, 2025 at 10:46:33AM +0200, Sander Vanheule wrote:
> 
> > Would you be open to providing a new type of flat cache with sparse
> > initalisation (e.g. REGCACHE_FLAT_SPARSE) to provide the behavior provided
> > by
> > this patches? Most of the code could be shared with REGCACHE_FLAT.
> 
> Taking a step back for a minute, what's the actual problem you're trying
> to solve here?  Why use a flat cache rather than a maple tree cache for
> your application?

The device I want to use this for has a small contiguous register space, so a
flat cache should be sufficient. The driver can also works with maple (or RB-
tree) cache.

The problem with the current flat cache, is that it provides different caching
behavior than the other cache types. I want to keep the register values written
by the bootloader, so I can't provide defaults. That means a flat cache will
seed these registers with 0x00 values, which don't reflect the hardware state.
Any RMW operation can then cause the part of the register to be cleared, as the
read comes from the (invalid) cache.

This difference in caching behavior isn't (wasn't) immediately clear to me from
the documentation. Don't the different cache types exist to optimize speed or
memory for different use-cases? Because then I would only expect differences in
memory/speed, not in way the cache is initialized.

Best,
Sander

On Thu, Oct 16, 2025 at 02:08:41PM +0200, Sander Vanheule wrote:
> On Thu, 2025-10-16 at 12:53 +0100, Mark Brown wrote:

> > Taking a step back for a minute, what's the actual problem you're trying
> > to solve here?  Why use a flat cache rather than a maple tree cache for
> > your application?

> The device I want to use this for has a small contiguous register space, so a
> flat cache should be sufficient. The driver can also works with maple (or RB-
> tree) cache.

I strongly recommend just doing that, unless you have a specific need to
use something else you should use a maple tree cache.

> This difference in caching behavior isn't (wasn't) immediately clear to me from
> the documentation. Don't the different cache types exist to optimize speed or
> memory for different use-cases? Because then I would only expect differences in
> memory/speed, not in way the cache is initialized.

The flat cache is a bit of a sharp edge, it's specifically there to be
the absolute bare minimum cache.  It's there mainly for MMIO devices
that are on the edge of being able to use regmap at all for performance
reasons, and as a simple way of guaranteeing that we never do any
allocations at runtime for things that do register access in hardirq
context.

The documentation could definitely use some improvement here.

On Thu, 2025-10-16 at 14:55 +0100, Mark Brown wrote:
> On Thu, Oct 16, 2025 at 02:08:41PM +0200, Sander Vanheule wrote:
> > This difference in caching behavior isn't (wasn't) immediately clear to me from
> > the documentation. Don't the different cache types exist to optimize speed or
> > memory for different use-cases? Because then I would only expect differences in
> > memory/speed, not in way the cache is initialized.
> 
> The flat cache is a bit of a sharp edge, it's specifically there to be
> the absolute bare minimum cache.  It's there mainly for MMIO devices
> that are on the edge of being able to use regmap at all for performance
> reasons, and as a simple way of guaranteeing that we never do any
> allocations at runtime for things that do register access in hardirq
> context.
> 
> The documentation could definitely use some improvement here.

The proposed patch also won't perform any allocations outside of the regmap
initialization. There is the additional overhead of checking a bitmap, yes, but
that's probably not a huge problem if the main issue are the allocations?

A number of REGCACHE_FLAT users provide only num_reg_defaults_raw. That looked
strange to me, because why would a user provide a buffer size, but no buffer?
This even conflicts with the documentation, as "Number of elements in
reg_defaults_raw" doesn't make sense if reg_defaults_raw is NULL. As you are
probably aware, this configuration causes the regmap init to read registers from
the device to generate a default state, which is then used to fill the flat
cache. So this effectively replaces the (invalid) zero-initialization. What
feels problematic to me, is that only the flat cache requires this sort of
"hack" for it to contain values that correctly reflect the hardware state.

Some quick grepping turned up 35 drivers which use REGCACHE_FLAT without any
cache initialization (see below), thus defaulting to the all-zero cache. This
includes the driver I think you found issues with on the pine64: sun8i-codec.c
If this driver (accidentally) relies on the zero-initialization, I would expect
the spurious interrupts to also pop up when switching to a maple cache, or when
using the num_reg_defaults_raw workaround.

drivers/clk/clk-si521xx.c
drivers/gpio/gpio-104-dio-48e.c
drivers/gpio/gpio-104-idio-16.c
drivers/gpio/gpio-gpio-mm.c
drivers/gpio/gpio-pcie-idio-24.c
drivers/gpio/gpio-pci-idio-16.c
drivers/gpio/gpio-ws16c48.c
drivers/gpu/drm/panel/panel-abt-y030xx067a.c
drivers/gpu/drm/panel/panel-novatek-nt39016.c
drivers/i2c/muxes/i2c-mux-ltc4306.c
drivers/iio/addac/stx104.c
drivers/iio/health/max30100.c
drivers/mfd/sec-i2c.c
drivers/platform/mellanox/mlxreg-dpu.c
drivers/pwm/pwm-fsl-ftm.c
sound/soc/atmel/mchp-pdmc.c
sound/soc/atmel/mchp-spdifrx.c
sound/soc/atmel/mchp-spdiftx.c
sound/soc/bcm/bcm63xx-i2s-whistler.c
sound/soc/codecs/msm8916-wcd-digital.c
sound/soc/loongson/loongson_i2s_pci.c
sound/soc/loongson/loongson_i2s_plat.c
sound/soc/qcom/lpass-cpu.c
sound/soc/rockchip/rockchip_spdif.c
sound/soc/soc-ops-test.c
sound/soc/stm/stm32_sai_sub.c
sound/soc/sunxi/sun8i-codec.c
sound/soc/tegra/tegra20_ac97.c
sound/soc/tegra/tegra20_das.c
sound/soc/tegra/tegra20_i2s.c
sound/soc/tegra/tegra20_spdif.c
sound/soc/tegra/tegra210_ahub.c
sound/soc/tegra/tegra30_ahub.c
sound/soc/tegra/tegra30_i2s.c
sound/soc/xtensa/xtfpga-i2s.c

On Thu, Oct 16, 2025 at 10:49:40PM +0200, Sander Vanheule wrote:

> The proposed patch also won't perform any allocations outside of the regmap
> initialization. There is the additional overhead of checking a bitmap, yes, but
> that's probably not a huge problem if the main issue are the allocations?

The allocations are more of an issue, but they're both potential issues.
Note that you can also avoid allocations with rbtree or maple by
prefilling the cache, it's just harder to absolutely guarantee that you
got everything right.

> A number of REGCACHE_FLAT users provide only num_reg_defaults_raw. That looked
> strange to me, because why would a user provide a buffer size, but no buffer?
> This even conflicts with the documentation, as "Number of elements in
> reg_defaults_raw" doesn't make sense if reg_defaults_raw is NULL. As you are
> probably aware, this configuration causes the regmap init to read registers from
> the device to generate a default state, which is then used to fill the flat
> cache. So this effectively replaces the (invalid) zero-initialization. What
> feels problematic to me, is that only the flat cache requires this sort of
> "hack" for it to contain values that correctly reflect the hardware state.

Yes, that's absoutely a hack for the lack of sparseness in the flat
cache.

> Some quick grepping turned up 35 drivers which use REGCACHE_FLAT without any
> cache initialization (see below), thus defaulting to the all-zero cache. This
> includes the driver I think you found issues with on the pine64: sun8i-codec.c
> If this driver (accidentally) relies on the zero-initialization, I would expect
> the spurious interrupts to also pop up when switching to a maple cache, or when
> using the num_reg_defaults_raw workaround.

Yes, that driver is relying on the current behaviour - I'd expect at
least some of the other drivers are too.  

There's two separate things here, there's what the driver you're working
on should do (which is to use a maple tree cache from the sounds of it)
and there's potential improvements to the flat cache, or adding a new
sparse flat cache.

Hi Mark,

On Fri, 2025-10-17 at 14:56 +0100, Mark Brown wrote:
> On Thu, Oct 16, 2025 at 10:49:40PM +0200, Sander Vanheule wrote:
> > Some quick grepping turned up 35 drivers which use REGCACHE_FLAT without any
> > cache initialization (see below), thus defaulting to the all-zero cache. This
> > includes the driver I think you found issues with on the pine64: sun8i-codec.c
> > If this driver (accidentally) relies on the zero-initialization, I would expect
> > the spurious interrupts to also pop up when switching to a maple cache, or when
> > using the num_reg_defaults_raw workaround.
> 
> Yes, that driver is relying on the current behaviour - I'd expect at
> least some of the other drivers are too.  
> 
> There's two separate things here, there's what the driver you're working
> on should do (which is to use a maple tree cache from the sounds of it)
> and there's potential improvements to the flat cache, or adding a new
> sparse flat cache.

For my driver I went with a maple cache, as that's the one I can get functional.

Meanwhile, I also noticed others on the linux-gpio list being unaware of the pitfalls of
the flat cache [1]. So I decided to send a v4 [2] of this patch, albeit split up in a new
flat-sparse variant and a warning for the current flat cache. Even with clearer
documentation on the limitations of the flat cache, devs/reviewers/maintainers would need
to be aware. I feel like it's better to just have less surprises.

[1] https://lore.kernel.org/linux-gpio/e461ca08-ad28-44fe-85f1-afe332c1d43d@topic.nl/
[2] https://lore.kernel.org/lkml/20251022200408.63027-1-sander@svanheule.net/T/

Best,
Sander