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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Flawed error rollback logic during RAM mode toggle causes permanent driver and hardware state desynchronization.
- [High] The `st->ram_fwu_cancel` flag is never cleared if `ad9910_ram_fwu_prepare()` returns an error, permanently bricking firmware uploads.
--

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

iio: frequency: ad9910: add RAM mode support

This commit adds RAM mode support to the AD9910 driver. It introduces
a RAM control channel for firmware upload, per-profile configuration,
and modifies read/write paths to handle RAM enablement.

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index a37e3dab105bb..f6550cc9717e0 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c
> @@ -1121,6 +1222,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;
> +				}

[Severity: High]
If the hardware write fails, this rollback loop only restores the software
cache. Should it also call ad9910_reg64_write() to revert the hardware
shadow registers that were successfully written prior to the failure?
Without it, the hardware might commit partially written profile data on the
next IO update.

> +				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]
If this final register update fails, the function returns immediately.
Does this leave the profiles permanently swapped in both software and
hardware without rolling back to their original state?

[ ... ]

> @@ -1401,6 +1572,137 @@ static int ad9910_read_label(struct iio_dev *indio_dev,
> +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;
[ ... ]
> +	guard(mutex)(&st->lock);
> +	if (st->ram_fwu_cancel)
> +		return FW_UPLOAD_ERR_CANCELED;

[Severity: High]
If an upload is cancelled and the framework sets st->ram_fwu_cancel = true,
this prepare callback returns FW_UPLOAD_ERR_CANCELED.

Since fw_upload_main() in the firmware upload core explicitly bypasses
the cleanup() callback when prepare() fails, is st->ram_fwu_cancel
ever reset to false?

Could this leave the firmware upload permanently blocked until the
module is reloaded?

> +
> +	return FW_UPLOAD_ERR_NONE;
> +}

[ ... ]

> +static void ad9910_ram_fwu_cleanup(struct fw_upload *fw_upload)
> +{
> +	struct ad9910_state *st = fw_upload->dd_handle;
> +
> +	guard(mutex)(&st->lock);
> +	st->ram_fwu_cancel = false;
> +}

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