| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aW40ylvMwVhqNQMw@smile.fi.intel.com>
Date: Mon, 19 Jan 2026 15:42:34 +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 v4 3/7] iio: frequency: adf41513: driver implementation
On Mon, Jan 19, 2026 at 11:21:59AM +0000, Rodrigo Alencar wrote:
> On 26/01/19 09:31AM, Andy Shevchenko wrote:
> > On Fri, Jan 16, 2026 at 02:32:22PM +0000, Rodrigo Alencar via B4 Relay wrote:
...
> > > +struct adf41513_pll_settings {
> > > + enum adf41513_pll_mode mode;
> >
> > Sounds to me like a room to improve the layout here,
>
> I am targeting a 32-bit cpu, just moved in_value down:
> would this be fine? (pahole output):
Likely.
> struct adf41513_pll_settings {
> enum adf41513_pll_mode mode; /* 0 4 */
Wondering if this can be shorter if moved down...
> u8 r_counter; /* 4 1 */
> u8 ref_doubler; /* 5 1 */
> u8 ref_div2; /* 6 1 */
> u8 prescaler; /* 7 1 */
> u64 target_frequency_uhz; /* 8 8 */
> u64 actual_frequency_uhz; /* 16 8 */
> u64 pfd_frequency_uhz; /* 24 8 */
> u32 frac1; /* 32 4 */
> u32 frac2; /* 36 4 */
> u32 mod2; /* 40 4 */
> u16 int_value; /* 44 2 */
>
> /* size: 48, cachelines: 1, members: 12 */
> /* padding: 2 */
> /* last cacheline: 48 bytes */
> };
...at least I have had in mind that "mode" should be moved to be near
to "int_value". But I think it will take 4 bytes still as we don't use
short enums compile wise.
> > > + /* reference path parameters */
> > > + u8 r_counter;
> > > + u8 ref_doubler;
> > > + u8 ref_div2;
> > > + u8 prescaler;
> > > +
> > > + /* frequency parameters */
> > > + u64 target_frequency_uhz;
> > > + u64 actual_frequency_uhz;
> > > + u64 pfd_frequency_uhz;
> > > +
> > > + /* pll parameters */
> > > + u16 int_value;
> > > + u32 frac1;
> > > + u32 frac2;
> > > + u32 mod2;
> > > +};
...
> > > +static int adf41513_parse_uhz(const char *str, u64 *freq_uhz)
> > > +{
> > > + u64 uhz = 0;
> > > + int f_count = ADF41513_HZ_DECIMAL_PRECISION;
> > > + bool frac_part = false;
> > > +
> > > + if (str[0] == '+')
> > > + str++;
> > > +
> > > + while (*str && f_count > 0) {
> > > + if ('0' <= *str && *str <= '9') {
> > > + uhz = uhz * 10 + *str - '0';
> > > + if (frac_part)
> > > + f_count--;
> > > + } else if (*str == '\n') {
> > > + if (*(str + 1) == '\0')
> > > + break;
> > > + return -EINVAL;
> >
> > > + } else if (*str == '.' && !frac_part) {
> >
> > This can be found by strchr() / strrchr() (depending on the expectations of
> > the input).
> >
> > > + frac_part = true;
> > > + } else {
> > > + return -EINVAL;
> > > + }
> > > + str++;
> > > + }
> >
> > With the above the rest becomes just a couple of simple_strtoull() calls with
> > a couple of int_pow(10) calls (and some validation on top).
> >
> > > + for (; f_count > 0; f_count--)
> > > + uhz *= 10;
> >
> > This is int_pow(10).
> >
> > > + *freq_uhz = uhz;
> > > +
> > > + return 0;
> > > +}
>
> The current implementation is kind of a stripped version of
> __iio_str_to_fixpoint(). Would you prefer something like this, then?:
Do they have most of the parts in common? If so, why can't we use
__iio_str_to_fixpoint() directly? Or why can't we slightly refactor
that to give us the results we need here?
> static int adf41513_parse_uhz(const char *str, u64 *freq_uhz)
> {
> u64 integer_part = 0, fractional_part = 0;
> const char *decimal_point;
> char *endptr;
> int frac_digits;
>
> if (str[0] == '+')
> str++;
>
> /* Find decimal point */
> decimal_point = strchr(str, '.');
> if (decimal_point) {
> /* Parse integer part (if exists before decimal point) */
> if (decimal_point > str) {
I don't think you need this check, simple_strtoull() should return 0.
Also check the ranges, perhaps you want in some cases simple_strtoul().
> integer_part = simple_strtoull(str, &endptr, 10);
> if (endptr != decimal_point)
> return -EINVAL;
> }
>
> /* Parse fractional part */
> fractional_part = simple_strtoull(decimal_point + 1, &endptr, 10);
The idea of using the simple strtoull() (second "l") is to check for overflows,
so if the number is > UINT_MAX we probably should return -ERANGE.
We have somewhere already such a code in the kernel, maybe it's a time to have
advanced version of simple_strtouint().
drivers/crypto/intel/qat/qat_common/qat_uclo.c:206: ae = simple_strtoull(str, &end, 10);
drivers/crypto/intel/qat/qat_common/qat_uclo.c-207- if (ae > UINT_MAX || str == end || (end - str) > 19)
drivers/crypto/intel/qat/qat_common/qat_uclo.c-208- return -EINVAL;
> if (*endptr != '\0' && *endptr != '\n')
> return -EINVAL;
>
> /* Adjust for desired precision */
> frac_digits = strcspn(decimal_point + 1, "\n");
This is already precalculated: endptr - decimal_point (+ 1 ?).
> if (frac_digits > ADF41513_HZ_DECIMAL_PRECISION)
> fractional_part /= int_pow(10, frac_digits - ADF41513_HZ_DECIMAL_PRECISION);
> else
> fractional_part *= int_pow(10, ADF41513_HZ_DECIMAL_PRECISION - frac_digits);
> } else {
> /* No decimal point - just parse the integer */
> ret = kstrtoull(str, 10, &integer_part);
> if (ret)
> return ret;
> }
>
> /* Combine integer and fractional parts */
> *freq_uhz = integer_part * int_pow(10, ADF41513_HZ_DECIMAL_PRECISION) + fractional_part;
>
> return 0;
> }
...
> > > +static int adf41513_uhz_to_str(u64 freq_uhz, char *buf)
> > > +{
> > > + u32 frac_part;
> > > + u64 int_part = div_u64_rem(freq_uhz, MICRO, &frac_part);
> >
> > Perhaps MICROHZ_PER_HZ? This will be consistent with the int_value in
> > _calc_*() below.
>
> Here, the meaning is different. int_part is in Hz and frac_part in uHz.
> Will add the suffixes to the variables.
Yes, but here it's a constant divisor, and not a multiplier.
Meaning that one divides µHz by µHz.
> > > + return sysfs_emit(buf, "%llu.%06u\n", int_part, frac_part);
> > > +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists