Re: [PATCH V6 10/11] iio: imu: inv_icm42607: Add Gyroscope to icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Uninitialized variable 'size' and inverted logic in FIFO parsing loop condition.
- [High] Self-deadlock due to double acquisition of `st->lock` in hardware FIFO flush.
- [High] Race condition in device initialization leads to NULL pointer dereference in IRQ handler.
- [Medium] Gyroscope remains indefinitely powered on in software state after a raw read.
- [Medium] The gyroscope IIO device does not implement the `read_avail` API.
--

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

iio: imu: inv_icm42607: Add Gyroscope to icm42607

Add gyroscope functions to the icm42607 driver.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index a69cd0a61050e..c8545dd68cdb0 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
[ ... ]
> @@ -435,6 +478,7 @@ static irqreturn_t inv_icm42607_irq_timestamp(int irq, void *_data)
>  {
>  	struct inv_icm42607_state *st = _data;
>  
> +	st->timestamp.gyro = iio_get_time_ns(st->indio_gyro);
>  	st->timestamp.accel = iio_get_time_ns(st->indio_accel);

Could this result in a NULL pointer dereference?

In inv_icm42607_core_probe(), the interrupt is initialized and the accelerometer
device is registered and exposed to userspace before st->indio_gyro is
allocated:

	st->indio_accel = inv_icm42607_accel_init(st);
	if (IS_ERR(st->indio_accel))
		...
	st->indio_gyro = inv_icm42607_gyro_init(st);

If userspace enables the accelerometer immediately, the interrupt could fire
while st->indio_gyro is still NULL, causing iio_get_time_ns() to dereference it.

[ ... ]
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c
> new file mode 100644
> index 0000000000000..529929a5349f9
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_gyro.c
[ ... ]
> +static int inv_icm42607_gyro_read_sensor(struct inv_icm42607_state *st,
> +					 struct iio_chan_spec const *chan,
> +					 s16 *val)
> +{
[ ... ]
> +	/* enable gyro sensor */
> +	conf.mode = INV_ICM42607_SENSOR_MODE_LOW_NOISE;
> +	ret = inv_icm42607_set_gyro_conf(st, &conf, NULL);
> +	if (ret)
> +		return ret;
> +
> +	/* read gyro register data */
> +	data = &st->buffer[0];
> +	ret = regmap_bulk_read(st->map, reg, data, sizeof(*data));
> +	if (ret)
> +		return ret;
> +
> +	*val = be16_to_cpup(data);
> +	if (*val == INV_ICM42607_DATA_INVALID)
> +		return -EINVAL;
> +
> +	return 0;
> +}

Should the gyroscope be powered down before returning?

The device is explicitly powered up to INV_ICM42607_SENSOR_MODE_LOW_NOISE,
but it is never restored to INV_ICM42607_SENSOR_MODE_OFF. While
pm_runtime_put_autosuspend() is used, if the accelerometer is running
concurrently, the device usage count won't reach zero and the gyroscope
will remain powered on indefinitely.

[ ... ]
> +static int inv_icm42607_gyro_hwfifo_flush(struct iio_dev *indio_dev,
> +					  unsigned int count)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	int ret;
> +
> +	if (count == 0)
> +		return 0;
> +
> +	/* Make sure we don't try to count more than the buffer can hold. */
> +	count = min(count, sizeof(st->fifo.data));
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = inv_icm42607_buffer_hwfifo_flush(st, count);

Can this lead to a self-deadlock?

The st->lock mutex is acquired here using guard(mutex)(&st->lock),
but inv_icm42607_buffer_hwfifo_flush() subsequently calls
inv_icm42607_buffer_fifo_read(), which unconditionally attempts to
acquire the exact same st->lock.

[ ... ]
> +static const struct iio_info inv_icm42607_gyro_info = {
> +	.read_raw = inv_icm42607_gyro_read_raw,
> +	.write_raw = inv_icm42607_gyro_write_raw,
> +	.write_raw_get_fmt = inv_icm42607_gyro_write_raw_get_fmt,
> +	.update_scan_mode = inv_icm42607_gyro_update_scan_mode,
> +	.hwfifo_set_watermark = inv_icm42607_gyro_hwfifo_set_watermark,
> +	.hwfifo_flush_to_buffer = inv_icm42607_gyro_hwfifo_flush,
> +};

Is the .read_avail callback intentionally missing?

Without it, the driver does not expose supported sampling frequencies
and scale factors to userspace.

[ ... ]
> +int inv_icm42607_gyro_parse_fifo(struct iio_dev *indio_dev)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	struct inv_icm42607_sensor_state *gyro_st = iio_priv(indio_dev);
> +	struct inv_sensors_timestamp *ts = &gyro_st->ts;
> +	ssize_t i, size;
> +	unsigned int no;
> +	const void *accel, *gyro, *timestamp;
> +	const s8 *temp;
> +	unsigned int odr;
> +	s64 ts_val;
> +	struct inv_icm42607_gyro_buffer buffer = { };
> +
> +	/* parse all fifo packets */
> +	for (i = 0, no = 0; i < st->fifo.count && i + size > st->fifo.count;
> +	     i += size, ++no) {
> +		size = inv_icm42607_fifo_decode_packet(&st->fifo.data[i],

Will this evaluate the loop condition with an uninitialized size variable?

The variable size is declared but uninitialized, so the expression
i + size > st->fifo.count will use an indeterminate value on the first
iteration.

Additionally, should this check verify that the packet fits within
the buffer (e.g. <=) rather than exceeding it (>)?

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