| Message ID | 20220202140208.391394-8-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Miscellaneous IIO core enhancements | expand |
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; > }
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
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); + 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; }
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(-)