Re: [PATCH v2 3/3] iio: accel: icm42370: Add FIFO buffer functionality

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Missing `available_scan_masks` causes IIO buffer data corruption when channels are disabled.
- [High] Incorrect endianness parsing and scaling of the hardware `FIFO_COUNT` register.
- [High] Enabling only the TEMP channel causes the IIO buffer to hang indefinitely.
- [High] Hardware registers are accessed without acquiring PM runtime in sysfs and IIO core callbacks.
- [Medium] Mixing `goto` error handling with `guard(mutex)` causes device lock to be held during sleeps.
- [Medium] The internal FIFO state tracker `data->fifo.on` leaks when hardware configuration fails.
- [Medium] The sensor hardware is left powered on indefinitely if configuration steps fail during buffer state transitions.
- [Medium] The FIFO watermark interrupt is permanently disabled if updating the watermark register fails.
--

--- Patch [3]: [PATCH v2 3/3] iio: accel: icm42370: Add FIFO buffer functionality ---
commit 04755a7b27e94b2f885caf4cb6fed3815002934a
Author: Kanak Shilledar <kanak.shilledar@axis.com>
Subject: iio: accel: icm42370: Add FIFO buffer functionality

Add support for the hardware FIFO buffer to support high data rate capture.
This includes necessary IIO buffer setup ops, watermark management and FIFO
packet decoding. The accelerometer has 2.25kB FIFO size.

> diff --git a/drivers/iio/accel/inv_icm42370_buffer.c b/drivers/iio/accel/inv_icm42370_buffer.c
> new file mode 100644
> index 0000000000000..c6a4d313c92c5
> --- /dev/null
> +++ b/drivers/iio/accel/inv_icm42370_buffer.c
[ ... ]
> +int inv_icm42370_buffer_update_watermark(struct inv_icm42370_data *data)
> +{
[ ... ]
> +	/* changing FIFO watermark requires to turn off watermark interrupt */
> +	ret = regmap_update_bits_check(
> +		data->map, INV_ICM42370_REG_INT_SOURCE0,
> +		INV_ICM42370_INT_SOURCE0_FIFO_THS_INT1_EN, 0, &restore);
> +	if (ret)
> +		return ret;
> +
> +	raw_wm = INV_ICM42370_FIFO_WATERMARK_VAL(wm_size);
> +	memcpy(data->buffer, &raw_wm, sizeof(raw_wm));
> +	ret = regmap_bulk_write(data->map, INV_ICM42370_REG_FIFO_WATERMARK,
> +				data->buffer, sizeof(raw_wm));
> +	if (ret)
> +		return ret;
        ^^^^^^^^^^
[Severity: Medium]
Will this return leave the watermark interrupt permanently disabled? If
regmap_bulk_write() fails, the function exits before the restore block is
reached, which might prevent future watermark interrupts.

> +
> +	/* restore watermark interrupt */
> +	if (restore) {
> +		ret = regmap_set_bits(
> +			data->map, INV_ICM42370_REG_INT_SOURCE0,
> +			INV_ICM42370_INT_SOURCE0_FIFO_THS_INT1_EN);
[ ... ]
> +static int inv_icm42370_buffer_postenable(struct iio_dev *indio_dev)
> +{
[ ... ]
> +	/* when FIFO_CONFIG_STREAM bit is set FIFO is enabled, so
> +	 * increase the count
> +	 */
> +	data->fifo.on++;
> +
> +	ret = regmap_bulk_read(data->map, INV_ICM42370_REG_FIFO_COUNT, data->buffer,
> +			       2);
> +	if (ret)
> +		return ret;
        ^^^^^^^^^^
[Severity: Medium]
Does this error path leak the internal FIFO state tracker? The
data->fifo.on counter is incremented, but subsequent failures exit the
function without rolling it back. This might lead to silent failures when
enabling data capture later.

> +
> +	ret = regmap_set_bits(data->map, INV_ICM42370_REG_INT_SOURCE0,
> +			      INV_ICM42370_INT_SOURCE0_FIFO_THS_INT1_EN);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
[ ... ]
> +static int inv_icm42370_buffer_postdisable(struct iio_dev *indio_dev)
> +{
[ ... ]
> +	guard(mutex)(&data->lock);
> +
> +	inv_sensors_timestamp_apply_odr(ts, 0, 0, 0);
> +
> +	ret = inv_icm42370_buffer_set_fifo_en(data, data->fifo.en & ~sensor);
> +	if (ret)
> +		goto out_unlock;
> +
> +	*watermark = 0;
> +	ret = inv_icm42370_buffer_update_watermark(data);
> +	if (ret)
> +		goto out_unlock;
        ^^^^^^^^^^^^^^
[Severity: Medium]
If the watermark update fails, could this leave the sensor hardware powered
on indefinitely? The failure skips the INV_ICM42370_SENSOR_MODE_OFF
configuration below, which might cause a power leak.

> +
> +	conf.mode = INV_ICM42370_SENSOR_MODE_OFF;
> +	ret = inv_icm42370_set_accel_conf(data, &conf, &sleep_sensor);
> +	if (ret)
> +		goto out_unlock;
> +
> +out_unlock:
> +	/* sleep maximum required time */
> +	sleep = max(sleep_sensor, sleep_temp);
> +	if (sleep)
> +		msleep(sleep);
> +
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return ret;
        ^^^^^^^^^^
[Severity: Medium]
Are we holding the mutex during the sleep and autosuspend operations? Since
guard() is used for locking, but error handling relies on goto out_unlock,
the lock is not released at the label but held until the function exits.

> +}
[ ... ]
> +int inv_icm42370_buffer_fifo_read(struct inv_icm42370_data *data,
> +				  unsigned int max)
> +{
[ ... ]
> +	/* Check and limit number of samples if requested. */
> +	fifo_nb = le16_to_cpup(raw_fifo_count);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Since the FIFO_COUNT is configured to use Big Endian in buffer_init(), is it
correct to parse it using le16_to_cpup() here? This might cause a massive
miscalculation of the count on little-endian systems.

> +	if (fifo_nb == 0)
> +		return 0;
> +	if (max > 0 && fifo_nb > max)
> +		fifo_nb = max;
> +
> +	/*
> +	 * Read all FIFO data into the internal buffer, clamping the
> +	 * device-reported count to the buffer capacity.
> +	 */
> +	data->fifo.count = min(fifo_nb * packet_size, INV_ICM42370_FIFO_SIZE_MAX);
                               ^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Does this inadvertently inflate the read length? If the hardware count is
configured to report bytes instead of samples, multiplying it by packet_size
might result in reading vastly more bytes than the FIFO contains.

> +	ret = regmap_noinc_read(data->map, INV_ICM42370_REG_FIFO_DATA,
> +				data->fifo.data, data->fifo.count);
[ ... ]
> +int inv_icm42370_buffer_init(struct inv_icm42370_data *data)
> +{
[ ... ]
> +	/*
> +	 * Default FIFO configuration (bits 6 to 5)
> +	 * - FIFO count in bytes
> +	 * - FIFO count in big endian
> +	 */
> +	val = INV_ICM42370_INTF_CONFIG0_FIFO_COUNT_ENDIAN;
> +	ret = regmap_update_bits(data->map, INV_ICM42370_REG_INTF_CONFIG0,
> +				 GENMASK(6, 5), val);
[ ... ]
> diff --git a/drivers/iio/accel/inv_icm42370_core.c b/drivers/iio/accel/inv_icm42370_core.c
> index 6266362e83f6a..b1713a2f533c8 100644
> --- a/drivers/iio/accel/inv_icm42370_core.c
> +++ b/drivers/iio/accel/inv_icm42370_core.c
[ ... ]
> +/* enable accelerometer sensor and FIFO write */
> +static int inv_icm42370_accel_update_scan_mode(struct iio_dev *indio_dev,
> +					       const unsigned long *scan_mask)
> +{
> +	struct inv_icm42370_data *st = iio_priv(indio_dev);
> +	struct inv_icm42370_conf conf = INV_ICM42370_SENSOR_CONF_INIT;
> +	unsigned int fifo_en = 0;
> +	unsigned int sleep_accel = 0;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
        ^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Is it possible this function gets called when the device is asleep? The IIO
core might invoke update_scan_mode before preenable. Missing a
pm_runtime_resume_and_get() here could lead to hardware accesses on a
suspended device.

> +
> +	if (*scan_mask & INV_ICM42370_SCAN_MASK_ACCEL_3AXIS) {
> +		/* enable accel sensor */
> +		conf.mode = st->conf.mode;
> +		conf.filter = st->filter;
> +
> +		ret = inv_icm42370_set_accel_conf(st, &conf, &sleep_accel);
> +		if (ret)
> +			goto out;
> +
> +		fifo_en |= INV_ICM42370_SENSOR_ACCEL;
> +	}
> +
> +	/* update data FIFO write */
> +	ret = inv_icm42370_buffer_set_fifo_en(st, fifo_en | st->fifo.en);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
What happens if userspace requests only the TEMP channel? In that case,
fifo_en evaluates to 0, meaning the sensor isn't powered on and the FIFO
won't fill. This might cause the IIO buffer to hang indefinitely since
watermark interrupts would never be generated.

[Severity: Medium]
Also, if inv_icm42370_buffer_set_fifo_en() fails here, the function returns
without rolling back the sensor to its OFF state. Could this leave the sensor
hardware powered on indefinitely and drain power?

> +
> +out:
> +	if (sleep_accel)
> +		msleep(sleep_accel);
> +
> +	return ret;
        ^^^^^^^^^^
[Severity: Medium]
Similar to the postdisable function, does using goto out with guard() cause
the mutex to be held during the msleep() call, stalling concurrent driver
operations?

> +}
[ ... ]
> +static int inv_icm42370_accel_hwfifo_set_watermark(struct iio_dev *indio_dev,
> +						   unsigned int val)
> +{
> +	struct inv_icm42370_data *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	st->fifo.watermark.accel = val;
> +	ret = inv_icm42370_buffer_update_watermark(st);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Does this interact with hardware registers without acquiring PM runtime? As a
sysfs callback, this might be executed while the device is in auto-suspend,
causing writes to a sleeping device.

> +
> +	return ret;
> +}
> +
> +static int inv_icm42370_accel_hwfifo_flush(struct iio_dev *indio_dev,
> +					   unsigned int count)
> +{
> +	struct inv_icm42370_data *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (count == 0)
> +		return 0;
> +
> +	mutex_lock(&st->lock);
> +
> +	ret = inv_icm42370_buffer_hwfifo_flush(st, count);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Is it possible this flush sequence executes on a suspended device? There
doesn't appear to be a pm_runtime lock here before accessing the hardware
registers.

> +	if (!ret)
> +		ret = st->fifo.nb.accel;
> +
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
[ ... ]
>  struct iio_dev *inv_icm42370_accel_init(struct iio_dev *indio_dev,
>  					struct inv_icm42370_data *data)
>  {
[ ... ]
>  	indio_dev->name = "inv_icm42370";
>  	indio_dev->info = &inv_icm42370_info;
> -	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_ALL_BUFFER_MODES;
>  	indio_dev->channels = inv_icm42370_accel_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(inv_icm42370_accel_channels);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Could the absence of indio_dev->available_scan_masks here cause data
corruption? Without it, if a user enables only a subset of channels, the IIO
core will expect the buffer to be packed densely.

>  
> +	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> +					  &inv_icm42370_buffer_ops);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>  	return indio_dev;
>  }
>  
> +int inv_icm42370_accel_parse_fifo(struct iio_dev *indio_dev)
> +{
[ ... ]
> +		if (temp)
> +			buffer.temp = (s16)*temp * 64;
> +		else
> +			buffer.temp = (s16)INV_ICM42370_DATA_INVALID;
> +
> +		ts_val = inv_sensors_timestamp_pop(ts);
> +		iio_push_to_buffers_with_timestamp(indio_dev, &buffer, ts_val);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Because the driver unconditionally pushes a fixed-layout buffer without manual
repacking, if available_scan_masks is missing, the IIO core will read the
Y-axis bytes as Z-axis data when certain channels are disabled. Is there a
plan to provide the available scan masks?

> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813-b4-inv_icm42370p-v2-0-11aedfdf76d3@axis.com?part=3