* Re: [PATCH v9 0/9] ADF41513/ADF41510 PLL frequency synthesizers [not found] <20260320-adf41513-iio-driver-v9-0-132f0d076374@analog.com> @ 2026-03-22 11:33 ` Jonathan Cameron [not found] ` <20260320-adf41513-iio-driver-v9-2-132f0d076374@analog.com> 1 sibling, 0 replies; 15+ messages in thread From: Jonathan Cameron @ 2026-03-22 11:33 UTC (permalink / raw) To: Rodrigo Alencar via B4 Relay Cc: rodrigo.alencar, linux-kernel, linux-iio, devicetree, linux-doc, David Lechner, Andy Shevchenko, Lars-Peter Clausen, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton, Petr Mladek, Steven Rostedt, Andy Shevchenko, Rasmus Villemoes, Sergey Senozhatsky, Shuah Khan, Krzysztof Kozlowski, Andy Shevchenko On Fri, 20 Mar 2026 16:27:25 +0000 Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@kernel.org> wrote: > This patch series adds support for the Analog Devices ADF41513 and ADF41510 > ultralow noise PLL frequency synthesizers. These devices are designed for > implementing local oscillators (LOs) in high-frequency applications. > The ADF41513 covers frequencies from 1 GHz to 26.5 GHz, while the ADF41510 > operates from 1 GHz to 10 GHz. > > Key features supported by this driver: > - Integer-N and fractional-N operation modes > - High maximum PFD frequency (250 MHz integer-N, 125 MHz fractional-N) > - 25-bit fixed modulus or 49-bit variable modulus fractional modes > - Digital lock detect functionality > - Phase resync capability for consistent output phase > - Load Enable vs Reference signal syncronization > > The series includes: > 1. PLL driver implementation > 2. Device tree bindings documentation > 3. IIO ABI documentation > > Signed-off-by: Rodrigo Alencar <rodrigo.alencar@analog.com> > --- > Changes in v9: > - Expose simple_strntoull() in a safer prototype instead of new kstrntoull() I'm leaving this part to the experts. Other than that aspect, I took another look through the driver and all looks good to me. Thanks, Jonathan ^ permalink raw reply [flat|nested] 15+ messages in thread
[parent not found: <20260320-adf41513-iio-driver-v9-2-132f0d076374@analog.com>]
* Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype [not found] ` <20260320-adf41513-iio-driver-v9-2-132f0d076374@analog.com> @ 2026-03-27 8:45 ` Petr Mladek 2026-03-27 9:17 ` Andy Shevchenko 2026-03-27 9:24 ` Rodrigo Alencar 0 siblings, 2 replies; 15+ messages in thread From: Petr Mladek @ 2026-03-27 8:45 UTC (permalink / raw) To: rodrigo.alencar Cc: linux-kernel, linux-iio, devicetree, linux-doc, Jonathan Cameron, David Lechner, Andy Shevchenko, Lars-Peter Clausen, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton, Steven Rostedt, Andy Shevchenko, Rasmus Villemoes, Sergey Senozhatsky, Shuah Khan On Fri 2026-03-20 16:27:27, Rodrigo Alencar via B4 Relay wrote: > From: Rodrigo Alencar <rodrigo.alencar@analog.com> > > Expose simple_strntoull(), by addressing its FIXME, i.e. its prototype is > slightly changed so that -ERANGE or -EINVAL can be evaluated by the user. > Flow of the function is not changed and error value is returned in the > end. Unsafe internal wrapper is created to reduce amount of changes. > > --- a/include/linux/kstrtox.h > +++ b/include/linux/kstrtox.h > @@ -148,4 +148,8 @@ extern long simple_strtol(const char *,char **,unsigned int); > extern unsigned long long simple_strtoull(const char *,char **,unsigned int); > extern long long simple_strtoll(const char *,char **,unsigned int); > > +extern ssize_t __must_check simple_strntoull(const char *startp, const char **endp, > + unsigned int base, size_t max_chars, > + unsigned long long *res); Sigh, naming is hard. I personally find it a bit confusing that the name is too similar to the unsafe API. IMHO, the semantic of the new API is closer to kstrtoull(). It just limits the size, so I would call it kstrntoull(). Also I would use int as the return parameter, see below. > #endif /* _LINUX_KSTRTOX_H */ > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 800b8ac49f53..6fb880f4013b 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -75,25 +75,66 @@ enum hash_pointers_policy { > }; > static enum hash_pointers_policy hash_pointers_mode __initdata; > > +/** > + * simple_strntoull - convert a string to an unsigned long long with a character limit > + * > + * @startp: The start of the string > + * @endp: A pointer to the end of the parsed string will be placed here I would write: * @endp: A pointer to the end of the parsed string (output) > + * @base: The number base to use > + * @max_chars: The maximum number of characters to parse > + * @res: Where to write the result of the conversion on success Nit: I would omit "on success" *res value is set to 0 on failure. Instead, I would write: * @res: Result of the conversion (output) > + * > + * Returns amount of processed characters on success, -ERANGE on overflow and > + * -EINVAL on parsing error. > + */ > noinline > -static unsigned long long simple_strntoull(const char *startp, char **endp, unsigned int base, size_t max_chars) > +ssize_t simple_strntoull(const char *startp, const char **endp, > + unsigned int base, size_t max_chars, > + unsigned long long *res) It might be enoungh to use "int" for the return value. The number of proceed characters is pretty limited by definition. And it would be similar to vsnprintf(), kstrtoull(), ... I guess that you wanted to match the "size_t max_chars" parameter. It makes some sense as well. Please, use "int" especially if we agreed to call the new API kstrntoull(). > { > const char *cp; > - unsigned long long result = 0ULL; > size_t prefix_chars; > unsigned int rv; > + ssize_t ret; > > cp = _parse_integer_fixup_radix(startp, &base); > prefix_chars = cp - startp; > if (prefix_chars < max_chars) { > - rv = _parse_integer_limit(cp, base, &result, max_chars - prefix_chars); > - /* FIXME */ > + rv = _parse_integer_limit(cp, base, res, max_chars - prefix_chars); > + if (rv & KSTRTOX_OVERFLOW) > + ret = -ERANGE; > + else if (rv == 0) > + ret = -EINVAL; > + else > + ret = rv + prefix_chars; > cp += (rv & ~KSTRTOX_OVERFLOW); > } else { > /* Field too short for prefix + digit, skip over without converting */ > cp = startp + max_chars; > + ret = -EINVAL; > + *res = 0ULL; > } > > + if (endp) > + *endp = cp; > + > + return ret; > +} > +EXPORT_SYMBOL(simple_strntoull); > + > +/* unsafe_strntoull ignores simple_strntoull() return value and endp const qualifier */ > +inline > +static unsigned long long unsafe_strntoull(const char *startp, char **endp, > + unsigned int base, size_t max_chars) > +{ > + unsigned long long result; > + const char *cp; > + > +#pragma GCC diagnostic push > +#pragma GCC diagnostic ignored "-Wunused-result" > + simple_strntoull(startp, &cp, base, max_chars, &result); > +#pragma GCC diagnostic pop > + > if (endp) > *endp = (char *)cp; IMHO, we do not need local "cp". We could simply pass the endp to the new simple_strntoull. Or do I miss anything? Best Regards, Petr ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype 2026-03-27 8:45 ` [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype Petr Mladek @ 2026-03-27 9:17 ` Andy Shevchenko 2026-03-27 10:11 ` Rodrigo Alencar 2026-03-27 10:44 ` David Laight 2026-03-27 9:24 ` Rodrigo Alencar 1 sibling, 2 replies; 15+ messages in thread From: Andy Shevchenko @ 2026-03-27 9:17 UTC (permalink / raw) To: Petr Mladek Cc: rodrigo.alencar, linux-kernel, linux-iio, devicetree, linux-doc, Jonathan Cameron, David Lechner, Andy Shevchenko, Lars-Peter Clausen, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton, Steven Rostedt, Rasmus Villemoes, Sergey Senozhatsky, Shuah Khan 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: ... > > +extern ssize_t __must_check simple_strntoull(const char *startp, const char **endp, > > + unsigned int base, size_t max_chars, > > + unsigned long long *res); > > Sigh, naming is hard. I personally find it a bit confusing that the > name is too similar to the unsafe API. > > IMHO, the semantic of the new API is closer to kstrtoull(). > It just limits the size, so I would call it kstrntoull(). It's not. kstrto*() quite strict about the input, this one is actually relaxed variant, so I wouldn't mix these two groups. > Also I would use int as the return parameter, see below. ... TBH, I am skeptical about this approach. My main objection is max_chars parameter. If we want to limit the input strictly to the given number of characters, we have to copy the string and then just use kstrto*() in a normal way. The whole idea of that parameter is to be able to parse the fractional part of the float number as 'iiiii.fffff', where 'i' is for integer part, and 'f' for the fractional. Since we have *endp, we may simply check that. In case if we want to parse only, say, 6 digits and input is longer there are a few options (in my personal preferences, the first is the better): - consider the input invalid - parse it as is up to the maximum and then do ceil() or floor() on top of that - copy only necessary amount of the (sub)string and parse that. The problem with precision is that we need to also consider floor() or ceil() and I don't think this should be burden of the library as it's individual preference of each of the callers (users). At least for the starter, we will see if it's only one approach is used, we may incorporate it into the library code. The easiest way out is to just consider the input invalid if it overflows the given type (s32 or s64). But we need to have an agreement what will be the representation of the fixed-width float numbers in the kernel? Currently IIO uses struct float // name is crafted for simplicity { int integer; int fraction; } This parser wants AFAIU to have at the end of the day something like struct float { s64 integer; s64 fraction; } but also wants to have the fraction part be limited in some cases to s32 or so: struct float { s64 integer; s32 fraction; // precision may be lost if input is longer } 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'. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype 2026-03-27 9:17 ` Andy Shevchenko @ 2026-03-27 10:11 ` Rodrigo Alencar 2026-03-27 10:21 ` Andy Shevchenko 2026-03-27 10:44 ` David Laight 1 sibling, 1 reply; 15+ messages in thread From: Rodrigo Alencar @ 2026-03-27 10:11 UTC (permalink / raw) To: Andy Shevchenko, Petr Mladek Cc: rodrigo.alencar, linux-kernel, linux-iio, devicetree, linux-doc, Jonathan Cameron, David Lechner, Andy Shevchenko, Lars-Peter Clausen, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton, Steven Rostedt, Rasmus Villemoes, Sergey Senozhatsky, Shuah Khan 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: > > ... > > > > +extern ssize_t __must_check simple_strntoull(const char *startp, const char **endp, > > > + unsigned int base, size_t max_chars, > > > + unsigned long long *res); > > > > Sigh, naming is hard. I personally find it a bit confusing that the > > name is too similar to the unsafe API. > > > > IMHO, the semantic of the new API is closer to kstrtoull(). > > It just limits the size, so I would call it kstrntoull(). > > It's not. kstrto*() quite strict about the input, this one is actually relaxed > variant, so I wouldn't mix these two groups. > > > Also I would use int as the return parameter, see below. > > ... > > TBH, I am skeptical about this approach. My main objection is max_chars > parameter. If we want to limit the input strictly to the given number of > characters, we have to copy the string and then just use kstrto*() in a normal > way. The whole idea of that parameter is to be able to parse the fractional > part of the float number as 'iiiii.fffff', where 'i' is for integer part, and > 'f' for the fractional. Since we have *endp, we may simply check that. A max_chars would not be only useful for that. It can prevent out-of-bounds reads when the input isn't NUL-terminated (like buffers, file chunks, network packets, memory-mapped data, ....). Even if there is a NUL later in memory, a regular strtoull() function may consume characters that are outside the field one intends to parse. > In case if we want to parse only, say, 6 digits and input is longer there are > a few options (in my personal preferences, the first is the better): > - consider the input invalid > - parse it as is up to the maximum and then do ceil() or floor() on top of that > - copy only necessary amount of the (sub)string and parse that. Yes, my use case is the fixed point parsing, but I suppose we are implementing things here for reuse. Also, the default behavior of the previous fixed point parsing in IIO is flooring the result, which leads to the same result as ignoring further digits. > The problem with precision is that we need to also consider floor() or ceil() > and I don't think this should be burden of the library as it's individual > preference of each of the callers (users). At least for the starter, we will > see if it's only one approach is used, we may incorporate it into the library > code. > > The easiest way out is to just consider the input invalid if it overflows the > given type (s32 or s64). > > But we need to have an agreement what will be the representation of the > fixed-width float numbers in the kernel? Currently IIO uses > struct float // name is crafted for simplicity > { > int integer; > int fraction; > } Yes, but to represent things like that, an assumption is made to the precision that "fraction" carries. > > This parser wants AFAIU to have at the end of the day something like > > struct float > { > s64 integer; > s64 fraction; > } > > but also wants to have the fraction part be limited in some cases to s32 > or so: > > struct float > { > s64 integer; > s32 fraction; // precision may be lost if input is longer > } > > 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. -- Kind regards, Rodrigo Alencar ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype 2026-03-27 10:11 ` Rodrigo Alencar @ 2026-03-27 10:21 ` Andy Shevchenko 2026-03-27 15:17 ` Rodrigo Alencar 0 siblings, 1 reply; 15+ messages in thread From: Andy Shevchenko @ 2026-03-27 10:21 UTC (permalink / raw) To: Rodrigo Alencar Cc: Petr Mladek, rodrigo.alencar, linux-kernel, linux-iio, devicetree, linux-doc, Jonathan Cameron, David Lechner, Andy Shevchenko, Lars-Peter Clausen, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton, Steven Rostedt, Rasmus Villemoes, Sergey Senozhatsky, Shuah Khan 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: ... > > > > +extern ssize_t __must_check simple_strntoull(const char *startp, const char **endp, > > > > + unsigned int base, size_t max_chars, > > > > + unsigned long long *res); > > > > > > Sigh, naming is hard. I personally find it a bit confusing that the > > > name is too similar to the unsafe API. > > > > > > IMHO, the semantic of the new API is closer to kstrtoull(). > > > It just limits the size, so I would call it kstrntoull(). > > > > It's not. kstrto*() quite strict about the input, this one is actually relaxed > > variant, so I wouldn't mix these two groups. > > > > > Also I would use int as the return parameter, see below. ... > > TBH, I am skeptical about this approach. My main objection is max_chars > > parameter. If we want to limit the input strictly to the given number of > > characters, we have to copy the string and then just use kstrto*() in a normal > > way. The whole idea of that parameter is to be able to parse the fractional > > part of the float number as 'iiiii.fffff', where 'i' is for integer part, and > > 'f' for the fractional. Since we have *endp, we may simply check that. > > A max_chars would not be only useful for that. It can prevent out-of-bounds > reads when the input isn't NUL-terminated (like buffers, file chunks, > network packets, memory-mapped data, ....). Even if there is a NUL later in > memory, a regular strtoull() function may consume characters that are outside > the field one intends to parse. Okay, but is it the current case or just an attempt to solve the problem that doesn't exist (yet)? > > In case if we want to parse only, say, 6 digits and input is longer there are > > a few options (in my personal preferences, the first is the better): > > - consider the input invalid > > - parse it as is up to the maximum and then do ceil() or floor() on top of that > > - copy only necessary amount of the (sub)string and parse that. > > Yes, my use case is the fixed point parsing, but I suppose we are implementing > things here for reuse. Yes, I'm full for reuse, but I want to have it balanced between complexity, existing use cases and possible reuse in the future. > Also, the default behavior of the previous fixed point > parsing in IIO is flooring the result, which leads to the same result as > ignoring further digits. Correct, I also lean to implying floor() (as you can read below). > > The problem with precision is that we need to also consider floor() or ceil() > > and I don't think this should be burden of the library as it's individual > > preference of each of the callers (users). At least for the starter, we will > > see if it's only one approach is used, we may incorporate it into the library > > code. > > > > The easiest way out is to just consider the input invalid if it overflows the > > given type (s32 or s64). > > > > But we need to have an agreement what will be the representation of the > > fixed-width float numbers in the kernel? Currently IIO uses > > struct float // name is crafted for simplicity > > { > > int integer; > > int fraction; > > } > > Yes, but to represent things like that, an assumption is made to the precision that > "fraction" carries. Correct. > > This parser wants AFAIU to have at the end of the day something like > > > > struct float > > { > > s64 integer; > > s64 fraction; > > } > > > > but also wants to have the fraction part be limited in some cases to s32 > > or so: > > > > struct float > > { > > s64 integer; > > s32 fraction; // precision may be lost if input is longer > > } > > > > 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. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype 2026-03-27 10:21 ` Andy Shevchenko @ 2026-03-27 15:17 ` Rodrigo Alencar 2026-03-30 12:49 ` Rodrigo Alencar 0 siblings, 1 reply; 15+ messages in thread From: Rodrigo Alencar @ 2026-03-27 15:17 UTC (permalink / raw) To: Andy Shevchenko, Rodrigo Alencar Cc: Petr Mladek, rodrigo.alencar, linux-kernel, linux-iio, devicetree, linux-doc, Jonathan Cameron, David Lechner, Andy Shevchenko, Lars-Peter Clausen, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton, Steven Rostedt, Rasmus Villemoes, Sergey Senozhatsky, Shuah Khan 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: > > ... > > > > > > +extern ssize_t __must_check simple_strntoull(const char *startp, const char **endp, > > > > > + unsigned int base, size_t max_chars, > > > > > + unsigned long long *res); > > > > > > > > Sigh, naming is hard. I personally find it a bit confusing that the > > > > name is too similar to the unsafe API. > > > > > > > > IMHO, the semantic of the new API is closer to kstrtoull(). > > > > It just limits the size, so I would call it kstrntoull(). > > > > > > It's not. kstrto*() quite strict about the input, this one is actually relaxed > > > variant, so I wouldn't mix these two groups. > > > > > > > Also I would use int as the return parameter, see below. > > ... > > > > TBH, I am skeptical about this approach. My main objection is max_chars > > > parameter. If we want to limit the input strictly to the given number of > > > characters, we have to copy the string and then just use kstrto*() in a normal > > > way. The whole idea of that parameter is to be able to parse the fractional > > > part of the float number as 'iiiii.fffff', where 'i' is for integer part, and > > > 'f' for the fractional. Since we have *endp, we may simply check that. > > > > A max_chars would not be only useful for that. It can prevent out-of-bounds > > reads when the input isn't NUL-terminated (like buffers, file chunks, > > network packets, memory-mapped data, ....). Even if there is a NUL later in > > memory, a regular strtoull() function may consume characters that are outside > > the field one intends to parse. > > Okay, but is it the current case or just an attempt to solve the problem that > doesn't exist (yet)? The current case can be seen as such. Copying the string and use regular ksrto*() requires an unecessary scan of string from the user side, which is something that _parse_integer_limit() already does, mostly because it checks for digits and stops at any non-digit character. In the IIO case, we also want control over the consumed characters because there are weird terminations like "dB", so having an implementation like this ends up with a cleaner sequence of steps. > > > In case if we want to parse only, say, 6 digits and input is longer there are > > > a few options (in my personal preferences, the first is the better): > > > - consider the input invalid > > > - parse it as is up to the maximum and then do ceil() or floor() on top of that > > > - copy only necessary amount of the (sub)string and parse that. > > > > Yes, my use case is the fixed point parsing, but I suppose we are implementing > > things here for reuse. > > Yes, I'm full for reuse, but I want to have it balanced between complexity, > existing use cases and possible reuse in the future. Not seeing complexity here as in this case I am just exposing something that already exists! No need for a completely different implementation. I just want to get an agreement on the naming and interface prototype. Bringing back the discussion again just because I suppose Petr havent even seen the v8 of this patch series. If kstrtox.h is the right place for this, kstrntoull() sounds like ideal. Specially because simple_strto*() is already labeled as unsafe and kstrnto*() != kstrto*(). > > Also, the default behavior of the previous fixed point > > parsing in IIO is flooring the result, which leads to the same result as > > ignoring further digits. > > Correct, I also lean to implying floor() (as you can read below). > > > > The problem with precision is that we need to also consider floor() or ceil() > > > and I don't think this should be burden of the library as it's individual > > > preference of each of the callers (users). At least for the starter, we will > > > see if it's only one approach is used, we may incorporate it into the library > > > code. > > > > > > The easiest way out is to just consider the input invalid if it overflows the > > > given type (s32 or s64). > > > > > > But we need to have an agreement what will be the representation of the > > > fixed-width float numbers in the kernel? Currently IIO uses > > > struct float // name is crafted for simplicity > > > { > > > int integer; > > > int fraction; > > > } > > > > Yes, but to represent things like that, an assumption is made to the precision that > > "fraction" carries. > > Correct. > > > > This parser wants AFAIU to have at the end of the day something like > > > > > > struct float > > > { > > > s64 integer; > > > s64 fraction; > > > } > > > > > > but also wants to have the fraction part be limited in some cases to s32 > > > or so: > > > > > > struct float > > > { > > > s64 integer; > > > s32 fraction; // precision may be lost if input is longer > > > } > > > > > > 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). -- Kind regards, Rodrigo Alencar ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype 2026-03-27 15:17 ` Rodrigo Alencar @ 2026-03-30 12:49 ` Rodrigo Alencar 2026-03-31 13:01 ` Rodrigo Alencar 2026-04-01 12:22 ` Petr Mladek 0 siblings, 2 replies; 15+ messages in thread From: Rodrigo Alencar @ 2026-03-30 12:49 UTC (permalink / raw) To: Rodrigo Alencar, Andy Shevchenko Cc: Petr Mladek, rodrigo.alencar, linux-kernel, linux-iio, devicetree, linux-doc, Jonathan Cameron, David Lechner, Andy Shevchenko, Lars-Peter Clausen, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton, Steven Rostedt, Rasmus Villemoes, Sergey Senozhatsky, Shuah Khan 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: static int _kstrtodec64(const char *s, unsigned int scale, u64 *res) { 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++; } if (*s == '\n') s++; if (*s) return -EINVAL; if (check_mul_overflow(_res, int_pow(10, scale), &_res) || check_add_overflow(_res, _frac, &_res)) return -ERANGE; *res = _res; return 0; } noinline int kstrtoudec64(const char *s, unsigned int scale, u64 *res) { if (s[0] == '+') s++; return _kstrtodec64(s, scale, res); } EXPORT_SYMBOL(kstrtoudec64); noinline int kstrtosdec64(const char *s, unsigned int scale, s64 *res) { u64 tmp; int rv; if (s[0] == '-') { rv = _kstrtodec64(s + 1, scale, &tmp); if (rv < 0) return rv; if ((s64)-tmp > 0) return -ERANGE; *res = -tmp; } else { rv = kstrtoudec64(s, scale, &tmp); if (rv < 0) return rv; if ((s64)tmp < 0) return -ERANGE; *res = tmp; } return 0; } EXPORT_SYMBOL(kstrtosdec64); e.g., kstrtosdec64() or kstrtoudec64() parses "3.1415" with scale 3 into 3141 -- Kind regards, Rodrigo Alencar ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype 2026-03-30 12:49 ` Rodrigo Alencar @ 2026-03-31 13:01 ` Rodrigo Alencar 2026-04-12 15:05 ` Jonathan Cameron 2026-04-01 12:22 ` Petr Mladek 1 sibling, 1 reply; 15+ messages in thread From: Rodrigo Alencar @ 2026-03-31 13:01 UTC (permalink / raw) To: Rodrigo Alencar, Andy Shevchenko Cc: Petr Mladek, rodrigo.alencar, linux-kernel, linux-iio, devicetree, linux-doc, Jonathan Cameron, David Lechner, Andy Shevchenko, Lars-Peter Clausen, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton, Steven Rostedt, Rasmus Villemoes, Sergey Senozhatsky, Shuah Khan On 26/03/30 01:49PM, 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: > > static int _kstrtodec64(const char *s, unsigned int scale, u64 *res) > { > 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++; > } > > if (*s == '\n') > s++; > if (*s) > return -EINVAL; > > if (check_mul_overflow(_res, int_pow(10, scale), &_res) || > check_add_overflow(_res, _frac, &_res)) > return -ERANGE; > > *res = _res; > return 0; > } > > noinline > int kstrtoudec64(const char *s, unsigned int scale, u64 *res) > { > if (s[0] == '+') > s++; > return _kstrtodec64(s, scale, res); > } > EXPORT_SYMBOL(kstrtoudec64); > > noinline > int kstrtosdec64(const char *s, unsigned int scale, s64 *res) > { > u64 tmp; > int rv; > > if (s[0] == '-') { > rv = _kstrtodec64(s + 1, scale, &tmp); > if (rv < 0) > return rv; > if ((s64)-tmp > 0) > return -ERANGE; > *res = -tmp; > } else { > rv = kstrtoudec64(s, scale, &tmp); > if (rv < 0) > return rv; > if ((s64)tmp < 0) > return -ERANGE; > *res = tmp; > } > return 0; > } > EXPORT_SYMBOL(kstrtosdec64); > > e.g., kstrtosdec64() or kstrtoudec64() parses "3.1415" with scale 3 into 3141 Hi Jonathan, developing more on that, I wouldn't need to create a iio_str_to_fixpoint64(), what do you think on new format types: #define IIO_VAL_DECIMAL64_1 101 #define IIO_VAL_DECIMAL64_2 102 #define IIO_VAL_DECIMAL64_3 103 #define IIO_VAL_DECIMAL64_4 104 #define IIO_VAL_DECIMAL64_5 105 #define IIO_VAL_DECIMAL64_6 106 #define IIO_VAL_DECIMAL64_7 107 #define IIO_VAL_DECIMAL64_8 108 #define IIO_VAL_DECIMAL64_9 109 #define IIO_VAL_DECIMAL64_10 110 #define IIO_VAL_DECIMAL64_11 111 #define IIO_VAL_DECIMAL64_12 112 #define IIO_VAL_DECIMAL64_13 113 #define IIO_VAL_DECIMAL64_14 114 #define IIO_VAL_DECIMAL64_15 115 #define IIO_VAL_DECIMAL64_MILLI IIO_VAL_DECIMAL64_3 #define IIO_VAL_DECIMAL64_MICRO IIO_VAL_DECIMAL64_6 #define IIO_VAL_DECIMAL64_NANO IIO_VAL_DECIMAL64_9 #define IIO_VAL_DECIMAL64_PICO IIO_VAL_DECIMAL64_12 #define IIO_VAL_DECIMAL64_FEMTO IIO_VAL_DECIMAL64_15 which gets stored as 64-bit, and represent the decimal scaled value. That would also work for the PLL driver (using IIO_VAL_DECIMAL64_MICRO): - It supports frequency range from 1 to 26 GHz with micro Hz resolution - In the driver a 64-bit value: (val * MICRO + val2) is already created anyways. I would leverage something like kstrtodec64() in iio_write_channel_info(). That way, I would drop the changes on the iio fixpoint parse, which I think it would do better with something like kstrntoull() to be able to handle that "dB" suffix. So for now, I may have the following approaches: - new kstrntoull() function: to have control over the parsing, whithout requiring NUL-termination, avoiding unecessary string scanning or copying. covered in v8. - expose a "safe" simple_strntoull(): minimal changes to vsprintf.c, this is covered by this patch series (v9), and it similar solution to kstrntoull(). - new kstrtodec64() function: parse decimal numbers as 64-bit with NUL-termination. Might be covered in a v10, if it is a good idea. let me know your thoughts. -- Kind regards, Rodrigo Alencar ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype 2026-03-31 13:01 ` Rodrigo Alencar @ 2026-04-12 15:05 ` Jonathan Cameron 0 siblings, 0 replies; 15+ messages in thread From: Jonathan Cameron @ 2026-04-12 15:05 UTC (permalink / raw) To: Rodrigo Alencar Cc: Andy Shevchenko, Petr Mladek, rodrigo.alencar, linux-kernel, linux-iio, devicetree, linux-doc, David Lechner, Andy Shevchenko, Lars-Peter Clausen, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton, Steven Rostedt, Rasmus Villemoes, Sergey Senozhatsky, Shuah Khan On Tue, 31 Mar 2026 14:01:04 +0100 Rodrigo Alencar <455.rodrigo.alencar@gmail.com> wrote: > On 26/03/30 01:49PM, 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: > > > > static int _kstrtodec64(const char *s, unsigned int scale, u64 *res) > > { > > 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++; > > } > > > > if (*s == '\n') > > s++; > > if (*s) > > return -EINVAL; > > > > if (check_mul_overflow(_res, int_pow(10, scale), &_res) || > > check_add_overflow(_res, _frac, &_res)) > > return -ERANGE; > > > > *res = _res; > > return 0; > > } > > > > noinline > > int kstrtoudec64(const char *s, unsigned int scale, u64 *res) > > { > > if (s[0] == '+') > > s++; > > return _kstrtodec64(s, scale, res); > > } > > EXPORT_SYMBOL(kstrtoudec64); > > > > noinline > > int kstrtosdec64(const char *s, unsigned int scale, s64 *res) > > { > > u64 tmp; > > int rv; > > > > if (s[0] == '-') { > > rv = _kstrtodec64(s + 1, scale, &tmp); > > if (rv < 0) > > return rv; > > if ((s64)-tmp > 0) > > return -ERANGE; > > *res = -tmp; > > } else { > > rv = kstrtoudec64(s, scale, &tmp); > > if (rv < 0) > > return rv; > > if ((s64)tmp < 0) > > return -ERANGE; > > *res = tmp; > > } > > return 0; > > } > > EXPORT_SYMBOL(kstrtosdec64); > > > > e.g., kstrtosdec64() or kstrtoudec64() parses "3.1415" with scale 3 into 3141 > > Hi Jonathan, > > developing more on that, I wouldn't need to create a iio_str_to_fixpoint64(), > what do you think on new format types: > > #define IIO_VAL_DECIMAL64_1 101 > #define IIO_VAL_DECIMAL64_2 102 > #define IIO_VAL_DECIMAL64_3 103 > #define IIO_VAL_DECIMAL64_4 104 > #define IIO_VAL_DECIMAL64_5 105 > #define IIO_VAL_DECIMAL64_6 106 > #define IIO_VAL_DECIMAL64_7 107 > #define IIO_VAL_DECIMAL64_8 108 > #define IIO_VAL_DECIMAL64_9 109 > #define IIO_VAL_DECIMAL64_10 110 > #define IIO_VAL_DECIMAL64_11 111 > #define IIO_VAL_DECIMAL64_12 112 > #define IIO_VAL_DECIMAL64_13 113 > #define IIO_VAL_DECIMAL64_14 114 > #define IIO_VAL_DECIMAL64_15 115 Seems unlikely more than a few of these would ever be used. If you want to keep the offsets define them in terms of first one + whatever makes sense (maybe with a base value to make that easier - then MICRO is BASE + 6 etc) > > #define IIO_VAL_DECIMAL64_MILLI IIO_VAL_DECIMAL64_3 > #define IIO_VAL_DECIMAL64_MICRO IIO_VAL_DECIMAL64_6 > #define IIO_VAL_DECIMAL64_NANO IIO_VAL_DECIMAL64_9 > #define IIO_VAL_DECIMAL64_PICO IIO_VAL_DECIMAL64_12 > #define IIO_VAL_DECIMAL64_FEMTO IIO_VAL_DECIMAL64_15 > > which gets stored as 64-bit, and represent the decimal scaled value. > That would also work for the PLL driver (using IIO_VAL_DECIMAL64_MICRO): > - It supports frequency range from 1 to 26 GHz with micro Hz resolution > - In the driver a 64-bit value: (val * MICRO + val2) is already created > anyways. > I would leverage something like kstrtodec64() in iio_write_channel_info(). > > That way, I would drop the changes on the iio fixpoint parse, which I think > it would do better with something like kstrntoull() to be able to handle that > "dB" suffix. > > So for now, I may have the following approaches: > - new kstrntoull() function: to have control over the parsing, whithout > requiring NUL-termination, avoiding unecessary string scanning or copying. > covered in v8. > - expose a "safe" simple_strntoull(): minimal changes to vsprintf.c, this > is covered by this patch series (v9), and it similar solution to kstrntoull(). > - new kstrtodec64() function: parse decimal numbers as 64-bit with NUL-termination. > Might be covered in a v10, if it is a good idea. > > let me know your thoughts. For string parsing I'm not a particular expert, so I only really care about useability of the end result. Having types would work, but I'd kind of want an 'odd ones' to stand out. Hence only defining them for parsing cases we already support (but with 64 bit values) would be my preference if you got ahead with this new approach. Jonathan > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype 2026-03-30 12:49 ` Rodrigo Alencar 2026-03-31 13:01 ` Rodrigo Alencar @ 2026-04-01 12:22 ` Petr Mladek 2026-04-01 13:16 ` Rodrigo Alencar 1 sibling, 1 reply; 15+ messages in thread From: Petr Mladek @ 2026-04-01 12:22 UTC (permalink / raw) To: Rodrigo Alencar Cc: Andy Shevchenko, rodrigo.alencar, linux-kernel, linux-iio, devicetree, linux-doc, Jonathan Cameron, David Lechner, Andy Shevchenko, Lars-Peter Clausen, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton, Steven Rostedt, Rasmus Villemoes, Sergey Senozhatsky, Shuah Khan 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. 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) 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. + _float_ means that it will be able to read float numbers + @precisions parameter defines the number of digits accepted after the radix point. It is also used as multiplier for scaling the output number. + @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, ... > { > 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. > } > > 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. > 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. Best Regards, Petr PS: I am sorry if I used some mathematical terms a wrong way. I am not a native speaker. And it is a long time since I worked with float numbers. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype 2026-04-01 12:22 ` Petr Mladek @ 2026-04-01 13:16 ` Rodrigo Alencar 0 siblings, 0 replies; 15+ messages in thread From: Rodrigo Alencar @ 2026-04-01 13:16 UTC (permalink / raw) To: Petr Mladek, Rodrigo Alencar Cc: Andy Shevchenko, rodrigo.alencar, linux-kernel, linux-iio, devicetree, linux-doc, Jonathan Cameron, David Lechner, Andy Shevchenko, Lars-Peter Clausen, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton, Steven Rostedt, Rasmus Villemoes, Sergey Senozhatsky, Shuah Khan 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype 2026-03-27 9:17 ` Andy Shevchenko 2026-03-27 10:11 ` Rodrigo Alencar @ 2026-03-27 10:44 ` David Laight 2026-03-27 10:57 ` Andy Shevchenko 1 sibling, 1 reply; 15+ messages in thread From: David Laight @ 2026-03-27 10:44 UTC (permalink / raw) To: Andy Shevchenko Cc: Petr Mladek, rodrigo.alencar, linux-kernel, linux-iio, devicetree, linux-doc, Jonathan Cameron, David Lechner, Andy Shevchenko, Lars-Peter Clausen, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton, Steven Rostedt, Rasmus Villemoes, Sergey Senozhatsky, Shuah Khan On Fri, 27 Mar 2026 11:17:16 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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: > > ... > > > > +extern ssize_t __must_check simple_strntoull(const char *startp, const char **endp, > > > + unsigned int base, size_t max_chars, > > > + unsigned long long *res); > > > > Sigh, naming is hard. I personally find it a bit confusing that the > > name is too similar to the unsafe API. > > > > IMHO, the semantic of the new API is closer to kstrtoull(). > > It just limits the size, so I would call it kstrntoull(). > > It's not. kstrto*() quite strict about the input, this one is actually relaxed > variant, so I wouldn't mix these two groups. > > > Also I would use int as the return parameter, see below. > > ... > > TBH, I am skeptical about this approach. My main objection is max_chars > parameter. If we want to limit the input strictly to the given number of > characters, we have to copy the string and then just use kstrto*() in a normal > way. The whole idea of that parameter is to be able to parse the fractional > part of the float number as 'iiiii.fffff', where 'i' is for integer part, and > 'f' for the fractional. Since we have *endp, we may simply check that. > > In case if we want to parse only, say, 6 digits and input is longer there are > a few options (in my personal preferences, the first is the better): > - consider the input invalid > - parse it as is up to the maximum and then do ceil() or floor() on top of that > - copy only necessary amount of the (sub)string and parse that. Isn't there a bigger problem? If you want a max of 6 digits you need to correctly parse 3.1 3.159265 3.159256358979 3.0001 3.000159 3.00015926535 3.000100 (etc). That seems to always require checking the length and then multiply/divide by 10. Then there is 'round to even' which rounds these two in opposite directions: 4.500000000000000000000000000000000000000000000000000 4.500000000000000000000000000000000000000000000000001 I suspect you really want a completely different function for reading fractional parts of floating point numbers. It isn't as though the actual digit conversion is hard. > > The problem with precision is that we need to also consider floor() or ceil() > and I don't think this should be burden of the library as it's individual > preference of each of the callers (users). At least for the starter, we will > see if it's only one approach is used, we may incorporate it into the library > code. > > The easiest way out is to just consider the input invalid if it overflows the > given type (s32 or s64). > > But we need to have an agreement what will be the representation of the > fixed-width float numbers in the kernel? Currently IIO uses > struct float // name is crafted for simplicity > { > int integer; > int fraction; > } > > This parser wants AFAIU to have at the end of the day something like > > struct float > { > s64 integer; > s64 fraction; > } > > but also wants to have the fraction part be limited in some cases to s32 > or so: > > struct float > { > s64 integer; > s32 fraction; // precision may be lost if input is longer > } Are those 'fraction' counts of (say) 10^-6 (like times in seconds+usecs) or true binary values where the value could be treated as a u64 (or u128) for addition and subtraction. So parse the latter you don't need to know the length (and it can be converted the to former by multiplying by 10^6). David > > 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'. > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype 2026-03-27 10:44 ` David Laight @ 2026-03-27 10:57 ` Andy Shevchenko 2026-03-27 13:28 ` David Laight 0 siblings, 1 reply; 15+ messages in thread From: Andy Shevchenko @ 2026-03-27 10:57 UTC (permalink / raw) To: David Laight Cc: Petr Mladek, rodrigo.alencar, linux-kernel, linux-iio, devicetree, linux-doc, Jonathan Cameron, David Lechner, Andy Shevchenko, Lars-Peter Clausen, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton, Steven Rostedt, Rasmus Villemoes, Sergey Senozhatsky, Shuah Khan On Fri, Mar 27, 2026 at 10:44:40AM +0000, David Laight wrote: > On Fri, 27 Mar 2026 11:17:16 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: ... > > TBH, I am skeptical about this approach. My main objection is max_chars > > parameter. If we want to limit the input strictly to the given number of > > characters, we have to copy the string and then just use kstrto*() in a normal > > way. The whole idea of that parameter is to be able to parse the fractional > > part of the float number as 'iiiii.fffff', where 'i' is for integer part, and > > 'f' for the fractional. Since we have *endp, we may simply check that. > > > > In case if we want to parse only, say, 6 digits and input is longer there are > > a few options (in my personal preferences, the first is the better): > > - consider the input invalid > > - parse it as is up to the maximum and then do ceil() or floor() on top of that > > - copy only necessary amount of the (sub)string and parse that. > > Isn't there a bigger problem? > If you want a max of 6 digits you need to correctly parse 3.1 3.159265 > 3.159256358979 3.0001 3.000159 3.00015926535 3.000100 (etc). > That seems to always require checking the length and then multiply/divide > by 10. Yep. > Then there is 'round to even' which rounds these two in opposite directions: > 4.500000000000000000000000000000000000000000000000000 > 4.500000000000000000000000000000000000000000000000001 These are wrong inputs and if we want to have them cut, it will be just a cut. (Yeah, which will have different result for negative numbers.) > I suspect you really want a completely different function for reading > fractional parts of floating point numbers. > It isn't as though the actual digit conversion is hard. > > > The problem with precision is that we need to also consider floor() or ceil() > > and I don't think this should be burden of the library as it's individual > > preference of each of the callers (users). At least for the starter, we will > > see if it's only one approach is used, we may incorporate it into the library > > code. > > > > The easiest way out is to just consider the input invalid if it overflows the > > given type (s32 or s64). > > > > But we need to have an agreement what will be the representation of the > > fixed-width float numbers in the kernel? Currently IIO uses > > struct float // name is crafted for simplicity > > { > > int integer; > > int fraction; > > } > > > > This parser wants AFAIU to have at the end of the day something like > > > > struct float > > { > > s64 integer; > > s64 fraction; > > } > > > > but also wants to have the fraction part be limited in some cases to s32 > > or so: > > > > struct float > > { > > s64 integer; > > s32 fraction; // precision may be lost if input is longer > > } > > Are those 'fraction' counts of (say) 10^-6 (like times in seconds+usecs) > or true binary values where the value could be treated as a u64 (or u128) > for addition and subtraction. It depends. IIO has scale on top of that, so the fraction part can be 10⁻³, 10⁻⁶, 10⁻⁹. I don't remember by heart if the ABI requires all digits to be placed, I think we don't require that. > So parse the latter you don't need to know the length > (and it can be converted the to former by multiplying by 10^6). > > > 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'. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype 2026-03-27 10:57 ` Andy Shevchenko @ 2026-03-27 13:28 ` David Laight 0 siblings, 0 replies; 15+ messages in thread From: David Laight @ 2026-03-27 13:28 UTC (permalink / raw) To: Andy Shevchenko Cc: Petr Mladek, rodrigo.alencar, linux-kernel, linux-iio, devicetree, linux-doc, Jonathan Cameron, David Lechner, Andy Shevchenko, Lars-Peter Clausen, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton, Steven Rostedt, Rasmus Villemoes, Sergey Senozhatsky, Shuah Khan On Fri, 27 Mar 2026 12:57:56 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Fri, Mar 27, 2026 at 10:44:40AM +0000, David Laight wrote: ,,, > > > but also wants to have the fraction part be limited in some cases to s32 > > > or so: > > > > > > struct float > > > { > > > s64 integer; > > > s32 fraction; // precision may be lost if input is longer > > > } > > > > Are those 'fraction' counts of (say) 10^-6 (like times in seconds+usecs) > > or true binary values where the value could be treated as a u64 (or u128) > > for addition and subtraction. > > It depends. IIO has scale on top of that, so the fraction part can be 10⁻³, > 10⁻⁶, 10⁻⁹. I don't remember by heart if the ABI requires all digits to be > placed, I think we don't require that. Seems like you want this function (untested): u64 strtofrac(const char *buf, const char **end, unsigned int len) { u64 val = 0; unsigned int digit; while (len--) { digit = *buf - '0'; if (digit <= 9) { buf++; val += digit; } val *= 10; } while (*buf - '0' <= 9u) buf++; *end = buf; return val; } David ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype 2026-03-27 8:45 ` [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype Petr Mladek 2026-03-27 9:17 ` Andy Shevchenko @ 2026-03-27 9:24 ` Rodrigo Alencar 1 sibling, 0 replies; 15+ messages in thread From: Rodrigo Alencar @ 2026-03-27 9:24 UTC (permalink / raw) To: Petr Mladek, rodrigo.alencar Cc: linux-kernel, linux-iio, devicetree, linux-doc, Jonathan Cameron, David Lechner, Andy Shevchenko, Lars-Peter Clausen, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, Andrew Morton, Steven Rostedt, Andy Shevchenko, Rasmus Villemoes, Sergey Senozhatsky, Shuah Khan On 26/03/27 09:45AM, Petr Mladek wrote: > On Fri 2026-03-20 16:27:27, Rodrigo Alencar via B4 Relay wrote: > > From: Rodrigo Alencar <rodrigo.alencar@analog.com> > > > > Expose simple_strntoull(), by addressing its FIXME, i.e. its prototype is > > slightly changed so that -ERANGE or -EINVAL can be evaluated by the user. > > Flow of the function is not changed and error value is returned in the > > end. Unsafe internal wrapper is created to reduce amount of changes. > > > > --- a/include/linux/kstrtox.h > > +++ b/include/linux/kstrtox.h > > @@ -148,4 +148,8 @@ extern long simple_strtol(const char *,char **,unsigned int); > > extern unsigned long long simple_strtoull(const char *,char **,unsigned int); > > extern long long simple_strtoll(const char *,char **,unsigned int); > > > > +extern ssize_t __must_check simple_strntoull(const char *startp, const char **endp, > > + unsigned int base, size_t max_chars, > > + unsigned long long *res); > > Sigh, naming is hard. I personally find it a bit confusing that the > name is too similar to the unsafe API. > > IMHO, the semantic of the new API is closer to kstrtoull(). > It just limits the size, so I would call it kstrntoull(). > > Also I would use int as the return parameter, see below. Thanks for look into this one. kstrntoull() was what I used in v8: https://lore.kernel.org/r/20260303-adf41513-iio-driver-v8-0-8dd2417cc465@analog.com There was a discussion around the naming: https://lore.kernel.org/all/4mtdzxfj656sjr66npabfvrr7yd7q26l2unhsihjtniz4ossfj@g3qnzonoary6/ please suggest how the function prototype should look like. ... > > +/* unsafe_strntoull ignores simple_strntoull() return value and endp const qualifier */ > > +inline > > +static unsigned long long unsafe_strntoull(const char *startp, char **endp, > > + unsigned int base, size_t max_chars) > > +{ > > + unsigned long long result; > > + const char *cp; > > + > > +#pragma GCC diagnostic push > > +#pragma GCC diagnostic ignored "-Wunused-result" > > + simple_strntoull(startp, &cp, base, max_chars, &result); > > +#pragma GCC diagnostic pop > > + > > if (endp) > > *endp = (char *)cp; > > IMHO, we do not need local "cp". We could simply pass the endp > to the new simple_strntoull. Or do I miss anything? Basically the unsafe version drops the const qualifier and compiler complains that pointer types do not match. Maybe an extra warning can be suppressed there. -- Kind regards, Rodrigo Alencar ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2026-04-12 15:06 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260320-adf41513-iio-driver-v9-0-132f0d076374@analog.com>
2026-03-22 11:33 ` [PATCH v9 0/9] ADF41513/ADF41510 PLL frequency synthesizers Jonathan Cameron
[not found] ` <20260320-adf41513-iio-driver-v9-2-132f0d076374@analog.com>
2026-03-27 8:45 ` [PATCH v9 2/9] lib: vsprintf: export simple_strntoull() in a safe prototype Petr Mladek
2026-03-27 9:17 ` Andy Shevchenko
2026-03-27 10:11 ` Rodrigo Alencar
2026-03-27 10:21 ` Andy Shevchenko
2026-03-27 15:17 ` Rodrigo Alencar
2026-03-30 12:49 ` Rodrigo Alencar
2026-03-31 13:01 ` Rodrigo Alencar
2026-04-12 15:05 ` Jonathan Cameron
2026-04-01 12:22 ` Petr Mladek
2026-04-01 13:16 ` Rodrigo Alencar
2026-03-27 10:44 ` David Laight
2026-03-27 10:57 ` Andy Shevchenko
2026-03-27 13:28 ` David Laight
2026-03-27 9:24 ` Rodrigo Alencar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox