Re: [PATCH V7 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 8 potential issue(s) to consider:
- [High] TOCTOU race in event configuration leaks PM references and leaves sensor permanently powered on.
- [High] Disabling the IIO buffer unconditionally powers off the accelerometer, breaking Wake-on-Motion.
- [High] Deadlock avoidance in `inv_icm42607_suspend()` allows the threaded IRQ handler to access a suspended bus.
- [High] I2C failure in `inv_icm42607_mreg_write()` leaves the device trapped in the wrong register bank.
- [Medium] Error path in WoM enablement leaves the accelerometer's software state cached as ON, causing a permanent power leak.
- [Medium] Error path in ODR write desynchronizes the FIFO watermark and timing state from the hardware.
- [Medium] The driver leaks a runtime PM usage count if unbound while the Wake-on-Motion event is active.
- [Low] The driver silently ignores WoM threshold configurations for the ICM42607P variant while returning success to userspace.
--

commit 38592461f791657cf708b555278a27f572c997e7
Author: Chris Morgan <macromorgan@hotmail.com>
Subject: 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 b354a84fd2ed4..504245b0282bf 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c

[ ... ]

> @@ -209,6 +221,181 @@ static int inv_icm42607_accel_read_sensor(struct iio_dev *indio_dev,
>  	return 0;
>  }
>  
> +static unsigned int inv_icm42607_accel_convert_roc_to_wom(u64 roc,
> +							  int accel_hz, int accel_uhz)

[ ... ]

> +static int inv_icm42607_accel_set_wom_threshold(struct inv_icm42607_state *st,
> +						u64 value,
> +						int accel_hz, int accel_uhz)
> +{
> +	unsigned int threshold;
> +	int i, ret;
> +
> +	/* convert roc to wom threshold and convert back to handle clipping */
> +	threshold = inv_icm42607_accel_convert_roc_to_wom(value, accel_hz, accel_uhz);
> +	value = inv_icm42607_accel_convert_wom_to_roc(threshold, accel_hz, accel_uhz);
> +
> +	dev_dbg(regmap_get_device(st->map), "wom_threshold: 0x%x\n", threshold);
> +
> +	st->apex.wom.value = value;
> +
> +	/* 42607P cannot set WoM threshold. */
> +	if (st->hw->whoami == INV_ICM42607P_WHOAMI)
> +		return 0;

Should this return -EOPNOTSUPP instead of 0? Returning 0 updates the cached
value in st->apex.wom.value and signals success to userspace, even though the
hardware configuration isn't changed for the 42607P variant.

> +
> +	/* set accel WoM threshold for the 3 axes */
> +	for (i = 0; i < 3; i++) {
> +		ret = inv_icm42607_mreg_write(st->map, INV_ICM42607_MREG_2,
> +					      INV_ICM42607_REG_ACCEL_WOM_X_THR + i,
> +					      threshold);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int _inv_icm42607_accel_enable_wom(struct iio_dev *indio_dev)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	struct inv_icm42607_sensor_state *accel_st = iio_priv(indio_dev);
> +	struct inv_icm42607_sensor_conf conf = INV_ICM42607_SENSOR_CONF_INIT;
> +	unsigned int sleep_ms = 0;
> +	int ret;
> +
> +	scoped_guard(mutex, &st->lock) {
> +		/* turn on accel sensor */
> +		conf.mode = accel_st->power_mode;
> +		conf.filter = accel_st->filter;
> +		ret = inv_icm42607_set_accel_conf(st, &conf, &sleep_ms);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (sleep_ms)
> +		msleep(sleep_ms);
> +
> +	scoped_guard(mutex, &st->lock) {
> +		ret = inv_icm42607_enable_wom(st);
> +		if (ret)
> +			return ret;

If inv_icm42607_enable_wom() fails here, does the accelerometer remain powered
on? The earlier call to inv_icm42607_set_accel_conf() enabled it, but an
early return leaves the hardware in the ON state without reverting
st->conf.accel.mode to OFF. 

During system or runtime suspend, this ON state would be cached in
st->suspended.accel, and on all future resumes, the accelerometer might be
redundantly powered on.

> +		st->apex.on++;
> +		st->apex.wom.enable = true;
> +	}
> +
> +	return 0;
> +}
> +
> +static int inv_icm42607_accel_enable_wom(struct iio_dev *indio_dev)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	struct device *pdev = regmap_get_device(st->map);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(pdev);

Is a devres action or remove callback needed to disable the event and drop the
PM runtime usage count? If the device is unbound via sysfs or the module is
unloaded while the Wake-on-Motion event is active, the PM usage count might
be leaked, keeping the hardware powered on.

> +	if (ret)
> +		return ret;
> +
> +	ret = _inv_icm42607_accel_enable_wom(indio_dev);
> +	if (ret) {
> +		pm_runtime_put_autosuspend(pdev);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int _inv_icm42607_accel_disable_wom(struct iio_dev *indio_dev)
> +{
> +	struct inv_icm42607_state *st = iio_device_get_drvdata(indio_dev);
> +	struct inv_icm42607_sensor_conf conf = INV_ICM42607_SENSOR_CONF_INIT;
> +	unsigned int sleep_ms = 0;
> +	int ret;
> +
> +	scoped_guard(mutex, &st->lock) {
> +		/*
> +		 * Consider that turning off WoM is always working to avoid
> +		 * blocking the chip in on mode and prevent going back to sleep.
> +		 * If there is an error, the chip will anyway go back to sleep
> +		 * and the feature will not work anymore.
> +		 */
> +		st->apex.wom.enable = false;
> +		st->apex.on--;
> +		inv_icm42607_disable_wom(st);
> +		/* turn off accel sensor if not used */
> +		if (!st->apex.on && !iio_buffer_enabled(indio_dev)) {
> +			conf.mode = INV_ICM42607_SENSOR_MODE_OFF;
> +			ret = inv_icm42607_set_accel_conf(st, &conf, &sleep_ms);
> +			if (ret)
> +				return ret;
> +		}
> +	}

Does disabling the IIO buffer unconditionally power off the accelerometer in
inv_icm42607_buffer_postdisable()? If a user enables the Wake-on-Motion
feature (which requires the accelerometer to remain in LOW_NOISE mode) and
subsequently starts then stops the IIO buffer, it appears the sensor could
be physically powered down, preventing the Wake-on-Motion detection from
functioning.

[ ... ]

> @@ -329,6 +516,10 @@ static int inv_icm42607_accel_write_odr(struct iio_dev *indio_dev,
>  	if (ret)
>  		return ret;
>  
> +	ret = inv_icm42607_accel_set_wom_threshold(st, st->apex.wom.value, val, val2);
> +	if (ret)
> +		return ret;
> +

If this fails, does it leave the hardware ODR out of sync with the driver's
FIFO timing state? The earlier code updated the hardware ODR, but returning
early here skips inv_icm42607_buffer_update_fifo_period() and
inv_icm42607_buffer_update_watermark().

>  	inv_icm42607_buffer_update_fifo_period(st);
>  	inv_icm42607_buffer_update_watermark(st);
>  

[ ... ]

> +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);
> +
> +	return inv_icm42607_accel_disable_wom(indio_dev);
> +}

Does this introduce a race condition? Since the lock is dropped before calling
inv_icm42607_accel_enable_wom(), if two threads concurrently enable the event,
both could pass the check and execute the enable function.

This would call pm_runtime_resume_and_get() twice and increment st->apex.on
twice. A subsequent disable operation will only execute once due to the same
check, leaking a runtime PM reference and keeping the device powered on.

[ ... ]

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 1e685451f7850..8ab8b5f739f2b 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> @@ -17,6 +17,57 @@
>  #include "inv_icm42607.h"
>  #include "inv_icm42607_buffer.h"
>  
> +/**
> + *  inv_icm42607_mreg_write() - Write value to user bank

[ ... ]

> +	/*
> +	 * Write bank, register, and value in one bulk write. Then pause
> +	 * the required time according to datasheet.
> +	 */
> +	ret = regmap_bulk_write(map, INV_ICM42607_BLK_SEL_W, buffer, 3);
> +	fsleep(INV_ICM42607_MREG_DELAY_US);
> +	if (ret)
> +		return ret;
> +
> +	/* Switch back to bank1 and pause the required amount of time. */
> +	ret = regmap_write(map, INV_ICM42607_BLK_SEL_W, INV_ICM42607_MREG_1);

If this regmap_bulk_write() fails, will the device be left permanently in bank
2 or 3? Returning early skips the write to switch back to INV_ICM42607_MREG_1.
All subsequent generic register accesses (such as reading INT_STATUS2 in the
IRQ handler) would read from the wrong bank and receive invalid data.

> +	fsleep(INV_ICM42607_MREG_DELAY_US);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

[ ... ]

> @@ -466,6 +668,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);
> +	struct device *accel_dev;
> +	bool wakeup;
> +	int accel_conf;
>  	int ret;
>  
>  	guard(mutex)(&st->lock);
> @@ -482,12 +687,36 @@ static int inv_icm42607_suspend(struct device *dev)
>  			return ret;
>  	}
>  
> -	ret = inv_icm42607_set_pwr_mgmt0(st, INV_ICM42607_SENSOR_MODE_OFF,
> -					 INV_ICM42607_SENSOR_MODE_OFF,
> -					 false, NULL);
> -	if (ret)
> -		return ret;
> -	inv_icm42607_disable_vddio_reg(st);
> +	/* 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_nosync(st->irq);

Could this lead to an I2C/SPI access on a suspended bus?

If the threaded IRQ handler is currently executing and blocked waiting for
st->lock, disable_irq_nosync() will return without waiting for it. Once this
suspend function drops the lock, the PM core proceeds to suspend the parent
controller.

The unblocked IRQ handler would then acquire the lock and attempt to read via
regmap_bulk_read(), potentially causing a timeout or kernel panic on the
suspended bus.

> +		ret = inv_icm42607_set_pwr_mgmt0(st, INV_ICM42607_SENSOR_MODE_OFF,
> +						 accel_conf, false, NULL);
> +		if (ret) {
> +			enable_irq(st->irq);
> +			disable_irq_wake(st->irq);
> +			return ret;
> +		}
> +	} else {
> +		/* disable APEX features and accel if wakeup disabled */
> +		if (st->apex.wom.enable) {
> +			ret = inv_icm42607_disable_wom(st);
> +			if (ret)
> +				return ret;
> +		}
> +		accel_conf = INV_ICM42607_SENSOR_MODE_OFF;
> +		ret = inv_icm42607_set_pwr_mgmt0(st, INV_ICM42607_SENSOR_MODE_OFF,
> +						 INV_ICM42607_SENSOR_MODE_OFF,
> +						 false, NULL);
> +		if (ret)
> +			return ret;
> +		inv_icm42607_disable_vddio_reg(st);
> +	}
>  
>  	return 0;
>  }

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