[PATCH v2 00/12] Miscellaneous IIO core enhancements

[PATCH v2 00/12] Miscellaneous IIO core enhancements

[Miquel Raynal]

Hello,

As part of a previous discussion with Jonathan (I know, it was three
months ago, sorry for the delay :-$), it appeared necessary to do a
little bit of cleanup in the IIO core and at least provide a little bit
more documentation for developers diving into the IIO subsystem for the
first time. My first move was to write a blog post trying to explain
(from my point of view but with the review of Jonathan) what is
necessary to understand when writing and IIO kernel driver:
https://bootlin.com/blog/the-backbone-of-a-linux-industrial-i-o-driver/

Here is now the second halve of this work, with few things that I tried
to address.

* The currentmode indio_dev entry was misused by several device drivers,
  I tried to understand which ones were wrong (there are two still
  accessing this variable left, I decided not to touch them because I
  think it is legitimate) and I tried to find an alternative.

* Then, I tried to make new accesses to this variable harder in order to
  avoid future mistakes, by creating a helper to read it from outside the
  core and moving it to the opaque structure.

* I continued with the IIO modes which are completely
  undocumented. Following a few exchanges with Jonathan, I tried to take
  most of this discussion and put it into a kernel doc header.

* When doing the above task, I realized that there was a particular case
  handled for kfifo buffers which I tried to simplify, but if this is a
  mistake, I'll just drop the patch.

Hope this will help!

Cheers,
Miquèl

Changes in v2:
* Added Alexandru Reviewed-by when relevant.
* Indicated that "modes" is a bitmask as suggested.
* Added a mention in the "modes" kdoc about the function that could
  alter its content in the core.
* Added more driver maintainers in Cc.
* Avoided deleting the INDIO_BUFFER_TRIGGERED mode from kfifos.
* Dropped entirely the currentmode setter helper.
* Avoided using the currentmode getter from places where we have access
  to the opaque structure (ie. from the core itself).
* Added a patch to move the iio_buffer_enabled() definition in the core
  as it cannot be inlined anymore.
* Dropped completely the use of the mlock lock form the st-sensor
  drivers by inserting an additional local lock when this was
  needed. Hopefully I didn't fail that, this will need testing.
* Reworded the modes kdoc as advised by Jonathan.

Miquel Raynal (12):
  iio: core: Enhance the kernel doc of modes and currentmodes iio_dev
    entries
  iio: magnetometer: rm3100: Stop abusing the ->currentmode
  iio: adc: stm32-dfsdm: Avoid dereferencing ->currentmode
  iio: st_sensors: Drop the protection on _avail functions
  iio: st_sensors: Add a local lock for protecting odr
  iio: st_sensors: Stop abusing mlock to ensure internal coherency
  iio: st_sensors: Use iio_device_claim/release_direct_mode() when
    relevant
  iio: Un-inline iio_buffer_enabled()
  iio: core: Hide read accesses to iio_dev->currentmode
  iio: core: Move the currentmode entry to the opaque structure
  iio: core: Simplify the registration of kfifo buffers
  iio: core: Clarify the modes

 drivers/iio/accel/bmc150-accel-core.c         |  4 +-
 drivers/iio/accel/fxls8962af-core.c           |  1 -
 drivers/iio/accel/sca3000.c                   |  1 -
 drivers/iio/accel/ssp_accel_sensor.c          |  1 -
 drivers/iio/accel/st_accel_core.c             |  5 +-
 drivers/iio/adc/at91-sama5d2_adc.c            |  4 +-
 drivers/iio/adc/ina2xx-adc.c                  |  1 -
 drivers/iio/adc/stm32-dfsdm-adc.c             |  5 +-
 drivers/iio/adc/ti_am335x_adc.c               |  4 +-
 drivers/iio/buffer/kfifo_buf.c                | 10 +--
 .../cros_ec_sensors/cros_ec_sensors_core.c    |  3 +-
 drivers/iio/common/scmi_sensors/scmi_iio.c    |  1 -
 .../iio/common/st_sensors/st_sensors_core.c   | 60 +++++++++--------
 drivers/iio/gyro/ssp_gyro_sensor.c            |  1 -
 drivers/iio/gyro/st_gyro_core.c               |  5 +-
 drivers/iio/health/max30100.c                 |  1 -
 drivers/iio/health/max30102.c                 |  1 -
 .../iio/imu/inv_icm42600/inv_icm42600_accel.c |  1 -
 .../iio/imu/inv_icm42600/inv_icm42600_gyro.c  |  1 -
 .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    |  1 -
 drivers/iio/industrialio-buffer.c             | 12 ++--
 drivers/iio/industrialio-core.c               | 27 ++++++++
 drivers/iio/industrialio-trigger.c            |  2 +-
 drivers/iio/light/apds9960.c                  |  1 -
 drivers/iio/magnetometer/rm3100-core.c        | 15 +----
 drivers/iio/magnetometer/st_magn_core.c       |  5 +-
 drivers/iio/pressure/st_pressure_core.c       |  8 +--
 .../staging/iio/impedance-analyzer/ad5933.c   |  1 -
 include/linux/iio/common/st_sensors.h         |  3 +
 include/linux/iio/iio-opaque.h                |  4 ++
 include/linux/iio/iio.h                       | 67 +++++++++++++++----
 include/linux/iio/kfifo_buf.h                 |  5 +-
 32 files changed, 149 insertions(+), 112 deletions(-)

-- 
2.27.0

[PATCH v2 01/12] iio: core: Enhance the kernel doc of modes and currentmodes iio_dev entries

[Miquel Raynal]

Let's provide more details about these two variables because their
understanding may not be straightforward for someone not used to the IIO
subsystem internal logic. The different modes will soon be also be more
documented for the same reason.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/iio/iio.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index faf00f2c0be6..f191b80466cd 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -488,8 +488,15 @@ struct iio_buffer_setup_ops {
 
 /**
  * struct iio_dev - industrial I/O device
- * @modes:		[DRIVER] operating modes supported by device
- * @currentmode:	[INTERN] current operating mode
+ * @modes:		[DRIVER] bitmask listing all the operating modes
+ *			supported by the IIO device. This list should be
+ *			initialized before registering the IIO device. It can
+ *			also be filed up by the IIO core, as a result of
+ *			enabling particular features in the driver
+ *			(see iio_triggered_event_setup()).
+ * @currentmode:	[INTERN] operating mode currently in use, may be
+ *			eventually checked by device drivers but should be
+ *			considered read-only as this is a core internal bit
  * @dev:		[DRIVER] device structure, should be assigned a parent
  *			and owner
  * @buffer:		[DRIVER] any buffer present
-- 
2.27.0

[PATCH v2 02/12] iio: magnetometer: rm3100: Stop abusing the ->currentmode

[Miquel Raynal]

This is an internal variable for the core, here it is set to a "default"
value by the driver in order to later be able to perform checks against
it. None of this is needed because this check actually cares about the
buffers being enabled or not. So it is an unproper side-channel access
to the information "are the buffers enabled?", returned officially by
the iio_buffer_enabled() helper. Use this helper instead.

Cc: Song Qiang <songqiang1304521@gmail.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/magnetometer/rm3100-core.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/magnetometer/rm3100-core.c b/drivers/iio/magnetometer/rm3100-core.c
index 13914273c999..be0057f82218 100644
--- a/drivers/iio/magnetometer/rm3100-core.c
+++ b/drivers/iio/magnetometer/rm3100-core.c
@@ -141,18 +141,10 @@ static irqreturn_t rm3100_irq_handler(int irq, void *d)
 	struct iio_dev *indio_dev = d;
 	struct rm3100_data *data = iio_priv(indio_dev);
 
-	switch (indio_dev->currentmode) {
-	case INDIO_DIRECT_MODE:
+	if (!iio_buffer_enabled(indio_dev))
 		complete(&data->measuring_done);
-		break;
-	case INDIO_BUFFER_TRIGGERED:
+	else
 		iio_trigger_poll(data->drdy_trig);
-		break;
-	default:
-		dev_err(indio_dev->dev.parent,
-			"device mode out of control, current mode: %d",
-			indio_dev->currentmode);
-	}
 
 	return IRQ_WAKE_THREAD;
 }
@@ -377,7 +369,7 @@ static int rm3100_set_samp_freq(struct iio_dev *indio_dev, int val, int val2)
 			goto unlock_return;
 	}
 
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+	if (iio_buffer_enabled(indio_dev)) {
 		/* Writing TMRC registers requires CMM reset. */
 		ret = regmap_write(regmap, RM3100_REG_CMM, 0);
 		if (ret < 0)
@@ -553,7 +545,6 @@ int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq)
 	indio_dev->channels = rm3100_channels;
 	indio_dev->num_channels = ARRAY_SIZE(rm3100_channels);
 	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
-	indio_dev->currentmode = INDIO_DIRECT_MODE;
 
 	if (!irq)
 		data->use_interrupt = false;
-- 
2.27.0

[PATCH v2 03/12] iio: adc: stm32-dfsdm: Avoid dereferencing ->currentmode

[Miquel Raynal]

This is an internal variable of the core, let's use the
iio_buffer_enabled() helper which is exported for the following purpose:
telling if the current mode is a buffered mode, which is precisely what
this driver looks for.

Cc: Olivier Moysan <olivier.moysan@foss.st.com>
Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index 1cfefb3b5e56..a3b8827d3bbf 100644
--- a/drivers/iio/adc/stm32-dfsdm-adc.c
+++ b/drivers/iio/adc/stm32-dfsdm-adc.c
@@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
 	 * In continuous mode, use fast mode configuration,
 	 * if it provides a better resolution.
 	 */
-	if (adc->nconv == 1 && !trig &&
-	    (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
+	if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {
 		if (fl->flo[1].res >= fl->flo[0].res) {
 			fl->fast = 1;
 			flo = &fl->flo[1];
@@ -562,7 +561,7 @@ static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
 		cr1 = DFSDM_CR1_RCH(chan->channel);
 
 		/* Continuous conversions triggered by SPI clk in buffer mode */
-		if (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)
+		if (iio_buffer_enabled(indio_dev))
 			cr1 |= DFSDM_CR1_RCONT(1);
 
 		cr1 |= DFSDM_CR1_RSYNC(fl->sync_mode);
-- 
2.27.0

[PATCH v2 04/12] iio: st_sensors: Drop the protection on _avail functions

[Miquel Raynal]

The use of a lock there seems pointless. Besides preventing to read
these information from userspace while buffers are enabled (which is not
supposed to happen), it only protect read accesses over static const
values, which are never supposed to be written anyway.

Drop these lock calls.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Cc: Denis Ciocca <denis.ciocca@st.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/common/st_sensors/st_sensors_core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index eb452d0c423c..6c027150a5a4 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -641,7 +641,6 @@ ssize_t st_sensors_sysfs_sampling_frequency_avail(struct device *dev,
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
 
-	mutex_lock(&indio_dev->mlock);
 	for (i = 0; i < ST_SENSORS_ODR_LIST_MAX; i++) {
 		if (sdata->sensor_settings->odr.odr_avl[i].hz == 0)
 			break;
@@ -649,7 +648,6 @@ ssize_t st_sensors_sysfs_sampling_frequency_avail(struct device *dev,
 		len += scnprintf(buf + len, PAGE_SIZE - len, "%d ",
 				sdata->sensor_settings->odr.odr_avl[i].hz);
 	}
-	mutex_unlock(&indio_dev->mlock);
 	buf[len - 1] = '\n';
 
 	return len;
@@ -663,7 +661,6 @@ ssize_t st_sensors_sysfs_scale_avail(struct device *dev,
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
 
-	mutex_lock(&indio_dev->mlock);
 	for (i = 0; i < ST_SENSORS_FULLSCALE_AVL_MAX; i++) {
 		if (sdata->sensor_settings->fs.fs_avl[i].num == 0)
 			break;
@@ -673,7 +670,6 @@ ssize_t st_sensors_sysfs_scale_avail(struct device *dev,
 
 		len += scnprintf(buf + len, PAGE_SIZE - len, "%u.%06u ", q, r);
 	}
-	mutex_unlock(&indio_dev->mlock);
 	buf[len - 1] = '\n';
 
 	return len;
-- 
2.27.0

[PATCH v2 05/12] iio: st_sensors: Add a local lock for protecting odr

[Miquel Raynal]

Right now the (framework) mlock lock is (ab)used for multiple purposes:
1- protecting concurrent accesses over the odr local cache
2- avoid changing samplig frequency whilst buffer is running

Let's start by handling situation #1 with a local lock.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Cc: Denis Ciocca <denis.ciocca@st.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../iio/common/st_sensors/st_sensors_core.c   | 28 +++++++++++++------
 include/linux/iio/common/st_sensors.h         |  3 ++
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index 6c027150a5a4..d0419234a747 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -71,16 +71,18 @@ static int st_sensors_match_odr(struct st_sensor_settings *sensor_settings,
 
 int st_sensors_set_odr(struct iio_dev *indio_dev, unsigned int odr)
 {
-	int err;
+	int err = 0;
 	struct st_sensor_odr_avl odr_out = {0, 0};
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
 
+	mutex_lock(&sdata->odr_lock);
+
 	if (!sdata->sensor_settings->odr.mask)
-		return 0;
+		goto unlock_mutex;
 
 	err = st_sensors_match_odr(sdata->sensor_settings, odr, &odr_out);
 	if (err < 0)
-		goto st_sensors_match_odr_error;
+		goto unlock_mutex;
 
 	if ((sdata->sensor_settings->odr.addr ==
 					sdata->sensor_settings->pw.addr) &&
@@ -103,7 +105,9 @@ int st_sensors_set_odr(struct iio_dev *indio_dev, unsigned int odr)
 	if (err >= 0)
 		sdata->odr = odr_out.hz;
 
-st_sensors_match_odr_error:
+unlock_mutex:
+	mutex_unlock(&sdata->odr_lock);
+
 	return err;
 }
 EXPORT_SYMBOL(st_sensors_set_odr);
@@ -162,6 +166,8 @@ int st_sensors_set_enable(struct iio_dev *indio_dev, bool enable)
 	struct st_sensor_odr_avl odr_out = {0, 0};
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
 
+	mutex_lock(&sdata->odr_lock);
+
 	if (enable) {
 		tmp_value = sdata->sensor_settings->pw.value_on;
 		if ((sdata->sensor_settings->odr.addr ==
@@ -171,7 +177,7 @@ int st_sensors_set_enable(struct iio_dev *indio_dev, bool enable)
 			err = st_sensors_match_odr(sdata->sensor_settings,
 							sdata->odr, &odr_out);
 			if (err < 0)
-				goto set_enable_error;
+				goto unlock_mutex;
 			tmp_value = odr_out.value;
 			found = true;
 		}
@@ -179,7 +185,7 @@ int st_sensors_set_enable(struct iio_dev *indio_dev, bool enable)
 				sdata->sensor_settings->pw.addr,
 				sdata->sensor_settings->pw.mask, tmp_value);
 		if (err < 0)
-			goto set_enable_error;
+			goto unlock_mutex;
 
 		sdata->enabled = true;
 
@@ -191,12 +197,14 @@ int st_sensors_set_enable(struct iio_dev *indio_dev, bool enable)
 				sdata->sensor_settings->pw.mask,
 				sdata->sensor_settings->pw.value_off);
 		if (err < 0)
-			goto set_enable_error;
+			goto unlock_mutex;
 
 		sdata->enabled = false;
 	}
 
-set_enable_error:
+unlock_mutex:
+	mutex_unlock(&sdata->odr_lock);
+
 	return err;
 }
 EXPORT_SYMBOL(st_sensors_set_enable);
@@ -361,6 +369,8 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev,
 	struct st_sensors_platform_data *of_pdata;
 	int err = 0;
 
+	mutex_init(&sdata->odr_lock);
+
 	/* If OF/DT pdata exists, it will take precedence of anything else */
 	of_pdata = st_sensors_dev_probe(indio_dev->dev.parent, pdata);
 	if (IS_ERR(of_pdata))
@@ -558,8 +568,10 @@ int st_sensors_read_info_raw(struct iio_dev *indio_dev,
 		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;
 
diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index 22f67845cdd3..db4a1b260348 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -237,6 +237,7 @@ struct st_sensor_settings {
  * @hw_irq_trigger: if we're using the hardware interrupt on the sensor.
  * @hw_timestamp: Latest timestamp from the interrupt handler, when in use.
  * @buffer_data: Data used by buffer part.
+ * @odr_lock: Local lock for preventing concurrent ODR accesses/changes
  */
 struct st_sensor_data {
 	struct iio_trigger *trig;
@@ -261,6 +262,8 @@ struct st_sensor_data {
 	s64 hw_timestamp;
 
 	char buffer_data[ST_SENSORS_MAX_BUFFER_SIZE] ____cacheline_aligned;
+
+	struct mutex odr_lock;
 };
 
 #ifdef CONFIG_IIO_BUFFER
-- 
2.27.0

[PATCH v2 06/12] iio: st_sensors: Stop abusing mlock to ensure internal coherency

[Miquel Raynal]

An odr_lock has been introduced to protect local accesses to the odr
internal cache and ensure the cached value always reflected the actual
value. Using the mlock() for this purpose is no longer needed, so let's
drop these extra mutex_lock/unlock() calls.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Cc: Denis Ciocca <denis.ciocca@st.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/accel/st_accel_core.c       | 5 ++---
 drivers/iio/gyro/st_gyro_core.c         | 5 ++---
 drivers/iio/magnetometer/st_magn_core.c | 5 ++---
 drivers/iio/pressure/st_pressure_core.c | 8 ++------
 4 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
index 31ea19d0ba71..d314125269e4 100644
--- a/drivers/iio/accel/st_accel_core.c
+++ b/drivers/iio/accel/st_accel_core.c
@@ -1139,10 +1139,9 @@ static int st_accel_write_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (val2)
 			return -EINVAL;
-		mutex_lock(&indio_dev->mlock);
+
 		err = st_sensors_set_odr(indio_dev, val);
-		mutex_unlock(&indio_dev->mlock);
-		return err;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/iio/gyro/st_gyro_core.c b/drivers/iio/gyro/st_gyro_core.c
index 201050b76fe5..46e3df1bfacb 100644
--- a/drivers/iio/gyro/st_gyro_core.c
+++ b/drivers/iio/gyro/st_gyro_core.c
@@ -415,10 +415,9 @@ static int st_gyro_write_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (val2)
 			return -EINVAL;
-		mutex_lock(&indio_dev->mlock);
+
 		err = st_sensors_set_odr(indio_dev, val);
-		mutex_unlock(&indio_dev->mlock);
-		return err;
+		break;
 	default:
 		err = -EINVAL;
 	}
diff --git a/drivers/iio/magnetometer/st_magn_core.c b/drivers/iio/magnetometer/st_magn_core.c
index 0806a1e65ce4..7b48e7a29cee 100644
--- a/drivers/iio/magnetometer/st_magn_core.c
+++ b/drivers/iio/magnetometer/st_magn_core.c
@@ -549,10 +549,9 @@ static int st_magn_write_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (val2)
 			return -EINVAL;
-		mutex_lock(&indio_dev->mlock);
+
 		err = st_sensors_set_odr(indio_dev, val);
-		mutex_unlock(&indio_dev->mlock);
-		return err;
+		break;
 	default:
 		err = -EINVAL;
 	}
diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index 26a1ee43d56e..05a909eeaff0 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -560,16 +560,12 @@ static int st_press_write_raw(struct iio_dev *indio_dev,
 			      int val2,
 			      long mask)
 {
-	int err;
-
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (val2)
 			return -EINVAL;
-		mutex_lock(&indio_dev->mlock);
-		err = st_sensors_set_odr(indio_dev, val);
-		mutex_unlock(&indio_dev->mlock);
-		return err;
+
+		return st_sensors_set_odr(indio_dev, val);
 	default:
 		return -EINVAL;
 	}
-- 
2.27.0

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

[Miquel Raynal]

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);
+	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;
 }
-- 
2.27.0

[PATCH v2 08/12] iio: Un-inline iio_buffer_enabled()

[Miquel Raynal]

As we are going to hide the currentmode inside the opaque structure,
this helper would soon need to call a non-inline function which would
simply drop the benefit of having the helper defined inline in a header.

One alternative is to move this helper in the core as there is no more
interest in defining it inline in a header. We will pay the minor cost
either way.

Let's do like the iio_device_id() helper which also refers to the opaque
structure and gets defined in the core.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/industrialio-core.c | 12 ++++++++++++
 include/linux/iio/iio.h         | 11 +----------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 409c278a4c2c..64381d0a3d01 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -184,6 +184,18 @@ int iio_device_id(struct iio_dev *indio_dev)
 }
 EXPORT_SYMBOL_GPL(iio_device_id);
 
+/**
+ * iio_buffer_enabled() - helper function to test if the buffer is enabled
+ * @indio_dev:		IIO device structure for device
+ */
+bool iio_buffer_enabled(struct iio_dev *indio_dev)
+{
+	return indio_dev->currentmode
+		& (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
+		   INDIO_BUFFER_SOFTWARE);
+}
+EXPORT_SYMBOL_GPL(iio_buffer_enabled);
+
 /**
  * iio_sysfs_match_string_with_gaps - matches given string in an array with gaps
  * @array: array of strings
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index f191b80466cd..faabb852128a 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -550,6 +550,7 @@ struct iio_dev {
 };
 
 int iio_device_id(struct iio_dev *indio_dev);
+bool iio_buffer_enabled(struct iio_dev *indio_dev);
 
 const struct iio_chan_spec
 *iio_find_channel_from_si(struct iio_dev *indio_dev, int si);
@@ -679,16 +680,6 @@ struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv);
 __printf(2, 3)
 struct iio_trigger *devm_iio_trigger_alloc(struct device *parent,
 					   const char *fmt, ...);
-/**
- * iio_buffer_enabled() - helper function to test if the buffer is enabled
- * @indio_dev:		IIO device structure for device
- **/
-static inline bool iio_buffer_enabled(struct iio_dev *indio_dev)
-{
-	return indio_dev->currentmode
-		& (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
-		   INDIO_BUFFER_SOFTWARE);
-}
 
 /**
  * iio_get_debugfs_dentry() - helper function to get the debugfs_dentry
-- 
2.27.0

[PATCH v2 09/12] iio: core: Hide read accesses to iio_dev->currentmode

[Miquel Raynal]

In order to later move this variable within the opaque structure, let's
create a helper for accessing it in read-only mode. This helper will be
exposed to device drivers and kept accessible for the few that could need
it. The write access to this variable however should be fully reserved to
the core so in a second step we will hide this variable into the opaque
structure.

Cc: Eugen Hristev <eugen.hristev@microchip.com>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Ludovic Desroches <ludovic.desroches@microchip.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/accel/bmc150-accel-core.c |  4 ++--
 drivers/iio/adc/at91-sama5d2_adc.c    |  4 ++--
 drivers/iio/industrialio-core.c       | 11 +++++++++++
 include/linux/iio/iio.h               |  1 +
 4 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c
index e6081dd0a880..0ecded6d87df 100644
--- a/drivers/iio/accel/bmc150-accel-core.c
+++ b/drivers/iio/accel/bmc150-accel-core.c
@@ -1525,7 +1525,7 @@ static int bmc150_accel_buffer_postenable(struct iio_dev *indio_dev)
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 	int ret = 0;
 
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
+	if (iio_device_get_current_mode(indio_dev) == INDIO_BUFFER_TRIGGERED)
 		return 0;
 
 	mutex_lock(&data->mutex);
@@ -1557,7 +1557,7 @@ static int bmc150_accel_buffer_predisable(struct iio_dev *indio_dev)
 {
 	struct bmc150_accel_data *data = iio_priv(indio_dev);
 
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
+	if (iio_device_get_current_mode(indio_dev) == INDIO_BUFFER_TRIGGERED)
 		return 0;
 
 	mutex_lock(&data->mutex);
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 854b1f81d807..b764823ce57e 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1117,7 +1117,7 @@ static int at91_adc_buffer_prepare(struct iio_dev *indio_dev)
 		return at91_adc_configure_touch(st, true);
 
 	/* if we are not in triggered mode, we cannot enable the buffer. */
-	if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
+	if (!(iio_device_get_current_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
 		return -EINVAL;
 
 	/* we continue with the triggered buffer */
@@ -1159,7 +1159,7 @@ static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev)
 		return at91_adc_configure_touch(st, false);
 
 	/* if we are not in triggered mode, nothing to do here */
-	if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
+	if (!(iio_device_get_current_mode(indio_dev) & INDIO_ALL_TRIGGERED_MODES))
 		return -EINVAL;
 
 	/*
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 64381d0a3d01..f8fcfe12dfa2 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -2071,6 +2071,17 @@ void iio_device_release_direct_mode(struct iio_dev *indio_dev)
 }
 EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
 
+/**
+ * iio_device_get_current_mode() - helper function providing read-only access to
+ *				   the @currentmode variable
+ * @indio_dev:			   IIO device structure for device
+ */
+int iio_device_get_current_mode(struct iio_dev *indio_dev)
+{
+	return indio_dev->currentmode;
+}
+EXPORT_SYMBOL_GPL(iio_device_get_current_mode);
+
 subsys_initcall(iio_init);
 module_exit(iio_exit);
 
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index faabb852128a..31098ffa7dc9 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -550,6 +550,7 @@ struct iio_dev {
 };
 
 int iio_device_id(struct iio_dev *indio_dev);
+int iio_device_get_current_mode(struct iio_dev *indio_dev);
 bool iio_buffer_enabled(struct iio_dev *indio_dev);
 
 const struct iio_chan_spec
-- 
2.27.0

[PATCH v2 10/12] iio: core: Move the currentmode entry to the opaque structure

[Miquel Raynal]

This entry should, under no situation, be modified by device
drivers. Now that we have limited its read access to device drivers
really needing it and did so through a dedicated helper, we can
easily move this variable to the opaque structure in order to
prevent any further modification from non-authorized code (out of the
core, basically).

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>
---
 drivers/iio/industrialio-buffer.c  | 12 ++++++------
 drivers/iio/industrialio-core.c    | 10 +++++++---
 drivers/iio/industrialio-trigger.c |  2 +-
 include/linux/iio/iio-opaque.h     |  4 ++++
 include/linux/iio/iio.h            |  4 ----
 5 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 94eb9f6cf128..2a5a09693b08 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1065,7 +1065,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 	indio_dev->active_scan_mask = config->scan_mask;
 	indio_dev->scan_timestamp = config->scan_timestamp;
 	indio_dev->scan_bytes = config->scan_bytes;
-	indio_dev->currentmode = config->mode;
+	iio_dev_opaque->currentmode = config->mode;
 
 	iio_update_demux(indio_dev);
 
@@ -1101,7 +1101,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 			goto err_disable_buffers;
 	}
 
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+	if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED) {
 		ret = iio_trigger_attach_poll_func(indio_dev->trig,
 						   indio_dev->pollfunc);
 		if (ret)
@@ -1120,7 +1120,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 	return 0;
 
 err_detach_pollfunc:
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+	if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED) {
 		iio_trigger_detach_poll_func(indio_dev->trig,
 					     indio_dev->pollfunc);
 	}
@@ -1132,7 +1132,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 	if (indio_dev->setup_ops->postdisable)
 		indio_dev->setup_ops->postdisable(indio_dev);
 err_undo_config:
-	indio_dev->currentmode = INDIO_DIRECT_MODE;
+	iio_dev_opaque->currentmode = INDIO_DIRECT_MODE;
 	indio_dev->active_scan_mask = NULL;
 
 	return ret;
@@ -1162,7 +1162,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
 			ret = ret2;
 	}
 
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+	if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED) {
 		iio_trigger_detach_poll_func(indio_dev->trig,
 					     indio_dev->pollfunc);
 	}
@@ -1181,7 +1181,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
 
 	iio_free_scan_mask(indio_dev, indio_dev->active_scan_mask);
 	indio_dev->active_scan_mask = NULL;
-	indio_dev->currentmode = INDIO_DIRECT_MODE;
+	iio_dev_opaque->currentmode = INDIO_DIRECT_MODE;
 
 	return ret;
 }
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index f8fcfe12dfa2..b757273e6652 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -190,7 +190,9 @@ EXPORT_SYMBOL_GPL(iio_device_id);
  */
 bool iio_buffer_enabled(struct iio_dev *indio_dev)
 {
-	return indio_dev->currentmode
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+
+	return iio_dev_opaque->currentmode
 		& (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE |
 		   INDIO_BUFFER_SOFTWARE);
 }
@@ -2073,12 +2075,14 @@ EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
 
 /**
  * iio_device_get_current_mode() - helper function providing read-only access to
- *				   the @currentmode variable
+ *				   the opaque @currentmode variable
  * @indio_dev:			   IIO device structure for device
  */
 int iio_device_get_current_mode(struct iio_dev *indio_dev)
 {
-	return indio_dev->currentmode;
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+
+	return iio_dev_opaque->currentmode;
 }
 EXPORT_SYMBOL_GPL(iio_device_get_current_mode);
 
diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index f504ed351b3e..585b6cef8fcc 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -444,7 +444,7 @@ static ssize_t iio_trigger_write_current(struct device *dev,
 	int ret;
 
 	mutex_lock(&indio_dev->mlock);
-	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+	if (iio_dev_opaque->currentmode == INDIO_BUFFER_TRIGGERED) {
 		mutex_unlock(&indio_dev->mlock);
 		return -EBUSY;
 	}
diff --git a/include/linux/iio/iio-opaque.h b/include/linux/iio/iio-opaque.h
index 2be12b7b5dc5..6b3586b3f952 100644
--- a/include/linux/iio/iio-opaque.h
+++ b/include/linux/iio/iio-opaque.h
@@ -7,6 +7,9 @@
  * struct iio_dev_opaque - industrial I/O device opaque information
  * @indio_dev:			public industrial I/O device information
  * @id:			used to identify device internally
+ * @currentmode:		operating mode currently in use, may be eventually
+ *				checked by device drivers but should be considered
+ *				read-only as this is a core internal bit
  * @driver_module:		used to make it harder to undercut users
  * @info_exist_lock:		lock to prevent use during removal
  * @trig_readonly:		mark the current trigger immutable
@@ -36,6 +39,7 @@
  */
 struct iio_dev_opaque {
 	struct iio_dev			indio_dev;
+	int				currentmode;
 	int				id;
 	struct module			*driver_module;
 	struct mutex			info_exist_lock;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 31098ffa7dc9..85cb924debd9 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -494,9 +494,6 @@ struct iio_buffer_setup_ops {
  *			also be filed up by the IIO core, as a result of
  *			enabling particular features in the driver
  *			(see iio_triggered_event_setup()).
- * @currentmode:	[INTERN] operating mode currently in use, may be
- *			eventually checked by device drivers but should be
- *			considered read-only as this is a core internal bit
  * @dev:		[DRIVER] device structure, should be assigned a parent
  *			and owner
  * @buffer:		[DRIVER] any buffer present
@@ -523,7 +520,6 @@ struct iio_buffer_setup_ops {
  */
 struct iio_dev {
 	int				modes;
-	int				currentmode;
 	struct device			dev;
 
 	struct iio_buffer		*buffer;
-- 
2.27.0

[PATCH v2 11/12] iio: core: Simplify the registration of kfifo buffers

[Miquel Raynal]

Among all the users of the kfifo buffers, no one uses the
INDIO_BUFFER_HARDWARE mode. So let's take this as a general rule and
simplify a little bit the internals - overall the documentation - by
eliminating unused specific cases. Use the INDIO_BUFFER_SOFTWARE mode by
default with kfifo buffers, which will basically mimic what all the "non
direct" modes do.

Cc: Benson Leung <bleung@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Jyoti Bhayana <jbhayana@google.com>
Cc: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Cc: Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>
Cc: Michael Hennerich <Michael.Hennerich@analog.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/accel/fxls8962af-core.c                    |  1 -
 drivers/iio/accel/sca3000.c                            |  1 -
 drivers/iio/accel/ssp_accel_sensor.c                   |  1 -
 drivers/iio/adc/ina2xx-adc.c                           |  1 -
 drivers/iio/adc/ti_am335x_adc.c                        |  4 +---
 drivers/iio/buffer/kfifo_buf.c                         | 10 +---------
 .../iio/common/cros_ec_sensors/cros_ec_sensors_core.c  |  3 +--
 drivers/iio/common/scmi_sensors/scmi_iio.c             |  1 -
 drivers/iio/gyro/ssp_gyro_sensor.c                     |  1 -
 drivers/iio/health/max30100.c                          |  1 -
 drivers/iio/health/max30102.c                          |  1 -
 drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c      |  1 -
 drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c       |  1 -
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c         |  1 -
 drivers/iio/light/apds9960.c                           |  1 -
 drivers/staging/iio/impedance-analyzer/ad5933.c        |  1 -
 include/linux/iio/kfifo_buf.h                          |  5 ++---
 17 files changed, 5 insertions(+), 30 deletions(-)

diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
index 32989d91b982..cf494937a346 100644
--- a/drivers/iio/accel/fxls8962af-core.c
+++ b/drivers/iio/accel/fxls8962af-core.c
@@ -1209,7 +1209,6 @@ int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
 			return ret;
 
 		ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
-						  INDIO_BUFFER_SOFTWARE,
 						  &fxls8962af_buffer_ops);
 		if (ret)
 			return ret;
diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index 43ecacbdc95a..13b467c74636 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -1474,7 +1474,6 @@ static int sca3000_probe(struct spi_device *spi)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &sca3000_ring_setup_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/accel/ssp_accel_sensor.c b/drivers/iio/accel/ssp_accel_sensor.c
index 04dcb2b657ee..d20f8d299c11 100644
--- a/drivers/iio/accel/ssp_accel_sensor.c
+++ b/drivers/iio/accel/ssp_accel_sensor.c
@@ -113,7 +113,6 @@ static int ssp_accel_probe(struct platform_device *pdev)
 	indio_dev->available_scan_masks = ssp_accel_scan_mask;
 
 	ret = devm_iio_kfifo_buffer_setup(&pdev->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &ssp_accel_buffer_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 4f9992a51e64..05fdfd06e3b0 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -1027,7 +1027,6 @@ static int ina2xx_probe(struct i2c_client *client,
 	indio_dev->name = id->name;
 
 	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &ina2xx_setup_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index dbdc1ef48566..567d43a30955 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -376,9 +376,7 @@ static int tiadc_iio_buffered_hardware_setup(struct device *dev,
 {
 	int ret;
 
-	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
-					  setup_ops);
+	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, setup_ops);
 	if (ret)
 		return ret;
 
diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
index 416d35a61ae2..35d8b4077376 100644
--- a/drivers/iio/buffer/kfifo_buf.c
+++ b/drivers/iio/buffer/kfifo_buf.c
@@ -259,8 +259,6 @@ static struct iio_buffer *devm_iio_kfifo_allocate(struct device *dev)
  * devm_iio_kfifo_buffer_setup_ext - Allocate a kfifo buffer & attach it to an IIO device
  * @dev: Device object to which to attach the life-time of this kfifo buffer
  * @indio_dev: The device the buffer should be attached to
- * @mode_flags: The mode flags for this buffer (INDIO_BUFFER_SOFTWARE and/or
- *		INDIO_BUFFER_TRIGGERED).
  * @setup_ops: The setup_ops required to configure the HW part of the buffer (optional)
  * @buffer_attrs: Extra sysfs buffer attributes for this IIO buffer
  *
@@ -271,22 +269,16 @@ static struct iio_buffer *devm_iio_kfifo_allocate(struct device *dev)
  */
 int devm_iio_kfifo_buffer_setup_ext(struct device *dev,
 				    struct iio_dev *indio_dev,
-				    int mode_flags,
 				    const struct iio_buffer_setup_ops *setup_ops,
 				    const struct attribute **buffer_attrs)
 {
 	struct iio_buffer *buffer;
 
-	if (!mode_flags)
-		return -EINVAL;
-
 	buffer = devm_iio_kfifo_allocate(dev);
 	if (!buffer)
 		return -ENOMEM;
 
-	mode_flags &= kfifo_access_funcs.modes;
-
-	indio_dev->modes |= mode_flags;
+	indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
 	indio_dev->setup_ops = setup_ops;
 
 	buffer->attrs = buffer_attrs;
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
index b2725c6adc7f..a4cf1d9a8a49 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
@@ -333,8 +333,7 @@ int cros_ec_sensors_core_init(struct platform_device *pdev,
 			 * We can not use trigger here, as events are generated
 			 * as soon as sample_frequency is set.
 			 */
-			ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev,
-							      INDIO_BUFFER_SOFTWARE, NULL,
+			ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev, NULL,
 							      cros_ec_sensor_fifo_attributes);
 			if (ret)
 				return ret;
diff --git a/drivers/iio/common/scmi_sensors/scmi_iio.c b/drivers/iio/common/scmi_sensors/scmi_iio.c
index d538bf3ab1ef..793d628db55f 100644
--- a/drivers/iio/common/scmi_sensors/scmi_iio.c
+++ b/drivers/iio/common/scmi_sensors/scmi_iio.c
@@ -686,7 +686,6 @@ static int scmi_iio_dev_probe(struct scmi_device *sdev)
 
 		err = devm_iio_kfifo_buffer_setup(&scmi_iio_dev->dev,
 						  scmi_iio_dev,
-						  INDIO_BUFFER_SOFTWARE,
 						  &scmi_iio_buffer_ops);
 		if (err < 0) {
 			dev_err(dev,
diff --git a/drivers/iio/gyro/ssp_gyro_sensor.c b/drivers/iio/gyro/ssp_gyro_sensor.c
index 46ed12771d2f..eebc036717af 100644
--- a/drivers/iio/gyro/ssp_gyro_sensor.c
+++ b/drivers/iio/gyro/ssp_gyro_sensor.c
@@ -113,7 +113,6 @@ static int ssp_gyro_probe(struct platform_device *pdev)
 	indio_dev->available_scan_masks = ssp_gyro_scan_mask;
 
 	ret = devm_iio_kfifo_buffer_setup(&pdev->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &ssp_gyro_buffer_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
index 36ba7611d9ce..ad5717965223 100644
--- a/drivers/iio/health/max30100.c
+++ b/drivers/iio/health/max30100.c
@@ -433,7 +433,6 @@ static int max30100_probe(struct i2c_client *client,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &max30100_buffer_setup_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/health/max30102.c b/drivers/iio/health/max30102.c
index 2292876c55e2..abbcef563807 100644
--- a/drivers/iio/health/max30102.c
+++ b/drivers/iio/health/max30102.c
@@ -542,7 +542,6 @@ static int max30102_probe(struct i2c_client *client,
 	}
 
 	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &max30102_buffer_setup_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
index 383cc3250342..c3f433ad3af6 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
@@ -731,7 +731,6 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
 	indio_dev->available_scan_masks = inv_icm42600_accel_scan_masks;
 
 	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &inv_icm42600_buffer_ops);
 	if (ret)
 		return ERR_PTR(ret);
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
index cec1dd0e0464..9d94a8518e3c 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
@@ -743,7 +743,6 @@ struct iio_dev *inv_icm42600_gyro_init(struct inv_icm42600_state *st)
 	indio_dev->setup_ops = &inv_icm42600_buffer_ops;
 
 	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &inv_icm42600_buffer_ops);
 	if (ret)
 		return ERR_PTR(ret);
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index 16730a780964..f80c62849d30 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -746,7 +746,6 @@ int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
 			continue;
 
 		ret = devm_iio_kfifo_buffer_setup(hw->dev, hw->iio_devs[i],
-						  INDIO_BUFFER_SOFTWARE,
 						  &st_lsm6dsx_buffer_ops);
 		if (ret)
 			return ret;
diff --git a/drivers/iio/light/apds9960.c b/drivers/iio/light/apds9960.c
index 4141c0fa7bc4..09b831f9f40b 100644
--- a/drivers/iio/light/apds9960.c
+++ b/drivers/iio/light/apds9960.c
@@ -1003,7 +1003,6 @@ static int apds9960_probe(struct i2c_client *client,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &apds9960_buffer_setup_ops);
 	if (ret)
 		return ret;
diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
index 793918e1c45f..f177b20f0f2d 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -749,7 +749,6 @@ static int ad5933_probe(struct i2c_client *client,
 	indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
 
 	ret = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
-					  INDIO_BUFFER_SOFTWARE,
 					  &ad5933_ring_setup_ops);
 	if (ret)
 		return ret;
diff --git a/include/linux/iio/kfifo_buf.h b/include/linux/iio/kfifo_buf.h
index ccd2ceae7b25..8a83fb58232d 100644
--- a/include/linux/iio/kfifo_buf.h
+++ b/include/linux/iio/kfifo_buf.h
@@ -12,11 +12,10 @@ void iio_kfifo_free(struct iio_buffer *r);
 
 int devm_iio_kfifo_buffer_setup_ext(struct device *dev,
 				    struct iio_dev *indio_dev,
-				    int mode_flags,
 				    const struct iio_buffer_setup_ops *setup_ops,
 				    const struct attribute **buffer_attrs);
 
-#define devm_iio_kfifo_buffer_setup(dev, indio_dev, mode_flags, setup_ops)	\
-	devm_iio_kfifo_buffer_setup_ext((dev), (indio_dev), (mode_flags), (setup_ops), NULL)
+#define devm_iio_kfifo_buffer_setup(dev, indio_dev, setup_ops)	\
+	devm_iio_kfifo_buffer_setup_ext((dev), (indio_dev), (setup_ops), NULL)
 
 #endif
-- 
2.27.0

[PATCH v2 12/12] iio: core: Clarify the modes

[Miquel Raynal]

As part of a previous discussion with Jonathan Cameron [1], it appeared
necessary to clarify the meaning of each mode so that new developers
could understand better what they should use or not use and when.

The idea of renaming these modes as been let aside because naming is a
big deal and requires a lot of thinking. So for now let's focus on
correctly explaining what each mode implies.

[1] https://lore.kernel.org/linux-iio/20210930165510.2295e6c4@jic23-huawei/

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/iio/iio.h | 46 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 85cb924debd9..66eccae2b9d1 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -315,7 +315,51 @@ static inline bool iio_channel_has_available(const struct iio_chan_spec *chan,
 s64 iio_get_time_ns(const struct iio_dev *indio_dev);
 unsigned int iio_get_time_res(const struct iio_dev *indio_dev);
 
-/* Device operating modes */
+/**
+ * Device operating modes
+ * @INDIO_DIRECT_MODE: There is an access to the timely single value available.
+ * On most devices, this is a single-shot read. On some devices with data
+ * streams without an 'on-demand' function, this might also be the 'last value'
+ * feature. Above all, this mode internally means that we are not in any of the
+ * other modes, and sysfs reads should work.
+ * Device drivers should inform the core if they support this mode.
+ * @INDIO_BUFFER_TRIGGERED: Common mode when dealing with kfifo buffers.
+ * It indicates that an explicit trigger is required. This requests the core to
+ * attach a poll function when enabling the buffer, which is indicated by the
+ * _TRIGGERED suffix.
+ * The core will ensure this mode is set when registering a triggered buffer
+ * with iio_triggered_buffer_setup().
+ * @INDIO_BUFFER_SOFTWARE: Another kfifo buffer mode, but not event triggered.
+ * No poll function can be attached because there is no triggered infrastructure
+ * we can use to cause capture. There is a kfifo that the hardware will fill,
+ * but not "only one scan at a time". Typically, hardware will have a buffer
+ * that can hold multiple scans. Software may read one or more scans at a single
+ * time and push the available data to a Kfifo. This means the core will not
+ * attach any poll function when enabling the buffer.
+ * The core will ensure this mode is set when registering a simple kfifo buffer
+ * with devm_iio_kfifo_buffer_setup().
+ * @INDIO_BUFFER_HARDWARE: For specific hardware, if unsure do not use this mode.
+ * Same as above but this time the buffer is not a kfifo where we have direct
+ * access to the data. Instead, the consumer driver must access the data through
+ * non software visible channels (or DMA when there is no demux possible in
+ * software)
+ * The core will ensure this mode is set when registering a dmaengine buffer
+ * with devm_iio_dmaengine_buffer_setup().
+ * @INDIO_EVENT_TRIGGERED: Very unusual mode.
+ * Triggers usually refer to an external event which will start data capture.
+ * Here it is kind of the opposite as, a particular state of the data might
+ * produce an event which can be considered as an event. We don't necessarily
+ * have access to the data itself, but to the event produced. For example, this
+ * can be a threshold detector. The internal path of this mode is very close to
+ * the INDIO_BUFFER_TRIGGERED mode.
+ * The core will ensure this mode is set when registering a triggered event.
+ * @INDIO_HARDWARE_TRIGGERED: Very unusual mode.
+ * Here, triggers can result in data capture and can be routed to multiple
+ * hardware components, which make them close to regular triggers in the way
+ * they must be managed by the core, but without the entire interrupts/poll
+ * functions burden. Interrupts are irrelevant as the data flow is hardware
+ * mediated and distributed.
+ */
 #define INDIO_DIRECT_MODE		0x01
 #define INDIO_BUFFER_TRIGGERED		0x02
 #define INDIO_BUFFER_SOFTWARE		0x04
-- 
2.27.0

Re: [PATCH v2 03/12] iio: adc: stm32-dfsdm: Avoid dereferencing ->currentmode

[Fabrice Gasnier]

On 2/2/22 3:01 PM, Miquel Raynal wrote:
> This is an internal variable of the core, let's use the
> iio_buffer_enabled() helper which is exported for the following purpose:
> telling if the current mode is a buffered mode, which is precisely what
> this driver looks for.
> 
> Cc: Olivier Moysan <olivier.moysan@foss.st.com>
> Cc: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---

Hi Miquel,

Thanks for your patch, and your answer in v1.

Please feel free to add my:
Reviewed-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Best Regards,
Fabrice

>  drivers/iio/adc/stm32-dfsdm-adc.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
> index 1cfefb3b5e56..a3b8827d3bbf 100644
> --- a/drivers/iio/adc/stm32-dfsdm-adc.c
> +++ b/drivers/iio/adc/stm32-dfsdm-adc.c
> @@ -466,8 +466,7 @@ static int stm32_dfsdm_channels_configure(struct iio_dev *indio_dev,
>  	 * In continuous mode, use fast mode configuration,
>  	 * if it provides a better resolution.
>  	 */
> -	if (adc->nconv == 1 && !trig &&
> -	    (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)) {
> +	if (adc->nconv == 1 && !trig && iio_buffer_enabled(indio_dev)) {
>  		if (fl->flo[1].res >= fl->flo[0].res) {
>  			fl->fast = 1;
>  			flo = &fl->flo[1];
> @@ -562,7 +561,7 @@ static int stm32_dfsdm_filter_configure(struct iio_dev *indio_dev,
>  		cr1 = DFSDM_CR1_RCH(chan->channel);
>  
>  		/* Continuous conversions triggered by SPI clk in buffer mode */
> -		if (indio_dev->currentmode & INDIO_BUFFER_SOFTWARE)
> +		if (iio_buffer_enabled(indio_dev))
>  			cr1 |= DFSDM_CR1_RCONT(1);
>  
>  		cr1 |= DFSDM_CR1_RSYNC(fl->sync_mode);
> 

Re: [PATCH v2 06/12] iio: st_sensors: Stop abusing mlock to ensure internal coherency

[Jonathan Cameron]

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

> An odr_lock has been introduced to protect local accesses to the odr
> internal cache and ensure the cached value always reflected the actual
> value. Using the mlock() for this purpose is no longer needed, so let's
> drop these extra mutex_lock/unlock() calls.
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Cc: Denis Ciocca <denis.ciocca@st.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Obviously a different issue but all the write_raw() functions should
use the pattern used in st_pressure_core.c and do early
returns seeing as there doesn't seem to be any cleanup to do.

We can tidy that up in a follow up patch as I'm sure there are other
areas in these drivers where direct returns would be nicer than
what is there currently!

Otherwise looks good to me.

> ---
>  drivers/iio/accel/st_accel_core.c       | 5 ++---
>  drivers/iio/gyro/st_gyro_core.c         | 5 ++---
>  drivers/iio/magnetometer/st_magn_core.c | 5 ++---
>  drivers/iio/pressure/st_pressure_core.c | 8 ++------
>  4 files changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
> index 31ea19d0ba71..d314125269e4 100644
> --- a/drivers/iio/accel/st_accel_core.c
> +++ b/drivers/iio/accel/st_accel_core.c
> @@ -1139,10 +1139,9 @@ static int st_accel_write_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		if (val2)
>  			return -EINVAL;
> -		mutex_lock(&indio_dev->mlock);
> +
>  		err = st_sensors_set_odr(indio_dev, val);
> -		mutex_unlock(&indio_dev->mlock);
> -		return err;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/drivers/iio/gyro/st_gyro_core.c b/drivers/iio/gyro/st_gyro_core.c
> index 201050b76fe5..46e3df1bfacb 100644
> --- a/drivers/iio/gyro/st_gyro_core.c
> +++ b/drivers/iio/gyro/st_gyro_core.c
> @@ -415,10 +415,9 @@ static int st_gyro_write_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		if (val2)
>  			return -EINVAL;
> -		mutex_lock(&indio_dev->mlock);
> +
>  		err = st_sensors_set_odr(indio_dev, val);
> -		mutex_unlock(&indio_dev->mlock);
> -		return err;
> +		break;
>  	default:
>  		err = -EINVAL;
>  	}
> diff --git a/drivers/iio/magnetometer/st_magn_core.c b/drivers/iio/magnetometer/st_magn_core.c
> index 0806a1e65ce4..7b48e7a29cee 100644
> --- a/drivers/iio/magnetometer/st_magn_core.c
> +++ b/drivers/iio/magnetometer/st_magn_core.c
> @@ -549,10 +549,9 @@ static int st_magn_write_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		if (val2)
>  			return -EINVAL;
> -		mutex_lock(&indio_dev->mlock);
> +
>  		err = st_sensors_set_odr(indio_dev, val);
> -		mutex_unlock(&indio_dev->mlock);
> -		return err;
> +		break;
>  	default:
>  		err = -EINVAL;
>  	}
> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
> index 26a1ee43d56e..05a909eeaff0 100644
> --- a/drivers/iio/pressure/st_pressure_core.c
> +++ b/drivers/iio/pressure/st_pressure_core.c
> @@ -560,16 +560,12 @@ static int st_press_write_raw(struct iio_dev *indio_dev,
>  			      int val2,
>  			      long mask)
>  {
> -	int err;
> -
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		if (val2)
>  			return -EINVAL;
> -		mutex_lock(&indio_dev->mlock);
> -		err = st_sensors_set_odr(indio_dev, val);
> -		mutex_unlock(&indio_dev->mlock);
> -		return err;
> +
> +		return st_sensors_set_odr(indio_dev, val);
>  	default:
>  		return -EINVAL;
>  	}

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

[Jonathan Cameron]

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

Re: [PATCH v2 12/12] iio: core: Clarify the modes

[Jonathan Cameron]

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

> As part of a previous discussion with Jonathan Cameron [1], it appeared
> necessary to clarify the meaning of each mode so that new developers
> could understand better what they should use or not use and when.
> 
> The idea of renaming these modes as been let aside because naming is a
> big deal and requires a lot of thinking. So for now let's focus on
> correctly explaining what each mode implies.
> 
> [1] https://lore.kernel.org/linux-iio/20210930165510.2295e6c4@jic23-huawei/
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Very nice.  One random comment on another area where our docs could do with
improvements and one trivial suggested clarification for this patch.

Thanks for your hard work tidying this up!

I've looked over all the other patches in the series and where I haven't
commented they look good to me.

Thanks,

Jonathan

> ---
>  include/linux/iio/iio.h | 46 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 85cb924debd9..66eccae2b9d1 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -315,7 +315,51 @@ static inline bool iio_channel_has_available(const struct iio_chan_spec *chan,
>  s64 iio_get_time_ns(const struct iio_dev *indio_dev);
>  unsigned int iio_get_time_res(const struct iio_dev *indio_dev);
>  
> -/* Device operating modes */
> +/**
> + * Device operating modes
> + * @INDIO_DIRECT_MODE: There is an access to the timely single value available.
> + * On most devices, this is a single-shot read. On some devices with data
> + * streams without an 'on-demand' function, this might also be the 'last value'
> + * feature. Above all, this mode internally means that we are not in any of the
> + * other modes, and sysfs reads should work.
> + * Device drivers should inform the core if they support this mode.
> + * @INDIO_BUFFER_TRIGGERED: Common mode when dealing with kfifo buffers.
> + * It indicates that an explicit trigger is required. This requests the core to
> + * attach a poll function when enabling the buffer, which is indicated by the
> + * _TRIGGERED suffix.
> + * The core will ensure this mode is set when registering a triggered buffer
> + * with iio_triggered_buffer_setup().

This isn't the best place for it, but we could probably do with clear
documentation somewhere of how to set default triggers for a device and
how to restrict which device / trigger combinations are possible.
Maybe we should build on your good work here with some extra detail in the
main Documentation.  The validate case is touched on there, but I don't
see anything about defaults
indio_dev->trig = iio_trigger_get(trig); 
that's needed to ensure correct reference counting.



> + * @INDIO_BUFFER_SOFTWARE: Another kfifo buffer mode, but not event triggered.
> + * No poll function can be attached because there is no triggered infrastructure
> + * we can use to cause capture. There is a kfifo that the hardware will fill,
Slight misleading. Perhaps better as:
There is a kfifo that the driver will fill, but not "only one scan at a time".

> + * but not "only one scan at a time". Typically, hardware will have a buffer
> + * that can hold multiple scans. Software may read one or more scans at a single
> + * time and push the available data to a Kfifo. This means the core will not
> + * attach any poll function when enabling the buffer.
> + * The core will ensure this mode is set when registering a simple kfifo buffer
> + * with devm_iio_kfifo_buffer_setup().
> + * @INDIO_BUFFER_HARDWARE: For specific hardware, if unsure do not use this mode.
> + * Same as above but this time the buffer is not a kfifo where we have direct
> + * access to the data. Instead, the consumer driver must access the data through
> + * non software visible channels (or DMA when there is no demux possible in
> + * software)
> + * The core will ensure this mode is set when registering a dmaengine buffer
> + * with devm_iio_dmaengine_buffer_setup().
> + * @INDIO_EVENT_TRIGGERED: Very unusual mode.
> + * Triggers usually refer to an external event which will start data capture.
> + * Here it is kind of the opposite as, a particular state of the data might
> + * produce an event which can be considered as an event. We don't necessarily
> + * have access to the data itself, but to the event produced. For example, this
> + * can be a threshold detector. The internal path of this mode is very close to
> + * the INDIO_BUFFER_TRIGGERED mode.
> + * The core will ensure this mode is set when registering a triggered event.
> + * @INDIO_HARDWARE_TRIGGERED: Very unusual mode.
> + * Here, triggers can result in data capture and can be routed to multiple
> + * hardware components, which make them close to regular triggers in the way
> + * they must be managed by the core, but without the entire interrupts/poll
> + * functions burden. Interrupts are irrelevant as the data flow is hardware
> + * mediated and distributed.
> + */
>  #define INDIO_DIRECT_MODE		0x01
>  #define INDIO_BUFFER_TRIGGERED		0x02
>  #define INDIO_BUFFER_SOFTWARE		0x04

Re: [PATCH v2 06/12] iio: st_sensors: Stop abusing mlock to ensure internal coherency

[Miquel Raynal]

Hi Jonathan,

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

> On Wed,  2 Feb 2022 15:02:02 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > An odr_lock has been introduced to protect local accesses to the odr
> > internal cache and ensure the cached value always reflected the actual
> > value. Using the mlock() for this purpose is no longer needed, so let's
> > drop these extra mutex_lock/unlock() calls.
> > 
> > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > Cc: Denis Ciocca <denis.ciocca@st.com>
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> 
> Obviously a different issue but all the write_raw() functions should
> use the pattern used in st_pressure_core.c and do early
> returns seeing as there doesn't seem to be any cleanup to do.

I honestly hesitated when dropping the mlocks there. I've done it in
v3, so that we don't hurt our eyes on this anymore :)

> 
> We can tidy that up in a follow up patch as I'm sure there are other
> areas in these drivers where direct returns would be nicer than
> what is there currently!

Thanks,
Miquèl

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

[Miquel Raynal]

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

Re: [PATCH v2 12/12] iio: core: Clarify the modes

[Miquel Raynal]

Hi Jonathan,

jic23@kernel.org wrote on Sun, 6 Feb 2022 16:09:37 +0000:

> On Wed,  2 Feb 2022 15:02:08 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > As part of a previous discussion with Jonathan Cameron [1], it appeared
> > necessary to clarify the meaning of each mode so that new developers
> > could understand better what they should use or not use and when.
> > 
> > The idea of renaming these modes as been let aside because naming is a
> > big deal and requires a lot of thinking. So for now let's focus on
> > correctly explaining what each mode implies.
> > 
> > [1] https://lore.kernel.org/linux-iio/20210930165510.2295e6c4@jic23-huawei/
> > 
> > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> Very nice.  One random comment on another area where our docs could do with
> improvements and one trivial suggested clarification for this patch.
> 
> Thanks for your hard work tidying this up!
> 
> I've looked over all the other patches in the series and where I haven't
> commented they look good to me.

Inline comments addressed, thanks for all the feedback!

Thanks,
Miquèl