Re: [PATCH V12 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:
- [High] The DMA buffer `buffer` is placed at the beginning of `struct inv_icm42607_state`, sharing a cacheline with subsequent members like `hw`, `map`, and `lock`. This can lead to memory corruption and deadlocks during DMA operations.
- [High] The temperature sensor is permanently left enabled after a single raw read, continuously wasting power and adding unnecessary latency to subsequent runtime resumes.
--

commit 18abfa4e34a17ed9cef9a8a6e5d8c222ba930a49
Author: Chris Morgan <macromorgan@hotmail.com>

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.h b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> index 35c9210ca86fe..5416754907913 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607.h
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607.h
[ ... ]
> @@ -124,6 +125,7 @@ struct inv_icm42607_suspended {
>   *  @orientation:	sensor chip orientation relative to main hardware.
>   */
>  struct inv_icm42607_state {
> +	__be16 buffer[3] __aligned(IIO_DMA_MINALIGN);
>  	const struct inv_icm42607_hw *hw;
>  	struct regmap *map;

[Severity: High]
Does this DMA buffer alignment ensure the end of the buffer is also padded to
the cacheline boundary?

Since the buffer is only 6 bytes, fields following it like hw, map, and the
mutex lock will share the same cacheline. When performing a DMA transfer, the
controller might invalidate the entire cacheline, which could destroy
concurrent writes to the lock or other shared state and lead to memory
corruption or deadlocks.

Usually, buffers used for DMA need to occupy their own cacheline. Placing
them at the end of the struct with __aligned(IIO_DMA_MINALIGN) can prevent
this cacheline sharing.

>  	struct regulator *vddio_supply;

[ ... ]
> 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..b259fc9c9fd29
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_temp.c
> @@ -0,0 +1,76 @@
[ ... ]
> +static int inv_icm42607_temp_read(struct inv_icm42607_state *st, s16 *temp)
> +{
[ ... ]
> +	ret = inv_icm42607_set_temp_conf(st, true, NULL);
> +	if (ret)
> +		return ret;
> +
> +	raw = &st->buffer[0];
> +	ret = regmap_bulk_read(st->map, INV_ICM42607_REG_TEMP_DATA1,
> +			       raw, sizeof(*raw));
> +	if (ret)
> +		return ret;
> +
> +	*temp = be16_to_cpup(raw);
> +	if (*temp == INV_ICM42607_DATA_INVALID)
> +		return -EINVAL;
> +
> +	return 0;
> +}

[Severity: High]
Will the temperature sensor remain permanently enabled after this read
completes?

Since inv_icm42607_set_temp_conf(st, true, NULL) is called but never disabled
before returning, st->conf.temp_en will stay true. 

Because the driver saves and restores this state during runtime PM suspend
and resume, the temperature sensor will stay powered on whenever the chip is
active, consuming extra power.

Additionally, this leaked state might force an artificial 77ms sleep delay on
every subsequent runtime PM resume. When the device resumes,
inv_icm42607_set_pwr_mgmt0() will see the sensor transitioning from off to
on:

inv_icm42607_set_pwr_mgmt0() {
...
	st->conf.temp_en = temp;

	sleepval_ms = 0;
	if (temp && !oldtemp)
		sleepval_ms = max(sleepval_ms, INV_ICM42607_TEMP_STARTUP_TIME_MS);
...
}

Should the temperature sensor configuration be disabled before returning from
inv_icm42607_temp_read()?

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