| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <559153A3.1000006@metafoo.de> Date: Mon, 29 Jun 2015 16:18:11 +0200 From: Lars-Peter Clausen <lars@...afoo.de> To: Nicolas Boichat <drinkcat@...omium.org> CC: Mark Brown <broonie@...nel.org>, Arjan van de Ven <arjan@...ux.intel.com>, Mauro Carvalho Chehab <mchehab@....samsung.com>, Antti Palosaari <crope@....fi>, Ingo Molnar <mingo@...hat.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, linux-kernel@...r.kernel.org, Bard Liao <bardliao@...ltek.com>, Oder Chiou <oder_chiou@...ltek.com>, Liam Girdwood <lgirdwood@...il.com>, Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.de>, alsa-devel@...a-project.org, Anatol Pomozov <anatol.pomozov@...il.com> Subject: Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep On 06/29/2015 04:03 PM, Nicolas Boichat wrote: > On Mon, Jun 29, 2015 at 8:59 PM, Lars-Peter Clausen <lars@...afoo.de> wrote: >> On 06/29/2015 02:51 PM, Nicolas Boichat wrote: >>> >>> On Fri, Jun 26, 2015 at 11:16 AM, Nicolas Boichat <drinkcat@...omium.org> >>> wrote: >>>> >>>> >>>> On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <broonie@...nel.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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists