[PATCH 1/2] regmap: pass map name to lockdep

[PATCH 1/2] regmap: pass map name to lockdep

[Antti Palosaari]

lockdep complains recursive locking and deadlock when two different
regmap instances are called in a nested order. That happen easily
for example when both I2C client and muxed/repeater I2C adapter are
using regmap. As a solution, pass regmap name for lockdep in order
to force lockdep validate regmap mutex per driver - not as all regmap
instances grouped together.

Here is example connection, where nested regmap is used to control
I2C mux.
 __________         ___________         ___________
|  USB IF  |       |   demod   |       |   tuner   |
|----------|       |-----------|       |-----------|
|          |--I2C--|-----/ ----|--I2C--|           |
|I2C master|       |  I2C mux  |       | I2C slave |
|__________|       |___________|       |___________|

Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/base/regmap/regmap.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index d2f8a81..8d8ad37 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -559,6 +559,12 @@ struct regmap *regmap_init(struct device *dev,
 			mutex_init(&map->mutex);
 			map->lock = regmap_lock_mutex;
 			map->unlock = regmap_unlock_mutex;
+			if (config->name) {
+				static struct lock_class_key key;
+
+				lockdep_set_class_and_name(&map->mutex, &key,
+							   config->name);
+			}
 		}
 		map->lock_arg = map;
 	}
-- 
http://palosaari.fi/

[PATCH 2/2] rtl2832: add name for RegMap

[Antti Palosaari]

Pass module name to regmap in order to silence lockdep recursive
deadlock warning. Lockdep validator groups mutexes per mutex name by
default. Due to that tuner and demod regmap mutexes were seen as a
single mutex. Tuner register access causes demod register access,
because of I2C mux/repeater and that is seen as a recursive locking
- even those locks are different instances (tuner vs. demod).

Defining name for mutex allows lockdep to separate mutexes and error
is not shown.

Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/media/dvb-frontends/rtl2832.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/dvb-frontends/rtl2832.c b/drivers/media/dvb-frontends/rtl2832.c
index f44dc50..f41bbd0 100644
--- a/drivers/media/dvb-frontends/rtl2832.c
+++ b/drivers/media/dvb-frontends/rtl2832.c
@@ -1187,6 +1187,7 @@ static int rtl2832_probe(struct i2c_client *client,
 		},
 	};
 	static const struct regmap_config regmap_config = {
+		.name = KBUILD_MODNAME,
 		.reg_bits    =  8,
 		.val_bits    =  8,
 		.volatile_reg = rtl2832_volatile_reg,
-- 
http://palosaari.fi/

Re: [PATCH 1/2] regmap: pass map name to lockdep

[Lars-Peter Clausen]

On 12/18/2014 10:05 PM, Antti Palosaari wrote:
> lockdep complains recursive locking and deadlock when two different
> regmap instances are called in a nested order. That happen easily
> for example when both I2C client and muxed/repeater I2C adapter are
> using regmap. As a solution, pass regmap name for lockdep in order
> to force lockdep validate regmap mutex per driver - not as all regmap
> instances grouped together.

That's not how it works. Locks are grouped by lock class, the name is just for 
pretty printing. The only reason you do not get a warning anymore is because 
you have now different lock classes one for configs with a name and one for 
configs without a name.

You really need a way to specify a custom lock class per regmap instance in 
order to solve this problem.

- Lars

Re: [PATCH 1/2] regmap: pass map name to lockdep

[Antti Palosaari]

On 12/18/2014 11:34 PM, Lars-Peter Clausen wrote:
> On 12/18/2014 10:05 PM, Antti Palosaari wrote:
>> lockdep complains recursive locking and deadlock when two different
>> regmap instances are called in a nested order. That happen easily
>> for example when both I2C client and muxed/repeater I2C adapter are
>> using regmap. As a solution, pass regmap name for lockdep in order
>> to force lockdep validate regmap mutex per driver - not as all regmap
>> instances grouped together.
>
> That's not how it works. Locks are grouped by lock class, the name is
> just for pretty printing. The only reason you do not get a warning
> anymore is because you have now different lock classes one for configs
> with a name and one for configs without a name.
>
> You really need a way to specify a custom lock class per regmap instance
> in order to solve this problem.

I looked example for that solution from v4l controls. So it is also wrong?

https://patchwork.linuxtv.org/patch/17262/


Do you think I should change to mutex_lock_nested() as documented in
Documentation/locking/lockdep-design.txt ?

Should these macros used at all:
include/linux/lockdep.h

There is not much documentation, especially how these recursive lock 
warnings should be silenced.


regards
Antti
-- 
http://palosaari.fi/

Re: [PATCH 1/2] regmap: pass map name to lockdep

[Lars-Peter Clausen]

On 12/19/2014 11:58 AM, Antti Palosaari wrote:
> On 12/18/2014 11:34 PM, Lars-Peter Clausen wrote:
>> On 12/18/2014 10:05 PM, Antti Palosaari wrote:
>>> lockdep complains recursive locking and deadlock when two different
>>> regmap instances are called in a nested order. That happen easily
>>> for example when both I2C client and muxed/repeater I2C adapter are
>>> using regmap. As a solution, pass regmap name for lockdep in order
>>> to force lockdep validate regmap mutex per driver - not as all regmap
>>> instances grouped together.
>>
>> That's not how it works. Locks are grouped by lock class, the name is
>> just for pretty printing. The only reason you do not get a warning
>> anymore is because you have now different lock classes one for configs
>> with a name and one for configs without a name.
>>
>> You really need a way to specify a custom lock class per regmap instance
>> in order to solve this problem.
>
> I looked example for that solution from v4l controls. So it is also wrong?
>
> https://patchwork.linuxtv.org/patch/17262/

No, that's correct. It creates one lock class per v4l2_ctrl_handler_init() 
invocation site.

>
>
> Do you think I should change to mutex_lock_nested() as documented in
> Documentation/locking/lockdep-design.txt ?

No, mutex_lock_nested() only works if you can identify lock subclasses.

>
> Should these macros used at all:
> include/linux/lockdep.h
>
> There is not much documentation, especially how these recursive lock
> warnings should be silenced.

You have a couple of options, either do what v4l2_ctrl_handler_init() and 
create a lock class key per regmap_init_*() invocation site. Or just add a 
lock class key per regmap instance. Or add a helper function which allows to 
change the lock class of a regmap instance that can be used by drivers where 
we expect that there will be nested locking. E.g. like in a bus master.

- Lars

Re: [PATCH 1/2] regmap: pass map name to lockdep

[Antti Palosaari]

On 12/19/2014 03:45 PM, Lars-Peter Clausen wrote:
> On 12/19/2014 11:58 AM, Antti Palosaari wrote:
>> On 12/18/2014 11:34 PM, Lars-Peter Clausen wrote:
>>> On 12/18/2014 10:05 PM, Antti Palosaari wrote:
>>>> lockdep complains recursive locking and deadlock when two different
>>>> regmap instances are called in a nested order. That happen easily
>>>> for example when both I2C client and muxed/repeater I2C adapter are
>>>> using regmap. As a solution, pass regmap name for lockdep in order
>>>> to force lockdep validate regmap mutex per driver - not as all regmap
>>>> instances grouped together.
>>>
>>> That's not how it works. Locks are grouped by lock class, the name is
>>> just for pretty printing. The only reason you do not get a warning
>>> anymore is because you have now different lock classes one for configs
>>> with a name and one for configs without a name.
>>>
>>> You really need a way to specify a custom lock class per regmap instance
>>> in order to solve this problem.
>>
>> I looked example for that solution from v4l controls. So it is also
>> wrong?
>>
>> https://patchwork.linuxtv.org/patch/17262/
>
> No, that's correct. It creates one lock class per
> v4l2_ctrl_handler_init() invocation site.

ah, yes, it it includes that piece of code, which declares key for each 
caller module.

>> Do you think I should change to mutex_lock_nested() as documented in
>> Documentation/locking/lockdep-design.txt ?
>
> No, mutex_lock_nested() only works if you can identify lock subclasses.
>
>>
>> Should these macros used at all:
>> include/linux/lockdep.h
>>
>> There is not much documentation, especially how these recursive lock
>> warnings should be silenced.
>
> You have a couple of options, either do what v4l2_ctrl_handler_init()
> and create a lock class key per regmap_init_*() invocation site. Or just
> add a lock class key per regmap instance. Or add a helper function which
> allows to change the lock class of a regmap instance that can be used by
> drivers where we expect that there will be nested locking. E.g. like in
> a bus master.

I think I will add key to regmap config. Then driver which uses regmap, 
could declare key and pass it to regmap.

regards
Antti


-- 
http://palosaari.fi/