Re: [PATCH V9 08/11] iio: imu: inv_icm42607: Add Accelerometer for icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Concurrency bug and data race in FIFO flush and parsing sequence due to split locking.
- [High] Self-deadlock in system PM sleep callbacks due to recursive mutex acquisition.
- [High] Unaligned memory access exception caused by casting a misaligned packed structure member.
--

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

iio: imu: inv_icm42607: Add Accelerometer for icm42607

Add icm42607 accelerometer sensor for icm42607.

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

[ ... ]

> +int inv_icm42607_accel_parse_fifo(struct iio_dev *indio_dev)
> +{

[ ... ]

> +	/* parse all fifo packets */
> +	for (i = 0, no = 0; i < st->fifo.count; i += size, ++no) {
> +		size = inv_icm42607_fifo_decode_packet(&st->fifo.data[i],
> +				&accel, &gyro, &temp, &timestamp, &odr);
> +		/* quit if error or FIFO is empty */
> +		if (size <= 0)
> +			return size;
> +
> +		/* If the packet size could cause us to overflow, return. */
> +		if (i + size > st->fifo.count)
> +			return -EIO;
> +
> +		/* skip packet if no accel data or data is invalid */
> +		if (accel == NULL || !inv_icm42607_fifo_is_data_valid(accel))

[Severity: High]
Will this cause an unaligned memory access trap on architectures with
strict alignment requirements?

The accel pointer is populated by inv_icm42607_fifo_decode_packet() which
returns the address of pack1->data. Because pack1 is a packed structure
with a 1-byte header, the pack1->data pointer sits at an odd address.

When inv_icm42607_fifo_is_data_valid() casts this misaligned pointer to a
non-packed structure and accesses 16-bit members via be16_to_cpu(), it
could trigger a hardware fault.

> +			continue;

[ ... ]

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> index e065d60ac1197..5b69bf895b350 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c

[ ... ]

>  int inv_icm42607_buffer_hwfifo_flush(struct inv_icm42607_state *st,
>  				     unsigned int count)
>  {
> +	struct inv_icm42607_sensor_state *accel_st = iio_priv(st->indio_accel);
> +	struct inv_sensors_timestamp *ts;
> +	s64 accel_ts;
>  	int ret;
>  
> +	accel_ts = iio_get_time_ns(st->indio_accel);
> +
>  	ret = inv_icm42607_buffer_fifo_read(st, count);
> +	if (ret)
> +		return ret;
>  
> -	return ret;
> +	if (st->fifo.nb.total == 0)
> +		return 0;
> +
> +	if (st->fifo.nb.accel > 0) {
> +		ts = &accel_st->ts;
> +		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel, accel_ts);

[Severity: High]
Is it safe to access st->fifo.nb and update the timestamp locklessly here?

The preceding call to inv_icm42607_buffer_fifo_read() acquires and then
releases st->lock via a scoped guard before returning. Because the lock
is dropped, a concurrent sysfs hwfifo_flush or IRQ handler could interleave
here and overwrite st->fifo.data and st->fifo.nb before they are parsed,
leading to a data race and corrupted timestamp calculations.

> +		ret = inv_icm42607_accel_parse_fifo(st->indio_accel);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
>  }

[ ... ]

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index cf9d0c6f3f4b8..6b623fb679f34 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c

[ ... ]

> @@ -383,6 +444,8 @@ static int inv_icm42607_suspend(struct device *dev)
>  
>  	guard(mutex)(&st->lock);
>  
> +	st->suspended.accel = st->conf.accel.mode;
> +	st->suspended.temp = st->conf.temp_en;
>  	if (pm_runtime_suspended(dev))
>  		return 0;

[Severity: High]
Could this lead to a self-deadlock?

We acquire st->lock via the scoped guard at the top of 
inv_icm42607_suspend(). Shortly after this snippet, the function calls 
pm_runtime_force_suspend() which synchronously executes the driver's 
runtime PM suspend callback.

If the runtime suspend callback also attempts to acquire st->lock, it will
deadlock since the mutex is not recursive. The same locking structure is
present in inv_icm42607_resume() with pm_runtime_force_resume().

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