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

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Jonathan,

jic23@kernel.org wrote on Sun, 6 Feb 2022 15:55:20 +0000:

> 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).
> 

Yeah, I do agree this is a risk and I actually bet that the mlock mutex
taken with iio_device_claim_direct_mode() was more than enough, but
you're right that this this is maybe just a consequence of the
current implementation and not actually enforced by the API, and for
this reason we might want to enforce the serialization with our own
lock.

I've extended the coverage of the lock to fix that.

> 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;
> >  }  
> 


Thanks,
Miquèl