Re: [PATCH v9 13/17] 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] Failure to rollback hardware shadow registers and software cache on error paths leaves DDS state permanently corrupted when toggling RAM mode.
- [Medium] The `st->ram_fwu_cancel` flag can be permanently leaked if an upload is cancelled concurrently while `ad9910_ram_fwu_prepare()` returns an early error.
- [Medium] The driver prematurely updates its software register cache before the hardware write succeeds during RAM firmware upload cleanup.
--

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

iio: frequency: ad9910: add RAM mode support

This patch adds RAM control channel support to the AD9910 DDS driver. It
includes RAM data loading via the firmware upload interface, per-profile
configuration, and sampling frequency control. When RAM is enabled, the DDS
profile parameters are sourced from a shadow register cache.

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index 6c794e1b4b1c8..844cc0cc8f3eb 100644
> --- 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]
Does this error path leave the hardware shadow registers out of sync with the
software cache?

If ad9910_reg64_write() fails for a profile during ad9910_write_raw(), the
hardware shadow registers for previous profiles have already been modified,
but this fallback loop only reverts the software cache.

If an IO_UPDATE pulse is subsequently triggered by a different operation,
could it unintentionally apply the partially swapped configurations to the
hardware?

Also, if the profile swaps succeed but the CFR1 register update below fails,
neither the software nor hardware state seems to be rolled back.

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

[ ... ]

> @@ -1436,6 +1607,142 @@ 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;

[Severity: Medium]
If a sysfs cancel operation occurs concurrently with firmware upload and sets
st->ram_fwu_cancel to true, could one of these early validation checks
returning an error permanently leak the cancel flag?

The fw_upload framework does not call cleanup() if prepare() fails. If the
flag is not cleared on these early error paths in ad9910_ram_fwu_prepare(),
could it cause subsequent valid firmware uploads to be spuriously rejected?

> +
> +	guard(mutex)(&st->lock);
> +	if (st->ram_fwu_cancel) {
> +		/* cleanup() is not called when prepare fails */
> +		st->ram_fwu_cancel = false;
> +		return FW_UPLOAD_ERR_CANCELED;
> +	}
> +
> +	return FW_UPLOAD_ERR_NONE;
> +}
> +
> +static enum fw_upload_err ad9910_ram_fwu_write(struct fw_upload *fw_upload,
> +					       const u8 *data, u32 offset,
> +					       u32 size, u32 *written)
> +{

[ ... ]

> +	/* restore active single tone profile regardless */
> +	st->reg[AD9910_REG_PROFILE(st->profile)].val64 = backup;
> +	ret2 = ad9910_reg64_write(st, AD9910_REG_PROFILE(st->profile), backup, true);
> +	if (ret || ret2)
> +		return FW_UPLOAD_ERR_RW_ERROR;

[Severity: Medium]
Does this unconditionally desynchronize the software cache if the hardware
write fails during ad9910_ram_fwu_write()?

By pre-assigning the software cache st->reg before checking if
ad9910_reg64_write() succeeds, a failed SPI transfer could leave the
hardware with the RAM upload configuration, while the software cache will
falsely indicate it has been restored to the backup state.

> +
> +	*written = size;
> +
> +	return FW_UPLOAD_ERR_NONE;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-ad9910-iio-driver-v9-0-459d1df5ac56@analog.com?part=13