linux-kernel - [PATCH v2] iio: gts-helpers: Round gains and scales

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>] [day] [month] [year] [list]
Message-ID: <37d3aa193e69577353d314e94463a08d488ddd8d.1701780964.git.mazziesaccount@gmail.com>
Date:   Tue, 5 Dec 2023 15:02:02 +0200
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Matti Vaittinen <mazziesaccount@...il.com>,
        Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Matti Vaittinen <mazziesaccount@...il.com>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Subhajit Ghosh <subhajit.ghosh@...aklogic.com>
Subject: [PATCH v2] iio: gts-helpers: Round gains and scales

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.

Finally, the loop-based implementation of the 64-bit division which is
used by GTS-helpers may be running for a _very long_ time. This patch
does also replace this implementation by a well-known variant of a 64-bit
division div64_u64().

The related discussion can be found from:
https://lore.kernel.org/all/84d7c283-e8e5-4c98-835c-fe3f6ff94f4b@gmail.com/

Do rounding when computing scales and gains.

Fixes: 38416c28e168 ("iio: light: Add gain-time-scale helpers")
Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>

---

Revision history:
v1 => v2:
- fix rounding in iio_gts_get_gain()
  The iio_gts_get_gain() is accidentally doing >> 2 when it attempts to
  divide by 2. Fix this.
- use proper 64bit division div64_u64() instead of a loop and extra 32bit
  function
- Fix iio_gts_total_gain_to_scale() documentation as it's not computing
  gain.
- Add a comment on rounding logic in iio_gts_total_gain_to_scale(). It's
  fair to point out Jonathan questioned the rounding logic and suggested
  using the more of a "de-facto" method of adding divider / 2 to value
  being divided, and checking for overflow. The proper handling of the
  overflow however resulted even more confusing code (in my  subjective
  opinion) than the (more) straightforward approach of checking the size
  of the reminder and adding +1 to result when needed.

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!
---
 drivers/iio/industrialio-gts-helper.c | 31 +++++++++++++++++++--------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
index 7653261d2dc2..19091193e430 100644
--- a/drivers/iio/industrialio-gts-helper.c
+++ b/drivers/iio/industrialio-gts-helper.c
@@ -7,6 +7,7 @@
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/export.h>
+#include <linux/math64.h>
 #include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/overflow.h>
@@ -28,28 +29,32 @@
  *		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;
-	int tmp = 1;
+	u64 full = max, half_div;
+	int tmp = 0;
 
 	if (scale > full || !scale)
 		return -EINVAL;
 
-	if (U64_MAX - full < scale) {
-		/* Risk of overflow */
-		if (full - scale < scale)
+	half_div = scale >> 1;
+
+	if (U64_MAX - full < half_div) {
+		/*
+		 * Would overflow when adding half_div to full. Hence we need
+		 * to subtract scale from full if full is big enough.
+		 */
+		if (full - scale <= half_div)
 			return 1;
 
 		full -= scale;
 		tmp++;
 	}
 
-	while (full > scale * (u64)tmp)
-		tmp++;
+	tmp += div64_u64(full + half_div, scale);
 
 	return tmp;
 }
@@ -140,10 +145,18 @@ int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain,
 				int *scale_int, int *scale_nano)
 {
 	u64 tmp;
+	int rem;
 
 	tmp = gts->max_scale;
 
-	do_div(tmp, total_gain);
+	rem = do_div(tmp, total_gain);
+
+	/*
+	 * Round up if remainder is equal to or greater than the half of
+	 * the divider.
+	 */
+	if (total_gain > 1 && rem >= total_gain / 2)
+		tmp += 1ULL;
 
 	return iio_gts_delinearize(tmp, NANO, scale_int, scale_nano);
 }
-- 
2.41.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)