| Message ID | 1419114892-4550-1-git-send-email-crope@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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
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.
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
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
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.
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.
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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);
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(+)