Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype

[Rodrigo Alencar]

On 26/04/01 02:22PM, Petr Mladek wrote:
> On Mon 2026-03-30 13:49:48, Rodrigo Alencar wrote:
> > On 26/03/27 03:17PM, Rodrigo Alencar wrote:
> > > On 26/03/27 12:21PM, Andy Shevchenko wrote:
> > > > On Fri, Mar 27, 2026 at 10:11:56AM +0000, Rodrigo Alencar wrote:
> > > > > On 26/03/27 11:17AM, Andy Shevchenko wrote:
> > > > > > On Fri, Mar 27, 2026 at 09:45:17AM +0100, Petr Mladek wrote:
> > > > > > > On Fri 2026-03-20 16:27:27, Rodrigo Alencar via B4 Relay wrote:
> > 
> > ...
> > 
> > > > > > Maybe we want to have kstrtof32() and kstrtof64() for these two cases?
> > > > > > 
> > > > > > With that we will always consider the fraction part as 32- or 64-bit,
> > > > > > imply floor() on the fraction for the sake of simplicity and require
> > > > > > it to be NUL-terminated with possible trailing '\n'.
> > > > > 
> > > > > I think this is a good idea, but calling it float or fixed point itself
> > > > > is a bit confusing as float often refers to the IEEE 754 standard and
> > > > > fixed point types is often expressed in Q-format.
> > > > 
> > > > Yeah... I am lack of better naming.
> > > 
> > > decimals is the name, but they are often represented as:
> > > 
> > > 	DECIMAL = INT * 10^X + FRAC
> > > 
> > > in a single 64-bit number, which would be fine for my end use case.
> > > However IIO decimal fixed point parsing is out there for quite some time a
> > > lot of drivers use that. The interface often relies on breaking parsed values
> > > into an integer array (for standard attributes int val and int val2 are expected).
> > 
> > Thinking about this again and in IIO drivers we end up doing something like:
> > 
> > val64 = (u64)val * MICRO + val2;
> > 
> > so that drivers often work with scaled versions of the decimal value.
> > then, would it make sense to have a function that already outputs such value?
> > That would allow to have more freedom over the 64-bit split between integer
> > and fractional parts.
> > As a draft:
> 
> My understanding is that you want to allow parsing frequencies
> in the range from microHz to GHz.

Correct, the ABI requires the values in Hz, and I would like to support
micro Hz resolution, so that 10 GHz can be represensted as:

	10000000000.000000 
 
> So, you might want to support input in simple float numbers
> with some precision, for example, 1.2GHz, 0.345Hz, ...
> 
> By simple, I mean that there is no x10^3 or so.
> 
> > static int _kstrtodec64(const char *s, unsigned int scale, u64 *res)
> 
> I would personally change this to something like:
> 
>   static int _unit_float_ktstrtodec64(const char *s, unsigned int precision, u64 *res, char **unit)

I don't really need "unit" for my specific use case, IIUC this pattern is not
something to be handled by kstrto*(), because those function should requires NUL
termination. I am not sure why is that, but I like the idea of returning a
const char* pointer to the end of the conversion (that was the whole point of
having something like kstrntoull())
 
> It would allow to read float number in the the format XXXX.YYYYunit,
> for example 1.2Ghz
> 
> , where:
> 
>   + _unit_ means that it might set @unit pointer which point to the unit
>     string right after the number part.

as mentioned, the units is defined in the ABI, so this part is not really needed.

>   + _float_ means that it will be able to read float numbers

its a decimal fixed precision, decimal point should not float.

> 
>   + @precisions parameter defines the number of digits accepted
>     after the radix point. It is also used as multiplier for scaling
>     the output number.

precision != scale, for this case we have a fixed precision of 64-bits.
while scale is passed as parameter.

Reference:
	https://www.ibm.com/docs/ro/informix-servers/12.10.0?topic=types-decimalps-data 

> 
>   + @res is pointer to the read number multiplied by the given
>     @precision
> 
>   + @unit will be set to string after the number
> 
> For example:
> 
>   + s="1.2GHz", precision=3 will result in *res=1200, *unit="GHz"
>   + s="0.0100004", precision=3 will result in *res=10, *unit=""
>   + s=1.234567GHz, precision=3 will result in *res=1235, *unit="GHz"
> 
> Note that the result is rounded in the last example.
> 
> The function might be used like simple_strtoull() in memparse(),
> see lib/cmdline.c. Which is able to read the given size in B
> and handle various units like kB, GB, ...

As dicussed above, scaling the value based on the units is not my use case.

> 
> > {
> > 	u64 _res = 0, _frac = 0;
> > 	unsigned int rv;
> > 
> > 	if (*s != '.') {
> > 		rv = _parse_integer(s, 10, &_res);
> > 		if (rv & KSTRTOX_OVERFLOW)
> > 			return -ERANGE;
> > 		if (rv == 0)
> > 			return -EINVAL;
> > 		s += rv;
> > 	}
> > 
> > 	if (*s == '.') {
> > 		s++;
> > 		rv = _parse_integer_limit(s, 10, &_frac, scale);
> > 		if (rv & KSTRTOX_OVERFLOW)
> > 			return -ERANGE;
> > 		if (rv == 0)
> > 			return -EINVAL;
> > 		s += rv;
> > 		if (rv < scale)
> > 			_frac *= int_pow(10, scale - rv);
> > 		while (isdigit(*s)) /* truncate */
> > 			s++;
> 
> We might/should use the first digit to round the _frac.

flooring should not be a problem if it is documented like that.
I suppose we cannot afford to carry over all roundings from
subsequent digits. If so, we would be parsing it all and use
div64 which I would like avoid.

> > 	}
> > 
> > 	if (*s == '\n')
> > 		s++;
> > 	if (*s)
> > 		return -EINVAL;
> 
> I would omit this. Instead I would set @unit pointer so that the
> caller might handle units defined after the number.

I understand that this is the whole point of creating a kstrto*()
function.

> > 	if (check_mul_overflow(_res, int_pow(10, scale), &_res) ||
> > 	    check_add_overflow(_res, _frac, &_res))
> > 		return -ERANGE;
> > 
> > 	*res = _res;
> > 	return 0;
> > }
> 
> Otherwise, this approach looks sensible to me. IMHO, some generic
> API for reading numbers with misc units should be usable in more
> situations. And it would make the kernel interface more user
> friendly.
> 
> Of course, we must not over-engineer it. But the above does not
> look much more complex than we already have.

I really appreciate your time looking into this, thanks.
 
-- 
Kind regards,

Rodrigo Alencar