Re: [PATCH v6 11/16] iio: frequency: ad9910: add digital ramp generator support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Negative values written to DRG_RAMP_UP or DRG_RAMP_DOWN raw attributes are silently converted and clamped to U32_MAX instead of returning an error.
--

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

iio: frequency: ad9910: add digital ramp generator support

This patch adds support for Digital Ramp Generator channels to the AD9910
driver. It includes configuration for destination selection, dwell mode,
limits, step sizes, and step rates.

> diff --git a/drivers/iio/frequency/ad9910.c b/drivers/iio/frequency/ad9910.c
> index 262702b62738d..3fe97aa887c3a 100644
> --- a/drivers/iio/frequency/ad9910.c
> +++ b/drivers/iio/frequency/ad9910.c

[ ... ]

> @@ -831,22 +1108,62 @@ static int ad9910_write_raw(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  		}
>  	case IIO_CHAN_INFO_RAW:
> -		if (val < 0)
> -			return -EINVAL;
> -
>  		switch (chan->channel) {
>  		case AD9910_CHANNEL_PROFILE_0 ... AD9910_CHANNEL_PROFILE_7:
> +			if (val < 0)
> +				return -EINVAL;
> +
>  			tmp32 = chan->channel - AD9910_CHANNEL_PROFILE_0;
>  			tmp64 = FIELD_PREP(AD9910_PROFILE_ST_ASF_MSK,
>  					   min_t(u64, val, AD9910_ASF_MAX));
>  			return ad9910_reg64_update(st, AD9910_REG_PROFILE(tmp32),
>  						   AD9910_PROFILE_ST_ASF_MSK,
>  						   tmp64, true);
> +		case AD9910_CHANNEL_DRG_RAMP_UP:
> +			tmp64 = iio_val_s64_compose(val, val2);
> +			tmp64 = min_t(u64, tmp64, U32_MAX);

[Severity: Medium]
When a negative value is written to the DRG_RAMP_UP or DRG_RAMP_DOWN raw
attributes, does this bypass error checking?

Since iio_val_s64_compose() returns an s64, a negative input will be
implicitly cast to u64 when assigned to tmp64. This causes it to wrap to a
large positive number, which min_t() will then clamp to U32_MAX rather than
rejecting the invalid negative input.

Should we check for negative values before calling iio_val_s64_compose(),
similar to what is done for the profile channels?

> +			tmp64 = FIELD_PREP(AD9910_DRG_LIMIT_UPPER_MSK, tmp64);
> +			return ad9910_reg64_update(st, AD9910_REG_DRG_LIMIT,
> +						   AD9910_DRG_LIMIT_UPPER_MSK,
> +						   tmp64, true);
> +		case AD9910_CHANNEL_DRG_RAMP_DOWN:
> +			tmp64 = iio_val_s64_compose(val, val2);
> +			tmp64 = min_t(u64, tmp64, U32_MAX);
> +			tmp64 = FIELD_PREP(AD9910_DRG_LIMIT_LOWER_MSK, tmp64);
> +			return ad9910_reg64_update(st, AD9910_REG_DRG_LIMIT,
> +						   AD9910_DRG_LIMIT_LOWER_MSK,
> +						   tmp64, true);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-ad9910-iio-driver-v6-0-79125ffbe430@analog.com?part=11