[PATCH 1/3] regcache: flat: Use cache element type in sizeof

[PATCH 1/3] regcache: flat: Use cache element type in sizeof

[Andrew F. Davis]

A single cache element may not always be unsigned int, use a
cache element in sizeof over hard-coding its type.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 drivers/base/regmap/regcache-flat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regcache-flat.c b/drivers/base/regmap/regcache-flat.c
index 4d2e50bfc726..2ea5fc84a374 100644
--- a/drivers/base/regmap/regcache-flat.c
+++ b/drivers/base/regmap/regcache-flat.c
@@ -31,7 +31,7 @@ static int regcache_flat_init(struct regmap *map)
 		return -EINVAL;
 
 	map->cache = kcalloc(regcache_flat_get_index(map, map->max_register)
-			     + 1, sizeof(unsigned int), GFP_KERNEL);
+			     + 1, sizeof(*cache), GFP_KERNEL);
 	if (!map->cache)
 		return -ENOMEM;
 
-- 
2.15.1

[PATCH 2/3] regcache: flat: Un-inline index lookup from cache access

[Andrew F. Davis]

This makes the code slightly more readable and allows for cleaner
addition of functionality in later patches.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 drivers/base/regmap/regcache-flat.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/base/regmap/regcache-flat.c b/drivers/base/regmap/regcache-flat.c
index 2ea5fc84a374..99a7792210b3 100644
--- a/drivers/base/regmap/regcache-flat.c
+++ b/drivers/base/regmap/regcache-flat.c
@@ -37,9 +37,12 @@ static int regcache_flat_init(struct regmap *map)
 
 	cache = map->cache;
 
-	for (i = 0; i < map->num_reg_defaults; i++)
-		cache[regcache_flat_get_index(map, map->reg_defaults[i].reg)] =
-				map->reg_defaults[i].def;
+	for (i = 0; i < map->num_reg_defaults; i++) {
+		unsigned int reg = map->reg_defaults[i].reg;
+		unsigned int index = regcache_flat_get_index(map, reg);
+
+		cache[index] = map->reg_defaults[i].def;
+	}
 
 	return 0;
 }
@@ -56,8 +59,9 @@ static int regcache_flat_read(struct regmap *map,
 			      unsigned int reg, unsigned int *value)
 {
 	unsigned int *cache = map->cache;
+	unsigned int index = regcache_flat_get_index(map, reg);
 
-	*value = cache[regcache_flat_get_index(map, reg)];
+	*value = cache[index];
 
 	return 0;
 }
@@ -66,8 +70,9 @@ static int regcache_flat_write(struct regmap *map, unsigned int reg,
 			       unsigned int value)
 {
 	unsigned int *cache = map->cache;
+	unsigned int index = regcache_flat_get_index(map, reg);
 
-	cache[regcache_flat_get_index(map, reg)] = value;
+	cache[index] = value;
 
 	return 0;
 }
-- 
2.15.1

[PATCH 3/3] regcache: flat: Add valid bit to this cache type

[Andrew F. Davis]

Other regmap cache types (LZO, RBtree) report back un-successful register
lookups when a value has not been previously written into the cache. This
allows regmap core to perform a real un-cached lookup to fetch the value.
The Flat type cache does not and so all read succeed reporting zero for the
registers value, even when the actual value is not known.

We fix this by changing the flat cache element type to a struct
containing both the register's value and a bool signifying if this value
is an actual cached value or not. This also opens up a path to implement
additional regcache_ops such as "sync" and "drop" that rely on such
validity information.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 drivers/base/regmap/regcache-flat.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/base/regmap/regcache-flat.c b/drivers/base/regmap/regcache-flat.c
index 99a7792210b3..89a2226f9a81 100644
--- a/drivers/base/regmap/regcache-flat.c
+++ b/drivers/base/regmap/regcache-flat.c
@@ -16,6 +16,11 @@
 
 #include "internal.h"
 
+struct regcache_flat_reg {
+	unsigned int value; /* Value of this register */
+	bool valid; /* Is value valid? */
+} __packed;
+
 static inline unsigned int regcache_flat_get_index(const struct regmap *map,
 						   unsigned int reg)
 {
@@ -25,7 +30,7 @@ static inline unsigned int regcache_flat_get_index(const struct regmap *map,
 static int regcache_flat_init(struct regmap *map)
 {
 	int i;
-	unsigned int *cache;
+	struct regcache_flat_reg *cache;
 
 	if (!map || map->reg_stride_order < 0 || !map->max_register)
 		return -EINVAL;
@@ -41,7 +46,8 @@ static int regcache_flat_init(struct regmap *map)
 		unsigned int reg = map->reg_defaults[i].reg;
 		unsigned int index = regcache_flat_get_index(map, reg);
 
-		cache[index] = map->reg_defaults[i].def;
+		cache[index].value = map->reg_defaults[i].def;
+		cache[index].valid = true;
 	}
 
 	return 0;
@@ -58,10 +64,13 @@ static int regcache_flat_exit(struct regmap *map)
 static int regcache_flat_read(struct regmap *map,
 			      unsigned int reg, unsigned int *value)
 {
-	unsigned int *cache = map->cache;
+	struct regcache_flat_reg *cache = map->cache;
 	unsigned int index = regcache_flat_get_index(map, reg);
 
-	*value = cache[index];
+	if (!cache[index].valid)
+		return -ENOENT;
+
+	*value = cache[index].value;
 
 	return 0;
 }
@@ -69,10 +78,11 @@ static int regcache_flat_read(struct regmap *map,
 static int regcache_flat_write(struct regmap *map, unsigned int reg,
 			       unsigned int value)
 {
-	unsigned int *cache = map->cache;
+	struct regcache_flat_reg *cache = map->cache;
 	unsigned int index = regcache_flat_get_index(map, reg);
 
-	cache[index] = value;
+	cache[index].value = value;
+	cache[index].valid = true;
 
 	return 0;
 }
-- 
2.15.1

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

[Mark Brown]

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

On Sun, Jan 07, 2018 at 05:22:34PM -0600, Andrew F. Davis wrote:
> Other regmap cache types (LZO, RBtree) report back un-successful register
> lookups when a value has not been previously written into the cache. This
> allows regmap core to perform a real un-cached lookup to fetch the value.
> The Flat type cache does not and so all read succeed reporting zero for the
> registers value, even when the actual value is not known.

> We fix this by changing the flat cache element type to a struct
> containing both the register's value and a bool signifying if this value
> is an actual cached value or not. This also opens up a path to implement
> additional regcache_ops such as "sync" and "drop" that rely on such
> validity information.

The reason we don't do this for the flat cache is that its whole purpose
is to be as fast as possible in order to make it easier for MMIO devices.  
Things that can take the hit of extra complexity should just use a rbtree
cache since if the registers are all bunched together it rapidly becomes
effectively the same data structure anyway.  Unless you really know why
you're using a flat cache you probably shouldn't.

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

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

[Andrew F. Davis]

On 01/08/2018 06:08 AM, Mark Brown wrote:
> On Sun, Jan 07, 2018 at 05:22:34PM -0600, Andrew F. Davis wrote:
>> Other regmap cache types (LZO, RBtree) report back un-successful register
>> lookups when a value has not been previously written into the cache. This
>> allows regmap core to perform a real un-cached lookup to fetch the value.
>> The Flat type cache does not and so all read succeed reporting zero for the
>> registers value, even when the actual value is not known.
> 
>> We fix this by changing the flat cache element type to a struct
>> containing both the register's value and a bool signifying if this value
>> is an actual cached value or not. This also opens up a path to implement
>> additional regcache_ops such as "sync" and "drop" that rely on such
>> validity information.
> 
> The reason we don't do this for the flat cache is that its whole purpose
> is to be as fast as possible in order to make it easier for MMIO devices.  
> Things that can take the hit of extra complexity should just use a rbtree
> cache since if the registers are all bunched together it rapidly becomes
> effectively the same data structure anyway.  Unless you really know why
> you're using a flat cache you probably shouldn't.
> 

I can understand the need for a fast cache type, but without this change
the implementation is simply wrong IMHO. Reading from a register that
has not been read/written to should not just assume the default value is
0, it should first read and load the real initial value into the cache,
just like other cache types do.

Also, it looks like this cache type is used mostly in I2C devices
(outside of sound/soc/, where in there, like you said MMIO devices are
more common).

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

[Mark Brown]

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

On Mon, Jan 08, 2018 at 10:25:28AM -0600, Andrew F. Davis wrote:

> I can understand the need for a fast cache type, but without this change
> the implementation is simply wrong IMHO. Reading from a register that
> has not been read/written to should not just assume the default value is
> 0, it should first read and load the real initial value into the cache,
> just like other cache types do.

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.

> Also, it looks like this cache type is used mostly in I2C devices
> (outside of sound/soc/, where in there, like you said MMIO devices are
> more common).

Sounds like a good thing to do an audit and contribute fixes for...

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

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

[Andrew F. Davis]

On 01/08/2018 10:36 AM, Mark Brown wrote:
> On Mon, Jan 08, 2018 at 10:25:28AM -0600, Andrew F. Davis wrote:
> 
>> I can understand the need for a fast cache type, but without this change
>> the implementation is simply wrong IMHO. Reading from a register that
>> has not been read/written to should not just assume the default value is
>> 0, it should first read and load the real initial value into the cache,
>> just like other cache types do.
> 
> 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.

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.

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..

>> Also, it looks like this cache type is used mostly in I2C devices
>> (outside of sound/soc/, where in there, like you said MMIO devices are
>> more common).
> 
> 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.

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

[Mark Brown]

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

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.

> 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.

> 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.

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

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

[Andrew F. Davis]

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.
> 

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

[Mark Brown]

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

On Tue, Jan 09, 2018 at 11:47:19AM -0600, Andrew F. Davis wrote:
> On 01/09/2018 10:24 AM, Mark Brown wrote:

> > 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.

The devices the cache infrastructure was originally designed for don't
have readback support in the hardware and people hardly ever error check
reads so we have the assumptions we have.  

> > 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?

Look through all the devices, do something to make sure that their uses
make sense and have a list of the ones that remain explaining why
they're safe.

> 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.

A lot of those should probably be converted to rbtree (sec-core will
never suspend for example).  

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

Re: [PATCH 1/3] regcache: flat: Use cache element type in sizeof

[Mark Brown]

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

On Sun, Jan 07, 2018 at 05:22:32PM -0600, Andrew F. Davis wrote:
> A single cache element may not always be unsigned int, use a
> cache element in sizeof over hard-coding its type.

A cache element is *always* unsigned int.  What you mean to say here is
that this helps if in future someone changes the code to use a different
type, it's not fixing a memory corruption problem that exists now which
is what your commit message suggests.

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

Re: [PATCH 1/3] regcache: flat: Use cache element type in sizeof

[Andrew F. Davis]

On 01/08/2018 05:59 AM, Mark Brown wrote:
> On Sun, Jan 07, 2018 at 05:22:32PM -0600, Andrew F. Davis wrote:
>> A single cache element may not always be unsigned int, use a
>> cache element in sizeof over hard-coding its type.
> 
> A cache element is *always* unsigned int.  What you mean to say here is
> that this helps if in future someone changes the code to use a different
> type, it's not fixing a memory corruption problem that exists now which
> is what your commit message suggests.
> 

Right, will re-word.