| Message ID | 1421419194-1849-2-git-send-email-p.osmialowsk@samsung.com |
|---|---|
| State | RFC |
| Headers | show |
On Fri, Jan 16, 2015 at 03:39:53PM +0100, Paul Osmialowski wrote: > This uses the enhancement of i2c API in order to address following problem > caused by circular lock dependency: Please don't just dump enormous backtraces into commit messages as explanations, explain in words what the problem you are trying to address is. If the backtrace is longer than the commit message things probably aren't working well, and similarly if the first thing the reader sees is several screenfuls of backtrace that's not really aiding understanding. This is all particularly important with something like locking where a clear understanding of the rules and assumptions being made is very important to ensuring correctness, it's easy to just paper over a specific problem and miss a bigger problem or introduce new ones. > Apparently regulator and clock try to acquire lock which is protecting the > same regmap. Communication over i2c requires clock to be started. Both things > require access to the same regmap in order to complete. > To solve this, i2c clock should be started before attempting operation on > regulator (which requires locked regmap). It sounds awfully like something is not doing the right thing with preparing clocks somewhere along the line but since there's no analysis it's hard to tell (I don't propose to spend time trawling backtraces for something I don't know). Please also use blank lines between paragraphs like all the other commit messages, it makes things easier to read. > Exposing preparation and unpreparation stage of i2c transfer serves this > purpose. I don't know what this means, sorry. I'm also very worried about the fact that this is being discussed purely in terms of I2C - why would this not affect other buses? > Note that this change does not require modifications in other places. > > Signed-off-by: Paul Osmialowski <p.osmialowsk@samsung.com> > --- > drivers/base/regmap/internal.h | 2 + > drivers/base/regmap/regmap-i2c.c | 18 +++++++ > drivers/base/regmap/regmap.c | 107 ++++++++++++++++++++++++++++++++++++++- > include/linux/regmap.h | 7 +++ > 4 files changed, 133 insertions(+), 1 deletion(-) Modification may not be required in other places but this is an *enormous* change in the code which I can't really review. > + int (*reg_prepare_sync_io)(void *context); > + void (*reg_unprepare_sync_io)(void *context); The first question here is why this only affects synchronous I/O or alternatively why these operations have _sync in the name if they aren't for synchronous I/O. > + if (bus) { > + map->reg_prepare_sync_io = regmap_bus_prepare_sync_io; > + map->reg_unprepare_sync_io = regmap_bus_unprepare_sync_io; > + } Why are we using these indirections instead of assigning the operation directly? They... > +static int regmap_bus_prepare_sync_io(void *context) > +{ > + struct regmap *map = context; > + > + if (map->bus->prepare_sync_io) > + return map->bus->prepare_sync_io(map->bus_context); > + > + return 0; > +} ...seem to simply check for the operation which appears redundant especially given that the caller... > +static void regmap_unprepare_sync_io(struct regmap *map) > +{ > + void *context = _regmap_map_get_context(map); > + > + if (map->reg_unprepare_sync_io) > + map->reg_unprepare_sync_io(context); > +} ...does essentially the same check again on every call anyway. > @@ -1491,12 +1536,18 @@ int regmap_write(struct regmap *map, unsigned int reg, unsigned int val) > if (reg % map->reg_stride) > return -EINVAL; > > + ret = regmap_prepare_sync_io(map); > + if (ret) > + return ret; > + > map->lock(map->lock_arg); So what are the rules for calling this operation and how are they different to those for locking the map? It looks like they might be the same in which case it seems better to combine them rather than having to update every single caller and remember why they're always being worked with in tandem.
On Fri, 16 Jan 2015, Mark Brown wrote: > On Fri, Jan 16, 2015 at 03:39:53PM +0100, Paul Osmialowski wrote: > >> This uses the enhancement of i2c API in order to address following problem >> caused by circular lock dependency: > > Please don't just dump enormous backtraces into commit messages as > explanations, explain in words what the problem you are trying to > address is. If the backtrace is longer than the commit message things > probably aren't working well, and similarly if the first thing the > reader sees is several screenfuls of backtrace that's not really aiding > understanding. > > This is all particularly important with something like locking where a > clear understanding of the rules and assumptions being made is very > important to ensuring correctness, it's easy to just paper over a > specific problem and miss a bigger problem or introduce new ones. Got it. I couldn't estimate how much is too much, sorry for that. > >> Apparently regulator and clock try to acquire lock which is protecting the >> same regmap. Communication over i2c requires clock to be started. Both things >> require access to the same regmap in order to complete. >> To solve this, i2c clock should be started before attempting operation on >> regulator (which requires locked regmap). > > It sounds awfully like something is not doing the right thing with > preparing clocks somewhere along the line but since there's no > analysis it's hard to tell (I don't propose to spend time trawling > backtraces for something I don't know). I have alternative solution for this particular problem waiting for local review which splits regmaps so it is not shared between two things anymore and I guess it will gain acceptance easier. Thing is, this alternative solution solves problem for this particular chip (mfd max77686) while approach discussed here is a (merely) step into more general solution (when more complicated sharing of regmaps causes problem with multi-function devices). > > Please also use blank lines between paragraphs like all the other commit > messages, it makes things easier to read. > Got it. >> Exposing preparation and unpreparation stage of i2c transfer serves this >> purpose. > > I don't know what this means, sorry. I'm also very worried about the > fact that this is being discussed purely in terms of I2C - why would > this not affect other buses? > I tried to open some gate for further extension to any bus that is used for regmap communications. Currently it goes down to regmap-i2c.c since I enhanced i2c API for this. Anyone who feels it is useful or saves oneself from locking troubles can voluntarily adapt other regmap-i2c.* places (as needed?). My whole point is that I proposed a way to solve nasty deadlock which is better to fix than just leave as it is. I got a feeling that situation I adressed here may occur others too, so I proposed this extension that allows future adaptations. I don't expect it to be accepted easily (i.e. I'm new here and have mixed feelins about proposing changes that go so far), therefore I prepared other solution for this particular deadlock that occurs on this particular device. >> Note that this change does not require modifications in other places. >> >> Signed-off-by: Paul Osmialowski <p.osmialowsk@samsung.com> >> --- >> drivers/base/regmap/internal.h | 2 + >> drivers/base/regmap/regmap-i2c.c | 18 +++++++ >> drivers/base/regmap/regmap.c | 107 ++++++++++++++++++++++++++++++++++++++- >> include/linux/regmap.h | 7 +++ >> 4 files changed, 133 insertions(+), 1 deletion(-) > > Modification may not be required in other places but this is an > *enormous* change in the code which I can't really review. > >> + int (*reg_prepare_sync_io)(void *context); >> + void (*reg_unprepare_sync_io)(void *context); > > The first question here is why this only affects synchronous I/O or > alternatively why these operations have _sync in the name if they aren't > for synchronous I/O. > IMHO this whole idea is against asynchronous I/O. >> + if (bus) { >> + map->reg_prepare_sync_io = regmap_bus_prepare_sync_io; >> + map->reg_unprepare_sync_io = regmap_bus_unprepare_sync_io; >> + } > > Why are we using these indirections instead of assigning the operation > directly? They... > I followed the pattern used throughout this file. >> +static int regmap_bus_prepare_sync_io(void *context) >> +{ >> + struct regmap *map = context; >> + >> + if (map->bus->prepare_sync_io) >> + return map->bus->prepare_sync_io(map->bus_context); >> + >> + return 0; >> +} > > ...seem to simply check for the operation which appears redundant > especially given that the caller... > Indeed, this checking is mostly for ensuring that old behaviour is kept intact. >> +static void regmap_unprepare_sync_io(struct regmap *map) >> +{ >> + void *context = _regmap_map_get_context(map); >> + >> + if (map->reg_unprepare_sync_io) >> + map->reg_unprepare_sync_io(context); >> +} > > ...does essentially the same check again on every call anyway. > I hope it doesn't hurt too much. Keeping existing pattern of the file and ensuring old behaviour is kept intact has its price :( It may seem reduntant, but I'd better hear what original authors of this file think about it. >> @@ -1491,12 +1536,18 @@ int regmap_write(struct regmap *map, unsigned int reg, unsigned int val) >> if (reg % map->reg_stride) >> return -EINVAL; >> >> + ret = regmap_prepare_sync_io(map); >> + if (ret) >> + return ret; >> + >> map->lock(map->lock_arg); > > So what are the rules for calling this operation and how are they > different to those for locking the map? It looks like they might be the > same in which case it seems better to combine them rather than having > to update every single caller and remember why they're always being > worked with in tandem. > At first I thought about putting this preparation call into lock() callback. Then I realised that the same callback is used for async communication too where async is set true AFTER the lock is obtained. Thanks for your comments. Hope for more. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 16, 2015 at 06:36:14PM +0100, Paul Osmialowski wrote: > On Fri, 16 Jan 2015, Mark Brown wrote: > >I don't know what this means, sorry. I'm also very worried about the > >fact that this is being discussed purely in terms of I2C - why would > >this not affect other buses? > I tried to open some gate for further extension to any bus that is used for > regmap communications. Currently it goes down to regmap-i2c.c since I > enhanced i2c API for this. Anyone who feels it is useful or saves oneself > from locking troubles can voluntarily adapt other regmap-i2c.* places (as > needed?). > My whole point is that I proposed a way to solve nasty deadlock which is > better to fix than just leave as it is. I got a feeling that situation I > adressed here may occur others too, so I proposed this extension that allows > future adaptations. I don't expect it to be accepted easily (i.e. I'm new > here and have mixed feelins about proposing changes that go so far), > therefore I prepared other solution for this particular deadlock that occurs > on this particular device. What I'm saying is that I want to understand this change from a point of view that isn't tied to I2C - at the regmap level what is this doing, I2C is a bus that has some properties which you're saying needs some changes, what are those properties and those changes? > >>+ void (*reg_unprepare_sync_io)(void *context); > >The first question here is why this only affects synchronous I/O or > >alternatively why these operations have _sync in the name if they aren't > >for synchronous I/O. > IMHO this whole idea is against asynchronous I/O. Can you be more specific please? If something needs preparing it seems like it'd need preparing over an async transaction just as much as over a synchronous one. > >>+ if (bus) { > >>+ map->reg_prepare_sync_io = regmap_bus_prepare_sync_io; > >>+ map->reg_unprepare_sync_io = regmap_bus_unprepare_sync_io; > >>+ } > >Why are we using these indirections instead of assigning the operation > >directly? They... > I followed the pattern used throughout this file. Not in this pattern where the caller needs to check too.
On Fri, 16 Jan 2015, Mark Brown wrote: > On Fri, Jan 16, 2015 at 06:36:14PM +0100, Paul Osmialowski wrote: >> On Fri, 16 Jan 2015, Mark Brown wrote: > >>> I don't know what this means, sorry. I'm also very worried about the >>> fact that this is being discussed purely in terms of I2C - why would >>> this not affect other buses? > >> I tried to open some gate for further extension to any bus that is used for >> regmap communications. Currently it goes down to regmap-i2c.c since I >> enhanced i2c API for this. Anyone who feels it is useful or saves oneself >> from locking troubles can voluntarily adapt other regmap-i2c.* places (as >> needed?). > >> My whole point is that I proposed a way to solve nasty deadlock which is >> better to fix than just leave as it is. I got a feeling that situation I >> adressed here may occur others too, so I proposed this extension that allows >> future adaptations. I don't expect it to be accepted easily (i.e. I'm new >> here and have mixed feelins about proposing changes that go so far), >> therefore I prepared other solution for this particular deadlock that occurs >> on this particular device. > > What I'm saying is that I want to understand this change from a point of > view that isn't tied to I2C - at the regmap level what is this doing, From the regmap point of view, it allows its functions to have a chance to prepare transfer medium for (synchronous) transfer (no matter what bus handles it) before it actually start to happen (then unprepare it when it's done) and crucially before any lock is obtained in functions like regmap_write(), regmap_read() or regmap_update_bits. Maybe adding a pair of callbacks (map->reg_write_sync_prepared(), map->reg_read_sync_prepared()) would make situation clearer. > I2C is a bus that has some properties which you're saying needs some > changes, what are those properties and those changes? I'm not saying I2C as a bus requires changes. What I'm saying is that I2C API can be extended to allow more detailed control on what happens with the transfer. > >>>> + void (*reg_unprepare_sync_io)(void *context); > >>> The first question here is why this only affects synchronous I/O or >>> alternatively why these operations have _sync in the name if they aren't >>> for synchronous I/O. > >> IMHO this whole idea is against asynchronous I/O. > > Can you be more specific please? If something needs preparing it seems > like it'd need preparing over an async transaction just as much as over > a synchronous one. > Even with those preparation and unpreparation stages, this transfer is still synchronous. For example, it starts when regmap_read() starts and ends when regmap_read() ends. Nothing is queued or deferred. Namely, when max_gen_clk_unprepare() function calls regmap_update_bits() it expects that when regmap_update_bits() returned, no outstanding transfer are happening nor waiting to proceed. Everything must be completed before returning to max_gen_clk_unprepare(). >>>> + if (bus) { >>>> + map->reg_prepare_sync_io = regmap_bus_prepare_sync_io; >>>> + map->reg_unprepare_sync_io = regmap_bus_unprepare_sync_io; >>>> + } > >>> Why are we using these indirections instead of assigning the operation >>> directly? They... > >> I followed the pattern used throughout this file. > > Not in this pattern where the caller needs to check too. > I don't persist on that. Apparently, you're the author of this file, though regmap_init() function was later expanded by other guys. They never assigned bus callback function pointers directly to map operation callbacks. It is possible to replace 'map->reg_prepare_sync_io = regmap_bus_prepare_sync_io' with 'map->reg_prepare_sync_io = map->bus->prepare_sync_io' - this will compile and this will work properly. But IMHO it wouldn't match with what the others did. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 19, 2015 at 10:31:22AM +0100, Paul Osmialowski wrote: > On Fri, 16 Jan 2015, Mark Brown wrote: > >What I'm saying is that I want to understand this change from a point of > >view that isn't tied to I2C - at the regmap level what is this doing, > From the regmap point of view, it allows its functions to have a chance to > prepare transfer medium for (synchronous) transfer (no matter what bus > handles it) before it actually start to happen (then unprepare it when it's > done) and crucially before any lock is obtained in functions like > regmap_write(), regmap_read() or regmap_update_bits. OK, so that's what should go in the changelog (along with an explanation of why this preparation is required at all) - but I still don't see the async bit of this I'm afraid. > Maybe adding a pair of callbacks (map->reg_write_sync_prepared(), > map->reg_read_sync_prepared()) would make situation clearer. No, I don't think so - it'd just complicate the callers. > >I2C is a bus that has some properties which you're saying needs some > >changes, what are those properties and those changes? > I'm not saying I2C as a bus requires changes. What I'm saying is that I2C > API can be extended to allow more detailed control on what happens with the > transfer. My point here is that your explanation is in terms of I2C specifics and not really at a generic regmap level. > >Can you be more specific please? If something needs preparing it seems > >like it'd need preparing over an async transaction just as much as over > >a synchronous one. > Even with those preparation and unpreparation stages, this transfer is still > synchronous. For example, it starts when regmap_read() starts and ends when > regmap_read() ends. Nothing is queued or deferred. Namely, when > max_gen_clk_unprepare() function calls regmap_update_bits() it expects that > when regmap_update_bits() returned, no outstanding transfer are happening > nor waiting to proceed. Everything must be completed before returning to > max_gen_clk_unprepare(). That doesn't address my question - all you're saying is that in a synchronous call path things are synchronous which is fine but obviously regmap supports async I/O too. > >Not in this pattern where the caller needs to check too. > I don't persist on that. Apparently, you're the author of this file, though > regmap_init() function was later expanded by other guys. They never assigned > bus callback function pointers directly to map operation callbacks. It is > possible to replace 'map->reg_prepare_sync_io = regmap_bus_prepare_sync_io' > with 'map->reg_prepare_sync_io = map->bus->prepare_sync_io' - this will > compile and this will work properly. But IMHO it wouldn't match with what > the others did. If you look at the other callbacks they're doing other things beyond simply forwarding the functions on. That's the problem here, the functions just add a layer of indirection and nothing else.
On Mon, 19 Jan 2015, Mark Brown wrote: > On Mon, Jan 19, 2015 at 10:31:22AM +0100, Paul Osmialowski wrote: >> On Fri, 16 Jan 2015, Mark Brown wrote: > >>> What I'm saying is that I want to understand this change from a point of >>> view that isn't tied to I2C - at the regmap level what is this doing, > >> From the regmap point of view, it allows its functions to have a chance to >> prepare transfer medium for (synchronous) transfer (no matter what bus >> handles it) before it actually start to happen (then unprepare it when it's >> done) and crucially before any lock is obtained in functions like >> regmap_write(), regmap_read() or regmap_update_bits. > > OK, so that's what should go in the changelog (along with an explanation > of why this preparation is required at all) - but I still don't see the > async bit of this I'm afraid. > I don't think preparation stage should be exposed for asynchronous transfer. Due to its nature, it shouldn't cause circular lock dependency as we can observe for synchronous io. Since i2c does not support async transfer anyway (so (map->async && map->bus->async_write) will be always false for i2c transfers), let's use spi as an example. With spi we have curious situation where both sync and async are handled by the same __spi_async() function, though for sync transfer wait_for_completion() is called soon after __spi_async() in order to ensure that transfer is completed. Actual transfer is handled by spi_transfer_one_message() called from spi_pump_messages(). Note that spi_pump_message() before it calls spi_transfer_one_message() also calls prepare_message() callback (if provided): if (master->prepare_message) { ret = master->prepare_message(master, master->cur_msg); So we have some candidate to expose if we would like to modify regmap-spi.c similarly to regmap-i2c.c. Thing is, we don't need to expose it for asynchronous transfer. If master->prepare_message() callback obtains some lock, which is unfortunately held by other task that waits for a lock already obtained by regmap_write_async() it will not wait forever since call path started in regmap_write_async() is not blocked by things like wait_for_completion() and should soon end calling map->unlock(map->lock_arg). >> Maybe adding a pair of callbacks (map->reg_write_sync_prepared(), >> map->reg_read_sync_prepared()) would make situation clearer. > > No, I don't think so - it'd just complicate the callers. > >>> I2C is a bus that has some properties which you're saying needs some >>> changes, what are those properties and those changes? > >> I'm not saying I2C as a bus requires changes. What I'm saying is that I2C >> API can be extended to allow more detailed control on what happens with the >> transfer. > > My point here is that your explanation is in terms of I2C specifics and > not really at a generic regmap level. > >>> Can you be more specific please? If something needs preparing it seems >>> like it'd need preparing over an async transaction just as much as over >>> a synchronous one. > >> Even with those preparation and unpreparation stages, this transfer is still >> synchronous. For example, it starts when regmap_read() starts and ends when >> regmap_read() ends. Nothing is queued or deferred. Namely, when >> max_gen_clk_unprepare() function calls regmap_update_bits() it expects that >> when regmap_update_bits() returned, no outstanding transfer are happening >> nor waiting to proceed. Everything must be completed before returning to >> max_gen_clk_unprepare(). > > That doesn't address my question - all you're saying is that in a > synchronous call path things are synchronous which is fine but obviously > regmap supports async I/O too. > >>> Not in this pattern where the caller needs to check too. > >> I don't persist on that. Apparently, you're the author of this file, though >> regmap_init() function was later expanded by other guys. They never assigned >> bus callback function pointers directly to map operation callbacks. It is >> possible to replace 'map->reg_prepare_sync_io = regmap_bus_prepare_sync_io' >> with 'map->reg_prepare_sync_io = map->bus->prepare_sync_io' - this will >> compile and this will work properly. But IMHO it wouldn't match with what >> the others did. > > If you look at the other callbacks they're doing other things beyond > simply forwarding the functions on. That's the problem here, the > functions just add a layer of indirection and nothing else. > Yes, I added layer of indirection just because the others added layer of indirection - there is no functional requirement behind that. It can be easily removed. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 20, 2015 at 12:14:31PM +0100, Paul Osmialowski wrote: > On Mon, 19 Jan 2015, Mark Brown wrote: > >OK, so that's what should go in the changelog (along with an explanation > >of why this preparation is required at all) - but I still don't see the > >async bit of this I'm afraid. > I don't think preparation stage should be exposed for asynchronous transfer. > Due to its nature, it shouldn't cause circular lock dependency as we can > observe for synchronous io. Since i2c does not support async transfer anyway > (so (map->async && map->bus->async_write) will be always false for i2c > transfers), let's use spi as an example. Again I come back to explaining this out of the context of this particular issue in terms of something that's comprehensible at the regmap level. > With spi we have curious situation where both sync and async are handled by > the same __spi_async() function, though for sync transfer > wait_for_completion() is called soon after __spi_async() in order to ensure > that transfer is completed. That's not actually that strange really, in general synchronous operation can always be implemented as a simple wrapper around asynchronous operation. > Actual transfer is handled by spi_transfer_one_message() called from > spi_pump_messages(). Note that spi_pump_message() before it calls > spi_transfer_one_message() also calls prepare_message() callback (if > provided): We are *massively* into implementation details here, if we're relying on these things then they could change at any time (and in fact what you say above is partly specific and is not true even in the default case for current code). Think in terms of abstractions and locking guarantees rather than the details of the current code.
diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h index 0da5865..fc8cbc9 100644 --- a/drivers/base/regmap/internal.h +++ b/drivers/base/regmap/internal.h @@ -94,6 +94,8 @@ struct regmap { const struct regmap_access_table *volatile_table; const struct regmap_access_table *precious_table; + int (*reg_prepare_sync_io)(void *context); + void (*reg_unprepare_sync_io)(void *context); int (*reg_read)(void *context, unsigned int reg, unsigned int *val); int (*reg_write)(void *context, unsigned int reg, unsigned int val); diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c index 053150a..09e95a6 100644 --- a/drivers/base/regmap/regmap-i2c.c +++ b/drivers/base/regmap/regmap-i2c.c @@ -87,6 +87,22 @@ static struct regmap_bus regmap_smbus_word = { .reg_read = regmap_smbus_word_reg_read, }; +static int regmap_i2c_transfer_prepare(void *context) +{ + struct device *dev = context; + struct i2c_client *i2c = to_i2c_client(dev); + + return i2c_transfer_prepare(i2c->adapter); +} + +static void regmap_i2c_transfer_unprepare(void *context) +{ + struct device *dev = context; + struct i2c_client *i2c = to_i2c_client(dev); + + i2c_transfer_unprepare(i2c->adapter); +} + static int regmap_i2c_write(void *context, const void *data, size_t count) { struct device *dev = context; @@ -165,6 +181,8 @@ static int regmap_i2c_read(void *context, } static struct regmap_bus regmap_i2c = { + .prepare_sync_io = regmap_i2c_transfer_prepare, + .unprepare_sync_io = regmap_i2c_transfer_unprepare, .write = regmap_i2c_write, .gather_write = regmap_i2c_gather_write, .read = regmap_i2c_read, diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index d2f8a81..69e7d6b 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -36,6 +36,9 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg, unsigned int mask, unsigned int val, bool *change); +static int regmap_bus_prepare_sync_io(void *context); +static void regmap_bus_unprepare_sync_io(void *context); + static int _regmap_bus_reg_read(void *context, unsigned int reg, unsigned int *val); static int _regmap_bus_read(void *context, unsigned int reg, @@ -617,6 +620,11 @@ struct regmap *regmap_init(struct device *dev, map->reg_read = _regmap_bus_read; } + if (bus) { + map->reg_prepare_sync_io = regmap_bus_prepare_sync_io; + map->reg_unprepare_sync_io = regmap_bus_unprepare_sync_io; + } + reg_endian = regmap_get_reg_endian(bus, config); val_endian = regmap_get_val_endian(dev, bus, config); @@ -1440,11 +1448,48 @@ static int _regmap_bus_raw_write(void *context, unsigned int reg, map->format.val_bytes); } +static int regmap_bus_prepare_sync_io(void *context) +{ + struct regmap *map = context; + + if (map->bus->prepare_sync_io) + return map->bus->prepare_sync_io(map->bus_context); + + return 0; +} + +static void regmap_bus_unprepare_sync_io(void *context) +{ + struct regmap *map = context; + + if (map->bus->unprepare_sync_io) + map->bus->unprepare_sync_io(map->bus_context); +} + static inline void *_regmap_map_get_context(struct regmap *map) { return (map->bus) ? map : map->bus_context; } +static int regmap_prepare_sync_io(struct regmap *map) +{ + void *context = _regmap_map_get_context(map); + + if (map->reg_prepare_sync_io) + return map->reg_prepare_sync_io(context); + + return 0; +} + +static void regmap_unprepare_sync_io(struct regmap *map) +{ + void *context = _regmap_map_get_context(map); + + if (map->reg_unprepare_sync_io) + map->reg_unprepare_sync_io(context); +} + + int _regmap_write(struct regmap *map, unsigned int reg, unsigned int val) { @@ -1491,12 +1536,18 @@ int regmap_write(struct regmap *map, unsigned int reg, unsigned int val) if (reg % map->reg_stride) return -EINVAL; + ret = regmap_prepare_sync_io(map); + if (ret) + return ret; + map->lock(map->lock_arg); ret = _regmap_write(map, reg, val); map->unlock(map->lock_arg); + regmap_unprepare_sync_io(map); + return ret; } EXPORT_SYMBOL_GPL(regmap_write); @@ -1558,12 +1609,18 @@ int regmap_raw_write(struct regmap *map, unsigned int reg, if (val_len % map->format.val_bytes) return -EINVAL; + ret = regmap_prepare_sync_io(map); + if (ret) + return ret; + map->lock(map->lock_arg); ret = _regmap_raw_write(map, reg, val, val_len); map->unlock(map->lock_arg); + regmap_unprepare_sync_io(map); + return ret; } EXPORT_SYMBOL_GPL(regmap_raw_write); @@ -1669,7 +1726,7 @@ EXPORT_SYMBOL_GPL(regmap_fields_update_bits); int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val, size_t val_count) { - int ret = 0, i; + int ret = 0, i, retv; size_t val_bytes = map->format.val_bytes; if (map->bus && !map->format.parse_inplace) @@ -1682,6 +1739,9 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val, * them we have a series of single write operations. */ if (!map->bus || map->use_single_rw) { + retv = regmap_prepare_sync_io(map); + if (retv) + return retv; map->lock(map->lock_arg); for (i = 0; i < val_count; i++) { unsigned int ival; @@ -1713,15 +1773,21 @@ int regmap_bulk_write(struct regmap *map, unsigned int reg, const void *val, } out: map->unlock(map->lock_arg); + regmap_unprepare_sync_io(map); } else { void *wval; if (!val_count) return -EINVAL; + retv = regmap_prepare_sync_io(map); + if (retv) + return retv; + wval = kmemdup(val, val_count * val_bytes, GFP_KERNEL); if (!wval) { dev_err(map->dev, "Error in memory allocation\n"); + regmap_unprepare_sync_io(map); return -ENOMEM; } for (i = 0; i < val_count * val_bytes; i += val_bytes) @@ -1732,6 +1798,8 @@ out: map->unlock(map->lock_arg); kfree(wval); + + regmap_unprepare_sync_io(map); } return ret; } @@ -1936,12 +2004,18 @@ int regmap_multi_reg_write(struct regmap *map, const struct reg_default *regs, { int ret; + ret = regmap_prepare_sync_io(map); + if (ret) + return ret; + map->lock(map->lock_arg); ret = _regmap_multi_reg_write(map, regs, num_regs); map->unlock(map->lock_arg); + regmap_unprepare_sync_io(map); + return ret; } EXPORT_SYMBOL_GPL(regmap_multi_reg_write); @@ -1970,6 +2044,10 @@ int regmap_multi_reg_write_bypassed(struct regmap *map, int ret; bool bypass; + ret = regmap_prepare_sync_io(map); + if (ret) + return ret; + map->lock(map->lock_arg); bypass = map->cache_bypass; @@ -1981,6 +2059,8 @@ int regmap_multi_reg_write_bypassed(struct regmap *map, map->unlock(map->lock_arg); + regmap_unprepare_sync_io(map); + return ret; } EXPORT_SYMBOL_GPL(regmap_multi_reg_write_bypassed); @@ -2148,12 +2228,18 @@ int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val) if (reg % map->reg_stride) return -EINVAL; + ret = regmap_prepare_sync_io(map); + if (ret) + return ret; + map->lock(map->lock_arg); ret = _regmap_read(map, reg, val); map->unlock(map->lock_arg); + regmap_unprepare_sync_io(map); + return ret; } EXPORT_SYMBOL_GPL(regmap_read); @@ -2184,6 +2270,10 @@ int regmap_raw_read(struct regmap *map, unsigned int reg, void *val, if (reg % map->reg_stride) return -EINVAL; + ret = regmap_prepare_sync_io(map); + if (ret) + return ret; + map->lock(map->lock_arg); if (regmap_volatile_range(map, reg, val_count) || map->cache_bypass || @@ -2208,6 +2298,8 @@ int regmap_raw_read(struct regmap *map, unsigned int reg, void *val, out: map->unlock(map->lock_arg); + regmap_unprepare_sync_io(map); + return ret; } EXPORT_SYMBOL_GPL(regmap_raw_read); @@ -2370,10 +2462,16 @@ int regmap_update_bits(struct regmap *map, unsigned int reg, { int ret; + ret = regmap_prepare_sync_io(map); + if (ret) + return ret; + map->lock(map->lock_arg); ret = _regmap_update_bits(map, reg, mask, val, NULL); map->unlock(map->lock_arg); + regmap_unprepare_sync_io(map); + return ret; } EXPORT_SYMBOL_GPL(regmap_update_bits); @@ -2430,9 +2528,16 @@ int regmap_update_bits_check(struct regmap *map, unsigned int reg, { int ret; + ret = regmap_prepare_sync_io(map); + if (ret) + return ret; + map->lock(map->lock_arg); ret = _regmap_update_bits(map, reg, mask, val, change); map->unlock(map->lock_arg); + + regmap_unprepare_sync_io(map); + return ret; } EXPORT_SYMBOL_GPL(regmap_update_bits_check); diff --git a/include/linux/regmap.h b/include/linux/regmap.h index 4419b99..74aace5 100644 --- a/include/linux/regmap.h +++ b/include/linux/regmap.h @@ -265,6 +265,8 @@ struct regmap_range_cfg { struct regmap_async; +typedef int (*regmap_hw_prepare_sync_io)(void *context); +typedef void (*regmap_hw_unprepare_sync_io)(void *context); typedef int (*regmap_hw_write)(void *context, const void *data, size_t count); typedef int (*regmap_hw_gather_write)(void *context, @@ -291,6 +293,9 @@ typedef void (*regmap_hw_free_context)(void *context); * to perform locking. This field is ignored if custom lock/unlock * functions are used (see fields lock/unlock of * struct regmap_config). + * @prepare_sync_io: Prepare for read/write operation (if needed, e.g. to obtain + * locks earlier). + * @unprepare_sync_io: Unprepare after read/write operation (if needed). * @write: Write operation. * @gather_write: Write operation with split register/value, return -ENOTSUPP * if not implemented on a given device. @@ -311,6 +316,8 @@ typedef void (*regmap_hw_free_context)(void *context); */ struct regmap_bus { bool fast_io; + regmap_hw_prepare_sync_io prepare_sync_io; + regmap_hw_unprepare_sync_io unprepare_sync_io; regmap_hw_write write; regmap_hw_gather_write gather_write; regmap_hw_async_write async_write;
This uses the enhancement of i2c API in order to address following problem caused by circular lock dependency: -> #1 (prepare_lock){+.+.+.}: [ 2.730502] [<c0061e50>] __lock_acquire+0x3c0/0x8a4 [ 2.735970] [<c0062868>] lock_acquire+0x6c/0x8c [ 2.741090] [<c04a2724>] mutex_lock_nested+0x68/0x464 [ 2.746733] [<c0395e84>] clk_prepare_lock+0x40/0xe8 [ 2.752201] [<c0397698>] clk_unprepare+0x18/0x28 [ 2.757409] [<c034cbb0>] s3c24xx_i2c_xfer+0xc8/0x124 [ 2.762964] [<c03457e0>] __i2c_transfer+0x74/0x8c [ 2.768259] [<c0347234>] i2c_transfer+0x78/0xec [ 2.773380] [<c02a177c>] regmap_i2c_read+0x48/0x64 [ 2.778761] [<c029d5c0>] _regmap_raw_read+0xa8/0xfc [ 2.784230] [<c029d920>] _regmap_bus_read+0x28/0x48 [ 2.789699] [<c029ce00>] _regmap_read+0x74/0xdc [ 2.794819] [<c029d0ec>] _regmap_update_bits+0x24/0x70 [ 2.800549] [<c029e348>] regmap_update_bits+0x40/0x5c [ 2.806190] [<c024c3c4>] _regulator_do_disable+0x68/0x7c [ 2.812093] [<c024f4d8>] _regulator_disable+0x90/0x12c [ 2.817822] [<c024f5a8>] regulator_disable+0x34/0x60 [ 2.823377] [<c0363070>] mmc_regulator_set_ocr+0x74/0xdc [ 2.829279] [<c03783e8>] sdhci_set_power+0x38/0x20c [ 2.834748] [<c0379c10>] sdhci_do_set_ios+0x180/0x450 [ 2.840389] [<c0379f00>] sdhci_set_ios+0x20/0x2c [ 2.845597] [<c0364978>] mmc_set_initial_state+0x5c/0xbc [ 2.851500] [<c0364f48>] mmc_power_off+0x2c/0x60 [ 2.856708] [<c0365c00>] mmc_rescan+0x268/0x27c [ 2.861829] [<c003a128>] process_one_work+0x18c/0x3f8 [ 2.867471] [<c003a400>] worker_thread+0x38/0x2f8 [ 2.872766] [<c003f66c>] kthread+0xe4/0x104 [ 2.877540] [<c000f0a8>] ret_from_fork+0x14/0x2c [ 2.882749] -> #0 (&map->mutex){+.+...}: [ 2.886828] [<c0061224>] validate_chain.isra.25+0x3bc/0x548 [ 2.892990] [<c0061e50>] __lock_acquire+0x3c0/0x8a4 [ 2.898459] [<c0062868>] lock_acquire+0x6c/0x8c [ 2.903580] [<c04a2724>] mutex_lock_nested+0x68/0x464 [ 2.909222] [<c029ce9c>] regmap_read+0x34/0x5c [ 2.914257] [<c039a994>] max_gen_clk_is_prepared+0x1c/0x38 [ 2.920333] [<c0396ec4>] clk_unprepare_unused_subtree+0x64/0x98 [ 2.926842] [<c0396f78>] clk_disable_unused+0x80/0xd8 [ 2.932484] [<c00089d0>] do_one_initcall+0xac/0x1f0 [ 2.937953] [<c068bd44>] do_basic_setup+0x90/0xc8 [ 2.943248] [<c068be00>] kernel_init_freeable+0x84/0x120 [ 2.949150] [<c0491248>] kernel_init+0x8/0xec [ 2.954097] [<c000f0a8>] ret_from_fork+0x14/0x2c [ 2.959307] [ 2.959307] other info that might help us debug this: [ 2.959307] [ 2.967293] Possible unsafe locking scenario: [ 2.967293] [ 2.973194] CPU0 CPU1 [ 2.977708] ---- ---- [ 2.982221] lock(prepare_lock); [ 2.985520] lock(&map->mutex); [ 2.991248] lock(prepare_lock); [ 2.997063] lock(&map->mutex); [ 3.000276] [ 3.000276] *** DEADLOCK *** Apparently regulator and clock try to acquire lock which is protecting the same regmap. Communication over i2c requires clock to be started. Both things require access to the same regmap in order to complete. To solve this, i2c clock should be started before attempting operation on regulator (which requires locked regmap). Exposing preparation and unpreparation stage of i2c transfer serves this purpose. Proposed changes in regmap and regmap-i2c make use of exposed i2c transfer preparation and unpreparation stages. Note that this change does not require modifications in other places. Signed-off-by: Paul Osmialowski <p.osmialowsk@samsung.com> --- drivers/base/regmap/internal.h | 2 + drivers/base/regmap/regmap-i2c.c | 18 +++++++ drivers/base/regmap/regmap.c | 107 ++++++++++++++++++++++++++++++++++++++- include/linux/regmap.h | 7 +++ 4 files changed, 133 insertions(+), 1 deletion(-)