Re: [PATCH RFC v4 04/10] iio: frequency: ad9910: add basic parallel port support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The AD9910_PP_FREQ_OFFSET write function fails to reject negative fractional values when the integer part is 0, causing a massive integer underflow when computing the frequency tuning word.
- [Medium] The parallel port channel exposes an `enable` sysfs attribute but lacks support in raw accessors, causing read/write operations to unconditionally return `-EINVAL`.
--

commit 6d64943a7cb666034a2bfe3e59647d035b7c3fb6
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>

iio: frequency: ad9910: add basic parallel port support

This commit adds a parallel port channel to the AD9910 driver with frequency
scale, frequency offset, phase offset, and amplitude offset extended
attributes. Enabling and disabling parallel mode will be implemented later
with buffer setup ops.

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index c75f2ef178c25..b069b849e8d72 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c

[ ... ]

> @@ -511,6 +528,121 @@ static ssize_t ad9910_ext_info_write(struct iio_dev *indio_dev,
[ ... ]
> +static ssize_t ad9910_pp_attrs_write(struct iio_dev *indio_dev,
> +				     uintptr_t private,
> +				     const struct iio_chan_spec *chan,
> +				     const char *buf, size_t len)
> +{
> +	struct ad9910_state *st = iio_priv(indio_dev);
> +	int val, val2;
> +	u32 tmp32;
> +	int ret;
> +
> +	ret = iio_str_to_fixpoint(buf, MICRO / 10, &val, &val2);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	switch (private) {
> +	case AD9910_PP_FREQ_OFFSET:
> +		if (!in_range(val, 0, st->data.sysclk_freq_hz / 2))
> +			return -EINVAL;
> +
> +		tmp32 = ad9910_rational_scale((u64)val * MICRO + val2, BIT_ULL(32),
> +					      (u64)MICRO * st->data.sysclk_freq_hz);

If a negative fraction between 0 and -1 is parsed (for example, "-0.5"),
won't iio_str_to_fixpoint() set val to 0 and val2 to a negative number like
-500000?

Since the bounds check only verifies the integer part (val), 0 passes. Could
the offset calculation (u64)val * MICRO + val2 then underflow to an enormous
unsigned value when adding the negative signed integer to 0ULL?

It looks like this would write a mathematically incorrect, out-of-bounds
frequency tuning word to the device hardware. Should there be an explicit
check for negative values here?

> +		ret = ad9910_reg32_write(st, AD9910_REG_FTW, tmp32, true);
> +		if (ret)
> +			return ret;
> +		break;

[ ... ]

> @@ -564,6 +707,15 @@ static const struct iio_chan_spec ad9910_channels[] = {
[ ... ]
>  	[AD9910_CHAN_IDX_PROFILE_7] = AD9910_PROFILE_CHAN(7),
> +	[AD9910_CHAN_IDX_PARALLEL_PORT] = {
> +		.type = IIO_ALTVOLTAGE,
> +		.indexed = 1,
> +		.output = 1,
> +		.channel = AD9910_CHANNEL_PARALLEL_PORT,
> +		.address = AD9910_CHAN_IDX_PARALLEL_PORT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_ENABLE),

Does this expose an enable sysfs attribute that unconditionally returns
-EINVAL when accessed?

Looking at ad9910_read_raw(), the IIO_CHAN_INFO_ENABLE case only handles the
profile channels:

case IIO_CHAN_INFO_ENABLE:
	switch (chan->channel) {
	case AD9910_CHANNEL_PROFILE_0 ... AD9910_CHANNEL_PROFILE_7:
		[ ... ]
		break;
	default:
		return -EINVAL;
	}

If parallel mode enabling is planned for the future with buffer setup ops as
noted in the commit message, should this attribute bit be omitted for now to
avoid userspace enumeration tool failures?

> +		.ext_info = ad9910_pp_ext_info,
> +	},
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-ad9910-iio-driver-v4-0-d26bfd20ee3d@analog.com?part=4