Re: [PATCH V13 6/9] iio: imu: inv_icm42607: Add Temp Support in icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Prematurely mutating the temperature software state before calling the power management configuration bypasses state change detection, skipping the mandatory startup delay and causing reads of uninitialized data.

Pre-existing issues:
- [High] The temperature sensor read fails (returns -EINVAL) when the gyroscope and accelerometer are off because the device is placed in Sleep mode instead of Idle mode, disabling the internal RC oscillator.
--

commit d36f0c461ef9d5076de90b75b6238be45c35148d
Author: Chris Morgan <macromorgan@hotmail.com>
Subject: iio: imu: inv_icm42607: Add Temp Support in icm42607

Add functions for reading temperature sensor data.

[ ... ]

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 64f5d263de4f2..644cd7f821b92 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
[ ... ]
> @@ -162,6 +162,24 @@ static int inv_icm42607_set_pwr_mgmt0(struct inv_icm42607_state *st,
>  	return 0;
>  }
>  
> +int inv_icm42607_set_temp_conf(struct inv_icm42607_state *st, bool enable,
> +			       unsigned int *sleep_ms)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	val = FIELD_PREP(INV_ICM42607_TEMP_CONFIG0_FILTER_MASK,
> +			 INV_ICM42607_FILTER_BW_34HZ);
> +	ret = regmap_update_bits(st->map, INV_ICM42607_REG_TEMP_CONFIG0,
> +				 INV_ICM42607_TEMP_CONFIG0_FILTER_MASK, val);
> +	if (ret)
> +		return ret;
> +
> +	return inv_icm42607_set_pwr_mgmt0(st, st->conf.gyro.mode,
> +					  st->conf.accel.mode, enable,
> +					  sleep_ms);

[Severity: High]
This is a pre-existing issue, but will this temperature read fail with -EINVAL
if the gyroscope and accelerometer are off?

Looking at inv_icm42607_set_pwr_mgmt0(), it constructs the PWR_MGMT0 register
value using only the gyro and accel mode masks:

    val = FIELD_PREP(INV_ICM42607_PWR_MGMT0_GYRO_MODE_MASK, gyro);
    val |= FIELD_PREP(INV_ICM42607_PWR_MGMT0_ACCEL_MODE_MASK, accel);
    ret = regmap_write(st->map, INV_ICM42607_REG_PWR_MGMT0, val);

If both gyro and accel are off, val is 0, which places the device in Sleep mode
instead of Idle mode. The internal RC oscillator required for temperature reads
is disabled, causing the data register to return 0x8000
(INV_ICM42607_DATA_INVALID).

Should the IDLE bit be set when temp is true and the other sensors are
inactive?

> +}
> +
[ ... ]

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c
> new file mode 100644
> index 0000000000000..9a60e1a478b08
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c
[ ... ]
> +static int inv_icm42607_temp_read(struct inv_icm42607_state *st, s16 *temp)
> +{
> +	struct device *dev = regmap_get_device(st->map);
> +	u8 raw[2];
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	st->conf.temp_en = true;
> +	ret = inv_icm42607_set_temp_conf(st, st->conf.temp_en, NULL);

[Severity: High]
Does setting st->conf.temp_en to true before this call bypass the hardware
configuration and startup delay?

Because st->conf.temp_en is mutated before the call, when
inv_icm42607_set_temp_conf() calls inv_icm42607_set_pwr_mgmt0(), it will
evaluate the state transition like this:

    bool oldtemp = st->conf.temp_en;
    ...
    if (gyro == oldgyro && accel == oldaccel && temp == oldtemp)
        return 0;

Since temp and oldtemp are now both true, the function returns early without
writing to the power management register or waiting for the startup delay.
Could this lead to reading uninitialized or desynchronized data?

> +	st->conf.temp_en = false;
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_bulk_read(st->map, INV_ICM42607_REG_TEMP_DATA1,
> +			       raw, sizeof(raw));
> +	if (ret)
> +		return ret;
[ ... ]

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