[PATCH 1/2] regmap: regcache: Fixup locking for custom lock callbacks

[PATCH 1/2] regmap: regcache: Fixup locking for custom lock callbacks

[Lars-Peter Clausen]

The parameter passed to the regmap lock/unlock callbacks needs to be
map->lock_arg, regcache passes just map. This works fine in the case that no
custom locking callbacks are used, since in this case map->lock_arg equals map,
but will break when custom locking callbacks are used. The issue was introduced
in commit 0d4529c5 ("regmap: make lock/unlock functions customizable") and is
fixed by this patch.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/base/regmap/regcache-rbtree.c |  4 ++--
 drivers/base/regmap/regcache.c        | 24 ++++++++++++------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/base/regmap/regcache-rbtree.c b/drivers/base/regmap/regcache-rbtree.c
index 4e131c5..69b443f 100644
--- a/drivers/base/regmap/regcache-rbtree.c
+++ b/drivers/base/regmap/regcache-rbtree.c
@@ -143,7 +143,7 @@ static int rbtree_show(struct seq_file *s, void *ignored)
 	int registers = 0;
 	int this_registers, average;
 
-	map->lock(map);
+	map->lock(map->lock_arg);
 
 	mem_size = sizeof(*rbtree_ctx);
 	mem_size += BITS_TO_LONGS(map->cache_present_nbits) * sizeof(long);
@@ -170,7 +170,7 @@ static int rbtree_show(struct seq_file *s, void *ignored)
 	seq_printf(s, "%d nodes, %d registers, average %d registers, used %zu bytes\n",
 		   nodes, registers, average, mem_size);
 
-	map->unlock(map);
+	map->unlock(map->lock_arg);
 
 	return 0;
 }
diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 8a0ab5f..4bfa219 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -270,7 +270,7 @@ int regcache_sync(struct regmap *map)
 
 	BUG_ON(!map->cache_ops || !map->cache_ops->sync);
 
-	map->lock(map);
+	map->lock(map->lock_arg);
 	/* Remember the initial bypass state */
 	bypass = map->cache_bypass;
 	dev_dbg(map->dev, "Syncing %s cache\n",
@@ -306,7 +306,7 @@ out:
 	trace_regcache_sync(map->dev, name, "stop");
 	/* Restore the bypass state */
 	map->cache_bypass = bypass;
-	map->unlock(map);
+	map->unlock(map->lock_arg);
 
 	return ret;
 }
@@ -333,7 +333,7 @@ int regcache_sync_region(struct regmap *map, unsigned int min,
 
 	BUG_ON(!map->cache_ops || !map->cache_ops->sync);
 
-	map->lock(map);
+	map->lock(map->lock_arg);
 
 	/* Remember the initial bypass state */
 	bypass = map->cache_bypass;
@@ -352,7 +352,7 @@ out:
 	trace_regcache_sync(map->dev, name, "stop region");
 	/* Restore the bypass state */
 	map->cache_bypass = bypass;
-	map->unlock(map);
+	map->unlock(map->lock_arg);
 
 	return ret;
 }
@@ -378,7 +378,7 @@ int regcache_drop_region(struct regmap *map, unsigned int min,
 	if (!map->cache_present && !(map->cache_ops && map->cache_ops->drop))
 		return -EINVAL;
 
-	map->lock(map);
+	map->lock(map->lock_arg);
 
 	trace_regcache_drop_region(map->dev, min, max);
 
@@ -389,7 +389,7 @@ int regcache_drop_region(struct regmap *map, unsigned int min,
 	if (map->cache_ops && map->cache_ops->drop)
 		ret = map->cache_ops->drop(map, min, max);
 
-	map->unlock(map);
+	map->unlock(map->lock_arg);
 
 	return ret;
 }
@@ -409,11 +409,11 @@ EXPORT_SYMBOL_GPL(regcache_drop_region);
  */
 void regcache_cache_only(struct regmap *map, bool enable)
 {
-	map->lock(map);
+	map->lock(map->lock_arg);
 	WARN_ON(map->cache_bypass && enable);
 	map->cache_only = enable;
 	trace_regmap_cache_only(map->dev, enable);
-	map->unlock(map);
+	map->unlock(map->lock_arg);
 }
 EXPORT_SYMBOL_GPL(regcache_cache_only);
 
@@ -428,9 +428,9 @@ EXPORT_SYMBOL_GPL(regcache_cache_only);
  */
 void regcache_mark_dirty(struct regmap *map)
 {
-	map->lock(map);
+	map->lock(map->lock_arg);
 	map->cache_dirty = true;
-	map->unlock(map);
+	map->unlock(map->lock_arg);
 }
 EXPORT_SYMBOL_GPL(regcache_mark_dirty);
 
@@ -447,11 +447,11 @@ EXPORT_SYMBOL_GPL(regcache_mark_dirty);
  */
 void regcache_cache_bypass(struct regmap *map, bool enable)
 {
-	map->lock(map);
+	map->lock(map->lock_arg);
 	WARN_ON(map->cache_only && enable);
 	map->cache_bypass = enable;
 	trace_regmap_cache_bypass(map->dev, enable);
-	map->unlock(map);
+	map->unlock(map->lock_arg);
 }
 EXPORT_SYMBOL_GPL(regcache_cache_bypass);
 
-- 
1.8.0

[PATCH 2/2] regmap: Make regmap-mmio usable from different contexts

[Lars-Peter Clausen]

regmap-mmio uses a spinlock with spin_lock() and spin_unlock() for locking.
Which means in order to avoid race conditions the lock always needs to be taken
from the same context. In practice at least one of the calls to the regmap API
happens from sleepable context, which means all calls to the regmap API have to
be made from sleepable context. This not only prevents us from using regmap-mmio
from interrupt context but also from other sections where IRQs are disabled.
E.g. if interrupts are disabled because a interrupt safe spinlock is locked.
Not being able to use regmap from different contexts limits the number of
usecases for regmap-mmio quite a bit.

This patch updates the adds a flags parameter to the regmap lock and unlock
callbacks and uses spin_lock_irqsave() and spin_unlock_restore() for the mmio
case. This allows us to use regmap-mmio from different contexts.

For the non-mmio case where regmap uses a mutex for locking the flags parameter
will be ignored and it is still only possible to use regmap API from sleepable
context.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

---

E.g. the Tegra ASoC drivers should suffer from this since the ALSA trigger
callback is called with interrupts off.

---
 drivers/base/regmap/regcache-rbtree.c |  5 +--
 drivers/base/regmap/regcache.c        | 33 ++++++++++++--------
 drivers/base/regmap/regmap.c          | 57 ++++++++++++++++++++---------------
 drivers/mfd/sta2x11-mfd.c             |  6 ++--
 drivers/staging/imx-drm/imx-tve.c     |  4 +--
 include/linux/regmap.h                |  4 +--
 6 files changed, 64 insertions(+), 45 deletions(-)

diff --git a/drivers/base/regmap/regcache-rbtree.c b/drivers/base/regmap/regcache-rbtree.c
index 69b443f..d424f71 100644
--- a/drivers/base/regmap/regcache-rbtree.c
+++ b/drivers/base/regmap/regcache-rbtree.c
@@ -142,8 +142,9 @@ static int rbtree_show(struct seq_file *s, void *ignored)
 	int nodes = 0;
 	int registers = 0;
 	int this_registers, average;
+	unsigned long flags;
 
-	map->lock(map->lock_arg);
+	map->lock(map->lock_arg, &flags);
 
 	mem_size = sizeof(*rbtree_ctx);
 	mem_size += BITS_TO_LONGS(map->cache_present_nbits) * sizeof(long);
@@ -170,7 +171,7 @@ static int rbtree_show(struct seq_file *s, void *ignored)
 	seq_printf(s, "%d nodes, %d registers, average %d registers, used %zu bytes\n",
 		   nodes, registers, average, mem_size);
 
-	map->unlock(map->lock_arg);
+	map->unlock(map->lock_arg, &flags);
 
 	return 0;
 }
diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 4bfa219..8497cd1 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -267,10 +267,11 @@ int regcache_sync(struct regmap *map)
 	unsigned int i;
 	const char *name;
 	unsigned int bypass;
+	unsigned long flags;
 
 	BUG_ON(!map->cache_ops || !map->cache_ops->sync);
 
-	map->lock(map->lock_arg);
+	map->lock(map->lock_arg, &flags);
 	/* Remember the initial bypass state */
 	bypass = map->cache_bypass;
 	dev_dbg(map->dev, "Syncing %s cache\n",
@@ -306,7 +307,7 @@ out:
 	trace_regcache_sync(map->dev, name, "stop");
 	/* Restore the bypass state */
 	map->cache_bypass = bypass;
-	map->unlock(map->lock_arg);
+	map->unlock(map->lock_arg, &flags);
 
 	return ret;
 }
@@ -330,10 +331,11 @@ int regcache_sync_region(struct regmap *map, unsigned int min,
 	int ret = 0;
 	const char *name;
 	unsigned int bypass;
+	unsigned long flags;
 
 	BUG_ON(!map->cache_ops || !map->cache_ops->sync);
 
-	map->lock(map->lock_arg);
+	map->lock(map->lock_arg, &flags);
 
 	/* Remember the initial bypass state */
 	bypass = map->cache_bypass;
@@ -352,7 +354,7 @@ out:
 	trace_regcache_sync(map->dev, name, "stop region");
 	/* Restore the bypass state */
 	map->cache_bypass = bypass;
-	map->unlock(map->lock_arg);
+	map->unlock(map->lock_arg, &flags);
 
 	return ret;
 }
@@ -372,13 +374,14 @@ EXPORT_SYMBOL_GPL(regcache_sync_region);
 int regcache_drop_region(struct regmap *map, unsigned int min,
 			 unsigned int max)
 {
+	unsigned long flags;
 	unsigned int reg;
 	int ret = 0;
 
 	if (!map->cache_present && !(map->cache_ops && map->cache_ops->drop))
 		return -EINVAL;
 
-	map->lock(map->lock_arg);
+	map->lock(map->lock_arg, &flags);
 
 	trace_regcache_drop_region(map->dev, min, max);
 
@@ -389,7 +392,7 @@ int regcache_drop_region(struct regmap *map, unsigned int min,
 	if (map->cache_ops && map->cache_ops->drop)
 		ret = map->cache_ops->drop(map, min, max);
 
-	map->unlock(map->lock_arg);
+	map->unlock(map->lock_arg, &flags);
 
 	return ret;
 }
@@ -409,11 +412,13 @@ EXPORT_SYMBOL_GPL(regcache_drop_region);
  */
 void regcache_cache_only(struct regmap *map, bool enable)
 {
-	map->lock(map->lock_arg);
+	unsigned long flags;
+
+	map->lock(map->lock_arg, &flags);
 	WARN_ON(map->cache_bypass && enable);
 	map->cache_only = enable;
 	trace_regmap_cache_only(map->dev, enable);
-	map->unlock(map->lock_arg);
+	map->unlock(map->lock_arg, &flags);
 }
 EXPORT_SYMBOL_GPL(regcache_cache_only);
 
@@ -428,9 +433,11 @@ EXPORT_SYMBOL_GPL(regcache_cache_only);
  */
 void regcache_mark_dirty(struct regmap *map)
 {
-	map->lock(map->lock_arg);
+	unsigned long flags;
+
+	map->lock(map->lock_arg, &flags);
 	map->cache_dirty = true;
-	map->unlock(map->lock_arg);
+	map->unlock(map->lock_arg, &flags);
 }
 EXPORT_SYMBOL_GPL(regcache_mark_dirty);
 
@@ -447,11 +454,13 @@ EXPORT_SYMBOL_GPL(regcache_mark_dirty);
  */
 void regcache_cache_bypass(struct regmap *map, bool enable)
 {
-	map->lock(map->lock_arg);
+	unsigned long flags;
+
+	map->lock(map->lock_arg, &flags);
 	WARN_ON(map->cache_only && enable);
 	map->cache_bypass = enable;
 	trace_regmap_cache_bypass(map->dev, enable);
-	map->unlock(map->lock_arg);
+	map->unlock(map->lock_arg, &flags);
 }
 EXPORT_SYMBOL_GPL(regcache_cache_bypass);
 
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 307f5a1..8369e2c 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -287,28 +287,28 @@ static unsigned int regmap_parse_32_native(const void *buf)
 	return *(u32 *)buf;
 }
 
-static void regmap_lock_mutex(void *__map)
+static void regmap_lock_mutex(void *__map, unsigned long *flags)
 {
 	struct regmap *map = __map;
 	mutex_lock(&map->mutex);
 }
 
-static void regmap_unlock_mutex(void *__map)
+static void regmap_unlock_mutex(void *__map, unsigned long *flags)
 {
 	struct regmap *map = __map;
 	mutex_unlock(&map->mutex);
 }
 
-static void regmap_lock_spinlock(void *__map)
+static void regmap_lock_spinlock(void *__map, unsigned long *flags)
 {
 	struct regmap *map = __map;
-	spin_lock(&map->spinlock);
+	spin_lock_irqsave(&map->spinlock, *flags);
 }
 
-static void regmap_unlock_spinlock(void *__map)
+static void regmap_unlock_spinlock(void *__map, unsigned long *flags)
 {
 	struct regmap *map = __map;
-	spin_unlock(&map->spinlock);
+	spin_unlock_irqrestore(&map->spinlock, *flags);
 }
 
 static void dev_get_regmap_release(struct device *dev, void *res)
@@ -1198,16 +1198,17 @@ int _regmap_write(struct regmap *map, unsigned int reg,
  */
 int regmap_write(struct regmap *map, unsigned int reg, unsigned int val)
 {
+	unsigned long flags;
 	int ret;
 
 	if (reg % map->reg_stride)
 		return -EINVAL;
 
-	map->lock(map->lock_arg);
+	map->lock(map->lock_arg, &flags);
 
 	ret = _regmap_write(map, reg, val);
 
-	map->unlock(map->lock_arg);
+	map->unlock(map->lock_arg, &flags);
 
 	return ret;
 }
@@ -1232,6 +1233,7 @@ EXPORT_SYMBOL_GPL(regmap_write);
 int regmap_raw_write(struct regmap *map, unsigned int reg,
 		     const void *val, size_t val_len)
 {
+	unsigned long flags;
 	int ret;
 
 	if (!regmap_can_raw_write(map))
@@ -1239,11 +1241,11 @@ int regmap_raw_write(struct regmap *map, unsigned int reg,
 	if (val_len % map->format.val_bytes)
 		return -EINVAL;
 
-	map->lock(map->lock_arg);
+	map->lock(map->lock_arg, &flags);
 
 	ret = _regmap_raw_write(map, reg, val, val_len, false);
 
-	map->unlock(map->lock_arg);
+	map->unlock(map->lock_arg, &flags);
 
 	return ret;
 }
@@ -1266,6 +1268,7 @@ EXPORT_SYMBOL_GPL(regmap_raw_write);
 int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val,
 		     size_t val_count)
 {
+	unsigned long flags;
 	int ret = 0, i;
 	size_t val_bytes = map->format.val_bytes;
 	void *wval;
@@ -1277,7 +1280,7 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val,
 	if (reg % map->reg_stride)
 		return -EINVAL;
 
-	map->lock(map->lock_arg);
+	map->lock(map->lock_arg, &flags);
 
 	/* No formatting is require if val_byte is 1 */
 	if (val_bytes == 1) {
@@ -1314,7 +1317,7 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val,
 		kfree(wval);
 
 out:
-	map->unlock(map->lock_arg);
+	map->unlock(map->lock_arg, &flags);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regmap_bulk_write);
@@ -1344,6 +1347,7 @@ EXPORT_SYMBOL_GPL(regmap_bulk_write);
 int regmap_raw_write_async(struct regmap *map, unsigned int reg,
 			   const void *val, size_t val_len)
 {
+	unsigned long flags;
 	int ret;
 
 	if (val_len % map->format.val_bytes)
@@ -1351,11 +1355,11 @@ int regmap_raw_write_async(struct regmap *map, unsigned int reg,
 	if (reg % map->reg_stride)
 		return -EINVAL;
 
-	map->lock(map->lock_arg);
+	map->lock(map->lock_arg, &flags);
 
 	ret = _regmap_raw_write(map, reg, val, val_len, true);
 
-	map->unlock(map->lock_arg);
+	map->unlock(map->lock_arg, &flags);
 
 	return ret;
 }
@@ -1462,16 +1466,17 @@ static int _regmap_read(struct regmap *map, unsigned int reg,
  */
 int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val)
 {
+	unsigned long flags;
 	int ret;
 
 	if (reg % map->reg_stride)
 		return -EINVAL;
 
-	map->lock(map->lock_arg);
+	map->lock(map->lock_arg, &flags);
 
 	ret = _regmap_read(map, reg, val);
 
-	map->unlock(map->lock_arg);
+	map->unlock(map->lock_arg, &flags);
 
 	return ret;
 }
@@ -1493,6 +1498,7 @@ int regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
 {
 	size_t val_bytes = map->format.val_bytes;
 	size_t val_count = val_len / val_bytes;
+	unsigned long flags;
 	unsigned int v;
 	int ret, i;
 
@@ -1503,7 +1509,7 @@ int regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
 	if (reg % map->reg_stride)
 		return -EINVAL;
 
-	map->lock(map->lock_arg);
+	map->lock(map->lock_arg, &flags);
 
 	if (regmap_volatile_range(map, reg, val_count) || map->cache_bypass ||
 	    map->cache_type == REGCACHE_NONE) {
@@ -1525,7 +1531,7 @@ int regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
 	}
 
  out:
-	map->unlock(map->lock_arg);
+	map->unlock(map->lock_arg, &flags);
 
 	return ret;
 }
@@ -1631,12 +1637,13 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg,
 int regmap_update_bits(struct regmap *map, unsigned int reg,
 		       unsigned int mask, unsigned int val)
 {
+	unsigned long flags;
 	bool change;
 	int ret;
 
-	map->lock(map->lock_arg);
+	map->lock(map->lock_arg, &flags);
 	ret = _regmap_update_bits(map, reg, mask, val, &change);
-	map->unlock(map->lock_arg);
+	map->unlock(map->lock_arg, &flags);
 
 	return ret;
 }
@@ -1658,11 +1665,12 @@ int regmap_update_bits_check(struct regmap *map, unsigned int reg,
 			     unsigned int mask, unsigned int val,
 			     bool *change)
 {
+	unsigned long flags;
 	int ret;
 
-	map->lock(map->lock_arg);
+	map->lock(map->lock_arg, &flags);
 	ret = _regmap_update_bits(map, reg, mask, val, change);
-	map->unlock(map->lock_arg);
+	map->unlock(map->lock_arg, &flags);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(regmap_update_bits_check);
@@ -1752,6 +1760,7 @@ EXPORT_SYMBOL_GPL(regmap_async_complete);
 int regmap_register_patch(struct regmap *map, const struct reg_default *regs,
 			  int num_regs)
 {
+	unsigned long flags;
 	int i, ret;
 	bool bypass;
 
@@ -1759,7 +1768,7 @@ int regmap_register_patch(struct regmap *map, const struct reg_default *regs,
 	if (map->patch)
 		return -EBUSY;
 
-	map->lock(map->lock_arg);
+	map->lock(map->lock_arg, &flags);
 
 	bypass = map->cache_bypass;
 
@@ -1787,7 +1796,7 @@ int regmap_register_patch(struct regmap *map, const struct reg_default *regs,
 out:
 	map->cache_bypass = bypass;
 
-	map->unlock(map->lock_arg);
+	map->unlock(map->lock_arg, &flags);
 
 	return ret;
 }
diff --git a/drivers/mfd/sta2x11-mfd.c b/drivers/mfd/sta2x11-mfd.c
index d70a3430..2fe1d36 100644
--- a/drivers/mfd/sta2x11-mfd.c
+++ b/drivers/mfd/sta2x11-mfd.c
@@ -154,20 +154,20 @@ EXPORT_SYMBOL(sta2x11_mfd_get_regs_data);
  * Special sta2x11-mfd regmap lock/unlock functions
  */
 
-static void sta2x11_regmap_lock(void *__lock)
+static void sta2x11_regmap_lock(void *__lock, unsigned long *flags)
 {
 	spinlock_t *lock = __lock;
 	spin_lock(lock);
 }
 
-static void sta2x11_regmap_unlock(void *__lock)
+static void sta2x11_regmap_unlock(void *__lock, unsigned long *flags)
 {
 	spinlock_t *lock = __lock;
 	spin_unlock(lock);
 }
 
 /* OTP (one time programmable registers do not require locking */
-static void sta2x11_regmap_nolock(void *__lock)
+static void sta2x11_regmap_nolock(void *__lock, unsigned long *flags)
 {
 }
 
diff --git a/drivers/staging/imx-drm/imx-tve.c b/drivers/staging/imx-drm/imx-tve.c
index ac16344..455aad2 100644
--- a/drivers/staging/imx-drm/imx-tve.c
+++ b/drivers/staging/imx-drm/imx-tve.c
@@ -131,13 +131,13 @@ struct imx_tve {
 	int hsync_pin;
 };
 
-static void tve_lock(void *__tve)
+static void tve_lock(void *__tve, unsigned long *flags)
 {
 	struct imx_tve *tve = __tve;
 	spin_lock(&tve->lock);
 }
 
-static void tve_unlock(void *__tve)
+static void tve_unlock(void *__tve, unsigned long *flags)
 {
 	struct imx_tve *tve = __tve;
 	spin_unlock(&tve->lock);
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index d6f3221..377a589 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -85,8 +85,8 @@ struct regmap_access_table {
 	unsigned int n_no_ranges;
 };
 
-typedef void (*regmap_lock)(void *);
-typedef void (*regmap_unlock)(void *);
+typedef void (*regmap_lock)(void *, unsigned long *flags);
+typedef void (*regmap_unlock)(void *, unsigned long *flags);
 
 /**
  * Configuration for the register map of a device.
-- 
1.8.0

Re: [PATCH 2/2] regmap: Make regmap-mmio usable from different contexts

[Mark Brown]

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

On Thu, May 23, 2013 at 03:06:16PM +0200, Lars-Peter Clausen wrote:

> This patch updates the adds a flags parameter to the regmap lock and unlock
> callbacks and uses spin_lock_irqsave() and spin_unlock_restore() for the mmio
> case. This allows us to use regmap-mmio from different contexts.

This seems really invasive, why not just have the lock that gets passed
in point to a struct which has both the lock and the flags?  As far as
the core is concerned the lock is just whatever data is required to do
the locking, the fact that it's actually two values is an implementation
detail of this locking implementation.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] regmap: Make regmap-mmio usable from different contexts

[Lars-Peter Clausen]

On 05/23/2013 04:05 PM, Mark Brown wrote:
> On Thu, May 23, 2013 at 03:06:16PM +0200, Lars-Peter Clausen wrote:
> 
>> This patch updates the adds a flags parameter to the regmap lock and unlock
>> callbacks and uses spin_lock_irqsave() and spin_unlock_restore() for the mmio
>> case. This allows us to use regmap-mmio from different contexts.
> 
> This seems really invasive, why not just have the lock that gets passed
> in point to a struct which has both the lock and the flags?  As far as
> the core is concerned the lock is just whatever data is required to do
> the locking, the fact that it's actually two values is an implementation
> detail of this locking implementation.

I think that won't work. spin_lock_irqsave() will write to the flags
parameter before it has successfully taken the look. So if a process running
on another CPU tries to acquire the the lock while it is already held we'll
end up overwriting the flags. E.g:

CPU0				CPU1
spin_lock_irqsave()
  - write flags

				spin_lock_irqsave()
				  - overwrite flags

spin_unlock_irqrestore()
  - restore wrong flags

Hence flags needs to go onto the stack.

- Lars

Re: [PATCH 2/2] regmap: Make regmap-mmio usable from different contexts

[Mark Brown]

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

On Thu, May 23, 2013 at 04:20:27PM +0200, Lars-Peter Clausen wrote:
> On 05/23/2013 04:05 PM, Mark Brown wrote:

> > This seems really invasive, why not just have the lock that gets passed
> > in point to a struct which has both the lock and the flags?  As far as
> > the core is concerned the lock is just whatever data is required to do
> > the locking, the fact that it's actually two values is an implementation
> > detail of this locking implementation.

> I think that won't work. spin_lock_irqsave() will write to the flags
> parameter before it has successfully taken the look. So if a process running
> on another CPU tries to acquire the the lock while it is already held we'll
> end up overwriting the flags. E.g:

So you'd have to allocate a struct on the stack with a pointer and the
flags in it and initialise the pointer.  Not awesome but not the end of
the world.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] regmap: Make regmap-mmio usable from different contexts

[Lars-Peter Clausen]

On 05/23/2013 04:31 PM, Mark Brown wrote:
> On Thu, May 23, 2013 at 04:20:27PM +0200, Lars-Peter Clausen wrote:
>> On 05/23/2013 04:05 PM, Mark Brown wrote:
> 
>>> This seems really invasive, why not just have the lock that gets passed
>>> in point to a struct which has both the lock and the flags?  As far as
>>> the core is concerned the lock is just whatever data is required to do
>>> the locking, the fact that it's actually two values is an implementation
>>> detail of this locking implementation.
> 
>> I think that won't work. spin_lock_irqsave() will write to the flags
>> parameter before it has successfully taken the look. So if a process running
>> on another CPU tries to acquire the the lock while it is already held we'll
>> end up overwriting the flags. E.g:
> 
> So you'd have to allocate a struct on the stack with a pointer and the
> flags in it and initialise the pointer.  Not awesome but not the end of
> the world.

I'm not sure I understand what you mean. This needs to be done for each
caller of a lock()/unlock() pair. We can't allocate the flags on the stack
inside the lock() function since the stack will be gone once the function exits.

- Lars

Re: [PATCH 2/2] regmap: Make regmap-mmio usable from different contexts

[Mark Brown]

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

On Thu, May 23, 2013 at 04:36:53PM +0200, Lars-Peter Clausen wrote:
> On 05/23/2013 04:31 PM, Mark Brown wrote:

> > So you'd have to allocate a struct on the stack with a pointer and the
> > flags in it and initialise the pointer.  Not awesome but not the end of
> > the world.

> I'm not sure I understand what you mean. This needs to be done for each
> caller of a lock()/unlock() pair. We can't allocate the flags on the stack
> inside the lock() function since the stack will be gone once the function exits.

Allocate the struct on the stack - if you can allocate flags on the
stack you can allocate the struct on the stack too.  Though I suppose
you still need to change all the callers for this which is just nasty
and makes it seem pointless to bother with abstraction here.

This is all stuff you should've been going through in the changelog when
you're jumping off into something that involves changing every single
user...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] regmap: Make regmap-mmio usable from different contexts

[Stephen Warren]

On 05/23/2013 07:06 AM, Lars-Peter Clausen wrote:
> regmap-mmio uses a spinlock with spin_lock() and spin_unlock() for locking.
> Which means in order to avoid race conditions the lock always needs to be taken
> from the same context.

I'm not really sure what this means. I assume contexts are
atomic-vs-nonatomic? If so, spinlocks should work fine for this, right?

I guess the core of the issue is that you want to replace spin_lock()
with spin_lock_irqsave(). I'd like to see that explicitly described in
the commit description, if that is the core aspect of this change.

Re: the other comments about the API change: I think this can be done
non-invasively:

static void regmap_lock_spinlock(void *__map)
{
	struct regmap *map = __map;
	unsigned long local_flags;

	spin_lock_irqsave(&map->spinlock, local_flags);
	/*
	 * Here, we have the lock locked, so we own the flags,
	 * and can write to them.
	 */
	map->spinlock_flags = local_flags;
}

static void regmap_unlock_spinlock(void *__map, unsigned long *flags)
{
	struct regmap *map = __map;
	spin_unlock_irqrestore(&map->spinlock, map->spinlock_flags);
}

... and obviously add a spinlock_flags field to struct regmap (perhaps
start unioning the mutex and spinlock data fields there if you want to
save space).

Re: [PATCH 2/2] regmap: Make regmap-mmio usable from different contexts

[Lars-Peter Clausen]

On 05/23/2013 05:42 PM, Stephen Warren wrote:
> On 05/23/2013 07:06 AM, Lars-Peter Clausen wrote:
>> regmap-mmio uses a spinlock with spin_lock() and spin_unlock() for locking.
>> Which means in order to avoid race conditions the lock always needs to be taken
>> from the same context.
> 
> I'm not really sure what this means. I assume contexts are
> atomic-vs-nonatomic?

Yes.

> If so, spinlocks should work fine for this, right?

No, you have to take special care if you want to take the same spinlock from
different contexts. And you have to take even more care if the code that
takes the lock can run in different contexts.

> 
> I guess the core of the issue is that you want to replace spin_lock()
> with spin_lock_irqsave(). I'd like to see that explicitly described in
> the commit description, if that is the core aspect of this change.

Hm, it does.

	regmap-mmio uses a spinlock with spin_lock() and spin_unlock() for
	locking.
	...
	This patch updates the adds a flags parameter to the regmap lock
	and unlock callbacks and uses spin_lock_irqsave() and
	spin_unlock_restore() ...


> 
> Re: the other comments about the API change: I think this can be done
> non-invasively:
> 
> static void regmap_lock_spinlock(void *__map)
> {
> 	struct regmap *map = __map;
> 	unsigned long local_flags;
> 
> 	spin_lock_irqsave(&map->spinlock, local_flags);
> 	/*
> 	 * Here, we have the lock locked, so we own the flags,
> 	 * and can write to them.
> 	 */
> 	map->spinlock_flags = local_flags;
> }
> 
> static void regmap_unlock_spinlock(void *__map, unsigned long *flags)
> {
> 	struct regmap *map = __map;
> 	spin_unlock_irqrestore(&map->spinlock, map->spinlock_flags);
> }
> 
> ... and obviously add a spinlock_flags field to struct regmap (perhaps
> start unioning the mutex and spinlock data fields there if you want to
> save space).

Hm, that might work.

Re: [PATCH 2/2] regmap: Make regmap-mmio usable from different contexts

[Stephen Warren]

On 05/23/2013 09:50 AM, Lars-Peter Clausen wrote:
> On 05/23/2013 05:42 PM, Stephen Warren wrote:
>> On 05/23/2013 07:06 AM, Lars-Peter Clausen wrote:
>>> regmap-mmio uses a spinlock with spin_lock() and spin_unlock() for locking.
>>> Which means in order to avoid race conditions the lock always needs to be taken
>>> from the same context.
>>
>> I'm not really sure what this means. I assume contexts are
>> atomic-vs-nonatomic?
> 
> Yes.
> 
>> If so, spinlocks should work fine for this, right?
> 
> No, you have to take special care if you want to take the same spinlock from
> different contexts. And you have to take even more care if the code that
> takes the lock can run in different contexts.

OK, but what is that "special care" that must be taken? I assume from
this patch, you mean using spin_lock_irqsave() rather than spin_lock().
So the issue isn't so much that spin locks are used, but rather how
they're used. The code still has a spin_lock_t before and after...

>> I guess the core of the issue is that you want to replace spin_lock()
>> with spin_lock_irqsave(). I'd like to see that explicitly described in
>> the commit description, if that is the core aspect of this change.
> 
> Hm, it does.
> 
> 	regmap-mmio uses a spinlock with spin_lock() and spin_unlock() for
> 	locking.
> 	...
> 	This patch updates the adds a flags parameter to the regmap lock
> 	and unlock callbacks and uses spin_lock_irqsave() and
> 	spin_unlock_restore() ...

To me, that doesn't make me realize that the core part of this change is
to switch between the two different APIs that operate on the spin lock.
I'd expect the commit description to say something more like:

regmap-mmio uses a spinlock for locking. In order for this to work
correctly from different contexts (atomic vs non-atomic), this spinlock
must be locked using spin_lock_irqsave() rather than spin_lock(). To
support this, a spinlock_flags field is added to struct regmap to store
the flags between regmap_{,un}lock_spinlock().

Re: [PATCH 2/2] regmap: Make regmap-mmio usable from different contexts

[Mark Brown]

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

On Thu, May 23, 2013 at 05:50:15PM +0200, Lars-Peter Clausen wrote:
> On 05/23/2013 05:42 PM, Stephen Warren wrote:

> > I guess the core of the issue is that you want to replace spin_lock()
> > with spin_lock_irqsave(). I'd like to see that explicitly described in
> > the commit description, if that is the core aspect of this change.

> Hm, it does.

> 	regmap-mmio uses a spinlock with spin_lock() and spin_unlock() for
> 	locking.
> 	...
> 	This patch updates the adds a flags parameter to the regmap lock
> 	and unlock callbacks and uses spin_lock_irqsave() and
> 	spin_unlock_restore() ...

That's pretty buried in the changelog though - really all the first
paragraph needs to say is that we want to use the irqsave versions to
allow interaction with interrupt context.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] regmap: Make regmap-mmio usable from different contexts

[Mark Brown]

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

On Thu, May 23, 2013 at 09:42:37AM -0600, Stephen Warren wrote:

> I guess the core of the issue is that you want to replace spin_lock()
> with spin_lock_irqsave(). I'd like to see that explicitly described in
> the commit description, if that is the core aspect of this change.

Yes, I was just going to rewrite the commit log to make this a lot
clearer - the entire first paragraph could pretty much just be replaced
by just saying that we need spin_lock_irqsave() to allow interaction
with interrupt context.

> ... and obviously add a spinlock_flags field to struct regmap (perhaps
> start unioning the mutex and spinlock data fields there if you want to
> save space).

This should work, yes - care to respin Lars-Peter?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] regmap: regcache: Fixup locking for custom lock callbacks

[Mark Brown]

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

On Thu, May 23, 2013 at 03:06:15PM +0200, Lars-Peter Clausen wrote:
> The parameter passed to the regmap lock/unlock callbacks needs to be
> map->lock_arg, regcache passes just map. This works fine in the case that no
> custom locking callbacks are used, since in this case map->lock_arg equals map,
> but will break when custom locking callbacks are used. The issue was introduced
> in commit 0d4529c5 ("regmap: make lock/unlock functions customizable") and is
> fixed by this patch.

I've applied this but can you please also generate a version that
applies against v3.10-rc2 since this should really go in as a bug fix
but the patch depends on recent changes in the regcache code.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]