[PATCH] regmap: Ensure range selector registers are updated after cache sync

[PATCH] regmap: Ensure range selector registers are updated after cache sync

[Mark Brown]

When we sync the register cache we do so with the cache bypassed in order
to avoid overhead from writing the synced values back into the cache. If
the regmap has ranges and the selector register for those ranges is in a
register which is cached this has the unfortunate side effect of meaning
that the physical and cached copies of the selector register can be out of
sync after a cache sync. The cache will have whatever the selector was when
the sync started and the hardware will have the selector for the register
that was synced last.

Fix this by rewriting all cached selector registers after every sync,
ensuring that the hardware and cache have the same content. This will
result in extra writes that wouldn't otherwise be needed but is simple
so hopefully robust. We don't read from the hardware since not all
devices have physical read support.

Given that nobody noticed this until now it is likely that we are rarely if
ever hitting this case.

Reported-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/base/regmap/regcache.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index c5d151e9c481..92592f944a3d 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -334,6 +334,11 @@ static int regcache_default_sync(struct regmap *map, unsigned int min,
 	return 0;
 }
 
+static int rbtree_all(const void *key, const struct rb_node *node)
+{
+	return 0;
+}
+
 /**
  * regcache_sync - Sync the register cache with the hardware.
  *
@@ -351,6 +356,7 @@ int regcache_sync(struct regmap *map)
 	unsigned int i;
 	const char *name;
 	bool bypass;
+	struct rb_node *node;
 
 	if (WARN_ON(map->cache_type == REGCACHE_NONE))
 		return -EINVAL;
@@ -392,6 +398,30 @@ int regcache_sync(struct regmap *map)
 	/* Restore the bypass state */
 	map->cache_bypass = bypass;
 	map->no_sync_defaults = false;
+
+	/*
+	 * If we did any paging with cache bypassed and a cached
+	 * paging register then the register and cache state might
+	 * have gone out of sync, force writes of all the paging
+	 * registers.
+	 */
+	rb_for_each(node, 0, &map->range_tree, rbtree_all) {
+		struct regmap_range_node *this =
+			rb_entry(node, struct regmap_range_node, node);
+
+		/* If there's nothing in the cache there's nothing to sync */
+		ret = regcache_read(map, this->selector_reg, &i);
+		if (ret != 0)
+			continue;
+
+		ret = _regmap_write(map, this->selector_reg, i);
+		if (ret != 0) {
+			dev_err(map->dev, "Failed to write %x = %x: %d\n",
+				this->selector_reg, i, ret);
+			break;
+		}
+	}
+
 	map->unlock(map->lock_arg);
 
 	regmap_async_complete(map);

---
base-commit: 611da07b89fdd53f140d7b33013f255bf0ed8f34
change-id: 20231026-regmap-fix-selector-sync-ad1514fd15df

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

Re: [PATCH] regmap: Ensure range selector registers are updated after cache sync

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]

On Thu, Oct 26, 2023 at 04:49:19PM +0100, Mark Brown wrote:
> When we sync the register cache we do so with the cache bypassed in order
> to avoid overhead from writing the synced values back into the cache. If
> the regmap has ranges and the selector register for those ranges is in a
> register which is cached this has the unfortunate side effect of meaning
> that the physical and cached copies of the selector register can be out of
> sync after a cache sync. The cache will have whatever the selector was when
> the sync started and the hardware will have the selector for the register
> that was synced last.

Given the nearness to the release I've dropped this into my CI and am
intending to just apply it as soon as that's done in the hopes that it
hits tomorrow's -next and gets a bit more coverage, it would be great if
you could confirm if this fixes the systems where you saw the original
issue.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] regmap: Ensure range selector registers are updated after cache sync

[Hector Martin]

On 27/10/2023 01.22, Mark Brown wrote:
> On Thu, Oct 26, 2023 at 04:49:19PM +0100, Mark Brown wrote:
>> When we sync the register cache we do so with the cache bypassed in order
>> to avoid overhead from writing the synced values back into the cache. If
>> the regmap has ranges and the selector register for those ranges is in a
>> register which is cached this has the unfortunate side effect of meaning
>> that the physical and cached copies of the selector register can be out of
>> sync after a cache sync. The cache will have whatever the selector was when
>> the sync started and the hardware will have the selector for the register
>> that was synced last.
> 
> Given the nearness to the release I've dropped this into my CI and am
> intending to just apply it as soon as that's done in the hopes that it
> hits tomorrow's -next and gets a bit more coverage, it would be great if
> you could confirm if this fixes the systems where you saw the original
> issue.

Can confirm, this fixes the sleep borking issue on my end after
reverting my workaround.

Tested-by: Hector Martin <marcan@marcan.st>

- Hector

Re: [PATCH] regmap: Ensure range selector registers are updated after cache sync

[Mark Brown]

On Thu, 26 Oct 2023 16:49:19 +0100, Mark Brown wrote:
> When we sync the register cache we do so with the cache bypassed in order
> to avoid overhead from writing the synced values back into the cache. If
> the regmap has ranges and the selector register for those ranges is in a
> register which is cached this has the unfortunate side effect of meaning
> that the physical and cached copies of the selector register can be out of
> sync after a cache sync. The cache will have whatever the selector was when
> the sync started and the hardware will have the selector for the register
> that was synced last.
> 
> [...]

Applied to

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

Thanks!

[1/1] regmap: Ensure range selector registers are updated after cache sync
      commit: 0ec7731655de196bc1e4af99e495b38778109d22

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