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

[Nicolas Boichat]

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.

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!