Re: [PATCH] iio: gts-helpers: Round gains and scales
From: Matti Vaittinen
Date: Tue Nov 28 2023 - 06:57:05 EST
On 11/27/23 09:48, Matti Vaittinen wrote:
On 11/26/23 19:26, Jonathan Cameron wrote:
On Tue, 31 Oct 2023 11:50:46 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
The GTS helpers do flooring of scale when calculating available scales.
This results available-scales to be reported smaller than they should
when the division in scale computation resulted remainder greater than
half of the divider. (decimal part of result > 0.5)
Furthermore, when gains are computed based on scale, the gain resulting
from the scale computation is also floored. As a consequence the
floored scales reported by available scales may not match the gains that
can be set.
The related discussion can be found from:
https://lore.kernel.org/all/84d7c283-e8e5-4c98-835c-fe3f6ff94f4b@xxxxxxxxx/
Do rounding when computing scales and gains.
Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
Hi Matti,
A few questions inline about the maths.
I appreciate the questions :) Thanks!
---
Subjahit, is there any chance you test this patch with your driver? Can
you drop the:
if (val2 % 10)
val2 += 1;
from scale setting and do you see written and read scales matching?
I did run a few Kunit tests on this change - but I'm still a bit jumpy
on it... Reviewing/testing is highly appreciated!
Just in case someone is interested in seeing the Kunit tests, they're
somewhat unpolished & crude and can emit noisy debug prints - but can
anyways be found from:
https://github.com/M-Vaittinen/linux/commits/iio-gts-helpers-test-v6.6
---
drivers/iio/industrialio-gts-helper.c | 58 +++++++++++++++++++++++----
1 file changed, 50 insertions(+), 8 deletions(-)
diff --git a/drivers/iio/industrialio-gts-helper.c
b/drivers/iio/industrialio-gts-helper.c
index 7653261d2dc2..7dc144ac10c8 100644
--- a/drivers/iio/industrialio-gts-helper.c
+++ b/drivers/iio/industrialio-gts-helper.c
@@ -18,6 +18,32 @@
#include <linux/iio/iio-gts-helper.h>
#include <linux/iio/types.h>
+static int iio_gts_get_gain_32(u64 full, unsigned int scale)
+{
+ unsigned int full32 = (unsigned int) full;
+ unsigned int rem;
+ int result;
+
+ if (full == (u64)full32) {
+ unsigned int rem;
+
+ result = full32 / scale;
+ rem = full32 - scale * result;
+ if (rem >= scale / 2)
+ result++;
+
+ return result;
+ }
+
+ rem = do_div(full, scale);
As below, can we just add scale/2 to full in the do_div?
The rationale for doing is it in this way is to prevent (theoretical?)
overflow when adding scale/2 to full. Maybe this warrants adding a comment?
+ if ((u64)rem >= scale / 2)
+ result = full + 1;
+ else
+ result = full;
+
+ return result;
+}
+
/**
* iio_gts_get_gain - Convert scale to total gain
*
@@ -28,30 +54,42 @@
* scale is 64 100 000 000.
* @scale: Linearized scale to compute the gain for.
*
- * Return: (floored) gain corresponding to the scale. -EINVAL if
scale
+ * Return: (rounded) gain corresponding to the scale. -EINVAL if
scale
* is invalid.
*/
static int iio_gts_get_gain(const u64 max, const u64 scale)
{
- u64 full = max;
+ u64 full = max, half_div;
+ unsigned int scale32 = (unsigned int) scale;
int tmp = 1;
- if (scale > full || !scale)
+ if (scale / 2 > full || !scale)
Seems odd. Why are we checking scale / 2 here?
I am pretty sure I have been thinking of rounding 0.5 to 1.
return -EINVAL;
+ /*
+ * The loop-based implementation below will potentially run _long_
+ * if we have a small scale and large 'max' - which may be
needed when
+ * GTS is used for channels returning specific units. Luckily we
can
+ * avoid the loop when scale is small and fits in 32 bits.
+ */
+ if ((u64)scale32 == scale)
+ return iio_gts_get_gain_32(full, scale32);
+
if (U64_MAX - full < scale) {
/* Risk of overflow */
- if (full - scale < scale)
+ if (full - scale / 2 < scale)
return 1;
full -= scale;
tmp++;
}
- while (full > scale * (u64)tmp)
+ half_div = scale >> 2;
Why divide by 4? Looks like classic issue with using shifts for division
causing confusion.
Yes. Looks like a brainfart to me. I need to fire-up my tests and revise
this (and the check you asked about above). It seems to take a while
from me to wrap my head around this again...
Thanks for pointing this out!
+
+ while (full + half_div >= scale * (u64)tmp)
tmp++;
Oh. This is a problem. Adding half_div to full here can cause the scale
* (u64)tmp to overflow. The overflow-prevention above only ensures full
is smaller than the U64_MAX - scale. Here we should ensure full +
half_div is less than U64_MAX - scale to ensure the loop always stops.
All in all, this is horrible. Just ran a quick and dirty test on my
laptop, and using 0xFFFF FFFF FFFF FFFF as full and 0x1 0000 0000 as
scale (without the half_div addition) ran this loop for several seconds.
Sigh. My brains jammed. I know this can not be an unique problem. I am
sure there exists a better solution somewhere - any pointers would be
appreciated :)
- return tmp;
+ return tmp - 1;
}
/**
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~