Re: [PATCH V7 06/11] iio: imu: inv_icm42607: Add Buffer support for icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Missing bounds check in `inv_icm42607_buffer_fifo_read` allows out-of-bounds slab memory read.
- [High] Unaligned memory access in `inv_icm42607_fifo_is_data_valid()` causes undefined behavior and alignment faults.
- [High] Software state desynchronization in `inv_icm42607_buffer_predisable` due to missing teardown on error paths.
- [Medium] Redundant and potentially destructive assignment logic in `inv_icm42607_buffer_set_fifo_en` overwrites FIFO configuration.
- [Medium] Dead code for sleep delay in `inv_icm42607_buffer_postdisable` bypasses required hardware power-down delays.
- [Low] The macro `INV_ICM42607_SENSOR_CONF_INIT` initializes more elements than the struct contains.
--

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

iio: imu: inv_icm42607: Add Buffer support for icm42607

This commit adds all FIFO parsing and reading functions to support the
inv_icm42607 hardware.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607.h b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
[ ... ]
> @@ -84,6 +88,7 @@ struct inv_icm42607_sensor_conf {
>  	int odr;
>  	int filter;
>  };
> +#define INV_ICM42607_SENSOR_CONF_INIT		{-1, -1, -1, -1}

This isn't a bug, but struct inv_icm42607_sensor_conf is defined with exactly
three fields. Will initializing it with four values cause excess elements in
struct initializer compiler warnings?

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
[ ... ]
> +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);

Does writing 0 to the FIFO_CONFIG1 register here when fifo_en is empty
inadvertently overwrite the INV_ICM42607_FIFO_CONFIG1_BYPASS mode set
previously in inv_icm42607_buffer_predisable()?

Also, does setting the exact same mask (INV_ICM42607_FIFO_CONFIG1_MODE) for
all three sensors properly configure the hardware, or are distinct bits
expected here?

[ ... ]
> +static int inv_icm42607_buffer_predisable(struct iio_dev *indio_dev)
> +{
[ ... ]
> +	/* set FIFO in bypass mode */
> +	ret = regmap_write(st->map, INV_ICM42607_REG_FIFO_CONFIG1,
> +			   INV_ICM42607_FIFO_CONFIG1_BYPASS);
> +	if (ret)
> +		return ret;
> +
> +	/* 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;
> +
> +	/* disable FIFO threshold interrupt */
> +	ret = regmap_update_bits(st->map, INV_ICM42607_REG_INT_SOURCE0,
> +				 INV_ICM42607_INT_SOURCE0_FIFO_THS_INT1_EN, 0);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * FIFO should only be 1 at this point, so explicitly set it to 0
> +	 * instead of decrementing it.
> +	 */
> +	st->fifo.on = 0;
> +
> +	return 0;
> +}

If any of these regmap operations fail and return early, st->fifo.on remains
greater than 0. Since the IIO core continues the buffer teardown process even
if predisable returns an error, does this leave the software state permanently
desynchronized? 

If so, future calls to inv_icm42607_buffer_postenable() might see st->fifo.on
as non-zero and silently skip initialization, leaving the device unresponsive
until the module is reloaded.

[ ... ]
> +static int inv_icm42607_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	struct device *dev = regmap_get_device(st->map);
> +	unsigned int sensor;
> +	unsigned int *watermark;
> +	unsigned int sleep_temp = 0;
> +	unsigned int sleep_sensor = 0;
> +	unsigned int sleep;
> +	int ret;
[ ... ]
> +	/* sleep maximum required time */
> +	sleep = max(sleep_sensor, sleep_temp);
> +	if (sleep)
> +		msleep(sleep);

Is there missing logic to update sleep_sensor and sleep_temp?

Since they are initialized to 0 and never changed, this sleep will never
occur. Could bypassing this hardware-specific power-down delay lead to issues
when rapidly toggling power states?

[ ... ]
> +int inv_icm42607_buffer_fifo_read(struct inv_icm42607_state *st,
> +				  unsigned int max)
> +{
[ ... ]
> +	/* compute number of samples for each sensor */
> +	for (i = 0; i < st->fifo.count; i += size) {
> +		size = inv_icm42607_fifo_decode_packet(&st->fifo.data[i],
> +				&accel, &gyro, &temp, &timestamp);
> +		/* Make sure the size is at least 1 valid packet. */
> +		if (size < INV_ICM42607_FIFO_1SENSOR_PACKET_SIZE)
> +			break;
> +		if (gyro != NULL && inv_icm42607_fifo_is_data_valid(gyro))
> +			st->fifo.nb.gyro++;
> +		if (accel != NULL && inv_icm42607_fifo_is_data_valid(accel))
> +			st->fifo.nb.accel++;
> +		st->fifo.nb.total++;
> +	}

Can an incomplete partial packet at the end of the read result in size
extending past the valid populated data? 

There is no validation to ensure that i + size <= st->fifo.count. If
st->fifo.count is near the maximum array size, could i + size exceed the
bounds of the st->fifo.data array, leading to an out-of-bounds memory read
in inv_icm42607_fifo_is_data_valid()?

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.h b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.h
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.h
[ ... ]
> +/* FIFO data packet */
> +struct inv_icm42607_fifo_sensor_data {
> +	__be16 x;
> +	__be16 y;
> +	__be16 z;
> +};
> +
> +#define INV_ICM42607_FIFO_DATA_INVALID		-32768
> +
> +static inline bool
> +inv_icm42607_fifo_is_data_valid(const struct inv_icm42607_fifo_sensor_data *s)
> +{
> +	s16 x, y, z;
> +
> +	x = be16_to_cpu(s->x);

Because the first field of the packet layout struct
(inv_icm42607_fifo_2sensors_packet) is a 1-byte header, the accel and gyro
fields reside at odd byte offsets.

Since struct inv_icm42607_fifo_sensor_data is not marked as __packed, the
compiler assumes 2-byte alignment for its __be16 fields. Will passing these
unaligned pointers into inv_icm42607_fifo_is_data_valid() trigger undefined
behavior and alignment faults on architectures that enforce strict alignment?

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