| 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: <CAJCx=gkFU+PK+gROVRKGiNuXdjsBDFmBva0zqoHdQQ=YEy2xRw@mail.gmail.com>
Date: Sat, 8 Apr 2017 14:34:48 -0700
From: Matt Ranostay <matt.ranostay@...sulko.com>
To: Andreas Klinger <ak@...klinger.de>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Jonathan Cameron <jic23@...nel.org>,
Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald <pmeerw@...erw.net>,
Vlad Dogaru <ddvlad@...il.com>,
Akinobu Mita <akinobu.mita@...il.com>,
Wei Yongjun <yongjun_wei@...ndmicro.com.cn>,
Aniroop Mathur <a.mathur@...sung.com>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] IIO: bmp280-core.c: fix error in humidity calculation
On Fri, Apr 7, 2017 at 12:27 PM, Andreas Klinger <ak@...klinger.de> wrote:
> Hi Matt,
>
> i've extracted the error condition in a small userspace application for
> demonstration. Just compile and see that in this case the variable H3 is zero
> but the whole term evaluates differently if treated as unsigned or signed.
>
> The whole example including the values of the variables are taken from the real
> driver.
>
Ok now I see what you are referencing from the datasheet, and can
confirm the values are incorrect.
Just a suggestion rather than have all those casts it would be easier
to just change H1 and H3 types
to signed integers.
Thanks,
Matt
> Andreas
>
>
> Now the example:
>
> ---
>
> /*
> * humidity.c
> * this test program is just for demonstrating the difference in the
> * calculation of humidity compensation with BME280 sensor
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License version 2 as
> * published by the Free Software Foundation.
> *
> */
>
> #include <stdio.h>
> #include <stdlib.h>
>
> int main(int argn, char* argv[])
> {
> int adc_humidity = 28275;
> int t_fine = 50623;
> unsigned int H1 = 75, H3 = 0;
> int H2 = 360, H4 = 324, H5 = 0, H6 = 30;
> int var;
>
> var = -26177;
> /* extracted errornous term with cast */
> printf("with cast: %d\n", (((var * H6) >> 10) * (((var * (int)H3) >> 11)
> + (int)32768)) >> 10);
> /* extracted errornous term now without a cast */
> printf("without cast: %d\n", (((var * H6) >> 10) * (((var * H3) >> 11)
> + 32768)) >> 10);
>
> printf("\n");
> printf("t_fine: %d; humidity: %d\nH: %d; %d; %d; %d; %d; %d\n",
> t_fine, adc_humidity, H1, H2, H3, H4, H5, H6);
> printf("\n");
>
> /* the whole example taken from the driver */
> /* with the cast as proposed by the documentation */
> var = ((int)t_fine) - (int)76800;
> var = ((((adc_humidity << 14) - (H4 << 20) - (H5 * var))
> + (int)16384) >> 15) * (((((((var * H6) >> 10)
> * (((var * (int)H3) >> 11) + (int)32768)) >> 10)
> + (int)2097152) * H2 + 8192) >> 14);
> var -= ((((var >> 15) * (var >> 15)) >> 7) * (int)H1) >> 4;
>
> printf("with cast: var: %d\n", var>>12);
> printf("\n");
>
> /* now the same calculation without the cast */
> var = (t_fine) - 76800;
> var = ((((adc_humidity << 14) - (H4 << 20) - (H5 * var))
> + 16384) >> 15) * (((((((var * H6) >> 10)
> * (((var * H3) >> 11) + 32768)) >> 10)
> + 2097152) * H2 + 8192) >> 14);
> var -= ((((var >> 15) * (var >> 15)) >> 7) * H1) >> 4;
>
> printf("without cast: var: %d\n", var>>12);
>
> return 0;
> }
>
> ---
>
> Output:
>
> with cast: -24544
> without cast: 4169760
>
> t_fine: 50623; humidity: 28275
> H: 75; 360; 0; 324; 0; 30
>
> with cast: var: 41671
>
> without cast: var: 124497
>
> ---
>
> Matt Ranostay <matt.ranostay@...sulko.com> schrieb am Thu, 06. Apr 23:13:
>> On Mon, Apr 3, 2017 at 11:16 PM, Matt Ranostay
>> <matt.ranostay@...sulko.com> wrote:
>> > On Sun, Apr 2, 2017 at 10:29 AM, Andreas Klinger <ak@...klinger.de> wrote:
>> >> Linus Walleij <linus.walleij@...aro.org> schrieb am Sun, 02. Apr 16:56:
>> >>> On Sun, Apr 2, 2017 at 11:32 AM, Jonathan Cameron <jic23@...nel.org> wrote:
>> >>> > On 27/03/17 11:06, Andreas Klinger wrote:
>> >>> >> While calculating the compensation of the humidity there are negative
>> >>> >> values interpreted as unsigned because of unsigned variables used.
>> >>> >> These values need to be casted to signed as indicated by the documentation
>> >>> >> of the sensor.
>> >>> >>
>> >>> >> Signed-off-by: Andreas Klinger <ak@...klinger.de>
>> >>> >
>> >>> > Looks superficially right to me, but would like an Ack from Linus Walleij.
>> >>>
>> >>> I didn't work on these calculations, only infrastructure for the driver
>> >>> but FWIW:
>> >>> Acked-by: Linus Walleij <linus.walleij@...aro.org>
>> >>>
>> >>> Matt Ranostay @Intel is the person you probably want to ACK it though,
>> >>> he added the humidity calculations :)
>> >>
>> >> It just turned out that there must be another error in the calculation. I'll fix
>> >> and test it first.
>> >>
>> >> When i'm done i'll send a new version of the patch.
>> >>
>> >
>> > Ok will hold off till review till the next patchset... BTW this
>> > datasheet's calculations made my eyes bleed so it is quiet likely it
>> > has bugs...
>>
>> Hmm the datatypes you are casting to s32 are already signed
>> integers... Interested in what you found on what the actual bug is.
>>
>> >
>> > Thanks,
>> >
>> > Matt
>> >
>> >> Andreas
>> >>
>> >>>
>> >>> Yours,
>> >>> Linus Walleij
>> >>
>> >> --
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> >> the body of a message to majordomo@...r.kernel.org
>> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
Powered by blists - more mailing lists