[RFC 1/3] i2c: Enhancement of i2c API to address circular lock dependency problem

[RFC 1/3] i2c: Enhancement of i2c API to address circular lock dependency problem

[Paul Osmialowski]

This enhancement of i2c API is designed 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.
Note that this change does not require modifications in other places:
old behaviour is kept preserved. Anyone who requires this new way of using i2c
transfer can adapt their drivers voluntarily.
Separate commit adapts regmap to use this new feature.

Signed-off-by: Paul Osmialowski <p.osmialowsk@samsung.com>
---
 Documentation/i2c/writing-clients |  15 ++++++
 drivers/i2c/i2c-core.c            | 105 ++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h               |  16 ++++++
 3 files changed, 136 insertions(+)

diff --git a/Documentation/i2c/writing-clients b/Documentation/i2c/writing-clients
index a755b14..d6732a8 100644
--- a/Documentation/i2c/writing-clients
+++ b/Documentation/i2c/writing-clients
@@ -343,6 +343,21 @@ stop bit is sent between transaction. The i2c_msg structure contains
 for each message the client address, the number of bytes of the message
 and the message data itself.
 
+	int i2c_transfer_prepare(struct i2c_adapter *adap);
+	void i2c_transfer_unprepare(struct i2c_adapter *adap);
+
+These routines are used for calling exposed prepare and unprepare stages of
+the i2c_transfer() function. Normally i2c_transfer() does some operations
+(e.g. clock preparation) before actual transfer. These operations are undone
+when transfer is finished. Unfortunately these preparations may take locks,
+even causing circular lock dependency. Sometimes the only way to avoid this
+is to call i2c_transfer_prepare() earlier. Note that these routines do
+nothing when bus driver does not implement `master_prepare_xfer' nor
+`master_unprepare_xfer' callbacks pointed by struct i2c_algorithm.
+Note that underlying bus driver implementing `master_xfer` callback
+should be aware of the fact that preparation stage was or was not invoked
+before i2c_transfer() function call and act accordingly.
+
 You can read the file `i2c-protocol' for more information about the
 actual I2C protocol.
 
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 39d25a8..d649475 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2064,6 +2064,111 @@ module_exit(i2c_exit);
  */
 
 /**
+ * __i2c_transfer_prepare - unlocked flavor of prepare stage of transfer of
+ *                          a single or combined I2C message (may sleep).
+ * @adap: Handle to I2C bus
+ *
+ * This stage, if exposed by underlying driver, can be called before transfer
+ * in order to avoid deadlock caused by circular lock dependency.
+ *
+ * Returns 0 on success.
+ *
+ * Adapter lock must be held when calling this function. No debug logging
+ * takes place.
+ */
+int __i2c_transfer_prepare(struct i2c_adapter *adap)
+{
+	if (adap->algo->master_prepare_xfer)
+		return adap->algo->master_prepare_xfer(adap);
+
+	return 0;
+}
+EXPORT_SYMBOL(__i2c_transfer_prepare);
+
+/**
+ * i2c_transfer_prepare - prepare transfer of a single or combined I2C message
+ *                        (may sleep).
+ * @adap: Handle to I2C bus
+ *
+ * Returns 0 on success.
+ *
+ * This stage, if exposed by underlying driver, can be called before transfer
+ * in order to avoid deadlock caused by circular lock dependency.
+ */
+int i2c_transfer_prepare(struct i2c_adapter *adap)
+{
+	int ret = 0;
+
+	if (adap->algo->master_prepare_xfer) {
+		if (in_atomic() || irqs_disabled()) {
+			ret = i2c_trylock_adapter(adap);
+			if (!ret)
+				/* I2C activity is ongoing. */
+				return -EAGAIN;
+		} else {
+			i2c_lock_adapter(adap);
+		}
+		ret = __i2c_transfer_prepare(adap);
+		i2c_unlock_adapter(adap);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(i2c_transfer_prepare);
+
+/**
+ * __i2c_transfer_unprepare - unlocked flavor of prepare stage of transfer of
+ *                          a single or combined I2C message (may sleep).
+ * @adap: Handle to I2C bus
+ *
+ * This stage, if exposed by underlying driver, can be called afrer transfer
+ * in order to avoid deadlock caused by circular lock dependency.
+ *
+ * Adapter lock must be held when calling this function. No debug logging
+ * takes place.
+ */
+void __i2c_transfer_unprepare(struct i2c_adapter *adap)
+{
+	if (adap->algo->master_unprepare_xfer)
+		adap->algo->master_unprepare_xfer(adap);
+}
+EXPORT_SYMBOL(__i2c_transfer_unprepare);
+
+/**
+ * i2c_transfer_unprepare - unprepare after transfer of a single or combined
+ *                          I2C message (may sleep).
+ * @adap: Handle to I2C bus
+ *
+ * This stage, if exposed by underlying driver, can be called afrer transfer
+ * in order to avoid deadlock caused by circular lock dependency.
+ */
+void i2c_transfer_unprepare(struct i2c_adapter *adap)
+{
+	if (adap->algo->master_unprepare_xfer) {
+		if (in_atomic() || irqs_disabled()) {
+			/* Wait max 20 ms */
+			int timeout = 20;
+
+			while (!(i2c_trylock_adapter(adap))) {
+				if (timeout == 0) {
+					/* I2C activity is still ongoing. */
+					pr_err("%s: can't unprepare transfer\n",
+					__func__);
+					return;
+				}
+				timeout--;
+				mdelay(1);
+			}
+		} else {
+			i2c_lock_adapter(adap);
+		}
+		__i2c_transfer_unprepare(adap);
+		i2c_unlock_adapter(adap);
+	}
+}
+EXPORT_SYMBOL(i2c_transfer_unprepare);
+
+/**
  * __i2c_transfer - unlocked flavor of i2c_transfer
  * @adap: Handle to I2C bus
  * @msgs: One or more messages to execute before STOP is issued to
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index e3a1721..fce2a5a 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -64,6 +64,14 @@ extern int i2c_master_send(const struct i2c_client *client, const char *buf,
 extern int i2c_master_recv(const struct i2c_client *client, char *buf,
 			   int count);
 
+/* Transfer prepare/unprepare stage (if needed/exposed).
+ */
+extern int i2c_transfer_prepare(struct i2c_adapter *adap);
+extern void i2c_transfer_unprepare(struct i2c_adapter *adap);
+/* Unlocked flavor */
+extern int __i2c_transfer_prepare(struct i2c_adapter *adap);
+extern void __i2c_transfer_unprepare(struct i2c_adapter *adap);
+
 /* Transfer num messages.
  */
 extern int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
@@ -371,6 +379,12 @@ i2c_register_board_info(int busnum, struct i2c_board_info const *info,
  * @master_xfer: Issue a set of i2c transactions to the given I2C adapter
  *   defined by the msgs array, with num messages available to transfer via
  *   the adapter specified by adap.
+ * @master_prepare_xfer: Exposed preparation stage of @master_xfer - use when
+ *   master_xfer must be split into stages: prepare, xfer, unprepare.
+ *   Set to NULL when not exposed.
+ * @master_unprepare_xfer: Exposed unpreparation stage of @master_xfer -
+ *   use when master_xfer must be split into stages: prepare, xfer, unprepare.
+ *   Set to NULL when not exposed.
  * @smbus_xfer: Issue smbus transactions to the given I2C adapter. If this
  *   is not present, then the bus layer will try and convert the SMBus calls
  *   into I2C transfers instead.
@@ -397,6 +411,8 @@ struct i2c_algorithm {
 	   processed, or a negative value on error */
 	int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
 			   int num);
+	int (*master_prepare_xfer)(struct i2c_adapter *adap);
+	void (*master_unprepare_xfer)(struct i2c_adapter *adap);
 	int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
 			   unsigned short flags, char read_write,
 			   u8 command, int size, union i2c_smbus_data *data);
-- 
1.9.1

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

[Paul Osmialowski]

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(-)

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;
-- 
1.9.1

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4048 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Paul Osmialowski]

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.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2156 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Paul Osmialowski]

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.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2944 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Paul Osmialowski]

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.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1745 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RFC 3/3] i2c: s3c2410: Adopt i2c-s3c2410 driver for new enhancement of i2c API

[Paul Osmialowski]

This adopts i2c-s3c2410 driver for new enhancement of i2c API that
exposes preparation and unpreparation stages of i2c transfer.

Signed-off-by: Paul Osmialowski <p.osmialowsk@samsung.com>
---
 drivers/i2c/busses/i2c-s3c2410.c | 69 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 63 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index bff20a5..4af8a9c 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -130,6 +130,8 @@ struct s3c24xx_i2c {
 #endif
 	struct regmap		*sysreg;
 	unsigned int		sys_i2c_cfg;
+
+	int			xfer_prepared;
 };
 
 static struct platform_device_id s3c24xx_driver_ids[] = {
@@ -771,6 +773,58 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
 	return ret;
 }
 
+/* s3c24xx_i2c_prepare_xfer
+ *
+ * prepare for transferring message across the i2c bus (may sleep)
+ *
+ * returns 0 on success
+ */
+
+static int s3c24xx_i2c_prepare_xfer(struct i2c_adapter *adap)
+{
+	struct s3c24xx_i2c *i2c = (struct s3c24xx_i2c *)adap->algo_data;
+
+	if (i2c->xfer_prepared == 0) {
+		int ret;
+
+		pm_runtime_get_sync(&adap->dev);
+
+		ret = clk_prepare_enable(i2c->clk);
+		if (ret) {
+			pm_runtime_put(&adap->dev);
+			return ret;
+		}
+
+	}
+
+	i2c->xfer_prepared++;
+
+	return 0;
+}
+
+/* s3c24xx_i2c_unprepare_xfer
+ *
+ * unprepare after transferring message across the i2c bus (may sleep)
+ */
+
+static void s3c24xx_i2c_unprepare_xfer(struct i2c_adapter *adap)
+{
+	struct s3c24xx_i2c *i2c = (struct s3c24xx_i2c *)adap->algo_data;
+
+	if (i2c->xfer_prepared == 1) {
+		clk_disable_unprepare(i2c->clk);
+		pm_runtime_put(&adap->dev);
+	}
+
+	i2c->xfer_prepared--;
+
+	if (unlikely(i2c->xfer_prepared < 0)) {
+		pr_err("%s: i2c->xfer_prepared = %d < 0\n",
+			__func__, i2c->xfer_prepared);
+		i2c->xfer_prepared = 0;
+	}
+}
+
 /* s3c24xx_i2c_xfer
  *
  * first port of call from the i2c bus code when an message needs
@@ -784,16 +838,16 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
 	int retry;
 	int ret;
 
-	pm_runtime_get_sync(&adap->dev);
-	clk_prepare_enable(i2c->clk);
+	ret = s3c24xx_i2c_prepare_xfer(adap);
+	if (ret)
+		return ret;
 
 	for (retry = 0; retry < adap->retries; retry++) {
 
 		ret = s3c24xx_i2c_doxfer(i2c, msgs, num);
 
 		if (ret != -EAGAIN) {
-			clk_disable_unprepare(i2c->clk);
-			pm_runtime_put(&adap->dev);
+			s3c24xx_i2c_unprepare_xfer(adap);
 			return ret;
 		}
 
@@ -802,8 +856,8 @@ static int s3c24xx_i2c_xfer(struct i2c_adapter *adap,
 		udelay(100);
 	}
 
-	clk_disable_unprepare(i2c->clk);
-	pm_runtime_put(&adap->dev);
+	s3c24xx_i2c_unprepare_xfer(adap);
+
 	return -EREMOTEIO;
 }
 
@@ -818,6 +872,8 @@ static u32 s3c24xx_i2c_func(struct i2c_adapter *adap)
 
 static const struct i2c_algorithm s3c24xx_i2c_algorithm = {
 	.master_xfer		= s3c24xx_i2c_xfer,
+	.master_prepare_xfer	= s3c24xx_i2c_prepare_xfer,
+	.master_unprepare_xfer	= s3c24xx_i2c_unprepare_xfer,
 	.functionality		= s3c24xx_i2c_func,
 };
 
@@ -1152,6 +1208,7 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 	i2c->adap.retries = 2;
 	i2c->adap.class = I2C_CLASS_DEPRECATED;
 	i2c->tx_setup = 50;
+	i2c->xfer_prepared = 0;
 
 	init_waitqueue_head(&i2c->wait);
 
-- 
1.9.1

Re: [RFC 3/3] i2c: s3c2410: Adopt i2c-s3c2410 driver for new enhancement of i2c API

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 447 bytes --]

On Fri, Jan 16, 2015 at 03:39:54PM +0100, Paul Osmialowski wrote:
> This adopts i2c-s3c2410 driver for new enhancement of i2c API that
> exposes preparation and unpreparation stages of i2c transfer.

This doesn't seem to have any dependency on the previous patch at all...
it probably does want a better commit log, probably the subject should
say what new enhancement of the API is being adopted.  ("Use
prepare/unprepare transfer" for example.)

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC 1/3] i2c: Enhancement of i2c API to address circular lock dependency problem

[Tomasz Figa]

Hi,

[CCing more people]

2015-01-16 23:39 GMT+09:00 Paul Osmialowski <p.osmialowsk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>:
> This enhancement of i2c API is designed 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.

I stumbled upon this issue (and reported it) quite long time ago
already, but apparently nobody cared too much (including myself,
unfortunately). Please see [1] for details.

[1] https://lkml.org/lkml/2014/7/2/171

We have here a typical ABBA deadlock scenario, between I2C clock
providers and other (logical) devices on the same (physical) I2C
device, for which a regmap is used to share the registers between
drivers. Remaining factor here is I2C controller driver, which must
perform clock gating in I2C ops to trigger this deadlock.

The problem is that any operation on such I2C clock will first try to
acquire clk_prepare_mutex and then, through driver's callback, access
the regmap, acquiring its mutex (then an I2C transfer will happen, but
it irrelevant in this context). On opposite side we have drivers for
other functionality exposed by this I2C device, which will access the
regmap, acquiring its mutex and causing I2C transfers to happen.

The key here is that I2C transfers might require some clocks to be
prepared, so clk_prepare() might get called from this context and
cause a deadlock, because clk_prepare_mutex might have been already
acquired by another context, waiting for regmap mutex, which has been
already acquired by this context.

Now, for the solution, the approach proposed by Paul, as far as I
could understand it by reading the code (it's definitely lacking a
cover letter with detailed explanation), should solve the issue by
enforcing correct locking order at regmap level. However I wonder if
we really need a heavy solution like this or we could just make I2C
drivers not require clock preparation in I2C transfers, as suggested
by Peter De Schrijver in [1], which should fix the issue as well.

So, the question is, do we actually have hardware that _really_
requires _actual_ preparation or all the clk_prepare_enable()s in I2C
drivers (at least in i2c-s3c2410) are just to simplify the code?

Best regards,
Tomasz

Re: [RFC 1/3] i2c: Enhancement of i2c API to address circular lock dependency problem

[Krzysztof Kozlowski]

W dniu 18.01.2015 o 07:30, Tomasz Figa pisze:
> Hi,
>
> [CCing more people]
>
> 2015-01-16 23:39 GMT+09:00 Paul Osmialowski <p.osmialowsk@samsung.com>:
>> This enhancement of i2c API is designed 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.
>
> I stumbled upon this issue (and reported it) quite long time ago
> already, but apparently nobody cared too much (including myself,
> unfortunately). Please see [1] for details.
>
> [1] https://lkml.org/lkml/2014/7/2/171
>
> We have here a typical ABBA deadlock scenario, between I2C clock
> providers and other (logical) devices on the same (physical) I2C
> device, for which a regmap is used to share the registers between
> drivers. Remaining factor here is I2C controller driver, which must
> perform clock gating in I2C ops to trigger this deadlock.
>
> The problem is that any operation on such I2C clock will first try to
> acquire clk_prepare_mutex and then, through driver's callback, access
> the regmap, acquiring its mutex (then an I2C transfer will happen, but
> it irrelevant in this context). On opposite side we have drivers for
> other functionality exposed by this I2C device, which will access the
> regmap, acquiring its mutex and causing I2C transfers to happen.
>
> The key here is that I2C transfers might require some clocks to be
> prepared, so clk_prepare() might get called from this context and
> cause a deadlock, because clk_prepare_mutex might have been already
> acquired by another context, waiting for regmap mutex, which has been
> already acquired by this context.
>
> Now, for the solution, the approach proposed by Paul, as far as I
> could understand it by reading the code (it's definitely lacking a
> cover letter with detailed explanation), should solve the issue by
> enforcing correct locking order at regmap level. However I wonder if
> we really need a heavy solution like this or we could just make I2C
> drivers not require clock preparation in I2C transfers, as suggested
> by Peter De Schrijver in [1], which should fix the issue as well.
>
> So, the question is, do we actually have hardware that _really_
> requires _actual_ preparation or all the clk_prepare_enable()s in I2C
> drivers (at least in i2c-s3c2410) are just to simplify the code?


Hi Tomasz,

I completely forgot that you already thought about this deadlock in 2014. I 
think we can try the no-prepare way for i2c-s3c2410. However this would be 
only workaround for specific chip. Other buses (like SPI) would require 
similar changes.

I wondered why this was not observed (at least not observed by me with 
lockdep) on Gear 2 (Rinato) board. This is quite similar case: the S2MPS14 
PMIC provides regulators and 32kHz clocks. I think it is exactly the same 
pattern as for max77686... but somehow lockdep never reported that deadlock 
there.

Anyway thanks for pointing out old discussion.

Best regards,
Krzysztof

>
> Best regards,
> Tomasz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [RFC 1/3] i2c: Enhancement of i2c API to address circular lock dependency problem

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

On Sun, Jan 18, 2015 at 11:54:56AM +0100, Krzysztof Kozlowski wrote:
> W dniu 18.01.2015 o 07:30, Tomasz Figa pisze:

> >So, the question is, do we actually have hardware that _really_
> >requires _actual_ preparation or all the clk_prepare_enable()s in I2C
> >drivers (at least in i2c-s3c2410) are just to simplify the code?

> I completely forgot that you already thought about this deadlock in 2014. I
> think we can try the no-prepare way for i2c-s3c2410. However this would be
> only workaround for specific chip. Other buses (like SPI) would require
> similar changes.

Right, and it's every single driver which would need an update too which
is a bit icky and sad.  On the other hand a more detailed fix is going
to involve trying to make the clock API locking more fine grained which
isn't fun.

> I wondered why this was not observed (at least not observed by me with
> lockdep) on Gear 2 (Rinato) board. This is quite similar case: the S2MPS14
> PMIC provides regulators and 32kHz clocks. I think it is exactly the same
> pattern as for max77686... but somehow lockdep never reported that deadlock
> there.

Mostly the clocks on PMICs never get changed at runtime which helps a
lot here.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC 1/3] i2c: Enhancement of i2c API to address circular lock dependency problem

[Mike Turquette]

Quoting Mark Brown (2015-01-18 05:41:24)
> On Sun, Jan 18, 2015 at 11:54:56AM +0100, Krzysztof Kozlowski wrote:
> > W dniu 18.01.2015 o 07:30, Tomasz Figa pisze:
> 
> > >So, the question is, do we actually have hardware that _really_
> > >requires _actual_ preparation or all the clk_prepare_enable()s in I2C
> > >drivers (at least in i2c-s3c2410) are just to simplify the code?
> 
> > I completely forgot that you already thought about this deadlock in 2014. I
> > think we can try the no-prepare way for i2c-s3c2410. However this would be
> > only workaround for specific chip. Other buses (like SPI) would require
> > similar changes.
> 
> Right, and it's every single driver which would need an update too which
> is a bit icky and sad.  On the other hand a more detailed fix is going
> to involve trying to make the clock API locking more fine grained which
> isn't fun.

Not fun is right. Please see Stephen's attempt here:

http://lkml.kernel.org/r/<1409792466-5092-1-git-send-email-sboyd@codeaurora.org>

I'm hoping this approach will be revisited soon.

Regards,
Mike

> 
> > I wondered why this was not observed (at least not observed by me with
> > lockdep) on Gear 2 (Rinato) board. This is quite similar case: the S2MPS14
> > PMIC provides regulators and 32kHz clocks. I think it is exactly the same
> > pattern as for max77686... but somehow lockdep never reported that deadlock
> > there.
> 
> Mostly the clocks on PMICs never get changed at runtime which helps a
> lot here.