On Mon, 25 Jan 2021 09:53:44 +0800
Guoqing Chi <chi962464zy@xxxxxxx> 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>
> Reviewed-by: Tom Rix <trix@xxxxxxxxxx>
Hi. Looking at this again, I'm not entirely sure I understand what the
race is. Could you give any example?
Individual regmap accesses have their own internal protections
against races. We don't have an read modify write cycles that
I can see here, so I don't think there are any races.
On another note however, there is a regmap_bulk_read into
a variable on the stack which is a problem for spi. Unlike i2c
the spi patch in regmap for bulk_reads can be zero copy which
means that the variable on the stack can be dma'd into. That's
a potential issue for some systems in which you can end up wiping
out whatever else gets changed in the same cacheline.
To fix that, the variable should be in it's own cacheline. Either
do that by using kmalloc etc to put it on the stack, or add a
suitable buffer to priv, marked ___cacheline_aligned. Though once
you do that you will need locks to protect it as you've done
here.
Jonathan
> ---
> 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;
> + 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;
>
