Re: [PATCH V5 09/11] iio: imu: inv_icm42607: Add Wake on Movement to icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Self-deadlock in inv_icm42607_accel_write_event_config() due to nested mutex locking.
- [High] ABBA deadlock in inv_icm42607_suspend() between the driver mutex and disable_irq().
- [High] Data corruption and uninitialized memory access due to using a __be16 array for 8-bit regmap I/O.
- [High] TOCTOU race condition in inv_icm42607_accel_write_event_config() causes PM usage counter imbalances.
- [Medium] Integer overflow in Wake-on-Motion threshold calculation.
- [Medium] IIO device is registered before its interrupt handler is fully initialized.
--

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

iio: imu: inv_icm42607: Add Wake on Movement to icm42607

Add support for wake on movement for the icm42607 driver.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
> index f90ba46bd7e48..99aaa370bd124 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
[ ... ]
> @@ -209,6 +221,178 @@ static int inv_icm42607_accel_read_sensor(struct iio_dev *indio_dev,
[ ... ]
> +static unsigned int inv_icm42607_accel_convert_roc_to_wom(u64 roc,
> +							  int accel_hz, int accel_uhz)
> +{
[ ... ]
> +	freq_uhz = (u64)accel_hz * MICRO + (u64)accel_uhz;
> +	value = div64_u64(roc * MICRO, freq_uhz * (u64)convert);

Is there a risk of integer overflow when multiplying roc by MICRO?

If the user-supplied rate of change exceeds approximately 18,446, roc * MICRO
will exceed U64_MAX. Could this wrap-around lead to applying a miscalculated
threshold to the sensor?

[ ... ]
> +static int inv_icm42607_accel_set_wom_threshold(struct inv_icm42607_state *st,
> +						u64 value,
> +						int accel_hz, int accel_uhz)
> +{
[ ... ]
> +	/* set accel WoM threshold for the 3 axes */
> +	st->buffer[0] = threshold;
> +	st->buffer[1] = threshold;
> +	st->buffer[2] = threshold;
> +	ret = regmap_bulk_write(st->map, INV_ICM42607_REG_ACCEL_WOM_X_THR, st->buffer, 3);

Can this write corrupted data? 

st->buffer is declared as __be16 buffer[3]. Assigning an 8-bit threshold
to st->buffer[0], st->buffer[1], and st->buffer[2] writes 16-bit values.

A 3-byte regmap_bulk_write() then reads only the first 3 bytes from memory,
which writes the first byte of st->buffer[0], the second byte of
st->buffer[0], and the first byte of st->buffer[1]. Could this write 0 and
misaligned data to the WoM threshold registers?

[ ... ]
> +static int inv_icm42607_accel_write_event_config(struct iio_dev *indio_dev,
> +						 const struct iio_chan_spec *chan,
> +						 enum iio_event_type type,
> +						 enum iio_event_direction dir,
> +						 bool state)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +
> +	/* handle only WoM (roc rising) event */
> +	if (type != IIO_EV_TYPE_ROC || dir != IIO_EV_DIR_RISING)
> +		return -EINVAL;
> +
> +	scoped_guard(mutex, &st->lock) {
> +		if (st->apex.wom.enable == state)
> +			return 0;
> +
> +	if (state)
> +		return inv_icm42607_accel_enable_wom(indio_dev);

Will this sequence cause a self-deadlock?

This function acquires st->lock via scoped_guard. It then calls
inv_icm42607_accel_enable_wom(), which calls pm_runtime_resume_and_get().

If the device is suspended, this synchronously invokes inv_icm42607_resume(),
which also attempts to acquire st->lock. Additionally,
_inv_icm42607_accel_enable_wom() explicitly acquires st->lock again.

Since kernel mutexes are not recursive, does this guarantee a deadlock?

> +	}
> +
> +	return inv_icm42607_accel_disable_wom(indio_dev);

Does this drop the lock before disabling WoM, opening a race condition?

If multiple threads write 0 to the sysfs node simultaneously, they could all
pass the st->apex.wom.enable == state check inside the lock, then
sequentially call inv_icm42607_accel_disable_wom() outside the lock.

Could this cause unbalanced pm_runtime_put_autosuspend() calls and
underflow st->apex.on?

> +}
> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index ce59f9337ed9b..e5d031ca631d6 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
[ ... ]
> @@ -343,6 +375,112 @@ static int inv_icm42607_setup(struct inv_icm42607_state *st,
[ ... ]
> +static irqreturn_t inv_icm42607_irq_handler(int irq, void *_data)
> +{
[ ... ]
> +	if (st->apex.on) {
> +		unsigned int status2, status3;
> +
> +		/* read INT_STATUS2 and INT_STATUS3 in 1 operation */
> +		ret = regmap_bulk_read(st->map, INV_ICM42607_REG_INT_STATUS2, st->buffer, 2);
> +		if (ret) {
> +			dev_err(dev, "Interrupt status read error %d\n", ret);
> +			return IRQ_HANDLED;
> +		}
> +
> +		status2 = st->buffer[0];
> +		status3 = st->buffer[1];

Can this read uninitialized memory? 

st->buffer is declared as __be16 buffer[3], so its elements are 16 bits.
A 2-byte regmap_bulk_read() populates only the first two bytes of the array.

Assigning status2 = st->buffer[0] reads both bytes as a single 16-bit integer,
and status3 = st->buffer[1] reads bytes 2 and 3, which contain uninitialized
memory.

[ ... ]
> @@ -437,6 +580,11 @@ int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw
>  	if (IS_ERR(st->indio_accel))
>  		return PTR_ERR(st->indio_accel);
>  
> +	/* Initialize interrupt handling */
> +	ret = inv_icm42607_irq_init(st, irq, irq_type, open_drain);
> +	if (ret)
> +		return ret;
> +
>  	return 0;

Is the IIO device registered before its interrupt handler is fully initialized?

st->indio_accel is initialized, which calls devm_iio_device_register(), before
inv_icm42607_irq_init() is called.

Could userspace open the device and enable events or buffers before the
hardware interrupt is ready?

[ ... ]
> @@ -448,6 +596,9 @@ EXPORT_SYMBOL_NS_GPL(inv_icm42607_core_probe, "IIO_ICM42607");
>  static int inv_icm42607_suspend(struct device *dev)
>  {
>  	struct inv_icm42607_state *st = dev_get_drvdata(dev);
[ ... ]
>  	guard(mutex)(&st->lock);
[ ... ]
> -	ret = inv_icm42607_set_pwr_mgmt0(st, INV_ICM42607_SENSOR_MODE_OFF,
> -					 INV_ICM42607_SENSOR_MODE_OFF,
> -					 false, NULL);
> -	if (ret)
> -		return ret;
> -	regulator_disable(st->vddio_supply);
> +	/* keep chip on and wake-up capable if APEX and wakeup on */
> +	accel_dev = &st->indio_accel->dev;
> +	wakeup = st->apex.on && device_may_wakeup(accel_dev);
> +	if (wakeup) {
> +		/* keep accel on and setup irq for wakeup */
> +		accel_conf = st->conf.accel.mode;
> +		enable_irq_wake(st->irq);
> +		disable_irq(st->irq);

Could this cause a deadlock between the driver mutex and the interrupt handler?

inv_icm42607_suspend() takes st->lock via guard(mutex) and then calls
disable_irq().

Since disable_irq() waits for any currently executing interrupt handlers
to finish, and the threaded handler inv_icm42607_irq_handler() also attempts
to acquire st->lock, will this deadlock if the handler is running during
suspend?

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