Re: [PATCH v5 09/13] 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] Incomplete hardware rollback during a profile swap failure leaves AD9910 shadow registers corrupted.
- [High] Missing software cache and hardware rollback when CFR1 register update fails.
- [High] Unprotected software profile cache corruption on firmware upload failure.
- [High] Firmware upload `.write` callback fails on chunked payloads and performs out-of-bounds memory reads.
- [Medium] The RAM firmware upload sysfs node and debugfs symlinks are registered with a `(null)` name.
--

commit 3876f09fc86743085e5099fea5762c34dc150ecf
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, per-profile configuration, profile switching,
sampling frequency control, and ram-enable-aware read/write paths.

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index 4ad80475139d7..86ed350011cff 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c

[ ... ]

> @@ -1202,6 +1306,41 @@ static int ad9910_write_raw(struct iio_dev *indio_dev,
>  			return ad9910_reg32_update(st, AD9910_REG_CFR2,
>  						   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;

If a write fails mid-loop, this rollback block restores the software cache
but doesn't issue SPI writes to revert the hardware shadow registers.
Could the next successful SPI write with an IO_UPDATE pulse inadvertently
latch these corrupted, partially-swapped profiles into the active hardware
state?

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

If ad9910_reg32_update() fails here, the function returns immediately without
reverting the swapped profiles. Should there be a rollback of the swapped
profiles in this error path to prevent the device from operating with corrupted
synthesizer parameters?

>  		default:
>  			return -EINVAL;
>  		}

[ ... ]

> @@ -1475,6 +1657,126 @@ static int ad9910_read_label(struct iio_dev *indio_dev,
> +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;
> +
> +	if (offset != 0)
> +		return FW_UPLOAD_ERR_INVALID_SIZE;

The sysfs firmware upload framework chunks data into blocks bounded by
PAGE_SIZE (typically 4096 bytes). Since the maximum AD9910 firmware size
can be 4176 bytes, this driver will reject any chunks after the first.
Can this be adjusted to support firmware files larger than PAGE_SIZE?

> +
> +	guard(mutex)(&st->lock);
> +
> +	if (st->ram_fwu_cancel)
> +		return FW_UPLOAD_ERR_CANCELED;
> +
> +	if (AD9910_RAM_ENABLED(st))
> +		return FW_UPLOAD_ERR_HW_ERROR;
> +
> +	for (idx = 0; idx < AD9910_NUM_PROFILES; idx++)
> +		st->reg_profile[idx] = get_unaligned_be64(&fw_data->profiles[idx]) |
> +				       AD9910_PROFILE_RAM_OPEN_MSK;

Here the software cache st->reg_profile[] is overwritten before the SPI
hardware operations. If any subsequent hardware writes fail, the function
aborts and the cache is left with partially uploaded profiles.
Could the original cache be backed up and restored on failure?

> +
> +	ret = ad9910_reg32_update(st, AD9910_REG_CFR1,
> +				  AD9910_CFR1_RAM_PLAYBACK_DEST_MSK |
> +				  AD9910_CFR1_INT_PROFILE_CTL_MSK,
> +				  get_unaligned_be32(&fw_data->cfr1), true);
> +	if (ret)
> +		return FW_UPLOAD_ERR_RW_ERROR;
> +
> +	wcount = get_unaligned_be16(&fw_data->wcount);
> +	if (!wcount) {
> +		*written = size;
> +		return FW_UPLOAD_ERR_NONE; /* nothing else to write */
> +	}
> +
> +	ret = ad9910_profile_set(st, st->profile);
> +	if (ret)
> +		return FW_UPLOAD_ERR_HW_ERROR;
> +
> +	/* 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);

If the firmware is larger than PAGE_SIZE, size will be 4096 on the first
call, but wcount could request more bytes than are present in the chunk.
Does this memcpy() risk reading out-of-bounds memory from the data buffer?

[ ... ]

> @@ -1688,7 +2014,25 @@ static int ad9910_probe(struct spi_device *spi)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "device setup failed\n");
>  
> -	return devm_iio_device_register(dev, indio_dev);
> +	snprintf(st->ram_fwu_name, sizeof(st->ram_fwu_name), "%s:ram",
> +		 dev_name(&indio_dev->dev));

The IIO core does not assign the device name to indio_dev->dev until
devm_iio_device_register() is called. Will dev_name() return "(null)" here,
resulting in sysfs nodes named "/sys/class/firmware/(null):ram"?
This might also cause probe failures due to sysfs name collisions if multiple
AD9910 devices are present on the system.

> +	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");
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ad9910_debugfs_init(st, indio_dev);
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517-ad9910-iio-driver-v5-0-31599c88314a@analog.com?part=9