Re: [PATCH v6 12/16] 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] Hardware and software profile states become permanently desynchronized if an error occurs while enabling or disabling the RAM channel.
- [High] Firmware upload cancellation requests can be silently ignored due to unconditionally resetting the cancel flag.
- [Low] Selecting the `FW_UPLOAD` boolean config symbol from a tristate driver causes an unmet dependency warning when built as a module.
--

commit 75d72698ebee0a7f4f951b2be1d393940055952b
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
interface and per-profile configuration.

> diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
> index 6033f9155b4f7..994d199080ebf 100644
> --- a/drivers/iio/frequency/Kconfig
> +++ b/drivers/iio/frequency/Kconfig
> @@ -29,6 +29,9 @@ config AD9910
>  	tristate "Analog Devices AD9910 Direct Digital Synthesizer"
>  	depends on SPI
>  	depends on GPIOLIB
> +	select CRC32
> +	select FW_LOADER
> +	select FW_UPLOAD

[Severity: Low]
Does selecting FW_UPLOAD from a tristate driver trigger an unmet dependency
warning when built as a module?

Because FW_UPLOAD is a boolean symbol that depends on FW_LOADER, and
FW_LOADER can resolve to 'm', selecting FW_UPLOAD forces it to 'y'. Could
this be avoided by using "depends on FW_UPLOAD" instead?

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index 3fe97aa887c3a..c4e179dda715e 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c
[ ... ]
> @@ -1066,6 +1167,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;
> +			}
> +
> +			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]
Can the hardware and software states become permanently desynchronized if an
error occurs during this transition?

If a write fails midway through the profile loop, the rollback loop restores
the software cache but doesn't seem to rewrite the original values to the
hardware SPI buffers. This leaves the successfully written swapped profiles
still pending in the hardware. 

Additionally, if the entire profile swap completes but the subsequent
ad9910_reg32_update() for CFR1 fails, the function returns immediately
without any rollback.

Would the next unrelated operation that triggers an IO update unintentionally
commit these partially or completely swapped profiles to the active registers?

[ ... ]
> @@ -1335,6 +1506,126 @@ static int ad9910_read_label(struct iio_dev *indio_dev,
>  	return sysfs_emit(label, "%s\n", ad9910_channel_str[chan->address]);
>  }
>  
> +static enum fw_upload_err ad9910_ram_fwu_prepare(struct fw_upload *fw_upload,
> +						 const u8 *data, u32 size)
> +{
> +	struct ad9910_state *st = fw_upload->dd_handle;
> +	const struct ad9910_ram_fw *fw_data = (const struct ad9910_ram_fw *)data;
> +	size_t wcount, bcount;
> +
> +	if (size < sizeof(struct ad9910_ram_fw))
> +		return FW_UPLOAD_ERR_INVALID_SIZE;
> +
> +	if (get_unaligned_be32(&fw_data->magic) != AD9910_RAM_FW_MAGIC)
> +		return FW_UPLOAD_ERR_FW_INVALID;
> +
> +	if (get_unaligned_be16(&fw_data->version) != AD9910_RAM_FW_V1)
> +		return FW_UPLOAD_ERR_FW_INVALID;
> +
> +	wcount = get_unaligned_be16(&fw_data->wcount);
> +	bcount = size - sizeof(struct ad9910_ram_fw);
> +	if (wcount > AD9910_RAM_SIZE_MAX_WORDS ||
> +	    bcount != (wcount * AD9910_RAM_WORD_SIZE))
> +		return FW_UPLOAD_ERR_INVALID_SIZE;
> +
> +	bcount += sizeof(fw_data->cfr1) + sizeof(fw_data->profiles);
> +	if (crc32(0, &fw_data->cfr1, bcount) != get_unaligned_be32(&fw_data->crc))
> +		return FW_UPLOAD_ERR_FW_INVALID;
> +
> +	guard(mutex)(&st->lock);
> +	st->ram_fwu_cancel = false;

[Severity: High]
Does unconditionally clearing the cancellation flag here introduce a race
condition where cancellations requested prior to the worker thread starting
are lost?

If a user cancels the upload via sysfs immediately after triggering it, the
framework calls ad9910_ram_fwu_cancel(), which correctly sets
st->ram_fwu_cancel = true.

However, when the background worker thread begins and calls
ad9910_ram_fwu_prepare(), it will overwrite the cancellation state with
false. Could this allow the firmware upload to proceed and flash the device
instead of aborting?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-ad9910-iio-driver-v6-0-79125ffbe430@analog.com?part=12