linux-kernel - Re: [PATCH] iio:temperature:mlx90632: Reduce number of equal calulcations

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAKv63uuFNOksmDUKEapvX60gg9QE+32Dak_2=M2cYeueFiHjPg@mail.gmail.com>
Date:   Tue, 4 Aug 2020 09:57:38 +0200
From:   Crt Mori <cmo@...exis.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] iio:temperature:mlx90632: Reduce number of equal calulcations

Hi Andy,
Thanks for the comments. This is indeed a cut-out section of what I
wanted to submit next.

On Mon, 3 Aug 2020 at 18:35, Andy Shevchenko <andy.shevchenko@...il.com> wrote:
>
> On Mon, Aug 3, 2020 at 6:17 PM Crt Mori <cmo@...exis.com> wrote:
> >
> > TAdut4 was calculated each iteration although it did not change. In light
> > of near future additions of the Extended range DSP calculations, this
> > function refactoring will help reduce unrelated changes in that series as
> > well as reduce the number of new functions needed.
>
> Okay!
>
> > Also converted shifts in this function of signed integers to divisions as
> > that is less implementation-defined behavior.
>
> This is what I'm wondering about. Why?
>
> ...

The reason for this is that whenever something is wrong with the
calculation I am looking into the shifts which are
implementation-defined and might not keep the signed bit. Division
however would.

>
> > -       Ha_customer = ((s64)Ha * 1000000LL) >> 14ULL;
> > -       Hb_customer = ((s64)Hb * 100) >> 10ULL;
> > +       Ha_customer = div64_s64((s64)Ha * 1000000LL, 16384);
> > +       Hb_customer = div64_s64((s64)Hb * 100, 1024);
>
> Have you checked the code on 32-bit machines?
> As far as I can see the div64_*64() do not have power of two divisor
> optimizations. I bet it will generate a bulk of unneeded code.
>
> ...
>
> > -       calcedKsTO = ((s64)((s64)Ga * (prev_object_temp - 25 * 1000LL)
> > -                            * 1000LL)) >> 36LL;
> > -       calcedKsTA = ((s64)(Fb * (TAdut - 25 * 1000000LL))) >> 36LL;
> > -       Alpha_corr = div64_s64((((s64)(Fa * 10000000000LL) >> 46LL)
> > -                               * Ha_customer), 1000LL);
>
> > +       calcedKsTO = div64_s64((s64)((s64)Ga * (prev_object_temp - 25 * 1000LL)
> > +                                    * 1000LL), 68719476736);
> > +       calcedKsTA = div64_s64((s64)(Fb * (TAdut - 25 * 1000000LL)), 68719476736);
> > +       Alpha_corr = div64_s64(div64_s64((s64)(Fa * 10000000000LL), 70368744177664)
> > +                              * Ha_customer, 1000LL);
>
> This is less readable and full of magic numbers in comparison to the
> above (however, also full of magics, but at least gives better hint).
>
> ...

These are coefficients so there is not much to unmagic. I can keep the
shifts, if you think that is more readable or add comments after lines
with 2^46 or something?
>
> > +       TAdut4 = (div64_s64(TAdut, 10000LL) + 27315) *
> > +               (div64_s64(TAdut, 10000LL) + 27315) *
> > +               (div64_s64(TAdut, 10000LL)  + 27315) *
> > +               (div64_s64(TAdut, 10000LL) + 27315);
>
> Shouldn't you switch to definitions from units.h? (perhaps as a separate change)
>
> --
> With Best Regards,
> Andy Shevchenko