Re: [PATCH V8 08/10] iio: imu: inv_icm42607: Add Accelerometer for icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Data race on shared hardware FIFO state resulting in data loss and duplication.
- [High] Hardware interrupt is never requested, rendering FIFO watermark interrupts unhandled.
- [Medium] Uninitialized array padding exposed to userspace as valid available sampling frequencies.
- [Medium] Incorrect system wakeup configuration for the logical IIO device.
- [Low] Semantic mismatch using Low-Power filter enum while the sensor is placed in Low-Noise mode.
- [Low] Lockless reads of shared configuration state in sysfs read handlers.
--

commit 084a1e35ff43644b9bd6bf8af6b8015a9b6bcef4
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..623d607046091
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_accel.c
[ ... ]
> +static int inv_icm42607_accel_read_odr(struct inv_icm42607_state *st,
> +				       int *val, int *val2)
> +{
> +	unsigned int odr;
> +	unsigned int i;
> +
> +	odr = st->conf.accel.odr;

Is it safe to read st->conf.accel.odr here without holding st->lock?

Since sysfs reads and writes for IIO_CHAN_INFO_SAMP_FREQ are not synchronized
by the direct mode lock, could this race with a concurrent write mutating
st->conf?

[ ... ]
> +static int inv_icm42607_accel_read_avail(struct iio_dev *indio_dev,
> +					 struct iio_chan_spec const *chan,
> +					 const int **vals,
> +					 int *type, int *length, long mask)
> +{
[ ... ]
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*vals = (const int *)inv_icm42607_accel_odr;
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		*length = ARRAY_SIZE(inv_icm42607_accel_odr) * 2;
> +		return IIO_AVAIL_LIST;

Since inv_icm42607_accel_odr uses designated initializers starting at index 5,
elements 0 through 4 are implicitly zero-initialized. 

Will returning the entire array size here expose these uninitialized elements
as valid 0 Hz sampling frequencies to userspace?

[ ... ]
> +struct iio_dev *inv_icm42607_accel_init(struct inv_icm42607_state *st)
> +{
[ ... ]
> +	accel_st->power_mode = INV_ICM42607_SENSOR_MODE_LOW_NOISE;
> +	accel_st->filter = INV_ICM42607_FILTER_AVG_16X;

This isn't a bug, but FILTER_AVG values are intended for low-power mode
averaging. Since the sensor is set to INV_ICM42607_SENSOR_MODE_LOW_NOISE,
inv_icm42607_set_accel_conf() will apply this value to the bandwidth filter
mask instead. 

Is it intentional to use an averaging enum to configure a bandwidth register?

[ ... ]
> +	/* accel events are wakeup capable */
> +	ret = devm_device_init_wakeup(&indio_dev->dev);

Should this target the physical hardware device (regmap_get_device(st->map))
instead of the logical IIO device?

The PM subsystem relies on the physical device that owns the IRQ line to arm
the wakeup interrupt during system suspend. Additionally,
inv_icm42607_suspend() unconditionally sets INV_ICM42607_SENSOR_MODE_OFF. 

Will this prevent hardware wake-on-motion events from occurring?

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> index a011f1f728b98..0e723a7dd5623 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_buffer.c
[ ... ]
>  int inv_icm42607_buffer_hwfifo_flush(struct inv_icm42607_state *st,
>  				     unsigned int count)
>  {
> +	struct inv_icm42607_sensor_state *accel_st = iio_priv(st->indio_accel);
> +	struct inv_sensors_timestamp *ts;
> +	s64 accel_ts;
>  	int ret;
>  
> +	accel_ts = iio_get_time_ns(st->indio_accel);
>  
>  	ret = inv_icm42607_buffer_fifo_read(st, count);
> +	if (ret)
> +		return ret;
>  
> -	return ret;
> +	if (st->fifo.nb.total == 0)
> +		return 0;
> +
> +	if (st->fifo.nb.accel > 0) {
> +		ts = &accel_st->ts;
> +		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel, accel_ts);
> +		ret = inv_icm42607_accel_parse_fifo(st->indio_accel);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
>  }

Since both inv_icm42607_buffer_fifo_read() and inv_icm42607_accel_parse_fifo()
acquire and release st->lock internally, the mutex is dropped between reading
the hardware FIFO and parsing it.

If another sysfs hwfifo_flush executes concurrently for the gyroscope device,
could it overwrite st->fifo.data and reset st->fifo.nb while the lock is
dropped?

If so, this might cause the resumed thread to parse overwritten data with
stale counters, potentially leading to data loss or corrupted timestamps.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 5d9155e4c41f9..4bbcbbe8905f4 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
[ ... ]
> @@ -379,6 +435,11 @@ int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw
>  	pm_runtime_set_autosuspend_delay(dev, INV_ICM42607_SUSPEND_DELAY_MS);
>  	pm_runtime_use_autosuspend(dev);
>  
> +	/* Initialize IIO device for Accel */
> +	st->indio_accel = inv_icm42607_accel_init(st);
> +	if (IS_ERR(st->indio_accel))
> +		return PTR_ERR(st->indio_accel);
> +
>  	return 0;
>  }

The driver configures hardware watermark interrupts and retrieves the INT1
irq line via fwnode_irq_get_byname() during probe, but I don't see a call to
devm_request_threaded_irq() being added.

Does the lack of an interrupt handler prevent the driver from automatically
draining the FIFO and leave the newly added inv_icm42607_buffer_fifo_parse()
routine unhandled?

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