regcache: flat: Use cache element type in sizeof

PATCH In review Andrew F. Davis 2 participants 12 messages Jan 7, 2018 – Jan 10, 2018

Subsystems (2)

Driver core Register map abstraction

lore

Share & tools

lore

Andrew F. Davis

author

2018-01-07 23:22 UTC #0 lore raw

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

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 <[email protected]>

---
 drivers/base/regmap/regcache-flat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/base/regmap/regcache-flat.c +1 -1

index 4d2e50bfc726..2ea5fc84a374 100644
@@ -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;

Signature · 1 line

2.15.1

Andrew F. Davis

author

2018-01-07 23:23 UTC #1 lore raw

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

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

Signed-off-by Andrew F. Davis <[email protected]>

---
 drivers/base/regmap/regcache-flat.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

drivers/base/regmap/regcache-flat.c +10 -5

index 2ea5fc84a374..99a7792210b3 100644
@@ -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;
 }

Signature · 1 line

2.15.1

Andrew F. Davis

author

2018-01-07 23:22 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/base/regmap/regcache-flat.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

drivers/base/regmap/regcache-flat.c +16 -6

index 99a7792210b3..89a2226f9a81 100644
@@ -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;
 }

Signature · 1 line

2.15.1

Mark Brown

maintainer

2018-01-08 12:08 UTC #3 lore raw

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

Quoted 11 lines · 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.

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.

Andrew F. Davis

author

2018-01-08 16:25 UTC #4 lore raw

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

Quoted 13 lines · Mark Brown

Quoted 11 lines · 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.

Quoted 7 lines

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

Mark Brown

maintainer

2018-01-08 16:36 UTC #5 lore raw

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

Quoted 5 lines · Andrew F. Davis

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.

Quoted 3 lines

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

Andrew F. Davis

author

2018-01-08 16:47 UTC #6 lore raw

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

Quoted 8 lines · Mark Brown

Quoted 5 lines · Andrew F. Davis
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.

Quoted 5 lines

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

Quoted 6 lines

Quoted 3 lines
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.

Mark Brown

maintainer

2018-01-09 16:24 UTC #7 lore raw

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

Quoted 9 lines · Andrew F. Davis

Quoted 4 lines · Mark Brown
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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

Quoted 1 line
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.

Andrew F. Davis

author

2018-01-09 17:47 UTC #8 lore raw

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

Quoted 8 lines · Mark Brown

Quoted 6 lines · Andrew F. Davis
Quoted 4 lines · Mark Brown
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.

Quoted 6 lines

Quoted 2 lines
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.

Quoted 9 lines

Quoted 3 lines
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.

Quoted 16 lines

Quoted 5 lines
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.
Quoted 5 lines
Quoted 1 line
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.

Mark Brown

maintainer

2018-01-10 12:02 UTC #9 lore raw

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

Quoted 8 lines · Andrew F. Davis

Quoted 2 lines · Mark Brown
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.

Quoted 6 lines

Quoted 4 lines
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.

Quoted 12 lines

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

Quoted 3 lines

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

Mark Brown

maintainer

2018-01-08 11:59 UTC #10 lore raw

In reply to Andrew F. Davis

Quoted 2 lines · Andrew F. Davis

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.

Andrew F. Davis

author

2018-01-08 16:26 UTC #11 lore raw

Quoted 9 lines · Mark Brown

Quoted 2 lines · Andrew F. Davis
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.