Re: [PATCH 2/4] iio: imu: inv_mpu6050: fix possible deadlock between mutex and iio

[Jonathan Cameron <jic23@jic23.retrosnub.co.uk>]

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;
>  	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;
>