| 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
| ||
|
Date: Mon, 10 Apr 2017 18:47:50 +0200
From: Andreas Klinger <ak@...klinger.de>
To: Matt Ranostay <matt.ranostay@...sulko.com>
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.
>
That's right. But i wanted to keep close to the documentation.
> 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
> >
> > --
> --
> 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
--
Andreas Klinger
Grabenreith 27
84508 Burgkirchen
+49 8623 919966
ak@...klinger.de
www.it-klinger.de
www.grabenreith.de
Powered by blists - more mailing lists