Re: [PATCH v2 2/3] iio: accel: Add support for ICM42370P

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [High] Accessing MREG registers mutates a global configuration struct and permanently overwrites the active device configuration without restoring it.
- [High] The required hardware stabilization delay is bypassed, leading to premature MREG register access.
- [High] ODR mapping arrays are inverted, misconfiguring sensor sampling frequencies.
- [High] Runtime PM setup exists without implementing dev_pm_ops callbacks.
- [High] Missing iio_device_claim_direct causes concurrency race on calibbias read.
- [Medium] Incorrect negative minimum calculation in calibbias bounds check rejects valid offsets.
- [Medium] Missing `.read_avail` callback breaks `_available` sysfs attributes.
- [Medium] Unbalanced pm_runtime_use_autosuspend leaks device reference.
- [Medium] MREG error paths fail to restore block selection register.
- [Medium] IRQ handler accesses hardware registers without PM runtime guard.
- [Medium] Endianness missing in IIO_TEMP scan_type definition (Speculative).
- [Medium] Lockless reads of `data->conf` fields in sysfs read handlers.
--

--- Patch [2]: [PATCH v2 2/3] iio: accel: Add support for ICM42370P ---
commit 4a22a747a5f16a585b80aa50c0ea6c3944149d0e
Author: Kanak Shilledar <kanak.shilledar@axis.com>
Subject: iio: accel: Add support for ICM42370P

This commit adds support for the Invensense ICM42370P MEMS MotionTracking
accelerometer over I2C. It implements raw reads, basic sysfs calibration
interfaces, and mandatory temperature sensor enablement.

> diff --git a/drivers/iio/accel/inv_icm42370.h b/drivers/iio/accel/inv_icm42370.h
> index 0000000000000..a4a822f04355f
> --- /dev/null
> +++ b/drivers/iio/accel/inv_icm42370.h
[ ... ]
> +#define INV_ICM42370_TEMP_CHAN(_index) \
> +	{								\
> +		.type = IIO_TEMP,					\
> +		.info_mask_separate =					\
> +			BIT(IIO_CHAN_INFO_RAW) |			\
> +			BIT(IIO_CHAN_INFO_OFFSET) |			\
> +			BIT(IIO_CHAN_INFO_SCALE),			\
> +		.scan_index = _index,					\
> +		.scan_type = {						\
> +			.sign = 's',					\
> +			.realbits = 16,					\
> +			.storagebits = 16,				\
> +		},							\
> +	}

[Severity: Medium]
Does this missing endianness definition cause issues on little-endian
architectures? The accelerometer channel specifies IIO_BE, but this
temperature channel omits it. Since the sensor outputs big-endian
data, userspace reading the temperature from the IIO buffer might
decode mangled values.

> diff --git a/drivers/iio/accel/inv_icm42370_core.c b/drivers/iio/accel/inv_icm42370_core.c
> index 0000000000000..6266362e83f6a
> --- /dev/null
> +++ b/drivers/iio/accel/inv_icm42370_core.c
[ ... ]
> +int inv_icm42370_mreg_write(struct inv_icm42370_data *data, u8 bank, u8 addr, u8 val)
> +{
> +	int ret;
> +	unsigned int sleep;
> +
> +	/* set default conf to ensure mreg access */
> +	ret = inv_icm42370_set_accel_conf(data, &inv_icm42370_default_conf, &sleep);
> +	if (ret)
> +		return ret;
> +
> +	ret = inv_icm42370_mreg_check(data->map);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->map, INV_ICM42370_REG_BLK_SEL_W, bank);
> +	if (ret)
> +		return -EINVAL;
> +
> +	ret = regmap_write(data->map, INV_ICM42370_REG_MADDR_W, addr);
> +	if (ret)
> +		return -EINVAL;
> +
> +	ret = regmap_write(data->map, INV_ICM42370_REG_M_W, val);
> +	if (ret)
> +		return -EINVAL;
> +
> +	fsleep(10);
> +	return regmap_write(data->map, INV_ICM42370_REG_BLK_SEL_W, 0x00);
> +}

[Severity: Medium]
If regmap_write fails at INV_ICM42370_REG_MADDR_W or
INV_ICM42370_REG_M_W, the function returns immediately. Does this leave
the hardware bank selection permanently altered since we never write
0x00 to INV_ICM42370_REG_BLK_SEL_W on the error paths? This might cause
subsequent normal data reads to fetch garbage from the wrong bank.

[ ... ]
> +int inv_icm42370_mreg_read(struct inv_icm42370_data *data, u8 bank, u8 addr, u8 *val)
> +{
> +	int ret;
> +	unsigned int sleep, read_val;
> +
> +	/* set default conf to ensure mreg access */
> +	ret = inv_icm42370_set_accel_conf(data, &inv_icm42370_default_conf, &sleep);
> +	if (ret)
> +		return ret;
> +
> +	ret = inv_icm42370_mreg_check(data->map);

[Severity: High]
Is it intentional to pass the global inv_icm42370_default_conf struct
directly to inv_icm42370_set_accel_conf()?

Looking at inv_icm42370_set_accel_conf(), it mutates the passed
configuration:

oldconf->fs = conf->fs;
oldconf->odr = conf->odr;

This will permanently overwrite the active device configuration in
data->conf and mutate the shared global struct.

[Severity: High]
Additionally, inv_icm42370_set_accel_conf() populates the sleep
variable with the required hardware stabilization time, but the code
proceeds directly to inv_icm42370_mreg_check() without actually
sleeping. Can this cause hardware communication failures due to
bypassing the startup delay?

[ ... ]
> +static irqreturn_t inv_icm42370_irq_handler(int irq, void *_data)
> +{
> +	struct inv_icm42370_data *data = _data;
> +	unsigned int status;
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = regmap_read(data->map, INV_ICM42370_REG_INT_STATUS, &status);

[Severity: Medium]
Should this interrupt handler use pm_runtime_get_if_active() before
accessing the hardware? If a spurious interrupt fires while the device
is in a runtime-suspended state, attempting to read
INV_ICM42370_REG_INT_STATUS without waking the hardware could lead to
I2C bus transfer errors.

[ ... ]
> +static int inv_icm42370_accel_write_offset(struct inv_icm42370_data *data,
> +					   struct iio_chan_spec const *chan,
> +					   int val, int val2)
> +{
> +	struct device *dev = regmap_get_device(data->map);
> +	s64 val64;
> +	s32 min, max;
> +	u8 regval;
> +	s16 offset;
> +	int ret;
> +
> +	if (chan->type != IIO_ACCEL)
> +		return -EINVAL;
> +
> +	/* inv_icm42370_accel_calibbias: min - step - max in micro */
> +	min = inv_icm42370_accel_calibbias[0] * (long)MEGA +
> +	      inv_icm42370_accel_calibbias[1];
> +	max = inv_icm42370_accel_calibbias[4] * (long)MEGA +
> +	      inv_icm42370_accel_calibbias[5];

[Severity: Medium]
Is the calculation for the negative minimum boundary correct here?
Because inv_icm42370_accel_calibbias[0] is negative (-10), adding the
fractional part (42010) instead of subtracting it raises the minimum
floor (e.g., -9957990 instead of -10042010). Will this reject valid
extreme negative calibration offsets?

[ ... ]
> +static int inv_icm42370_accel_read_scale(struct iio_dev *indio_dev, int *val,
> +					 int *val2)
> +{
> +	struct inv_icm42370_data *data = iio_priv(indio_dev);
> +	unsigned int idx;
> +
> +	idx = data->conf.fs;

[Severity: Medium]
Should data->lock be held when reading data->conf.fs here and
data->conf.odr in inv_icm42370_accel_read_odr()? Concurrently writing
to the scale or sampling frequency sysfs nodes might result in stale
reads or data races.

[ ... ]
> +static int inv_icm42370_accel_write_odr(struct iio_dev *indio_dev, int val,
> +					int val2)
> +{
> +	struct inv_icm42370_data *data = iio_priv(indio_dev);
> +	struct inv_sensors_timestamp *ts = &data->ts;
> +	struct device *dev = regmap_get_device(data->map);
> +	unsigned int idx;
> +	struct inv_icm42370_conf conf = INV_ICM42370_SENSOR_CONF_INIT;
> +	int ret;
> +
> +	for (idx = 0; idx < ARRAY_SIZE(inv_icm42370_accel_odr); idx += 2) {
> +		if (val == inv_icm42370_accel_odr[idx] &&
> +		    val2 == inv_icm42370_accel_odr[idx + 1])
> +			break;
> +	}
> +	if (idx >= ARRAY_SIZE(inv_icm42370_accel_odr))
> +		return -EINVAL;
> +
> +	conf.odr = inv_icm42370_accel_odr_conv[idx / 2];

[Severity: High]
Can this map the wrong frequency to the sensor? The
inv_icm42370_accel_odr array defines frequencies in ascending order,
but inv_icm42370_accel_odr_conv maps the hardware enums in descending
order. Requesting 1.5625 Hz uses index 0, which appears to configure
the hardware to 1.6 kHz.

[ ... ]
> +static int inv_icm42370_accel_read_raw(struct iio_dev *indio_dev,
> +				       struct iio_chan_spec const *chan,
> +				       int *val, int *val2, long mask)
> +{
> +	struct inv_icm42370_data *data = iio_priv(indio_dev);
> +	s16 value;
> +	int ret;
> +
> +	switch (chan->type) {
> +	case IIO_ACCEL:
> +		break;
> +
> +	case IIO_TEMP:
> +		return inv_icm42370_temp_read_raw(indio_dev, chan, val, val2, mask);
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (!iio_device_claim_direct(indio_dev))
> +			return -EBUSY;
> +
> +		ret = inv_icm42370_accel_read_sensor(indio_dev, chan, &value);
> +		iio_device_release_direct(indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		*val = value;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		return inv_icm42370_accel_read_scale(indio_dev, val, val2);
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		return inv_icm42370_accel_read_odr(data, val, val2);
> +
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		return inv_icm42370_accel_read_offset(data, chan, val, val2);

[Severity: High]
Should iio_device_claim_direct() be called before reading the
calibration bias? The MREG reads underneath
inv_icm42370_accel_read_offset() silently alter the hardware's scale
and ODR settings. If this happens while the IIO buffer is actively
streaming, it could disrupt the active sampling state.

[ ... ]
> +static const struct iio_info inv_icm42370_info = {
> +	.read_raw = inv_icm42370_accel_read_raw,
> +	.write_raw = inv_icm42370_accel_write_raw,
> +};

[Severity: Medium]
Does this missing .read_avail callback break the _available sysfs
attributes? The channel definitions set
info_mask_shared_by_type_available and
info_mask_shared_by_all_available, which causes the IIO core to expose
those sysfs entries. Without a callback, reads will unconditionally
return -EINVAL.

[ ... ]
> +	/* setup runtime power management */
> +	ret = devm_pm_runtime_set_active_enabled(dev);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_set_autosuspend_delay(dev, INV_ICM42370_SUSPEND_DELAY_MS);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	ret = devm_iio_device_register(dev, indio_dev);

[Severity: Medium]
Is there a missing cleanup action for pm_runtime_use_autosuspend(dev)?
If the driver is unbound or the probe fails after this point, the
device will remain marked for autosuspend indefinitely, leaving an
unbalanced usage counter.

> diff --git a/drivers/iio/accel/inv_icm42370_i2c.c b/drivers/iio/accel/inv_icm42370_i2c.c
> index 0000000000000..c4533671051c4
> --- /dev/null
> +++ b/drivers/iio/accel/inv_icm42370_i2c.c
[ ... ]
> +static struct i2c_driver inv_icm42370_driver = {
> +	.driver = {
> +		.name = "inv-icm42370-i2c",
> +		.of_match_table = inv_icm42370_of_matches,
> +	},
> +	.probe = inv_icm42370_probe,
> +	.id_table = inv_icm42370_id,
> +};

[Severity: High]
The core probe enables runtime PM and autosuspend, but this I2C driver
structure lacks a .pm operations pointer. Without assigning dev_pm_ops
and implementing runtime_suspend, does the hardware ever actually
power down when idle?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813-b4-inv_icm42370p-v2-0-11aedfdf76d3@axis.com?part=2