Re: [PATCH v6 2/2] iio: adc: add MAX40080 current-sense amplifier driver

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Stale measurements: each raw sysfs read returns the previous conversion's data due to an un-flushed FIFO.
- [Medium] Missing READ_ONCE() for `st->range` in `max40080_read_raw()` can lead to data races and torn scale values.
--

--- Patch [2]: [PATCH v6 2/2] iio: adc: add MAX40080 current-sense amplifier driver ---
commit 557f2827eeb4edfa93bf2f63edf6a0664453dad6
Author: Stefan Popa <[email protected]>

iio: adc: add MAX40080 current-sense amplifier driver

The patch adds a direct-mode IIO driver for the MAX40080 current-sense
amplifier, exposing current and voltage channels with configurable
oversampling.

> diff --git a/drivers/iio/adc/max40080.c b/drivers/iio/adc/max40080.c
> new file mode 100644
> index 0000000000000..82572536b704d
> --- /dev/null
> +++ b/drivers/iio/adc/max40080.c

[ ... ]

> +static int max40080_read_iv(struct max40080_state *st, u32 *iv)
> +{
> +	u32 tmp = 0;
> +	int ret, io_ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = max40080_trigger_measurement(st);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Wait for the conversion to complete by polling the FIFO valid bit
> +	 * (or bail out on an I2C error). Polling the device's own status makes
> +	 * this independent of the actual conversion time, which varies with the
> +	 * oversampling ratio and the bus speed. The timeout is only a safety
> +	 * ceiling: the worst case is the maximum 128x averaging on both the
> +	 * current and voltage channels at the slowest 15 ksps base rate plus the
> +	 * inter-channel switching time, i.e. roughly 20 ms; 50 ms leaves ample
> +	 * margin.
> +	 */
> +	ret = read_poll_timeout(max40080_read_iv_once, io_ret,
> +				io_ret || (tmp & MAX40080_IV_VALID_MSK),
> +				500, 50000, false, st, &tmp);

[Severity: High]
Does this polling loop read a stale sample?

Writing the SINGLE mode configuration in max40080_init() triggers an initial
conversion, leaving a sample in the FIFO. Because the FIFO already contains
this unread sample, the read_poll_timeout() loop here might instantly succeed
and pop the stale sample before the newly triggered measurement has a chance
to complete.

This would cause each sysfs read to return the previous conversion's data.

> +	/*
> +	 * Propagate the last-read value even on timeout so the caller can
> +	 * inspect it for debugging.
> +	 */
> +	*iv = tmp;
> +	if (ret)
> +		return ret;
> +
> +	return io_ret;
> +}

[ ... ]

> +static int max40080_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct max40080_state *st = iio_priv(indio_dev);
> +	unsigned int range;
> +	int ret;
> +
> +	switch (mask) {

[ ... ]

> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_CURRENT) {
> +			/*
> +			 * The selectable current-sense range is exposed through
> +			 * scale: each RANGE setting has its own precomputed
> +			 * mA-per-code value. Userspace picks the range by writing
> +			 * the matching scale.
> +			 *
> +			 * Take a local copy of range to ensure val and val2 come
> +			 * from the same setting even if a concurrent write changes
> +			 * st->range between the two accesses.
> +			 */
> +			range = st->range;
> +			*val = st->current_scale[range][0];
> +			*val2 = st->current_scale[range][1];

[Severity: Medium]
Could this result in a torn read if st->range is modified concurrently?

Without a READ_ONCE() for st->range, the compiler might optimize away the local
variable and reload st->range directly from memory between reading
st->current_scale[range][0] and st->current_scale[range][1].

If max40080_write_raw() modifies st->range concurrently, this could return an
invalid combination of integer and fractional components.

> +			return IIO_VAL_INT_PLUS_NANO;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2