Skip to contentSkip to conversation

Revert "regmap: Grab the lock in regcache_cache_only()"

PATCH Applied Dimitris Papastamos 2 participants 15 messages Sep 29, 2011

Subsystems (2)
Driver core Register map abstraction
lore
Share & tools
lore

Dimitris Papastamos

author
#0 lore raw

PATCH 1/8 Revert "regmap: Grab the lock in regcache_cache_only()"

This reverts commit 38f6916976f9e964748f097be3688e334fb60f3d.

---
 drivers/base/regmap/regcache.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
drivers/base/regmap/regcache.c +0 -2
index 744ed145..f46e247 100644
@@ -273,9 +273,7 @@ EXPORT_SYMBOL_GPL(regcache_sync);
  */
 void regcache_cache_only(struct regmap *map, bool enable)
 {
-	mutex_lock(&map->sync_lock);
 	map->cache_only = enable;
-	mutex_unlock(&map->sync_lock);
 }
 EXPORT_SYMBOL_GPL(regcache_cache_only);
Signature · 1 line
1.7.6.4
author
#1 lore raw

PATCH 2/8 regmap: Make _regmap_write() global

Signed-off-by Dimitris Papastamos <[email protected]>
---
 drivers/base/regmap/internal.h |    3 +++
 drivers/base/regmap/regmap.c   |    4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)
drivers/base/regmap/internal.h +3 -0
index 2d51b1b..348ff02 100644
@@ -91,6 +91,9 @@ bool regmap_readable(struct regmap *map, unsigned int reg);
 bool regmap_volatile(struct regmap *map, unsigned int reg);
 bool regmap_precious(struct regmap *map, unsigned int reg);
 
+int _regmap_write(struct regmap *map, unsigned int reg,
+		  unsigned int val);
+
 #ifdef CONFIG_DEBUG_FS
 extern void regmap_debugfs_initcall(void);
 extern void regmap_debugfs_init(struct regmap *map);
drivers/base/regmap/regmap.c +2 -2
index 3596465..d786ddc 100644
@@ -296,8 +296,8 @@ static int _regmap_raw_write(struct regmap *map, unsigned int reg,
 	return ret;
 }
 
-static int _regmap_write(struct regmap *map, unsigned int reg,
-			 unsigned int val)
+int _regmap_write(struct regmap *map, unsigned int reg,
+		  unsigned int val)
 {
 	int ret;
 	BUG_ON(!map->format.format_write && !map->format.format_val);
Signature · 1 line
1.7.6.4
#2 lore raw

Re: [PATCH 2/8] regmap: Make _regmap_write() global

Quoted 1 line · Dimitris Papastamos
Signed-off-by Dimitris Papastamos <[email protected]>

Before we do this we need to make sync() take the lock, otherwise we end up with no locks at all being taken during the sync.

author
#4 lore raw

PATCH 3/8 regmap: Use the lockless _regmap_write() for syncing

In reply to Dimitris Papastamos
Signed-off-by Dimitris Papastamos <[email protected]>
---
 drivers/base/regmap/regcache-indexed.c |    4 ++--
 drivers/base/regmap/regcache-lzo.c     |    2 +-
 drivers/base/regmap/regcache-rbtree.c  |    2 +-
 drivers/base/regmap/regcache.c         |    2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)
drivers/base/regmap/regcache-indexed.c +2 -2
index 268497a..2e10bb1 100644
@@ -45,8 +45,8 @@ static int regcache_indexed_sync(struct regmap *map)
 	int ret;
 
 	for (i = 0; i < map->num_reg_defaults; i++) {
-		ret = regmap_write(map, map->reg_defaults[i].reg,
-				   map->reg_defaults[i].def);
+		ret = _regmap_write(map, map->reg_defaults[i].reg,
+				    map->reg_defaults[i].def);
 		if (ret < 0)
 			return ret;
 		dev_dbg(map->dev, "Synced register %#x, value %#x\n",
drivers/base/regmap/regcache-lzo.c +1 -1
index 9079cb5..ad6af92 100644
@@ -339,7 +339,7 @@ static int regcache_lzo_sync(struct regmap *map)
 		if (ret)
 			return ret;
 		map->cache_bypass = 1;
-		ret = regmap_write(map, i, val);
+		ret = _regmap_write(map, i, val);
 		map->cache_bypass = 0;
 		if (ret)
 			return ret;
drivers/base/regmap/regcache-rbtree.c +1 -1
index de32ced..40f23dd 100644
@@ -327,7 +327,7 @@ static int regcache_rbtree_sync(struct regmap *map)
 			if (val == def)
 				continue;
 			map->cache_bypass = 1;
-			ret = regmap_write(map, regtmp, val);
+			ret = _regmap_write(map, regtmp, val);
 			map->cache_bypass = 0;
 			if (ret)
 				return ret;
drivers/base/regmap/regcache.c +1 -1
index f46e247..b6efde7 100644
@@ -242,7 +242,7 @@ int regcache_sync(struct regmap *map)
 			if (ret < 0)
 				goto out;
 			map->cache_bypass = 1;
-			ret = regmap_write(map, i, val);
+			ret = _regmap_write(map, i, val);
 			map->cache_bypass = 0;
 			if (ret < 0)
 				goto out;
Signature · 1 line
1.7.6.4
author
#5 lore raw

PATCH 4/8 regmap: Guard regcache_sync() with map->lock

In reply to Dimitris Papastamos
Signed-off-by Dimitris Papastamos <[email protected]>
---
 drivers/base/regmap/regcache.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/base/regmap/regcache.c +2 -0
index b6efde7..674da82 100644
@@ -230,6 +230,7 @@ int regcache_sync(struct regmap *map)
 
 	BUG_ON(!map->cache_ops);
 
+	mutex_lock(&map->lock);
 	dev_dbg(map->dev, "Syncing %s cache\n",
 		map->cache_ops->name);
 	name = map->cache_ops->name;

@@ -254,6 +255,7 @@ int regcache_sync(struct regmap *map)
 	}
 out:
 	trace_regcache_sync(map->dev, name, "stop");
+	mutex_unlock(&map->lock);
 
 	return ret;
 }
Signature · 1 line
1.7.6.4
#6 lore raw

Re: [PATCH 4/8] regmap: Guard regcache_sync() with map->lock

Quoted 1 line · Dimitris Papastamos
Signed-off-by Dimitris Papastamos <[email protected]>

Ah, you're doing that now. This and the previous commit should be a single commit pushing the lock outwards - right now either commit by itself will break things.

author
#7 lore raw

Re: [PATCH 4/8] regmap: Guard regcache_sync() with map->lock

Quoted 6 lines · Mark Brown
Quoted 1 line · Dimitris Papastamos
Signed-off-by Dimitris Papastamos <[email protected]>

Ah, you're doing that now. This and the previous commit should be a single commit pushing the lock outwards - right now either commit by itself will break things.

Aw yes I've thought about it, just wasn't too sure about squashing the two commits together. Will fix and re-send.

author
#8 lore raw

PATCH 5/8 regmap: Save/restore the bypass state upon syncing

In reply to Dimitris Papastamos
Signed-off-by Dimitris Papastamos <[email protected]>
---
 drivers/base/regmap/regcache.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/base/regmap/regcache.c +5 -0
index 674da82..5362578 100644
@@ -227,10 +227,13 @@ int regcache_sync(struct regmap *map)
 	unsigned int val;
 	unsigned int i;
 	const char *name;
+	unsigned int bypass;
 
 	BUG_ON(!map->cache_ops);
 
 	mutex_lock(&map->lock);
+	/* Remember the initial bypass state */
+	bypass = map->cache_bypass;
 	dev_dbg(map->dev, "Syncing %s cache\n",
 		map->cache_ops->name);
 	name = map->cache_ops->name;

@@ -255,6 +258,8 @@ int regcache_sync(struct regmap *map)
 	}
 out:
 	trace_regcache_sync(map->dev, name, "stop");
+	/* Restore the bypass state */
+	map->cache_bypass = bypass;
 	mutex_unlock(&map->lock);
 
 	return ret;
Signature · 1 line
1.7.6.4
author
#9 lore raw

PATCH 6/8 regmap: Grab the lock in regcache_cache_only()

In reply to Dimitris Papastamos
Signed-off-by Dimitris Papastamos <[email protected]>
---
 drivers/base/regmap/regcache.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/base/regmap/regcache.c +2 -0
index 5362578..f91bc7b 100644
@@ -280,7 +280,9 @@ EXPORT_SYMBOL_GPL(regcache_sync);
  */
 void regcache_cache_only(struct regmap *map, bool enable)
 {
+	mutex_lock(&map->lock);
 	map->cache_only = enable;
+	mutex_unlock(&map->lock);
 }
 EXPORT_SYMBOL_GPL(regcache_cache_only);
Signature · 1 line
1.7.6.4
author
#10 lore raw

PATCH 7/8 regmap: Implement regcache_cache_bypass helper function

In reply to Dimitris Papastamos

Ensure we've got a function so users can enable/disable the cache bypass option.

Signed-off-by Dimitris Papastamos <[email protected]>
---
 drivers/base/regmap/regcache.c |   19 +++++++++++++++++++
 include/linux/regmap.h         |    1 +
 2 files changed, 20 insertions(+), 0 deletions(-)
drivers/base/regmap/regcache.c +19 -0
index f91bc7b..eaa2e20 100644
@@ -286,6 +286,25 @@ void regcache_cache_only(struct regmap *map, bool enable)
 }
 EXPORT_SYMBOL_GPL(regcache_cache_only);
 
+/**
+ * regcache_cache_bypass: Put a register map into cache bypass mode
+ *
+ * @map: map to configure
+ * @cache_only: flag if changes should not be written to the hardware
+ *
+ * When a register map is marked with the cache bypass option, writes
+ * to the register map API will only update the hardware and not the
+ * the cache directly.  This is useful when syncing the cache back to
+ * the hardware.
+ */
+void regcache_cache_bypass(struct regmap *map, bool enable)
+{
+	mutex_lock(&map->lock);
+	map->cache_bypass = enable;
+	mutex_unlock(&map->lock);
+}
+EXPORT_SYMBOL_GPL(regcache_cache_bypass);
+
 bool regcache_set_val(void *base, unsigned int idx,
 		      unsigned int val, unsigned int word_size)
 {
include/linux/regmap.h +1 -0
index 76ac255..3daac2d 100644
@@ -142,5 +142,6 @@ int regmap_update_bits(struct regmap *map, unsigned int reg,
 
 int regcache_sync(struct regmap *map);
 void regcache_cache_only(struct regmap *map, bool enable);
+void regcache_cache_bypass(struct regmap *map, bool enable);
 
 #endif
Signature · 1 line
1.7.6.4
author
#11 lore raw

PATCH 8/8 regmap: Ensure we scream if we enable cache bypass/only at the same time

In reply to Dimitris Papastamos
Signed-off-by Dimitris Papastamos <[email protected]>
---
 drivers/base/regmap/regcache.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/base/regmap/regcache.c +2 -0
index eaa2e20..82e619e 100644
@@ -280,6 +280,7 @@ EXPORT_SYMBOL_GPL(regcache_sync);
  */
 void regcache_cache_only(struct regmap *map, bool enable)
 {
+	BUG_ON(map->cache_bypass && enable);
 	mutex_lock(&map->lock);
 	map->cache_only = enable;
 	mutex_unlock(&map->lock);

@@ -299,6 +300,7 @@ EXPORT_SYMBOL_GPL(regcache_cache_only);
  */
 void regcache_cache_bypass(struct regmap *map, bool enable)
 {
+	BUG_ON(map->cache_only && enable);
 	mutex_lock(&map->lock);
 	map->cache_bypass = enable;
 	mutex_unlock(&map->lock);
Signature · 1 line
1.7.6.4
#12 lore raw

Re: [PATCH 8/8] regmap: Ensure we scream if we enable cache bypass/only at the same time

drivers/base/regmap/regcache.c EXPORT_SYMBOL_GPL(regcache_sync); · line 284

Quoted 4 lines · Dimitris Papastamos

void regcache_cache_only(struct regmap *map, bool enable) {

drivers/base/regmap/regcache.c +1 -0
+	BUG_ON(map->cache_bypass && enable);
 	mutex_lock(&map->lock);

I'd move these inside the lock otherwise there's an enable/enable race where two threads check that things are safe simultaneously. I'd also change to WARN_ON(), if nothing else it's hard to see BUG_ON()s that occur prior to console output and PMICs are likely to trigger that sort of stuff.

author
#13 lore raw

Re: [PATCH 8/8] regmap: Ensure we scream if we enable cache bypass/only at the same time

Quoted 12 lines · Mark Brown

drivers/base/regmap/regcache.c EXPORT_SYMBOL_GPL(regcache_sync); · line 284

Quoted 4 lines · Dimitris Papastamos

void regcache_cache_only(struct regmap *map, bool enable) {

drivers/base/regmap/regcache.c +1 -0
+	BUG_ON(map->cache_bypass && enable);
 	mutex_lock(&map->lock);

I'd move these inside the lock otherwise there's an enable/enable race where two threads check that things are safe simultaneously. I'd also change to WARN_ON(), if nothing else it's hard to see BUG_ON()s that occur prior to console output and PMICs are likely to trigger that sort of stuff.

Aw yes, of course :)

#14 lore raw
In reply to Dimitris Papastamos
Quoted 1 line · Dimitris Papastamos

This reverts commit 38f6916976f9e964748f097be3688e334fb60f3d.

When sending things like this you should always explain what you're doing and why. As things are this won't apply since I already fixed this by using the main lock.