Re: [PATCH V14 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:
- [High] Missing update of software cache for sensor power mode causes unintended hardware power-downs.
- [Medium] Use of non-monotonic `ktime_get_real()` for hardware delay tracking.
--

commit 99de30070cdb6e08bed49a3aac077583415f3f84
Author: Chris Morgan <macromorgan@hotmail.com>
Subject: iio: imu: inv_icm42607: Add Accelerometer for icm42607

This patch adds accelerometer sensor support for the icm42607 device. 
It introduces the accelerometer IIO channel configurations and integrates 
the sensor state management.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index ad23386a37b78..26cedec0c97ce 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c

[ ... ]

> @@ -106,6 +107,15 @@ const struct inv_icm42607_hw inv_icm42607p_hw_data = {

[ ... ]

> +static int inv_icm42607_set_pwr_mgmt0(struct inv_icm42607_state *st,
> +				      enum inv_icm42607_sensor_mode gyro,
> +				      enum inv_icm42607_sensor_mode accel)
> +{

[ ... ]

> +	if (!gyro && oldgyro) {
> +		disable_wait = ktime_us_delta(st->conf.gyro_stop,
> +					      ktime_get_real());

[Severity: Medium]
Is ktime_get_real() safe to use here for hardware delay tracking?

Because ktime_get_real() returns wall-clock time, it is susceptible to
time jumps, such as an NTP synchronization.

If the clock jumps forwards while the gyroscope is being powered off,
could the calculated disable_wait become negative and get clamped to 0,
thereby bypassing the 45ms minimum on-time requirement?

> +		disable_wait = clamp(disable_wait, 0,
> +				     INV_ICM42607_GYRO_STOP_TIME_US);
> +		fsleep(disable_wait);
> +	}

[ ... ]

> +	if (gyro && !oldgyro) {
> +		sleepval_us = max(sleepval_us, INV_ICM42607_GYRO_STARTUP_TIME_US);
> +		/* Track the earliest we can turn off the gyroscope. */
> +		st->conf.gyro_stop = ktime_add_us(ktime_get_real(),
> +						  INV_ICM42607_GYRO_STOP_TIME_US);

[Severity: Medium]
Would using a monotonic time source like ktime_get() prevent potential 
issues with time jumps during this delay calculation?

> +	}
> +
> +	fsleep(sleepval_us);
> +
> +	return 0;
> +}

[ ... ]

> @@ -171,6 +181,160 @@ static int inv_icm42607_set_pwr_mgmt0(struct inv_icm42607_state *st,

[ ... ]

> +int inv_icm42607_set_sensor_conf(struct inv_icm42607_state *st,
> +				 struct inv_icm42607_sensor_conf *conf,
> +				 enum iio_chan_type chan_type)
> +{

[ ... ]

> +	if (chan_type == IIO_ANGL_VEL)
> +		return inv_icm42607_set_pwr_mgmt0(st, conf->mode, st->conf.accel.mode);
> +
> +	return inv_icm42607_set_pwr_mgmt0(st, st->conf.gyro.mode, conf->mode);
> +}

[Severity: High]
Does this code update the software state cache for the sensor mode?

It appears that inv_icm42607_set_sensor_conf() applies the new mode to
the hardware by calling inv_icm42607_set_pwr_mgmt0(), but returns without
saving conf->mode into oldconf->mode (or st->conf.accel.mode / 
st->conf.gyro.mode). 

If this cache remains at its initial OFF value, could a subsequent
configuration of the other sensor read this stale OFF state and
erroneously power down the currently active sensor?

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