Re: [PATCH RFC v4 06/10] iio: frequency: ad9910: add RAM mode support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] State Corruption on Hardware Write Failure during RAM mode toggle
- [High] Integer Underflow from negative fraction bypasses bounds checks
- [Medium] Missing IIO_CHAN_INFO_SCALE interface for RAM channel prevents single-tone amplitude configuration
--

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

iio: frequency: ad9910: add RAM mode support

This commit adds RAM control channel support to the AD9910 driver. It
introduces RAM data loading via firmware upload, profile configuration,
and profile parameter caching when RAM mode is active.

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index d6c88ec51145e..a153cd01e6f58 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c

[ ... ]

> @@ -1020,6 +1091,17 @@ static const struct iio_chan_spec ad9910_channels[] = {
>  				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  		.ext_info = ad9910_drg_ramp_ext_info,
>  	},
> +	[AD9910_CHAN_IDX_RAM] = {
> +		.type = IIO_ALTVOLTAGE,
> +		.indexed = 1,
> +		.output = 1,
> +		.channel = AD9910_CHANNEL_RAM,
> +		.address = AD9910_CHAN_IDX_RAM,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_ENABLE) |
> +				      BIT(IIO_CHAN_INFO_FREQUENCY) |
> +				      BIT(IIO_CHAN_INFO_PHASE) |
> +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +	},
>  };

Is BIT(IIO_CHAN_INFO_SCALE) missing from this mask?

ad9910_write_raw_get_fmt() expects AD9910_CHANNEL_RAM to support
IIO_CHAN_INFO_SCALE for single-tone amplitude configuration. Without the bit
set here, and without handling it in ad9910_read_raw() and ad9910_write_raw(),
users cannot adjust the single-tone amplitude while RAM mode is enabled.

[ ... ]

> @@ -1207,6 +1306,26 @@ static int ad9910_write_raw(struct iio_dev *indio_dev,
>  			return ad9910_reg32_update(st, AD9910_REG_CFR2,
>  						   AD9910_CFR2_DRG_ENABLE_MSK,
>  						   tmp32, true);
> +		case AD9910_CHANNEL_RAM:
> +			if (AD9910_RAM_ENABLED(st) == !!val)
> +				return 0;
> +
> +			/* switch profile configs */
> +			for (i = 0; i < AD9910_NUM_PROFILES; i++) {
> +				tmp64 = st->reg[AD9910_REG_PROFILE(i)].val64;
> +				ret = ad9910_reg64_write(st,
> +							 AD9910_REG_PROFILE(i),
> +							 st->reg_profile[i],
> +							 false);
> +				if (ret)
> +					return ret;

Can this loop corrupt the software state if an SPI write fails partway through?

If ad9910_reg64_write() returns an error, the function exits immediately without
rolling back the profiles that were already swapped. A subsequent retry would
then iterate over the profiles again, swapping the previously swapped profiles
back and moving the remaining ones forward. This leaves the hardware and
software caches permanently desynchronized.

> +				st->reg_profile[i] = tmp64;
> +			}
> +
> +			tmp32 = FIELD_PREP(AD9910_CFR1_RAM_ENABLE_MSK, !!val);
> +			return ad9910_reg32_update(st, AD9910_REG_CFR1,
> +						   AD9910_CFR1_RAM_ENABLE_MSK,
> +						   tmp32, true);
>  		default:
>  			return -EINVAL;
>  		}

[ ... ]

> @@ -1246,6 +1370,8 @@ static int ad9910_write_raw(struct iio_dev *indio_dev,
>  			return ad9910_reg64_update(st, AD9910_REG_DRG_LIMIT,
>  						   AD9910_DRG_LIMIT_LOWER_MSK,
>  						   tmp64, true);
> +		case AD9910_CHANNEL_RAM:
> +			return ad9910_reg32_write(st, AD9910_REG_FTW, tmp64, true);

Does the IIO_CHAN_INFO_FREQUENCY block handle negative fractional inputs
correctly when the integer part is zero?

If a user writes a value like -0.1, IIO parses it as val = 0 and
val2 = -100000. Earlier in the IIO_CHAN_INFO_FREQUENCY block, the bounds
check only inspects val:

	case IIO_CHAN_INFO_FREQUENCY:
		if (!in_range(val, 0, st->data.sysclk_freq_hz / 2))
			return -EINVAL;

		tmp64 = ad9910_rational_scale((u64)val * MICRO + val2, ...);

Since val is 0, the check passes. The mathematical operation then adds a
negative integer to an unsigned 64-bit integer, underflowing to a massive
value. This bypasses the bounds check and silently configures a huge RF
frequency instead of safely returning -EINVAL.

This same underflow pattern appears to affect IIO_CHAN_INFO_SCALE and
IIO_CHAN_INFO_SAMP_FREQ.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-ad9910-iio-driver-v4-0-d26bfd20ee3d@analog.com?part=6