Re: [PATCH] regcache: disable cache_only before rewriting paging registers

[Bui Duc Phuc]

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