Re: [PATCH v6 2/8] iio: core: add fixed point parsing with 64-bit parts

[Rodrigo Alencar]

On 26/02/02 09:57AM, Nuno Sá wrote:
> On Fri, 2026-01-30 at 10:06 +0000, Rodrigo Alencar via B4 Relay wrote:
> > From: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> > 
> > Add iio_str_to_fixpoint64() function that leverages simple_strtoull()
> > to parse numbers from a string.
> > A helper function __iio_str_to_fixpoint64() replaces
> > __iio_str_to_fixpoint() implementation, extending its usage for
> > 64-bit fixed-point parsing.

...

> >  /**
> >   * __iio_str_to_fixpoint() - Parse a fixed-point number from a string
> >   * @str: The string to parse
> > @@ -895,63 +1026,43 @@ static ssize_t iio_read_channel_info_avail(struct device *dev,
> >  static int __iio_str_to_fixpoint(const char *str, int fract_mult,
> >  				 int *integer, int *fract, bool scale_db)
> >  {
> > -	int i = 0, f = 0;
> > -	bool integer_part = true, negative = false;
> > +	s64 integer64, fract64;
> > +	int ret;
> >  
> > -	if (fract_mult == 0) {
> > -		*fract = 0;
> > +	ret = __iio_str_to_fixpoint64(str, fract_mult, &integer64, &fract64,
> > +				      scale_db);
> > +	if (ret)
> > +		return ret;
> 
> I know it feels tempting to do the above while adding the 64bit variant. But isn't the
> overflow safety also an issue on the 32bit variant? IMO, we should first have a patch
> adding the overflow safety with a Fixes tag and then add 64bit support.

I think handling 64-bit support after taclking the overflow issue
would require changes on top of previous ones, which might get a messy
commit history, no? Mostly because the 64-bit variant of the function
is being used inside the 32-bit one. Also, the added auxiliary function
that implements the overflow check parses u64, which allowed for the
removal of the while loop in the __iio_str_to_fixpoint() implementation.
 
>  
> >  
> > -		return kstrtoint(str, 0, integer);
> > -	}
> > +	if (integer64 < INT_MIN || integer64 > UINT_MAX ||
> > +	    fract64 < INT_MIN || fract64 > UINT_MAX)
> > +		return -ERANGE;
> >  
> > -	if (str[0] == '-') {
> > -		negative = true;
> > -		str++;
> > -	} else if (str[0] == '+') {
> > -		str++;
> > -	}
> > -
> > -	while (*str) {
> > -		if ('0' <= *str && *str <= '9') {
> > -			if (integer_part) {
> > -				i = i * 10 + *str - '0';
> > -			} else {
> > -				f += fract_mult * (*str - '0');
> > -				fract_mult /= 10;
> > -			}
> > -		} else if (*str == '\n') {
> > -			if (*(str + 1) == '\0')
> > -				break;
> > -			return -EINVAL;
> > -		} else if (!strncmp(str, " dB", sizeof(" dB") - 1) && scale_db) {
> > -			/* Ignore the dB suffix */
> > -			str += sizeof(" dB") - 1;
> > -			continue;
> > -		} else if (!strncmp(str, "dB", sizeof("dB") - 1) && scale_db) {
> > -			/* Ignore the dB suffix */
> > -			str += sizeof("dB") - 1;
> > -			continue;
> > -		} else if (*str == '.' && integer_part) {
> > -			integer_part = false;
> > -		} else {
> > -			return -EINVAL;
> > -		}
> > -		str++;
> > -	}
> > -
> > -	if (negative) {
> > -		if (i)
> > -			i = -i;
> > -		else
> > -			f = -f;
> > -	}
> > -
> > -	*integer = i;
> > -	*fract = f;
> > +	*integer = integer64;
> > +	*fract = fract64;
> 
> Hmmm, aren't we truncating the values? They are still int pointers...

Yes, truncation happens here. integer64 and fract64 are range checked
before this assignment.
 
-- 
Kind regards,

Rodrigo Alencar