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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing lock protection around `st->range` and `st->oversampling_ratio` updates causes hardware state desynchronization and torn reads.
--

--- Patch [2]: [PATCH v3 2/2] iio: adc: add MAX40080 current-sense amplifier driver ---
commit 6f9d9d6fa7d25018a7143edc9a8c9183463f1900
Author: Stefan Popa <stefan.popa@analog.com>

iio: adc: add MAX40080 current-sense amplifier driver

The MAX40080 is a bidirectional current-sense amplifier with an
integrated 12-bit ADC and an I2C/SMBus interface. It measures the
voltage across an external shunt resistor and the input bus voltage,
storing the results in an internal FIFO.

Link: https://www.analog.com/media/en/technical-documentation/data-sheets/MAX40080.pdf

> diff --git a/drivers/iio/adc/max40080.c b/drivers/iio/adc/max40080.c
> new file mode 100644
> index 0000000000000..a0c1144cfda7c
> --- /dev/null
> +++ b/drivers/iio/adc/max40080.c
[ ... ]
> +static int max40080_set_range(struct max40080_state *st, unsigned int range)
> +{
> +	int ret;
> +
> +	ret = max40080_update_bits(st, MAX40080_REG_CFG, MAX40080_CFG_RANGE_MSK,
> +				   FIELD_PREP(MAX40080_CFG_RANGE_MSK, range));
> +	if (ret)
> +		return ret;
> +
> +	st->range = range;

[Severity: Medium]
Could a race condition here cause the software cache to become desynchronized
with the hardware state?

Since max40080_update_bits() releases st->lock before returning, concurrent
writes to the scale attribute could interleave their hardware updates and
software cache updates, leaving the hardware set to one range and the cache
set to another.

> +
> +	return 0;
> +}
[ ... ]
> +static int max40080_set_oversampling_ratio(struct max40080_state *st, int val)
> +{
> +	int filter;
> +	int ret;
> +
> +	filter = max40080_oversampling_to_filter(val);
> +	if (filter < 0)
> +		return filter;
> +
> +	ret = max40080_update_bits(st, MAX40080_REG_CFG, MAX40080_CFG_FILTER_MSK,
> +				   FIELD_PREP(MAX40080_CFG_FILTER_MSK, filter));
> +	if (ret)
> +		return ret;
> +
> +	st->oversampling_ratio = val;

[Severity: Medium]
Similar to st->range above, could st->oversampling_ratio become desynchronized
with the hardware state due to being updated outside the lock?

> +
> +	return 0;
> +}
> +
> +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);
> +	int ret;
> +
> +	switch (mask) {
[ ... ]
> +	case IIO_CHAN_INFO_SCALE:
> +		/*
> +		 * 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.
> +		 */
> +		switch (chan->type) {
> +		case IIO_CURRENT:
> +			*val = st->current_scale[st->range][0];
> +			*val2 = st->current_scale[st->range][1];

[Severity: Medium]
Can this cause a torn read if st->range is updated concurrently?

Since st->range is used twice as an array index without taking st->lock,
concurrent writes could cause val and val2 to be mixed from different ranges.

> +			return IIO_VAL_INT_PLUS_NANO;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715063652.368501-1-stefan.popa@analog.com?part=2