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) > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
