[PATCH v2 0/6] Add rounding macros and enable KUnit tests

[PATCH v2 0/6] Add rounding macros and enable KUnit tests

[Devarsh Thakkar]

This adds new rounding macros to round to closest integer along with
enabling KUnit tests for all math.h macros.  It also updates kernel-doc to
render math.h related documentation and lastly updates the imagination jpeg
encoder to use the newly introduced rounding macros to find nearest value.

Also to note that math.h is orphaned and so these changes need
to be pulled by subsystem using them as discussed in below threads:

Link:
https://lore.kernel.org/all/1db64af7-3688-4bcc-a671-9440d8f02d1a@xs4all.nl/
https://lore.kernel.org/all/ZmHDWeuezCEgj20m@smile.fi.intel.com/

Changelog:
V2:
- Fix grammar in new rounding off macro descriptions
- Use roundup for rounclosest to avoid overflow

Previous patch series:
https://lore.kernel.org/all/20240708155943.2314427-1-devarsht@ti.com/#t

Rangediff (V2->V1):
https://gist.github.com/devarsht/a3c4420869d3f66fdc88e0af6a0f0a95

Daniel Latypov (1):
  lib: Add basic KUnit test for lib/math

Devarsh Thakkar (5):
  math.h: Add macros for rounding to the closest value
  math.h: Use kernel-doc syntax for division macros
  Documentation: core-api: Add math.h macros and functions
  lib: math_kunit: Add tests for new macros related to rounding to
    nearest value
  media: imagination: Round to closest multiple for cropping region

 Documentation/core-api/kernel-api.rst         |   6 +
 .../platform/imagination/e5010-jpeg-enc.c     |   8 +-
 include/linux/math.h                          |  86 ++++-
 lib/math/Kconfig                              |  14 +
 lib/math/Makefile                             |   1 +
 lib/math/math_kunit.c                         | 329 ++++++++++++++++++
 6 files changed, 434 insertions(+), 10 deletions(-)
 create mode 100644 lib/math/math_kunit.c

-- 
2.39.1

[PATCH v2 1/6] math.h: Add macros for rounding to the closest value

[Devarsh Thakkar]

Add below rounding related macros:

round_closest_up(x, y) : Rounds x to the closest multiple of y where y is a
power of 2, with a preference to round up in case two nearest values are
possible.

round_closest_down(x, y) : Rounds x to the closest multiple of y where y is
a power of 2, with a preference to round down in case two nearest values
are possible.

roundclosest(x, y) : Rounds x to the closest multiple of y, this macro
should generally be used only when y is not multiple of 2 as otherwise
round_closest* macros should be used which are much faster.

Examples:
 * round_closest_up(17, 4) = 16
 * round_closest_up(15, 4) = 16
 * round_closest_up(14, 4) = 16
 * round_closest_down(17, 4) = 16
 * round_closest_down(15, 4) = 16
 * round_closest_down(14, 4) = 12
 * roundclosest(21, 5) = 20
 * roundclosest(19, 5) = 20
 * roundclosest(17, 5) = 15

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
NOTE: This patch is inspired from the Mentor Graphics IPU driver [1]
which uses similar macro locally and which is updated in further patch
in the series to use this generic macro instead along with other drivers
having similar requirements.

Link: https://elixir.bootlin.com/linux/v6.8.9/source/drivers/gpu/ipu-v3/ipu-image-convert.c#L480 [1]

Past discussions and alignment on this:
https://lore.kernel.org/all/7e3ad816-6a2a-4e02-9b41-03a8562812ad@ti.com/#r
https://lore.kernel.org/all/ZkISG6p1tn9Do-xY@smile.fi.intel.com/#r
https://lore.kernel.org/all/ZlTt-YWzyRyhmT9n@smile.fi.intel.com/
https://lore.kernel.org/all/ZmHDWeuezCEgj20m@smile.fi.intel.com/
https://lore.kernel.org/all/ZloMFfGKLry6EWNL@smile.fi.intel.com/

Changelog:
V2:
- Fix grammar in macro description
- Update roundclosest macro to use roundup to avoid overflow scenario
---
 include/linux/math.h | 63 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/include/linux/math.h b/include/linux/math.h
index f5f18dc3616b..b59a02a007d7 100644
--- a/include/linux/math.h
+++ b/include/linux/math.h
@@ -34,6 +34,52 @@
  */
 #define round_down(x, y) ((x) & ~__round_mask(x, y))
 
+/**
+ * round_closest_up - round closest to be multiple of the specified value
+ *                    (which is power of 2) with preference to rounding up
+ * @x: the value to round
+ * @y: multiple to round closest to (must be a power of 2)
+ *
+ * Rounds @x to the closest multiple of @y (which must be a power of 2).
+ * The value can be rounded up or rounded down depending on the rounded
+ * value's closeness to the specified value. Also, there can be two closest
+ * values, i.e. the difference between the specified value and its rounded-up
+ * and rounded-down values could be the same. In that case, the rounded-up
+ * value is preferred.
+ *
+ * To perform arbitrary rounding to the closest value (not multiple of 2), use
+ * roundclosest().
+ *
+ * Examples:
+ * * round_closest_up(17, 4) = 16
+ * * round_closest_up(15, 4) = 16
+ * * round_closest_up(14, 4) = 16
+ */
+#define round_closest_up(x, y) round_down((x) + (y) / 2, (y))
+
+/**
+ * round_closest_down - round closest to be multiple of the specified value
+ *                      (which is power of 2) with preference to rounding down
+ * @x: the value to round
+ * @y: multiple to round closest to (must be a power of 2)
+ *
+ * Rounds @x to the closest multiple of @y (which must be a power of 2).
+ * The value can be rounded up or rounded down depending on the rounded
+ * value's closeness to the specified value. Also, there can be two closest
+ * values, i.e. the difference between the specified value and its rounded-up
+ * and rounded-down values could be the same. In that case, the rounded-down
+ * value is preferred.
+ *
+ * To perform arbitrary rounding to the closest value (not multiple of 2), use
+ * roundclosest().
+ *
+ * Examples:
+ * * round_closest_down(17, 4) = 16
+ * * round_closest_down(15, 4) = 16
+ * * round_closest_down(14, 4) = 12
+ */
+#define round_closest_down(x, y) round_up((x) - (y) / 2, (y))
+
 #define DIV_ROUND_UP __KERNEL_DIV_ROUND_UP
 
 #define DIV_ROUND_DOWN_ULL(ll, d) \
@@ -77,6 +123,23 @@
 }							\
 )
 
+/**
+ * roundclosest - round to the nearest multiple
+ * @x: the value to round
+ * @y: multiple to round nearest to
+ *
+ * Rounds @x to the nearest multiple of @y.
+ * The rounded value can be greater or less than @x depending
+ * upon its nearness to @x. If @y is always a power of 2, consider
+ * using the faster round_closest_up() or round_closest_down().
+ *
+ * Examples:
+ * * roundclosest(21, 5) = 20
+ * * roundclosest(19, 5) = 20
+ * * roundclosest(17, 5) = 15
+ */
+#define roundclosest(x, y) roundup((x) - (y) / 2, (y))
+
 /*
  * Divide positive or negative dividend by positive or negative divisor
  * and round to closest integer. Result is undefined for negative
-- 
2.39.1

[PATCH v2 2/6] math.h: Use kernel-doc syntax for division macros

[Devarsh Thakkar]

Enable reST documentation for division macros DIV_ROUND_CLOSEST and
DIV_ROUND_CLOSEST_ULL by using kernel-doc markup and syntax for documenting
them.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V2: Fix spelling for division and reST
---
 include/linux/math.h | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/include/linux/math.h b/include/linux/math.h
index b59a02a007d7..f3ba3ebe4fcb 100644
--- a/include/linux/math.h
+++ b/include/linux/math.h
@@ -140,9 +140,14 @@
  */
 #define roundclosest(x, y) roundup((x) - (y) / 2, (y))
 
-/*
- * Divide positive or negative dividend by positive or negative divisor
- * and round to closest integer. Result is undefined for negative
+/**
+ * DIV_ROUND_CLOSEST - Divide positive or negative dividend by positive or
+ *		       negative divisor and round to closest value
+ * @x: dividend value
+ * @divisor: divisor value
+ *
+ * Divide positive or negative dividend value @x by positive or negative
+ * @divisor value and round to closest integer. Result is undefined for negative
  * divisors if the dividend variable type is unsigned and for negative
  * dividends if the divisor variable type is unsigned.
  */
@@ -157,9 +162,15 @@
 		(((__x) - ((__d) / 2)) / (__d));	\
 }							\
 )
-/*
- * Same as above but for u64 dividends. divisor must be a 32-bit
- * number.
+
+/**
+ * DIV_ROUND_CLOSEST_ULL - Divide 64-bit unsigned dividend by 32-bit divisor and
+ *			   round to closest value
+ * @x: unsigned 64-bit dividend
+ * @divisor: 32-bit divisor
+ *
+ * Divide unsigned 64-bit dividend value @x by 32-bit @divisor value
+ * and round to closest integer. Result is undefined for negative divisors.
  */
 #define DIV_ROUND_CLOSEST_ULL(x, divisor)(		\
 {							\
-- 
2.39.1

[PATCH v2 3/6] Documentation: core-api: Add math.h macros and functions

[Devarsh Thakkar]

Add documentation for rounding, absolute value, division and 32-bit scaling
related macros and functions exported by math.h header file.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Randy Dunlap <rdunlap@infradead.org>
Tested-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V2: Update commit message
---
 Documentation/core-api/kernel-api.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/core-api/kernel-api.rst b/Documentation/core-api/kernel-api.rst
index ae92a2571388..7de494e76fa6 100644
--- a/Documentation/core-api/kernel-api.rst
+++ b/Documentation/core-api/kernel-api.rst
@@ -185,6 +185,12 @@ Division Functions
 .. kernel-doc:: lib/math/gcd.c
    :export:
 
+Rounding, absolute value, division and 32-bit scaling functions
+---------------------------------------------------------------
+
+.. kernel-doc:: include/linux/math.h
+   :internal:
+
 UUID/GUID
 ---------
 
-- 
2.39.1

[PATCH v2 4/6] lib: Add basic KUnit test for lib/math

[Devarsh Thakkar]

From: Daniel Latypov <dlatypov@google.com>

Add basic test coverage for files that don't require any config options:
* part of math.h (what seem to be the most commonly used macros)
* gcd.c
* lcm.c
* int_sqrt.c
* reciprocal_div.c
(Ignored int_pow.c since it's a simple textbook algorithm.)

These tests aren't particularly interesting, but they
* provide short and simple examples of parameterized tests
* provide a place to add tests for any new files in this dir
* are written so adding new test cases to cover edge cases should be
  easy
  * looking at code coverage, we hit all the branches in the .c files

Signed-off-by: Daniel Latypov <dlatypov@google.com>
Reviewed-by: David Gow <davidgow@google.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
[devarsht: Rebase to 6.11, remove kernel.h, update Kconfig and change license to GPL]
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V2: Rebased to 6.11
---
 lib/math/Kconfig      |  14 ++
 lib/math/Makefile     |   1 +
 lib/math/math_kunit.c | 294 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 309 insertions(+)
 create mode 100644 lib/math/math_kunit.c

diff --git a/lib/math/Kconfig b/lib/math/Kconfig
index 0634b428d0cb..f738d8a433ea 100644
--- a/lib/math/Kconfig
+++ b/lib/math/Kconfig
@@ -15,3 +15,17 @@ config PRIME_NUMBERS
 
 config RATIONAL
 	tristate
+
+config MATH_KUNIT_TEST
+	tristate "KUnit test for math helper functions"
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
+
+	help
+	  This builds unit test for math helper functions as defined in lib/math
+	  and math.h.
+
+	  For more information on KUNIT and unit tests in general, please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+	  If unsure, say N.
diff --git a/lib/math/Makefile b/lib/math/Makefile
index 91fcdb0c9efe..dcf65d10dab2 100644
--- a/lib/math/Makefile
+++ b/lib/math/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_RATIONAL)		+= rational.o
 
 obj-$(CONFIG_TEST_DIV64)	+= test_div64.o
 obj-$(CONFIG_RATIONAL_KUNIT_TEST) += rational-test.o
+obj-$(CONFIG_MATH_KUNIT_TEST) += math_kunit.o
diff --git a/lib/math/math_kunit.c b/lib/math/math_kunit.c
new file mode 100644
index 000000000000..be27f2afb8e4
--- /dev/null
+++ b/lib/math/math_kunit.c
@@ -0,0 +1,294 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Simple KUnit suite for math helper funcs that are always enabled.
+ *
+ * Copyright (C) 2020, Google LLC.
+ * Author: Daniel Latypov <dlatypov@google.com>
+ */
+
+#include <kunit/test.h>
+#include <linux/gcd.h>
+#include <linux/lcm.h>
+#include <linux/math.h>
+#include <linux/module.h>
+#include <linux/reciprocal_div.h>
+#include <linux/types.h>
+
+static void abs_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, abs((char)0), (char)0);
+	KUNIT_EXPECT_EQ(test, abs((char)42), (char)42);
+	KUNIT_EXPECT_EQ(test, abs((char)-42), (char)42);
+
+	/* The expression in the macro is actually promoted to an int. */
+	KUNIT_EXPECT_EQ(test, abs((short)0),  0);
+	KUNIT_EXPECT_EQ(test, abs((short)42),  42);
+	KUNIT_EXPECT_EQ(test, abs((short)-42),  42);
+
+	KUNIT_EXPECT_EQ(test, abs(0),  0);
+	KUNIT_EXPECT_EQ(test, abs(42),  42);
+	KUNIT_EXPECT_EQ(test, abs(-42),  42);
+
+	KUNIT_EXPECT_EQ(test, abs(0L), 0L);
+	KUNIT_EXPECT_EQ(test, abs(42L), 42L);
+	KUNIT_EXPECT_EQ(test, abs(-42L), 42L);
+
+	KUNIT_EXPECT_EQ(test, abs(0LL), 0LL);
+	KUNIT_EXPECT_EQ(test, abs(42LL), 42LL);
+	KUNIT_EXPECT_EQ(test, abs(-42LL), 42LL);
+
+	/* Unsigned types get casted to signed. */
+	KUNIT_EXPECT_EQ(test, abs(0ULL), 0LL);
+	KUNIT_EXPECT_EQ(test, abs(42ULL), 42LL);
+}
+
+static void int_sqrt_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, int_sqrt(0UL), 0UL);
+	KUNIT_EXPECT_EQ(test, int_sqrt(1UL), 1UL);
+	KUNIT_EXPECT_EQ(test, int_sqrt(4UL), 2UL);
+	KUNIT_EXPECT_EQ(test, int_sqrt(5UL), 2UL);
+	KUNIT_EXPECT_EQ(test, int_sqrt(8UL), 2UL);
+	KUNIT_EXPECT_EQ(test, int_sqrt(1UL << 30), 1UL << 15);
+}
+
+static void round_up_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, round_up(0, 1), 0);
+	KUNIT_EXPECT_EQ(test, round_up(1, 2), 2);
+	KUNIT_EXPECT_EQ(test, round_up(3, 2), 4);
+	KUNIT_EXPECT_EQ(test, round_up((1 << 30) - 1, 2), 1 << 30);
+	KUNIT_EXPECT_EQ(test, round_up((1 << 30) - 1, 1 << 29), 1 << 30);
+}
+
+static void round_down_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, round_down(0, 1), 0);
+	KUNIT_EXPECT_EQ(test, round_down(1, 2), 0);
+	KUNIT_EXPECT_EQ(test, round_down(3, 2), 2);
+	KUNIT_EXPECT_EQ(test, round_down((1 << 30) - 1, 2), (1 << 30) - 2);
+	KUNIT_EXPECT_EQ(test, round_down((1 << 30) - 1, 1 << 29), 1 << 29);
+}
+
+/* These versions can round to numbers that aren't a power of two */
+static void roundup_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, roundup(0, 1), 0);
+	KUNIT_EXPECT_EQ(test, roundup(1, 2), 2);
+	KUNIT_EXPECT_EQ(test, roundup(3, 2), 4);
+	KUNIT_EXPECT_EQ(test, roundup((1 << 30) - 1, 2), 1 << 30);
+	KUNIT_EXPECT_EQ(test, roundup((1 << 30) - 1, 1 << 29), 1 << 30);
+
+	KUNIT_EXPECT_EQ(test, roundup(3, 2), 4);
+	KUNIT_EXPECT_EQ(test, roundup(4, 3), 6);
+}
+
+static void rounddown_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, rounddown(0, 1), 0);
+	KUNIT_EXPECT_EQ(test, rounddown(1, 2), 0);
+	KUNIT_EXPECT_EQ(test, rounddown(3, 2), 2);
+	KUNIT_EXPECT_EQ(test, rounddown((1 << 30) - 1, 2), (1 << 30) - 2);
+	KUNIT_EXPECT_EQ(test, rounddown((1 << 30) - 1, 1 << 29), 1 << 29);
+
+	KUNIT_EXPECT_EQ(test, rounddown(3, 2), 2);
+	KUNIT_EXPECT_EQ(test, rounddown(4, 3), 3);
+}
+
+static void div_round_up_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, DIV_ROUND_UP(0, 1), 0);
+	KUNIT_EXPECT_EQ(test, DIV_ROUND_UP(20, 10), 2);
+	KUNIT_EXPECT_EQ(test, DIV_ROUND_UP(21, 10), 3);
+	KUNIT_EXPECT_EQ(test, DIV_ROUND_UP(21, 20), 2);
+	KUNIT_EXPECT_EQ(test, DIV_ROUND_UP(21, 99), 1);
+}
+
+static void div_round_closest_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, DIV_ROUND_CLOSEST(0, 1), 0);
+	KUNIT_EXPECT_EQ(test, DIV_ROUND_CLOSEST(20, 10), 2);
+	KUNIT_EXPECT_EQ(test, DIV_ROUND_CLOSEST(21, 10), 2);
+	KUNIT_EXPECT_EQ(test, DIV_ROUND_CLOSEST(25, 10), 3);
+}
+
+/* Generic test case for unsigned long inputs. */
+struct test_case {
+	unsigned long a, b;
+	unsigned long result;
+};
+
+static struct test_case gcd_cases[] = {
+	{
+		.a = 0, .b = 0,
+		.result = 0,
+	},
+	{
+		.a = 0, .b = 1,
+		.result = 1,
+	},
+	{
+		.a = 2, .b = 2,
+		.result = 2,
+	},
+	{
+		.a = 2, .b = 4,
+		.result = 2,
+	},
+	{
+		.a = 3, .b = 5,
+		.result = 1,
+	},
+	{
+		.a = 3 * 9, .b = 3 * 5,
+		.result = 3,
+	},
+	{
+		.a = 3 * 5 * 7, .b = 3 * 5 * 11,
+		.result = 15,
+	},
+	{
+		.a = 1 << 21,
+		.b = (1 << 21) - 1,
+		.result = 1,
+	},
+};
+
+KUNIT_ARRAY_PARAM(gcd, gcd_cases, NULL);
+
+static void gcd_test(struct kunit *test)
+{
+	const char *message_fmt = "gcd(%lu, %lu)";
+	const struct test_case *test_param = test->param_value;
+
+	KUNIT_EXPECT_EQ_MSG(test, test_param->result,
+			    gcd(test_param->a, test_param->b),
+			    message_fmt, test_param->a,
+			    test_param->b);
+
+	if (test_param->a == test_param->b)
+		return;
+
+	/* gcd(a,b) == gcd(b,a) */
+	KUNIT_EXPECT_EQ_MSG(test, test_param->result,
+			    gcd(test_param->b, test_param->a),
+			    message_fmt, test_param->b,
+			    test_param->a);
+}
+
+static struct test_case lcm_cases[] = {
+	{
+		.a = 0, .b = 0,
+		.result = 0,
+	},
+	{
+		.a = 0, .b = 1,
+		.result = 0,
+	},
+	{
+		.a = 1, .b = 2,
+		.result = 2,
+	},
+	{
+		.a = 2, .b = 2,
+		.result = 2,
+	},
+	{
+		.a = 3 * 5, .b = 3 * 7,
+		.result = 3 * 5 * 7,
+	},
+};
+
+KUNIT_ARRAY_PARAM(lcm, lcm_cases, NULL);
+
+static void lcm_test(struct kunit *test)
+{
+	const char *message_fmt = "lcm(%lu, %lu)";
+	const struct test_case *test_param = test->param_value;
+
+	KUNIT_EXPECT_EQ_MSG(test, test_param->result,
+			    lcm(test_param->a, test_param->b),
+			    message_fmt, test_param->a,
+			    test_param->b);
+
+	if (test_param->a == test_param->b)
+		return;
+
+	/* lcm(a,b) == lcm(b,a) */
+	KUNIT_EXPECT_EQ_MSG(test, test_param->result,
+			    lcm(test_param->b, test_param->a),
+			    message_fmt, test_param->b,
+			    test_param->a);
+}
+
+struct u32_test_case {
+	u32 a, b;
+	u32 result;
+};
+
+static struct u32_test_case reciprocal_div_cases[] = {
+	{
+		.a = 0, .b = 1,
+		.result = 0,
+	},
+	{
+		.a = 42, .b = 20,
+		.result = 2,
+	},
+	{
+		.a = 42, .b = 9999,
+		.result = 0,
+	},
+	{
+		.a = (1 << 16), .b = (1 << 14),
+		.result = 1 << 2,
+	},
+};
+
+KUNIT_ARRAY_PARAM(reciprocal_div, reciprocal_div_cases, NULL);
+
+static void reciprocal_div_test(struct kunit *test)
+{
+	const struct u32_test_case *test_param = test->param_value;
+	struct reciprocal_value rv = reciprocal_value(test_param->b);
+
+	KUNIT_EXPECT_EQ_MSG(test, test_param->result,
+			    reciprocal_divide(test_param->a, rv),
+			    "reciprocal_divide(%u, %u)",
+			    test_param->a, test_param->b);
+}
+
+static void reciprocal_scale_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, reciprocal_scale(0u, 100), 0u);
+	KUNIT_EXPECT_EQ(test, reciprocal_scale(1u, 100), 0u);
+	KUNIT_EXPECT_EQ(test, reciprocal_scale(1u << 4, 1 << 28), 1u);
+	KUNIT_EXPECT_EQ(test, reciprocal_scale(1u << 16, 1 << 28), 1u << 12);
+	KUNIT_EXPECT_EQ(test, reciprocal_scale(~0u, 1 << 28), (1u << 28) - 1);
+}
+
+static struct kunit_case math_test_cases[] = {
+	KUNIT_CASE(abs_test),
+	KUNIT_CASE(int_sqrt_test),
+	KUNIT_CASE(round_up_test),
+	KUNIT_CASE(round_down_test),
+	KUNIT_CASE(roundup_test),
+	KUNIT_CASE(rounddown_test),
+	KUNIT_CASE(div_round_up_test),
+	KUNIT_CASE(div_round_closest_test),
+	KUNIT_CASE_PARAM(gcd_test, gcd_gen_params),
+	KUNIT_CASE_PARAM(lcm_test, lcm_gen_params),
+	KUNIT_CASE_PARAM(reciprocal_div_test, reciprocal_div_gen_params),
+	KUNIT_CASE(reciprocal_scale_test),
+	{}
+};
+
+static struct kunit_suite math_test_suite = {
+	.name = "lib-math",
+	.test_cases = math_test_cases,
+};
+
+kunit_test_suites(&math_test_suite);
+
+MODULE_DESCRIPTION("Math helper functions kunit test");
+MODULE_LICENSE("GPL");
-- 
2.39.1

[PATCH v2 5/6] lib: math_kunit: Add tests for new macros related to rounding to nearest value

[Devarsh Thakkar]

Add tests for round_closest_up/down and roundclosest macros which round
to nearest multiple of specified argument. These are tested with kunit
tool as shared here [1] :

Link: https://gist.github.com/devarsht/3f9042825be3da4e133b8f4eda067876 [1]
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
V2: No change
---
 lib/math/math_kunit.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/lib/math/math_kunit.c b/lib/math/math_kunit.c
index be27f2afb8e4..05022f010be6 100644
--- a/lib/math/math_kunit.c
+++ b/lib/math/math_kunit.c
@@ -70,6 +70,26 @@ static void round_down_test(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, round_down((1 << 30) - 1, 1 << 29), 1 << 29);
 }
 
+static void round_closest_up_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, round_closest_up(17, 4), 16);
+	KUNIT_EXPECT_EQ(test, round_closest_up(15, 4), 16);
+	KUNIT_EXPECT_EQ(test, round_closest_up(14, 4), 16);
+	KUNIT_EXPECT_EQ(test, round_closest_up((1 << 30) - 1, 1 << 30), 1 << 30);
+	KUNIT_EXPECT_EQ(test, round_closest_up((1 << 30) + 1, 1 << 30), 1 << 30);
+	KUNIT_EXPECT_EQ(test, round_closest_up((1 << 30) - 1, 2), 1 << 30);
+}
+
+static void round_closest_down_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, round_closest_down(17, 4), 16);
+	KUNIT_EXPECT_EQ(test, round_closest_down(15, 4), 16);
+	KUNIT_EXPECT_EQ(test, round_closest_down(14, 4), 12);
+	KUNIT_EXPECT_EQ(test, round_closest_down((1 << 30) - 1, 1 << 30), 1 << 30);
+	KUNIT_EXPECT_EQ(test, round_closest_down((1 << 30) + 1, 1 << 30), 1 << 30);
+	KUNIT_EXPECT_EQ(test, round_closest_down((1 << 30) - 1, 2), (1 << 30) - 2);
+}
+
 /* These versions can round to numbers that aren't a power of two */
 static void roundup_test(struct kunit *test)
 {
@@ -95,6 +115,18 @@ static void rounddown_test(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, rounddown(4, 3), 3);
 }
 
+static void roundclosest_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, roundclosest(21, 5), 20);
+	KUNIT_EXPECT_EQ(test, roundclosest(19, 5), 20);
+	KUNIT_EXPECT_EQ(test, roundclosest(17, 5), 15);
+	KUNIT_EXPECT_EQ(test, roundclosest((1 << 30), 3), (1 << 30) - 1);
+	KUNIT_EXPECT_EQ(test, roundclosest((1 << 30) - 1, 1 << 29), 1 << 30);
+
+	KUNIT_EXPECT_EQ(test, roundclosest(4, 3), 3);
+	KUNIT_EXPECT_EQ(test, roundclosest(5, 3), 6);
+}
+
 static void div_round_up_test(struct kunit *test)
 {
 	KUNIT_EXPECT_EQ(test, DIV_ROUND_UP(0, 1), 0);
@@ -272,8 +304,11 @@ static struct kunit_case math_test_cases[] = {
 	KUNIT_CASE(int_sqrt_test),
 	KUNIT_CASE(round_up_test),
 	KUNIT_CASE(round_down_test),
+	KUNIT_CASE(round_closest_up_test),
+	KUNIT_CASE(round_closest_down_test),
 	KUNIT_CASE(roundup_test),
 	KUNIT_CASE(rounddown_test),
+	KUNIT_CASE(roundclosest_test),
 	KUNIT_CASE(div_round_up_test),
 	KUNIT_CASE(div_round_closest_test),
 	KUNIT_CASE_PARAM(gcd_test, gcd_gen_params),
-- 
2.39.1

[PATCH v2 6/6] media: imagination: Round to closest multiple for cropping region

[Devarsh Thakkar]

If neither of the flags to round down (V4L2_SEL_FLAG_LE) or round up
(V4L2_SEL_FLAG_GE) are specified by the user, then round to nearest
multiple of requested value while updating the crop rectangle coordinates.

Use the rounding macro which gives preference to rounding down in case two
nearest values (high and low) are possible to raise the probability of
cropping rectangle falling inside the bound region.

This complies with the VIDIOC_G_SELECTION, VIDIOC_S_SELECTION ioctl
description as documented in v4l uapi [1] which specifies that driver
should choose crop rectangle as close as possible if no flags are passed by
user-space, as quoted below :

"``0`` - The driver can adjust the rectangle size freely and shall choose a
crop/compose rectangle as close as possible to the requested
 one."

Link: https://www.kernel.org/doc/Documentation/userspace-api/media/v4l/vidioc-g-selection.rst [1]
Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V2: No change
---
 drivers/media/platform/imagination/e5010-jpeg-enc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
index 187f2d8abfbb..6c3687445803 100644
--- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
+++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
@@ -514,10 +514,10 @@ static int e5010_s_selection(struct file *file, void *fh, struct v4l2_selection
 
 	switch (s->flags) {
 	case 0:
-		s->r.width = round_down(s->r.width, queue->fmt->frmsize.step_width);
-		s->r.height = round_down(s->r.height, queue->fmt->frmsize.step_height);
-		s->r.left = round_down(s->r.left, queue->fmt->frmsize.step_width);
-		s->r.top = round_down(s->r.top, 2);
+		s->r.width = round_closest_down(s->r.width, queue->fmt->frmsize.step_width);
+		s->r.height = round_closest_down(s->r.height, queue->fmt->frmsize.step_height);
+		s->r.left = round_closest_down(s->r.left, queue->fmt->frmsize.step_width);
+		s->r.top = round_closest_down(s->r.top, 2);
 
 		if (s->r.left + s->r.width > queue->width)
 			s->r.width = round_down(s->r.width + s->r.left - queue->width,
-- 
2.39.1

Re: [PATCH v2 1/6] math.h: Add macros for rounding to the closest value

[Andy Shevchenko]

On Mon, Aug 26, 2024 at 08:38:17PM +0530, Devarsh Thakkar wrote:
> Add below rounding related macros:
> 
> round_closest_up(x, y) : Rounds x to the closest multiple of y where y is a
> power of 2, with a preference to round up in case two nearest values are
> possible.
> 
> round_closest_down(x, y) : Rounds x to the closest multiple of y where y is
> a power of 2, with a preference to round down in case two nearest values
> are possible.
> 
> roundclosest(x, y) : Rounds x to the closest multiple of y, this macro
> should generally be used only when y is not multiple of 2 as otherwise
> round_closest* macros should be used which are much faster.

I understand the point, but if you need to send a v3, please explain
the equivalency between roundclosest() and one (or both?) of the
round_closest_*() in case the argument is power-of-2.

Because from the above I don't see what I'll get in such a case.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] math.h: Add macros for rounding to the closest value

[Devarsh Thakkar]

Hi Andy,

Thanks for the review.

On 26/08/24 23:14, Andy Shevchenko wrote:
> On Mon, Aug 26, 2024 at 08:38:17PM +0530, Devarsh Thakkar wrote:
>> Add below rounding related macros:
>>
>> round_closest_up(x, y) : Rounds x to the closest multiple of y where y is a
>> power of 2, with a preference to round up in case two nearest values are
>> possible.
>>
>> round_closest_down(x, y) : Rounds x to the closest multiple of y where y is
>> a power of 2, with a preference to round down in case two nearest values
>> are possible.
>>
>> roundclosest(x, y) : Rounds x to the closest multiple of y, this macro
>> should generally be used only when y is not multiple of 2 as otherwise
>> round_closest* macros should be used which are much faster.
> 
> I understand the point, but if you need to send a v3, please explain
> the equivalency between roundclosest() and one (or both?) of the
> round_closest_*() in case the argument is power-of-2.
> 

The equivalency between roundclosest w.r.t round_closest is same as
equivalency between existing macros rounddown w.r.t round_down. Functionally
both are same but the former is recommended to be used only for the scenario
where multiple is not power of 2 and latter is faster but is strictly for the
scenario where multiple is power of 2. I think the same is already summarized
well in commit message and further elaborated in the patch itself as part of
header file comments [1] so I personally don't think any update is required
w.r.t this.

[1]: https://lore.kernel.org/all/20240826150822.4057164-2-devarsht@ti.com

Regards
Devarsh

Re: [PATCH v2 1/6] math.h: Add macros for rounding to the closest value

[Jani Nikula]

On Tue, 27 Aug 2024, Devarsh Thakkar <devarsht@ti.com> wrote:
> Hi Andy,
>
> Thanks for the review.
>
> On 26/08/24 23:14, Andy Shevchenko wrote:
>> On Mon, Aug 26, 2024 at 08:38:17PM +0530, Devarsh Thakkar wrote:
>>> Add below rounding related macros:
>>>
>>> round_closest_up(x, y) : Rounds x to the closest multiple of y where y is a
>>> power of 2, with a preference to round up in case two nearest values are
>>> possible.
>>>
>>> round_closest_down(x, y) : Rounds x to the closest multiple of y where y is
>>> a power of 2, with a preference to round down in case two nearest values
>>> are possible.
>>>
>>> roundclosest(x, y) : Rounds x to the closest multiple of y, this macro
>>> should generally be used only when y is not multiple of 2 as otherwise
>>> round_closest* macros should be used which are much faster.
>> 
>> I understand the point, but if you need to send a v3, please explain
>> the equivalency between roundclosest() and one (or both?) of the
>> round_closest_*() in case the argument is power-of-2.
>> 
>
> The equivalency between roundclosest w.r.t round_closest is same as
> equivalency between existing macros rounddown w.r.t round_down. Functionally
> both are same but the former is recommended to be used only for the scenario
> where multiple is not power of 2 and latter is faster but is strictly for the
> scenario where multiple is power of 2. I think the same is already summarized
> well in commit message and further elaborated in the patch itself as part of
> header file comments [1] so I personally don't think any update is required
> w.r.t this.

I still don't think rounddown vs. round_down naming is a good example to
model anything after.

I have yet to hear a single compelling argument in favor of having a
single underscore in the middle of a name having implications about the
inputs of a macro/function.

The macros being added here are at 2 or 3 in Rusty's scale [1]. We could
aim for 6 (The name tells you how to use it), but also do opportunistic
8 (The compiler will warn if you get it wrong) for compile-time
constants.

As-is, these, and round_up/round_down, are just setting a trap for an
unsuspecting kernel developer to fall into.


BR,
Jani.


[1] https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html


>
> [1]: https://lore.kernel.org/all/20240826150822.4057164-2-devarsht@ti.com
>
> Regards
> Devarsh

-- 
Jani Nikula, Intel

Re: [PATCH v2 1/6] math.h: Add macros for rounding to the closest value

[Devarsh Thakkar]

Hi Nikula,

Thanks for the review.

On 27/08/24 18:10, Jani Nikula wrote:
> On Tue, 27 Aug 2024, Devarsh Thakkar <devarsht@ti.com> wrote:

[..]

>> The equivalency between roundclosest w.r.t round_closest is same as
>> equivalency between existing macros rounddown w.r.t round_down. Functionally
>> both are same but the former is recommended to be used only for the scenario
>> where multiple is not power of 2 and latter is faster but is strictly for the
>> scenario where multiple is power of 2. I think the same is already summarized
>> well in commit message and further elaborated in the patch itself as part of
>> header file comments [1] so I personally don't think any update is required
>> w.r.t this.
> 
> I still don't think rounddown vs. round_down naming is a good example to
> model anything after.
> 
> I have yet to hear a single compelling argument in favor of having a
> single underscore in the middle of a name having implications about the
> inputs of a macro/function.
> 
> The macros being added here are at 2 or 3 in Rusty's scale [1]. We could
> aim for 6 (The name tells you how to use it), but also do opportunistic
> 8 (The compiler will warn if you get it wrong) for compile-time
> constants.
> 

The macros use existing round_up/round_down underneath, so need to check if
they have compile-time checks but either-way this should not block the current
series as the series does not aim to enhance the existing round_up/round_down
macros.

> As-is, these, and round_up/round_down, are just setting a trap for an
> unsuspecting kernel developer to fall into.
> 

I understand what you are saying but I believe this was already discussed in
original patch series [1] where you had raised the same issue and it was even
discussed if we want to go with a new naming convention (like
round_closest_up_pow_2) [2] or not and the final consensus reached on naming
was to align with the existing round*() macros [3]. Moreover, I didn't hear
back any further arguments on this in further 8 revisions carrying this patch,
so thought this was aligned per wider consensus.

Anyways, to re-iterate the discussion do we want to change to below naming scheme?

round_closest_up_pow_2
round_closest_down_pow_2
roundclosest

or any other suggestions for naming ?

If there is a wider consensus on the suggested name (and ok to deviate from
existing naming convention), then we can go ahead with that.

[1]: https://lore.kernel.org/all/ZkIG0-01pz632l4R@smile.fi.intel.com/
[2]: https://lore.kernel.org/all/5ebcf480-81c6-4c2d-96e8-727d44f21ca9@ti.com/
[3]:
https://lore.kernel.org/all/ZkIG0-01pz632l4R@smile.fi.intel.com/#:~:text=series%20is%20that%3A%0A%2D-,align,-naming%20(with%20the

Regards
Devarsh
> 
> BR,
> Jani.
> 
> 
> [1] https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html
> 
> 
>>
>> [1]: https://lore.kernel.org/all/20240826150822.4057164-2-devarsht@ti.com
>>
>> Regards
>> Devarsh
> 

Re: [PATCH v2 1/6] math.h: Add macros for rounding to the closest value

[Jani Nikula]

On Tue, 27 Aug 2024, Devarsh Thakkar <devarsht@ti.com> wrote:
> Hi Nikula,
>
> Thanks for the review.
>
> On 27/08/24 18:10, Jani Nikula wrote:
>> On Tue, 27 Aug 2024, Devarsh Thakkar <devarsht@ti.com> wrote:
>
> [..]
>
>>> The equivalency between roundclosest w.r.t round_closest is same as
>>> equivalency between existing macros rounddown w.r.t round_down. Functionally
>>> both are same but the former is recommended to be used only for the scenario
>>> where multiple is not power of 2 and latter is faster but is strictly for the
>>> scenario where multiple is power of 2. I think the same is already summarized
>>> well in commit message and further elaborated in the patch itself as part of
>>> header file comments [1] so I personally don't think any update is required
>>> w.r.t this.
>> 
>> I still don't think rounddown vs. round_down naming is a good example to
>> model anything after.
>> 
>> I have yet to hear a single compelling argument in favor of having a
>> single underscore in the middle of a name having implications about the
>> inputs of a macro/function.
>> 
>> The macros being added here are at 2 or 3 in Rusty's scale [1]. We could
>> aim for 6 (The name tells you how to use it), but also do opportunistic
>> 8 (The compiler will warn if you get it wrong) for compile-time
>> constants.
>> 
>
> The macros use existing round_up/round_down underneath, so need to check if
> they have compile-time checks but either-way this should not block the current
> series as the series does not aim to enhance the existing round_up/round_down
> macros.
>
>> As-is, these, and round_up/round_down, are just setting a trap for an
>> unsuspecting kernel developer to fall into.
>> 
>
> I understand what you are saying but I believe this was already discussed in
> original patch series [1] where you had raised the same issue and it was even
> discussed if we want to go with a new naming convention (like
> round_closest_up_pow_2) [2] or not and the final consensus reached on naming
> was to align with the existing round*() macros [3]. Moreover, I didn't hear
> back any further arguments on this in further 8 revisions carrying this patch,
> so thought this was aligned per wider consensus.
>
> Anyways, to re-iterate the discussion do we want to change to below naming scheme?
>
> round_closest_up_pow_2
> round_closest_down_pow_2
> roundclosest
>
> or any other suggestions for naming ?
>
> If there is a wider consensus on the suggested name (and ok to deviate from
> existing naming convention), then we can go ahead with that.

The stupid thing here is, I still don't remember which one is the
generic thing, rounddown() or round_down(). I have to look it up every
single time to be sure. I refuse to believe I'd be the only one.

It's okay to accidentally use the generic version, no harm done. It's
definitely not okay to accidentally use the special pow-2 version, so it
should have a special name. I think _pow2() or _pow_2() is a fine
suffix.

Another stupid thing is, I'd name the generic thing round_down(). But
whoopsie, that's not the generic thing.

This naming puts an unnecessary extra burden on people. It's a stupid
"convention" to follow. It's a mistake. Not repeating a mistake trumps
following the pattern.

There, I've said it. Up to the people who ack and commit to decide.


BR,
Jani.


>
> [1]: https://lore.kernel.org/all/ZkIG0-01pz632l4R@smile.fi.intel.com/
> [2]: https://lore.kernel.org/all/5ebcf480-81c6-4c2d-96e8-727d44f21ca9@ti.com/
> [3]:
> https://lore.kernel.org/all/ZkIG0-01pz632l4R@smile.fi.intel.com/#:~:text=series%20is%20that%3A%0A%2D-,align,-naming%20(with%20the
>
> Regards
> Devarsh
>> 
>> BR,
>> Jani.
>> 
>> 
>> [1] https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html
>> 
>> 
>>>
>>> [1]: https://lore.kernel.org/all/20240826150822.4057164-2-devarsht@ti.com
>>>
>>> Regards
>>> Devarsh
>> 

-- 
Jani Nikula, Intel

Re: [PATCH v2 1/6] math.h: Add macros for rounding to the closest value

[Jiri Slaby]

On 29. 08. 24, 11:19, Jani Nikula wrote:
> The stupid thing here is, I still don't remember which one is the
> generic thing, rounddown() or round_down(). I have to look it up every
> single time to be sure. I refuse to believe I'd be the only one.
> 
> It's okay to accidentally use the generic version, no harm done. It's
> definitely not okay to accidentally use the special pow-2 version, so it
> should have a special name. I think _pow2() or _pow_2() is a fine
> suffix.

Concur.

-- 
js
suse labs

Re: [PATCH v2 1/6] math.h: Add macros for rounding to the closest value

[Dave Martin]

On Mon, Aug 26, 2024 at 08:38:17PM +0530, Devarsh Thakkar wrote:
> Add below rounding related macros:
> 
> round_closest_up(x, y) : Rounds x to the closest multiple of y where y is a
> power of 2, with a preference to round up in case two nearest values are
> possible.
> 
> round_closest_down(x, y) : Rounds x to the closest multiple of y where y is
> a power of 2, with a preference to round down in case two nearest values
> are possible.
> 
> roundclosest(x, y) : Rounds x to the closest multiple of y, this macro
> should generally be used only when y is not multiple of 2 as otherwise
> round_closest* macros should be used which are much faster.
> 
> Examples:
>  * round_closest_up(17, 4) = 16
>  * round_closest_up(15, 4) = 16
>  * round_closest_up(14, 4) = 16
>  * round_closest_down(17, 4) = 16
>  * round_closest_down(15, 4) = 16
>  * round_closest_down(14, 4) = 12
>  * roundclosest(21, 5) = 20
>  * roundclosest(19, 5) = 20
>  * roundclosest(17, 5) = 15
> 
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> NOTE: This patch is inspired from the Mentor Graphics IPU driver [1]
> which uses similar macro locally and which is updated in further patch
> in the series to use this generic macro instead along with other drivers
> having similar requirements.
> 
> Link: https://elixir.bootlin.com/linux/v6.8.9/source/drivers/gpu/ipu-v3/ipu-image-convert.c#L480 [1]
> 
> Past discussions and alignment on this:
> https://lore.kernel.org/all/7e3ad816-6a2a-4e02-9b41-03a8562812ad@ti.com/#r
> https://lore.kernel.org/all/ZkISG6p1tn9Do-xY@smile.fi.intel.com/#r
> https://lore.kernel.org/all/ZlTt-YWzyRyhmT9n@smile.fi.intel.com/
> https://lore.kernel.org/all/ZmHDWeuezCEgj20m@smile.fi.intel.com/
> https://lore.kernel.org/all/ZloMFfGKLry6EWNL@smile.fi.intel.com/
> 
> Changelog:
> V2:
> - Fix grammar in macro description
> - Update roundclosest macro to use roundup to avoid overflow scenario
> ---
>  include/linux/math.h | 63 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/include/linux/math.h b/include/linux/math.h
> index f5f18dc3616b..b59a02a007d7 100644
> --- a/include/linux/math.h
> +++ b/include/linux/math.h
> @@ -34,6 +34,52 @@
>   */
>  #define round_down(x, y) ((x) & ~__round_mask(x, y))
>  
> +/**
> + * round_closest_up - round closest to be multiple of the specified value
> + *                    (which is power of 2) with preference to rounding up
> + * @x: the value to round
> + * @y: multiple to round closest to (must be a power of 2)
> + *
> + * Rounds @x to the closest multiple of @y (which must be a power of 2).
> + * The value can be rounded up or rounded down depending on the rounded
> + * value's closeness to the specified value. Also, there can be two closest
> + * values, i.e. the difference between the specified value and its rounded-up
> + * and rounded-down values could be the same. In that case, the rounded-up
> + * value is preferred.
> + *
> + * To perform arbitrary rounding to the closest value (not multiple of 2), use
> + * roundclosest().
> + *
> + * Examples:
> + * * round_closest_up(17, 4) = 16
> + * * round_closest_up(15, 4) = 16
> + * * round_closest_up(14, 4) = 16
> + */
> +#define round_closest_up(x, y) round_down((x) + (y) / 2, (y))
> +
> +/**
> + * round_closest_down - round closest to be multiple of the specified value
> + *                      (which is power of 2) with preference to rounding down
> + * @x: the value to round
> + * @y: multiple to round closest to (must be a power of 2)
> + *
> + * Rounds @x to the closest multiple of @y (which must be a power of 2).
> + * The value can be rounded up or rounded down depending on the rounded
> + * value's closeness to the specified value. Also, there can be two closest
> + * values, i.e. the difference between the specified value and its rounded-up
> + * and rounded-down values could be the same. In that case, the rounded-down
> + * value is preferred.
> + *
> + * To perform arbitrary rounding to the closest value (not multiple of 2), use
> + * roundclosest().
> + *
> + * Examples:
> + * * round_closest_down(17, 4) = 16
> + * * round_closest_down(15, 4) = 16
> + * * round_closest_down(14, 4) = 12
> + */
> +#define round_closest_down(x, y) round_up((x) - (y) / 2, (y))
> +
>  #define DIV_ROUND_UP __KERNEL_DIV_ROUND_UP
>  
>  #define DIV_ROUND_DOWN_ULL(ll, d) \
> @@ -77,6 +123,23 @@
>  }							\
>  )
>  
> +/**
> + * roundclosest - round to the nearest multiple
> + * @x: the value to round
> + * @y: multiple to round nearest to
> + *
> + * Rounds @x to the nearest multiple of @y.
> + * The rounded value can be greater or less than @x depending
> + * upon its nearness to @x. If @y is always a power of 2, consider
> + * using the faster round_closest_up() or round_closest_down().
> + *
> + * Examples:
> + * * roundclosest(21, 5) = 20
> + * * roundclosest(19, 5) = 20
> + * * roundclosest(17, 5) = 15
> + */
> +#define roundclosest(x, y) roundup((x) - (y) / 2, (y))

roundup() looks like it already does the wrong thing for negative
numbers:

	roundup(-10, 5)
	= (-10 + 4) / 5 * 5
	= -6 / 5 * 5
	= -1 * 5
	= -5

(DIV_ROUND_UP looks less broken in this regard, though it's complicated
and I haven't tried to understand it fully.)

Disregarding the issue of negative inputs, the proposed definition of
roundclosest() looks like it still doesn't always do the right thing
close to the upper limits of types, even when the expected result is
representable in the type.

For example, if I've understood this correctly, we can get:

	roundclosest(0xFFFFFFFFU, 0xFFFFU)
	= roundup(0xFFFFFFFF - 0x7FFFU, 0xFFFFU)
	= roundup(0xFFFF8000, 0xFFFFU)
	= ((0xFFFF8000 + (0xFFFFU - 1)) / 0xFFFFU) * 0xFFFFU
	= ((0xFFFF8000 + 0xFFFEU) / 0xFFFFU) * 0xFFFFU
	= (0x00007FFE / 0xFFFFU) * 0xFFFFU
	= 0

(Expected result: 0x00010001U * 0xFFFFU, = 0xFFFFFFFFU.)

I suppose this could be documented around, but it seems like a
potential trap and not something the caller would expect.

Cheers
---Dave

Re: [PATCH v2 5/6] lib: math_kunit: Add tests for new macros related to rounding to nearest value

[Dave Martin]

On Mon, Aug 26, 2024 at 08:38:21PM +0530, Devarsh Thakkar wrote:
> Add tests for round_closest_up/down and roundclosest macros which round
> to nearest multiple of specified argument. These are tested with kunit
> tool as shared here [1] :

The tests here don't seem to be well targeted at the actual edge cases
where incrementing or decremting the dividend by 1 is expected to
change the result, or when round_closest_up() and round_closest_down()
are expected to differ.

There's also no coverage of:

	* negative inputs

	* types other than int

	* inputs close to type limits

(I've highlighted a few specific issues below, though there seems to be
a more general lack of coverage here.)

> 
> Link: https://gist.github.com/devarsht/3f9042825be3da4e133b8f4eda067876 [1]
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> V2: No change
> ---
>  lib/math/math_kunit.c | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/lib/math/math_kunit.c b/lib/math/math_kunit.c
> index be27f2afb8e4..05022f010be6 100644
> --- a/lib/math/math_kunit.c
> +++ b/lib/math/math_kunit.c
> @@ -70,6 +70,26 @@ static void round_down_test(struct kunit *test)
>  	KUNIT_EXPECT_EQ(test, round_down((1 << 30) - 1, 1 << 29), 1 << 29);
>  }
>  
> +static void round_closest_up_test(struct kunit *test)
> +{
> +	KUNIT_EXPECT_EQ(test, round_closest_up(17, 4), 16);
> +	KUNIT_EXPECT_EQ(test, round_closest_up(15, 4), 16);
> +	KUNIT_EXPECT_EQ(test, round_closest_up(14, 4), 16);
> +	KUNIT_EXPECT_EQ(test, round_closest_up((1 << 30) - 1, 1 << 30), 1 << 30);
> +	KUNIT_EXPECT_EQ(test, round_closest_up((1 << 30) + 1, 1 << 30), 1 << 30);

These miss the edge case where the result is expected to change; could
we also check:

	round_closest_up(1 << 29, 1 << 30)


> +	KUNIT_EXPECT_EQ(test, round_closest_up((1 << 30) - 1, 2), 1 << 30);
> +}
> +
> +static void round_closest_down_test(struct kunit *test)
> +{
> +	KUNIT_EXPECT_EQ(test, round_closest_down(17, 4), 16);
> +	KUNIT_EXPECT_EQ(test, round_closest_down(15, 4), 16);
> +	KUNIT_EXPECT_EQ(test, round_closest_down(14, 4), 12);
> +	KUNIT_EXPECT_EQ(test, round_closest_down((1 << 30) - 1, 1 << 30), 1 << 30);
> +	KUNIT_EXPECT_EQ(test, round_closest_down((1 << 30) + 1, 1 << 30), 1 << 30);
> +	KUNIT_EXPECT_EQ(test, round_closest_down((1 << 30) - 1, 2), (1 << 30) - 2);
> +}
> +
>  /* These versions can round to numbers that aren't a power of two */
>  static void roundup_test(struct kunit *test)
>  {
> @@ -95,6 +115,18 @@ static void rounddown_test(struct kunit *test)
>  	KUNIT_EXPECT_EQ(test, rounddown(4, 3), 3);
>  }
>  
> +static void roundclosest_test(struct kunit *test)
> +{
> +	KUNIT_EXPECT_EQ(test, roundclosest(21, 5), 20);
> +	KUNIT_EXPECT_EQ(test, roundclosest(19, 5), 20);

This seems to miss the edge cases (e.g., roundclosest(20 * N + 10, 20),
roundclosest(20 * N +/- 9, 20) for some N.

> +	KUNIT_EXPECT_EQ(test, roundclosest(17, 5), 15);
> +	KUNIT_EXPECT_EQ(test, roundclosest((1 << 30), 3), (1 << 30) - 1);
> +	KUNIT_EXPECT_EQ(test, roundclosest((1 << 30) - 1, 1 << 29), 1 << 30);
> +
> +	KUNIT_EXPECT_EQ(test, roundclosest(4, 3), 3);
> +	KUNIT_EXPECT_EQ(test, roundclosest(5, 3), 6);
> +}
> +

[...]

Cheers
---Dave

Re: [PATCH v2 1/6] math.h: Add macros for rounding to the closest value

[Devarsh Thakkar]

Hi Andy, Randy, Sebastian,

On 29/08/24 15:24, Jiri Slaby wrote:
> On 29. 08. 24, 11:19, Jani Nikula wrote:
>> The stupid thing here is, I still don't remember which one is the
>> generic thing, rounddown() or round_down(). I have to look it up every
>> single time to be sure. I refuse to believe I'd be the only one.
>>
>> It's okay to accidentally use the generic version, no harm done. It's
>> definitely not okay to accidentally use the special pow-2 version, so it
>> should have a special name. I think _pow2() or _pow_2() is a fine
>> suffix.
> 
> Concur.
> 

We have got 2 votes to change round_closest_up to round_closest_up_pow_2 and
likewise for round_closest_down to round_closest_up_pow_2.

Kindly let us know if you have any concerns w.r.t above name change. Else, I
was thinking to proceed with the suggestion.

Regards
Devarsh

Re: [PATCH v2 1/6] math.h: Add macros for rounding to the closest value

[Randy Dunlap]

On 8/29/24 2:54 AM, Jiri Slaby wrote:
> On 29. 08. 24, 11:19, Jani Nikula wrote:
>> The stupid thing here is, I still don't remember which one is the
>> generic thing, rounddown() or round_down(). I have to look it up every
>> single time to be sure. I refuse to believe I'd be the only one.
>>
>> It's okay to accidentally use the generic version, no harm done. It's
>> definitely not okay to accidentally use the special pow-2 version, so it
>> should have a special name. I think _pow2() or _pow_2() is a fine
>> suffix.
> 
> Concur.
> 

Ack here also. I prefer _pow2().