Re: [PATCH V8 06/10] iio: imu: inv_icm42607: Add Buffer support for icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] inv_icm42607_buffer_set_fifo_en() writes an invalid state to the FIFO mode register when disabling the last sensor, clobbering the bypass mode.
- [High] Unaligned memory accesses when dereferencing packed struct members trigger kernel panics on strict-alignment architectures.
- [Medium] Teardown state variables are not unconditionally cleared on error paths, causing permanent buffer desynchronization.
--

commit 741f353925a2f661ebf37513ef1196056f86f563
Author: Chris Morgan <macromorgan@hotmail.com>

iio: imu: inv_icm42607: Add Buffer support for icm42607

This commit adds FIFO parsing and reading functions to support the
buffer in the inv_icm42607 driver.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> new file mode 100644
> index 0000000000000..a011f1f728b98
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c

[ ... ]

> +struct inv_icm42607_fifo_1sensor_packet {
> +	u8 header;
> +	struct inv_icm42607_fifo_sensor_data data;
> +	s8 temp;
> +} __packed;

[ ... ]

> +ssize_t inv_icm42607_fifo_decode_packet(const void *packet, const void **accel,
> +					const void **gyro, const int8_t **temp,
> +					const void **timestamp)
> +{
> +	const struct inv_icm42607_fifo_1sensor_packet *pack1 = packet;

[ ... ]

> +	/* accel only */
> +	if (header & INV_ICM42607_FIFO_HEADER_ACCEL) {
> +		*accel = &pack1->data;

Could this pointer assignment lead to an alignment fault on strict-alignment
architectures?

Because struct inv_icm42607_fifo_1sensor_packet is marked __packed and
starts with a u8, the data member sits at an odd byte offset. This unaligned
address is assigned to accel, which is later passed to
inv_icm42607_fifo_is_data_valid() where it expects a naturally aligned
struct inv_icm42607_fifo_sensor_data pointer.

Dereferencing this pointer could trigger kernel panics on architectures
that strictly enforce alignment.

> +		*gyro = NULL;
> +		*temp = &pack1->temp;
> +		*timestamp = NULL;
> +		return INV_ICM42607_FIFO_1SENSOR_PACKET_SIZE;
> +	}

[ ... ]

> +int inv_icm42607_buffer_set_fifo_en(struct inv_icm42607_state *st,
> +				    unsigned int fifo_en)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	/* update FIFO EN bits for accel and gyro */
> +	val = 0;
> +	if (fifo_en & INV_ICM42607_SENSOR_GYRO)
> +		val |= INV_ICM42607_FIFO_CONFIG1_MODE;
> +	if (fifo_en & INV_ICM42607_SENSOR_ACCEL)
> +		val |= INV_ICM42607_FIFO_CONFIG1_MODE;
> +	if (fifo_en & INV_ICM42607_SENSOR_TEMP)
> +		val |= INV_ICM42607_FIFO_CONFIG1_MODE;
> +
> +	ret = regmap_write(st->map, INV_ICM42607_REG_FIFO_CONFIG1, val);

Could this write an invalid state when disabling the last sensor?

If fifo_en is 0, val evaluates to 0 and is directly written to the FIFO
mode register. However, the correct disabled state for this register appears
to be INV_ICM42607_FIFO_CONFIG1_BYPASS.

Writing 0 might overwrite the bypass mode and leave the FIFO in an unspecified
state. It also looks like INV_ICM42607_FIFO_CONFIG1_MODE is redundantly ORed
multiple times for each active sensor.

> +	if (ret)
> +		return ret;
> +
> +	st->fifo.en = fifo_en;
> +	inv_icm42607_buffer_update_fifo_period(st);
> +
> +	return 0;
> +}

[ ... ]

> +static int inv_icm42607_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	if (st->fifo.on > 1) {
> +		st->fifo.on--;
> +		return 0;
> +	}
> +
> +	/* set FIFO in bypass mode */
> +	ret = regmap_write(st->map, INV_ICM42607_REG_FIFO_CONFIG1,
> +			   INV_ICM42607_FIFO_CONFIG1_BYPASS);
> +	if (ret)
> +		return ret;

Could returning early here leave the driver state permanently desynchronized?

If this or the subsequent operations fail, the function returns early and
st->fifo.on is never reset to 0. Since the IIO core ignores teardown
errors and considers the buffer disabled anyway, a subsequent call to
inv_icm42607_buffer_postenable() will see st->fifo.on > 0, assume the
hardware is already fully initialized, and skip programming the hardware.

This would leave the hardware permanently disabled while the software thinks
it is streaming, leading to silent data loss.

> +
> +	/* flush FIFO data */
> +	ret = regmap_write(st->map, INV_ICM42607_REG_SIGNAL_PATH_RESET,
> +			   INV_ICM42607_SIGNAL_PATH_RESET_FIFO_FLUSH);
> +	if (ret)
> +		return ret;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518200526.458421-1-macroalpha82@gmail.com?part=6