Re: [PATCH 06/10] iio: frequency: ad9910: add axi backend driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] iio: ad9910: incorrect destination mapping swaps amplitude and frequency
- [High] iio: frequency: ad9910-axi: zero-divisor panic if MMIO read returns ~0
- [Medium] iio: ad9910-axi: profile channels return -EINVAL on disable
--

commit 07c9b224d9d92ea624369fff596857cafd508264
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>
Date:   Thu Oct 15 16:49:46 2026 +0000

iio: frequency: ad9910: add axi backend driver

Add the driver for the AD9910 AXI backend IP core, which interfaces with
the data and control pins of the AD9910 DDS. It registers as an IIO
backend that provides buffer output, parallel data destination configuration,
and PWM integration for ramp direction control.

> diff --git a/drivers/iio/frequency/ad9910-axi-backend.c b/drivers/iio/frequency/ad9910-axi-backend.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/iio/frequency/ad9910-axi-backend.c

[ ... ]

> +static int ad9910_axi_chan_enable(struct iio_backend *back, unsigned int chan)
> +{
> +	struct ad9910_axi_state *st = iio_backend_get_priv(back);
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	switch (chan) {
> +	/*
> +	 * Profiles are mutually exclusive, as the PROFILE[2:0] pins always
> +	 * select one of them. Enabling a profile channel selects it, which
> +	 * implicitly disables the previously selected one, hence there is
> +	 * nothing to do when disabling a profile channel.
> +	 */
> +	case AD9910_CHAN_IDX_PROFILE_0 ... AD9910_CHAN_IDX_PROFILE_7:
> +		chan -= AD9910_CHAN_IDX_PROFILE_0;
> +		return regmap_update_bits(st->regmap, AD9910_AXI_REG_PROFILE,
> +					  AD9910_AXI_REG_PROFILE_MSK,
> +					  FIELD_PREP(AD9910_AXI_REG_PROFILE_MSK, chan));
> +	case AD9910_CHAN_IDX_PARALLEL_AMP ... AD9910_CHAN_IDX_PARALLEL_POLAR_PHASE:
> +		chan = min(chan - AD9910_CHAN_IDX_PARALLEL_AMP, AD9910_DEST_POLAR);

[Severity: High]
Does this subtraction incorrectly map IIO channel indices to hardware
destination codes?

Looking at enum ad9910_channel_idx, AD9910_CHAN_IDX_PARALLEL_AMP is ordered
before AD9910_CHAN_IDX_PARALLEL_FREQ. In this subtraction, Amplitude evaluates
to 0 and Frequency evaluates to 2.

However, the hardware destination codes defined in ad9910.h map
AD9910_DEST_FREQUENCY to 0 and AD9910_DEST_AMPLITUDE to 2.

Will writing this mapped value to AD9910_AXI_REG_F_CFG cause the IIO
parallel amplitude channel to inadvertently configure the hardware for a
frequency destination, and vice versa?

> +		ret = regmap_update_bits(st->regmap, AD9910_AXI_REG_F_CFG,
> +					 AD9910_AXI_REG_F_CFG_MSK, chan);
> +		if (ret)
> +			return ret;
> +
> +		return regmap_set_bits(st->regmap, AD9910_AXI_REG_UPDATE_CTRL,
> +				       AD9910_AXI_REG_UPDATE_PAR_IF_EN_MSK);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad9910_axi_chan_disable(struct iio_backend *back, unsigned int chan)
> +{
> +	struct ad9910_axi_state *st = iio_backend_get_priv(back);
> +
> +	switch (chan) {
> +	case AD9910_CHAN_IDX_PARALLEL_AMP ... AD9910_CHAN_IDX_PARALLEL_POLAR_PHASE:
> +		return regmap_clear_bits(st->regmap, AD9910_AXI_REG_UPDATE_CTRL,
> +					 AD9910_AXI_REG_UPDATE_PAR_IF_EN_MSK);
> +	default:
> +		return -EINVAL;

[Severity: Medium]
Will profile channels fall into this default case and return an error instead
of succeeding cleanly?

The ad9910_axi_chan_enable() function supports enabling profile channels
(AD9910_CHAN_IDX_PROFILE_0 to 7) and successfully returns 0. However, this
disable function lacks a corresponding case for them.

When the IIO core attempts to disable a previously enabled profile channel,
will this unconditionally return -EINVAL and potentially leave the device
state inconsistent or emit unwanted kernel log warnings during teardown?

> +	}
> +}

[ ... ]

> +static int ad9910_axi_read_raw(struct iio_backend *back,
> +			       struct iio_chan_spec const *chan, int *val,
> +			       int *val2, long mask)
> +{
> +	struct ad9910_axi_state *st = iio_backend_get_priv(back);
> +	u32 div;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		switch (chan->address) {
> +		case AD9910_CHAN_IDX_PARALLEL_AMP ... AD9910_CHAN_IDX_PARALLEL_POLAR_PHASE: {
> +			guard(mutex)(&st->lock);
> +
> +			ret = regmap_read(st->regmap, AD9910_AXI_REG_PAR_RATE,
> +					  &div);
> +			if (ret)
> +				return ret;
> +
> +			iio_val_s64_decompose(div_s64(st->pd_clk_freq_uhz, div + 1),
> +					      val, val2);

[Severity: High]
Could this lead to a division-by-zero panic if the MMIO read returns ~0?

If the AXI IP core or bus becomes unresponsive or disconnected, the MMIO read
via regmap_read() may silently return 0xFFFFFFFF (~0) without an error code.

Since div is a u32, div + 1 would overflow to 0. This 0 is then passed
directly to div_s64() as the divisor.

Because ad9910_axi_read_raw can be triggered from userspace by reading sysfs
attributes, could a local attacker or unprivileged user reliably trigger a
kernel panic if the hardware enters an error state?

> +			return IIO_VAL_DECIMAL64_MICRO;
> +		}
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-ad9910-backend-support-v1-0-3c21648c8e26@analog.com?part=6