Re: [PATCH v2 07/12] iio: st_sensors: Use iio_device_claim/release_direct_mode() when relevant

[Jonathan Cameron <jic23@kernel.org>]

On Wed,  2 Feb 2022 15:02:03 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> The st_sensors_core driver hardcodes the content of the
> iio_device_claim_direct_mode() and iio_device_release_direct_mode()
> helpers. Let's get rid of this handcrafted implementation and use the
> proper core helpers instead. Additionally, this lowers the tab level
> (which is always good) and prevents the use of the ->currentmode
> variable which is not supposed to be used like this anyway.
> 
> Cc: Denis Ciocca <denis.ciocca@st.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../iio/common/st_sensors/st_sensors_core.c   | 32 +++++++++----------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index d0419234a747..e61622e3bc85 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -559,28 +559,26 @@ int st_sensors_read_info_raw(struct iio_dev *indio_dev,
>  	int err;
>  	struct st_sensor_data *sdata = iio_priv(indio_dev);
>  
> -	mutex_lock(&indio_dev->mlock);
> -	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> -		err = -EBUSY;
> +	err = iio_device_claim_direct_mode(indio_dev);
> +	if (err)
> +		return err;
> +
> +	err = st_sensors_set_enable(indio_dev, true);
> +	if (err < 0)
>  		goto out;
> -	} else {
> -		err = st_sensors_set_enable(indio_dev, true);
> -		if (err < 0)
> -			goto out;
>  
> -		mutex_lock(&sdata->odr_lock);
> -		msleep((sdata->sensor_settings->bootime * 1000) / sdata->odr);
> -		err = st_sensors_read_axis_data(indio_dev, ch, val);
> -		mutex_unlock(&sdata->odr_lock);
> -		if (err < 0)
> -			goto out;
> +	mutex_lock(&sdata->odr_lock);

This is problematic I think as the lock taken around getting sdata->odr
in set_sensors_set_enable() but then dropped briefly before being
reacquired here.  If someone sneaks a write in that window, it
looks like we might sleep for the wrong amount of time because sdata->odr
has changed.  I think you need to hold the lock across the whole
enable/read/disable cycle (disable probably not necessary but it
would be more obviously correct if we did hold it across that as well).

Clearly this actually got introduced in the earlier patch but diff
wasn't showing a wide enough bit of code so I missed it.

Note it is fairly common to use iio_device_claim_direct_mode() to
prevent data rate changes whilst doing buffered capture as that tends
to make the data messy and can lead to skipped samples etc.  Doing
that would have the side effect of closing the race.
It is a bit undocumented though in the sense that I don't think
we have ever stated that iio_device_claim_direct_mode() will block
against another iio_device_claim_direct_mode() so accesses are
serialized.  So better to have the local lock enforce the
necessary serialization.  Whilst I doubt we will change the
implementation of iio_device_claim_direct_mode() any time
soon you never know.

Thanks,

Jonathan


> +	msleep((sdata->sensor_settings->bootime * 1000) / sdata->odr);
> +	err = st_sensors_read_axis_data(indio_dev, ch, val);
> +	mutex_unlock(&sdata->odr_lock);
> +	if (err < 0)
> +		goto out;
>  
> -		*val = *val >> ch->scan_type.shift;
> +	*val = *val >> ch->scan_type.shift;
>  
> -		err = st_sensors_set_enable(indio_dev, false);
> -	}
> +	err = st_sensors_set_enable(indio_dev, false);
>  out:
> -	mutex_unlock(&indio_dev->mlock);
> +	iio_device_release_direct_mode(indio_dev);
>  
>  	return err;
>  }