Re: [PATCH v2] iio: imu: bmi160: add mutex_lock for avoiding race

[Tom Rix]

On 1/19/21 3:22 AM, Guoqing Chi wrote:
> From: chiguoqing <chi962464zy@xxxxxxx>
>
> Adding mutex_lock, when read and write reg need to use this lock to
> avoid race.
>
> Signed-off-by: Guoqing Chi <chiguoqing@xxxxxxxxxx>
> ---
> v2:Follow write function to fix read function.
> Adding mutex init in core probe function.
> Adding break in switch case at read and write function.
>
>  drivers/iio/imu/bmi160/bmi160.h      |  2 ++
>  drivers/iio/imu/bmi160/bmi160_core.c | 34 +++++++++++++++++++---------
>  2 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
> index 32c2ea2d7112..0c189a8b5b53 100644
> --- a/drivers/iio/imu/bmi160/bmi160.h
> +++ b/drivers/iio/imu/bmi160/bmi160.h
> @@ -3,9 +3,11 @@
>  #define BMI160_H_
>  
>  #include <linux/iio/iio.h>
> +#include <linux/mutex.h>
>  #include <linux/regulator/consumer.h>
>  
>  struct bmi160_data {
> +	struct mutex lock;
>  	struct regmap *regmap;
>  	struct iio_trigger *trig;
>  	struct regulator_bulk_data supplies[2];
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index 290b5ef83f77..e303378f4841 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -452,26 +452,32 @@ static int bmi160_read_raw(struct iio_dev *indio_dev,
>  	int ret;
>  	struct bmi160_data *data = iio_priv(indio_dev);
>  
> +	mutex_lock(&data->lock);
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  		ret = bmi160_get_data(data, chan->type, chan->channel2, val);
> -		if (ret)
> -			return ret;
> -		return IIO_VAL_INT;
> +		if (!ret)
> +			ret = IIO_VAL_INT;
> +		break;
>  	case IIO_CHAN_INFO_SCALE:
>  		*val = 0;
>  		ret = bmi160_get_scale(data,
>  				       bmi160_to_sensor(chan->type), val2);
> -		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> +		if (!ret)
> +			ret = IIO_VAL_INT_PLUS_MICRO;

Looking better, another question..

Why does the write() function return the results directly while the read() function

translates them to other values ?

Tom

> +		break;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		ret = bmi160_get_odr(data, bmi160_to_sensor(chan->type),
>  				     val, val2);
> -		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> +		if (!ret)
> +			ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
>  	default:
> -		return -EINVAL;
> +		ret = -EINVAL;
>  	}
> +	mutex_unlock(&data->lock);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int bmi160_write_raw(struct iio_dev *indio_dev,
> @@ -479,19 +485,24 @@ static int bmi160_write_raw(struct iio_dev *indio_dev,
>  			    int val, int val2, long mask)
>  {
>  	struct bmi160_data *data = iio_priv(indio_dev);
> +	int result;
>  
> +	mutex_lock(&data->lock);
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SCALE:
> -		return bmi160_set_scale(data,
> +		result = bmi160_set_scale(data,
>  					bmi160_to_sensor(chan->type), val2);
> +		break;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		return bmi160_set_odr(data, bmi160_to_sensor(chan->type),
> +		result = bmi160_set_odr(data, bmi160_to_sensor(chan->type),
>  				      val, val2);
> +		break;
>  	default:
> -		return -EINVAL;
> +		result = -EINVAL;
>  	}
> +	mutex_unlock(&data->lock);
>  
> -	return 0;
> +	return result;
>  }
>  
>  static
> @@ -838,6 +849,7 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
>  		return -ENOMEM;
>  
>  	data = iio_priv(indio_dev);
> +	mutex_init(&data->lock);
>  	dev_set_drvdata(dev, indio_dev);
>  	data->regmap = regmap;
>