Re: [PATCH v6 20/24] iio: imu: inv_mpu6050: change the i2c gate to be mux-locked

[Daniel Baluta]

On Sun, Apr 3, 2016 at 1:54 PM, Jonathan Cameron  wrote:
> On 03/04/16 09:52, Peter Rosin wrote:
>> From: Peter Rosin 
>>
>> The root i2c adapter lock is then no longer held by the i2c mux during
>> accesses behind the i2c gate, and such accesses need to take that lock
>> just like any other ordinary i2c accesses do.
>>
>> So, declare the i2c gate mux-locked, and zap the code that makes the
>> unlocked i2c accesses and just use ordinary regmap_write accesses.
>>
>> This also happens to fix the deadlock described in
>> http://patchwork.ozlabs.org/patch/584776/ authored by
>> Adriana Reus  and submitted by
>> Daniel Baluta 
>>
>> --8<--
>> iio: imu: inv_mpu6050: Fix deadlock between i2c adapter lock and mpu lock
>>
>> This deadlock occurs if the accel/gyro and the sensor on the auxiliary
>> I2C (in my setup it's an ak8975) are working at the same time.
>>
>> Scenario:
>>
>>   T1  T2
>>  
>> inv_mpu6050_read_fifo  aux sensor op (eg. ak8975_read_raw)
>> | |
>> mutex_lock(_dev->mlock)   i2c_transfer
>> | |
>> i2c transaction i2c adapter lock
>> | |
>> i2c adapter locki2c_mux_master_xfer
>>   |
>> inv_mpu6050_select_bypass
>>   |
>> mutex_lock(_dev->mlock)
>>
>> When we operate on an mpu sensor the order of locking is mpu lock
>> followed by the i2c adapter lock. However, when we operate the auxiliary
>> sensor the order of locking is the other way around.
>>
>> ...
>> --8<--
>>
>> The reason this patch fixes the deadlock is that T2 does not grab the
>> i2c adapter lock until the very end (and grabs the newfangled i2c mux
>> lock where it previously grabbed the i2c adapter lock).
>>
>> Signed-off-by: Peter Rosin 
> This one obviously wants a ack from Adriana or Daniel in addition to mine.
> I'm more than happy for these to go through the i2c tree btw.
>
> Acked-by: Jonathan Cameron 

Acked-by: Daniel Baluta 


>> ---
>>  Documentation/i2c/i2c-topology|  2 +-
>>  drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 56 
>> +++
>>  2 files changed, 13 insertions(+), 45 deletions(-)
>>
>> diff --git a/Documentation/i2c/i2c-topology b/Documentation/i2c/i2c-topology
>> index 7a10edd0874f..346623a80bd1 100644
>> --- a/Documentation/i2c/i2c-topology
>> +++ b/Documentation/i2c/i2c-topology
>> @@ -50,7 +50,7 @@ i2c-mux-pinctrl   Normally parent-locked, 
>> mux-locked iff
>>  i2c-mux-reg   Parent-locked
>>
>>  In drivers/iio/
>> -imu/inv_mpu6050/  Parent-locked
>> +imu/inv_mpu6050/  Mux-locked
>>
>>  In drivers/media/
>>  dvb-frontends/m88ds3103   Parent-locked
>> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c 
>> b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>> index 0d429d788106..71ad31a275c9 100644
>> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
>> @@ -24,45 +24,16 @@ static const struct regmap_config inv_mpu_regmap_config 
>> = {
>>   .val_bits = 8,
>>  };
>>
>> -/*
>> - * The i2c read/write needs to happen in unlocked mode. As the parent
>> - * adapter is common. If we use locked versions, it will fail as
>> - * the mux adapter will lock the parent i2c adapter, while calling
>> - * select/deselect functions.
>> - */
>> -static int inv_mpu6050_write_reg_unlocked(struct i2c_client *client,
>> -   u8 reg, u8 d)
>> -{
>> - int ret;
>> - u8 buf[2] = {reg, d};
>> - struct i2c_msg msg[1] = {
>> - {
>> - .addr = client->addr,
>> - .flags = 0,
>> - .len = sizeof(buf),
>> - .buf = buf,
>> - }
>> - };
>> -
>> - ret = __i2c_transfer(client->adapter, msg, 1);
>> - if (ret != 1)
>> - return ret;
>> -
>> - return 0;
>> -}
>> -
>>  static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
>>  {
>> - struct i2c_client *client = i2c_mux_priv(muxc);
>> - struct iio_dev *indio_dev = dev_get_drvdata(>dev);
>> + struct iio_dev *indio_dev = i2c_mux_priv(muxc);
>>   struct inv_mpu6050_state *st = iio_priv(indio_dev);
>>   int ret = 0;
>>
>>   /* Use the same mutex which was used everywhere to protect power-op */
>>   mutex_lock(_dev->mlock);
>>   if (!st->powerup_count) {
>> - ret = inv_mpu6050_write_reg_unlocked(client,
>> -  

Re: [PATCH v6 20/24] iio: imu: inv_mpu6050: change the i2c gate to be mux-locked

[Jonathan Cameron]

On 03/04/16 09:52, Peter Rosin wrote:
> From: Peter Rosin 
> 
> The root i2c adapter lock is then no longer held by the i2c mux during
> accesses behind the i2c gate, and such accesses need to take that lock
> just like any other ordinary i2c accesses do.
> 
> So, declare the i2c gate mux-locked, and zap the code that makes the
> unlocked i2c accesses and just use ordinary regmap_write accesses.
> 
> This also happens to fix the deadlock described in
> http://patchwork.ozlabs.org/patch/584776/ authored by
> Adriana Reus  and submitted by
> Daniel Baluta 
> 
> --8<--
> iio: imu: inv_mpu6050: Fix deadlock between i2c adapter lock and mpu lock
> 
> This deadlock occurs if the accel/gyro and the sensor on the auxiliary
> I2C (in my setup it's an ak8975) are working at the same time.
> 
> Scenario:
> 
>   T1  T2
>  
> inv_mpu6050_read_fifo  aux sensor op (eg. ak8975_read_raw)
> | |
> mutex_lock(_dev->mlock)   i2c_transfer
> | |
> i2c transaction i2c adapter lock
> | |
> i2c adapter locki2c_mux_master_xfer
>   |
> inv_mpu6050_select_bypass
>   |
> mutex_lock(_dev->mlock)
> 
> When we operate on an mpu sensor the order of locking is mpu lock
> followed by the i2c adapter lock. However, when we operate the auxiliary
> sensor the order of locking is the other way around.
> 
> ...
> --8<--
> 
> The reason this patch fixes the deadlock is that T2 does not grab the
> i2c adapter lock until the very end (and grabs the newfangled i2c mux
> lock where it previously grabbed the i2c adapter lock).
> 
> Signed-off-by: Peter Rosin 
This one obviously wants a ack from Adriana or Daniel in addition to mine.
I'm more than happy for these to go through the i2c tree btw.

Acked-by: Jonathan Cameron 
> ---
>  Documentation/i2c/i2c-topology|  2 +-
>  drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 56 
> +++
>  2 files changed, 13 insertions(+), 45 deletions(-)
> 
> diff --git a/Documentation/i2c/i2c-topology b/Documentation/i2c/i2c-topology
> index 7a10edd0874f..346623a80bd1 100644
> --- a/Documentation/i2c/i2c-topology
> +++ b/Documentation/i2c/i2c-topology
> @@ -50,7 +50,7 @@ i2c-mux-pinctrl   Normally parent-locked, 
> mux-locked iff
>  i2c-mux-reg   Parent-locked
>  
>  In drivers/iio/
> -imu/inv_mpu6050/  Parent-locked
> +imu/inv_mpu6050/  Mux-locked
>  
>  In drivers/media/
>  dvb-frontends/m88ds3103   Parent-locked
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c 
> b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> index 0d429d788106..71ad31a275c9 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> @@ -24,45 +24,16 @@ static const struct regmap_config inv_mpu_regmap_config = 
> {
>   .val_bits = 8,
>  };
>  
> -/*
> - * The i2c read/write needs to happen in unlocked mode. As the parent
> - * adapter is common. If we use locked versions, it will fail as
> - * the mux adapter will lock the parent i2c adapter, while calling
> - * select/deselect functions.
> - */
> -static int inv_mpu6050_write_reg_unlocked(struct i2c_client *client,
> -   u8 reg, u8 d)
> -{
> - int ret;
> - u8 buf[2] = {reg, d};
> - struct i2c_msg msg[1] = {
> - {
> - .addr = client->addr,
> - .flags = 0,
> - .len = sizeof(buf),
> - .buf = buf,
> - }
> - };
> -
> - ret = __i2c_transfer(client->adapter, msg, 1);
> - if (ret != 1)
> - return ret;
> -
> - return 0;
> -}
> -
>  static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
>  {
> - struct i2c_client *client = i2c_mux_priv(muxc);
> - struct iio_dev *indio_dev = dev_get_drvdata(>dev);
> + struct iio_dev *indio_dev = i2c_mux_priv(muxc);
>   struct inv_mpu6050_state *st = iio_priv(indio_dev);
>   int ret = 0;
>  
>   /* Use the same mutex which was used everywhere to protect power-op */
>   mutex_lock(_dev->mlock);
>   if (!st->powerup_count) {
> - ret = inv_mpu6050_write_reg_unlocked(client,
> -  st->reg->pwr_mgmt_1, 0);
> + ret = regmap_write(st->map, st->reg->pwr_mgmt_1, 0);
>   if (ret)
>   goto write_error;
>  
> @@ -71,10 +42,9 @@ static int 

[PATCH v6 20/24] iio: imu: inv_mpu6050: change the i2c gate to be mux-locked

[Peter Rosin]

From: Peter Rosin 

The root i2c adapter lock is then no longer held by the i2c mux during
accesses behind the i2c gate, and such accesses need to take that lock
just like any other ordinary i2c accesses do.

So, declare the i2c gate mux-locked, and zap the code that makes the
unlocked i2c accesses and just use ordinary regmap_write accesses.

This also happens to fix the deadlock described in
http://patchwork.ozlabs.org/patch/584776/ authored by
Adriana Reus  and submitted by
Daniel Baluta 

--8<--
iio: imu: inv_mpu6050: Fix deadlock between i2c adapter lock and mpu lock

This deadlock occurs if the accel/gyro and the sensor on the auxiliary
I2C (in my setup it's an ak8975) are working at the same time.

Scenario:

  T1T2
   
inv_mpu6050_read_fifo  aux sensor op (eg. ak8975_read_raw)
| |
mutex_lock(_dev->mlock)   i2c_transfer
| |
i2c transaction i2c adapter lock
| |
i2c adapter locki2c_mux_master_xfer
  |
inv_mpu6050_select_bypass
  |
mutex_lock(_dev->mlock)

When we operate on an mpu sensor the order of locking is mpu lock
followed by the i2c adapter lock. However, when we operate the auxiliary
sensor the order of locking is the other way around.

...
--8<--

The reason this patch fixes the deadlock is that T2 does not grab the
i2c adapter lock until the very end (and grabs the newfangled i2c mux
lock where it previously grabbed the i2c adapter lock).

Signed-off-by: Peter Rosin 
---
 Documentation/i2c/i2c-topology|  2 +-
 drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 56 +++
 2 files changed, 13 insertions(+), 45 deletions(-)

diff --git a/Documentation/i2c/i2c-topology b/Documentation/i2c/i2c-topology
index 7a10edd0874f..346623a80bd1 100644
--- a/Documentation/i2c/i2c-topology
+++ b/Documentation/i2c/i2c-topology
@@ -50,7 +50,7 @@ i2c-mux-pinctrl   Normally parent-locked, mux-locked 
iff
 i2c-mux-reg   Parent-locked
 
 In drivers/iio/
-imu/inv_mpu6050/  Parent-locked
+imu/inv_mpu6050/  Mux-locked
 
 In drivers/media/
 dvb-frontends/m88ds3103   Parent-locked
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c 
b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
index 0d429d788106..71ad31a275c9 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
@@ -24,45 +24,16 @@ static const struct regmap_config inv_mpu_regmap_config = {
.val_bits = 8,
 };
 
-/*
- * The i2c read/write needs to happen in unlocked mode. As the parent
- * adapter is common. If we use locked versions, it will fail as
- * the mux adapter will lock the parent i2c adapter, while calling
- * select/deselect functions.
- */
-static int inv_mpu6050_write_reg_unlocked(struct i2c_client *client,
- u8 reg, u8 d)
-{
-   int ret;
-   u8 buf[2] = {reg, d};
-   struct i2c_msg msg[1] = {
-   {
-   .addr = client->addr,
-   .flags = 0,
-   .len = sizeof(buf),
-   .buf = buf,
-   }
-   };
-
-   ret = __i2c_transfer(client->adapter, msg, 1);
-   if (ret != 1)
-   return ret;
-
-   return 0;
-}
-
 static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id)
 {
-   struct i2c_client *client = i2c_mux_priv(muxc);
-   struct iio_dev *indio_dev = dev_get_drvdata(>dev);
+   struct iio_dev *indio_dev = i2c_mux_priv(muxc);
struct inv_mpu6050_state *st = iio_priv(indio_dev);
int ret = 0;
 
/* Use the same mutex which was used everywhere to protect power-op */
mutex_lock(_dev->mlock);
if (!st->powerup_count) {
-   ret = inv_mpu6050_write_reg_unlocked(client,
-st->reg->pwr_mgmt_1, 0);
+   ret = regmap_write(st->map, st->reg->pwr_mgmt_1, 0);
if (ret)
goto write_error;
 
@@ -71,10 +42,9 @@ static int inv_mpu6050_select_bypass(struct i2c_mux_core 
*muxc, u32 chan_id)
}
if (!ret) {
st->powerup_count++;
-   ret = inv_mpu6050_write_reg_unlocked(client,
-st->reg->int_pin_cfg,
-INV_MPU6050_INT_PIN_CFG |
-