lkml.org 
[lkml]   [2015]   [Jan]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC 2/3] regmap: Use the enhancement of i2c API to address circular dependency problem


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.



\
 
 \ /
  Last update: 2015-01-16 19:01    [W:0.063 / U:1.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site