Re: [PATCH v6 20/24] iio: imu: inv_mpu6050: change the i2c gate to be mux-locked
From: Jonathan Cameron
Date: Sun Apr 03 2016 - 06:54:16 EST
On 03/04/16 09:52, Peter Rosin wrote:
> From: Peter Rosin <peda@xxxxxxxxxx>
>
> 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 <adriana.reus@xxxxxxxxx> and submitted by
> Daniel Baluta <daniel.baluta@xxxxxxxxx>
>
> ----------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(&indio_dev->mlock) i2c_transfer
> | |
> i2c transaction i2c adapter lock
> | |
> i2c adapter lock i2c_mux_master_xfer
> |
> inv_mpu6050_select_bypass
> |
> mutex_lock(&indio_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 <peda@xxxxxxxxxx>
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 <jic23@xxxxxxxxxx>
> ---
> 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(&client->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(&indio_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 |
> - INV_MPU6050_BIT_BYPASS_EN);
> + ret = regmap_write(st->map, st->reg->int_pin_cfg,
> + INV_MPU6050_INT_PIN_CFG |
> + INV_MPU6050_BIT_BYPASS_EN);
> }
> write_error:
> mutex_unlock(&indio_dev->mlock);
> @@ -84,18 +54,16 @@ write_error:
>
> static int inv_mpu6050_deselect_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(&client->dev);
> + struct iio_dev *indio_dev = i2c_mux_priv(muxc);
> struct inv_mpu6050_state *st = iio_priv(indio_dev);
>
> mutex_lock(&indio_dev->mlock);
> /* It doesn't really mattter, if any of the calls fails */
> - inv_mpu6050_write_reg_unlocked(client, st->reg->int_pin_cfg,
> - INV_MPU6050_INT_PIN_CFG);
> + regmap_write(st->map, st->reg->int_pin_cfg, INV_MPU6050_INT_PIN_CFG);
> st->powerup_count--;
> if (!st->powerup_count)
> - inv_mpu6050_write_reg_unlocked(client, st->reg->pwr_mgmt_1,
> - INV_MPU6050_BIT_SLEEP);
> + regmap_write(st->map, st->reg->pwr_mgmt_1,
> + INV_MPU6050_BIT_SLEEP);
> mutex_unlock(&indio_dev->mlock);
>
> return 0;
> @@ -133,15 +101,15 @@ static int inv_mpu_probe(struct i2c_client *client,
> return result;
>
> st = iio_priv(dev_get_drvdata(&client->dev));
> - st->muxc = i2c_mux_one_adapter(client->adapter, &client->dev, 0, 0,
> - 0, 0, 0,
> + st->muxc = i2c_mux_one_adapter(client->adapter, &client->dev, 0,
> + I2C_MUX_LOCKED, 0, 0, 0,
> inv_mpu6050_select_bypass,
> inv_mpu6050_deselect_bypass);
> if (IS_ERR(st->muxc)) {
> result = PTR_ERR(st->muxc);
> goto out_unreg_device;
> }
> - st->muxc->priv = client;
> + st->muxc->priv = dev_get_drvdata(&client->dev);
>
> result = inv_mpu_acpi_create_mux_client(client);
> if (result)
>