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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VfmSfmezqwwRfHZ797Y9rYDu3hgL5vGvPwbzGjCXsKWcQ@mail.gmail.com>
Date:   Mon, 3 Aug 2020 19:34:52 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Crt Mori <cmo@...exis.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

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?

...

> -       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).

...

> +       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