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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Hardware and software state become desynchronized on RAM mode toggle error path.
- [High] Missing software rollback when AD9910_REG_CFR1 update fails leaves profile caches permanently swapped.
- [High] Permanent loss of the active Single Tone profile configuration in software cache on restore failure.
- [High] Use-After-Free of devm-allocated state structure due to firmware_upload framework race.
- [Medium] Leaked fw_upload cancel flag spuriously fails the subsequent firmware upload attempt.
--

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

This commit adds RAM control channel support for the AD9910 DDS. It introduces
a firmware upload interface for RAM data loading and handles per-profile
configuration. The patch also modifies read/write paths to redirect single-tone
parameters to a shadow register cache when RAM is active.

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index 6c794e1b4b1c8..fef0e3b360a7d 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c
[ ... ]
> @@ -1119,7 +1220,7 @@ static int ad9910_write_raw(struct iio_dev *indio_dev,
>  	struct ad9910_state *st = iio_priv(indio_dev);
>  	u64 tmp64;
>  	u32 tmp32;
> -	int ret;
> +	int ret, i;
>  
>  	guard(mutex)(&st->lock);
[ ... ]
> +		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 recovery loop leave the hardware and software states desynchronized?

If an SPI write fails during the profile swap loop, this block reverts the
software cache but does not rewrite the hardware shadow registers over SPI.

Because the hardware shadow registers retain the swapped values, the next
IO_UPDATE triggered by any other channel write will latch these corrupted
values into the active profile.

> +
> +			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, is it problematic that the function
returns without reverting the profile swap?

By the time this is called, all 8 profiles have already been swapped in
both hardware and software. Returning the error here leaves the profiles
permanently swapped while the RAM mode state remains unchanged, which
appears to break subsequent single-tone and RAM operations.

[ ... ]
> +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;
[ ... ]
> +	guard(mutex)(&st->lock);
> +	if (st->ram_fwu_cancel) {
> +		st->ram_fwu_cancel = false;
> +		return FW_UPLOAD_ERR_CANCELED;
> +	}
> +
> +	return FW_UPLOAD_ERR_NONE;
> +}

[Severity: Medium]
Can a canceled firmware upload spuriously fail a subsequent upload attempt?

If an upload is canceled after the write operation finishes, or during an
indivisible SPI write, st->ram_fwu_cancel is set to true in
ad9910_ram_fwu_cancel(). The driver does not implement an ops->cleanup
callback to reset this state when the upload session ends.

When a new upload begins, ad9910_ram_fwu_prepare() will find this stale
flag, clear it, and unconditionally return FW_UPLOAD_ERR_CANCELED, causing
the new upload to fail before it starts.

[ ... ]
> +static enum fw_upload_err ad9910_ram_fwu_write(struct fw_upload *fw_upload,
> +					       const u8 *data, u32 offset,
> +					       u32 size, u32 *written)
> +{
> +	const struct ad9910_ram_fw *fw_data = (const struct ad9910_ram_fw *)data;
> +	struct ad9910_state *st = fw_upload->dd_handle;
> +	int ret, ret2, idx, wcount;
> +	u64 tmp64, backup;
[ ... ]
> +	/* backup profile register and update it with required address range */
> +	backup = st->reg[AD9910_REG_PROFILE(st->profile)].val64;
> +	tmp64 = AD9910_PROFILE_RAM_STEP_RATE_MSK |
> +		FIELD_PREP(AD9910_PROFILE_RAM_START_ADDR_MSK, 0) |
> +		FIELD_PREP(AD9910_PROFILE_RAM_END_ADDR_MSK, wcount - 1);
> +	ret = ad9910_reg64_write(st, AD9910_REG_PROFILE(st->profile), tmp64, true);
> +	if (ret)
> +		return FW_UPLOAD_ERR_RW_ERROR;
> +
> +	memcpy(&st->tx_buf[1], fw_data->words, wcount * AD9910_RAM_WORD_SIZE);
> +
> +	/* write ram data and restore profile register */
> +	ret = ad9910_spi_write(st, AD9910_REG_RAM,
> +			       wcount * AD9910_RAM_WORD_SIZE, false);
> +	ret2 = ad9910_reg64_write(st, AD9910_REG_PROFILE(st->profile), backup, true);
> +	if (ret || ret2)
> +		return FW_UPLOAD_ERR_RW_ERROR;

[Severity: High]
Can this error path permanently lose the active Single Tone profile
configuration in the software cache?

The active profile's configuration is backed up and replaced with RAM
address boundaries earlier in this function. If the final
ad9910_reg64_write() to restore the backup fails, it returns early.

Because ad9910_reg64_write() only updates the software cache st->reg[] on
success, the software cache appears to be permanently left with the RAM
boundary payload where the frequency, phase, and amplitude bits are zeroed,
wiping the actual Single Tone configuration.

[ ... ]
> @@ -1663,6 +1967,19 @@ static int ad9910_probe(struct spi_device *spi)
[ ... ]
> +	st->ram_fwu = firmware_upload_register(THIS_MODULE, dev, st->ram_fwu_name,
> +					       &ad9910_ram_fwu_ops, st);
> +	if (IS_ERR(st->ram_fwu))
> +		return dev_err_probe(dev, PTR_ERR(st->ram_fwu),
> +				     "failed to register ram upload ops\n");
> +
> +	ret = devm_add_action_or_reset(dev, ad9910_ram_fwu_unregister, st->ram_fwu);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to add ram upload unregister action\n");
> +
>  	return devm_iio_device_register(dev, indio_dev);
>  }

[Severity: High]
Does tying the firmware upload unregistration to devres expose the driver
to a use-after-free race?

There is a known race condition in the core firmware_upload_unregister()
framework: if it observes the IDLE state while a concurrent sysfs write to
loading is about to queue the worker, it returns immediately without
flushing the workqueue.

If the driver unbinds, devres will free the ad9910_state structure. The
delayed worker could then subsequently execute ad9910_ram_fwu_prepare(),
dereferencing the freed state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-ad9910-iio-driver-v8-0-36939c3c07d2@analog.com?part=13