| 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: <YfmJ3P1gYaEkVjlY@shaak>
Date: Tue, 1 Feb 2022 14:28:28 -0500
From: Liam Beguin <liambeguin@...il.com>
To: Peter Rosin <peda@...ntia.se>
Cc: jic23@...nel.org, andy.shevchenko@...il.com, lars@...afoo.de,
linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, robh+dt@...nel.org
Subject: Re: [PATCH v13 06/11] iio: afe: rescale: make use of units.h
Hi Peter,
On Mon, Jan 31, 2022 at 03:50:22PM +0100, Peter Rosin wrote:
> Hi!
>
> I noticed that I have not reviewed this patch. Sorry for my low
> bandwidth.
>
> On 2022-01-30 17:10, Liam Beguin wrote:
> > Make use of well-defined SI metric prefixes to improve code readability.
> >
> > Signed-off-by: Liam Beguin <liambeguin@...il.com>
> > ---
> > drivers/iio/afe/iio-rescale.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > index 67273de46843..27c6664915ff 100644
> > --- a/drivers/iio/afe/iio-rescale.c
> > +++ b/drivers/iio/afe/iio-rescale.c
> > @@ -51,11 +51,11 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
> > }
> > fallthrough;
> > case IIO_VAL_FRACTIONAL_LOG2:
> > - tmp = (s64)*val * 1000000000LL;
> > + tmp = (s64)*val * GIGA;
> > tmp = div_s64(tmp, rescale->denominator);
> > tmp *= rescale->numerator;
> >
> > - tmp = div_s64_rem(tmp, 1000000000LL, &rem);
> > + tmp = div_s64_rem(tmp, GIGA, &rem);
>
> It is NOT easy for me to say which of GIGA/NANO is most fitting.
> There are a couple of considerations:
I agree with you that the choice behind GIGA/NANO can be a bit
confusing.
In my opinion, these defines makes the code easier to read if you
consider them as multipliers with no physical meaning, basically a
pretty name for a power of 10.
By this logic, we wouldn't ever use FEMTO to DECI.
Cheers,
Liam
> A) 1000000000 is just a big value (GIGA fits). Something big is
> needed to not lose too much precision.
> B) 1000000000 is what the IIO core uses to print fractional-log
> values with nano precision (NANO fits). This is not really
> relevant in this context.
> C) 1000000000 makes the int-plus-nano and fractional-log cases
> align (NANO fits). This last consideration is introduced with
> patch 4/11.
>
> There is simply no correct define to use. And whichever define is
> chosen makes the other interpretation less obvious. Which is not
> desirable, obscures things and make both GIGA and NANO bad
> options.
>
> So, I stepped back to the description provided by Andy in the
> comments of v11:
>
> On 2021-12-22 19:59, Andy Shevchenko wrote:
> | You should get the proper power after the operation.
> | Write a formula (mathematically speaking) and check each of them for this.
> |
> | 10^-5/10^-9 == 1*10^4 (Used NANO)
> | 10^-5/10^9 == 1/10^-14 (Used GIGA)
> |
> | See the difference?
>
> No, I don't really see the difference, that just makes me totally
> confused. Dividing by 10^-9 or multiplying by 10^9 is as we all
> know exactly the same, and the kernel cannot deal directly with
> 10^-9 so both will look the same in code (multiplying by 10^9). So,
> you must be referring to the "real formula" behind the code. But
> in that case, if the "real formula" behind the (then equivalent)
> code had instead been
>
> 10^-5*10^9 == 1*10^4 (Used GIGA)
> 10^-5*10^-9 == 1/10^-14 (Used NANO)
>
> the outcome is the opposite. NANO turns GIGA and vice versa.
>
> Since you can express the same thing differently in math too, it
> all crumbles for me. Because of this duality, it will be a matter
> of taste if GIGA or NANO fits best in any given instance. Sometimes
> (perhaps commonly) it will be decidedly easy to pick one of them,
> but in other cases (see above) we will end up with a conflict.
>
> What to do then? Or, what am I missing?
>
> My taste says NANO in this case, since A) is just some big number
> and not really about units and B) is as stated not really relevant.
> Which makes C) win the argument for me.
>
> > *val = tmp;
> >
> > if (!rem)
> > @@ -71,7 +71,7 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
> >
> > *val2 = rem / (int)tmp;
> > if (rem2)
> > - *val2 += div_s64((s64)rem2 * 1000000000LL, tmp);
> > + *val2 += div_s64((s64)rem2 * GIGA, tmp);
>
> Here, 1000000000 matches the above use. If we go with NANO above,
> we should go with NANO here as well.
>
> > return IIO_VAL_INT_PLUS_NANO;
> > case IIO_VAL_INT_PLUS_NANO:
> > @@ -332,8 +332,8 @@ static int rescale_current_sense_amplifier_props(struct device *dev,
> > * gain_div / (gain_mult * sense), while trying to keep the
> > * numerator/denominator from overflowing.
> > */
> > - factor = gcd(sense, 1000000);
> > - rescale->numerator = 1000000 / factor;
> > + factor = gcd(sense, MEGA);
> > + rescale->numerator = MEGA / factor;
>
> Here, the 1000000 number comes from the unit of the sense resistor
> (micro-ohms), so I would have preferred MICRO. But who can tell
> if we -mathematically speaking- have divided the given resistance
> integer by 10^6 (MEGA) or multiplied it with 10^-6 (MICRO) to
> account for the unit? Or if we divided the other values with
> 10^6 (MEGA) (or multiplied by 10^-6, MICRO) to make them fit the
> unit of the shunt resistance?
>
> All of the above is of course equivalent so both MEGA and MICRO
> are correct. But as stated, MICRO makes to most sense as that is
> what connects the code to reality and hints at where the value
> is coming from. For me anyway.
>
> > rescale->denominator = sense / factor;
> >
> > factor = gcd(rescale->numerator, gain_mult);
> > @@ -361,8 +361,8 @@ static int rescale_current_sense_shunt_props(struct device *dev,
> > return ret;
> > }
> >
> > - factor = gcd(shunt, 1000000);
> > - rescale->numerator = 1000000 / factor;
> > + factor = gcd(shunt, MEGA);
> > + rescale->numerator = MEGA / factor;
>
> Same here, 1000000 comes from the micro-ohms unit of the shunt
> resistor, so I would have preferred MICRO.
>
>
>
> Sorry for the long mail. I blame the duality of these ambiguous
> SI-defines that are a bit confusing to me.
>
> Cheers,
> Peter
>
> > rescale->denominator = shunt / factor;
> >
> > return 0;
Powered by blists - more mailing lists