Message-ID: <aXeQ4LHQOdy3K1tA@smile.fi.intel.com>
Date: Mon, 26 Jan 2026 18:05:52 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Rodrigo Alencar <455.rodrigo.alencar@...il.com>
Cc: rodrigo.alencar@...log.com, linux-kernel@...r.kernel.org,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-doc@...r.kernel.org, Jonathan Cameron <jic23@...nel.org>,
	David Lechner <dlechner@...libre.com>,
	Andy Shevchenko <andy@...nel.org>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH v5 2/8] iio: core: add fixed point parsing with 64-bit
 parts

On Mon, Jan 26, 2026 at 03:20:03PM +0000, Rodrigo Alencar wrote:
> On 26/01/26 04:53PM, Andy Shevchenko wrote:
> > On Mon, Jan 26, 2026 at 02:26:20PM +0000, Rodrigo Alencar wrote:
> > > On 26/01/26 03:35PM, Andy Shevchenko wrote:
> > > > On Mon, Jan 26, 2026 at 12:42:53PM +0000, Rodrigo Alencar wrote:
> > > > > On 26/01/26 01:49PM, Andy Shevchenko wrote:
> > > > > > On Fri, Jan 23, 2026 at 03:53:07PM +0000, Rodrigo Alencar via B4 Relay wrote:

...

> > > > > > > +static int __iio_str_to_fixpoint64(const char *str, u64 fract_mult,
> > > > > > > +				   s64 *integer, s64 *fract, bool scale_db)
> > > > > > > +{
> > > > > > > +	u64 i = 0, f = 0;
> > > > > > > +	char *end;
> > > > > > > +	int digit_count, precision = ffs(fract_mult);
> > > > > > > +	bool negative = false;
> > > > > > > +
> > > > > > > +	if (str[0] == '-') {
> > > > > > > +		negative = true;
> > > > > > > +		str++;
> > > > > > > +	} else if (str[0] == '+') {
> > > > > > > +		str++;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	i = simple_strtoull(str, &end, 10);
> > > > > > > +	digit_count = end - str;
> > > > > > > +	if (digit_count > 20)
> > > > > > > +		return -EINVAL;
> > > > > > 
> > > > > > Not really. If we are talking about decimal (only) cases we need to also count
> > > > > > leading 0:s.
> > > > > > 
> > > > > > 0000000000000000000000000000000025 is still 25, no overflow.
> > > > > > 
> > > > > > That's why I recommend to have a helper, maybe for now locally here, like
> > > > > > 
> > > > > > int safe_strtoull(..., unsigned long long *res)
> > > > > > {
> > > > > > 	...
> > > > > > }
> > > > > 
> > > > > Are you suggesting to not use simple_strtoull then?
> > > > 
> > > > Nope, I suggest to do an additional step before checking for the range.
> > > 
> > > You mean, conditionally skip leading 0's when parsing the integer part?
> > > e.g.
> > > 
> > > /*function entry and arg check */
> > > while(*str == '\0')
> > > 	str++;
> > > /* then call simple_strtoull() */
> > 
> > Not skipping, but counting them.
> > 
> > > simple_strtoull() is not overflow-safe,
> > 
> > Yes, I know. That's why all these additional checks are required,
> > 
> > > as it does not use
> > > check_mul_overflow() or check_add_overflow(), only checking the
> > > amount of digits is not enough.
> > 
> > Why? Can you elaborate how checking amount of digits is different to
> > check_mul_overflow()?
> 
> consider U64_MAX = 18_446_744_073_709_551_615 as the limit:
> - 19_000_000_000_000_000_000 contains the same amount of digits but overflows.
> - 18_446_744_073_710_000_000 contains the same amount of digits but overflows.

Yes, the first digits give up to 3 additional bits on top of 64-bit.

> to catch those cases, we need to check for the overflow, everytime we read a
> character and accumulate:
> 
> u64 acc;
> 
> while(isdigit(*str))
> 	if (check_mul_overflow(acc, 10, &acc) ||
> 	    check_add_overflow(acc, *str - '0', &acc))
> 		return -EOVERFLOW;
> 
> *res = acc;
> 
> acc can get weird results if not checked. 
> 
> > 
> > > Previous implementation of fixpoint parsing didn't care about that.
> > 
> > Do we have test cases for the current implementation?
> 
> No, I am adding a kunit test in this patch.
> 
> > > > > Understood, leading zeros can be ignored only when parsing the integer 
> > > > > part. Also, would be nice to have truncation of the fractional part
> > > > > while doing the parsing. How about:
> > > > > 
> > > > > static int iio_safe_strtoull(const char *str, const char **end,
> > > > > 			     size_t max_chars, u64 *res)
> > > > 
> > > > > - max_chars = 0: ignores leading 0's and process all digits
> > > > > - max_chars > 0: process only initial max_chars digits and ignores the rest
> > > > 
> > > > I'm not sure why we would need that. It should parse the whole line until the
> > > > first invalid character or overflow.
> > > 
> > > "process all digits" and "ignores the rest" would be for digits only, so it
> > > would stop until the first invalid character is found. I suppose proper
> > > overflow check is implemented with check_mul_overflow() and check_add_overflow(),
> > 
> > I don't see the need. Amount of digits defines the order of the number (in
> > power-of-ten).
> > 
> > > while iterating over the characters and accumulating the value.
> > 
> > The problem that you can refer to is the corner case when the first
> > (most significant digit(s)) are already give an overflow while being
> > inside the allowed length. But it also can be checked.
> > 
> 
> Yes. Not sure how to do that without checking every digit again.
> 
> > 
> > The benefit of simple_strto*() over kstrto*() that they do not require
> > a temporary buffer and work over constant data (always).
> 
> Agreed.
>  
> > If you see a way how to refactor lib/kstrtox.c and lib/vsprintf.c to have
> > an implementation there directly that may operate over constant buffers,
> > I will be glad to help with it. That would be good for existing cases,
> > such as Intel QAT driver, and any newcomers. I actually don't know why
> > the heck kstrto*() were made against non-constant buffers. Perhaps to
> > avoid this 'end' parameter...
> 
> That would be out of the scope of this patch.

Hmm... But if not doing that we will end up with a code duplication in one or
another form. The whole purpose of the patch is to avoid duplication.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists