[RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Nicolas Boichat]

From: Antti Palosaari <crope@iki.fi>

Lockdep validator complains about recursive locking and deadlock
when two different regmap instances are called in a nested order.
That happens anytime a regmap read/write call needs to access
another regmap.

This is because, for performance reason, lockdep groups all locks
initialized by the same mutex_init() in the same lock class.
Therefore all regmap mutexes are in the same lock class, leading
to lockdep "nested locking" warnings if a regmap accesses another
regmap. However, depending on the specifics of the driver, this
can be perfectly safe (e.g. if there is a clear hierarchy between
a "master" regmap that uses another "slave" regmap). In these
cases, the warning is false and should be silenced.

As a solution, add configuration option to pass custom lock class
key for lockdep validator, to be used in the regmap that needs to
access another regmap. This removes the need for uglier workarounds
in drivers, just to silence this warning (e.g. add custom mutex
lock/unlock functions).

Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Antti Palosaari <crope@iki.fi>
[drinkcat@chromium.org: Reworded the commit message and regmap.h
documentation]
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
---

I'm trying to revive Antti's patch [1], as we are hitting similar issue
with rt5677 driver. I only updated the commit message and documentation,
I kept both Signed-off-by and From lines, with a small note highlighting
my changes, let me know if that's not appropriate.

One issue that was raised by Mark at the time is that a per-regmap_config
lock class might not be enough (Mark mentioned clocks as an example). The
current implementation should be good enough as long as the clock regmaps
do not access each other. If this is a problem, maybe we should consider
replacing lockdep_lock_class_key by a boolean use_own_lock_class that would
allocate/use a per regmap instance lock class, or have devm_regmap_init
take an extra parameter specifying the lock class.

[1] https://www.mail-archive.com/linux-media%40vger.kernel.org/msg83490.html

 drivers/base/regmap/regmap.c | 3 +++
 include/linux/regmap.h       | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 6273ff0..f5d1131 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -560,6 +560,9 @@ struct regmap *regmap_init(struct device *dev,
 			mutex_init(&map->mutex);
 			map->lock = regmap_lock_mutex;
 			map->unlock = regmap_unlock_mutex;
+			if (config->lockdep_lock_class_key)
+				lockdep_set_class(&map->mutex,
+						config->lockdep_lock_class_key);
 		}
 		map->lock_arg = map;
 	}
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 116655d..09aaaf5 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -135,6 +135,12 @@ typedef void (*regmap_unlock)(void *);
  * @lock_arg:	  this field is passed as the only argument of lock/unlock
  *		  functions (ignored in case regular lock/unlock functions
  *		  are not overridden).
+ * @lock_class_key: Custom lock class key for lockdep validator. Use that to
+ *                  silence false lockdep nested locking warning, when this
+ *                  regmap needs to access another regmap during read/write
+ *                  operations (directly in read/write functions, or
+ *                  indirectly, e.g. through bus accesses).
+ *                  Valid only when regmap default mutex locking is used.
  * @reg_read:	  Optional callback that if filled will be used to perform
  *           	  all the reads from the registers. Should only be provided for
  *		  devices whose read operation cannot be represented as a simple
@@ -198,6 +204,7 @@ struct regmap_config {
 	regmap_lock lock;
 	regmap_unlock unlock;
 	void *lock_arg;
+	struct lock_class_key *lockdep_lock_class_key;
 
 	int (*reg_read)(void *context, unsigned int reg, unsigned int *val);
 	int (*reg_write)(void *context, unsigned int reg, unsigned int val);
-- 
2.4.3.573.g4eafbef

[RFC PATCH 2/2] ASoC: rt5677: Add lockdep class to silence lockdep warnings

[Nicolas Boichat]

Accessing rt5677->regmap requires read/write operations on
another regmap (rt5677->regmap_physical), which causes the
lockdep detector to throw a false warning, as both regmaps
are using the same lockdep class by default. Introduce a new
class for rt5677->regmap to silence this warning.

Sample warning:
[    2.569449] =============================================
[    2.569451] [ INFO: possible recursive locking detected ]
[    2.569454] 3.18.0 #311 Tainted: G S
[    2.569456] ---------------------------------------------
[    2.569458] swapper/0/1 is trying to acquire lock:
[    2.569469]  (&map->mutex){+.+...}, at: [<ffffffc00037dba0>]
regmap_lock_mutex+0x10/0x18
[    2.569470]
[    2.569470] but task is already holding lock:
[    2.569476]  (&map->mutex){+.+...}, at: [<ffffffc00037dba0>]
regmap_lock_mutex+0x10/0x18
[    2.569478]
[    2.569478] other info that might help us debug this:
[    2.569479]  Possible unsafe locking scenario:
[    2.569479]
[    2.569480]        CPU0
[    2.569481]        ----
[    2.569484]   lock(&map->mutex);
[    2.569486]   lock(&map->mutex);
[    2.569487]
[    2.569487]  *** DEADLOCK ***
[    2.569487]
[    2.569489]  May be due to missing lock nesting notation
[    2.569489]
[    2.569491] 3 locks held by swapper/0/1:
[    2.569499]  #0:  (&dev->mutex){......}, at: [<ffffffc000369e80>]
__driver_attach+0x38/0x98
[    2.569505]  #1:  (&dev->mutex){......}, at: [<ffffffc000369ea0>]
__driver_attach+0x58/0x98
[    2.569512]  #2:  (&map->mutex){+.+...}, at: [<ffffffc00037dba0>]
regmap_lock_mutex+0x10/0x18
[    2.569513]
[    2.569513] stack backtrace:
[    2.569517] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G S
3.18.0 #311
[    2.569521] Call trace:
[    2.569527] [<ffffffc000089198>] dump_backtrace+0x0/0x108
[    2.569530] [<ffffffc0000892b0>] show_stack+0x10/0x1c
[    2.569535] [<ffffffc0005e09b4>] dump_stack+0x74/0x94
[    2.569540] [<ffffffc0000d5d1c>] __lock_acquire+0x73c/0x1910
[    2.569544] [<ffffffc0000d7674>] lock_acquire+0xec/0x128
[    2.569548] [<ffffffc0005e3870>] mutex_lock_nested+0x58/0x354
[    2.569551] [<ffffffc00037db9c>] regmap_lock_mutex+0xc/0x18
[    2.569554] [<ffffffc00037ef50>] regmap_read+0x34/0x68
[    2.569559] [<ffffffc0004ddc84>] rt5677_read+0x9c/0xb4
[    2.569562] [<ffffffc00037ee8c>] _regmap_read+0xa8/0x138
[    2.569565] [<ffffffc00037ef60>] regmap_read+0x44/0x68
[    2.569569] [<ffffffc0004dd6a4>] rt5677_i2c_probe+0x25c/0x4a4
[    2.569574] [<ffffffc00042984c>] i2c_device_probe+0xfc/0x130
[    2.569577] [<ffffffc000369c58>] driver_probe_device+0xd4/0x23c
[    2.569580] [<ffffffc000369eb0>] __driver_attach+0x68/0x98
[    2.569584] [<ffffffc000368dc8>] bus_for_each_dev+0x70/0x90
[    2.569588] [<ffffffc0003697e4>] driver_attach+0x1c/0x28
[    2.569591] [<ffffffc000369400>] bus_add_driver+0xd8/0x1e0
[    2.569594] [<ffffffc00036a7f0>] driver_register+0xbc/0x10c
[    2.569598] [<ffffffc00042a4bc>] i2c_register_driver+0x48/0xac
[    2.569601] [<ffffffc0008d523c>] rt5677_i2c_driver_init+0x14/0x20
[    2.569605] [<ffffffc0000828dc>] do_one_initcall+0xf4/0x18c
[    2.569609] [<ffffffc0008a4ae8>] kernel_init_freeable+0x144/0x1e4
[    2.569613] [<ffffffc0005de3a4>] kernel_init+0x10/0xd4

Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
---
 sound/soc/codecs/rt5677.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index 31d969a..3007d13 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -4998,6 +4998,7 @@ static const struct regmap_config rt5677_regmap_physical = {
 	.num_ranges = ARRAY_SIZE(rt5677_ranges),
 };
 
+static struct lock_class_key rt5677_regmap_key;
 static const struct regmap_config rt5677_regmap = {
 	.reg_bits = 8,
 	.val_bits = 16,
@@ -5015,6 +5016,10 @@ static const struct regmap_config rt5677_regmap = {
 	.num_reg_defaults = ARRAY_SIZE(rt5677_reg),
 	.ranges = rt5677_ranges,
 	.num_ranges = ARRAY_SIZE(rt5677_ranges),
+
+	/* Silence incorrect lockdep warnings, as rt5677_regmap_physical is
+	 * accessed from within rt5677_regmap. */
+	.lockdep_lock_class_key = &rt5677_regmap_key,
 };
 
 static const struct i2c_device_id rt5677_i2c_id[] = {
-- 
2.4.3.573.g4eafbef

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Arjan van de Ven]

On 6/25/2015 2:35 AM, Nicolas Boichat wrote:
> From: Antti Palosaari <crope@iki.fi>
>
> Lockdep validator complains about recursive locking and deadlock
> when two different regmap instances are called in a nested order.
> That happens anytime a regmap read/write call needs to access
> another regmap.
>
> This is because, for performance reason, lockdep groups all locks
> initialized by the same mutex_init() in the same lock class.
> Therefore all regmap mutexes are in the same lock class, leading
> to lockdep "nested locking" warnings if a regmap accesses another
> regmap. However, depending on the specifics of the driver, this
> can be perfectly safe (e.g. if there is a clear hierarchy between
> a "master" regmap that uses another "slave" regmap). In these
> cases, the warning is false and should be silenced.
>
> As a solution, add configuration option to pass custom lock class
> key for lockdep validator, to be used in the regmap that needs to
> access another regmap. This removes the need for uglier workarounds
> in drivers, just to silence this warning (e.g. add custom mutex
> lock/unlock functions).

wouldn't it be better to use the mutex_lock_nested() and co to explicitly express your hierarchy?

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Mark Brown]

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

On Thu, Jun 25, 2015 at 06:21:43AM -0700, Arjan van de Ven wrote:

Please fix your mailer to word wrap within paragraphs.

> wouldn't it be better to use the mutex_lock_nested() and co to
> explicitly express your hierarchy?

That was one of my original suggestions - one of the problems with this
code is that it's very much been presented as "here's the solution", it
took a long time to even discover the problem they were trying to solve.
I don't know exactly what the issue is supposed to be here, AFAICT the
answers were about lock classes not about subclasses because apparently
it's essential that we use lock classes.

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

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Lars-Peter Clausen]

On 06/25/2015 03:21 PM, Arjan van de Ven wrote:
> On 6/25/2015 2:35 AM, Nicolas Boichat wrote:
>> From: Antti Palosaari <crope@iki.fi>
>>
>> Lockdep validator complains about recursive locking and deadlock
>> when two different regmap instances are called in a nested order.
>> That happens anytime a regmap read/write call needs to access
>> another regmap.
>>
>> This is because, for performance reason, lockdep groups all locks
>> initialized by the same mutex_init() in the same lock class.
>> Therefore all regmap mutexes are in the same lock class, leading
>> to lockdep "nested locking" warnings if a regmap accesses another
>> regmap. However, depending on the specifics of the driver, this
>> can be perfectly safe (e.g. if there is a clear hierarchy between
>> a "master" regmap that uses another "slave" regmap). In these
>> cases, the warning is false and should be silenced.
>>
>> As a solution, add configuration option to pass custom lock class
>> key for lockdep validator, to be used in the regmap that needs to
>> access another regmap. This removes the need for uglier workarounds
>> in drivers, just to silence this warning (e.g. add custom mutex
>> lock/unlock functions).
>
> wouldn't it be better to use the mutex_lock_nested() and co to explicitly
> express your hierarchy?

That would require that the hierarchy is known in advance. The hierarchy 
depends on the hardware topology. Different systems will have different 
hierarchies where the relationship between locks can change and it will be 
hard to find a hierarchy that works across all topologies.

A simple hierarchy would be to say one lockdep subclass is bus masters and 
one lockdep subclass are bus slaves. E.g. the SPI bus controller gets the 
master subclass and the device on the SPI bus gets the slave subclass.

The problem is that a device that is a bus master on one bus that uses 
regmap can be a slave on a different bus that uses regmap. And this can be 
nested arbitrarily deep.

E.g. the rt5677, for which Nicolas is trying to solve the problem is, slave 
device on a I2C bus, but also has an internal bus, which is used to access 
the DSP, for which it is the master.

- Lars

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Lars-Peter Clausen]

On 06/25/2015 03:21 PM, Arjan van de Ven wrote:
> On 6/25/2015 2:35 AM, Nicolas Boichat wrote:
>> From: Antti Palosaari <crope@iki.fi>
>>
>> Lockdep validator complains about recursive locking and deadlock
>> when two different regmap instances are called in a nested order.
>> That happens anytime a regmap read/write call needs to access
>> another regmap.
>>
>> This is because, for performance reason, lockdep groups all locks
>> initialized by the same mutex_init() in the same lock class.
>> Therefore all regmap mutexes are in the same lock class, leading
>> to lockdep "nested locking" warnings if a regmap accesses another
>> regmap. However, depending on the specifics of the driver, this
>> can be perfectly safe (e.g. if there is a clear hierarchy between
>> a "master" regmap that uses another "slave" regmap). In these
>> cases, the warning is false and should be silenced.
>>
>> As a solution, add configuration option to pass custom lock class
>> key for lockdep validator, to be used in the regmap that needs to
>> access another regmap. This removes the need for uglier workarounds
>> in drivers, just to silence this warning (e.g. add custom mutex
>> lock/unlock functions).
>
> wouldn't it be better to use the mutex_lock_nested() and co to explicitly
> express your hierarchy?

That would require that the hierarchy is known in advance. The hierarchy 
depends on the hardware topology. Different systems will have different 
hierarchies where the relationship between locks can change and it will be 
hard to find a hierarchy that works across all topologies.

A simple hierarchy would be to say one lockdep subclass is bus masters and 
one lockdep subclass are bus slaves. E.g. the SPI bus controller gets the 
master subclass and the device on the SPI bus gets the slave subclass.

The problem is that a device that is a bus master on one bus that uses 
regmap can be a slave on a different bus that uses regmap. And this can be 
nested arbitrarily deep.

E.g. the rt5677, for which Nicolas is trying to solve the problem is, slave 
device on a I2C bus, but also has an internal bus, which is used to access 
the DSP, for which it is the master.

- Lars

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Mark Brown]

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

On Thu, Jun 25, 2015 at 05:03:00PM +0200, Lars-Peter Clausen wrote:
> On 06/25/2015 03:21 PM, Arjan van de Ven wrote:

> >wouldn't it be better to use the mutex_lock_nested() and co to explicitly
> >express your hierarchy?

> That would require that the hierarchy is known in advance. The hierarchy
> depends on the hardware topology. Different systems will have different
> hierarchies where the relationship between locks can change and it will be
> hard to find a hierarchy that works across all topologies.

It depends on what you use as the key for the nested locking stuff.  If
you assign a key per regmap (casting the pointer to an integer, using an
IDR or something).  I don't know if that creates problems for the
locking code, I'd not expect so but then I'd not have expected the
problem in the first place.

As far as I can tell we're likely to end up needing a key per regmap or
something similar.

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

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Lars-Peter Clausen]

On 06/25/2015 05:33 PM, Mark Brown wrote:
> On Thu, Jun 25, 2015 at 05:03:00PM +0200, Lars-Peter Clausen wrote:
>> On 06/25/2015 03:21 PM, Arjan van de Ven wrote:
>
>>> wouldn't it be better to use the mutex_lock_nested() and co to explicitly
>>> express your hierarchy?
>
>> That would require that the hierarchy is known in advance. The hierarchy
>> depends on the hardware topology. Different systems will have different
>> hierarchies where the relationship between locks can change and it will be
>> hard to find a hierarchy that works across all topologies.
>
> It depends on what you use as the key for the nested locking stuff.  If
> you assign a key per regmap (casting the pointer to an integer, using an
> IDR or something).  I don't know if that creates problems for the
> locking code, I'd not expect so but then I'd not have expected the
> problem in the first place.

The maximum number of subclasses is 8 per lockclass, so a IDR that 
increments which each created regmap instance wouldn't really work.

And while on the other hand we probably wont have a hierarchy deeper than 8 
nested regmap instances it is not trivial to figure out which instance is at 
which level.

>
> As far as I can tell we're likely to end up needing a key per regmap or
> something similar.
>

Since the number of lockdep classes itself is also limited we should avoid 
creating extra lockdep classes when we can. I think the approach which 
having the option of specifying a lockdep class in the regmap config will be 
ok. The only case it can't handle if we nest instances with the same config, 
but I don't really see valid use scases for that at the moment.

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Lars-Peter Clausen]

On 06/25/2015 05:33 PM, Mark Brown wrote:
> On Thu, Jun 25, 2015 at 05:03:00PM +0200, Lars-Peter Clausen wrote:
>> On 06/25/2015 03:21 PM, Arjan van de Ven wrote:
>
>>> wouldn't it be better to use the mutex_lock_nested() and co to explicitly
>>> express your hierarchy?
>
>> That would require that the hierarchy is known in advance. The hierarchy
>> depends on the hardware topology. Different systems will have different
>> hierarchies where the relationship between locks can change and it will be
>> hard to find a hierarchy that works across all topologies.
>
> It depends on what you use as the key for the nested locking stuff.  If
> you assign a key per regmap (casting the pointer to an integer, using an
> IDR or something).  I don't know if that creates problems for the
> locking code, I'd not expect so but then I'd not have expected the
> problem in the first place.

The maximum number of subclasses is 8 per lockclass, so a IDR that 
increments which each created regmap instance wouldn't really work.

And while on the other hand we probably wont have a hierarchy deeper than 8 
nested regmap instances it is not trivial to figure out which instance is at 
which level.

>
> As far as I can tell we're likely to end up needing a key per regmap or
> something similar.
>

Since the number of lockdep classes itself is also limited we should avoid 
creating extra lockdep classes when we can. I think the approach which 
having the option of specifying a lockdep class in the regmap config will be 
ok. The only case it can't handle if we nest instances with the same config, 
but I don't really see valid use scases for that at the moment.

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Mark Brown]

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

On Thu, Jun 25, 2015 at 05:47:41PM +0200, Lars-Peter Clausen wrote:
> On 06/25/2015 05:33 PM, Mark Brown wrote:

> >It depends on what you use as the key for the nested locking stuff.  If
> >you assign a key per regmap (casting the pointer to an integer, using an
> >IDR or something).  I don't know if that creates problems for the
> >locking code, I'd not expect so but then I'd not have expected the
> >problem in the first place.

> The maximum number of subclasses is 8 per lockclass, so a IDR that
> increments which each created regmap instance wouldn't really work.

Oh, fail.  Yeah, subclasses just won't work at all here and we need full
classes.

> And while on the other hand we probably wont have a hierarchy deeper than 8
> nested regmap instances it is not trivial to figure out which instance is at
> which level.

Yes, indeed.

> >As far as I can tell we're likely to end up needing a key per regmap or
> >something similar.

> Since the number of lockdep classes itself is also limited we should avoid
> creating extra lockdep classes when we can. I think the approach which
> having the option of specifying a lockdep class in the regmap config will be
> ok. The only case it can't handle if we nest instances with the same config,
> but I don't really see valid use scases for that at the moment.

Oh, ffs.  This just keeps getting better.  I hadn't been aware of that
limitation.  We still have the problem that this needs to be something
users can understand rather than something that's just "define something
here in one of your drivers if you're running into problems with
spurious warnings" here.  That's always been the biggest problem here
(once we got past the "what is this supposed to do in the first place?"
issues).

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

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Nicolas Boichat]

On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <broonie@kernel.org> wrote:
[...]
>> >As far as I can tell we're likely to end up needing a key per regmap or
>> >something similar.
>
>> Since the number of lockdep classes itself is also limited we should avoid
>> creating extra lockdep classes when we can. I think the approach which
>> having the option of specifying a lockdep class in the regmap config will be
>> ok. The only case it can't handle if we nest instances with the same config,
>> but I don't really see valid use scases for that at the moment.
>
> Oh, ffs.  This just keeps getting better.  I hadn't been aware of that
> limitation.  We still have the problem that this needs to be something
> users can understand rather than something that's just "define something
> here in one of your drivers if you're running into problems with
> spurious warnings" here.  That's always been the biggest problem here
> (once we got past the "what is this supposed to do in the first place?"
> issues).

I found that V4L2 uses separate lockdep classes for each of their
v4l2_ctrl. This was introduced in 6cd247ef22e "[media] v4l2-ctrls:
eliminate lockdep false alarms for struct v4l2_ctrl_handler.lock"
(https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6cd247ef22e),
so we could possibly take that approach.

On my system, I have:
# cat /proc/lockdep_stats
 lock-classes:                         1241 [max: 8191]
 direct dependencies:                  7364 [max: 32768]
 indirect dependencies:               27686
 all direct dependencies:            158097
 dependency chains:                   10011 [max: 65536]
 dependency chain hlocks:             38887 [max: 327680]
 in-hardirq chains:                      92
 in-softirq chains:                     372
 in-process chains:                    9547
 stack-trace entries:                107703 [max: 524288]

So, at least on that platform, there is some room to grow...

I'm just afraid that implementing this may require creating a bunch of
macros to wrap all regmap_init_[i2c/spi/...] functions, as the lockdep
classes need to be statically allocated... Unless we find a different
solution than what V4L2 does.

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Nicolas Boichat]

On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <broonie@kernel.org> wrote:
[...]
>> >As far as I can tell we're likely to end up needing a key per regmap or
>> >something similar.
>
>> Since the number of lockdep classes itself is also limited we should avoid
>> creating extra lockdep classes when we can. I think the approach which
>> having the option of specifying a lockdep class in the regmap config will be
>> ok. The only case it can't handle if we nest instances with the same config,
>> but I don't really see valid use scases for that at the moment.
>
> Oh, ffs.  This just keeps getting better.  I hadn't been aware of that
> limitation.  We still have the problem that this needs to be something
> users can understand rather than something that's just "define something
> here in one of your drivers if you're running into problems with
> spurious warnings" here.  That's always been the biggest problem here
> (once we got past the "what is this supposed to do in the first place?"
> issues).

I found that V4L2 uses separate lockdep classes for each of their
v4l2_ctrl. This was introduced in 6cd247ef22e "[media] v4l2-ctrls:
eliminate lockdep false alarms for struct v4l2_ctrl_handler.lock"
(https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6cd247ef22e),
so we could possibly take that approach.

On my system, I have:
# cat /proc/lockdep_stats
 lock-classes:                         1241 [max: 8191]
 direct dependencies:                  7364 [max: 32768]
 indirect dependencies:               27686
 all direct dependencies:            158097
 dependency chains:                   10011 [max: 65536]
 dependency chain hlocks:             38887 [max: 327680]
 in-hardirq chains:                      92
 in-softirq chains:                     372
 in-process chains:                    9547
 stack-trace entries:                107703 [max: 524288]

So, at least on that platform, there is some room to grow...

I'm just afraid that implementing this may require creating a bunch of
macros to wrap all regmap_init_[i2c/spi/...] functions, as the lockdep
classes need to be statically allocated... Unless we find a different
solution than what V4L2 does.

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Nicolas Boichat]

On Fri, Jun 26, 2015 at 11:16 AM, Nicolas Boichat <drinkcat@chromium.org> wrote:
>
> On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <broonie@kernel.org> wrote:
> [...]
> >> >As far as I can tell we're likely to end up needing a key per regmap or
> >> >something similar.
> >
> >> Since the number of lockdep classes itself is also limited we should avoid
> >> creating extra lockdep classes when we can. I think the approach which
> >> having the option of specifying a lockdep class in the regmap config will be
> >> ok. The only case it can't handle if we nest instances with the same config,
> >> but I don't really see valid use scases for that at the moment.
> >
> > Oh, ffs.  This just keeps getting better.  I hadn't been aware of that
> > limitation.  We still have the problem that this needs to be something
> > users can understand rather than something that's just "define something
> > here in one of your drivers if you're running into problems with
> > spurious warnings" here.  That's always been the biggest problem here
> > (once we got past the "what is this supposed to do in the first place?"
> > issues).
>
> I found that V4L2 uses separate lockdep classes for each of their
> v4l2_ctrl. This was introduced in 6cd247ef22e "[media] v4l2-ctrls:
> eliminate lockdep false alarms for struct v4l2_ctrl_handler.lock"
> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6cd247ef22e),
> so we could possibly take that approach.
>
> On my system, I have:
> # cat /proc/lockdep_stats
>  lock-classes:                         1241 [max: 8191]
>  direct dependencies:                  7364 [max: 32768]
>  indirect dependencies:               27686
>  all direct dependencies:            158097
>  dependency chains:                   10011 [max: 65536]
>  dependency chain hlocks:             38887 [max: 327680]
>  in-hardirq chains:                      92
>  in-softirq chains:                     372
>  in-process chains:                    9547
>  stack-trace entries:                107703 [max: 524288]
>
> So, at least on that platform, there is some room to grow...
>
> I'm just afraid that implementing this may require creating a bunch of
> macros to wrap all regmap_init_[i2c/spi/...] functions, as the lockdep
> classes need to be statically allocated... Unless we find a different
> solution than what V4L2 does.

Following up on this. Lars-Peter's comments also highlights that we
have no good way to figure out which regmap requires a separate maps,
no clear hierarchy we can know about in advance, so we should put each
regmap in its own class.

The main issue is that the keys need to be allocated statically. We
have 2 options to do this:

1. mutex_init and v4l2_ctrl_handler_init solve this issue by being a
preprocessor macro that first allocates a static lock_class_key, then
calls the real init function.
This is not so practical in the case of regmap, as we have 14
different init functions ([devm_]regmap_init[_bus_type]), that would
each require a wrapper.

2. Bus registration takes a different approach
(https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=be871b7e5):
struct bus_type (statically allocated for each bus) has a lock_key
member: "struct lock_class_key lock_key;".
In the context of regmaps, that would mean adding a "lock_key" member
to regmap_config. I did a quick implementation of this idea, and it
seems to work, without modification to the rt5677 driver. The only
issue with this is that regmap_config cannot be const anymore: we'd
need to remove the const specifier in all drivers that use regmaps.

Both alternatives would mean that all regmaps created from 1. the same
line of code, or 2. the same regmap_config, would share the same
class. That may not be an issue, however (do we have an example of
different regmaps created from the same line/config that need to call
each other?), and the custom mutex workaround is still available....

Any preference between a bunch of macros, and adding a non-const
member to regmap_config? Or maybe someone has a better idea?

Thanks!

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Lars-Peter Clausen]

On 06/29/2015 02:51 PM, Nicolas Boichat wrote:
> On Fri, Jun 26, 2015 at 11:16 AM, Nicolas Boichat <drinkcat@chromium.org> wrote:
>>
>> On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <broonie@kernel.org> wrote:
>> [...]
>>>>> As far as I can tell we're likely to end up needing a key per regmap or
>>>>> something similar.
>>>
>>>> Since the number of lockdep classes itself is also limited we should avoid
>>>> creating extra lockdep classes when we can. I think the approach which
>>>> having the option of specifying a lockdep class in the regmap config will be
>>>> ok. The only case it can't handle if we nest instances with the same config,
>>>> but I don't really see valid use scases for that at the moment.
>>>
>>> Oh, ffs.  This just keeps getting better.  I hadn't been aware of that
>>> limitation.  We still have the problem that this needs to be something
>>> users can understand rather than something that's just "define something
>>> here in one of your drivers if you're running into problems with
>>> spurious warnings" here.  That's always been the biggest problem here
>>> (once we got past the "what is this supposed to do in the first place?"
>>> issues).
>>
>> I found that V4L2 uses separate lockdep classes for each of their
>> v4l2_ctrl. This was introduced in 6cd247ef22e "[media] v4l2-ctrls:
>> eliminate lockdep false alarms for struct v4l2_ctrl_handler.lock"
>> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6cd247ef22e),
>> so we could possibly take that approach.
>>
>> On my system, I have:
>> # cat /proc/lockdep_stats
>>   lock-classes:                         1241 [max: 8191]
>>   direct dependencies:                  7364 [max: 32768]
>>   indirect dependencies:               27686
>>   all direct dependencies:            158097
>>   dependency chains:                   10011 [max: 65536]
>>   dependency chain hlocks:             38887 [max: 327680]
>>   in-hardirq chains:                      92
>>   in-softirq chains:                     372
>>   in-process chains:                    9547
>>   stack-trace entries:                107703 [max: 524288]
>>
>> So, at least on that platform, there is some room to grow...
>>
>> I'm just afraid that implementing this may require creating a bunch of
>> macros to wrap all regmap_init_[i2c/spi/...] functions, as the lockdep
>> classes need to be statically allocated... Unless we find a different
>> solution than what V4L2 does.
>
> Following up on this. Lars-Peter's comments also highlights that we
> have no good way to figure out which regmap requires a separate maps,
> no clear hierarchy we can know about in advance, so we should put each
> regmap in its own class.
>
> The main issue is that the keys need to be allocated statically. We
> have 2 options to do this:
>
> 1. mutex_init and v4l2_ctrl_handler_init solve this issue by being a
> preprocessor macro that first allocates a static lock_class_key, then
> calls the real init function.
> This is not so practical in the case of regmap, as we have 14
> different init functions ([devm_]regmap_init[_bus_type]), that would
> each require a wrapper.
>
> 2. Bus registration takes a different approach
> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=be871b7e5):
> struct bus_type (statically allocated for each bus) has a lock_key
> member: "struct lock_class_key lock_key;".
> In the context of regmaps, that would mean adding a "lock_key" member
> to regmap_config. I did a quick implementation of this idea, and it
> seems to work, without modification to the rt5677 driver. The only
> issue with this is that regmap_config cannot be const anymore: we'd
> need to remove the const specifier in all drivers that use regmaps.

Yeah, I though about that as well, but the problem is the regmap_config is 
only valid during regmap_init() and can for example be placed on the stack. 
In which case it won't work anymore.

>
> Both alternatives would mean that all regmaps created from 1. the same
> line of code, or 2. the same regmap_config, would share the same
> class. That may not be an issue, however (do we have an example of
> different regmaps created from the same line/config that need to call
> each other?), and the custom mutex workaround is still available....
>
> Any preference between a bunch of macros, and adding a non-const
> member to regmap_config? Or maybe someone has a better idea?

Maybe we are just over-thinking this and should just add one key to each 
regmap instance. That solves the issue without requiring the any user 
interaction. The only downside is that it might impact the performance of 
lockdep and uses quite a few lock classes. Its probably manageable right now 
but could grow into a problem as regmap adoption further progresses. But 
maybe we can leave the hard work of figuring a better solution to our future 
selves.

- Lars

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Lars-Peter Clausen]

On 06/29/2015 02:51 PM, Nicolas Boichat wrote:
> On Fri, Jun 26, 2015 at 11:16 AM, Nicolas Boichat <drinkcat@chromium.org> wrote:
>>
>> On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <broonie@kernel.org> wrote:
>> [...]
>>>>> As far as I can tell we're likely to end up needing a key per regmap or
>>>>> something similar.
>>>
>>>> Since the number of lockdep classes itself is also limited we should avoid
>>>> creating extra lockdep classes when we can. I think the approach which
>>>> having the option of specifying a lockdep class in the regmap config will be
>>>> ok. The only case it can't handle if we nest instances with the same config,
>>>> but I don't really see valid use scases for that at the moment.
>>>
>>> Oh, ffs.  This just keeps getting better.  I hadn't been aware of that
>>> limitation.  We still have the problem that this needs to be something
>>> users can understand rather than something that's just "define something
>>> here in one of your drivers if you're running into problems with
>>> spurious warnings" here.  That's always been the biggest problem here
>>> (once we got past the "what is this supposed to do in the first place?"
>>> issues).
>>
>> I found that V4L2 uses separate lockdep classes for each of their
>> v4l2_ctrl. This was introduced in 6cd247ef22e "[media] v4l2-ctrls:
>> eliminate lockdep false alarms for struct v4l2_ctrl_handler.lock"
>> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6cd247ef22e),
>> so we could possibly take that approach.
>>
>> On my system, I have:
>> # cat /proc/lockdep_stats
>>   lock-classes:                         1241 [max: 8191]
>>   direct dependencies:                  7364 [max: 32768]
>>   indirect dependencies:               27686
>>   all direct dependencies:            158097
>>   dependency chains:                   10011 [max: 65536]
>>   dependency chain hlocks:             38887 [max: 327680]
>>   in-hardirq chains:                      92
>>   in-softirq chains:                     372
>>   in-process chains:                    9547
>>   stack-trace entries:                107703 [max: 524288]
>>
>> So, at least on that platform, there is some room to grow...
>>
>> I'm just afraid that implementing this may require creating a bunch of
>> macros to wrap all regmap_init_[i2c/spi/...] functions, as the lockdep
>> classes need to be statically allocated... Unless we find a different
>> solution than what V4L2 does.
>
> Following up on this. Lars-Peter's comments also highlights that we
> have no good way to figure out which regmap requires a separate maps,
> no clear hierarchy we can know about in advance, so we should put each
> regmap in its own class.
>
> The main issue is that the keys need to be allocated statically. We
> have 2 options to do this:
>
> 1. mutex_init and v4l2_ctrl_handler_init solve this issue by being a
> preprocessor macro that first allocates a static lock_class_key, then
> calls the real init function.
> This is not so practical in the case of regmap, as we have 14
> different init functions ([devm_]regmap_init[_bus_type]), that would
> each require a wrapper.
>
> 2. Bus registration takes a different approach
> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=be871b7e5):
> struct bus_type (statically allocated for each bus) has a lock_key
> member: "struct lock_class_key lock_key;".
> In the context of regmaps, that would mean adding a "lock_key" member
> to regmap_config. I did a quick implementation of this idea, and it
> seems to work, without modification to the rt5677 driver. The only
> issue with this is that regmap_config cannot be const anymore: we'd
> need to remove the const specifier in all drivers that use regmaps.

Yeah, I though about that as well, but the problem is the regmap_config is 
only valid during regmap_init() and can for example be placed on the stack. 
In which case it won't work anymore.

>
> Both alternatives would mean that all regmaps created from 1. the same
> line of code, or 2. the same regmap_config, would share the same
> class. That may not be an issue, however (do we have an example of
> different regmaps created from the same line/config that need to call
> each other?), and the custom mutex workaround is still available....
>
> Any preference between a bunch of macros, and adding a non-const
> member to regmap_config? Or maybe someone has a better idea?

Maybe we are just over-thinking this and should just add one key to each 
regmap instance. That solves the issue without requiring the any user 
interaction. The only downside is that it might impact the performance of 
lockdep and uses quite a few lock classes. Its probably manageable right now 
but could grow into a problem as regmap adoption further progresses. But 
maybe we can leave the hard work of figuring a better solution to our future 
selves.

- Lars

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Nicolas Boichat]

On Mon, Jun 29, 2015 at 8:59 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 06/29/2015 02:51 PM, Nicolas Boichat wrote:
>>
>> On Fri, Jun 26, 2015 at 11:16 AM, Nicolas Boichat <drinkcat@chromium.org>
>> wrote:
>>>
>>>
>>> On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <broonie@kernel.org> wrote:
>>> [...]
>>>>>>
>>>>>> As far as I can tell we're likely to end up needing a key per regmap
>>>>>> or
>>>>>> something similar.
>>>>
>>>>
>>>>> Since the number of lockdep classes itself is also limited we should
>>>>> avoid
>>>>> creating extra lockdep classes when we can. I think the approach which
>>>>> having the option of specifying a lockdep class in the regmap config
>>>>> will be
>>>>> ok. The only case it can't handle if we nest instances with the same
>>>>> config,
>>>>> but I don't really see valid use scases for that at the moment.
>>>>
>>>>
>>>> Oh, ffs.  This just keeps getting better.  I hadn't been aware of that
>>>> limitation.  We still have the problem that this needs to be something
>>>> users can understand rather than something that's just "define something
>>>> here in one of your drivers if you're running into problems with
>>>> spurious warnings" here.  That's always been the biggest problem here
>>>> (once we got past the "what is this supposed to do in the first place?"
>>>> issues).
>>>
>>>
>>> I found that V4L2 uses separate lockdep classes for each of their
>>> v4l2_ctrl. This was introduced in 6cd247ef22e "[media] v4l2-ctrls:
>>> eliminate lockdep false alarms for struct v4l2_ctrl_handler.lock"
>>>
>>> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6cd247ef22e),
>>> so we could possibly take that approach.
>>>
>>> On my system, I have:
>>> # cat /proc/lockdep_stats
>>>   lock-classes:                         1241 [max: 8191]
>>>   direct dependencies:                  7364 [max: 32768]
>>>   indirect dependencies:               27686
>>>   all direct dependencies:            158097
>>>   dependency chains:                   10011 [max: 65536]
>>>   dependency chain hlocks:             38887 [max: 327680]
>>>   in-hardirq chains:                      92
>>>   in-softirq chains:                     372
>>>   in-process chains:                    9547
>>>   stack-trace entries:                107703 [max: 524288]
>>>
>>> So, at least on that platform, there is some room to grow...
>>>
>>> I'm just afraid that implementing this may require creating a bunch of
>>> macros to wrap all regmap_init_[i2c/spi/...] functions, as the lockdep
>>> classes need to be statically allocated... Unless we find a different
>>> solution than what V4L2 does.
>>
>>
>> Following up on this. Lars-Peter's comments also highlights that we
>> have no good way to figure out which regmap requires a separate maps,
>> no clear hierarchy we can know about in advance, so we should put each
>> regmap in its own class.
>>
>> The main issue is that the keys need to be allocated statically. We
>> have 2 options to do this:
>>
>> 1. mutex_init and v4l2_ctrl_handler_init solve this issue by being a
>> preprocessor macro that first allocates a static lock_class_key, then
>> calls the real init function.
>> This is not so practical in the case of regmap, as we have 14
>> different init functions ([devm_]regmap_init[_bus_type]), that would
>> each require a wrapper.
>>
>> 2. Bus registration takes a different approach
>>
>> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=be871b7e5):
>> struct bus_type (statically allocated for each bus) has a lock_key
>> member: "struct lock_class_key lock_key;".
>> In the context of regmaps, that would mean adding a "lock_key" member
>> to regmap_config. I did a quick implementation of this idea, and it
>> seems to work, without modification to the rt5677 driver. The only
>> issue with this is that regmap_config cannot be const anymore: we'd
>> need to remove the const specifier in all drivers that use regmaps.
>
>
> Yeah, I though about that as well, but the problem is the regmap_config is
> only valid during regmap_init() and can for example be placed on the stack.
> In which case it won't work anymore.

Then we might need to make it a requirement... In any case, lockdep
will throw a warning if the lock_key is allocated on the stack (or
kalloc'ed).

>>
>> Both alternatives would mean that all regmaps created from 1. the same
>> line of code, or 2. the same regmap_config, would share the same
>> class. That may not be an issue, however (do we have an example of
>> different regmaps created from the same line/config that need to call
>> each other?), and the custom mutex workaround is still available....
>>
>> Any preference between a bunch of macros, and adding a non-const
>> member to regmap_config? Or maybe someone has a better idea?
>
>
> Maybe we are just over-thinking this and should just add one key to each
> regmap instance. That solves the issue without requiring the any user
> interaction.

Yes, I agree. What I'm trying to answer above is how to do that, at
least partially. I have no idea how to allocate one class per regmap,
the above only does one class per init call or per regmap_config.

regmap instances are kalloc'ed, so they cannot contain the
lock_class_key, which needs to be statically allocated (in .data).
Another option would be to preallocate a bunch of lock_class_key in
regmap.c, and pick from that, but that's terribly hacky...

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Lars-Peter Clausen]

On 06/29/2015 04:03 PM, Nicolas Boichat wrote:
> On Mon, Jun 29, 2015 at 8:59 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 06/29/2015 02:51 PM, Nicolas Boichat wrote:
>>>
>>> On Fri, Jun 26, 2015 at 11:16 AM, Nicolas Boichat <drinkcat@chromium.org>
>>> wrote:
>>>>
>>>>
>>>> On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <broonie@kernel.org> wrote:
>>>> [...]
>>>>>>>
>>>>>>> As far as I can tell we're likely to end up needing a key per regmap
>>>>>>> or
>>>>>>> something similar.
>>>>>
>>>>>
>>>>>> Since the number of lockdep classes itself is also limited we should
>>>>>> avoid
>>>>>> creating extra lockdep classes when we can. I think the approach which
>>>>>> having the option of specifying a lockdep class in the regmap config
>>>>>> will be
>>>>>> ok. The only case it can't handle if we nest instances with the same
>>>>>> config,
>>>>>> but I don't really see valid use scases for that at the moment.
>>>>>
>>>>>
>>>>> Oh, ffs.  This just keeps getting better.  I hadn't been aware of that
>>>>> limitation.  We still have the problem that this needs to be something
>>>>> users can understand rather than something that's just "define something
>>>>> here in one of your drivers if you're running into problems with
>>>>> spurious warnings" here.  That's always been the biggest problem here
>>>>> (once we got past the "what is this supposed to do in the first place?"
>>>>> issues).
>>>>
>>>>
>>>> I found that V4L2 uses separate lockdep classes for each of their
>>>> v4l2_ctrl. This was introduced in 6cd247ef22e "[media] v4l2-ctrls:
>>>> eliminate lockdep false alarms for struct v4l2_ctrl_handler.lock"
>>>>
>>>> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6cd247ef22e),
>>>> so we could possibly take that approach.
>>>>
>>>> On my system, I have:
>>>> # cat /proc/lockdep_stats
>>>>    lock-classes:                         1241 [max: 8191]
>>>>    direct dependencies:                  7364 [max: 32768]
>>>>    indirect dependencies:               27686
>>>>    all direct dependencies:            158097
>>>>    dependency chains:                   10011 [max: 65536]
>>>>    dependency chain hlocks:             38887 [max: 327680]
>>>>    in-hardirq chains:                      92
>>>>    in-softirq chains:                     372
>>>>    in-process chains:                    9547
>>>>    stack-trace entries:                107703 [max: 524288]
>>>>
>>>> So, at least on that platform, there is some room to grow...
>>>>
>>>> I'm just afraid that implementing this may require creating a bunch of
>>>> macros to wrap all regmap_init_[i2c/spi/...] functions, as the lockdep
>>>> classes need to be statically allocated... Unless we find a different
>>>> solution than what V4L2 does.
>>>
>>>
>>> Following up on this. Lars-Peter's comments also highlights that we
>>> have no good way to figure out which regmap requires a separate maps,
>>> no clear hierarchy we can know about in advance, so we should put each
>>> regmap in its own class.
>>>
>>> The main issue is that the keys need to be allocated statically. We
>>> have 2 options to do this:
>>>
>>> 1. mutex_init and v4l2_ctrl_handler_init solve this issue by being a
>>> preprocessor macro that first allocates a static lock_class_key, then
>>> calls the real init function.
>>> This is not so practical in the case of regmap, as we have 14
>>> different init functions ([devm_]regmap_init[_bus_type]), that would
>>> each require a wrapper.
>>>
>>> 2. Bus registration takes a different approach
>>>
>>> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=be871b7e5):
>>> struct bus_type (statically allocated for each bus) has a lock_key
>>> member: "struct lock_class_key lock_key;".
>>> In the context of regmaps, that would mean adding a "lock_key" member
>>> to regmap_config. I did a quick implementation of this idea, and it
>>> seems to work, without modification to the rt5677 driver. The only
>>> issue with this is that regmap_config cannot be const anymore: we'd
>>> need to remove the const specifier in all drivers that use regmaps.
>>
>>
>> Yeah, I though about that as well, but the problem is the regmap_config is
>> only valid during regmap_init() and can for example be placed on the stack.
>> In which case it won't work anymore.
>
> Then we might need to make it a requirement... In any case, lockdep
> will throw a warning if the lock_key is allocated on the stack (or
> kalloc'ed).

We can't, in some cases the config is dynamic and depends on other runtime 
parameters.

>
>>>
>>> Both alternatives would mean that all regmaps created from 1. the same
>>> line of code, or 2. the same regmap_config, would share the same
>>> class. That may not be an issue, however (do we have an example of
>>> different regmaps created from the same line/config that need to call
>>> each other?), and the custom mutex workaround is still available....
>>>
>>> Any preference between a bunch of macros, and adding a non-const
>>> member to regmap_config? Or maybe someone has a better idea?
>>
>>
>> Maybe we are just over-thinking this and should just add one key to each
>> regmap instance. That solves the issue without requiring the any user
>> interaction.
>
> Yes, I agree. What I'm trying to answer above is how to do that, at
> least partially. I have no idea how to allocate one class per regmap,
> the above only does one class per init call or per regmap_config.
>
> regmap instances are kalloc'ed, so they cannot contain the
> lock_class_key, which needs to be statically allocated (in .data).
> Another option would be to preallocate a bunch of lock_class_key in
> regmap.c, and pick from that, but that's terribly hacky...

Leaves us pretty much with only two options. Either add a lock key pointer 
to regmap_config which needs to be manually initialized. Or wrap all 
regmap_init() variants to create a static lock key. I'd slightly prefer the 
later. We can avoid most of the boiler-plate code by using some helper 
macros to generate the wrappers.

- Lars

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Mark Brown]

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

On Mon, Jun 29, 2015 at 04:18:11PM +0200, Lars-Peter Clausen wrote:

Please delete unneeded context from replies.

> Leaves us pretty much with only two options. Either add a lock key pointer
> to regmap_config which needs to be manually initialized. Or wrap all
> regmap_init() variants to create a static lock key. I'd slightly prefer the
> later. We can avoid most of the boiler-plate code by using some helper
> macros to generate the wrappers.

It's better to keep the bodges in the core, yes.

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

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Nicolas Boichat]

On Mon, Jun 29, 2015 at 04:34:11PM +0100, Mark Brown wrote:
> On Mon, Jun 29, 2015 at 04:18:11PM +0200, Lars-Peter Clausen wrote:
> > Leaves us pretty much with only two options. Either add a lock key pointer
> > to regmap_config which needs to be manually initialized. Or wrap all
> > regmap_init() variants to create a static lock key. I'd slightly prefer the
> > later. We can avoid most of the boiler-plate code by using some helper
> > macros to generate the wrappers.
> 
> It's better to keep the bodges in the core, yes.

Partial attempt below. Of course all other _init functions will need to be
converted as well. I'd like to get feedback before I do the rest of the work.
The macro part is quite repetitive and I don't think it can be simplified.

Thanks!

>8------------------------------------------------------8<
Subject: [PATCH] regmap: Use different lockdep classes for each regmap init
 call

Lockdep validator complains about recursive locking and deadlock
when two different regmap instances are called in a nested order.
That happens anytime a regmap read/write call needs to access
another regmap.

This is because, for performance reason, lockdep groups all locks
initialized by the same mutex_init() in the same lock class.
Therefore all regmap mutexes are in the same lock class, leading
to lockdep "nested locking" warnings if a regmap accesses another
regmap.

In general, it is impossible to establish in advance the hierarchy
of regmaps, so we make sure that each regmap init call initializes
its own static lock_class_key. This is done by wrapping all
regmap_init calls into macros.

This also allows us to give meaningful names to the lock_class_key.
For example, in rt5677 case, we have in /proc/lockdep_chains:
irq_context: 0
[ffffffc0018d2198] &dev->mutex
[ffffffc0018d2198] &dev->mutex
[ffffffc001bd7f60] rt5677:5104:(&rt5677_regmap)->_lock
[ffffffc001bd7f58] rt5677:5096:(&rt5677_regmap_physical)->_lock
[ffffffc001b95448] &(&base->lock)->rlock

The above would have resulted in a lockdep recursive warning
previously. This is not the case anymore as the lockdep validator
now clearly identifies the 2 locks as separate.

Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
---
 drivers/base/regmap/regmap-i2c.c | 22 ++++++----
 drivers/base/regmap/regmap.c     | 31 +++++++++-----
 include/linux/regmap.h           | 91 ++++++++++++++++++++++++++++++++++------
 3 files changed, 113 insertions(+), 31 deletions(-)

diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c
index 053150a..c1f9396 100644
--- a/drivers/base/regmap/regmap-i2c.c
+++ b/drivers/base/regmap/regmap-i2c.c
@@ -198,17 +198,20 @@ static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
  * The return value will be an ERR_PTR() on error or a valid pointer to
  * a struct regmap.
  */
-struct regmap *regmap_init_i2c(struct i2c_client *i2c,
-			       const struct regmap_config *config)
+struct regmap *__regmap_init_i2c(struct i2c_client *i2c,
+				 const struct regmap_config *config,
+				 struct lock_class_key *lock_key,
+				 const char *lock_name)
 {
 	const struct regmap_bus *bus = regmap_get_i2c_bus(i2c, config);
 
 	if (IS_ERR(bus))
 		return ERR_CAST(bus);
 
-	return regmap_init(&i2c->dev, bus, &i2c->dev, config);
+	return __regmap_init(&i2c->dev, bus, &i2c->dev, config,
+			     lock_key, lock_name);
 }
-EXPORT_SYMBOL_GPL(regmap_init_i2c);
+EXPORT_SYMBOL_GPL(__regmap_init_i2c);
 
 /**
  * devm_regmap_init_i2c(): Initialise managed register map
@@ -220,16 +223,19 @@ EXPORT_SYMBOL_GPL(regmap_init_i2c);
  * to a struct regmap.  The regmap will be automatically freed by the
  * device management code.
  */
-struct regmap *devm_regmap_init_i2c(struct i2c_client *i2c,
-				    const struct regmap_config *config)
+struct regmap *__devm_regmap_init_i2c(struct i2c_client *i2c,
+				      const struct regmap_config *config,
+				      struct lock_class_key *lock_key,
+				      const char *lock_name)
 {
 	const struct regmap_bus *bus = regmap_get_i2c_bus(i2c, config);
 
 	if (IS_ERR(bus))
 		return ERR_CAST(bus);
 
-	return devm_regmap_init(&i2c->dev, bus, &i2c->dev, config);
+	return __devm_regmap_init(&i2c->dev, bus, &i2c->dev, config,
+				  lock_key, lock_name);
 }
-EXPORT_SYMBOL_GPL(devm_regmap_init_i2c);
+EXPORT_SYMBOL_GPL(__devm_regmap_init_i2c);
 
 MODULE_LICENSE("GPL");
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index d2f8a81..b8e26af 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -526,10 +526,12 @@ static enum regmap_endian regmap_get_val_endian(struct device *dev,
  * a struct regmap.  This function should generally not be called
  * directly, it should be called by bus-specific init functions.
  */
-struct regmap *regmap_init(struct device *dev,
-			   const struct regmap_bus *bus,
-			   void *bus_context,
-			   const struct regmap_config *config)
+struct regmap *__regmap_init(struct device *dev,
+			     const struct regmap_bus *bus,
+			     void *bus_context,
+			     const struct regmap_config *config,
+			     struct lock_class_key *lock_key,
+			     const char *lock_name)
 {
 	struct regmap *map;
 	int ret = -EINVAL;
@@ -555,10 +557,14 @@ struct regmap *regmap_init(struct device *dev,
 			spin_lock_init(&map->spinlock);
 			map->lock = regmap_lock_spinlock;
 			map->unlock = regmap_unlock_spinlock;
+			lockdep_set_class_and_name(&map->spinlock,
+						lock_key, lock_name);
 		} else {
 			mutex_init(&map->mutex);
 			map->lock = regmap_lock_mutex;
 			map->unlock = regmap_unlock_mutex;
+			lockdep_set_class_and_name(&map->mutex,
+						lock_key, lock_name);
 		}
 		map->lock_arg = map;
 	}
@@ -898,7 +904,7 @@ err_map:
 err:
 	return ERR_PTR(ret);
 }
-EXPORT_SYMBOL_GPL(regmap_init);
+EXPORT_SYMBOL_GPL(__regmap_init);
 
 static void devm_regmap_release(struct device *dev, void *res)
 {
@@ -918,10 +924,12 @@ static void devm_regmap_release(struct device *dev, void *res)
  * directly, it should be called by bus-specific init functions.  The
  * map will be automatically freed by the device management code.
  */
-struct regmap *devm_regmap_init(struct device *dev,
-				const struct regmap_bus *bus,
-				void *bus_context,
-				const struct regmap_config *config)
+struct regmap *__devm_regmap_init(struct device *dev,
+				  const struct regmap_bus *bus,
+				  void *bus_context,
+				  const struct regmap_config *config,
+				  struct lock_class_key *lock_key,
+				  const char *lock_name)
 {
 	struct regmap **ptr, *regmap;
 
@@ -929,7 +937,8 @@ struct regmap *devm_regmap_init(struct device *dev,
 	if (!ptr)
 		return ERR_PTR(-ENOMEM);
 
-	regmap = regmap_init(dev, bus, bus_context, config);
+	regmap = __regmap_init(dev, bus, bus_context, config,
+			lock_key, lock_name);
 	if (!IS_ERR(regmap)) {
 		*ptr = regmap;
 		devres_add(dev, ptr);
@@ -939,7 +948,7 @@ struct regmap *devm_regmap_init(struct device *dev,
 
 	return regmap;
 }
-EXPORT_SYMBOL_GPL(devm_regmap_init);
+EXPORT_SYMBOL_GPL(__devm_regmap_init);
 
 static void regmap_field_init(struct regmap_field *rm_field,
 	struct regmap *regmap, struct reg_field reg_field)
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 1318e935..9505bca 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -17,6 +17,7 @@
 #include <linux/rbtree.h>
 #include <linux/err.h>
 #include <linux/bug.h>
+#include <linux/lockdep.h>
 
 struct module;
 struct device;
@@ -323,14 +324,18 @@ struct regmap_bus {
 	enum regmap_endian val_format_endian_default;
 };
 
-struct regmap *regmap_init(struct device *dev,
-			   const struct regmap_bus *bus,
-			   void *bus_context,
-			   const struct regmap_config *config);
+struct regmap *__regmap_init(struct device *dev,
+			     const struct regmap_bus *bus,
+			     void *bus_context,
+			     const struct regmap_config *config,
+			     struct lock_class_key *lock_key,
+			     const char *lock_name);
 int regmap_attach_dev(struct device *dev, struct regmap *map,
 				 const struct regmap_config *config);
-struct regmap *regmap_init_i2c(struct i2c_client *i2c,
-			       const struct regmap_config *config);
+struct regmap *__regmap_init_i2c(struct i2c_client *i2c,
+				 const struct regmap_config *config,
+				 struct lock_class_key *lock_key,
+				 const char *lock_name);
 struct regmap *regmap_init_spi(struct spi_device *dev,
 			       const struct regmap_config *config);
 struct regmap *regmap_init_spmi_base(struct spmi_device *dev,
@@ -341,12 +346,16 @@ struct regmap *regmap_init_mmio_clk(struct device *dev, const char *clk_id,
 				    void __iomem *regs,
 				    const struct regmap_config *config);
 
-struct regmap *devm_regmap_init(struct device *dev,
-				const struct regmap_bus *bus,
-				void *bus_context,
-				const struct regmap_config *config);
-struct regmap *devm_regmap_init_i2c(struct i2c_client *i2c,
-				    const struct regmap_config *config);
+struct regmap *__devm_regmap_init(struct device *dev,
+				  const struct regmap_bus *bus,
+				  void *bus_context,
+				  const struct regmap_config *config,
+				  struct lock_class_key *lock_key,
+				  const char *lock_name);
+struct regmap *__devm_regmap_init_i2c(struct i2c_client *i2c,
+				      const struct regmap_config *config,
+				      struct lock_class_key *lock_key,
+				      const char *lock_name);
 struct regmap *devm_regmap_init_spi(struct spi_device *dev,
 				    const struct regmap_config *config);
 struct regmap *devm_regmap_init_spmi_base(struct spmi_device *dev,
@@ -392,6 +401,64 @@ static inline struct regmap *devm_regmap_init_mmio(struct device *dev,
 	return devm_regmap_init_mmio_clk(dev, NULL, regs, config);
 }
 
+#ifdef CONFIG_LOCKDEP
+#define regmap_init(dev, bus, bus_context, config)			\
+(									\
+	({								\
+		static struct lock_class_key _key;			\
+		__regmap_init(dev, bus, bus_context, config,		\
+			&_key,						\
+			KBUILD_BASENAME ":"				\
+			__stringify(__LINE__) ":"			\
+			"(" #config ")->_lock");			\
+	})								\
+)
+#define regmap_init_i2c(i2c, config)					\
+(									\
+	({								\
+		static struct lock_class_key _key;			\
+		__regmap_init_i2c(i2c, config,				\
+				&_key,					\
+				KBUILD_BASENAME ":"			\
+				__stringify(__LINE__) ":"		\
+				"(" #config ")->_lock");		\
+	})								\
+)
+
+#define devm_regmap_init(dev, bus, bus_context, config)			\
+(									\
+	({								\
+		static struct lock_class_key _key;			\
+		__devm_regmap_init(dev, bus, bus_context, config,	\
+				&_key,					\
+				KBUILD_BASENAME ":"			\
+				__stringify(__LINE__) ":"		\
+				"(" #config ")->_lock");		\
+	})								\
+)
+#define devm_regmap_init_i2c(i2c, config)				\
+(									\
+	({								\
+		static struct lock_class_key _key;			\
+		__devm_regmap_init_i2c(i2c, config,			\
+				&_key,					\
+				KBUILD_BASENAME ":"			\
+				__stringify(__LINE__) ":"		\
+				"(" #config ")->_lock");		\
+	})								\
+)
+#else
+#define regmap_init(dev, bus, bus_context, config)			\
+	__regmap_init(dev, bus, bus_context, config, NULL, NULL)
+#define regmap_init_i2c(dev, bus, bus_context, config)			\
+	__regmap_init_i2c(dev, bus, bus_context, config, NULL, NULL)
+
+#define devm_regmap_init(dev, bus, bus_context, config)			\
+	__devm_regmap_init(dev, bus, bus_context, config, NULL, NULL)
+#define devm_regmap_init_i2c(dev, bus, bus_context, config)		\
+	__devm_regmap_init_i2c(dev, bus, bus_context, config, NULL, NULL)
+#endif
+
 void regmap_exit(struct regmap *map);
 int regmap_reinit_cache(struct regmap *map,
 			const struct regmap_config *config);
-- 
2.4.3.573.g4eafbef

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Nicolas Boichat]

On Mon, Jun 29, 2015 at 04:34:11PM +0100, Mark Brown wrote:
> On Mon, Jun 29, 2015 at 04:18:11PM +0200, Lars-Peter Clausen wrote:
> > Leaves us pretty much with only two options. Either add a lock key pointer
> > to regmap_config which needs to be manually initialized. Or wrap all
> > regmap_init() variants to create a static lock key. I'd slightly prefer the
> > later. We can avoid most of the boiler-plate code by using some helper
> > macros to generate the wrappers.
> 
> It's better to keep the bodges in the core, yes.

Partial attempt below. Of course all other _init functions will need to be
converted as well. I'd like to get feedback before I do the rest of the work.
The macro part is quite repetitive and I don't think it can be simplified.

Thanks!

>8------------------------------------------------------8<
Subject: [PATCH] regmap: Use different lockdep classes for each regmap init
 call

Lockdep validator complains about recursive locking and deadlock
when two different regmap instances are called in a nested order.
That happens anytime a regmap read/write call needs to access
another regmap.

This is because, for performance reason, lockdep groups all locks
initialized by the same mutex_init() in the same lock class.
Therefore all regmap mutexes are in the same lock class, leading
to lockdep "nested locking" warnings if a regmap accesses another
regmap.

In general, it is impossible to establish in advance the hierarchy
of regmaps, so we make sure that each regmap init call initializes
its own static lock_class_key. This is done by wrapping all
regmap_init calls into macros.

This also allows us to give meaningful names to the lock_class_key.
For example, in rt5677 case, we have in /proc/lockdep_chains:
irq_context: 0
[ffffffc0018d2198] &dev->mutex
[ffffffc0018d2198] &dev->mutex
[ffffffc001bd7f60] rt5677:5104:(&rt5677_regmap)->_lock
[ffffffc001bd7f58] rt5677:5096:(&rt5677_regmap_physical)->_lock
[ffffffc001b95448] &(&base->lock)->rlock

The above would have resulted in a lockdep recursive warning
previously. This is not the case anymore as the lockdep validator
now clearly identifies the 2 locks as separate.

Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
---
 drivers/base/regmap/regmap-i2c.c | 22 ++++++----
 drivers/base/regmap/regmap.c     | 31 +++++++++-----
 include/linux/regmap.h           | 91 ++++++++++++++++++++++++++++++++++------
 3 files changed, 113 insertions(+), 31 deletions(-)

diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c
index 053150a..c1f9396 100644
--- a/drivers/base/regmap/regmap-i2c.c
+++ b/drivers/base/regmap/regmap-i2c.c
@@ -198,17 +198,20 @@ static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
  * The return value will be an ERR_PTR() on error or a valid pointer to
  * a struct regmap.
  */
-struct regmap *regmap_init_i2c(struct i2c_client *i2c,
-			       const struct regmap_config *config)
+struct regmap *__regmap_init_i2c(struct i2c_client *i2c,
+				 const struct regmap_config *config,
+				 struct lock_class_key *lock_key,
+				 const char *lock_name)
 {
 	const struct regmap_bus *bus = regmap_get_i2c_bus(i2c, config);
 
 	if (IS_ERR(bus))
 		return ERR_CAST(bus);
 
-	return regmap_init(&i2c->dev, bus, &i2c->dev, config);
+	return __regmap_init(&i2c->dev, bus, &i2c->dev, config,
+			     lock_key, lock_name);
 }
-EXPORT_SYMBOL_GPL(regmap_init_i2c);
+EXPORT_SYMBOL_GPL(__regmap_init_i2c);
 
 /**
  * devm_regmap_init_i2c(): Initialise managed register map
@@ -220,16 +223,19 @@ EXPORT_SYMBOL_GPL(regmap_init_i2c);
  * to a struct regmap.  The regmap will be automatically freed by the
  * device management code.
  */
-struct regmap *devm_regmap_init_i2c(struct i2c_client *i2c,
-				    const struct regmap_config *config)
+struct regmap *__devm_regmap_init_i2c(struct i2c_client *i2c,
+				      const struct regmap_config *config,
+				      struct lock_class_key *lock_key,
+				      const char *lock_name)
 {
 	const struct regmap_bus *bus = regmap_get_i2c_bus(i2c, config);
 
 	if (IS_ERR(bus))
 		return ERR_CAST(bus);
 
-	return devm_regmap_init(&i2c->dev, bus, &i2c->dev, config);
+	return __devm_regmap_init(&i2c->dev, bus, &i2c->dev, config,
+				  lock_key, lock_name);
 }
-EXPORT_SYMBOL_GPL(devm_regmap_init_i2c);
+EXPORT_SYMBOL_GPL(__devm_regmap_init_i2c);
 
 MODULE_LICENSE("GPL");
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index d2f8a81..b8e26af 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -526,10 +526,12 @@ static enum regmap_endian regmap_get_val_endian(struct device *dev,
  * a struct regmap.  This function should generally not be called
  * directly, it should be called by bus-specific init functions.
  */
-struct regmap *regmap_init(struct device *dev,
-			   const struct regmap_bus *bus,
-			   void *bus_context,
-			   const struct regmap_config *config)
+struct regmap *__regmap_init(struct device *dev,
+			     const struct regmap_bus *bus,
+			     void *bus_context,
+			     const struct regmap_config *config,
+			     struct lock_class_key *lock_key,
+			     const char *lock_name)
 {
 	struct regmap *map;
 	int ret = -EINVAL;
@@ -555,10 +557,14 @@ struct regmap *regmap_init(struct device *dev,
 			spin_lock_init(&map->spinlock);
 			map->lock = regmap_lock_spinlock;
 			map->unlock = regmap_unlock_spinlock;
+			lockdep_set_class_and_name(&map->spinlock,
+						lock_key, lock_name);
 		} else {
 			mutex_init(&map->mutex);
 			map->lock = regmap_lock_mutex;
 			map->unlock = regmap_unlock_mutex;
+			lockdep_set_class_and_name(&map->mutex,
+						lock_key, lock_name);
 		}
 		map->lock_arg = map;
 	}
@@ -898,7 +904,7 @@ err_map:
 err:
 	return ERR_PTR(ret);
 }
-EXPORT_SYMBOL_GPL(regmap_init);
+EXPORT_SYMBOL_GPL(__regmap_init);
 
 static void devm_regmap_release(struct device *dev, void *res)
 {
@@ -918,10 +924,12 @@ static void devm_regmap_release(struct device *dev, void *res)
  * directly, it should be called by bus-specific init functions.  The
  * map will be automatically freed by the device management code.
  */
-struct regmap *devm_regmap_init(struct device *dev,
-				const struct regmap_bus *bus,
-				void *bus_context,
-				const struct regmap_config *config)
+struct regmap *__devm_regmap_init(struct device *dev,
+				  const struct regmap_bus *bus,
+				  void *bus_context,
+				  const struct regmap_config *config,
+				  struct lock_class_key *lock_key,
+				  const char *lock_name)
 {
 	struct regmap **ptr, *regmap;
 
@@ -929,7 +937,8 @@ struct regmap *devm_regmap_init(struct device *dev,
 	if (!ptr)
 		return ERR_PTR(-ENOMEM);
 
-	regmap = regmap_init(dev, bus, bus_context, config);
+	regmap = __regmap_init(dev, bus, bus_context, config,
+			lock_key, lock_name);
 	if (!IS_ERR(regmap)) {
 		*ptr = regmap;
 		devres_add(dev, ptr);
@@ -939,7 +948,7 @@ struct regmap *devm_regmap_init(struct device *dev,
 
 	return regmap;
 }
-EXPORT_SYMBOL_GPL(devm_regmap_init);
+EXPORT_SYMBOL_GPL(__devm_regmap_init);
 
 static void regmap_field_init(struct regmap_field *rm_field,
 	struct regmap *regmap, struct reg_field reg_field)
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 1318e935..9505bca 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -17,6 +17,7 @@
 #include <linux/rbtree.h>
 #include <linux/err.h>
 #include <linux/bug.h>
+#include <linux/lockdep.h>
 
 struct module;
 struct device;
@@ -323,14 +324,18 @@ struct regmap_bus {
 	enum regmap_endian val_format_endian_default;
 };
 
-struct regmap *regmap_init(struct device *dev,
-			   const struct regmap_bus *bus,
-			   void *bus_context,
-			   const struct regmap_config *config);
+struct regmap *__regmap_init(struct device *dev,
+			     const struct regmap_bus *bus,
+			     void *bus_context,
+			     const struct regmap_config *config,
+			     struct lock_class_key *lock_key,
+			     const char *lock_name);
 int regmap_attach_dev(struct device *dev, struct regmap *map,
 				 const struct regmap_config *config);
-struct regmap *regmap_init_i2c(struct i2c_client *i2c,
-			       const struct regmap_config *config);
+struct regmap *__regmap_init_i2c(struct i2c_client *i2c,
+				 const struct regmap_config *config,
+				 struct lock_class_key *lock_key,
+				 const char *lock_name);
 struct regmap *regmap_init_spi(struct spi_device *dev,
 			       const struct regmap_config *config);
 struct regmap *regmap_init_spmi_base(struct spmi_device *dev,
@@ -341,12 +346,16 @@ struct regmap *regmap_init_mmio_clk(struct device *dev, const char *clk_id,
 				    void __iomem *regs,
 				    const struct regmap_config *config);
 
-struct regmap *devm_regmap_init(struct device *dev,
-				const struct regmap_bus *bus,
-				void *bus_context,
-				const struct regmap_config *config);
-struct regmap *devm_regmap_init_i2c(struct i2c_client *i2c,
-				    const struct regmap_config *config);
+struct regmap *__devm_regmap_init(struct device *dev,
+				  const struct regmap_bus *bus,
+				  void *bus_context,
+				  const struct regmap_config *config,
+				  struct lock_class_key *lock_key,
+				  const char *lock_name);
+struct regmap *__devm_regmap_init_i2c(struct i2c_client *i2c,
+				      const struct regmap_config *config,
+				      struct lock_class_key *lock_key,
+				      const char *lock_name);
 struct regmap *devm_regmap_init_spi(struct spi_device *dev,
 				    const struct regmap_config *config);
 struct regmap *devm_regmap_init_spmi_base(struct spmi_device *dev,
@@ -392,6 +401,64 @@ static inline struct regmap *devm_regmap_init_mmio(struct device *dev,
 	return devm_regmap_init_mmio_clk(dev, NULL, regs, config);
 }
 
+#ifdef CONFIG_LOCKDEP
+#define regmap_init(dev, bus, bus_context, config)			\
+(									\
+	({								\
+		static struct lock_class_key _key;			\
+		__regmap_init(dev, bus, bus_context, config,		\
+			&_key,						\
+			KBUILD_BASENAME ":"				\
+			__stringify(__LINE__) ":"			\
+			"(" #config ")->_lock");			\
+	})								\
+)
+#define regmap_init_i2c(i2c, config)					\
+(									\
+	({								\
+		static struct lock_class_key _key;			\
+		__regmap_init_i2c(i2c, config,				\
+				&_key,					\
+				KBUILD_BASENAME ":"			\
+				__stringify(__LINE__) ":"		\
+				"(" #config ")->_lock");		\
+	})								\
+)
+
+#define devm_regmap_init(dev, bus, bus_context, config)			\
+(									\
+	({								\
+		static struct lock_class_key _key;			\
+		__devm_regmap_init(dev, bus, bus_context, config,	\
+				&_key,					\
+				KBUILD_BASENAME ":"			\
+				__stringify(__LINE__) ":"		\
+				"(" #config ")->_lock");		\
+	})								\
+)
+#define devm_regmap_init_i2c(i2c, config)				\
+(									\
+	({								\
+		static struct lock_class_key _key;			\
+		__devm_regmap_init_i2c(i2c, config,			\
+				&_key,					\
+				KBUILD_BASENAME ":"			\
+				__stringify(__LINE__) ":"		\
+				"(" #config ")->_lock");		\
+	})								\
+)
+#else
+#define regmap_init(dev, bus, bus_context, config)			\
+	__regmap_init(dev, bus, bus_context, config, NULL, NULL)
+#define regmap_init_i2c(dev, bus, bus_context, config)			\
+	__regmap_init_i2c(dev, bus, bus_context, config, NULL, NULL)
+
+#define devm_regmap_init(dev, bus, bus_context, config)			\
+	__devm_regmap_init(dev, bus, bus_context, config, NULL, NULL)
+#define devm_regmap_init_i2c(dev, bus, bus_context, config)		\
+	__devm_regmap_init_i2c(dev, bus, bus_context, config, NULL, NULL)
+#endif
+
 void regmap_exit(struct regmap *map);
 int regmap_reinit_cache(struct regmap *map,
 			const struct regmap_config *config);
-- 
2.4.3.573.g4eafbef

Re: [alsa-devel] [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Lars-Peter Clausen]

On 06/30/2015 06:56 AM, Nicolas Boichat wrote:
> On Mon, Jun 29, 2015 at 04:34:11PM +0100, Mark Brown wrote:
>> On Mon, Jun 29, 2015 at 04:18:11PM +0200, Lars-Peter Clausen wrote:
>>> Leaves us pretty much with only two options. Either add a lock key pointer
>>> to regmap_config which needs to be manually initialized. Or wrap all
>>> regmap_init() variants to create a static lock key. I'd slightly prefer the
>>> later. We can avoid most of the boiler-plate code by using some helper
>>> macros to generate the wrappers.
>>
>> It's better to keep the bodges in the core, yes.
>
> Partial attempt below. Of course all other _init functions will need to be
> converted as well. I'd like to get feedback before I do the rest of the work.

Looks good to me.

> The macro part is quite repetitive and I don't think it can be simplified.

How about something like

#ifdef CONFIG_LOCKDEP

#define regmap_lockdep_wrapper(fn, ...)				\
(								\
	({							\
		static struct lock_class_key _key;		\
		fn(__VA_ARGS__,	&_key,				\
			KBUILD_BASENAME ":"			\
			__stringify(__LINE__) ":"		\
			"(" #config ")->_lock");		\
	})							\
)

#else
#define regmap_lockdep_wrapper(fn, ...) fn(__VA_ARGS__, NULL, NULL)
#endif

#define regmap_init_i2c(i2c, config) \
	regmap_lockdep_wrapper(__regmap_init_i2c, i2c, config)
...
	

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Mark Brown]

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

On Mon, Jun 29, 2015 at 10:03:09PM +0800, Nicolas Boichat wrote:

> regmap instances are kalloc'ed, so they cannot contain the
> lock_class_key, which needs to be statically allocated (in .data).
> Another option would be to preallocate a bunch of lock_class_key in
> regmap.c, and pick from that, but that's terribly hacky...

Honestly this is all starting to sound like we're having to jump through
too many hoops for lockep (and other APIs are too from the sounds of it)
so we should be looking at lockdep here.

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

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Arjan van de Ven]

On 6/29/2015 7:22 AM, Mark Brown wrote:
> On Mon, Jun 29, 2015 at 10:03:09PM +0800, Nicolas Boichat wrote:
>
>> regmap instances are kalloc'ed, so they cannot contain the
>> lock_class_key, which needs to be statically allocated (in .data).
>> Another option would be to preallocate a bunch of lock_class_key in
>> regmap.c, and pick from that, but that's terribly hacky...
>
> Honestly this is all starting to sound like we're having to jump through
> too many hoops for lockep (and other APIs are too from the sounds of it)
> so we should be looking at lockdep here.

lockdep assumes that there is a known lock hierarchy, at least known
to the developer.

seems like for regmap there isn't

(I would be interested to know how you avoid ABBA deadlocks btw,
can you have 2 devices, one with a hierarchy one way, and another
with the hierarchy the other way?)

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Mark Brown]

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

On Mon, Jun 29, 2015 at 07:35:20AM -0700, Arjan van de Ven wrote:

> lockdep assumes that there is a known lock hierarchy, at least known
> to the developer.

> seems like for regmap there isn't

It's not that there's no heirachy of locks, it's that lockdep is unable
to understand what's going on since it's making simplifying assumptions
that just aren't true.  If I remember the problem correctly it's
grouping all locks allocated in the same place into one class which
doesn't work at all for scenarios where you've got a generic interface
providing services to many devices which may be stacked on top of each
other.

> (I would be interested to know how you avoid ABBA deadlocks btw,
> can you have 2 devices, one with a hierarchy one way, and another
> with the hierarchy the other way?)

I'm not sure I fully understand what you mean here, sorry - do you mean
in terms of classes or individual devices?  The relationships between
devices are all device and system defined, individual regmaps should be
treated as separate classes.  From this perspective it's basically
eqivalent to asking how the mutex code avoids misuse of mutexes.

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

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Arjan van de Ven]

On 6/29/2015 8:32 AM, Mark Brown wrote:
> On Mon, Jun 29, 2015 at 07:35:20AM -0700, Arjan van de Ven wrote:
>
>> lockdep assumes that there is a known lock hierarchy, at least known
>> to the developer.
>
>> seems like for regmap there isn't
>
> It's not that there's no heirachy of locks, it's that lockdep is unable
> to understand what's going on since it's making simplifying assumptions
> that just aren't true.  If I remember the problem correctly it's
> grouping all locks allocated in the same place into one class which
> doesn't work at all for scenarios where you've got a generic interface
> providing services to many devices which may be stacked on top of each
> other.

but the stacking *IS* a lock hierarchy.

it just seems that the hierarchy is implied rather than explicit.


>> (I would be interested to know how you avoid ABBA deadlocks btw,
>> can you have 2 devices, one with a hierarchy one way, and another
>> with the hierarchy the other way?)
>
> I'm not sure I fully understand what you mean here, sorry - do you mean
> in terms of classes or individual devices?  The relationships between
> devices are all device and system defined, individual regmaps should be
> treated as separate classes.  From this perspective it's basically
> eqivalent to asking how the mutex code avoids misuse of mutexes.

well what I meant is inividual devices/ranges

like device A is on devmap A but then ends up using devmap B underneath
(e.g. the lock nesting case)

what prevents there from being a device B that is on devmap B but that
uses devmap A underneath


>

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Mark Brown]

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

On Mon, Jun 29, 2015 at 08:36:01AM -0700, Arjan van de Ven wrote:
> On 6/29/2015 8:32 AM, Mark Brown wrote:
> >On Mon, Jun 29, 2015 at 07:35:20AM -0700, Arjan van de Ven wrote:

> >It's not that there's no heirachy of locks, it's that lockdep is unable
> >to understand what's going on since it's making simplifying assumptions
> >that just aren't true.  If I remember the problem correctly it's
> >grouping all locks allocated in the same place into one class which
> >doesn't work at all for scenarios where you've got a generic interface
> >providing services to many devices which may be stacked on top of each
> >other.

> but the stacking *IS* a lock hierarchy.

This is why I said "It's not that there is no heirachy of locks".  

> it just seems that the hierarchy is implied rather than explicit.

It's explicit for any given system, like I say it's just that lockdep's
simplifying assumptions don't cope.  As far as I can tell to do
something that robustly works without random magic thrown into
individual drivers with no clear logic we need to allocate a lock class
per regmap (or at least per regmap config that might be instantiated)
which is a problem as they need to be statically allocated.

> >>(I would be interested to know how you avoid ABBA deadlocks btw,
> >>can you have 2 devices, one with a hierarchy one way, and another
> >>with the hierarchy the other way?)

> >I'm not sure I fully understand what you mean here, sorry - do you mean
> >in terms of classes or individual devices?  The relationships between
> >devices are all device and system defined, individual regmaps should be
> >treated as separate classes.  From this perspective it's basically
> >eqivalent to asking how the mutex code avoids misuse of mutexes.

> well what I meant is inividual devices/ranges

> like device A is on devmap A but then ends up using devmap B underneath
> (e.g. the lock nesting case)

I'm not entirely sure what you mean by a "devmap" here - is that just a
regmap or do you mean something else?

> what prevents there from being a device B that is on devmap B but that
> uses devmap A underneath

Assuming you mean regmap nothing prevents that and we should be able to
detect if something messes up there.  It's a problem for the users, not
for regmap itself.

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

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Mark Brown]

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

On Mon, Jun 29, 2015 at 02:59:57PM +0200, Lars-Peter Clausen wrote:

> Maybe we are just over-thinking this and should just add one key to each
> regmap instance. That solves the issue without requiring the any user
> interaction. The only downside is that it might impact the performance of
> lockdep and uses quite a few lock classes. Its probably manageable right now
> but could grow into a problem as regmap adoption further progresses. But
> maybe we can leave the hard work of figuring a better solution to our future
> selves.

I thought the issue with that was that all lockdep classes need to be
allocated statically?  If we can just do something that scales with
regmap instances I'm not sure there's a big problem, even if adoption
increases we're talking about something that scales with the number of
devices in the system rather than the number of drivers using the API
which should be a lot more manageable.

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

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Mark Brown]

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

On Thu, Jun 25, 2015 at 05:35:03PM +0800, Nicolas Boichat wrote:

> I'm trying to revive Antti's patch [1], as we are hitting similar issue
> with rt5677 driver. I only updated the commit message and documentation,
> I kept both Signed-off-by and From lines, with a small note highlighting
> my changes, let me know if that's not appropriate.

This continues to have the same problems as the previous version of the
patch where I can't really tell how users are supposed to figure out if
they should use this or not.  There will be cases where there is a clear
use within the driver itself but as far as I can tell almost any driver
might be bitten by this if some underlying driver on a given system
happens to use a regmap so should be allocating a class.

> One issue that was raised by Mark at the time is that a per-regmap_config
> lock class might not be enough (Mark mentioned clocks as an example). The
> current implementation should be good enough as long as the clock regmaps
> do not access each other. If this is a problem, maybe we should consider
> replacing lockdep_lock_class_key by a boolean use_own_lock_class that would
> allocate/use a per regmap instance lock class, or have devm_regmap_init
> take an extra parameter specifying the lock class.

I think this is a problem and making this something that every regmap
does by default was my original suggestion, there's also Arjan's
suggestion of just using nested mutexes rather than classes.

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

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Lars-Peter Clausen]

[...]
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index 116655d..09aaaf5 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -135,6 +135,12 @@ typedef void (*regmap_unlock)(void *);
>    * @lock_arg:	  this field is passed as the only argument of lock/unlock
>    *		  functions (ignored in case regular lock/unlock functions
>    *		  are not overridden).
> + * @lock_class_key: Custom lock class key for lockdep validator. Use that to
> + *                  silence false lockdep nested locking warning, when this
> + *                  regmap needs to access another regmap during read/write
> + *                  operations (directly in read/write functions, or
> + *                  indirectly, e.g. through bus accesses).

The recommendation when to use this is the wrong way around. The presented 
criteria is true for all devices since the bus master might be using regmap 
to implements its IO. Any regmap instance that might be used from within 
another regmap instance needs a custom lock class. This includes bus masters 
as well as resource providers like clock chips or regulators.

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Nicolas Boichat]

On Thu, Jun 25, 2015 at 11:59 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> [...]
>>
>> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
>> index 116655d..09aaaf5 100644
>> --- a/include/linux/regmap.h
>> +++ b/include/linux/regmap.h
>> @@ -135,6 +135,12 @@ typedef void (*regmap_unlock)(void *);
>>    * @lock_arg:   this field is passed as the only argument of lock/unlock
>>    *              functions (ignored in case regular lock/unlock functions
>>    *              are not overridden).
>> + * @lock_class_key: Custom lock class key for lockdep validator. Use that
>> to
>> + *                  silence false lockdep nested locking warning, when
>> this
>> + *                  regmap needs to access another regmap during
>> read/write
>> + *                  operations (directly in read/write functions, or
>> + *                  indirectly, e.g. through bus accesses).
>
>
> The recommendation when to use this is the wrong way around. The presented
> criteria is true for all devices since the bus master might be using regmap
> to implements its IO. Any regmap instance that might be used from within
> another regmap instance needs a custom lock class. This includes bus masters
> as well as resource providers like clock chips or regulators.

I would have thought that it is easier to figure out that a regmap is
going to access another one, instead of figuring out all possible uses
of a regmap...

As it stands, I could only see 2 cases where this kind of warning
happens (I did not find any other recursive locking warning involving
regmaps...):
1. rt5677: The "master" regmap is a "virtual" regmap, that, depending
on the device mode (DSP or not), either directly access the register
on a physical regmap on i2c bus, or does it indirectly, by doing a
number of read/write on that same physical regmap.
2. drivers/media/dvb-frontends/rtl2832.c: That's Antti's case. If I
understand correctly, regmap access require transfers on a private i2c
bus, which, itself, uses a regmap.

I think both cases are _fairly_ clear, but of course that may not
cover everything (and I'm not sure if anyone would figure it out
before the warning shows up...), and I'm not sure if there are cases
that look similar but don't require a lockdep class.

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Nicolas Boichat]

On Thu, Jun 25, 2015 at 11:59 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> [...]
>>
>> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
>> index 116655d..09aaaf5 100644
>> --- a/include/linux/regmap.h
>> +++ b/include/linux/regmap.h
>> @@ -135,6 +135,12 @@ typedef void (*regmap_unlock)(void *);
>>    * @lock_arg:   this field is passed as the only argument of lock/unlock
>>    *              functions (ignored in case regular lock/unlock functions
>>    *              are not overridden).
>> + * @lock_class_key: Custom lock class key for lockdep validator. Use that
>> to
>> + *                  silence false lockdep nested locking warning, when
>> this
>> + *                  regmap needs to access another regmap during
>> read/write
>> + *                  operations (directly in read/write functions, or
>> + *                  indirectly, e.g. through bus accesses).
>
>
> The recommendation when to use this is the wrong way around. The presented
> criteria is true for all devices since the bus master might be using regmap
> to implements its IO. Any regmap instance that might be used from within
> another regmap instance needs a custom lock class. This includes bus masters
> as well as resource providers like clock chips or regulators.

I would have thought that it is easier to figure out that a regmap is
going to access another one, instead of figuring out all possible uses
of a regmap...

As it stands, I could only see 2 cases where this kind of warning
happens (I did not find any other recursive locking warning involving
regmaps...):
1. rt5677: The "master" regmap is a "virtual" regmap, that, depending
on the device mode (DSP or not), either directly access the register
on a physical regmap on i2c bus, or does it indirectly, by doing a
number of read/write on that same physical regmap.
2. drivers/media/dvb-frontends/rtl2832.c: That's Antti's case. If I
understand correctly, regmap access require transfers on a private i2c
bus, which, itself, uses a regmap.

I think both cases are _fairly_ clear, but of course that may not
cover everything (and I'm not sure if anyone would figure it out
before the warning shows up...), and I'm not sure if there are cases
that look similar but don't require a lockdep class.

Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Lars-Peter Clausen]

On 06/26/2015 04:34 AM, Nicolas Boichat wrote:
> On Thu, Jun 25, 2015 at 11:59 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> [...]
>>>
>>> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
>>> index 116655d..09aaaf5 100644
>>> --- a/include/linux/regmap.h
>>> +++ b/include/linux/regmap.h
>>> @@ -135,6 +135,12 @@ typedef void (*regmap_unlock)(void *);
>>>     * @lock_arg:   this field is passed as the only argument of lock/unlock
>>>     *              functions (ignored in case regular lock/unlock functions
>>>     *              are not overridden).
>>> + * @lock_class_key: Custom lock class key for lockdep validator. Use that
>>> to
>>> + *                  silence false lockdep nested locking warning, when
>>> this
>>> + *                  regmap needs to access another regmap during
>>> read/write
>>> + *                  operations (directly in read/write functions, or
>>> + *                  indirectly, e.g. through bus accesses).
>>
>>
>> The recommendation when to use this is the wrong way around. The presented
>> criteria is true for all devices since the bus master might be using regmap
>> to implements its IO. Any regmap instance that might be used from within
>> another regmap instance needs a custom lock class. This includes bus masters
>> as well as resource providers like clock chips or regulators.
>
> I would have thought that it is easier to figure out that a regmap is
> going to access another one, instead of figuring out all possible uses
> of a regmap...
>
> As it stands, I could only see 2 cases where this kind of warning
> happens (I did not find any other recursive locking warning involving
> regmaps...):
> 1. rt5677: The "master" regmap is a "virtual" regmap, that, depending
> on the device mode (DSP or not), either directly access the register
> on a physical regmap on i2c bus, or does it indirectly, by doing a
> number of read/write on that same physical regmap.
> 2. drivers/media/dvb-frontends/rtl2832.c: That's Antti's case. If I
> understand correctly, regmap access require transfers on a private i2c
> bus, which, itself, uses a regmap.
>
> I think both cases are _fairly_ clear, but of course that may not
> cover everything (and I'm not sure if anyone would figure it out
> before the warning shows up...), and I'm not sure if there are cases
> that look similar but don't require a lockdep class.
>

When you have a generic slave driver, you don't know what the bus master is 
going to do in e.g. i2c_transfer() or spi_sync(). It might very well be 
using regmap to do its IO. Or it might be enabling/disabling a clock or 
another resource that uses regmap to do its IO.