Re: [PATCH v4 12/12] iio: dac: ad5686: add gain control support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Data race in `ad5686_read_avail()` due to lockless access of `st->double_scale`, which is temporarily mutated in `ad5686_write_raw()`.
--

commit 7b5b8c7fbbe74efe7dd9f093ff8593661667e614
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>

iio: dac: ad5686: add gain control support

Adds gain control support to the ad5686 driver, allowing a 2x multiplier
to be applied to the output voltage either via a dedicated GAIN pin or
a control register bit field. It exposes available scales to userspace
and adds tracking of the double scale state.

> diff --git a/drivers/iio/dac/ad5686.c b/drivers/iio/dac/ad5686.c
> index 4dc681eb077d6..bad214b89257f 100644
> --- a/drivers/iio/dac/ad5686.c
> +++ b/drivers/iio/dac/ad5686.c
[ ... ]
> @@ -217,6 +229,84 @@ static int ad5686_write_raw(struct iio_dev *indio_dev,
>  
>  		return ad5686_write(st, AD5686_CMD_WRITE_INPUT_N_UPDATE_N,
>  				    chan->address, val << chan->scan_type.shift);
> +	case IIO_CHAN_INFO_SCALE:
> +		if (val == st->scale_avail[0] && val2 == st->scale_avail[1])
> +			double_scale = false;
> +		else if (val == st->scale_avail[2] && val2 == st->scale_avail[3])
> +			double_scale = true;
> +		else
> +			return -EINVAL;
> +
> +		if (st->double_scale == double_scale)
> +			return 0; /* no change */
> +
> +		st->double_scale = double_scale;
> +		switch (st->chip_info->regmap_type) {
> +		case AD5310_REGMAP:
> +			ret = ad5310_control_sync(st);
> +			break;
> +		case AD5683_REGMAP:
> +			ret = ad5683_control_sync(st);
> +			break;
> +		case AD5686_REGMAP:
> +			if (!st->gain_gpio) {
> +				ret = -EINVAL;
> +				break;
> +			}
> +
> +			ret = gpiod_set_value_cansleep(st->gain_gpio,
> +						       st->double_scale ? 1 : 0);
> +			break;
> +		default:
> +			ret = -EINVAL;
> +		}
> +		if (ret)
> +			st->double_scale = !double_scale; /* revert on failure */
> +		return ret;
[ ... ]
> +static int ad5686_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     const int **vals, int *type, int *length,
> +			     long mask)
> +{
> +	struct ad5686_state *st = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		*type = IIO_VAL_INT_PLUS_NANO;
> +
> +		if (st->chip_info->regmap_type == AD5686_REGMAP && !st->gain_gpio) {
> +			/*
> +			 * GAIN pin is board-strapped, so only the current
> +			 * scale is available.
> +			 */
> +			*vals = st->double_scale ? &st->scale_avail[2] :
> +						   &st->scale_avail[0];

[Severity: Medium]
Is there a potential data race on st->double_scale here?

In ad5686_write_raw(), st->double_scale is temporarily modified and then
reverted on failure under st->lock. However, ad5686_read_avail() accesses
st->double_scale without holding st->lock.

If a sysfs read on in_voltage_scale_available happens concurrently with a
failing sysfs write on in_voltage_scale for a board-strapped device, could
the read observe the temporary invalid state of st->double_scale before
it is reverted and returned to userspace?

> +			*length = 2;
> +			return IIO_AVAIL_LIST;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623-ad5686-new-features-v4-0-28962a57db0f@analog.com?part=12