[PATCH v2 00/10] Split i2c_lock_adapter into i2c_lock_root and i2c_lock_segment

[PATCH v2 00/10] Split i2c_lock_adapter into i2c_lock_root and i2c_lock_segment

[Peter Rosin]

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

[PATCH v2 01/10] tpm/tpm_i2c_infineon: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)

[Peter Rosin]

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 <peda@axentia.se>
---
 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

Re: [PATCH v2 01/10] tpm/tpm_i2c_infineon: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)

[Jarkko Sakkinen]

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 <peda@axentia.se>

Studied enough so that I can give

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Do not have hardware to test this, however.

/Jarkko

Re: [PATCH v2 01/10] tpm/tpm_i2c_infineon: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)

[Alexander Steffen]

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 <peda@axentia.se>
> 
> Studied enough so that I can give
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> 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 <Alexander.Steffen@infineon.com>

Alexander

Re: [PATCH v2 01/10] tpm/tpm_i2c_infineon: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)

[Jarkko Sakkinen]

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 <peda@axentia.se>
> > 
> > Studied enough so that I can give
> > 
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 
> > 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 <Alexander.Steffen@infineon.com>
> 
> Alexander

Given the scope of the change and since analogous change works for
every other subsystem, this should be enough! Thank you.

/Jarkko

Re: [PATCH v2 01/10] tpm/tpm_i2c_infineon: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)

[Jarkko Sakkinen]

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 <peda@axentia.se>
> > > 
> > > Studied enough so that I can give
> > > 
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > 
> > > 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 <Alexander.Steffen@infineon.com>
> > 
> > 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

[PATCH v2 02/10] i2c: mux: pca9541: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)

[Peter Rosin]

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 <peda@axentia.se>
---
 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

[PATCH v2 03/10] input: rohm_bu21023: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)

[Peter Rosin]

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 <peda@axentia.se>
---
 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

Re: [PATCH v2 03/10] input: rohm_bu21023: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)

[Dmitry Torokhov]

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 <peda@axentia.se>

Still

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

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

[PATCH v2 04/10] media: af9013: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)

[Peter Rosin]

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 <peda@axentia.se>
---
 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

[PATCH v2 05/10] media: drxk_hard: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)

[Peter Rosin]

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 <peda@axentia.se>
---
 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

[PATCH v2 06/10] media: rtl2830: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)

[Peter Rosin]

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 <peda@axentia.se>
---
 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

[PATCH v2 07/10] media: tda1004x: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)

[Peter Rosin]

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 <peda@axentia.se>
---
 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

[PATCH v2 08/10] media: tda18271: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)

[Peter Rosin]

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 <peda@axentia.se>
---
 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

[PATCH v2 09/10] mfd: 88pm860x-i2c: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)

[Peter Rosin]

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 <peda@axentia.se>
---
 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

Re: [PATCH v2 09/10] mfd: 88pm860x-i2c: switch to i2c_lock_bus(..., I2C_LOCK_SEGMENT)

[Lee Jones]

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 <peda@axentia.se>
> ---
>  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 <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 10/10] i2c: remove i2c_lock_adapter and use i2c_lock_bus directly

[Peter Rosin]

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 <peda@axentia.se>
---
 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

Re: [PATCH v2 10/10] i2c: remove i2c_lock_adapter and use i2c_lock_bus directly

[Jonathan Cameron]

On Wed, 20 Jun 2018 07:18:03 +0200
Peter Rosin <peda@axentia.se> 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 <peda@axentia.se>

For IIO: Acked-by: Jonathan Cameron <jonathan.cameron@huawei.com>

> ---
>  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 */

Re: [PATCH v2 10/10] i2c: remove i2c_lock_adapter and use i2c_lock_bus directly

[Sekhar Nori]

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 <peda@axentia.se>
> ---
>  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 <nsekhar@ti.com>

Thanks,
Sekhar

Re: [PATCH v2 00/10] Split i2c_lock_adapter into i2c_lock_root and i2c_lock_segment

[Wolfram Sang]

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!

Re: [PATCH v2 00/10] Split i2c_lock_adapter into i2c_lock_root and i2c_lock_segment

[Wolfram Sang]

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

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 00/10] Split i2c_lock_adapter into i2c_lock_root and i2c_lock_segment

[Mauro Carvalho Chehab]

Em Thu, 12 Jul 2018 23:28:51 +0200
Wolfram Sang <wsa@the-dreams.de> 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 <mchehab+samsung@kernel.org>

> 
> Thanks,
> 
>    Wolfram
> 



Thanks,
Mauro

Re: [PATCH v2 00/10] Split i2c_lock_adapter into i2c_lock_root and i2c_lock_segment

[Wolfram Sang]

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

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]