linux-kernel - Re: [PATCH] iio: cros_ec: Fix gyro scale calculation

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: <20190220160126.12568f92@archlinux>
Date:   Wed, 20 Feb 2019 16:01:26 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Enric Balletbo i Serra <enric.balletbo@...labora.com>
Cc:     linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Gwendal Grignou <gwendal@...omium.org>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Guenter Roeck <groeck@...omium.org>,
        Benson Leung <bleung@...omium.org>,
        Lars-Peter Clausen <lars@...afoo.de>, kernel@...labora.com,
        Hartmut Knaack <knaack.h@....de>
Subject: Re: [PATCH] iio: cros_ec: Fix gyro scale calculation

On Wed, 20 Feb 2019 16:03:00 +0100
Enric Balletbo i Serra <enric.balletbo@...labora.com> wrote:

> From: Gwendal Grignou <gwendal@...omium.org>
> 
> Calculation was copied from IIO_DEGREE_TO_RAD, but offset added to avoid
> rounding error is wrong. It should be only half of the divider.
> 
> Fixes: c14dca07a31d ("iio: cros_ec_sensors: add ChromeOS EC Contiguous Sensors driver")
> Signed-off-by: Gwendal Grignou <gwendal@...omium.org>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>

This one is kind of interesting. See below.

> ---
> 
>  drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> index 89cb0066a6e0..600942af9f9c 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> @@ -103,7 +103,7 @@ static int cros_ec_sensors_read(struct iio_dev *indio_dev,
>  			 * Do not use IIO_DEGREE_TO_RAD to avoid precision
>  			 * loss. Round to the nearest integer.
>  			 */
> -			*val = div_s64(val64 * 314159 + 9000000ULL, 1000);
> +			*val = div_s64(val64 * 314159 + 500ULL, 1000);
That is only one of two divides going on.  Firstly we divide by 1000 here,
then we provide it in fractional form which means that the actual value you get
from sysfs etc is 
val/val2.  It's this one we are protecting against rounding error on I guess.
Now this is even less obviously because it's not 18000 either, but 
18000 * 2^CROS_EC_SENSOR_BITS.

Which ultimately means neither answer is correct. Hmm.
Not totally sure what the right answer actually is..

Jonathan

>  			*val2 = 18000 << (CROS_EC_SENSOR_BITS - 1);
>  			ret = IIO_VAL_FRACTIONAL;
>  			break;