| Message ID | 1435224904-35517-1-git-send-email-drinkcat@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 6/25/2015 2:35 AM, Nicolas Boichat wrote: > From: Antti Palosaari <crope@iki.fi> > > 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 <crope@iki.fi> >> >> 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 <lars@metafoo.de> 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 <broonie@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.
On 06/26/2015 04:34 AM, Nicolas Boichat wrote: > On Thu, Jun 25, 2015 at 11:59 PM, Lars-Peter Clausen <lars@metafoo.de> 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 <drinkcat@chromium.org> wrote: > > On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <broonie@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!
On 06/29/2015 02:51 PM, Nicolas Boichat wrote: > On Fri, Jun 26, 2015 at 11:16 AM, Nicolas Boichat <drinkcat@chromium.org> wrote: >> >> On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <broonie@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. > > 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 <lars@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@chromium.org> >> wrote: >>> >>> >>> On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <broonie@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). >> >> 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 <lars@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@chromium.org> >>> wrote: >>>> >>>> >>>> On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <broonie@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
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.
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);