Hi!
With the introduction of mux-locked I2C muxes, the concept of
locking only a segment of the I2C adapter tree was added. At the
time, I did not want to cause a lot of extra churn, so left most
users of i2c_lock_adapter alone and apparently didn't think enough
about it; they simply continued to lock the whole adapter tree.
However, i2c_lock_adapter is in fact wrong for almost every caller
(there is naturally an exception) that is itself not a driver for
a root adapter. What normal drivers generally want is to only
lock the segment of the adapter tree that their device sits on.
In fact, if a device sits behind a mux-locked I2C mux, and its
driver calls i2c_lock_adapter followed by an unlocked I2C transfer,
things will deadlock (since even a mux-locked I2C adapter will lock
its parent at some point). If the device is not sitting behind a
mux-locked I2C mux (i.e. either directly on the root adapter or
behind a (chain of) parent-locked I2C muxes) the root/segment
distinction is of no consequence; the root adapter is locked either
way.
Mux-locked I2C muxes are probably not that common, and putting any
of the affected devices behind one is probably even rarer, which
is why we have not seen any deadlocks. At least not that I know
of...
Since silently changing the semantics of i2c_lock_adapter might
be quite a surprise, especially for out-of-tree users, this series
instead removes the function and forces all users to explicitly
name I2C_LOCK_SEGMENT or I2C_LOCK_ROOT_ADAPTER in a call to
i2c_lock_bus, as suggested by Wolfram. Yes, users will be a teensy
bit more wordy, but open-coding I2C locking from random drivers
should be avoided, so it's perhaps a good thing if it doesn't look
too neat?
I suggest that Wolfram takes this series through the I2C tree and
creates an immutable branch for the other subsystems. The series
is based on v4.18-r1.
I do not have *any* of the affected devices, and have thus only
done build tests.
Cheers,
Peter
PS. for more background on mux-locked vs. parent-locked etc, see
Documentation/i2c/i2c-topology
Changes since v1:
- rebased to v4.18-rc1, thus removing the i2c-tegra hunk from
the last patch
- Not adding i2c_lock_segment (et al) and remove i2c_lock_adapter
instead of renaming it to i2c_lock_root, since having 8 closely
related inline locking functions in include/linux/i2c.h was
a few too many. I.e., instead going from 5 to 8, we are now
going from 5 to 3.
Peter Rosin (10):
tpm/tpm_i2c_infineon: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)
i2c: mux: pca9541: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)
input: rohm_bu21023: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)
media: af9013: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)
media: drxk_hard: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)
media: rtl2830: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)
media: tda1004x: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)
media: tda18271: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)
mfd: 88pm860x-i2c: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)
i2c: remove i2c_lock_adapter and use i2c_lock_bus directly
drivers/char/tpm/tpm_i2c_infineon.c | 8 +++----
drivers/i2c/busses/i2c-brcmstb.c | 8 +++----
drivers/i2c/busses/i2c-davinci.c | 4 ++--
drivers/i2c/busses/i2c-gpio.c | 40 ++++++++++++++++----------------
drivers/i2c/busses/i2c-s3c2410.c | 4 ++--
drivers/i2c/busses/i2c-sprd.c | 8 +++----
drivers/i2c/i2c-core-slave.c | 8 +++----
drivers/i2c/muxes/i2c-mux-pca9541.c | 6 ++---
drivers/iio/temperature/mlx90614.c | 4 ++--
drivers/input/touchscreen/rohm_bu21023.c | 4 ++--
drivers/media/dvb-frontends/af9013.c | 8 +++----
drivers/media/dvb-frontends/drxk_hard.c | 4 ++--
drivers/media/dvb-frontends/rtl2830.c | 12 +++++-----
drivers/media/dvb-frontends/tda1004x.c | 6 ++---
drivers/media/tuners/tda18271-common.c | 8 +++----
drivers/mfd/88pm860x-i2c.c | 8 +++----
include/linux/i2c.h | 12 ----------
17 files changed, 70 insertions(+), 82 deletions(-)
--
2.11.0
Locking the root adapter for __i2c_transfer will deadlock if the
device sits behind a mux-locked I2C mux. Switch to the finer-grained
i2c_lock_bus with the I2C_LOCK_SEGMENT flag. If the device does not
sit behind a mux-locked mux, the two locking variants are equivalent.
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/media/dvb-frontends/tda1004x.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/dvb-frontends/tda1004x.c b/drivers/media/dvb-frontends/tda1004x.c
index 7dcfb4a4b2d0..9d3261bf5090 100644
--- a/drivers/media/dvb-frontends/tda1004x.c
+++ b/drivers/media/dvb-frontends/tda1004x.c
@@ -329,7 +329,7 @@ static int tda1004x_do_upload(struct tda1004x_state *state,
tda1004x_write_byteI(state, dspCodeCounterReg, 0);
fw_msg.addr = state->config->demod_address;
- i2c_lock_adapter(state->i2c);
+ i2c_lock_bus(state->i2c, I2C_LOCK_SEGMENT);
buf[0] = dspCodeInReg;
while (pos != len) {
// work out how much to send this time
@@ -342,14 +342,14 @@ static int tda1004x_do_upload(struct tda1004x_state *state,
fw_msg.len = tx_size + 1;
if (__i2c_transfer(state->i2c, &fw_msg, 1) != 1) {
printk(KERN_ERR "tda1004x: Error during firmware upload\n");
- i2c_unlock_adapter(state->i2c);
+ i2c_unlock_bus(state->i2c, I2C_LOCK_SEGMENT);
return -EIO;
}
pos += tx_size;
dprintk("%s: fw_pos=0x%x\n", __func__, pos);
}
- i2c_unlock_adapter(state->i2c);
+ i2c_unlock_bus(state->i2c, I2C_LOCK_SEGMENT);
/* give the DSP a chance to settle 03/10/05 Hac */
msleep(100);
--
2.11.0
Locking the root adapter for __i2c_transfer will deadlock if the
device sits behind a mux-locked I2C mux. Switch to the finer-grained
i2c_lock_bus with the I2C_LOCK_SEGMENT flag. If the device does not
sit behind a mux-locked mux, the two locking variants are equivalent.
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/media/dvb-frontends/af9013.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/media/dvb-frontends/af9013.c b/drivers/media/dvb-frontends/af9013.c
index 482bce49819a..99361c113bca 100644
--- a/drivers/media/dvb-frontends/af9013.c
+++ b/drivers/media/dvb-frontends/af9013.c
@@ -1312,10 +1312,10 @@ static int af9013_wregs(struct i2c_client *client, u8 cmd, u16 reg,
memcpy(&buf[3], val, len);
if (lock)
- i2c_lock_adapter(client->adapter);
+ i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
ret = __i2c_transfer(client->adapter, msg, 1);
if (lock)
- i2c_unlock_adapter(client->adapter);
+ i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
if (ret < 0) {
goto err;
} else if (ret != 1) {
@@ -1353,10 +1353,10 @@ static int af9013_rregs(struct i2c_client *client, u8 cmd, u16 reg,
buf[2] = cmd;
if (lock)
- i2c_lock_adapter(client->adapter);
+ i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
ret = __i2c_transfer(client->adapter, msg, 2);
if (lock)
- i2c_unlock_adapter(client->adapter);
+ i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
if (ret < 0) {
goto err;
} else if (ret != 2) {
--
2.11.0
Locking the root adapter for __i2c_transfer will deadlock if the
device sits behind a mux-locked I2C mux. Switch to the finer-grained
i2c_lock_bus with the I2C_LOCK_SEGMENT flag. If the device does not
sit behind a mux-locked mux, the two locking variants are equivalent.
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/media/tuners/tda18271-common.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/media/tuners/tda18271-common.c b/drivers/media/tuners/tda18271-common.c
index 7e81cd887c13..054b3b747dae 100644
--- a/drivers/media/tuners/tda18271-common.c
+++ b/drivers/media/tuners/tda18271-common.c
@@ -225,7 +225,7 @@ static int __tda18271_write_regs(struct dvb_frontend *fe, int idx, int len,
*/
if (lock_i2c) {
tda18271_i2c_gate_ctrl(fe, 1);
- i2c_lock_adapter(priv->i2c_props.adap);
+ i2c_lock_bus(priv->i2c_props.adap, I2C_LOCK_SEGMENT);
}
while (len) {
if (max > len)
@@ -246,7 +246,7 @@ static int __tda18271_write_regs(struct dvb_frontend *fe, int idx, int len,
len -= max;
}
if (lock_i2c) {
- i2c_unlock_adapter(priv->i2c_props.adap);
+ i2c_unlock_bus(priv->i2c_props.adap, I2C_LOCK_SEGMENT);
tda18271_i2c_gate_ctrl(fe, 0);
}
@@ -300,7 +300,7 @@ int tda18271_init_regs(struct dvb_frontend *fe)
* as those could cause bad things
*/
tda18271_i2c_gate_ctrl(fe, 1);
- i2c_lock_adapter(priv->i2c_props.adap);
+ i2c_lock_bus(priv->i2c_props.adap, I2C_LOCK_SEGMENT);
/* initialize registers */
switch (priv->id) {
@@ -516,7 +516,7 @@ int tda18271_init_regs(struct dvb_frontend *fe)
/* synchronize */
__tda18271_write_regs(fe, R_EP1, 1, false);
- i2c_unlock_adapter(priv->i2c_props.adap);
+ i2c_unlock_bus(priv->i2c_props.adap, I2C_LOCK_SEGMENT);
tda18271_i2c_gate_ctrl(fe, 0);
return 0;
--
2.11.0
Locking the root adapter for __i2c_transfer will deadlock if the
device sits behind a mux-locked I2C mux. Switch to the finer-grained
i2c_lock_bus with the I2C_LOCK_SEGMENT flag. If the device does not
sit behind a mux-locked mux, the two locking variants are equivalent.
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/mfd/88pm860x-i2c.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/mfd/88pm860x-i2c.c b/drivers/mfd/88pm860x-i2c.c
index 84e313107233..7b9052ea7413 100644
--- a/drivers/mfd/88pm860x-i2c.c
+++ b/drivers/mfd/88pm860x-i2c.c
@@ -146,14 +146,14 @@ int pm860x_page_reg_write(struct i2c_client *i2c, int reg,
unsigned char zero;
int ret;
- i2c_lock_adapter(i2c->adapter);
+ i2c_lock_bus(i2c->adapter, I2C_LOCK_SEGMENT);
read_device(i2c, 0xFA, 0, &zero);
read_device(i2c, 0xFB, 0, &zero);
read_device(i2c, 0xFF, 0, &zero);
ret = write_device(i2c, reg, 1, &data);
read_device(i2c, 0xFE, 0, &zero);
read_device(i2c, 0xFC, 0, &zero);
- i2c_unlock_adapter(i2c->adapter);
+ i2c_unlock_bus(i2c->adapter, I2C_LOCK_SEGMENT);
return ret;
}
EXPORT_SYMBOL(pm860x_page_reg_write);
@@ -164,14 +164,14 @@ int pm860x_page_bulk_read(struct i2c_client *i2c, int reg,
unsigned char zero = 0;
int ret;
- i2c_lock_adapter(i2c->adapter);
+ i2c_lock_bus(i2c->adapter, I2C_LOCK_SEGMENT);
read_device(i2c, 0xfa, 0, &zero);
read_device(i2c, 0xfb, 0, &zero);
read_device(i2c, 0xff, 0, &zero);
ret = read_device(i2c, reg, count, buf);
read_device(i2c, 0xFE, 0, &zero);
read_device(i2c, 0xFC, 0, &zero);
- i2c_unlock_adapter(i2c->adapter);
+ i2c_unlock_bus(i2c->adapter, I2C_LOCK_SEGMENT);
return ret;
}
EXPORT_SYMBOL(pm860x_page_bulk_read);
--
2.11.0
Locking the root adapter for __i2c_transfer will deadlock if the
device sits behind a mux-locked I2C mux. Switch to the finer-grained
i2c_lock_bus with the I2C_LOCK_SEGMENT flag. If the device does not
sit behind a mux-locked mux, the two locking variants are equivalent.
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/input/touchscreen/rohm_bu21023.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/rohm_bu21023.c b/drivers/input/touchscreen/rohm_bu21023.c
index bda0500c9b57..714affdd742f 100644
--- a/drivers/input/touchscreen/rohm_bu21023.c
+++ b/drivers/input/touchscreen/rohm_bu21023.c
@@ -304,7 +304,7 @@ static int rohm_i2c_burst_read(struct i2c_client *client, u8 start, void *buf,
msg[1].len = len;
msg[1].buf = buf;
- i2c_lock_adapter(adap);
+ i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
for (i = 0; i < 2; i++) {
if (__i2c_transfer(adap, &msg[i], 1) < 0) {
@@ -313,7 +313,7 @@ static int rohm_i2c_burst_read(struct i2c_client *client, u8 start, void *buf,
}
}
- i2c_unlock_adapter(adap);
+ i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
return ret;
}
--
2.11.0
The i2c_lock_adapter name is ambiguous since it is unclear if it
refers to the root adapter or the adapter you name in the argument.
The natural interpretation is the adapter you name in the argument,
but there are historical reasons for that not being the case; it
in fact locks the root adapter. Just remove the function and force
users to spell out the I2C_LOCK_ROOT_ADAPTER name to indicate what
is really going on. Also remove i2c_unlock_adapter, of course.
This patch was generated with
git grep -l 'i2c_\(un\)\?lock_adapter' \
| xargs sed -i 's/i2c_\(un\)\?lock_adapter(\([^)]*\))/'\
'i2c_\1lock_bus(\2, I2C_LOCK_ROOT_ADAPTER)/g'
followed by white-space touch-up.
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/i2c/busses/i2c-brcmstb.c | 8 ++++----
drivers/i2c/busses/i2c-davinci.c | 4 ++--
drivers/i2c/busses/i2c-gpio.c | 40 +++++++++++++++++++-------------------
drivers/i2c/busses/i2c-s3c2410.c | 4 ++--
drivers/i2c/busses/i2c-sprd.c | 8 ++++----
drivers/i2c/i2c-core-slave.c | 8 ++++----
drivers/iio/temperature/mlx90614.c | 4 ++--
include/linux/i2c.h | 12 ------------
8 files changed, 38 insertions(+), 50 deletions(-)
diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c
index 78792b4d6437..826d32049996 100644
--- a/drivers/i2c/busses/i2c-brcmstb.c
+++ b/drivers/i2c/busses/i2c-brcmstb.c
@@ -689,9 +689,9 @@ static int brcmstb_i2c_suspend(struct device *dev)
{
struct brcmstb_i2c_dev *i2c_dev = dev_get_drvdata(dev);
- i2c_lock_adapter(&i2c_dev->adapter);
+ i2c_lock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
i2c_dev->is_suspended = true;
- i2c_unlock_adapter(&i2c_dev->adapter);
+ i2c_unlock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
return 0;
}
@@ -700,10 +700,10 @@ static int brcmstb_i2c_resume(struct device *dev)
{
struct brcmstb_i2c_dev *i2c_dev = dev_get_drvdata(dev);
- i2c_lock_adapter(&i2c_dev->adapter);
+ i2c_lock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
brcmstb_i2c_set_bsc_reg_defaults(i2c_dev);
i2c_dev->is_suspended = false;
- i2c_unlock_adapter(&i2c_dev->adapter);
+ i2c_unlock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
return 0;
}
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 75d6ab177055..d945a2654c2f 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -714,14 +714,14 @@ static int i2c_davinci_cpufreq_transition(struct notifier_block *nb,
dev = container_of(nb, struct davinci_i2c_dev, freq_transition);
- i2c_lock_adapter(&dev->adapter);
+ i2c_lock_bus(&dev->adapter, I2C_LOCK_ROOT_ADAPTER);
if (val == CPUFREQ_PRECHANGE) {
davinci_i2c_reset_ctrl(dev, 0);
} else if (val == CPUFREQ_POSTCHANGE) {
i2c_davinci_calc_clk_dividers(dev);
davinci_i2c_reset_ctrl(dev, 1);
}
- i2c_unlock_adapter(&dev->adapter);
+ i2c_unlock_bus(&dev->adapter, I2C_LOCK_ROOT_ADAPTER);
return 0;
}
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index 005e6e0330c2..9d63337efa82 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -78,24 +78,24 @@ static struct dentry *i2c_gpio_debug_dir;
#define getscl(bd) ((bd)->getscl((bd)->data))
#define WIRE_ATTRIBUTE(wire) \
-static int fops_##wire##_get(void *data, u64 *val) \
-{ \
- struct i2c_gpio_private_data *priv = data; \
- \
- i2c_lock_adapter(&priv->adap); \
- *val = get##wire(&priv->bit_data); \
- i2c_unlock_adapter(&priv->adap); \
- return 0; \
-} \
-static int fops_##wire##_set(void *data, u64 val) \
-{ \
- struct i2c_gpio_private_data *priv = data; \
- \
- i2c_lock_adapter(&priv->adap); \
- set##wire(&priv->bit_data, val); \
- i2c_unlock_adapter(&priv->adap); \
- return 0; \
-} \
+static int fops_##wire##_get(void *data, u64 *val) \
+{ \
+ struct i2c_gpio_private_data *priv = data; \
+ \
+ i2c_lock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER); \
+ *val = get##wire(&priv->bit_data); \
+ i2c_unlock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER); \
+ return 0; \
+} \
+static int fops_##wire##_set(void *data, u64 val) \
+{ \
+ struct i2c_gpio_private_data *priv = data; \
+ \
+ i2c_lock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER); \
+ set##wire(&priv->bit_data, val); \
+ i2c_unlock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER); \
+ return 0; \
+} \
DEFINE_DEBUGFS_ATTRIBUTE(fops_##wire, fops_##wire##_get, fops_##wire##_set, "%llu\n")
WIRE_ATTRIBUTE(scl);
@@ -113,7 +113,7 @@ static int fops_incomplete_transfer_set(void *data, u64 addr)
/* ADDR (7 bit) + RD (1 bit) + SDA hi (1 bit) */
pattern = (addr << 2) | 3;
- i2c_lock_adapter(&priv->adap);
+ i2c_lock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER);
/* START condition */
setsda(bit_data, 0);
@@ -129,7 +129,7 @@ static int fops_incomplete_transfer_set(void *data, u64 addr)
udelay(bit_data->udelay);
}
- i2c_unlock_adapter(&priv->adap);
+ i2c_unlock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER);
return 0;
}
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 9fe2b6951895..2f2e28d60ef5 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -919,9 +919,9 @@ static int s3c24xx_i2c_cpufreq_transition(struct notifier_block *nb,
if ((val == CPUFREQ_POSTCHANGE && delta_f < 0) ||
(val == CPUFREQ_PRECHANGE && delta_f > 0)) {
- i2c_lock_adapter(&i2c->adap);
+ i2c_lock_bus(&i2c->adap, I2C_LOCK_ROOT_ADAPTER);
ret = s3c24xx_i2c_clockrate(i2c, &got);
- i2c_unlock_adapter(&i2c->adap);
+ i2c_unlock_bus(&i2c->adap, I2C_LOCK_ROOT_ADAPTER);
if (ret < 0)
dev_err(i2c->dev, "cannot find frequency (%d)\n", ret);
diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index 4053259bccb8..a94e724f51dc 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -590,9 +590,9 @@ static int __maybe_unused sprd_i2c_suspend_noirq(struct device *pdev)
{
struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
- i2c_lock_adapter(&i2c_dev->adap);
+ i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
i2c_dev->is_suspended = true;
- i2c_unlock_adapter(&i2c_dev->adap);
+ i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
return pm_runtime_force_suspend(pdev);
}
@@ -601,9 +601,9 @@ static int __maybe_unused sprd_i2c_resume_noirq(struct device *pdev)
{
struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
- i2c_lock_adapter(&i2c_dev->adap);
+ i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
i2c_dev->is_suspended = false;
- i2c_unlock_adapter(&i2c_dev->adap);
+ i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
return pm_runtime_force_resume(pdev);
}
diff --git a/drivers/i2c/i2c-core-slave.c b/drivers/i2c/i2c-core-slave.c
index 4a78c65e9971..47a9f70a24a9 100644
--- a/drivers/i2c/i2c-core-slave.c
+++ b/drivers/i2c/i2c-core-slave.c
@@ -47,9 +47,9 @@ int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb)
client->slave_cb = slave_cb;
- i2c_lock_adapter(client->adapter);
+ i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
ret = client->adapter->algo->reg_slave(client);
- i2c_unlock_adapter(client->adapter);
+ i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
if (ret) {
client->slave_cb = NULL;
@@ -69,9 +69,9 @@ int i2c_slave_unregister(struct i2c_client *client)
return -EOPNOTSUPP;
}
- i2c_lock_adapter(client->adapter);
+ i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
ret = client->adapter->algo->unreg_slave(client);
- i2c_unlock_adapter(client->adapter);
+ i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
if (ret == 0)
client->slave_cb = NULL;
diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
index d619e8634a00..13a4cec64ea8 100644
--- a/drivers/iio/temperature/mlx90614.c
+++ b/drivers/iio/temperature/mlx90614.c
@@ -433,11 +433,11 @@ static int mlx90614_wakeup(struct mlx90614_data *data)
dev_dbg(&data->client->dev, "Requesting wake-up");
- i2c_lock_adapter(data->client->adapter);
+ i2c_lock_bus(data->client->adapter, I2C_LOCK_ROOT_ADAPTER);
gpiod_direction_output(data->wakeup_gpio, 0);
msleep(MLX90614_TIMING_WAKEUP);
gpiod_direction_input(data->wakeup_gpio);
- i2c_unlock_adapter(data->client->adapter);
+ i2c_unlock_bus(data->client->adapter, I2C_LOCK_ROOT_ADAPTER);
data->ready_timestamp = jiffies +
msecs_to_jiffies(MLX90614_TIMING_STARTUP);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 254cd34eeae2..795e3a860afe 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -754,18 +754,6 @@ i2c_unlock_bus(struct i2c_adapter *adapter, unsigned int flags)
adapter->lock_ops->unlock_bus(adapter, flags);
}
-static inline void
-i2c_lock_adapter(struct i2c_adapter *adapter)
-{
- i2c_lock_bus(adapter, I2C_LOCK_ROOT_ADAPTER);
-}
-
-static inline void
-i2c_unlock_adapter(struct i2c_adapter *adapter)
-{
- i2c_unlock_bus(adapter, I2C_LOCK_ROOT_ADAPTER);
-}
-
/*flags for the client struct: */
#define I2C_CLIENT_PEC 0x04 /* Use Packet Error Checking */
#define I2C_CLIENT_TEN 0x10 /* we have a ten bit chip address */
--
2.11.0
Locking the root adapter for __i2c_transfer will deadlock if the
device sits behind a mux-locked I2C mux. Switch to the finer-grained
i2c_lock_bus with the I2C_LOCK_SEGMENT flag. If the device does not
sit behind a mux-locked mux, the two locking variants are equivalent.
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/i2c/muxes/i2c-mux-pca9541.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c
index 6a39adaf433f..bc7c8cee5a8c 100644
--- a/drivers/i2c/muxes/i2c-mux-pca9541.c
+++ b/drivers/i2c/muxes/i2c-mux-pca9541.c
@@ -345,11 +345,11 @@ static int pca9541_probe(struct i2c_client *client,
/*
* I2C accesses are unprotected here.
- * We have to lock the adapter before releasing the bus.
+ * We have to lock the I2C segment before releasing the bus.
*/
- i2c_lock_adapter(adap);
+ i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
pca9541_release_bus(client);
- i2c_unlock_adapter(adap);
+ i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
/* Create mux adapter */
--
2.11.0
Locking the root adapter for __i2c_transfer will deadlock if the
device sits behind a mux-locked I2C mux. Switch to the finer-grained
i2c_lock_bus with the I2C_LOCK_SEGMENT flag. If the device does not
sit behind a mux-locked mux, the two locking variants are equivalent.
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/char/tpm/tpm_i2c_infineon.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
index 6116cd05e228..9086edc9066b 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -117,7 +117,7 @@ static int iic_tpm_read(u8 addr, u8 *buffer, size_t len)
/* Lock the adapter for the duration of the whole sequence. */
if (!tpm_dev.client->adapter->algo->master_xfer)
return -EOPNOTSUPP;
- i2c_lock_adapter(tpm_dev.client->adapter);
+ i2c_lock_bus(tpm_dev.client->adapter, I2C_LOCK_SEGMENT);
if (tpm_dev.chip_type == SLB9645) {
/* use a combined read for newer chips
@@ -192,7 +192,7 @@ static int iic_tpm_read(u8 addr, u8 *buffer, size_t len)
}
out:
- i2c_unlock_adapter(tpm_dev.client->adapter);
+ i2c_unlock_bus(tpm_dev.client->adapter, I2C_LOCK_SEGMENT);
/* take care of 'guard time' */
usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
@@ -224,7 +224,7 @@ static int iic_tpm_write_generic(u8 addr, u8 *buffer, size_t len,
if (!tpm_dev.client->adapter->algo->master_xfer)
return -EOPNOTSUPP;
- i2c_lock_adapter(tpm_dev.client->adapter);
+ i2c_lock_bus(tpm_dev.client->adapter, I2C_LOCK_SEGMENT);
/* prepend the 'register address' to the buffer */
tpm_dev.buf[0] = addr;
@@ -243,7 +243,7 @@ static int iic_tpm_write_generic(u8 addr, u8 *buffer, size_t len,
usleep_range(sleep_low, sleep_hi);
}
- i2c_unlock_adapter(tpm_dev.client->adapter);
+ i2c_unlock_bus(tpm_dev.client->adapter, I2C_LOCK_SEGMENT);
/* take care of 'guard time' */
usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
--
2.11.0
Locking the root adapter for __i2c_transfer will deadlock if the
device sits behind a mux-locked I2C mux. Switch to the finer-grained
i2c_lock_bus with the I2C_LOCK_SEGMENT flag. If the device does not
sit behind a mux-locked mux, the two locking variants are equivalent.
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/media/dvb-frontends/rtl2830.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/media/dvb-frontends/rtl2830.c b/drivers/media/dvb-frontends/rtl2830.c
index 7bbfe11d11ed..91d12e6a03d5 100644
--- a/drivers/media/dvb-frontends/rtl2830.c
+++ b/drivers/media/dvb-frontends/rtl2830.c
@@ -24,9 +24,9 @@ static int rtl2830_bulk_write(struct i2c_client *client, unsigned int reg,
struct rtl2830_dev *dev = i2c_get_clientdata(client);
int ret;
- i2c_lock_adapter(client->adapter);
+ i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
ret = regmap_bulk_write(dev->regmap, reg, val, val_count);
- i2c_unlock_adapter(client->adapter);
+ i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
return ret;
}
@@ -36,9 +36,9 @@ static int rtl2830_update_bits(struct i2c_client *client, unsigned int reg,
struct rtl2830_dev *dev = i2c_get_clientdata(client);
int ret;
- i2c_lock_adapter(client->adapter);
+ i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
ret = regmap_update_bits(dev->regmap, reg, mask, val);
- i2c_unlock_adapter(client->adapter);
+ i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
return ret;
}
@@ -48,9 +48,9 @@ static int rtl2830_bulk_read(struct i2c_client *client, unsigned int reg,
struct rtl2830_dev *dev = i2c_get_clientdata(client);
int ret;
- i2c_lock_adapter(client->adapter);
+ i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
ret = regmap_bulk_read(dev->regmap, reg, val, val_count);
- i2c_unlock_adapter(client->adapter);
+ i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
return ret;
}
--
2.11.0
Locking the root adapter for __i2c_transfer will deadlock if the
device sits behind a mux-locked I2C mux. Switch to the finer-grained
i2c_lock_bus with the I2C_LOCK_SEGMENT flag. If the device does not
sit behind a mux-locked mux, the two locking variants are equivalent.
Signed-off-by: Peter Rosin <[email protected]>
---
drivers/media/dvb-frontends/drxk_hard.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/dvb-frontends/drxk_hard.c b/drivers/media/dvb-frontends/drxk_hard.c
index 5a26ad93be10..29c36f95d624 100644
--- a/drivers/media/dvb-frontends/drxk_hard.c
+++ b/drivers/media/dvb-frontends/drxk_hard.c
@@ -213,7 +213,7 @@ static inline u32 log10times100(u32 value)
static int drxk_i2c_lock(struct drxk_state *state)
{
- i2c_lock_adapter(state->i2c);
+ i2c_lock_bus(state->i2c, I2C_LOCK_SEGMENT);
state->drxk_i2c_exclusive_lock = true;
return 0;
@@ -224,7 +224,7 @@ static void drxk_i2c_unlock(struct drxk_state *state)
if (!state->drxk_i2c_exclusive_lock)
return;
- i2c_unlock_adapter(state->i2c);
+ i2c_unlock_bus(state->i2c, I2C_LOCK_SEGMENT);
state->drxk_i2c_exclusive_lock = false;
}
--
2.11.0
On Wed, Jun 20, 2018 at 07:17:56AM +0200, Peter Rosin wrote:
> Locking the root adapter for __i2c_transfer will deadlock if the
> device sits behind a mux-locked I2C mux. Switch to the finer-grained
> i2c_lock_bus with the I2C_LOCK_SEGMENT flag. If the device does not
> sit behind a mux-locked mux, the two locking variants are equivalent.
>
> Signed-off-by: Peter Rosin <[email protected]>
Still
Acked-by: Dmitry Torokhov <[email protected]>
Feel free to merge through I2C tree.
> ---
> drivers/input/touchscreen/rohm_bu21023.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/touchscreen/rohm_bu21023.c b/drivers/input/touchscreen/rohm_bu21023.c
> index bda0500c9b57..714affdd742f 100644
> --- a/drivers/input/touchscreen/rohm_bu21023.c
> +++ b/drivers/input/touchscreen/rohm_bu21023.c
> @@ -304,7 +304,7 @@ static int rohm_i2c_burst_read(struct i2c_client *client, u8 start, void *buf,
> msg[1].len = len;
> msg[1].buf = buf;
>
> - i2c_lock_adapter(adap);
> + i2c_lock_bus(adap, I2C_LOCK_SEGMENT);
>
> for (i = 0; i < 2; i++) {
> if (__i2c_transfer(adap, &msg[i], 1) < 0) {
> @@ -313,7 +313,7 @@ static int rohm_i2c_burst_read(struct i2c_client *client, u8 start, void *buf,
> }
> }
>
> - i2c_unlock_adapter(adap);
> + i2c_unlock_bus(adap, I2C_LOCK_SEGMENT);
>
> return ret;
> }
> --
> 2.11.0
>
Thanks.
--
Dmitry
On Wed, Jun 20, 2018 at 07:17:54AM +0200, Peter Rosin wrote:
> Locking the root adapter for __i2c_transfer will deadlock if the
> device sits behind a mux-locked I2C mux. Switch to the finer-grained
> i2c_lock_bus with the I2C_LOCK_SEGMENT flag. If the device does not
> sit behind a mux-locked mux, the two locking variants are equivalent.
>
> Signed-off-by: Peter Rosin <[email protected]>
Studied enough so that I can give
Reviewed-by: Jarkko Sakkinen <[email protected]>
Do not have hardware to test this, however.
/Jarkko
On Wed, Jun 20, 2018 at 07:17:53AM +0200, Peter Rosin wrote:
> Hi!
>
> With the introduction of mux-locked I2C muxes, the concept of
> locking only a segment of the I2C adapter tree was added. At the
> time, I did not want to cause a lot of extra churn, so left most
> users of i2c_lock_adapter alone and apparently didn't think enough
> about it; they simply continued to lock the whole adapter tree.
> However, i2c_lock_adapter is in fact wrong for almost every caller
> (there is naturally an exception) that is itself not a driver for
> a root adapter. What normal drivers generally want is to only
> lock the segment of the adapter tree that their device sits on.
>
> In fact, if a device sits behind a mux-locked I2C mux, and its
> driver calls i2c_lock_adapter followed by an unlocked I2C transfer,
> things will deadlock (since even a mux-locked I2C adapter will lock
> its parent at some point). If the device is not sitting behind a
> mux-locked I2C mux (i.e. either directly on the root adapter or
> behind a (chain of) parent-locked I2C muxes) the root/segment
> distinction is of no consequence; the root adapter is locked either
> way.
>
> Mux-locked I2C muxes are probably not that common, and putting any
> of the affected devices behind one is probably even rarer, which
> is why we have not seen any deadlocks. At least not that I know
> of...
>
> Since silently changing the semantics of i2c_lock_adapter might
> be quite a surprise, especially for out-of-tree users, this series
> instead removes the function and forces all users to explicitly
> name I2C_LOCK_SEGMENT or I2C_LOCK_ROOT_ADAPTER in a call to
> i2c_lock_bus, as suggested by Wolfram. Yes, users will be a teensy
> bit more wordy, but open-coding I2C locking from random drivers
> should be avoided, so it's perhaps a good thing if it doesn't look
> too neat?
>
> I suggest that Wolfram takes this series through the I2C tree and
> creates an immutable branch for the other subsystems. The series
> is based on v4.18-r1.
Applied to a seperate branch named "i2c/precise-locking-names" which I
will merge into for-next, so it will get proper testing already. Once we
get the missing acks from media, MFD, and IIO maintainers, I will merge
it into for-4.19.
Thank you, Peter!
On Wed, 20 Jun 2018 07:18:03 +0200
Peter Rosin <[email protected]> wrote:
> The i2c_lock_adapter name is ambiguous since it is unclear if it
> refers to the root adapter or the adapter you name in the argument.
> The natural interpretation is the adapter you name in the argument,
> but there are historical reasons for that not being the case; it
> in fact locks the root adapter. Just remove the function and force
> users to spell out the I2C_LOCK_ROOT_ADAPTER name to indicate what
> is really going on. Also remove i2c_unlock_adapter, of course.
>
> This patch was generated with
>
> git grep -l 'i2c_\(un\)\?lock_adapter' \
> | xargs sed -i 's/i2c_\(un\)\?lock_adapter(\([^)]*\))/'\
> 'i2c_\1lock_bus(\2, I2C_LOCK_ROOT_ADAPTER)/g'
>
> followed by white-space touch-up.
>
> Signed-off-by: Peter Rosin <[email protected]>
For IIO: Acked-by: Jonathan Cameron <[email protected]>
> ---
> drivers/i2c/busses/i2c-brcmstb.c | 8 ++++----
> drivers/i2c/busses/i2c-davinci.c | 4 ++--
> drivers/i2c/busses/i2c-gpio.c | 40 +++++++++++++++++++-------------------
> drivers/i2c/busses/i2c-s3c2410.c | 4 ++--
> drivers/i2c/busses/i2c-sprd.c | 8 ++++----
> drivers/i2c/i2c-core-slave.c | 8 ++++----
> drivers/iio/temperature/mlx90614.c | 4 ++--
> include/linux/i2c.h | 12 ------------
> 8 files changed, 38 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c
> index 78792b4d6437..826d32049996 100644
> --- a/drivers/i2c/busses/i2c-brcmstb.c
> +++ b/drivers/i2c/busses/i2c-brcmstb.c
> @@ -689,9 +689,9 @@ static int brcmstb_i2c_suspend(struct device *dev)
> {
> struct brcmstb_i2c_dev *i2c_dev = dev_get_drvdata(dev);
>
> - i2c_lock_adapter(&i2c_dev->adapter);
> + i2c_lock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
> i2c_dev->is_suspended = true;
> - i2c_unlock_adapter(&i2c_dev->adapter);
> + i2c_unlock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
>
> return 0;
> }
> @@ -700,10 +700,10 @@ static int brcmstb_i2c_resume(struct device *dev)
> {
> struct brcmstb_i2c_dev *i2c_dev = dev_get_drvdata(dev);
>
> - i2c_lock_adapter(&i2c_dev->adapter);
> + i2c_lock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
> brcmstb_i2c_set_bsc_reg_defaults(i2c_dev);
> i2c_dev->is_suspended = false;
> - i2c_unlock_adapter(&i2c_dev->adapter);
> + i2c_unlock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
>
> return 0;
> }
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 75d6ab177055..d945a2654c2f 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -714,14 +714,14 @@ static int i2c_davinci_cpufreq_transition(struct notifier_block *nb,
>
> dev = container_of(nb, struct davinci_i2c_dev, freq_transition);
>
> - i2c_lock_adapter(&dev->adapter);
> + i2c_lock_bus(&dev->adapter, I2C_LOCK_ROOT_ADAPTER);
> if (val == CPUFREQ_PRECHANGE) {
> davinci_i2c_reset_ctrl(dev, 0);
> } else if (val == CPUFREQ_POSTCHANGE) {
> i2c_davinci_calc_clk_dividers(dev);
> davinci_i2c_reset_ctrl(dev, 1);
> }
> - i2c_unlock_adapter(&dev->adapter);
> + i2c_unlock_bus(&dev->adapter, I2C_LOCK_ROOT_ADAPTER);
>
> return 0;
> }
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index 005e6e0330c2..9d63337efa82 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -78,24 +78,24 @@ static struct dentry *i2c_gpio_debug_dir;
> #define getscl(bd) ((bd)->getscl((bd)->data))
>
> #define WIRE_ATTRIBUTE(wire) \
> -static int fops_##wire##_get(void *data, u64 *val) \
> -{ \
> - struct i2c_gpio_private_data *priv = data; \
> - \
> - i2c_lock_adapter(&priv->adap); \
> - *val = get##wire(&priv->bit_data); \
> - i2c_unlock_adapter(&priv->adap); \
> - return 0; \
> -} \
> -static int fops_##wire##_set(void *data, u64 val) \
> -{ \
> - struct i2c_gpio_private_data *priv = data; \
> - \
> - i2c_lock_adapter(&priv->adap); \
> - set##wire(&priv->bit_data, val); \
> - i2c_unlock_adapter(&priv->adap); \
> - return 0; \
> -} \
> +static int fops_##wire##_get(void *data, u64 *val) \
> +{ \
> + struct i2c_gpio_private_data *priv = data; \
> + \
> + i2c_lock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER); \
> + *val = get##wire(&priv->bit_data); \
> + i2c_unlock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER); \
> + return 0; \
> +} \
> +static int fops_##wire##_set(void *data, u64 val) \
> +{ \
> + struct i2c_gpio_private_data *priv = data; \
> + \
> + i2c_lock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER); \
> + set##wire(&priv->bit_data, val); \
> + i2c_unlock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER); \
> + return 0; \
> +} \
> DEFINE_DEBUGFS_ATTRIBUTE(fops_##wire, fops_##wire##_get, fops_##wire##_set, "%llu\n")
>
> WIRE_ATTRIBUTE(scl);
> @@ -113,7 +113,7 @@ static int fops_incomplete_transfer_set(void *data, u64 addr)
> /* ADDR (7 bit) + RD (1 bit) + SDA hi (1 bit) */
> pattern = (addr << 2) | 3;
>
> - i2c_lock_adapter(&priv->adap);
> + i2c_lock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER);
>
> /* START condition */
> setsda(bit_data, 0);
> @@ -129,7 +129,7 @@ static int fops_incomplete_transfer_set(void *data, u64 addr)
> udelay(bit_data->udelay);
> }
>
> - i2c_unlock_adapter(&priv->adap);
> + i2c_unlock_bus(&priv->adap, I2C_LOCK_ROOT_ADAPTER);
>
> return 0;
> }
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index 9fe2b6951895..2f2e28d60ef5 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -919,9 +919,9 @@ static int s3c24xx_i2c_cpufreq_transition(struct notifier_block *nb,
>
> if ((val == CPUFREQ_POSTCHANGE && delta_f < 0) ||
> (val == CPUFREQ_PRECHANGE && delta_f > 0)) {
> - i2c_lock_adapter(&i2c->adap);
> + i2c_lock_bus(&i2c->adap, I2C_LOCK_ROOT_ADAPTER);
> ret = s3c24xx_i2c_clockrate(i2c, &got);
> - i2c_unlock_adapter(&i2c->adap);
> + i2c_unlock_bus(&i2c->adap, I2C_LOCK_ROOT_ADAPTER);
>
> if (ret < 0)
> dev_err(i2c->dev, "cannot find frequency (%d)\n", ret);
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index 4053259bccb8..a94e724f51dc 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -590,9 +590,9 @@ static int __maybe_unused sprd_i2c_suspend_noirq(struct device *pdev)
> {
> struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
>
> - i2c_lock_adapter(&i2c_dev->adap);
> + i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
> i2c_dev->is_suspended = true;
> - i2c_unlock_adapter(&i2c_dev->adap);
> + i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
>
> return pm_runtime_force_suspend(pdev);
> }
> @@ -601,9 +601,9 @@ static int __maybe_unused sprd_i2c_resume_noirq(struct device *pdev)
> {
> struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
>
> - i2c_lock_adapter(&i2c_dev->adap);
> + i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
> i2c_dev->is_suspended = false;
> - i2c_unlock_adapter(&i2c_dev->adap);
> + i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
>
> return pm_runtime_force_resume(pdev);
> }
> diff --git a/drivers/i2c/i2c-core-slave.c b/drivers/i2c/i2c-core-slave.c
> index 4a78c65e9971..47a9f70a24a9 100644
> --- a/drivers/i2c/i2c-core-slave.c
> +++ b/drivers/i2c/i2c-core-slave.c
> @@ -47,9 +47,9 @@ int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb)
>
> client->slave_cb = slave_cb;
>
> - i2c_lock_adapter(client->adapter);
> + i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
> ret = client->adapter->algo->reg_slave(client);
> - i2c_unlock_adapter(client->adapter);
> + i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
>
> if (ret) {
> client->slave_cb = NULL;
> @@ -69,9 +69,9 @@ int i2c_slave_unregister(struct i2c_client *client)
> return -EOPNOTSUPP;
> }
>
> - i2c_lock_adapter(client->adapter);
> + i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
> ret = client->adapter->algo->unreg_slave(client);
> - i2c_unlock_adapter(client->adapter);
> + i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
>
> if (ret == 0)
> client->slave_cb = NULL;
> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
> index d619e8634a00..13a4cec64ea8 100644
> --- a/drivers/iio/temperature/mlx90614.c
> +++ b/drivers/iio/temperature/mlx90614.c
> @@ -433,11 +433,11 @@ static int mlx90614_wakeup(struct mlx90614_data *data)
>
> dev_dbg(&data->client->dev, "Requesting wake-up");
>
> - i2c_lock_adapter(data->client->adapter);
> + i2c_lock_bus(data->client->adapter, I2C_LOCK_ROOT_ADAPTER);
> gpiod_direction_output(data->wakeup_gpio, 0);
> msleep(MLX90614_TIMING_WAKEUP);
> gpiod_direction_input(data->wakeup_gpio);
> - i2c_unlock_adapter(data->client->adapter);
> + i2c_unlock_bus(data->client->adapter, I2C_LOCK_ROOT_ADAPTER);
>
> data->ready_timestamp = jiffies +
> msecs_to_jiffies(MLX90614_TIMING_STARTUP);
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 254cd34eeae2..795e3a860afe 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -754,18 +754,6 @@ i2c_unlock_bus(struct i2c_adapter *adapter, unsigned int flags)
> adapter->lock_ops->unlock_bus(adapter, flags);
> }
>
> -static inline void
> -i2c_lock_adapter(struct i2c_adapter *adapter)
> -{
> - i2c_lock_bus(adapter, I2C_LOCK_ROOT_ADAPTER);
> -}
> -
> -static inline void
> -i2c_unlock_adapter(struct i2c_adapter *adapter)
> -{
> - i2c_unlock_bus(adapter, I2C_LOCK_ROOT_ADAPTER);
> -}
> -
> /*flags for the client struct: */
> #define I2C_CLIENT_PEC 0x04 /* Use Packet Error Checking */
> #define I2C_CLIENT_TEN 0x10 /* we have a ten bit chip address */
On 25.06.2018 12:24, Jarkko Sakkinen wrote:
> On Wed, Jun 20, 2018 at 07:17:54AM +0200, Peter Rosin wrote:
>> Locking the root adapter for __i2c_transfer will deadlock if the
>> device sits behind a mux-locked I2C mux. Switch to the finer-grained
>> i2c_lock_bus with the I2C_LOCK_SEGMENT flag. If the device does not
>> sit behind a mux-locked mux, the two locking variants are equivalent.
>>
>> Signed-off-by: Peter Rosin <[email protected]>
>
> Studied enough so that I can give
>
> Reviewed-by: Jarkko Sakkinen <[email protected]>
>
> Do not have hardware to test this, however.
I don't have a mux-locked I2C mux either, but at least I can confirm
that this change did not break my existing test setup (SLB9635/SLB9645
on Raspberry Pi 2B).
Tested-by: Alexander Steffen <[email protected]>
Alexander
On Tue, 2018-06-26 at 12:07 +0200, Alexander Steffen wrote:
> On 25.06.2018 12:24, Jarkko Sakkinen wrote:
> > On Wed, Jun 20, 2018 at 07:17:54AM +0200, Peter Rosin wrote:
> > > Locking the root adapter for __i2c_transfer will deadlock if the
> > > device sits behind a mux-locked I2C mux. Switch to the finer-grained
> > > i2c_lock_bus with the I2C_LOCK_SEGMENT flag. If the device does not
> > > sit behind a mux-locked mux, the two locking variants are equivalent.
> > >
> > > Signed-off-by: Peter Rosin <[email protected]>
> >
> > Studied enough so that I can give
> >
> > Reviewed-by: Jarkko Sakkinen <[email protected]>
> >
> > Do not have hardware to test this, however.
>
> I don't have a mux-locked I2C mux either, but at least I can confirm
> that this change did not break my existing test setup (SLB9635/SLB9645
> on Raspberry Pi 2B).
>
> Tested-by: Alexander Steffen <[email protected]>
>
> Alexander
Given the scope of the change and since analogous change works for
every other subsystem, this should be enough! Thank you.
/Jarkko
On Tue, 2018-06-26 at 15:05 +0300, Jarkko Sakkinen wrote:
> On Tue, 2018-06-26 at 12:07 +0200, Alexander Steffen wrote:
> > On 25.06.2018 12:24, Jarkko Sakkinen wrote:
> > > On Wed, Jun 20, 2018 at 07:17:54AM +0200, Peter Rosin wrote:
> > > > Locking the root adapter for __i2c_transfer will deadlock if the
> > > > device sits behind a mux-locked I2C mux. Switch to the finer-grained
> > > > i2c_lock_bus with the I2C_LOCK_SEGMENT flag. If the device does not
> > > > sit behind a mux-locked mux, the two locking variants are equivalent.
> > > >
> > > > Signed-off-by: Peter Rosin <[email protected]>
> > >
> > > Studied enough so that I can give
> > >
> > > Reviewed-by: Jarkko Sakkinen <[email protected]>
> > >
> > > Do not have hardware to test this, however.
> >
> > I don't have a mux-locked I2C mux either, but at least I can confirm
> > that this change did not break my existing test setup (SLB9635/SLB9645
> > on Raspberry Pi 2B).
> >
> > Tested-by: Alexander Steffen <[email protected]>
> >
> > Alexander
>
> Given the scope of the change and since analogous change works for
> every other subsystem, this should be enough! Thank you.
It is now applied to my tree (master). I will move it to the
next branch once James has updated security/next-general.
/Jarkko
On Wednesday 20 June 2018 10:48 AM, Peter Rosin wrote:
> The i2c_lock_adapter name is ambiguous since it is unclear if it
> refers to the root adapter or the adapter you name in the argument.
> The natural interpretation is the adapter you name in the argument,
> but there are historical reasons for that not being the case; it
> in fact locks the root adapter. Just remove the function and force
> users to spell out the I2C_LOCK_ROOT_ADAPTER name to indicate what
> is really going on. Also remove i2c_unlock_adapter, of course.
>
> This patch was generated with
>
> git grep -l 'i2c_\(un\)\?lock_adapter' \
> | xargs sed -i 's/i2c_\(un\)\?lock_adapter(\([^)]*\))/'\
> 'i2c_\1lock_bus(\2, I2C_LOCK_ROOT_ADAPTER)/g'
>
> followed by white-space touch-up.
>
> Signed-off-by: Peter Rosin <[email protected]>
> ---
> drivers/i2c/busses/i2c-brcmstb.c | 8 ++++----
> drivers/i2c/busses/i2c-davinci.c | 4 ++--
On DM644x and DA850 EVMs applying this series does not seem to break I2C
functionality. So:
Tested-by: Sekhar Nori <[email protected]>
Thanks,
Sekhar
On Wed, 20 Jun 2018, Peter Rosin wrote:
> Locking the root adapter for __i2c_transfer will deadlock if the
> device sits behind a mux-locked I2C mux. Switch to the finer-grained
> i2c_lock_bus with the I2C_LOCK_SEGMENT flag. If the device does not
> sit behind a mux-locked mux, the two locking variants are equivalent.
>
> Signed-off-by: Peter Rosin <[email protected]>
> ---
> drivers/mfd/88pm860x-i2c.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
If Wolfram is happy with this, then I am.
Since this file sees few changes - please merge through the I2C tree.
Acked-by: Lee Jones <[email protected]>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, Jun 26, 2018 at 11:37:36AM +0900, Wolfram Sang wrote:
> On Wed, Jun 20, 2018 at 07:17:53AM +0200, Peter Rosin wrote:
> > Hi!
> >
> > With the introduction of mux-locked I2C muxes, the concept of
> > locking only a segment of the I2C adapter tree was added. At the
> > time, I did not want to cause a lot of extra churn, so left most
> > users of i2c_lock_adapter alone and apparently didn't think enough
> > about it; they simply continued to lock the whole adapter tree.
> > However, i2c_lock_adapter is in fact wrong for almost every caller
> > (there is naturally an exception) that is itself not a driver for
> > a root adapter. What normal drivers generally want is to only
> > lock the segment of the adapter tree that their device sits on.
> >
> > In fact, if a device sits behind a mux-locked I2C mux, and its
> > driver calls i2c_lock_adapter followed by an unlocked I2C transfer,
> > things will deadlock (since even a mux-locked I2C adapter will lock
> > its parent at some point). If the device is not sitting behind a
> > mux-locked I2C mux (i.e. either directly on the root adapter or
> > behind a (chain of) parent-locked I2C muxes) the root/segment
> > distinction is of no consequence; the root adapter is locked either
> > way.
> >
> > Mux-locked I2C muxes are probably not that common, and putting any
> > of the affected devices behind one is probably even rarer, which
> > is why we have not seen any deadlocks. At least not that I know
> > of...
> >
> > Since silently changing the semantics of i2c_lock_adapter might
> > be quite a surprise, especially for out-of-tree users, this series
> > instead removes the function and forces all users to explicitly
> > name I2C_LOCK_SEGMENT or I2C_LOCK_ROOT_ADAPTER in a call to
> > i2c_lock_bus, as suggested by Wolfram. Yes, users will be a teensy
> > bit more wordy, but open-coding I2C locking from random drivers
> > should be avoided, so it's perhaps a good thing if it doesn't look
> > too neat?
> >
> > I suggest that Wolfram takes this series through the I2C tree and
> > creates an immutable branch for the other subsystems. The series
> > is based on v4.18-r1.
>
> Applied to a seperate branch named "i2c/precise-locking-names" which I
> will merge into for-next, so it will get proper testing already. Once we
> get the missing acks from media, MFD, and IIO maintainers, I will merge
> it into for-4.19.
Ping for media related acks.
Thanks,
Wolfram
Em Thu, 12 Jul 2018 23:28:51 +0200
Wolfram Sang <[email protected]> escreveu:
> On Tue, Jun 26, 2018 at 11:37:36AM +0900, Wolfram Sang wrote:
> > On Wed, Jun 20, 2018 at 07:17:53AM +0200, Peter Rosin wrote:
> > > Hi!
> > >
> > > With the introduction of mux-locked I2C muxes, the concept of
> > > locking only a segment of the I2C adapter tree was added. At the
> > > time, I did not want to cause a lot of extra churn, so left most
> > > users of i2c_lock_adapter alone and apparently didn't think enough
> > > about it; they simply continued to lock the whole adapter tree.
> > > However, i2c_lock_adapter is in fact wrong for almost every caller
> > > (there is naturally an exception) that is itself not a driver for
> > > a root adapter. What normal drivers generally want is to only
> > > lock the segment of the adapter tree that their device sits on.
> > >
> > > In fact, if a device sits behind a mux-locked I2C mux, and its
> > > driver calls i2c_lock_adapter followed by an unlocked I2C transfer,
> > > things will deadlock (since even a mux-locked I2C adapter will lock
> > > its parent at some point). If the device is not sitting behind a
> > > mux-locked I2C mux (i.e. either directly on the root adapter or
> > > behind a (chain of) parent-locked I2C muxes) the root/segment
> > > distinction is of no consequence; the root adapter is locked either
> > > way.
> > >
> > > Mux-locked I2C muxes are probably not that common, and putting any
> > > of the affected devices behind one is probably even rarer, which
> > > is why we have not seen any deadlocks. At least not that I know
> > > of...
> > >
> > > Since silently changing the semantics of i2c_lock_adapter might
> > > be quite a surprise, especially for out-of-tree users, this series
> > > instead removes the function and forces all users to explicitly
> > > name I2C_LOCK_SEGMENT or I2C_LOCK_ROOT_ADAPTER in a call to
> > > i2c_lock_bus, as suggested by Wolfram. Yes, users will be a teensy
> > > bit more wordy, but open-coding I2C locking from random drivers
> > > should be avoided, so it's perhaps a good thing if it doesn't look
> > > too neat?
> > >
> > > I suggest that Wolfram takes this series through the I2C tree and
> > > creates an immutable branch for the other subsystems. The series
> > > is based on v4.18-r1.
> >
> > Applied to a seperate branch named "i2c/precise-locking-names" which I
> > will merge into for-next, so it will get proper testing already. Once we
> > get the missing acks from media, MFD, and IIO maintainers, I will merge
> > it into for-4.19.
>
> Ping for media related acks.
For the media-related ones:
Acked-by: Mauro Carvalho Chehab <[email protected]>
>
> Thanks,
>
> Wolfram
>
Thanks,
Mauro
On Wed, Jun 20, 2018 at 07:17:53AM +0200, Peter Rosin wrote:
> Hi!
>
> With the introduction of mux-locked I2C muxes, the concept of
> locking only a segment of the I2C adapter tree was added. At the
> time, I did not want to cause a lot of extra churn, so left most
> users of i2c_lock_adapter alone and apparently didn't think enough
> about it; they simply continued to lock the whole adapter tree.
> However, i2c_lock_adapter is in fact wrong for almost every caller
> (there is naturally an exception) that is itself not a driver for
> a root adapter. What normal drivers generally want is to only
> lock the segment of the adapter tree that their device sits on.
>
> In fact, if a device sits behind a mux-locked I2C mux, and its
> driver calls i2c_lock_adapter followed by an unlocked I2C transfer,
> things will deadlock (since even a mux-locked I2C adapter will lock
> its parent at some point). If the device is not sitting behind a
> mux-locked I2C mux (i.e. either directly on the root adapter or
> behind a (chain of) parent-locked I2C muxes) the root/segment
> distinction is of no consequence; the root adapter is locked either
> way.
>
> Mux-locked I2C muxes are probably not that common, and putting any
> of the affected devices behind one is probably even rarer, which
> is why we have not seen any deadlocks. At least not that I know
> of...
>
> Since silently changing the semantics of i2c_lock_adapter might
> be quite a surprise, especially for out-of-tree users, this series
> instead removes the function and forces all users to explicitly
> name I2C_LOCK_SEGMENT or I2C_LOCK_ROOT_ADAPTER in a call to
> i2c_lock_bus, as suggested by Wolfram. Yes, users will be a teensy
> bit more wordy, but open-coding I2C locking from random drivers
> should be avoided, so it's perhaps a good thing if it doesn't look
> too neat?
>
> I suggest that Wolfram takes this series through the I2C tree and
> creates an immutable branch for the other subsystems. The series
> is based on v4.18-r1.
>
> I do not have *any* of the affected devices, and have thus only
> done build tests.
>
> Cheers,
> Peter
Applied to for-next, thanks! And thanks for all the acks. An immutable
branch can be found here:
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/precise-locking-names_immutable