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

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

[Antti Palosaari]

Lockdep validator complains recursive locking and deadlock when two
different regmap instances are called in a nested order, as regmap
groups locks by default. That happens easily for example when both
I2C client and I2C adapter are using regmap. As a solution, add
configuration option to pass custom lock class key for lockdep
validator.

Here is example schema, where nested regmap calls are issued, when
more than 1 block uses regmap.
 __________         ___________         ___________
|  USB IF  |       |   demod   |       |   tuner   |
|----------|       |-----------|       |-----------|
|          |--I2C--|-----/ ----|--I2C--|           |
|I2C master|       |  I2C mux  |       | I2C slave |
|__________|       |___________|       |___________|

Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/base/regmap/regmap.c | 3 +++
 include/linux/regmap.h       | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index d2f8a81..56064d3 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -559,6 +559,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 c5ed83f..f930370 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -134,6 +134,10 @@ 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 when
+ *                regmap in question is used for bus master IO in order to avoid
+ *                false lockdep nested locking warning. 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
@@ -197,6 +201,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);
-- 
http://palosaari.fi/

[PATCHv2 2/2] rtl2832: use custom lock class key for regmap

[Antti Palosaari]

There was nested locking error shown by lockdep validator when both
demod and tuner drivers were using regmap. That is false positive
coming from the reason lockdep groups mutexes to 'classes'. That
leads situation both tuner driver and demod driver regmap mutex is
seen as a same mutex, even those are different ones in a real life.
Lockdep uses keys to separate these clock classes. Use custom class
key to demod regmap in order to separate it from mutex used by tuner
regmap, thus seen it as a different lock also from lockdep point of
view.

Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 drivers/media/dvb-frontends/rtl2832.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/dvb-frontends/rtl2832.c b/drivers/media/dvb-frontends/rtl2832.c
index f44dc50..6cfe5b6 100644
--- a/drivers/media/dvb-frontends/rtl2832.c
+++ b/drivers/media/dvb-frontends/rtl2832.c
@@ -1186,6 +1186,7 @@ static int rtl2832_probe(struct i2c_client *client,
 			.range_max        = 5 * 0x100,
 		},
 	};
+	static struct lock_class_key key;
 	static const struct regmap_config regmap_config = {
 		.reg_bits    =  8,
 		.val_bits    =  8,
@@ -1194,6 +1195,7 @@ static int rtl2832_probe(struct i2c_client *client,
 		.ranges = regmap_range_cfg,
 		.num_ranges = ARRAY_SIZE(regmap_range_cfg),
 		.cache_type = REGCACHE_RBTREE,
+		.lockdep_lock_class_key = &key,
 	};
 
 	dev_dbg(&client->dev, "\n");
-- 
http://palosaari.fi/

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

[Mark Brown]

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

On Sun, Dec 21, 2014 at 12:34:51AM +0200, Antti Palosaari wrote:
> Lockdep validator complains recursive locking and deadlock when two
> different regmap instances are called in a nested order, as regmap
> groups locks by default. That happens easily for example when both

I don't know what "regmap groups locks by default" means.

> I2C client and I2C adapter are using regmap. As a solution, add
> configuration option to pass custom lock class key for lockdep
> validator.

Why is this configurable, how would a device know if the system it is in
needs a custom locking class and can safely use one?

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

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

[Antti Palosaari]

On 12/22/2014 02:44 PM, Mark Brown wrote:
> On Sun, Dec 21, 2014 at 12:34:51AM +0200, Antti Palosaari wrote:
>> Lockdep validator complains recursive locking and deadlock when two
>> different regmap instances are called in a nested order, as regmap
>> groups locks by default. That happens easily for example when both
>
> I don't know what "regmap groups locks by default" means.

It means, that lockdep does not track individual lock instances, but 
group of lock instances, which are called classes. In that case, there 
is 2 regmap mutexes, one in a I2C client driver and another in I2C 
adapter driver. Even those are different locks (mutexes) in a different 
driver, lockdep refers those as a single and same lock and due to that 
it thinks there is recursive lock => deadlock.

from: Documentation/locking/lockdep-design.txt

Lock-class
----------

The basic object the validator operates upon is a 'class' of locks.

A class of locks is a group of locks that are logically the same with
respect to locking rules, even if the locks may have multiple (possibly
tens of thousands of) instantiations. For example a lock in the inode
struct is one class, while each inode has its own instantiation of that
lock class.

The validator tracks the 'state' of lock-classes, and it tracks
dependencies between different lock-classes. The validator maintains a
rolling proof that the state and the dependencies are correct.

Unlike an lock instantiation, the lock-class itself never goes away: when
a lock-class is used for the first time after bootup it gets registered,
and all subsequent uses of that lock-class will be attached to this
lock-class.


>> I2C client and I2C adapter are using regmap. As a solution, add
>> configuration option to pass custom lock class key for lockdep
>> validator.
>
> Why is this configurable, how would a device know if the system it is in
> needs a custom locking class and can safely use one?

If RegMap instance is bus master, eg. I2C adapter, then you should 
define own custom key. If you don't define own key and there will be 
slave on that bus which uses RegMap too, there will be recursive locking 
from a lockdep point of view.


regards
Antti

-- 
http://palosaari.fi/

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

[Mark Brown]

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

On Mon, Dec 22, 2014 at 02:55:23PM +0200, Antti Palosaari wrote:
> On 12/22/2014 02:44 PM, Mark Brown wrote:
> >On Sun, Dec 21, 2014 at 12:34:51AM +0200, Antti Palosaari wrote:

> >>I2C client and I2C adapter are using regmap. As a solution, add
> >>configuration option to pass custom lock class key for lockdep
> >>validator.

> >Why is this configurable, how would a device know if the system it is in
> >needs a custom locking class and can safely use one?

> If RegMap instance is bus master, eg. I2C adapter, then you should define
> own custom key. If you don't define own key and there will be slave on that
> bus which uses RegMap too, there will be recursive locking from a lockdep
> point of view.

That doesn't really explain to me why this is configurable, why should
drivers have to worry about this?

Please also write technical terms like regmap normally.

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

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

[Antti Palosaari]

On 12/22/2014 03:31 PM, Mark Brown wrote:
> On Mon, Dec 22, 2014 at 02:55:23PM +0200, Antti Palosaari wrote:
>> On 12/22/2014 02:44 PM, Mark Brown wrote:
>>> On Sun, Dec 21, 2014 at 12:34:51AM +0200, Antti Palosaari wrote:
>
>>>> I2C client and I2C adapter are using regmap. As a solution, add
>>>> configuration option to pass custom lock class key for lockdep
>>>> validator.
>
>>> Why is this configurable, how would a device know if the system it is in
>>> needs a custom locking class and can safely use one?
>
>> If RegMap instance is bus master, eg. I2C adapter, then you should define
>> own custom key. If you don't define own key and there will be slave on that
>> bus which uses RegMap too, there will be recursive locking from a lockdep
>> point of view.
>
> That doesn't really explain to me why this is configurable, why should
> drivers have to worry about this?

Did you read the lockdep documentation I pointed previous mail?
from: Documentation/locking/lockdep-design.txt

There is not very detailed documentation available, but the section 
"Exception: Nested data dependencies leading to nested locking" explains 
something.

One possibility is to disable lockdep checking from that driver totally, 
then drivers do not need to care it about. But I don't think it is 
proper way. One solution is to use custom regmap locking available 
already, but Mauro nor me didn't like that hack:
[RFC HACK] rtl2832: implement own lock for RegMap
https://www.mail-archive.com/linux-media@vger.kernel.org/msg83323.html

> Please also write technical terms like regmap normally.

Lower-case letters?

regards
Antti

-- 
http://palosaari.fi/

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

[Mark Brown]

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

On Mon, Dec 22, 2014 at 03:53:10PM +0200, Antti Palosaari wrote:
> On 12/22/2014 03:31 PM, Mark Brown wrote:

> >>>Why is this configurable, how would a device know if the system it is in
> >>>needs a custom locking class and can safely use one?

> >>If RegMap instance is bus master, eg. I2C adapter, then you should define
> >>own custom key. If you don't define own key and there will be slave on that
> >>bus which uses RegMap too, there will be recursive locking from a lockdep
> >>point of view.

> >That doesn't really explain to me why this is configurable, why should
> >drivers have to worry about this?

> Did you read the lockdep documentation I pointed previous mail?

No, quite apart from the fact that you pasted a good chunk of it into
your mail I don't think it's a good idea to require people to have to
reverse engineer everything to figure out if they're supposed to use
this, or expect people reviewing code using this feature to do that in
order to figure out if it's being used correctly or not.

Suggesting that I'm not thinking hard enough isn't helping here; this
stuff needs to be clear and easy so that people naturally get it right
when they need to and don't break things as a result of confusion or
error, requiring people to directly work with infrequently used things
like lock classes with minimal explanation doesn't achieve that goal.

> One possibility is to disable lockdep checking from that driver totally,
> then drivers do not need to care it about. But I don't think it is proper
> way. One solution is to use custom regmap locking available already, but
> Mauro nor me didn't like that hack:

You don't seem to be answering any of my questions here...  for example,
you keep saying that this is something bus masters should do.  Why does
it make sense for people writing such drivers to have to figure out that
they need to do this and how to do it?  Are there some bus masters that
shouldn't be doing so?  Should anything that isn't a bus master have to
do it?

> >Please also write technical terms like regmap normally.

> Lower-case letters?

Yes, the way it's written in every place it's used in the kernel except
a few I see you've added.

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

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

[Mauro Carvalho Chehab]

Antti/Mark,

Any news with regards to this?

Regards,
Mauro

Em Mon, 22 Dec 2014 15:05:15 +0000
Mark Brown <broonie@kernel.org> escreveu:

> On Mon, Dec 22, 2014 at 03:53:10PM +0200, Antti Palosaari wrote:
> > On 12/22/2014 03:31 PM, Mark Brown wrote:
> 
> > >>>Why is this configurable, how would a device know if the system it is in
> > >>>needs a custom locking class and can safely use one?
> 
> > >>If RegMap instance is bus master, eg. I2C adapter, then you should define
> > >>own custom key. If you don't define own key and there will be slave on that
> > >>bus which uses RegMap too, there will be recursive locking from a lockdep
> > >>point of view.
> 
> > >That doesn't really explain to me why this is configurable, why should
> > >drivers have to worry about this?
> 
> > Did you read the lockdep documentation I pointed previous mail?
> 
> No, quite apart from the fact that you pasted a good chunk of it into
> your mail I don't think it's a good idea to require people to have to
> reverse engineer everything to figure out if they're supposed to use
> this, or expect people reviewing code using this feature to do that in
> order to figure out if it's being used correctly or not.
> 
> Suggesting that I'm not thinking hard enough isn't helping here; this
> stuff needs to be clear and easy so that people naturally get it right
> when they need to and don't break things as a result of confusion or
> error, requiring people to directly work with infrequently used things
> like lock classes with minimal explanation doesn't achieve that goal.
> 
> > One possibility is to disable lockdep checking from that driver totally,
> > then drivers do not need to care it about. But I don't think it is proper
> > way. One solution is to use custom regmap locking available already, but
> > Mauro nor me didn't like that hack:
> 
> You don't seem to be answering any of my questions here...  for example,
> you keep saying that this is something bus masters should do.  Why does
> it make sense for people writing such drivers to have to figure out that
> they need to do this and how to do it?  Are there some bus masters that
> shouldn't be doing so?  Should anything that isn't a bus master have to
> do it?
> 
> > >Please also write technical terms like regmap normally.
> 
> > Lower-case letters?
> 
> Yes, the way it's written in every place it's used in the kernel except
> a few I see you've added.

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

[Mark Brown]

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

On Mon, Feb 02, 2015 at 12:31:38PM -0200, Mauro Carvalho Chehab wrote:
> Antti/Mark,
> 
> Any news with regards to this?

Please don't top post or send content free nags.  I can't really
remember what this is about but I don't think my review comments were
ever addressed.

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

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

[Mauro Carvalho Chehab]

Em Mon, 22 Dec 2014 13:31:42 +0000
Mark Brown <broonie@kernel.org> escreveu:

> On Mon, Dec 22, 2014 at 02:55:23PM +0200, Antti Palosaari wrote:
> > On 12/22/2014 02:44 PM, Mark Brown wrote:
> > >On Sun, Dec 21, 2014 at 12:34:51AM +0200, Antti Palosaari wrote:
> 
> > >>I2C client and I2C adapter are using regmap. As a solution, add
> > >>configuration option to pass custom lock class key for lockdep
> > >>validator.
> 
> > >Why is this configurable, how would a device know if the system it is in
> > >needs a custom locking class and can safely use one?
> 
> > If RegMap instance is bus master, eg. I2C adapter, then you should define
> > own custom key. If you don't define own key and there will be slave on that
> > bus which uses RegMap too, there will be recursive locking from a lockdep
> > point of view.
> 
> That doesn't really explain to me why this is configurable, why should
> drivers have to worry about this?
>
> Please also write technical terms like regmap normally.

Hi Mark,

Basically, when there's no nested calls to regmap mutexes, the driver
should not care at all to set it. This likely covers most of the usecases
of the regmap API. However, on too complex drivers like the media ones, 
sometimes, we may have scenarios where:

	Driver A calls I2C driver B
	Driver B can call I2C driver C
	Driver A can call I2C driver C

So, there are three possible scenarios, with regards to mutex:
	driver A -> driver B (locking regmap driver B mutex)
	driver A -> driver C (locking regmap driver C mutex)
	driver A -> driver B (locking regmap driver B mutex) -> driver C (locking regmap driver C mutex)

Depending on the way those calls happen, there are no dead locks, but
disabling lockdep checks as a hole would prevent the code to detect the
bad lock scenarios.

However, on last case (A -> B -> C), the lockdep will complain about a
nested mutex, as it would consider that both B and C are locking the
very same mutex.

What this patch does is to offer a way for drivers B and C to define
different mutex groups (e. g. different mutex "IDs") that will teach
the lockdep code to threat regmap mutex on drivers B and C as different
mutexes.

I hope the above explanation helps.

Regards
-- 

Cheers,
Mauro

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

[Mark Brown]

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

On Mon, Dec 22, 2014 at 12:23:19PM -0200, Mauro Carvalho Chehab wrote:

> What this patch does is to offer a way for drivers B and C to define
> different mutex groups (e. g. different mutex "IDs") that will teach
> the lockdep code to threat regmap mutex on drivers B and C as different
> mutexes.

> I hope the above explanation helps.

No, not really - even the best explanation on the mailing list isn't
going to make the commit clearer (something like this needs to be at
least clear in the commit log and ideally the code) and it'd be much
better if the interface were improved so it's obvious that users are
doing the right thing when they use it.

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

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

[Mark Brown]

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

On Sun, Dec 21, 2014 at 12:34:51AM +0200, Antti Palosaari wrote:

> + * @lock_class_key: Custom lock class key for lockdep validator. Use that when
> + *                regmap in question is used for bus master IO in order to avoid
> + *                false lockdep nested locking warning. Valid only when regmap
> + *                default mutex locking is used.

Thinking about this further this comment definitely isn't accurate, it's
not just bus masters that are potentially affected but also things like
clock controllers that might need to be interacted with in order to do
I/O.  Thinking about those I'm even unsure that a per driver class
(which seems to be the idea here) will be enough, it's at least in
theory possible that two different instances of the same clock IP (or
generic regmap clock controller) will both need to be turned on for this
to work.

If it was just bus controllers it looks like we can probably just have
the clients set a flag saying that's what they are and then define the
class in the regmap core but I don't think that's all that's going on
here.

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