linux-kernel - Re: [PATCH 3/3] regcache: flat: Add valid bit to this cache type

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: <6767739a-fc25-d92a-7466-26f0c226aabc@ti.com>
Date:   Tue, 9 Jan 2018 11:47:19 -0600
From:   "Andrew F. Davis" <afd@...com>
To:     Mark Brown <broonie@...nel.org>
CC:     <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] regcache: flat: Add valid bit to this cache type

On 01/09/2018 10:24 AM, Mark Brown wrote:
> On Mon, Jan 08, 2018 at 10:46:58AM -0600, Andrew F. Davis wrote:
>> On 01/08/2018 10:36 AM, Mark Brown wrote:
> 
>>> Users are supposed to ensure that the cache is fully initialized either
>>> by supplying defaults or writing to all the registers.  Adding reads is
>>> problematic since we'd suddenly start reading from hardware which might
>>> not like it.
> 
>> This does not appear documented anywhere, and is counter to the basic
>> definition of how a cache should work.
> 
> That's a bit strong; we just used the supplied defaults from bss after
> all!  Feel free to contribute documentation.
> 

I meant it to be strong, a cache that never consults the backing storage
and requires write-allocate or pre-warming to every single location is
wrong, not just an odd type that requires more documentation.

>> If the hardware does not like reads then the registers should be marked
>> as not readable in their regmap config, if they don't do this then they
>> need fixed, not the cache scheme.
> 
> If you have the time and enthusiasm to do the audit that ensures that
> this happens then go for it; we can't just go and cause regressions for
> users who have devices that are currently working fine even if it's only
> by chance.
> 

I would like to help here, what would be involved?

A quick looks shows 42 instances of use of this cache type, of these
only 8 do not define a readable table or have defaults for every register:

sound/soc/sunxi/sun8i-codec.c
sound/soc/codecs/msm8916-wcd-digital.c
sound/soc/atmel/atmel-classd.c
drivers/pwm/pwm-fsl-ftm.c
drivers/mfd/sec-core.c
drivers/media/i2c/max2175.c
drivers/iio/health/max30100.c
drivers/i2c/muxes/i2c-mux-ltc4306.c

The are the only ones that should need to be looked at, and setting a
table of all zero reg_defaults would restore the old behavior until each
could be confirmed to be unaffected by this change.

>> For my device, I cannot know beforehand what is in some registers as
>> they change from device to device, and if I do a read to find out it
>> will *always* return 0 as this cache type is broken. Other types work as
>> expected, fetching the real initial value. My only alternative is to
>> turn off cache, read, turn on cache, write, then continue as before..
> 
> That's one of the expected mechanisms for drivers to use.
> 
>>> Sounds like a good thing to do an audit and contribute fixes for...
> 
>> My guess is most assumed what I did and they only got lucky as all their
>> device registers were default 0 anyway. In which case the results before
>> and after this patch will be the same.
> 
> If your guess right that'll be easy.
>