[alsa-devel] [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Lars-Peter Clausen]

On 06/29/2015 04:03 PM, Nicolas Boichat wrote:
> On Mon, Jun 29, 2015 at 8:59 PM, Lars-Peter Clausen <lars at 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 at chromium.org>
>>> wrote:
>>>>
>>>>
>>>> On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <broonie at 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=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