linux-kernel - Re: [PATCH] regmap: allow to define reg_update

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YYLnlbTFRUdLrmpW@Ansuel-xps.localdomain>
Date:   Wed, 3 Nov 2021 20:48:37 +0100
From:   Ansuel Smith <ansuelsmth@...il.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] regmap: allow to define reg_update_bits for no bus
 configuration

On Wed, Nov 03, 2021 at 05:01:16PM +0000, Mark Brown wrote:
> On Tue, Nov 02, 2021 at 10:41:38PM +0100, Ansuel Smith wrote:
> 
> > @@ -3064,7 +3065,7 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg,
> >  	if (change)
> >  		*change = false;
> >  
> > -	if (regmap_volatile(map, reg) && map->reg_update_bits) {
> > +	if ((regmap_volatile(map, reg) || !map->bus) && map->reg_update_bits) {
> >  		ret = map->reg_update_bits(map->bus_context, reg, mask, val);
> >  		if (ret == 0 && change)
> >  			*change = true;
> 
> I don't understand this change.  The point of the check for volatile
> there is that if the register isn't volatile then we need to ensure that
> the cache gets updated with any change that happens so we need to go
> through paths that include cache updates.  The presence or otherwise of
> a bus does not seem at all relevant here.

I put the check there to not duplicate the code. The idea here is:
if we are doing a regmap_update_bits operation on a no bus configuration
and the function have a dedicated reg_update_bits function, use that
instead of the normal _reg_read, check and _reg_write.
To workaround the CACHE problem, I can add a check and detect if cache is
disabled and only with that option permit to add a reg_update_bits
function to the map (for no bus configuration).

Again the problem here is in situation where lock is handled outside of
the read/write and the read/modify/write operation has to be done in one
go so splitting this operation in 2 step (like it's done for
regmap_update_bits) would be problematic.

Another solution would be to expose a way to change the cache externally
to the regmap operation so that if someone require cache operation and
require also a dedicated reg_update_bits, he can do that by implementing
that in his own function.
A third solution would be check if it's possible to cache the value
externally to function... Something that calls the reg_update_bits
dedicated function and then update the cache if needed.

But do we really need to add all this complexity when we can just deny
an option with cache enabled and force to use the normal way (else part
in this function)

Hope I was able to explain better why we need this change.

-- 
	Ansuel