From: bui duc phuc <phucduc.bui@gmail.com>

cache_only must be disabled before re-writing the paging selector
registers. Otherwise, _regmap_write() would update only the cache
and return without calling map->reg_write(), leaving the selector
register on the hardware unwritten and out of sync with the cache.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Note: This patch was created while working on the following patch series:
https://lore.kernel.org/all/20260713050312.38729-1-phucduc.bui@gmail.com/

 drivers/base/regmap/regcache.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 96cdae25b9c4..43ee7c6c32d8 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -406,6 +406,7 @@ int regcache_sync(struct regmap *map)
 	unsigned int i;
 	const char *name;
 	bool bypass;
+	bool cache_only;
 	struct rb_node *node;
 
 	if (WARN_ON(map->cache_type == REGCACHE_NONE))
@@ -455,6 +456,9 @@ int regcache_sync(struct regmap *map)
 	 * have gone out of sync, force writes of all the paging
 	 * registers.
 	 */
+	cache_only = map->cache_only;
+	map->cache_only = false;
+
 	rb_for_each(node, NULL, &map->range_tree, rbtree_all) {
 		struct regmap_range_node *this =
 			rb_entry(node, struct regmap_range_node, node);
@@ -472,6 +476,8 @@ int regcache_sync(struct regmap *map)
 		}
 	}
 
+	map->cache_only = cache_only;
+
 	map->unlock(map->lock_arg);
 
 	regmap_async_complete(map);
-- 
2.43.0

On Mon, Jul 13, 2026 at 02:30:58PM +0700, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> cache_only must be disabled before re-writing the paging selector
> registers. Otherwise, _regmap_write() would update only the cache
> and return without calling map->reg_write(), leaving the selector
> register on the hardware unwritten and out of sync with the cache.

No, any access to the hardware is buggy - the whole point of cache only
mode is that we can't access the hardware.  The paging register is part
of the context that needs to be rewritten when resyncing the cache.

Hi Mark,

Thanks for the feedback.

>
> No, any access to the hardware is buggy - the whole point of cache only
> mode is that we can't access the hardware.  The paging register is part
> of the context that needs to be rewritten when resyncing the cache.

To provide some context: I based this patch on your previous commit here:

https://patchew.org/linux/20231026-regmap-fix-selector-sync-v1-1-633ded82770d@kernel.org/

Reading that commit, I mistakenly understood that we must enforce
the hardware writes here at all costs. However, based on your explanation,
I see that I misunderstood the architectural intent.

Just to clarify for my understanding: does this mean regcache_sync()
is strictly
expected to be called only after cache_only has been set to false?

If so, I will drop this patch.

Best regards,
Phuc

On Mon, Jul 13, 2026 at 08:07:13PM +0700, Bui Duc Phuc wrote:

> Just to clarify for my understanding: does this mean regcache_sync()
> is strictly
> expected to be called only after cache_only has been set to false?

Yes, calling regcache_sync() with the cache enabled is not particularly
sensible - it basically exists to put all the changes made while a
device was in cache only mode (including due to suspend which is the
main case for that) onto the device.

Hi Mark,

Thanks for the clarification.

>
> Yes, calling regcache_sync() with the cache enabled is not particularly
> sensible - it basically exists to put all the changes made while a
> device was in cache only mode (including due to suspend which is the
> main case for that) onto the device.

That was actually my understanding as well. Initially, I was considering a
patch that would make it unnecessary to explicitly call
regcache_cache_only(map, true) again when regcache_sync() fails.

The idea was roughly:
1. check cache_only at the beginning of regcache_sync(), and if it is
    already true, emit a warning and return immediately;
2.  if regcache_sync() fails, automatically restore cache_only = true
     before returning.

However, after reviewing the existing users of regcache_sync(), I found
that there are several drivers that call regcache_sync() without first calling
regcache_cache_only(map, false). That made me think such a change
could break existing users, so I decided not to pursue that approach.

If the intended usage is indeed that regcache_cache_only(map, false) should
always be set before regcache_sync() is ever used, then I'm happy to revisit
this approach and also prepare follow-up patches for the drivers that don't
currently follow that pattern.

I'd appreciate your thoughts on whether that would be the right direction.

Best regards,
Phuc

On Mon, Jul 13, 2026 at 09:59:46PM +0700, Bui Duc Phuc wrote:

> However, after reviewing the existing users of regcache_sync(), I found
> that there are several drivers that call regcache_sync() without first calling
> regcache_cache_only(map, false). That made me think such a change
> could break existing users, so I decided not to pursue that approach.

> If the intended usage is indeed that regcache_cache_only(map, false) should
> always be set before regcache_sync() is ever used, then I'm happy to revisit
> this approach and also prepare follow-up patches for the drivers that don't
> currently follow that pattern.

You might not bother setting cache only in the suspend/resume path if
you know nothing else from the driver will touch the device until resume
is completed, something else might block anything that would try to
write.

Hi Mark,

Thanks for the explanation.

>
> You might not bother setting cache only in the suspend/resume path if
> you know nothing else from the driver will touch the device until resume
> is completed, something else might block anything that would try to
> write.

Going back to the original motivation for this patch, I'm trying to better
understand the intended semantics of regcache_sync().

------------------
if (!map->cache_bypass && !map->defer_caching) {
    ret = regcache_write(map, reg, val);
    if(ret != 0)
           return ret;
    if (map->cache_only) {
           map->cache_dirty = true;
           return 0;
    }
}

ret = map->reg_write(context, reg, val);
----------------------


As described in the commit message, if regcache_sync() is called while
cache_only is still enabled, it can return success even though the cache
and the hardware are still out of sync, with cache_dirty remaining true.

From the caller's perspective, that seems like a successful synchronization
even though no synchronization actually occurred.

From your explanation, I understand that some drivers intentionally do
not use cache_only in their suspend/resume paths because another
mechanism guarantees that no register accesses can occur while the
device is suspended.

If that's the case, then it seems that neither of these approaches would
be appropriate:
1. Automatically clearing cache_only before writing the selector register.
2. Making regcache_sync() reject calls when cache_only is still enabled.

Would it then be correct to understand that calling regcache_sync() while
cache_only is still enabled is simply considered incorrect API usage,
and that in such a case the responsibility lies with the caller, rather than
regcache_sync() itself ?
If so, would it make sense to document this requirement explicitly,
or perhaps emit a warning when regcache_sync() is called while
cache_only is still enabled?

Best regards,
Phuc

On Tue, Jul 14, 2026 at 04:58:45AM +0700, Bui Duc Phuc wrote:

> As described in the commit message, if regcache_sync() is called while
> cache_only is still enabled, it can return success even though the cache
> and the hardware are still out of sync, with cache_dirty remaining true.

> From the caller's perspective, that seems like a successful synchronization
> even though no synchronization actually occurred.

Right, we didn't explicitly add a check for that - I guess it never
occurred to anyone to do so since it's such a nonsensical thing to do.

> If that's the case, then it seems that neither of these approaches would
> be appropriate:
> 1. Automatically clearing cache_only before writing the selector register.
> 2. Making regcache_sync() reject calls when cache_only is still enabled.

Probably 2 is better.

> Would it then be correct to understand that calling regcache_sync() while
> cache_only is still enabled is simply considered incorrect API usage,
> and that in such a case the responsibility lies with the caller, rather than
> regcache_sync() itself ?

> If so, would it make sense to document this requirement explicitly,
> or perhaps emit a warning when regcache_sync() is called while
> cache_only is still enabled?

Yes.

Hi Mark,

Thank you for confirming.

>
> > As described in the commit message, if regcache_sync() is called while
> > cache_only is still enabled, it can return success even though the cache
> > and the hardware are still out of sync, with cache_dirty remaining true.
>
> > From the caller's perspective, that seems like a successful synchronization
> > even though no synchronization actually occurred.
>
> Right, we didn't explicitly add a check for that - I guess it never
> occurred to anyone to do so since it's such a nonsensical thing to do.
>

Just to note, the function already warns/blocks against a similarly unlikely
case right above:
if (WARN_ON(map->cache_type == REGCACHE_NONE))
return -EINVAL;
Calling sync with no cache type set seems just as unlikely in practice,
yet it's still guarded here.


> > If that's the case, then it seems that neither of these approaches would
> > be appropriate:
> > 1. Automatically clearing cache_only before writing the selector register.
> > 2. Making regcache_sync() reject calls when cache_only is still enabled.
>
> Probably 2 is better.
>
> > Would it then be correct to understand that calling regcache_sync() while
> > cache_only is still enabled is simply considered incorrect API usage,
> > and that in such a case the responsibility lies with the caller, rather than
> > regcache_sync() itself ?
>
> > If so, would it make sense to document this requirement explicitly,
> > or perhaps emit a warning when regcache_sync() is called while
> > cache_only is still enabled?
>
> Yes.

I will prepare a patch soon that implements both:

1. Returning -EINVAL with a WARN_ON(map->cache_only) check at the
   beginning of regcache_sync():

 + if (WARN_ON(map->cache_only))
 +       return -EINVAL;

2. Documenting this requirement in the function's comment block:

* This pushes cached changes made while cache_only (e.g. suspend) down
 * to hardware. The caller must clear cache_only first, or writes will
 * only update the cache.
 */

I will send the patch over once it's ready.

Best regards,
Phuc