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

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

[Tom Rix]

On 1/18/21 2:05 AM, chiguoqing wrote:
> Adding mutex_lock, when read and write reg need to use this lock to
> avoid race.
>
> Signed-off-by: Guoqing Chi <chiguoqing@yulong.com>
> ---
>  drivers/iio/imu/bmi160/bmi160.h      |  2 ++
>  drivers/iio/imu/bmi160/bmi160_core.c | 17 +++++++++++++----
>  2 files changed, 15 insertions(+), 4 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;
where is the mutex_init() ?
>  	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..b626e067c612 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -454,18 +454,24 @@ static int bmi160_read_raw(struct iio_dev *indio_dev,

how the read and write functions lock and return are not consistent.

to me, the write function looks better, so change the read function to look more like the write function

Tom

>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&data->lock);
>  		ret = bmi160_get_data(data, chan->type, chan->channel2, val);
> +		mutex_unlock(&data->lock);
>  		if (ret)
>  			return ret;
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
>  		*val = 0;
> +		mutex_lock(&data->lock);
>  		ret = bmi160_get_scale(data,
>  				       bmi160_to_sensor(chan->type), val2);
> +		mutex_unlock(&data->lock);
>  		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> +		mutex_lock(&data->lock);
>  		ret = bmi160_get_odr(data, bmi160_to_sensor(chan->type),
>  				     val, val2);
> +		mutex_unlock(&data->lock);
>  		return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
>  	default:
>  		return -EINVAL;
> @@ -479,19 +485,22 @@ 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);
>  	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);
>  	default:
> -		return -EINVAL;
> +		result = -EINVAL;
>  	}
> +	mutex_unlock(&data->lock);
>  
> -	return 0;
> +	return result;
>  }
>  
>  static