| 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
| ||
|
Date: Mon, 29 Jun 2015 22:03:09 +0800 From: Nicolas Boichat <drinkcat@...omium.org> To: Lars-Peter Clausen <lars@...afoo.de> 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 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). >> >> 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... -- 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