Re: [RFC 2/3] regmap: Use the enhancement of i2c API to address circular dependency problem

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxxx>
---
 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux