Re: [PATCH V6 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 7 potential issue(s) to consider:
- [High] Race condition in `inv_icm42607_accel_write_event_config` leads to PM runtime reference and state leaks.
- [High] The threaded IRQ handler self-deadlocks on the driver's mutex.
- [High] Devres initialization order causes a Use-After-Free during device removal and a potential NULL pointer dereference during probe.
- [High] The Wake-on-Motion thresholds are not configured for the Y and Z axes.
- [Medium] Integer overflow in `inv_icm42607_accel_convert_roc_to_wom` causes misconfiguration for large input values.
- [Medium] Error return in `_inv_icm42607_accel_disable_wom` prevents the accelerometer from powering off.
- [Low] Pointer type mismatch in `_inv_icm42607_accel_wom_value` causes a compiler warning.
--

commit cb98aa6ec3bd79c9e6c757530bb7aecf9c26e058
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 06f2126af22a8..4628bda356370 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
> @@ -209,6 +221,183 @@ 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)
> +{
> +	/* 1000/256mg per LSB converted in µm/s² */
> +	const unsigned int convert = (9807U * (MICRO / MILLI)) / 256U;
> +	u64 value;
> +	u64 freq_uhz;
> +
> +	/* return 0 only if roc is 0 */
> +	if (roc == 0)
> +		return 0;
> +
> +	freq_uhz = (u64)accel_hz * MICRO + (u64)accel_uhz;
> +	value = div64_u64(roc * MICRO, freq_uhz * (u64)convert);

Does this calculation overflow for large input values?

The roc value can be up to ~2.14 * 10^15. Multiplying this by MICRO again
could exceed the maximum value of a 64-bit unsigned integer, potentially
resulting in an incorrectly small threshold value wrapping around.

[ ... ]
> +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;
> +
[ ... ]
> +	/* 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,
> +					      threshold);

Does this code configure the Y and Z axes?

The register address INV_ICM42607_REG_ACCEL_WOM_X_THR is hardcoded, so it
looks like the X-axis register is overwritten three times while the Y and
Z thresholds are left uninitialized.

> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int _inv_icm42607_accel_disable_wom(struct iio_dev *indio_dev)
> +{
[ ... ]
> +	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--;
> +		ret = inv_icm42607_disable_wom(st);
> +		if (ret)
> +			return ret;

Is this early return intentional?

The comment states that failures should be ignored to avoid blocking the
chip in the on state, but the code immediately returns if
inv_icm42607_disable_wom() fails. This skips the subsequent code that
actually turns off the accelerometer, potentially leaving the chip powered on.

> +		/* 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;
> +		}
> +	}
[ ... ]
> @@ -467,6 +660,113 @@ static int inv_icm42607_accel_hwfifo_flush(struct iio_dev *indio_dev,
[ ... ]
> +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;
> +
> +	if (st->apex.wom.enable == state)
> +		return 0;

Can multiple concurrent writes from userspace bypass this lockless check?

If two threads write '1', both could pass the check, acquire PM runtime
references, and increment st->apex.on repeatedly within
inv_icm42607_accel_enable_wom(). A subsequent write of '0' would only decrement
the counters once, potentially leaking a runtime PM reference and preventing
suspend.

> +
> +	if (state)
> +		return inv_icm42607_accel_enable_wom(indio_dev);
> +
> +	return inv_icm42607_accel_disable_wom(indio_dev);
> +}
[ ... ]
> +static int _inv_icm42607_accel_wom_value(struct inv_icm42607_state *st,
> +					 int val, int val2)
> +{
> +	u64 value;
> +	unsigned int accel_hz, accel_uhz;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = inv_icm42607_accel_read_odr(st, &accel_hz, &accel_uhz);

This isn't a bug, but accel_hz and accel_uhz are declared as unsigned int
while inv_icm42607_accel_read_odr() expects int * arguments. This causes an
incompatible pointer type warning.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 56ce9a51cb9c9..a69cd0a61050e 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> @@ -347,6 +431,110 @@ static int inv_icm42607_setup(struct inv_icm42607_state *st,
[ ... ]
> +static irqreturn_t inv_icm42607_irq_handler(int irq, void *_data)
> +{
> +	struct inv_icm42607_state *st = _data;
> +	struct device *dev = regmap_get_device(st->map);
> +	unsigned int status;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
[ ... ]
> +	if (status & INV_ICM42607_INT_STATUS_FIFO_THS) {
> +		ret = inv_icm42607_buffer_fifo_read(st, 0);

Will this cause a self-deadlock?

The threaded IRQ handler acquires st->lock via guard(mutex) and then calls
inv_icm42607_buffer_fifo_read(), which also attempts to acquire the exact
same non-recursive mutex:

drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c:inv_icm42607_buffer_fifo_read() {
...
    guard(mutex)(&st->lock);

> +		if (ret) {
> +			dev_err(dev, "FIFO read error %d\n", ret);
> +			return IRQ_HANDLED;
> +		}
[ ... ]
> @@ -441,6 +634,11 @@ int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw
>  	pm_runtime_set_autosuspend_delay(dev, INV_ICM42607_SUSPEND_DELAY_MS);
>  	pm_runtime_use_autosuspend(dev);
>  
> +	/* Initialize interrupt handling */
> +	ret = inv_icm42607_irq_init(st, irq, irq_type, open_drain);
> +	if (ret)
> +		return ret;
> +
>  	/* Initialize IIO device for Accel */
>  	st->indio_accel = inv_icm42607_accel_init(st);

Is this interrupt request ordered correctly?

The IRQ is enabled via devm_request_threaded_irq in inv_icm42607_irq_init()
before the IIO device is allocated and its buffers are set up. If an early
interrupt fires, could it dereference a NULL st->indio_accel pointer?

Since devres frees resources in reverse order, could this also cause a
use-after-free if an interrupt fires during device teardown after the IIO
device has been freed?

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