Re: [PATCH RFC v4 07/10] iio: frequency: ad9910: add output shift keying support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The OSK channel cannot be scaled because ad9910_write_raw_get_fmt() does not support AD9910_CHANNEL_OSK.
- [Medium] Integer wrap-around on 32-bit platforms leads to incorrect scale_roc configuration for out-of-bounds user inputs.
- [Medium] Writing non-standard truthy values to the OSK enable attribute turns the channel off instead of on.
- [Low] The scale_roc_available sysfs attribute is incorrectly created with write permissions.
--

commit d92799d64693d2c01054913b1a546994813e0150
Author: Rodrigo Alencar <rodrigo.alencar@analog.com>
Subject: iio: frequency: ad9910: add output shift keying support

Add OSK channel with amplitude envelope control capabilities.

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index a153cd01e6f58..1fdbaba356d73 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c
> @@ -980,6 +985,134 @@ static ssize_t ad9910_drg_attrs_write(struct iio_dev *indio_dev,
>  	return len;
>  }
>  

[ ... ]

> +static ssize_t ad9910_osk_attrs_write(struct iio_dev *indio_dev,
> +				      uintptr_t private,
> +				      const struct iio_chan_spec *chan,
> +				      const char *buf, size_t len)
> +{

[ ... ]

> +	switch (private) {
> +	case AD9910_OSK_AUTO_ROC:
> +		nstep = ad9910_rational_scale((u64)val * NANO + val2, rate,
> +					      st->data.sysclk_freq_hz);
> +		idx = find_closest(nstep, ad9910_osk_nstep,
> +				   ARRAY_SIZE(ad9910_osk_nstep));

Is there a chance that a very large nstep value could cause issues on 32-bit
platforms here?

The find_closest() macro internally assigns the input to a local long variable.
If nstep is a u64 that exceeds LONG_MAX, it could wrap around to a negative
number, matching the 0th index instead of the maximum step size.

[ ... ]

> @@ -997,6 +1130,9 @@ static ssize_t ad9910_drg_attrs_write(struct iio_dev *indio_dev,
>  #define AD9910_DRG_EXT_INFO(_name, _ident) \
>  	AD9910_EXT_INFO_TMPL(_name, _ident, IIO_SEPARATE, drg_attrs)
>  
> +#define AD9910_OSK_EXT_INFO(_name, _ident) \
> +	AD9910_EXT_INFO_TMPL(_name, _ident, IIO_SEPARATE, osk_attrs)
> +
>  static const struct iio_chan_spec_ext_info ad9910_phy_ext_info[] = {
>  	AD9910_EXT_INFO("powerdown", AD9910_POWERDOWN, IIO_SEPARATE),
>  	{ }
> @@ -1018,6 +1154,12 @@ static const struct iio_chan_spec_ext_info ad9910_drg_ramp_ext_info[] = {
>  	{ }
>  };
>  
> +static const struct iio_chan_spec_ext_info ad9910_osk_ext_info[] = {
> +	AD9910_OSK_EXT_INFO("scale_roc", AD9910_OSK_AUTO_ROC),
> +	AD9910_OSK_EXT_INFO("scale_roc_available", AD9910_OSK_AUTO_ROC_AVAIL),
> +	{ }
> +};
> +

Does scale_roc_available end up with write permissions here?

The AD9910_OSK_EXT_INFO macro uses AD9910_EXT_INFO_TMPL, which assigns both
the .read and .write callbacks. This will cause the IIO core to create
the scale_roc_available sysfs attribute with 0644 permissions, but available
attributes are typically read-only.

[ ... ]

> @@ -1102,6 +1244,17 @@ static const struct iio_chan_spec ad9910_channels[] = {
>  				      BIT(IIO_CHAN_INFO_PHASE) |
>  				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  	},
> +	[AD9910_CHAN_IDX_OSK] = {
> +		.type = IIO_ALTVOLTAGE,
> +		.indexed = 1,
> +		.output = 1,
> +		.channel = AD9910_CHANNEL_OSK,
> +		.address = AD9910_CHAN_IDX_OSK,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_ENABLE) |
> +				      BIT(IIO_CHAN_INFO_SCALE) |
> +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.ext_info = ad9910_osk_ext_info,
> +	},
>  };

Since AD9910_CHANNEL_OSK is added with IIO_CHAN_INFO_SCALE here, does
ad9910_write_raw_get_fmt() also need to be updated to support this channel type?

If ad9910_write_raw_get_fmt() falls through to the default case and returns
-EINVAL, it looks like the IIO core might abort write operations for the scale
attribute before ad9910_write_raw() is even called.

[ ... ]

> @@ -1326,6 +1493,11 @@ static int ad9910_write_raw(struct iio_dev *indio_dev,
>  			return ad9910_reg32_update(st, AD9910_REG_CFR1,
>  						   AD9910_CFR1_RAM_ENABLE_MSK,
>  						   tmp32, true);
> +		case AD9910_CHANNEL_OSK:
> +			tmp32 = FIELD_PREP(AD9910_CFR1_OSK_ENABLE_MSK, val);
> +			return ad9910_reg32_update(st, AD9910_REG_CFR1,
> +						   AD9910_CFR1_OSK_ENABLE_MSK,
> +						   tmp32, true);
>  		default:
>  			return -EINVAL;
>  		}

If val is a non-standard truthy value (like 2), wouldn't FIELD_PREP() mask it
to 0 and inadvertently disable the channel instead of enabling it?

Should this use !!val to sanitize the input, similar to how the RAM channel
does it?

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