From: Antti Palosaari <[email protected]>
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 <[email protected]>
Cc: Mark Brown <[email protected]>
Signed-off-by: Antti Palosaari <[email protected]>
[[email protected]: Reworded the commit message and regmap.h
documentation]
Signed-off-by: Nicolas Boichat <[email protected]>
---
I'm trying to revive Antti's patch [1], as we are hitting similar issue
with rt5677 driver. I only updated the commit message and documentation,
I kept both Signed-off-by and From lines, with a small note highlighting
my changes, let me know if that's not appropriate.
One issue that was raised by Mark at the time is that a per-regmap_config
lock class might not be enough (Mark mentioned clocks as an example). The
current implementation should be good enough as long as the clock regmaps
do not access each other. If this is a problem, maybe we should consider
replacing lockdep_lock_class_key by a boolean use_own_lock_class that would
allocate/use a per regmap instance lock class, or have devm_regmap_init
take an extra parameter specifying the lock class.
[1] https://www.mail-archive.com/linux-media%40vger.kernel.org/msg83490.html
drivers/base/regmap/regmap.c | 3 +++
include/linux/regmap.h | 7 +++++++
2 files changed, 10 insertions(+)
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 6273ff0..f5d1131 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -560,6 +560,9 @@ struct regmap *regmap_init(struct device *dev,
mutex_init(&map->mutex);
map->lock = regmap_lock_mutex;
map->unlock = regmap_unlock_mutex;
+ if (config->lockdep_lock_class_key)
+ lockdep_set_class(&map->mutex,
+ config->lockdep_lock_class_key);
}
map->lock_arg = map;
}
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 116655d..09aaaf5 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -135,6 +135,12 @@ typedef void (*regmap_unlock)(void *);
* @lock_arg: this field is passed as the only argument of lock/unlock
* functions (ignored in case regular lock/unlock functions
* are not overridden).
+ * @lock_class_key: Custom lock class key for lockdep validator. Use that to
+ * silence false lockdep nested locking warning, when this
+ * regmap needs to access another regmap during read/write
+ * operations (directly in read/write functions, or
+ * indirectly, e.g. through bus accesses).
+ * Valid only when regmap default mutex locking is used.
* @reg_read: Optional callback that if filled will be used to perform
* all the reads from the registers. Should only be provided for
* devices whose read operation cannot be represented as a simple
@@ -198,6 +204,7 @@ struct regmap_config {
regmap_lock lock;
regmap_unlock unlock;
void *lock_arg;
+ struct lock_class_key *lockdep_lock_class_key;
int (*reg_read)(void *context, unsigned int reg, unsigned int *val);
int (*reg_write)(void *context, unsigned int reg, unsigned int val);
--
2.4.3.573.g4eafbef
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 <[email protected]>
---
sound/soc/codecs/rt5677.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index 31d969a..3007d13 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -4998,6 +4998,7 @@ static const struct regmap_config rt5677_regmap_physical = {
.num_ranges = ARRAY_SIZE(rt5677_ranges),
};
+static struct lock_class_key rt5677_regmap_key;
static const struct regmap_config rt5677_regmap = {
.reg_bits = 8,
.val_bits = 16,
@@ -5015,6 +5016,10 @@ static const struct regmap_config rt5677_regmap = {
.num_reg_defaults = ARRAY_SIZE(rt5677_reg),
.ranges = rt5677_ranges,
.num_ranges = ARRAY_SIZE(rt5677_ranges),
+
+ /* Silence incorrect lockdep warnings, as rt5677_regmap_physical is
+ * accessed from within rt5677_regmap. */
+ .lockdep_lock_class_key = &rt5677_regmap_key,
};
static const struct i2c_device_id rt5677_i2c_id[] = {
--
2.4.3.573.g4eafbef
On 6/25/2015 2:35 AM, Nicolas Boichat wrote:
> From: Antti Palosaari <[email protected]>
>
> 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 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.
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 <[email protected]>
>>
>> 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.
[...]
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index 116655d..09aaaf5 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -135,6 +135,12 @@ typedef void (*regmap_unlock)(void *);
> * @lock_arg: this field is passed as the only argument of lock/unlock
> * functions (ignored in case regular lock/unlock functions
> * are not overridden).
> + * @lock_class_key: Custom lock class key for lockdep validator. Use that to
> + * silence false lockdep nested locking warning, when this
> + * regmap needs to access another regmap during read/write
> + * operations (directly in read/write functions, or
> + * indirectly, e.g. through bus accesses).
The recommendation when to use this is the wrong way around. The presented
criteria is true for all devices since the bus master might be using regmap
to implements its IO. Any regmap instance that might be used from within
another regmap instance needs a custom lock class. This includes bus masters
as well as resource providers like clock chips or regulators.
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 Thu, Jun 25, 2015 at 11:59 PM, Lars-Peter Clausen <[email protected]> wrote:
> [...]
>>
>> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
>> index 116655d..09aaaf5 100644
>> --- a/include/linux/regmap.h
>> +++ b/include/linux/regmap.h
>> @@ -135,6 +135,12 @@ typedef void (*regmap_unlock)(void *);
>> * @lock_arg: this field is passed as the only argument of lock/unlock
>> * functions (ignored in case regular lock/unlock functions
>> * are not overridden).
>> + * @lock_class_key: Custom lock class key for lockdep validator. Use that
>> to
>> + * silence false lockdep nested locking warning, when
>> this
>> + * regmap needs to access another regmap during
>> read/write
>> + * operations (directly in read/write functions, or
>> + * indirectly, e.g. through bus accesses).
>
>
> The recommendation when to use this is the wrong way around. The presented
> criteria is true for all devices since the bus master might be using regmap
> to implements its IO. Any regmap instance that might be used from within
> another regmap instance needs a custom lock class. This includes bus masters
> as well as resource providers like clock chips or regulators.
I would have thought that it is easier to figure out that a regmap is
going to access another one, instead of figuring out all possible uses
of a regmap...
As it stands, I could only see 2 cases where this kind of warning
happens (I did not find any other recursive locking warning involving
regmaps...):
1. rt5677: The "master" regmap is a "virtual" regmap, that, depending
on the device mode (DSP or not), either directly access the register
on a physical regmap on i2c bus, or does it indirectly, by doing a
number of read/write on that same physical regmap.
2. drivers/media/dvb-frontends/rtl2832.c: That's Antti's case. If I
understand correctly, regmap access require transfers on a private i2c
bus, which, itself, uses a regmap.
I think both cases are _fairly_ clear, but of course that may not
cover everything (and I'm not sure if anyone would figure it out
before the warning shows up...), and I'm not sure if there are cases
that look similar but don't require a lockdep class.
On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <[email protected]> wrote:
[...]
>> >As far as I can tell we're likely to end up needing a key per regmap or
>> >something similar.
>
>> Since the number of lockdep classes itself is also limited we should avoid
>> creating extra lockdep classes when we can. I think the approach which
>> having the option of specifying a lockdep class in the regmap config will be
>> ok. The only case it can't handle if we nest instances with the same config,
>> but I don't really see valid use scases for that at the moment.
>
> Oh, ffs. This just keeps getting better. I hadn't been aware of that
> limitation. We still have the problem that this needs to be something
> users can understand rather than something that's just "define something
> here in one of your drivers if you're running into problems with
> spurious warnings" here. That's always been the biggest problem here
> (once we got past the "what is this supposed to do in the first place?"
> issues).
I found that V4L2 uses separate lockdep classes for each of their
v4l2_ctrl. This was introduced in 6cd247ef22e "[media] v4l2-ctrls:
eliminate lockdep false alarms for struct v4l2_ctrl_handler.lock"
(https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6cd247ef22e),
so we could possibly take that approach.
On my system, I have:
# cat /proc/lockdep_stats
lock-classes: 1241 [max: 8191]
direct dependencies: 7364 [max: 32768]
indirect dependencies: 27686
all direct dependencies: 158097
dependency chains: 10011 [max: 65536]
dependency chain hlocks: 38887 [max: 327680]
in-hardirq chains: 92
in-softirq chains: 372
in-process chains: 9547
stack-trace entries: 107703 [max: 524288]
So, at least on that platform, there is some room to grow...
I'm just afraid that implementing this may require creating a bunch of
macros to wrap all regmap_init_[i2c/spi/...] functions, as the lockdep
classes need to be statically allocated... Unless we find a different
solution than what V4L2 does.
On 06/26/2015 04:34 AM, Nicolas Boichat wrote:
> On Thu, Jun 25, 2015 at 11:59 PM, Lars-Peter Clausen <[email protected]> wrote:
>> [...]
>>>
>>> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
>>> index 116655d..09aaaf5 100644
>>> --- a/include/linux/regmap.h
>>> +++ b/include/linux/regmap.h
>>> @@ -135,6 +135,12 @@ typedef void (*regmap_unlock)(void *);
>>> * @lock_arg: this field is passed as the only argument of lock/unlock
>>> * functions (ignored in case regular lock/unlock functions
>>> * are not overridden).
>>> + * @lock_class_key: Custom lock class key for lockdep validator. Use that
>>> to
>>> + * silence false lockdep nested locking warning, when
>>> this
>>> + * regmap needs to access another regmap during
>>> read/write
>>> + * operations (directly in read/write functions, or
>>> + * indirectly, e.g. through bus accesses).
>>
>>
>> The recommendation when to use this is the wrong way around. The presented
>> criteria is true for all devices since the bus master might be using regmap
>> to implements its IO. Any regmap instance that might be used from within
>> another regmap instance needs a custom lock class. This includes bus masters
>> as well as resource providers like clock chips or regulators.
>
> I would have thought that it is easier to figure out that a regmap is
> going to access another one, instead of figuring out all possible uses
> of a regmap...
>
> As it stands, I could only see 2 cases where this kind of warning
> happens (I did not find any other recursive locking warning involving
> regmaps...):
> 1. rt5677: The "master" regmap is a "virtual" regmap, that, depending
> on the device mode (DSP or not), either directly access the register
> on a physical regmap on i2c bus, or does it indirectly, by doing a
> number of read/write on that same physical regmap.
> 2. drivers/media/dvb-frontends/rtl2832.c: That's Antti's case. If I
> understand correctly, regmap access require transfers on a private i2c
> bus, which, itself, uses a regmap.
>
> I think both cases are _fairly_ clear, but of course that may not
> cover everything (and I'm not sure if anyone would figure it out
> before the warning shows up...), and I'm not sure if there are cases
> that look similar but don't require a lockdep class.
>
When you have a generic slave driver, you don't know what the bus master is
going to do in e.g. i2c_transfer() or spi_sync(). It might very well be
using regmap to do its IO. Or it might be enabling/disabling a clock or
another resource that uses regmap to do its IO.
On Fri, Jun 26, 2015 at 11:16 AM, Nicolas Boichat <[email protected]> wrote:
>
> On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <[email protected]> wrote:
> [...]
> >> >As far as I can tell we're likely to end up needing a key per regmap or
> >> >something similar.
> >
> >> Since the number of lockdep classes itself is also limited we should avoid
> >> creating extra lockdep classes when we can. I think the approach which
> >> having the option of specifying a lockdep class in the regmap config will be
> >> ok. The only case it can't handle if we nest instances with the same config,
> >> but I don't really see valid use scases for that at the moment.
> >
> > Oh, ffs. This just keeps getting better. I hadn't been aware of that
> > limitation. We still have the problem that this needs to be something
> > users can understand rather than something that's just "define something
> > here in one of your drivers if you're running into problems with
> > spurious warnings" here. That's always been the biggest problem here
> > (once we got past the "what is this supposed to do in the first place?"
> > issues).
>
> I found that V4L2 uses separate lockdep classes for each of their
> v4l2_ctrl. This was introduced in 6cd247ef22e "[media] v4l2-ctrls:
> eliminate lockdep false alarms for struct v4l2_ctrl_handler.lock"
> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6cd247ef22e),
> so we could possibly take that approach.
>
> On my system, I have:
> # cat /proc/lockdep_stats
> lock-classes: 1241 [max: 8191]
> direct dependencies: 7364 [max: 32768]
> indirect dependencies: 27686
> all direct dependencies: 158097
> dependency chains: 10011 [max: 65536]
> dependency chain hlocks: 38887 [max: 327680]
> in-hardirq chains: 92
> in-softirq chains: 372
> in-process chains: 9547
> stack-trace entries: 107703 [max: 524288]
>
> So, at least on that platform, there is some room to grow...
>
> I'm just afraid that implementing this may require creating a bunch of
> macros to wrap all regmap_init_[i2c/spi/...] functions, as the lockdep
> classes need to be statically allocated... Unless we find a different
> solution than what V4L2 does.
Following up on this. Lars-Peter's comments also highlights that we
have no good way to figure out which regmap requires a separate maps,
no clear hierarchy we can know about in advance, so we should put each
regmap in its own class.
The main issue is that the keys need to be allocated statically. We
have 2 options to do this:
1. mutex_init and v4l2_ctrl_handler_init solve this issue by being a
preprocessor macro that first allocates a static lock_class_key, then
calls the real init function.
This is not so practical in the case of regmap, as we have 14
different init functions ([devm_]regmap_init[_bus_type]), that would
each require a wrapper.
2. Bus registration takes a different approach
(https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=be871b7e5):
struct bus_type (statically allocated for each bus) has a lock_key
member: "struct lock_class_key lock_key;".
In the context of regmaps, that would mean adding a "lock_key" member
to regmap_config. I did a quick implementation of this idea, and it
seems to work, without modification to the rt5677 driver. The only
issue with this is that regmap_config cannot be const anymore: we'd
need to remove the const specifier in all drivers that use regmaps.
Both alternatives would mean that all regmaps created from 1. the same
line of code, or 2. the same regmap_config, would share the same
class. That may not be an issue, however (do we have an example of
different regmaps created from the same line/config that need to call
each other?), and the custom mutex workaround is still available....
Any preference between a bunch of macros, and adding a non-const
member to regmap_config? Or maybe someone has a better idea?
Thanks!
On 06/29/2015 02:51 PM, Nicolas Boichat wrote:
> On Fri, Jun 26, 2015 at 11:16 AM, Nicolas Boichat <[email protected]> wrote:
>>
>> On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <[email protected]> wrote:
>> [...]
>>>>> As far as I can tell we're likely to end up needing a key per regmap or
>>>>> something similar.
>>>
>>>> Since the number of lockdep classes itself is also limited we should avoid
>>>> creating extra lockdep classes when we can. I think the approach which
>>>> having the option of specifying a lockdep class in the regmap config will be
>>>> ok. The only case it can't handle if we nest instances with the same config,
>>>> but I don't really see valid use scases for that at the moment.
>>>
>>> Oh, ffs. This just keeps getting better. I hadn't been aware of that
>>> limitation. We still have the problem that this needs to be something
>>> users can understand rather than something that's just "define something
>>> here in one of your drivers if you're running into problems with
>>> spurious warnings" here. That's always been the biggest problem here
>>> (once we got past the "what is this supposed to do in the first place?"
>>> issues).
>>
>> I found that V4L2 uses separate lockdep classes for each of their
>> v4l2_ctrl. This was introduced in 6cd247ef22e "[media] v4l2-ctrls:
>> eliminate lockdep false alarms for struct v4l2_ctrl_handler.lock"
>> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6cd247ef22e),
>> so we could possibly take that approach.
>>
>> On my system, I have:
>> # cat /proc/lockdep_stats
>> lock-classes: 1241 [max: 8191]
>> direct dependencies: 7364 [max: 32768]
>> indirect dependencies: 27686
>> all direct dependencies: 158097
>> dependency chains: 10011 [max: 65536]
>> dependency chain hlocks: 38887 [max: 327680]
>> in-hardirq chains: 92
>> in-softirq chains: 372
>> in-process chains: 9547
>> stack-trace entries: 107703 [max: 524288]
>>
>> So, at least on that platform, there is some room to grow...
>>
>> I'm just afraid that implementing this may require creating a bunch of
>> macros to wrap all regmap_init_[i2c/spi/...] functions, as the lockdep
>> classes need to be statically allocated... Unless we find a different
>> solution than what V4L2 does.
>
> Following up on this. Lars-Peter's comments also highlights that we
> have no good way to figure out which regmap requires a separate maps,
> no clear hierarchy we can know about in advance, so we should put each
> regmap in its own class.
>
> The main issue is that the keys need to be allocated statically. We
> have 2 options to do this:
>
> 1. mutex_init and v4l2_ctrl_handler_init solve this issue by being a
> preprocessor macro that first allocates a static lock_class_key, then
> calls the real init function.
> This is not so practical in the case of regmap, as we have 14
> different init functions ([devm_]regmap_init[_bus_type]), that would
> each require a wrapper.
>
> 2. Bus registration takes a different approach
> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=be871b7e5):
> struct bus_type (statically allocated for each bus) has a lock_key
> member: "struct lock_class_key lock_key;".
> In the context of regmaps, that would mean adding a "lock_key" member
> to regmap_config. I did a quick implementation of this idea, and it
> seems to work, without modification to the rt5677 driver. The only
> issue with this is that regmap_config cannot be const anymore: we'd
> need to remove the const specifier in all drivers that use regmaps.
Yeah, I though about that as well, but the problem is the regmap_config is
only valid during regmap_init() and can for example be placed on the stack.
In which case it won't work anymore.
>
> Both alternatives would mean that all regmaps created from 1. the same
> line of code, or 2. the same regmap_config, would share the same
> class. That may not be an issue, however (do we have an example of
> different regmaps created from the same line/config that need to call
> each other?), and the custom mutex workaround is still available....
>
> Any preference between a bunch of macros, and adding a non-const
> member to regmap_config? Or maybe someone has a better idea?
Maybe we are just over-thinking this and should just add one key to each
regmap instance. That solves the issue without requiring the any user
interaction. The only downside is that it might impact the performance of
lockdep and uses quite a few lock classes. Its probably manageable right now
but could grow into a problem as regmap adoption further progresses. But
maybe we can leave the hard work of figuring a better solution to our future
selves.
- Lars
On Mon, Jun 29, 2015 at 8:59 PM, Lars-Peter Clausen <[email protected]> wrote:
> On 06/29/2015 02:51 PM, Nicolas Boichat wrote:
>>
>> On Fri, Jun 26, 2015 at 11:16 AM, Nicolas Boichat <[email protected]>
>> wrote:
>>>
>>>
>>> On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <[email protected]> wrote:
>>> [...]
>>>>>>
>>>>>> As far as I can tell we're likely to end up needing a key per regmap
>>>>>> or
>>>>>> something similar.
>>>>
>>>>
>>>>> Since the number of lockdep classes itself is also limited we should
>>>>> avoid
>>>>> creating extra lockdep classes when we can. I think the approach which
>>>>> having the option of specifying a lockdep class in the regmap config
>>>>> will be
>>>>> ok. The only case it can't handle if we nest instances with the same
>>>>> config,
>>>>> but I don't really see valid use scases for that at the moment.
>>>>
>>>>
>>>> Oh, ffs. This just keeps getting better. I hadn't been aware of that
>>>> limitation. We still have the problem that this needs to be something
>>>> users can understand rather than something that's just "define something
>>>> here in one of your drivers if you're running into problems with
>>>> spurious warnings" here. That's always been the biggest problem here
>>>> (once we got past the "what is this supposed to do in the first place?"
>>>> issues).
>>>
>>>
>>> I found that V4L2 uses separate lockdep classes for each of their
>>> v4l2_ctrl. This was introduced in 6cd247ef22e "[media] v4l2-ctrls:
>>> eliminate lockdep false alarms for struct v4l2_ctrl_handler.lock"
>>>
>>> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6cd247ef22e),
>>> so we could possibly take that approach.
>>>
>>> On my system, I have:
>>> # cat /proc/lockdep_stats
>>> lock-classes: 1241 [max: 8191]
>>> direct dependencies: 7364 [max: 32768]
>>> indirect dependencies: 27686
>>> all direct dependencies: 158097
>>> dependency chains: 10011 [max: 65536]
>>> dependency chain hlocks: 38887 [max: 327680]
>>> in-hardirq chains: 92
>>> in-softirq chains: 372
>>> in-process chains: 9547
>>> stack-trace entries: 107703 [max: 524288]
>>>
>>> So, at least on that platform, there is some room to grow...
>>>
>>> I'm just afraid that implementing this may require creating a bunch of
>>> macros to wrap all regmap_init_[i2c/spi/...] functions, as the lockdep
>>> classes need to be statically allocated... Unless we find a different
>>> solution than what V4L2 does.
>>
>>
>> Following up on this. Lars-Peter's comments also highlights that we
>> have no good way to figure out which regmap requires a separate maps,
>> no clear hierarchy we can know about in advance, so we should put each
>> regmap in its own class.
>>
>> The main issue is that the keys need to be allocated statically. We
>> have 2 options to do this:
>>
>> 1. mutex_init and v4l2_ctrl_handler_init solve this issue by being a
>> preprocessor macro that first allocates a static lock_class_key, then
>> calls the real init function.
>> This is not so practical in the case of regmap, as we have 14
>> different init functions ([devm_]regmap_init[_bus_type]), that would
>> each require a wrapper.
>>
>> 2. Bus registration takes a different approach
>>
>> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=be871b7e5):
>> struct bus_type (statically allocated for each bus) has a lock_key
>> member: "struct lock_class_key lock_key;".
>> In the context of regmaps, that would mean adding a "lock_key" member
>> to regmap_config. I did a quick implementation of this idea, and it
>> seems to work, without modification to the rt5677 driver. The only
>> issue with this is that regmap_config cannot be const anymore: we'd
>> need to remove the const specifier in all drivers that use regmaps.
>
>
> Yeah, I though about that as well, but the problem is the regmap_config is
> only valid during regmap_init() and can for example be placed on the stack.
> In which case it won't work anymore.
Then we might need to make it a requirement... In any case, lockdep
will throw a warning if the lock_key is allocated on the stack (or
kalloc'ed).
>>
>> Both alternatives would mean that all regmaps created from 1. the same
>> line of code, or 2. the same regmap_config, would share the same
>> class. That may not be an issue, however (do we have an example of
>> different regmaps created from the same line/config that need to call
>> each other?), and the custom mutex workaround is still available....
>>
>> Any preference between a bunch of macros, and adding a non-const
>> member to regmap_config? Or maybe someone has a better idea?
>
>
> Maybe we are just over-thinking this and should just add one key to each
> regmap instance. That solves the issue without requiring the any user
> interaction.
Yes, I agree. What I'm trying to answer above is how to do that, at
least partially. I have no idea how to allocate one class per regmap,
the above only does one class per init call or per regmap_config.
regmap instances are kalloc'ed, so they cannot contain the
lock_class_key, which needs to be statically allocated (in .data).
Another option would be to preallocate a bunch of lock_class_key in
regmap.c, and pick from that, but that's terribly hacky...
On 06/29/2015 04:03 PM, Nicolas Boichat wrote:
> On Mon, Jun 29, 2015 at 8:59 PM, Lars-Peter Clausen <[email protected]> wrote:
>> On 06/29/2015 02:51 PM, Nicolas Boichat wrote:
>>>
>>> On Fri, Jun 26, 2015 at 11:16 AM, Nicolas Boichat <[email protected]>
>>> wrote:
>>>>
>>>>
>>>> On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <[email protected]> wrote:
>>>> [...]
>>>>>>>
>>>>>>> As far as I can tell we're likely to end up needing a key per regmap
>>>>>>> or
>>>>>>> something similar.
>>>>>
>>>>>
>>>>>> Since the number of lockdep classes itself is also limited we should
>>>>>> avoid
>>>>>> creating extra lockdep classes when we can. I think the approach which
>>>>>> having the option of specifying a lockdep class in the regmap config
>>>>>> will be
>>>>>> ok. The only case it can't handle if we nest instances with the same
>>>>>> config,
>>>>>> but I don't really see valid use scases for that at the moment.
>>>>>
>>>>>
>>>>> Oh, ffs. This just keeps getting better. I hadn't been aware of that
>>>>> limitation. We still have the problem that this needs to be something
>>>>> users can understand rather than something that's just "define something
>>>>> here in one of your drivers if you're running into problems with
>>>>> spurious warnings" here. That's always been the biggest problem here
>>>>> (once we got past the "what is this supposed to do in the first place?"
>>>>> issues).
>>>>
>>>>
>>>> I found that V4L2 uses separate lockdep classes for each of their
>>>> v4l2_ctrl. This was introduced in 6cd247ef22e "[media] v4l2-ctrls:
>>>> eliminate lockdep false alarms for struct v4l2_ctrl_handler.lock"
>>>>
>>>> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6cd247ef22e),
>>>> so we could possibly take that approach.
>>>>
>>>> On my system, I have:
>>>> # cat /proc/lockdep_stats
>>>> lock-classes: 1241 [max: 8191]
>>>> direct dependencies: 7364 [max: 32768]
>>>> indirect dependencies: 27686
>>>> all direct dependencies: 158097
>>>> dependency chains: 10011 [max: 65536]
>>>> dependency chain hlocks: 38887 [max: 327680]
>>>> in-hardirq chains: 92
>>>> in-softirq chains: 372
>>>> in-process chains: 9547
>>>> stack-trace entries: 107703 [max: 524288]
>>>>
>>>> So, at least on that platform, there is some room to grow...
>>>>
>>>> I'm just afraid that implementing this may require creating a bunch of
>>>> macros to wrap all regmap_init_[i2c/spi/...] functions, as the lockdep
>>>> classes need to be statically allocated... Unless we find a different
>>>> solution than what V4L2 does.
>>>
>>>
>>> Following up on this. Lars-Peter's comments also highlights that we
>>> have no good way to figure out which regmap requires a separate maps,
>>> no clear hierarchy we can know about in advance, so we should put each
>>> regmap in its own class.
>>>
>>> The main issue is that the keys need to be allocated statically. We
>>> have 2 options to do this:
>>>
>>> 1. mutex_init and v4l2_ctrl_handler_init solve this issue by being a
>>> preprocessor macro that first allocates a static lock_class_key, then
>>> calls the real init function.
>>> This is not so practical in the case of regmap, as we have 14
>>> different init functions ([devm_]regmap_init[_bus_type]), that would
>>> each require a wrapper.
>>>
>>> 2. Bus registration takes a different approach
>>>
>>> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=be871b7e5):
>>> struct bus_type (statically allocated for each bus) has a lock_key
>>> member: "struct lock_class_key lock_key;".
>>> In the context of regmaps, that would mean adding a "lock_key" member
>>> to regmap_config. I did a quick implementation of this idea, and it
>>> seems to work, without modification to the rt5677 driver. The only
>>> issue with this is that regmap_config cannot be const anymore: we'd
>>> need to remove the const specifier in all drivers that use regmaps.
>>
>>
>> Yeah, I though about that as well, but the problem is the regmap_config is
>> only valid during regmap_init() and can for example be placed on the stack.
>> In which case it won't work anymore.
>
> Then we might need to make it a requirement... In any case, lockdep
> will throw a warning if the lock_key is allocated on the stack (or
> kalloc'ed).
We can't, in some cases the config is dynamic and depends on other runtime
parameters.
>
>>>
>>> Both alternatives would mean that all regmaps created from 1. the same
>>> line of code, or 2. the same regmap_config, would share the same
>>> class. That may not be an issue, however (do we have an example of
>>> different regmaps created from the same line/config that need to call
>>> each other?), and the custom mutex workaround is still available....
>>>
>>> Any preference between a bunch of macros, and adding a non-const
>>> member to regmap_config? Or maybe someone has a better idea?
>>
>>
>> Maybe we are just over-thinking this and should just add one key to each
>> regmap instance. That solves the issue without requiring the any user
>> interaction.
>
> Yes, I agree. What I'm trying to answer above is how to do that, at
> least partially. I have no idea how to allocate one class per regmap,
> the above only does one class per init call or per regmap_config.
>
> regmap instances are kalloc'ed, so they cannot contain the
> lock_class_key, which needs to be statically allocated (in .data).
> Another option would be to preallocate a bunch of lock_class_key in
> regmap.c, and pick from that, but that's terribly hacky...
Leaves us pretty much with only two options. Either add a lock key pointer
to regmap_config which needs to be manually initialized. Or wrap all
regmap_init() variants to create a static lock key. I'd slightly prefer the
later. We can avoid most of the boiler-plate code by using some helper
macros to generate the wrappers.
- Lars
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 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 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 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 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 <[email protected]>
---
drivers/base/regmap/regmap-i2c.c | 22 ++++++----
drivers/base/regmap/regmap.c | 31 +++++++++-----
include/linux/regmap.h | 91 ++++++++++++++++++++++++++++++++++------
3 files changed, 113 insertions(+), 31 deletions(-)
diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c
index 053150a..c1f9396 100644
--- a/drivers/base/regmap/regmap-i2c.c
+++ b/drivers/base/regmap/regmap-i2c.c
@@ -198,17 +198,20 @@ static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
* The return value will be an ERR_PTR() on error or a valid pointer to
* a struct regmap.
*/
-struct regmap *regmap_init_i2c(struct i2c_client *i2c,
- const struct regmap_config *config)
+struct regmap *__regmap_init_i2c(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ struct lock_class_key *lock_key,
+ const char *lock_name)
{
const struct regmap_bus *bus = regmap_get_i2c_bus(i2c, config);
if (IS_ERR(bus))
return ERR_CAST(bus);
- return regmap_init(&i2c->dev, bus, &i2c->dev, config);
+ return __regmap_init(&i2c->dev, bus, &i2c->dev, config,
+ lock_key, lock_name);
}
-EXPORT_SYMBOL_GPL(regmap_init_i2c);
+EXPORT_SYMBOL_GPL(__regmap_init_i2c);
/**
* devm_regmap_init_i2c(): Initialise managed register map
@@ -220,16 +223,19 @@ EXPORT_SYMBOL_GPL(regmap_init_i2c);
* to a struct regmap. The regmap will be automatically freed by the
* device management code.
*/
-struct regmap *devm_regmap_init_i2c(struct i2c_client *i2c,
- const struct regmap_config *config)
+struct regmap *__devm_regmap_init_i2c(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ struct lock_class_key *lock_key,
+ const char *lock_name)
{
const struct regmap_bus *bus = regmap_get_i2c_bus(i2c, config);
if (IS_ERR(bus))
return ERR_CAST(bus);
- return devm_regmap_init(&i2c->dev, bus, &i2c->dev, config);
+ return __devm_regmap_init(&i2c->dev, bus, &i2c->dev, config,
+ lock_key, lock_name);
}
-EXPORT_SYMBOL_GPL(devm_regmap_init_i2c);
+EXPORT_SYMBOL_GPL(__devm_regmap_init_i2c);
MODULE_LICENSE("GPL");
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index d2f8a81..b8e26af 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -526,10 +526,12 @@ static enum regmap_endian regmap_get_val_endian(struct device *dev,
* a struct regmap. This function should generally not be called
* directly, it should be called by bus-specific init functions.
*/
-struct regmap *regmap_init(struct device *dev,
- const struct regmap_bus *bus,
- void *bus_context,
- const struct regmap_config *config)
+struct regmap *__regmap_init(struct device *dev,
+ const struct regmap_bus *bus,
+ void *bus_context,
+ const struct regmap_config *config,
+ struct lock_class_key *lock_key,
+ const char *lock_name)
{
struct regmap *map;
int ret = -EINVAL;
@@ -555,10 +557,14 @@ struct regmap *regmap_init(struct device *dev,
spin_lock_init(&map->spinlock);
map->lock = regmap_lock_spinlock;
map->unlock = regmap_unlock_spinlock;
+ lockdep_set_class_and_name(&map->spinlock,
+ lock_key, lock_name);
} else {
mutex_init(&map->mutex);
map->lock = regmap_lock_mutex;
map->unlock = regmap_unlock_mutex;
+ lockdep_set_class_and_name(&map->mutex,
+ lock_key, lock_name);
}
map->lock_arg = map;
}
@@ -898,7 +904,7 @@ err_map:
err:
return ERR_PTR(ret);
}
-EXPORT_SYMBOL_GPL(regmap_init);
+EXPORT_SYMBOL_GPL(__regmap_init);
static void devm_regmap_release(struct device *dev, void *res)
{
@@ -918,10 +924,12 @@ static void devm_regmap_release(struct device *dev, void *res)
* directly, it should be called by bus-specific init functions. The
* map will be automatically freed by the device management code.
*/
-struct regmap *devm_regmap_init(struct device *dev,
- const struct regmap_bus *bus,
- void *bus_context,
- const struct regmap_config *config)
+struct regmap *__devm_regmap_init(struct device *dev,
+ const struct regmap_bus *bus,
+ void *bus_context,
+ const struct regmap_config *config,
+ struct lock_class_key *lock_key,
+ const char *lock_name)
{
struct regmap **ptr, *regmap;
@@ -929,7 +937,8 @@ struct regmap *devm_regmap_init(struct device *dev,
if (!ptr)
return ERR_PTR(-ENOMEM);
- regmap = regmap_init(dev, bus, bus_context, config);
+ regmap = __regmap_init(dev, bus, bus_context, config,
+ lock_key, lock_name);
if (!IS_ERR(regmap)) {
*ptr = regmap;
devres_add(dev, ptr);
@@ -939,7 +948,7 @@ struct regmap *devm_regmap_init(struct device *dev,
return regmap;
}
-EXPORT_SYMBOL_GPL(devm_regmap_init);
+EXPORT_SYMBOL_GPL(__devm_regmap_init);
static void regmap_field_init(struct regmap_field *rm_field,
struct regmap *regmap, struct reg_field reg_field)
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 1318e935..9505bca 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -17,6 +17,7 @@
#include <linux/rbtree.h>
#include <linux/err.h>
#include <linux/bug.h>
+#include <linux/lockdep.h>
struct module;
struct device;
@@ -323,14 +324,18 @@ struct regmap_bus {
enum regmap_endian val_format_endian_default;
};
-struct regmap *regmap_init(struct device *dev,
- const struct regmap_bus *bus,
- void *bus_context,
- const struct regmap_config *config);
+struct regmap *__regmap_init(struct device *dev,
+ const struct regmap_bus *bus,
+ void *bus_context,
+ const struct regmap_config *config,
+ struct lock_class_key *lock_key,
+ const char *lock_name);
int regmap_attach_dev(struct device *dev, struct regmap *map,
const struct regmap_config *config);
-struct regmap *regmap_init_i2c(struct i2c_client *i2c,
- const struct regmap_config *config);
+struct regmap *__regmap_init_i2c(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ struct lock_class_key *lock_key,
+ const char *lock_name);
struct regmap *regmap_init_spi(struct spi_device *dev,
const struct regmap_config *config);
struct regmap *regmap_init_spmi_base(struct spmi_device *dev,
@@ -341,12 +346,16 @@ struct regmap *regmap_init_mmio_clk(struct device *dev, const char *clk_id,
void __iomem *regs,
const struct regmap_config *config);
-struct regmap *devm_regmap_init(struct device *dev,
- const struct regmap_bus *bus,
- void *bus_context,
- const struct regmap_config *config);
-struct regmap *devm_regmap_init_i2c(struct i2c_client *i2c,
- const struct regmap_config *config);
+struct regmap *__devm_regmap_init(struct device *dev,
+ const struct regmap_bus *bus,
+ void *bus_context,
+ const struct regmap_config *config,
+ struct lock_class_key *lock_key,
+ const char *lock_name);
+struct regmap *__devm_regmap_init_i2c(struct i2c_client *i2c,
+ const struct regmap_config *config,
+ struct lock_class_key *lock_key,
+ const char *lock_name);
struct regmap *devm_regmap_init_spi(struct spi_device *dev,
const struct regmap_config *config);
struct regmap *devm_regmap_init_spmi_base(struct spmi_device *dev,
@@ -392,6 +401,64 @@ static inline struct regmap *devm_regmap_init_mmio(struct device *dev,
return devm_regmap_init_mmio_clk(dev, NULL, regs, config);
}
+#ifdef CONFIG_LOCKDEP
+#define regmap_init(dev, bus, bus_context, config) \
+( \
+ ({ \
+ static struct lock_class_key _key; \
+ __regmap_init(dev, bus, bus_context, config, \
+ &_key, \
+ KBUILD_BASENAME ":" \
+ __stringify(__LINE__) ":" \
+ "(" #config ")->_lock"); \
+ }) \
+)
+#define regmap_init_i2c(i2c, config) \
+( \
+ ({ \
+ static struct lock_class_key _key; \
+ __regmap_init_i2c(i2c, config, \
+ &_key, \
+ KBUILD_BASENAME ":" \
+ __stringify(__LINE__) ":" \
+ "(" #config ")->_lock"); \
+ }) \
+)
+
+#define devm_regmap_init(dev, bus, bus_context, config) \
+( \
+ ({ \
+ static struct lock_class_key _key; \
+ __devm_regmap_init(dev, bus, bus_context, config, \
+ &_key, \
+ KBUILD_BASENAME ":" \
+ __stringify(__LINE__) ":" \
+ "(" #config ")->_lock"); \
+ }) \
+)
+#define devm_regmap_init_i2c(i2c, config) \
+( \
+ ({ \
+ static struct lock_class_key _key; \
+ __devm_regmap_init_i2c(i2c, config, \
+ &_key, \
+ KBUILD_BASENAME ":" \
+ __stringify(__LINE__) ":" \
+ "(" #config ")->_lock"); \
+ }) \
+)
+#else
+#define regmap_init(dev, bus, bus_context, config) \
+ __regmap_init(dev, bus, bus_context, config, NULL, NULL)
+#define regmap_init_i2c(dev, bus, bus_context, config) \
+ __regmap_init_i2c(dev, bus, bus_context, config, NULL, NULL)
+
+#define devm_regmap_init(dev, bus, bus_context, config) \
+ __devm_regmap_init(dev, bus, bus_context, config, NULL, NULL)
+#define devm_regmap_init_i2c(dev, bus, bus_context, config) \
+ __devm_regmap_init_i2c(dev, bus, bus_context, config, NULL, NULL)
+#endif
+
void regmap_exit(struct regmap *map);
int regmap_reinit_cache(struct regmap *map,
const struct regmap_config *config);
--
2.4.3.573.g4eafbef
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)
...