Re: [PATCH v4 6/6] iio: imu: st_lsm6dsx: prohibit the use of events and buffered reads simultaneously

[Jonathan Cameron <jic23@kernel.org>]

On Fri,  6 Sep 2019 14:17:16 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> When events and buffered reads is enabled simultaneously, and the first
> event accours the interrupt pin stays high.
> 
> This can be reverted when we find a solution to allow events and
> buffered reads simultaneously.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 3 +++
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c   | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> index ef579650fd52..94e8884a1db1 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> @@ -601,6 +601,9 @@ int st_lsm6dsx_update_fifo(struct st_lsm6dsx_sensor *sensor, bool enable)
>  	struct st_lsm6dsx_hw *hw = sensor->hw;
>  	int err;
>  
> +	if (hw->enable_event)
> +		return -EBUSY;
> +

This strikes me as racey.  What stops you getting past the check and before
you get to the next line of code, an event is enabled? 

More than likely I'm missing a reason that can't happen, but a comment
here to explain why will make it immediately obvious!

>  	mutex_lock(&hw->conf_lock);
>  
>  	if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) {
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 470821b54933..fdc44ff9601b 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -1277,6 +1277,9 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
>  	if (type != IIO_EV_TYPE_THRESH)
>  		return -EINVAL;
>  
> +	if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS)
> +		return -EBUSY;

Again, looks superficially racey.

> +
>  	if (state && hw->enable_event)
>  		return 0;
>