Re: [PATCH V19 6/9] iio: imu: inv_icm42607: Add Accelerometer for icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Sensor data read immediately after power-on fails with -EINVAL for low ODRs due to insufficient wait time.
- [Medium] A race condition during device removal leaves the hardware permanently powered on because devm-based IIO unregistration occurs after the driver's custom remove callback.
--

--- Patch [6]: [PATCH V19 6/9] iio: imu: inv_icm42607: Add Accelerometer for icm42607 ---
commit d5a95e354010c482038057c3ba35322667609d59
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..f563c77084201
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c

[ ... ]

> @@ -288,21 +288,21 @@ struct iio_dev *inv_icm42607_accel_init(struct inv_icm42607_state *st)
> +	indio_dev->name = name;
> +	indio_dev->info = &inv_icm42607_accel_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = inv_icm42607_accel_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(inv_icm42607_accel_channels);
> +	iio_device_set_drvdata(indio_dev, st);
> +
> +	ret = devm_iio_device_register(dev, indio_dev);

[Severity: Medium]
Could this lead to the hardware being left permanently powered on
during device removal?

During driver unbind, the custom remove callback (inv_icm42607_sensors_off)
will run before the devm cleanup unregisters this IIO device.

If a concurrent userspace sysfs read re-enables the sensor via runtime PM
before the devm IIO unregistration occurs:

sysfs read
  pm_runtime_get... (powers device on)
  inv_icm42607_read_sensor()
  pm_runtime_put_autosuspend... (queues async suspend)

Will the subsequent devm cleanup of runtime PM cancel the async suspend
and leave the device powered on indefinitely?

> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return indio_dev;
> +}

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index d85eaedef0706..c1648f5c35389 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c

[ ... ]

> @@ -355,18 +364,18 @@ int inv_icm42607_read_sensor(struct iio_dev *indio_dev,
> +	/* read sensor register data */
> +	ret = regmap_bulk_read(st->map, reg, &data, sizeof(data));
> +	if (ret)
> +		return ret;
> +
> +	*val = be16_to_cpu(data);
> +	if (*val == INV_ICM42607_DATA_INVALID)
> +		return -EINVAL;

[Severity: Medium]
Can this return -EINVAL during normal sysfs reads at low ODRs?

When the device is in autosuspend and a sysfs read occurs at a low ODR
(e.g., 1.5625 Hz), the driver wakes up the sensor and waits for 
INV_ICM42607_ACCEL_STARTUP_TIME_US (10ms).

Since the first sample at such a low ODR can take much longer (e.g., ~640ms)
to be ready, will the hardware return INV_ICM42607_DATA_INVALID here,
causing the read to fail rather than blocking until data is ready?

> +
> +	return 0;
> +}

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