[alsa-devel] [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep
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);
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[] = {
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?
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.
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
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.
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.
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).
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=6...), 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.
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=6...), 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=b...): 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!
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=6...), 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:
- 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.
- Bus registration takes a different approach
(https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=b...): 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
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=6...), 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:
- 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.
- Bus registration takes a different approach
(https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=b...): 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...
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=6...), 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:
- 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.
- Bus registration takes a different approach
(https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=b...): 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
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.
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);
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) ...
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.
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?)
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.
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
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.
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.
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.
[...]
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 functionsare 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.
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 functionsare not overridden).
- @lock_class_key: Custom lock class key for lockdep validator. Use that
to
silence false lockdep nested locking warning, whenthis
regmap needs to access another regmap duringread/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.
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, whenthis
regmap needs to access another regmap duringread/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...):
- 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.
participants (4)
-
Arjan van de Ven -
Lars-Peter Clausen -
Mark Brown -
Nicolas Boichat