[PATCH] regmap: don't use spinlocks with REGCACHE_{RBTREE,COMPRESSED}

[PATCH] regmap: don't use spinlocks with REGCACHE_{RBTREE,COMPRESSED}

[Andrew Bresticker]

Both REGCACHE_RBTREE and REGCACHE_COMPRESSED make GFP_KERNEL allocations
with the regmap lock held.  If we're initializing a regmap which would
normally use a spinlock (e.g. MMIO), fall back to using a mutex if one
of these caching types is to be used.

Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
---
 drivers/base/regmap/regmap.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 2b4e72f..fb618d2 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -438,8 +438,9 @@ struct regmap *regmap_init(struct device *dev,
 		map->unlock = config->unlock;
 		map->lock_arg = config->lock_arg;
 	} else {
-		if ((bus && bus->fast_io) ||
-		    config->fast_io) {
+		if (((bus && bus->fast_io) || config->fast_io) &&
+		    config->cache_type != REGCACHE_RBTREE &&
+		    config->cache_type != REGCACHE_COMPRESSED) {
 			spin_lock_init(&map->spinlock);
 			map->lock = regmap_lock_spinlock;
 			map->unlock = regmap_unlock_spinlock;
-- 
1.9.0.rc1.175.g0b1dcb5

Re: [PATCH] regmap: don't use spinlocks with REGCACHE_{RBTREE,COMPRESSED}

[Mark Brown]

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

On Wed, Feb 26, 2014 at 07:50:57PM -0800, Andrew Bresticker wrote:
> Both REGCACHE_RBTREE and REGCACHE_COMPRESSED make GFP_KERNEL allocations
> with the regmap lock held.  If we're initializing a regmap which would
> normally use a spinlock (e.g. MMIO), fall back to using a mutex if one
> of these caching types is to be used.

Have all the users been audited to verify that they're actually safe
with this?  I just took a quick look at the Tegra drivers and they're
doing regmap operations in their trigger operations which is done from
atomic context so they should run into problems trying to take mutexes
anyway.

I think we need to either ensure that all users allocate their caches at
probe time (which is fine and is hopefully what the current users are
doing), provide a mechanism for them to do cache allocations outside of
the spinlock (which sounds hairy) or convert them to flat cache.

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

Re: [PATCH] regmap: don't use spinlocks with REGCACHE_{RBTREE,COMPRESSED}

[Andrew Bresticker]

On Thu, Feb 27, 2014 at 3:37 AM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Feb 26, 2014 at 07:50:57PM -0800, Andrew Bresticker wrote:
>> Both REGCACHE_RBTREE and REGCACHE_COMPRESSED make GFP_KERNEL allocations
>> with the regmap lock held.  If we're initializing a regmap which would
>> normally use a spinlock (e.g. MMIO), fall back to using a mutex if one
>> of these caching types is to be used.
>
> Have all the users been audited to verify that they're actually safe
> with this?  I just took a quick look at the Tegra drivers and they're
> doing regmap operations in their trigger operations which is done from
> atomic context so they should run into problems trying to take mutexes
> anyway.

Oops, you're right, I didn't look through all the regmap operations
carefully enough.  It is in fact the Tegra drivers I've ran into this
issue with.

> I think we need to either ensure that all users allocate their caches at
> probe time (which is fine and is hopefully what the current users are
> doing), provide a mechanism for them to do cache allocations outside of
> the spinlock (which sounds hairy) or convert them to flat cache.

Allocations are made in the write path for rbtree and in both the read
and write paths for lzo.  I suppose we can pre-allocate everything at
init time, but I haven't looked too much at that.  For at least the
Tegra drivers, the best option for now appears to be to convert to a
flat cache.

Thanks,
Andrew

Re: [PATCH] regmap: don't use spinlocks with REGCACHE_{RBTREE,COMPRESSED}

[Mark Brown]

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

On Thu, Feb 27, 2014 at 06:32:53PM -0800, Andrew Bresticker wrote:
> On Thu, Feb 27, 2014 at 3:37 AM, Mark Brown <broonie@kernel.org> wrote:

> > Have all the users been audited to verify that they're actually safe
> > with this?  I just took a quick look at the Tegra drivers and they're
> > doing regmap operations in their trigger operations which is done from
> > atomic context so they should run into problems trying to take mutexes
> > anyway.

> Oops, you're right, I didn't look through all the regmap operations
> carefully enough.  It is in fact the Tegra drivers I've ran into this
> issue with.

Yeah, I suspected it might be :)

> > I think we need to either ensure that all users allocate their caches at
> > probe time (which is fine and is hopefully what the current users are
> > doing), provide a mechanism for them to do cache allocations outside of
> > the spinlock (which sounds hairy) or convert them to flat cache.

> Allocations are made in the write path for rbtree and in both the read
> and write paths for lzo.  I suppose we can pre-allocate everything at
> init time, but I haven't looked too much at that.  For at least the
> Tegra drivers, the best option for now appears to be to convert to a
> flat cache.

OK, that's certainly simple to implement.  Currently to allocate at
startup the standard thing is to supply the hardware defaults as data,
though going through and reading everything should also do the trick.

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