Detected by kernel circular locking dependency checker.
We are locking iio mutex (iio_device_claim_direct_mode) after locking our internal mutex. But when the buffer starts, iio first locks its mutex and then we lock our internal one.
To avoid possible deadlock, we need to use the same order everwhere. So we change the ordering by locking first iio mutex, then our internal mutex.
Fixes: 68cd6e5b206b ("iio: imu: inv_mpu6050: fix lock issues by using our own mutex") Cc: stable@vger.kernel.org Signed-off-by: Jean-Baptiste Maneyrol jmaneyrol@invensense.com --- drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 34 ++++++++++++++---------------- 1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c index aafa777..7358935 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c @@ -340,12 +340,9 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev, int result; int ret;
- result = iio_device_claim_direct_mode(indio_dev); - if (result) - return result; result = inv_mpu6050_set_power_itg(st, true); if (result) - goto error_release; + return result;
switch (chan->type) { case IIO_ANGL_VEL: @@ -386,14 +383,11 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev, result = inv_mpu6050_set_power_itg(st, false); if (result) goto error_power_off; - iio_device_release_direct_mode(indio_dev);
return ret;
error_power_off: inv_mpu6050_set_power_itg(st, false); -error_release: - iio_device_release_direct_mode(indio_dev); return result; }
@@ -407,9 +401,13 @@ inv_mpu6050_read_raw(struct iio_dev *indio_dev,
switch (mask) { case IIO_CHAN_INFO_RAW: + ret = iio_device_claim_direct_mode(indio_dev); + if (ret) + return ret; mutex_lock(&st->lock); ret = inv_mpu6050_read_channel_data(indio_dev, chan, val); mutex_unlock(&st->lock); + iio_device_release_direct_mode(indio_dev); return ret; case IIO_CHAN_INFO_SCALE: switch (chan->type) { @@ -532,17 +530,18 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev, struct inv_mpu6050_state *st = iio_priv(indio_dev); int result;
- mutex_lock(&st->lock); /* * we should only update scale when the chip is disabled, i.e. * not running */ result = iio_device_claim_direct_mode(indio_dev); if (result) - goto error_write_raw_unlock; + return result; + + mutex_lock(&st->lock); result = inv_mpu6050_set_power_itg(st, true); if (result) - goto error_write_raw_release; + goto error_write_raw_unlock;
switch (mask) { case IIO_CHAN_INFO_SCALE: @@ -581,10 +580,9 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev, }
result |= inv_mpu6050_set_power_itg(st, false); -error_write_raw_release: - iio_device_release_direct_mode(indio_dev); error_write_raw_unlock: mutex_unlock(&st->lock); + iio_device_release_direct_mode(indio_dev);
return result; } @@ -643,17 +641,18 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr, fifo_rate > INV_MPU6050_MAX_FIFO_RATE) return -EINVAL;
+ result = iio_device_claim_direct_mode(indio_dev); + if (result) + return result; + mutex_lock(&st->lock); if (fifo_rate == st->chip_config.fifo_rate) { result = 0; goto fifo_rate_fail_unlock; } - result = iio_device_claim_direct_mode(indio_dev); - if (result) - goto fifo_rate_fail_unlock; result = inv_mpu6050_set_power_itg(st, true); if (result) - goto fifo_rate_fail_release; + goto fifo_rate_fail_unlock;
d = INV_MPU6050_ONE_K_HZ / fifo_rate - 1; result = regmap_write(st->map, st->reg->sample_rate_div, d); @@ -667,10 +666,9 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr,
fifo_rate_fail_power_off: result |= inv_mpu6050_set_power_itg(st, false); -fifo_rate_fail_release: - iio_device_release_direct_mode(indio_dev); fifo_rate_fail_unlock: mutex_unlock(&st->lock); + iio_device_release_direct_mode(indio_dev); if (result) return result;
On Mon, 30 Apr 2018 12:14:09 +0200 Jean-Baptiste Maneyrol jmaneyrol@invensense.com wrote:
Detected by kernel circular locking dependency checker.
We are locking iio mutex (iio_device_claim_direct_mode) after locking our internal mutex. But when the buffer starts, iio first locks its mutex and then we lock our internal one.
To avoid possible deadlock, we need to use the same order everwhere. So we change the ordering by locking first iio mutex, then our internal mutex.
Fixes: 68cd6e5b206b ("iio: imu: inv_mpu6050: fix lock issues by using our own mutex") Cc: stable@vger.kernel.org Signed-off-by: Jean-Baptiste Maneyrol jmaneyrol@invensense.com
Hmm. There has been too much going on with this driver in this cycle for this to cleanly apply to the fixes-togreg branch. So it'll just have to wait for the next merge window and then we can look at back porting.
Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it.
Good find.
Thanks,
Jonathan
drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 34 ++++++++++++++---------------- 1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c index aafa777..7358935 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c @@ -340,12 +340,9 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev, int result; int ret;
- result = iio_device_claim_direct_mode(indio_dev);
- if (result)
return result;goto error_release;
return result;switch (chan->type) { case IIO_ANGL_VEL: @@ -386,14 +383,11 @@ static int inv_mpu6050_read_channel_data(struct iio_dev *indio_dev, result = inv_mpu6050_set_power_itg(st, false); if (result) goto error_power_off;
- iio_device_release_direct_mode(indio_dev);
return ret; error_power_off: inv_mpu6050_set_power_itg(st, false); -error_release:
- iio_device_release_direct_mode(indio_dev); return result;
} @@ -407,9 +401,13 @@ inv_mpu6050_read_raw(struct iio_dev *indio_dev, switch (mask) { case IIO_CHAN_INFO_RAW:
ret = iio_device_claim_direct_mode(indio_dev);if (ret)return ret;iio_device_release_direct_mode(indio_dev);@@ -532,17 +530,18 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev, struct inv_mpu6050_state *st = iio_priv(indio_dev); int result;
- mutex_lock(&st->lock); /*
*/ result = iio_device_claim_direct_mode(indio_dev); if (result)
- we should only update scale when the chip is disabled, i.e.
- not running
goto error_write_raw_unlock;
return result;- mutex_lock(&st->lock); result = inv_mpu6050_set_power_itg(st, true); if (result)
goto error_write_raw_release;
goto error_write_raw_unlock;switch (mask) { case IIO_CHAN_INFO_SCALE: @@ -581,10 +580,9 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev, } result |= inv_mpu6050_set_power_itg(st, false); -error_write_raw_release:
- iio_device_release_direct_mode(indio_dev);
error_write_raw_unlock: mutex_unlock(&st->lock);
- iio_device_release_direct_mode(indio_dev);
return result; } @@ -643,17 +641,18 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr, fifo_rate > INV_MPU6050_MAX_FIFO_RATE) return -EINVAL;
- result = iio_device_claim_direct_mode(indio_dev);
- if (result)
return result;- mutex_lock(&st->lock); if (fifo_rate == st->chip_config.fifo_rate) { result = 0; goto fifo_rate_fail_unlock; }
- result = iio_device_claim_direct_mode(indio_dev);
- if (result)
goto fifo_rate_fail_unlock;goto fifo_rate_fail_release;
goto fifo_rate_fail_unlock;d = INV_MPU6050_ONE_K_HZ / fifo_rate - 1; result = regmap_write(st->map, st->reg->sample_rate_div, d); @@ -667,10 +666,9 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr, fifo_rate_fail_power_off: result |= inv_mpu6050_set_power_itg(st, false); -fifo_rate_fail_release:
- iio_device_release_direct_mode(indio_dev);
fifo_rate_fail_unlock: mutex_unlock(&st->lock);
- iio_device_release_direct_mode(indio_dev); if (result) return result;
linux-stable-mirror@lists.linaro.org
-
Jean-Baptiste Maneyrol -
Jonathan Cameron