diff mbox series

[v12,09/16] iio: afe: rescale: fix accuracy for small fractional scales

Message ID 20220108205319.2046348-10-liambeguin@gmail.com
State Accepted
Commit f5fc003d48033559314f1c9de8198f58f14ed557
Headers show
Series [v12,01/16] iio: inkern: apply consumer scale on IIO_VAL_INT cases | expand

Commit Message

Liam Beguin Jan. 8, 2022, 8:53 p.m. UTC 
The approximation caused by integer divisions can be costly on smaller
scale values since the decimal part is significant compared to the
integer part. Switch to an IIO_VAL_INT_PLUS_NANO scale type in such
cases to maintain accuracy.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
Reviewed-by: Peter Rosin <peda@axentia.se>
---
 drivers/iio/afe/iio-rescale.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

Comments

Andy Shevchenko Jan. 9, 2022, 1:02 p.m. UTC | #1 
On Sat, Jan 8, 2022 at 10:53 PM Liam Beguin <liambeguin@gmail.com> wrote:
>
> The approximation caused by integer divisions can be costly on smaller
> scale values since the decimal part is significant compared to the
> integer part. Switch to an IIO_VAL_INT_PLUS_NANO scale type in such
> cases to maintain accuracy.

...

> +               tmp = 1 << *val2;

Unfortunately, potential UB is still here. I think you missed a subtle
detail in the implementation of BIT()/BIT_ULL(). Let's put it here:

  #define BIT(nr) (UL(1) << (nr))

where

  #define UL(x) (_UL(x))
  #define _UL(x) (_AC(x, UL))

For non-assembler case

  #define __AC(X,Y) (X##Y)
  #define _AC(X,Y) __AC(X,Y)

Now you may easily see the difference.

...

> +               rem2 = *val % (int)tmp;
> +               *val = *val / (int)tmp;
> +
> +               *val2 = rem / (int)tmp;

Hmm... You divide s64 by 10^9, which means that the maximum value can
be ~10^10 / 2 (because 2^64-1 ~= 10^19), but this _might_ be bigger
than 'int' can hold. Can you confirm that tmp can't be so big?
diff mbox series

Patch

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 68eb3e341939..ed3ef994997f 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -24,7 +24,7 @@  int rescale_process_scale(struct rescale *rescale, int scale_type,
 			  int *val, int *val2)
 {
 	s64 tmp;
-	s32 rem;
+	s32 rem, rem2;
 	u32 mult;
 	u32 neg;
 
@@ -43,9 +43,23 @@  int rescale_process_scale(struct rescale *rescale, int scale_type,
 		tmp = (s64)*val * 1000000000LL;
 		tmp = div_s64(tmp, rescale->denominator);
 		tmp *= rescale->numerator;
-		tmp = div_s64(tmp, 1000000000LL);
+
+		tmp = div_s64_rem(tmp, 1000000000LL, &rem);
 		*val = tmp;
-		return scale_type;
+
+		if (!rem)
+			return scale_type;
+
+		tmp = 1 << *val2;
+
+		rem2 = *val % (int)tmp;
+		*val = *val / (int)tmp;
+
+		*val2 = rem / (int)tmp;
+		if (rem2)
+			*val2 += div_s64((s64)rem2 * 1000000000LL, tmp);
+
+		return IIO_VAL_INT_PLUS_NANO;
 	case IIO_VAL_INT_PLUS_NANO:
 	case IIO_VAL_INT_PLUS_MICRO:
 		mult = scale_type == IIO_VAL_INT_PLUS_NANO ? GIGA : MEGA;