Re: [PATCH v10 13/17] iio: frequency: ad9910: add RAM mode support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incomplete error rollback when enabling or disabling RAM mode leaves the software cache and hardware SPI shadow registers corrupted and out of sync.
--

--- Patch [13]: [PATCH v10 13/17] iio: frequency: ad9910: add RAM mode support ---
commit c736cdf97db9c2e3b8047b8df7de313bdac03b16
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>

iio: frequency: ad9910: add RAM mode support

Add RAM control channel, which includes RAM data loading via firmware upload,
per-profile configuration, profile switching, sampling frequency control, and
read/write paths that swap register cache when RAM is active.

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c

[ ... ]

> @@ -1156,6 +1257,41 @@ static int ad9910_write_raw(struct iio_dev *indio_dev,
>  						   AD9910_CFR2_DRG_DEST_MSK |
>  						   AD9910_CFR2_DRG_ENABLE_MSK,
>  						   tmp32, true);
> +		case AD9910_CHANNEL_RAM:
> +			if (AD9910_RAM_ENABLED(st) == !!val)
> +				return 0;
> +
> +			/* swap 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)
> +					break;
> +				st->reg_profile[i] = tmp64;
> +			}
> +
> +			if (ret) {
> +				/*
> +				 * After the write failure, profiles 0..i-1 were
> +				 * already swapped in SW, but Hw registers are
> +				 * still pending an IO update, so swap them back
> +				 * in SW to keep the state consistent.
> +				 */
> +				while (i--) {
> +					tmp64 = st->reg[AD9910_REG_PROFILE(i)].val64;
> +					st->reg[AD9910_REG_PROFILE(i)].val64 = st->reg_profile[i];
> +					st->reg_profile[i] = tmp64;
> +				}
> +				return ret;

[Severity: High]
If a SPI error occurs inside the for loop above, this while loop reverts the
software arrays (st->reg and st->reg_profile), but does it leave the hardware
shadow registers modified?

The prior successful loop iterations issued SPI writes with update = false.
On the next successful driver operation that triggers an IO update, could
these corrupted profiles be latched into the active hardware since no SPI
writes are issued during the rollback?

> +			}
> +
> +			tmp32 = FIELD_PREP(AD9910_CFR1_RAM_ENABLE_MSK, !!val);
> +			return ad9910_reg32_update(st, AD9910_REG_CFR1,
> +						   AD9910_CFR1_RAM_ENABLE_MSK,
> +						   tmp32, true);

[Severity: High]
If this ad9910_reg32_update() for CFR1 fails, the function returns
immediately.

Since the profile configs were completely swapped in the preceding loop, does
this leave the software arrays (st->reg and st->reg_profile) permanently
inverted while the hardware's RAM mode state remains unchanged?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729-ad9910-iio-driver-v10-0-aa1f6167729e@analog.com?part=13