[PATCH next 0/7] minmax.h: Cleanups and minor optimisations

[PATCH next 0/7] minmax.h: Cleanups and minor optimisations

[David Laight]

Some tidyups and minor changes to minmax.h.

Patches 1, 2 and 5 have no functional changes.
Patch 3 changes the statically_true() tests in __types_ok() to use the
  local 'unique' variables rather than the caller supplied expressions.
  This will reduce the line length of the preprocessor output.
Patch 4 uses statically_true() for clamp's (lo) < (hi) check.
Patch 6 passes '__auto_type' as the 'fixed type' to reduce the number
  of variants.
Patch 7 removes a level of indirection from __sign_use() and 
  __is_nonneg().

David Laight (7):
  1) Add whitespace around operators and after commas
  2) Update some comments
  3) Reduce the #define expansion of min(), max() and clamp()
  4) Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()
  5) Move all the clamp() definitions after the min/max() ones.
  6) Simplify the variants of clamp()
  7) Remove some #defines that are only expanded once

 include/linux/minmax.h | 205 +++++++++++++++++++----------------------
 1 file changed, 95 insertions(+), 110 deletions(-)

-- 
2.17.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH next 1/7] minmax.h: Add whitespace around operators and after commas

[David Laight]

Signed-off-by: David Laight <david.laight@aculab.com>
---
 include/linux/minmax.h | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 98008dd92153..51b0d988e322 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -51,10 +51,10 @@
  * only need to be careful to not cause warnings for
  * pointer use.
  */
-#define __signed_type_use(x,ux) (2+__is_nonneg(x,ux))
-#define __unsigned_type_use(x,ux) (1+2*(sizeof(ux)<4))
-#define __sign_use(x,ux) (is_signed_type(typeof(ux))? \
-	__signed_type_use(x,ux):__unsigned_type_use(x,ux))
+#define __signed_type_use(x, ux) (2 + __is_nonneg(x, ux))
+#define __unsigned_type_use(x, ux) (1 + 2 * (sizeof(ux) < 4))
+#define __sign_use(x, ux) (is_signed_type(typeof(ux)) ? \
+	__signed_type_use(x, ux) : __unsigned_type_use(x, ux))
 
 /*
  * To avoid warnings about casting pointers to integers
@@ -74,15 +74,15 @@
 #ifdef CONFIG_64BIT
   #define __signed_type(ux) long
 #else
-  #define __signed_type(ux) typeof(__builtin_choose_expr(sizeof(ux)>4,1LL,1L))
+  #define __signed_type(ux) typeof(__builtin_choose_expr(sizeof(ux) > 4, 1LL, 1L))
 #endif
-#define __is_nonneg(x,ux) statically_true((__signed_type(ux))(x)>=0)
+#define __is_nonneg(x, ux) statically_true((__signed_type(ux))(x) >= 0)
 
-#define __types_ok(x,y,ux,uy) \
-	(__sign_use(x,ux) & __sign_use(y,uy))
+#define __types_ok(x, y, ux, uy) \
+	(__sign_use(x, ux) & __sign_use(y, uy))
 
-#define __types_ok3(x,y,z,ux,uy,uz) \
-	(__sign_use(x,ux) & __sign_use(y,uy) & __sign_use(z,uz))
+#define __types_ok3(x, y, z, ux, uy, uz) \
+	(__sign_use(x, ux) & __sign_use(y, uy) & __sign_use(z, uz))
 
 #define __cmp_op_min <
 #define __cmp_op_max >
@@ -97,7 +97,7 @@
 
 #define __careful_cmp_once(op, x, y, ux, uy) ({		\
 	__auto_type ux = (x); __auto_type uy = (y);	\
-	BUILD_BUG_ON_MSG(!__types_ok(x,y,ux,uy),	\
+	BUILD_BUG_ON_MSG(!__types_ok(x, y, ux, uy),	\
 		#op"("#x", "#y") signedness error");	\
 	__cmp(op, ux, uy); })
 
@@ -114,7 +114,7 @@
 	static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)), 	\
 			(lo) <= (hi), true),					\
 		"clamp() low limit " #lo " greater than high limit " #hi);	\
-	BUILD_BUG_ON_MSG(!__types_ok3(val,lo,hi,uval,ulo,uhi),			\
+	BUILD_BUG_ON_MSG(!__types_ok3(val, lo, hi, uval, ulo, uhi),		\
 		"clamp("#val", "#lo", "#hi") signedness error");		\
 	__clamp(uval, ulo, uhi); })
 
@@ -154,7 +154,7 @@
 
 #define __careful_op3(op, x, y, z, ux, uy, uz) ({			\
 	__auto_type ux = (x); __auto_type uy = (y);__auto_type uz = (z);\
-	BUILD_BUG_ON_MSG(!__types_ok3(x,y,z,ux,uy,uz),			\
+	BUILD_BUG_ON_MSG(!__types_ok3(x, y, z, ux, uy, uz),		\
 		#op"3("#x", "#y", "#z") signedness error");		\
 	__cmp(op, ux, __cmp(op, uy, uz)); })
 
@@ -326,9 +326,9 @@ static inline bool in_range32(u32 val, u32 start, u32 len)
  * Use these carefully: no type checking, and uses the arguments
  * multiple times. Use for obvious constants only.
  */
-#define MIN(a,b) __cmp(min,a,b)
-#define MAX(a,b) __cmp(max,a,b)
-#define MIN_T(type,a,b) __cmp(min,(type)(a),(type)(b))
-#define MAX_T(type,a,b) __cmp(max,(type)(a),(type)(b))
+#define MIN(a, b) __cmp(min, a, b)
+#define MAX(a, b) __cmp(max, a, b)
+#define MIN_T(type, a, b) __cmp(min, (type)(a), (type)(b))
+#define MAX_T(type, a, b) __cmp(max, (type)(a), (type)(b))
 
 #endif	/* _LINUX_MINMAX_H */
-- 
2.17.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH next 2/7] minmax.h: Update some comments

[David Laight]

- Change three to several.
- Remove the comment about retaining constant expressions, no longer true.
- Realign to nearer 80 columns and break on major punctiation.
- Add a leading comment to the block before __signed_type() and __is_nonneg()
  Otherwise the block explaining the cast is a bit 'floating'.
  Reword the rest of that comment to improve readability.

Signed-off-by: David Laight <david.laight@aculab.com>
---
 include/linux/minmax.h | 53 +++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 51b0d988e322..24e4b372649a 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -8,13 +8,10 @@
 #include <linux/types.h>
 
 /*
- * min()/max()/clamp() macros must accomplish three things:
+ * min()/max()/clamp() macros must accomplish several things:
  *
  * - Avoid multiple evaluations of the arguments (so side-effects like
  *   "x++" happen only once) when non-constant.
- * - Retain result as a constant expressions when called with only
- *   constant expressions (to avoid tripping VLA warnings in stack
- *   allocation usage).
  * - Perform signed v unsigned type-checking (to generate compile
  *   errors instead of nasty runtime surprises).
  * - Unsigned char/short are always promoted to signed int and can be
@@ -31,25 +28,23 @@
  *   bit #0 set if ok for unsigned comparisons
  *   bit #1 set if ok for signed comparisons
  *
- * In particular, statically non-negative signed integer
- * expressions are ok for both.
+ * In particular, statically non-negative signed integer expressions
+ * are ok for both.
  *
- * NOTE! Unsigned types smaller than 'int' are implicitly
- * converted to 'int' in expressions, and are accepted for
- * signed conversions for now. This is debatable.
+ * NOTE! Unsigned types smaller than 'int' are implicitly converted to 'int'
+ * in expressions, and are accepted for signed conversions for now.
+ * This is debatable.
  *
- * Note that 'x' is the original expression, and 'ux' is
- * the unique variable that contains the value.
+ * Note that 'x' is the original expression, and 'ux' is the unique variable
+ * that contains the value.
  *
- * We use 'ux' for pure type checking, and 'x' for when
- * we need to look at the value (but without evaluating
- * it for side effects! Careful to only ever evaluate it
- * with sizeof() or __builtin_constant_p() etc).
+ * We use 'ux' for pure type checking, and 'x' for when we need to look at the
+ * value (but without evaluating it for side effects!
+ * Careful to only ever evaluate it with sizeof() or __builtin_constant_p() etc).
  *
- * Pointers end up being checked by the normal C type
- * rules at the actual comparison, and these expressions
- * only need to be careful to not cause warnings for
- * pointer use.
+ * Pointers end up being checked by the normal C type rules at the actual
+ * comparison, and these expressions only need to be careful to not cause
+ * warnings for pointer use.
  */
 #define __signed_type_use(x, ux) (2 + __is_nonneg(x, ux))
 #define __unsigned_type_use(x, ux) (1 + 2 * (sizeof(ux) < 4))
@@ -57,19 +52,19 @@
 	__signed_type_use(x, ux) : __unsigned_type_use(x, ux))
 
 /*
- * To avoid warnings about casting pointers to integers
- * of different sizes, we need that special sign type.
+ * Check whether a signed value is always non-negative.
  *
- * On 64-bit we can just always use 'long', since any
- * integer or pointer type can just be cast to that.
+ * A cast is needed to avoid any warnings from values that aren't signed
+ * integer types (in which case the result doesn't matter).
  *
- * This does not work for 128-bit signed integers since
- * the cast would truncate them, but we do not use s128
- * types in the kernel (we do use 'u128', but they will
- * be handled by the !is_signed_type() case).
+ * On 64-bit any integer or pointer type can safely be cast to 'long'.
+ * But on 32-bit we need to avoid warnings about casting pointers to integers
+ * of different sizes without truncating 64-bit values so 'long' or 'long long'
+ * must be used depending on the size of the value.
  *
- * NOTE! The cast is there only to avoid any warnings
- * from when values that aren't signed integer types.
+ * This does not work for 128-bit signed integers since the cast would truncate
+ * them, but we do not use s128 types in the kernel (we do use 'u128',
+ * but they are handled by the !is_signed_type() case).
  */
 #ifdef CONFIG_64BIT
   #define __signed_type(ux) long
-- 
2.17.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH next 3/7] minmax.h: Reduce the #define expansion of min(), max() and clamp()

[David Laight]

Since the test for signed values being non-negative only relies on
__builtion_constant_p() (not is_constexpr()) it can use the 'ux'
variable instead of the caller supplied expression.
This means that the #define parameters are only expanded twice.
Once in the code and once quoted in the error message.

Signed-off-by: David Laight <david.laight@aculab.com>
---
 include/linux/minmax.h | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 24e4b372649a..6f7ea669d305 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -46,10 +46,10 @@
  * comparison, and these expressions only need to be careful to not cause
  * warnings for pointer use.
  */
-#define __signed_type_use(x, ux) (2 + __is_nonneg(x, ux))
-#define __unsigned_type_use(x, ux) (1 + 2 * (sizeof(ux) < 4))
-#define __sign_use(x, ux) (is_signed_type(typeof(ux)) ? \
-	__signed_type_use(x, ux) : __unsigned_type_use(x, ux))
+#define __signed_type_use(ux) (2 + __is_nonneg(ux))
+#define __unsigned_type_use(ux) (1 + 2 * (sizeof(ux) < 4))
+#define __sign_use(ux) (is_signed_type(typeof(ux)) ? \
+	__signed_type_use(ux) : __unsigned_type_use(ux))
 
 /*
  * Check whether a signed value is always non-negative.
@@ -71,13 +71,13 @@
 #else
   #define __signed_type(ux) typeof(__builtin_choose_expr(sizeof(ux) > 4, 1LL, 1L))
 #endif
-#define __is_nonneg(x, ux) statically_true((__signed_type(ux))(x) >= 0)
+#define __is_nonneg(ux) statically_true((__signed_type(ux))(ux) >= 0)
 
-#define __types_ok(x, y, ux, uy) \
-	(__sign_use(x, ux) & __sign_use(y, uy))
+#define __types_ok(ux, uy) \
+	(__sign_use(ux) & __sign_use(uy))
 
-#define __types_ok3(x, y, z, ux, uy, uz) \
-	(__sign_use(x, ux) & __sign_use(y, uy) & __sign_use(z, uz))
+#define __types_ok3(ux, uy, uz) \
+	(__sign_use(ux) & __sign_use(uy) & __sign_use(uz))
 
 #define __cmp_op_min <
 #define __cmp_op_max >
@@ -92,7 +92,7 @@
 
 #define __careful_cmp_once(op, x, y, ux, uy) ({		\
 	__auto_type ux = (x); __auto_type uy = (y);	\
-	BUILD_BUG_ON_MSG(!__types_ok(x, y, ux, uy),	\
+	BUILD_BUG_ON_MSG(!__types_ok(ux, uy),		\
 		#op"("#x", "#y") signedness error");	\
 	__cmp(op, ux, uy); })
 
@@ -109,7 +109,7 @@
 	static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)), 	\
 			(lo) <= (hi), true),					\
 		"clamp() low limit " #lo " greater than high limit " #hi);	\
-	BUILD_BUG_ON_MSG(!__types_ok3(val, lo, hi, uval, ulo, uhi),		\
+	BUILD_BUG_ON_MSG(!__types_ok3(uval, ulo, uhi),				\
 		"clamp("#val", "#lo", "#hi") signedness error");		\
 	__clamp(uval, ulo, uhi); })
 
@@ -149,7 +149,7 @@
 
 #define __careful_op3(op, x, y, z, ux, uy, uz) ({			\
 	__auto_type ux = (x); __auto_type uy = (y);__auto_type uz = (z);\
-	BUILD_BUG_ON_MSG(!__types_ok3(x, y, z, ux, uy, uz),		\
+	BUILD_BUG_ON_MSG(!__types_ok3(ux, uy, uz),			\
 		#op"3("#x", "#y", "#z") signedness error");		\
 	__cmp(op, ux, __cmp(op, uy, uz)); })
 
-- 
2.17.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()

[David Laight]

Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity
check of the bounds in clamp().
Gives better error coverage and one less expansion of the arguments.

Signed-off-by: David Laight <david.laight@aculab.com>
---
 include/linux/minmax.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 6f7ea669d305..91aa1b90c1bb 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -106,8 +106,7 @@
 	__auto_type uval = (val);						\
 	__auto_type ulo = (lo);							\
 	__auto_type uhi = (hi);							\
-	static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)), 	\
-			(lo) <= (hi), true),					\
+	BUILD_BUG_ON_MSG(statically_true(ulo > uhi),				\
 		"clamp() low limit " #lo " greater than high limit " #hi);	\
 	BUILD_BUG_ON_MSG(!__types_ok3(uval, ulo, uhi),				\
 		"clamp("#val", "#lo", "#hi") signedness error");		\
-- 
2.17.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH next 5/7] minmax.h: Move all the clamp() definitions after the min/max() ones

[David Laight]

At some point the definitions for clamp() got added in the middle
of the ones for min() and max().
Re-order the definitions so they are more sensibly grouped.

Signed-off-by: David Laight <david.laight@aculab.com>
---
 include/linux/minmax.h | 109 +++++++++++++++++++----------------------
 1 file changed, 51 insertions(+), 58 deletions(-)

diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 91aa1b90c1bb..75fb7a6ad4c6 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -99,22 +99,6 @@
 #define __careful_cmp(op, x, y) \
 	__careful_cmp_once(op, x, y, __UNIQUE_ID(x_), __UNIQUE_ID(y_))
 
-#define __clamp(val, lo, hi)	\
-	((val) >= (hi) ? (hi) : ((val) <= (lo) ? (lo) : (val)))
-
-#define __clamp_once(val, lo, hi, uval, ulo, uhi) ({				\
-	__auto_type uval = (val);						\
-	__auto_type ulo = (lo);							\
-	__auto_type uhi = (hi);							\
-	BUILD_BUG_ON_MSG(statically_true(ulo > uhi),				\
-		"clamp() low limit " #lo " greater than high limit " #hi);	\
-	BUILD_BUG_ON_MSG(!__types_ok3(uval, ulo, uhi),				\
-		"clamp("#val", "#lo", "#hi") signedness error");		\
-	__clamp(uval, ulo, uhi); })
-
-#define __careful_clamp(val, lo, hi) \
-	__clamp_once(val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_))
-
 /**
  * min - return minimum of two values of the same or compatible types
  * @x: first value
@@ -170,6 +154,22 @@
 #define max3(x, y, z) \
 	__careful_op3(max, x, y, z, __UNIQUE_ID(x_), __UNIQUE_ID(y_), __UNIQUE_ID(z_))
 
+/**
+ * min_t - return minimum of two values, using the specified type
+ * @type: data type to use
+ * @x: first value
+ * @y: second value
+ */
+#define min_t(type, x, y) __cmp_once(min, type, x, y)
+
+/**
+ * max_t - return maximum of two values, using the specified type
+ * @type: data type to use
+ * @x: first value
+ * @y: second value
+ */
+#define max_t(type, x, y) __cmp_once(max, type, x, y)
+
 /**
  * min_not_zero - return the minimum that is _not_ zero, unless both are zero
  * @x: value1
@@ -180,6 +180,22 @@
 	typeof(y) __y = (y);			\
 	__x == 0 ? __y : ((__y == 0) ? __x : min(__x, __y)); })
 
+#define __clamp(val, lo, hi)	\
+	((val) >= (hi) ? (hi) : ((val) <= (lo) ? (lo) : (val)))
+
+#define __clamp_once(val, lo, hi, uval, ulo, uhi) ({				\
+	__auto_type uval = (val);						\
+	__auto_type ulo = (lo);							\
+	__auto_type uhi = (hi);							\
+	BUILD_BUG_ON_MSG(statically_true(ulo > uhi),				\
+		"clamp() low limit " #lo " greater than high limit " #hi);	\
+	BUILD_BUG_ON_MSG(!__types_ok3(uval, ulo, uhi),				\
+		"clamp("#val", "#lo", "#hi") signedness error");		\
+	__clamp(uval, ulo, uhi); })
+
+#define __careful_clamp(val, lo, hi) \
+	__clamp_once(val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_))
+
 /**
  * clamp - return a value clamped to a given range with strict typechecking
  * @val: current value
@@ -191,28 +207,30 @@
  */
 #define clamp(val, lo, hi) __careful_clamp(val, lo, hi)
 
-/*
- * ..and if you can't take the strict
- * types, you can specify one yourself.
- *
- * Or not use min/max/clamp at all, of course.
- */
-
 /**
- * min_t - return minimum of two values, using the specified type
- * @type: data type to use
- * @x: first value
- * @y: second value
+ * clamp_t - return a value clamped to a given range using a given type
+ * @type: the type of variable to use
+ * @val: current value
+ * @lo: minimum allowable value
+ * @hi: maximum allowable value
+ *
+ * This macro does no typechecking and uses temporary variables of type
+ * @type to make all the comparisons.
  */
-#define min_t(type, x, y) __cmp_once(min, type, x, y)
+#define clamp_t(type, val, lo, hi) __careful_clamp((type)(val), (type)(lo), (type)(hi))
 
 /**
- * max_t - return maximum of two values, using the specified type
- * @type: data type to use
- * @x: first value
- * @y: second value
+ * clamp_val - return a value clamped to a given range using val's type
+ * @val: current value
+ * @lo: minimum allowable value
+ * @hi: maximum allowable value
+ *
+ * This macro does no typechecking and uses temporary variables of whatever
+ * type the input argument @val is.  This is useful when @val is an unsigned
+ * type and @lo and @hi are literals that will otherwise be assigned a signed
+ * integer type.
  */
-#define max_t(type, x, y) __cmp_once(max, type, x, y)
+#define clamp_val(val, lo, hi) clamp_t(typeof(val), val, lo, hi)
 
 /*
  * Do not check the array parameter using __must_be_array().
@@ -257,31 +275,6 @@
  */
 #define max_array(array, len) __minmax_array(max, array, len)
 
-/**
- * clamp_t - return a value clamped to a given range using a given type
- * @type: the type of variable to use
- * @val: current value
- * @lo: minimum allowable value
- * @hi: maximum allowable value
- *
- * This macro does no typechecking and uses temporary variables of type
- * @type to make all the comparisons.
- */
-#define clamp_t(type, val, lo, hi) __careful_clamp((type)(val), (type)(lo), (type)(hi))
-
-/**
- * clamp_val - return a value clamped to a given range using val's type
- * @val: current value
- * @lo: minimum allowable value
- * @hi: maximum allowable value
- *
- * This macro does no typechecking and uses temporary variables of whatever
- * type the input argument @val is.  This is useful when @val is an unsigned
- * type and @lo and @hi are literals that will otherwise be assigned a signed
- * integer type.
- */
-#define clamp_val(val, lo, hi) clamp_t(typeof(val), val, lo, hi)
-
 static inline bool in_range64(u64 val, u64 start, u64 len)
 {
 	return (val - start) < len;
-- 
2.17.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH next 6/7] minmax.h: Simplify the variants of clamp()

[David Laight]

Always pass a 'type' through to __clamp_once(), pass '__auto_type'
from clamp() itself.

The expansion of __types_ok3() is reasonable so it isn't worth the
added complexity of avoiding it when a fixed type is used for all
three values.

Signed-off-by: David Laight <david.laight@aculab.com>
---
 include/linux/minmax.h | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 75fb7a6ad4c6..2bbdd5b5e07e 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -183,29 +183,29 @@
 #define __clamp(val, lo, hi)	\
 	((val) >= (hi) ? (hi) : ((val) <= (lo) ? (lo) : (val)))
 
-#define __clamp_once(val, lo, hi, uval, ulo, uhi) ({				\
-	__auto_type uval = (val);						\
-	__auto_type ulo = (lo);							\
-	__auto_type uhi = (hi);							\
+#define __clamp_once(type, val, lo, hi, uval, ulo, uhi) ({			\
+	type uval = (val);							\
+	type ulo = (lo);							\
+	type uhi = (hi);							\
 	BUILD_BUG_ON_MSG(statically_true(ulo > uhi),				\
 		"clamp() low limit " #lo " greater than high limit " #hi);	\
 	BUILD_BUG_ON_MSG(!__types_ok3(uval, ulo, uhi),				\
 		"clamp("#val", "#lo", "#hi") signedness error");		\
 	__clamp(uval, ulo, uhi); })
 
-#define __careful_clamp(val, lo, hi) \
-	__clamp_once(val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_))
+#define __careful_clamp(type, val, lo, hi) \
+	__clamp_once(type, val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_))
 
 /**
- * clamp - return a value clamped to a given range with strict typechecking
+ * clamp - return a value clamped to a given range with typechecking
  * @val: current value
  * @lo: lowest allowable value
  * @hi: highest allowable value
  *
- * This macro does strict typechecking of @lo/@hi to make sure they are of the
- * same type as @val.  See the unnecessary pointer comparisons.
+ * This macro checks @val/@lo/@hi to make sure they have compatible
+ * signedness.
  */
-#define clamp(val, lo, hi) __careful_clamp(val, lo, hi)
+#define clamp(val, lo, hi) __careful_clamp(__auto_type, val, lo, hi)
 
 /**
  * clamp_t - return a value clamped to a given range using a given type
@@ -217,7 +217,7 @@
  * This macro does no typechecking and uses temporary variables of type
  * @type to make all the comparisons.
  */
-#define clamp_t(type, val, lo, hi) __careful_clamp((type)(val), (type)(lo), (type)(hi))
+#define clamp_t(type, val, lo, hi) __careful_clamp(type, val, lo, hi)
 
 /**
  * clamp_val - return a value clamped to a given range using val's type
@@ -230,7 +230,7 @@
  * type and @lo and @hi are literals that will otherwise be assigned a signed
  * integer type.
  */
-#define clamp_val(val, lo, hi) clamp_t(typeof(val), val, lo, hi)
+#define clamp_val(val, lo, hi) __careful_clamp(typeof(val), val, lo, hi)
 
 /*
  * Do not check the array parameter using __must_be_array().
-- 
2.17.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH next 7/7] minmax.h: Remove some #defines that are only expanded once

[David Laight]

The bodies of __signed_type_use() and __unsigned_type_use() are
much the same size as their names - so put the bodies in the only
line that expands them.

Similarly __signed_type() is defined separately for 64bit and then
used exactly once just below.

Change the test for __signed_type from CONFIG_64BIT to one based
on gcc defined macros so that the code is valid if it gets used
outside of a kernel build.

Signed-off-by: David Laight <david.laight@aculab.com>
---
 include/linux/minmax.h | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 2bbdd5b5e07e..eaaf5c008e4d 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -46,10 +46,8 @@
  * comparison, and these expressions only need to be careful to not cause
  * warnings for pointer use.
  */
-#define __signed_type_use(ux) (2 + __is_nonneg(ux))
-#define __unsigned_type_use(ux) (1 + 2 * (sizeof(ux) < 4))
 #define __sign_use(ux) (is_signed_type(typeof(ux)) ? \
-	__signed_type_use(ux) : __unsigned_type_use(ux))
+	(2 + __is_nonneg(ux)) : (1 + 2 * (sizeof(ux) < 4)))
 
 /*
  * Check whether a signed value is always non-negative.
@@ -57,7 +55,7 @@
  * A cast is needed to avoid any warnings from values that aren't signed
  * integer types (in which case the result doesn't matter).
  *
- * On 64-bit any integer or pointer type can safely be cast to 'long'.
+ * On 64-bit any integer or pointer type can safely be cast to 'long long'.
  * But on 32-bit we need to avoid warnings about casting pointers to integers
  * of different sizes without truncating 64-bit values so 'long' or 'long long'
  * must be used depending on the size of the value.
@@ -66,12 +64,12 @@
  * them, but we do not use s128 types in the kernel (we do use 'u128',
  * but they are handled by the !is_signed_type() case).
  */
-#ifdef CONFIG_64BIT
-  #define __signed_type(ux) long
+#if __SIZEOF_POINTER__ == __SIZEOF_LONG_LONG__
+#define __is_nonneg(ux) statically_true((long long)(ux) >= 0)
 #else
-  #define __signed_type(ux) typeof(__builtin_choose_expr(sizeof(ux) > 4, 1LL, 1L))
+#define __is_nonneg(ux) statically_true( \
+	(typeof(__builtin_choose_expr(sizeof(ux) > 4, 1LL, 1L)))(ux) >= 0)
 #endif
-#define __is_nonneg(ux) statically_true((__signed_type(ux))(ux) >= 0)
 
 #define __types_ok(ux, uy) \
 	(__sign_use(ux) & __sign_use(uy))
-- 
2.17.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH next 6/7] minmax.h: Simplify the variants of clamp()

[kernel test robot]

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on next-20241121]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Laight/minmax-h-Add-whitespace-around-operators-and-after-commas/20241121-152617
base:   next-20241121
patch link:    https://lore.kernel.org/r/8f69f4deac014f558bab186444bac2e8%40AcuMS.aculab.com
patch subject: [PATCH next 6/7] minmax.h: Simplify the variants of clamp()
config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20241123/202411230458.dhZwh3TT-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241123/202411230458.dhZwh3TT-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411230458.dhZwh3TT-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:28,
                    from include/linux/cpumask.h:11,
                    from arch/loongarch/include/asm/processor.h:9,
                    from arch/loongarch/include/asm/thread_info.h:15,
                    from include/linux/thread_info.h:60,
                    from include/asm-generic/current.h:6,
                    from ./arch/loongarch/include/generated/asm/current.h:1,
                    from include/linux/sched.h:12,
                    from include/linux/delay.h:13,
                    from drivers/iio/magnetometer/yamaha-yas530.c:27:
   drivers/iio/magnetometer/yamaha-yas530.c: In function 'yas537_measure':
>> include/linux/minmax.h:188:20: warning: overflow in conversion from 'long unsigned int' to 's32' {aka 'int'} changes value from '18446744073709543424' to '-8192' [-Woverflow]
     188 |         type ulo = (lo);                                                        \
         |                    ^
   include/linux/minmax.h:197:9: note: in expansion of macro '__clamp_once'
     197 |         __clamp_once(type, val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_))
         |         ^~~~~~~~~~~~
   include/linux/minmax.h:233:32: note: in expansion of macro '__careful_clamp'
     233 | #define clamp_val(val, lo, hi) __careful_clamp(typeof(val), val, lo, hi)
         |                                ^~~~~~~~~~~~~~~
   drivers/iio/magnetometer/yamaha-yas530.c:414:25: note: in expansion of macro 'clamp_val'
     414 |                         clamp_val(h[i], -BIT(13), BIT(13) - 1);
         |                         ^~~~~~~~~


vim +188 include/linux/minmax.h

   172	
   173	/**
   174	 * min_not_zero - return the minimum that is _not_ zero, unless both are zero
   175	 * @x: value1
   176	 * @y: value2
   177	 */
   178	#define min_not_zero(x, y) ({			\
   179		typeof(x) __x = (x);			\
   180		typeof(y) __y = (y);			\
   181		__x == 0 ? __y : ((__y == 0) ? __x : min(__x, __y)); })
   182	
   183	#define __clamp(val, lo, hi)	\
   184		((val) >= (hi) ? (hi) : ((val) <= (lo) ? (lo) : (val)))
   185	
   186	#define __clamp_once(type, val, lo, hi, uval, ulo, uhi) ({			\
   187		type uval = (val);							\
 > 188		type ulo = (lo);							\
   189		type uhi = (hi);							\
   190		BUILD_BUG_ON_MSG(statically_true(ulo > uhi),				\
   191			"clamp() low limit " #lo " greater than high limit " #hi);	\
   192		BUILD_BUG_ON_MSG(!__types_ok3(uval, ulo, uhi),				\
   193			"clamp("#val", "#lo", "#hi") signedness error");		\
   194		__clamp(uval, ulo, uhi); })
   195	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH next 6/7] minmax.h: Simplify the variants of clamp()

[kernel test robot]

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on next-20241121]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Laight/minmax-h-Add-whitespace-around-operators-and-after-commas/20241121-152617
base:   next-20241121
patch link:    https://lore.kernel.org/r/8f69f4deac014f558bab186444bac2e8%40AcuMS.aculab.com
patch subject: [PATCH next 6/7] minmax.h: Simplify the variants of clamp()
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20241128/202411282222.oF0B4110-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241128/202411282222.oF0B4110-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411282222.oF0B4110-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/iio/magnetometer/yamaha-yas530.c:30:
   In file included from include/linux/i2c.h:13:
   In file included from include/linux/acpi.h:14:
   In file included from include/linux/device.h:32:
   In file included from include/linux/device/driver.h:21:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/s390/include/asm/elf.h:181:
   In file included from arch/s390/include/asm/mmu_context.h:11:
   In file included from arch/s390/include/asm/pgalloc.h:18:
   In file included from include/linux/mm.h:2223:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/magnetometer/yamaha-yas530.c:414:20: warning: implicit conversion from 'unsigned long' to 'typeof (h[i])' (aka 'int') changes value from 18446744073709543424 to -8192 [-Wconstant-conversion]
     414 |                         clamp_val(h[i], -BIT(13), BIT(13) - 1);
         |                         ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:233:66: note: expanded from macro 'clamp_val'
     233 | #define clamp_val(val, lo, hi) __careful_clamp(typeof(val), val, lo, hi)
         |                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
   include/linux/minmax.h:197:26: note: expanded from macro '__careful_clamp'
     197 |         __clamp_once(type, val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_))
         |         ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:188:14: note: expanded from macro '__clamp_once'
     188 |         type ulo = (lo);                                                        \
         |              ~~~    ^~
   5 warnings generated.


vim +414 drivers/iio/magnetometer/yamaha-yas530.c

de8860b1ed4701 Linus Walleij 2020-12-24  358  
65f79b50103067 Jakob Hauser  2022-08-13  359  /**
65f79b50103067 Jakob Hauser  2022-08-13  360   * yas537_measure() - Make a measure from the hardware
65f79b50103067 Jakob Hauser  2022-08-13  361   * @yas5xx: The device state
65f79b50103067 Jakob Hauser  2022-08-13  362   * @t: the raw temperature measurement
65f79b50103067 Jakob Hauser  2022-08-13  363   * @x: the raw x axis measurement
65f79b50103067 Jakob Hauser  2022-08-13  364   * @y1: the y1 axis measurement
65f79b50103067 Jakob Hauser  2022-08-13  365   * @y2: the y2 axis measurement
65f79b50103067 Jakob Hauser  2022-08-13  366   * @return: 0 on success or error code
65f79b50103067 Jakob Hauser  2022-08-13  367   */
65f79b50103067 Jakob Hauser  2022-08-13  368  static int yas537_measure(struct yas5xx *yas5xx, u16 *t, u16 *x, u16 *y1, u16 *y2)
de8860b1ed4701 Linus Walleij 2020-12-24  369  {
65f79b50103067 Jakob Hauser  2022-08-13  370  	struct yas5xx_calibration *c = &yas5xx->calibration;
65f79b50103067 Jakob Hauser  2022-08-13  371  	unsigned int busy;
65f79b50103067 Jakob Hauser  2022-08-13  372  	u8 data[8];
65f79b50103067 Jakob Hauser  2022-08-13  373  	u16 xy1y2[3];
65f79b50103067 Jakob Hauser  2022-08-13  374  	s32 h[3], s[3];
65f79b50103067 Jakob Hauser  2022-08-13  375  	int i, ret;
65f79b50103067 Jakob Hauser  2022-08-13  376  
65f79b50103067 Jakob Hauser  2022-08-13  377  	mutex_lock(&yas5xx->lock);
65f79b50103067 Jakob Hauser  2022-08-13  378  
65f79b50103067 Jakob Hauser  2022-08-13  379  	/* Contrary to YAS530/532, also a "cont" bit is set, meaning unknown */
65f79b50103067 Jakob Hauser  2022-08-13  380  	ret = regmap_write(yas5xx->map, YAS537_MEASURE, YAS5XX_MEASURE_START |
65f79b50103067 Jakob Hauser  2022-08-13  381  			   YAS5XX_MEASURE_CONT);
65f79b50103067 Jakob Hauser  2022-08-13  382  	if (ret < 0)
65f79b50103067 Jakob Hauser  2022-08-13  383  		goto out_unlock;
65f79b50103067 Jakob Hauser  2022-08-13  384  
65f79b50103067 Jakob Hauser  2022-08-13  385  	/* Use same timeout like YAS530/532 but the bit is in data row 2 */
65f79b50103067 Jakob Hauser  2022-08-13  386  	ret = regmap_read_poll_timeout(yas5xx->map, YAS5XX_MEASURE_DATA + 2, busy,
65f79b50103067 Jakob Hauser  2022-08-13  387  				       !(busy & YAS5XX_MEASURE_DATA_BUSY),
65f79b50103067 Jakob Hauser  2022-08-13  388  				       500, 20000);
65f79b50103067 Jakob Hauser  2022-08-13  389  	if (ret) {
65f79b50103067 Jakob Hauser  2022-08-13  390  		dev_err(yas5xx->dev, "timeout waiting for measurement\n");
65f79b50103067 Jakob Hauser  2022-08-13  391  		goto out_unlock;
65f79b50103067 Jakob Hauser  2022-08-13  392  	}
65f79b50103067 Jakob Hauser  2022-08-13  393  
65f79b50103067 Jakob Hauser  2022-08-13  394  	ret = regmap_bulk_read(yas5xx->map, YAS5XX_MEASURE_DATA,
65f79b50103067 Jakob Hauser  2022-08-13  395  			       data, sizeof(data));
65f79b50103067 Jakob Hauser  2022-08-13  396  	if (ret)
65f79b50103067 Jakob Hauser  2022-08-13  397  		goto out_unlock;
65f79b50103067 Jakob Hauser  2022-08-13  398  
65f79b50103067 Jakob Hauser  2022-08-13  399  	mutex_unlock(&yas5xx->lock);
65f79b50103067 Jakob Hauser  2022-08-13  400  
65f79b50103067 Jakob Hauser  2022-08-13  401  	*t = get_unaligned_be16(&data[0]);
65f79b50103067 Jakob Hauser  2022-08-13  402  	xy1y2[0] = FIELD_GET(GENMASK(13, 0), get_unaligned_be16(&data[2]));
65f79b50103067 Jakob Hauser  2022-08-13  403  	xy1y2[1] = get_unaligned_be16(&data[4]);
65f79b50103067 Jakob Hauser  2022-08-13  404  	xy1y2[2] = get_unaligned_be16(&data[6]);
65f79b50103067 Jakob Hauser  2022-08-13  405  
65f79b50103067 Jakob Hauser  2022-08-13  406  	/* The second version of YAS537 needs to include calibration coefficients */
65f79b50103067 Jakob Hauser  2022-08-13  407  	if (yas5xx->version == YAS537_VERSION_1) {
65f79b50103067 Jakob Hauser  2022-08-13  408  		for (i = 0; i < 3; i++)
65f79b50103067 Jakob Hauser  2022-08-13  409  			s[i] = xy1y2[i] - BIT(13);
65f79b50103067 Jakob Hauser  2022-08-13  410  		h[0] = (c->k *   (128 * s[0] + c->a2 * s[1] + c->a3 * s[2])) / BIT(13);
65f79b50103067 Jakob Hauser  2022-08-13  411  		h[1] = (c->k * (c->a4 * s[0] + c->a5 * s[1] + c->a6 * s[2])) / BIT(13);
65f79b50103067 Jakob Hauser  2022-08-13  412  		h[2] = (c->k * (c->a7 * s[0] + c->a8 * s[1] + c->a9 * s[2])) / BIT(13);
65f79b50103067 Jakob Hauser  2022-08-13  413  		for (i = 0; i < 3; i++) {
65f79b50103067 Jakob Hauser  2022-08-13 @414  			clamp_val(h[i], -BIT(13), BIT(13) - 1);
65f79b50103067 Jakob Hauser  2022-08-13  415  			xy1y2[i] = h[i] + BIT(13);
65f79b50103067 Jakob Hauser  2022-08-13  416  		}
65f79b50103067 Jakob Hauser  2022-08-13  417  	}
65f79b50103067 Jakob Hauser  2022-08-13  418  
65f79b50103067 Jakob Hauser  2022-08-13  419  	*x = xy1y2[0];
65f79b50103067 Jakob Hauser  2022-08-13  420  	*y1 = xy1y2[1];
65f79b50103067 Jakob Hauser  2022-08-13  421  	*y2 = xy1y2[2];
65f79b50103067 Jakob Hauser  2022-08-13  422  
65f79b50103067 Jakob Hauser  2022-08-13  423  	return 0;
65f79b50103067 Jakob Hauser  2022-08-13  424  
65f79b50103067 Jakob Hauser  2022-08-13  425  out_unlock:
65f79b50103067 Jakob Hauser  2022-08-13  426  	mutex_unlock(&yas5xx->lock);
65f79b50103067 Jakob Hauser  2022-08-13  427  	return ret;
65f79b50103067 Jakob Hauser  2022-08-13  428  }
65f79b50103067 Jakob Hauser  2022-08-13  429  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

RE: [PATCH next 6/7] minmax.h: Simplify the variants of clamp()

[David Laight]

From: kernel test robot
> Sent: 28 November 2024 15:05
> 
> Hi David,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on next-20241121]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/David-Laight/minmax-h-Add-whitespace-around-
> operators-and-after-commas/20241121-152617
> base:   next-20241121
> patch link:    https://lore.kernel.org/r/8f69f4deac014f558bab186444bac2e8%40AcuMS.aculab.com
> patch subject: [PATCH next 6/7] minmax.h: Simplify the variants of clamp()
> config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20241128/202411282222.oF0B4110-
> lkp@intel.com/config)
> compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project
> 592c0fe55f6d9a811028b5f3507be91458ab2713)
> reproduce (this is a W=1 build): (https://download.01.org/0day-
> ci/archive/20241128/202411282222.oF0B4110-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202411282222.oF0B4110-lkp@intel.com/
...
> vim +414 drivers/iio/magnetometer/yamaha-yas530.c
...
> 65f79b50103067 Jakob Hauser  2022-08-13  407  	if (yas5xx->version == YAS537_VERSION_1) {
> 65f79b50103067 Jakob Hauser  2022-08-13  408  		for (i = 0; i < 3; i++)
> 65f79b50103067 Jakob Hauser  2022-08-13  409  			s[i] = xy1y2[i] - BIT(13);
> 65f79b50103067 Jakob Hauser  2022-08-13  410  		h[0] = (c->k *   (128 * s[0] + c->a2 * s[1] + c-> >a3 * s[2])) / BIT(13);
> 65f79b50103067 Jakob Hauser  2022-08-13  411  		h[1] = (c->k * (c->a4 * s[0] + c->a5 * s[1] + c-> >a6 * s[2])) / BIT(13);
> 65f79b50103067 Jakob Hauser  2022-08-13  412  		h[2] = (c->k * (c->a7 * s[0] + c->a8 * s[1] + c-> >a9 * s[2])) / BIT(13);
> 65f79b50103067 Jakob Hauser  2022-08-13  413  		for (i = 0; i < 3; i++) {
> 65f79b50103067 Jakob Hauser  2022-08-13 @414  			clamp_val(h[i], -BIT(13), BIT(13) - 1);
> 65f79b50103067 Jakob Hauser  2022-08-13  415  			xy1y2[i] = h[i] + BIT(13);
> 65f79b50103067 Jakob Hauser  2022-08-13  416  		}
> 65f79b50103067 Jakob Hauser  2022-08-13  417  	}

Duplicate, very buggy code...
It very much needs BIT(13) to be signed.
And it shouldn't ignore the result of clamp()

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[Guenter Roeck]

Hi,

On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote:
> Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity
> check of the bounds in clamp().
> Gives better error coverage and one less expansion of the arguments.
> 
> Signed-off-by: David Laight <david.laight@aculab.com>

This patch triggers a build error when trying to build parisc:allmodconfig.
See error message and bisect log below.

I don't think there is anything wrong with the patch. The underlying
problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which
tries to build the affected file even though CONFIG_DRM_I915 is not
enabled/supported on parisc.

Copying XE maintainers for feedback/advice.

Thanks,
Guenter

---
Building parisc:allmodconfig ... failed
--------------
Error log:
In file included from <command-line>:
drivers/gpu/drm/i915/display/intel_backlight.c: In function 'scale':
include/linux/compiler_types.h:542:45: error: call to '__compiletime_assert_415' declared with attribute error: clamp() low limit source_min greater than high limit source_max
  542 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
      |                                             ^
include/linux/compiler_types.h:523:25: note: in definition of macro '__compiletime_assert'
  523 |                         prefix ## suffix();                             \
      |                         ^~~~~~
include/linux/compiler_types.h:542:9: note: in expansion of macro '_compiletime_assert'
  542 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
      |         ^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
   39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
      |                                     ^~~~~~~~~~~~~~~~~~
include/linux/minmax.h:188:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
  188 |         BUILD_BUG_ON_MSG(statically_true(ulo > uhi),                            \
      |         ^~~~~~~~~~~~~~~~
include/linux/minmax.h:195:9: note: in expansion of macro '__clamp_once'
  195 |         __clamp_once(type, val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_))
      |         ^~~~~~~~~~~~
include/linux/minmax.h:206:28: note: in expansion of macro '__careful_clamp'
  206 | #define clamp(val, lo, hi) __careful_clamp(__auto_type, val, lo, hi)
      |                            ^~~~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_backlight.c:48:22: note: in expansion of macro 'clamp'
   48 |         source_val = clamp(source_val, source_min, source_max);
      |                      ^~~~~

---
# bad: [0907e7fb35756464aa34c35d6abb02998418164b] Add linux-next specific files for 20250117
# good: [5bc55a333a2f7316b58edc7573e8e893f7acb532] Linux 6.13-rc7
git bisect start 'HEAD' 'v6.13-rc7'
# bad: [195cedf4deacf84167c32b866ceac1cf4a16df15] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
git bisect bad 195cedf4deacf84167c32b866ceac1cf4a16df15
# bad: [e8c0711b153b0db806410d8b31ed23b590f4eab4] Merge branch 'xtensa-for-next' of git://github.com/jcmvbkbc/linux-xtensa.git
git bisect bad e8c0711b153b0db806410d8b31ed23b590f4eab4
# bad: [81d45722d699e594c66c150c8f7a0ec2e2bc9be6] Merge branch 'for-next/perf' of git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git
git bisect bad 81d45722d699e594c66c150c8f7a0ec2e2bc9be6
# bad: [7acb844a672defb15cf202a501815ec22c68c800] foo
git bisect bad 7acb844a672defb15cf202a501815ec22c68c800
# good: [fb2368075b135f174264071b851330649d55f9d0] mm/damon/core: add damos_filter->allow field
git bisect good fb2368075b135f174264071b851330649d55f9d0
# bad: [fc83c501e385753c90db7316faf9fd4158caaa96] minmax.h: remove some #defines that are only expanded once
git bisect bad fc83c501e385753c90db7316faf9fd4158caaa96
# good: [b04d305df1171448df5e87802c4d1f1022cc5784] ocfs2: use a folio in ocfs2_map_and_dirty_page()
git bisect good b04d305df1171448df5e87802c4d1f1022cc5784
# good: [7e01619507058f90ab603acec482951f3c452aaa] kthread: correct comments before kthread_queue_work()
git bisect good 7e01619507058f90ab603acec482951f3c452aaa
# good: [21b510a64c223707caa6db6176128779f0806a73] nilfs2: correct return value kernel-doc descriptions for ioctl functions
git bisect good 21b510a64c223707caa6db6176128779f0806a73
# good: [6afb87f23458f2d4e4334ee5a4efb8b0d07af68b] nilfs2: handle errors that nilfs_prepare_chunk() may return
git bisect good 6afb87f23458f2d4e4334ee5a4efb8b0d07af68b
# good: [8f6d46fed0bad163e5146fea1fdff150039235b2] minmax.h: reduce the #define expansion of min(), max() and clamp()
git bisect good 8f6d46fed0bad163e5146fea1fdff150039235b2
# bad: [7a70c678548d71e609b95dbddf2d411a02d13b54] minmax.h: move all the clamp() definitions after the min/max() ones
git bisect bad 7a70c678548d71e609b95dbddf2d411a02d13b54
# bad: [37f375aab0c585388b90d1af6968454fc2769cb9] minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()
git bisect bad 37f375aab0c585388b90d1af6968454fc2769cb9
# first bad commit: [37f375aab0c585388b90d1af6968454fc2769cb9] minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()

Re: Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[David Laight]

On Sat, 18 Jan 2025 08:13:06 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> Hi,
> 
> On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote:
> > Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity
> > check of the bounds in clamp().
> > Gives better error coverage and one less expansion of the arguments.
> > 
> > Signed-off-by: David Laight <david.laight@aculab.com>  
> 
> This patch triggers a build error when trying to build parisc:allmodconfig.
> See error message and bisect log below.
> 
> I don't think there is anything wrong with the patch. The underlying
> problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which
> tries to build the affected file even though CONFIG_DRM_I915 is not
> enabled/supported on parisc.

This has appeared before.
Any idea which inlined copy of scale() is causing the problem.
On the face of it they all look ok.

If you can reproduce it maybe try commenting out some of the calls.

	David

> 
> Copying XE maintainers for feedback/advice.
> 
> Thanks,
> Guenter
> 
> ---
> Building parisc:allmodconfig ... failed
> --------------
> Error log:
> In file included from <command-line>:
> drivers/gpu/drm/i915/display/intel_backlight.c: In function 'scale':
> include/linux/compiler_types.h:542:45: error: call to '__compiletime_assert_415' declared with attribute error: clamp() low limit source_min greater than high limit source_max
>   542 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>       |                                             ^
> include/linux/compiler_types.h:523:25: note: in definition of macro '__compiletime_assert'
>   523 |                         prefix ## suffix();                             \
>       |                         ^~~~~~
> include/linux/compiler_types.h:542:9: note: in expansion of macro '_compiletime_assert'
>   542 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>       |         ^~~~~~~~~~~~~~~~~~~
> include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
>    39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
>       |                                     ^~~~~~~~~~~~~~~~~~
> include/linux/minmax.h:188:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
>   188 |         BUILD_BUG_ON_MSG(statically_true(ulo > uhi),                            \
>       |         ^~~~~~~~~~~~~~~~
> include/linux/minmax.h:195:9: note: in expansion of macro '__clamp_once'
>   195 |         __clamp_once(type, val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_))
>       |         ^~~~~~~~~~~~
> include/linux/minmax.h:206:28: note: in expansion of macro '__careful_clamp'
>   206 | #define clamp(val, lo, hi) __careful_clamp(__auto_type, val, lo, hi)
>       |                            ^~~~~~~~~~~~~~~
> drivers/gpu/drm/i915/display/intel_backlight.c:48:22: note: in expansion of macro 'clamp'
>    48 |         source_val = clamp(source_val, source_min, source_max);
>       |                      ^~~~~
> 
> ---
> # bad: [0907e7fb35756464aa34c35d6abb02998418164b] Add linux-next specific files for 20250117
> # good: [5bc55a333a2f7316b58edc7573e8e893f7acb532] Linux 6.13-rc7
> git bisect start 'HEAD' 'v6.13-rc7'
> # bad: [195cedf4deacf84167c32b866ceac1cf4a16df15] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
> git bisect bad 195cedf4deacf84167c32b866ceac1cf4a16df15
> # bad: [e8c0711b153b0db806410d8b31ed23b590f4eab4] Merge branch 'xtensa-for-next' of git://github.com/jcmvbkbc/linux-xtensa.git
> git bisect bad e8c0711b153b0db806410d8b31ed23b590f4eab4
> # bad: [81d45722d699e594c66c150c8f7a0ec2e2bc9be6] Merge branch 'for-next/perf' of git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git
> git bisect bad 81d45722d699e594c66c150c8f7a0ec2e2bc9be6
> # bad: [7acb844a672defb15cf202a501815ec22c68c800] foo
> git bisect bad 7acb844a672defb15cf202a501815ec22c68c800
> # good: [fb2368075b135f174264071b851330649d55f9d0] mm/damon/core: add damos_filter->allow field
> git bisect good fb2368075b135f174264071b851330649d55f9d0
> # bad: [fc83c501e385753c90db7316faf9fd4158caaa96] minmax.h: remove some #defines that are only expanded once
> git bisect bad fc83c501e385753c90db7316faf9fd4158caaa96
> # good: [b04d305df1171448df5e87802c4d1f1022cc5784] ocfs2: use a folio in ocfs2_map_and_dirty_page()
> git bisect good b04d305df1171448df5e87802c4d1f1022cc5784
> # good: [7e01619507058f90ab603acec482951f3c452aaa] kthread: correct comments before kthread_queue_work()
> git bisect good 7e01619507058f90ab603acec482951f3c452aaa
> # good: [21b510a64c223707caa6db6176128779f0806a73] nilfs2: correct return value kernel-doc descriptions for ioctl functions
> git bisect good 21b510a64c223707caa6db6176128779f0806a73
> # good: [6afb87f23458f2d4e4334ee5a4efb8b0d07af68b] nilfs2: handle errors that nilfs_prepare_chunk() may return
> git bisect good 6afb87f23458f2d4e4334ee5a4efb8b0d07af68b
> # good: [8f6d46fed0bad163e5146fea1fdff150039235b2] minmax.h: reduce the #define expansion of min(), max() and clamp()
> git bisect good 8f6d46fed0bad163e5146fea1fdff150039235b2
> # bad: [7a70c678548d71e609b95dbddf2d411a02d13b54] minmax.h: move all the clamp() definitions after the min/max() ones
> git bisect bad 7a70c678548d71e609b95dbddf2d411a02d13b54
> # bad: [37f375aab0c585388b90d1af6968454fc2769cb9] minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()
> git bisect bad 37f375aab0c585388b90d1af6968454fc2769cb9
> # first bad commit: [37f375aab0c585388b90d1af6968454fc2769cb9] minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()
> 
> 

Re: Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[Guenter Roeck]

On Sat, Jan 18, 2025 at 05:09:59PM +0000, David Laight wrote:
> On Sat, 18 Jan 2025 08:13:06 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > Hi,
> > 
> > On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote:
> > > Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity
> > > check of the bounds in clamp().
> > > Gives better error coverage and one less expansion of the arguments.
> > > 
> > > Signed-off-by: David Laight <david.laight@aculab.com>  
> > 
> > This patch triggers a build error when trying to build parisc:allmodconfig.
> > See error message and bisect log below.
> > 
> > I don't think there is anything wrong with the patch. The underlying
> > problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which
> > tries to build the affected file even though CONFIG_DRM_I915 is not
> > enabled/supported on parisc.
> 
> This has appeared before.
> Any idea which inlined copy of scale() is causing the problem.
> On the face of it they all look ok.
> 
> If you can reproduce it maybe try commenting out some of the calls.
> 

See diff below. All three changes are needed.
No idea why the compiler would know that the values are invalid.

Guenter

---
diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
index fc1e517e074a..3b2c8bdfcf8d 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_backlight.c
@@ -76,10 +76,14 @@ static u32 clamp_user_to_hw(struct intel_connector *connector,
 static u32 scale_hw_to_user(struct intel_connector *connector,
 			    u32 hw_level, u32 user_max)
 {
+#if 0
 	struct intel_panel *panel = &connector->panel;
 
 	return scale(hw_level, panel->backlight.min, panel->backlight.max,
 		     0, user_max);
+#else
+	return 0;
+#endif
 }
 
 u32 intel_backlight_invert_pwm_level(struct intel_connector *connector, u32 val)
@@ -119,8 +123,10 @@ u32 intel_backlight_level_to_pwm(struct intel_connector *connector, u32 val)
 	drm_WARN_ON_ONCE(&i915->drm,
 			 panel->backlight.max == 0 || panel->backlight.pwm_level_max == 0);
 
+#if 0
 	val = scale(val, panel->backlight.min, panel->backlight.max,
 		    panel->backlight.pwm_level_min, panel->backlight.pwm_level_max);
+#endif
 
 	return intel_backlight_invert_pwm_level(connector, val);
 }
@@ -138,8 +144,12 @@ u32 intel_backlight_level_from_pwm(struct intel_connector *connector, u32 val)
 	     intel_has_quirk(display, QUIRK_INVERT_BRIGHTNESS)))
 		val = panel->backlight.pwm_level_max - (val - panel->backlight.pwm_level_min);
 
+#if 0
 	return scale(val, panel->backlight.pwm_level_min, panel->backlight.pwm_level_max,
 		     panel->backlight.min, panel->backlight.max);
+#else
+	return 0;
+#endif
 }
 
 static u32 lpt_get_backlight(struct intel_connector *connector, enum pipe unused)

Re: Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[David Laight]

On Sat, 18 Jan 2025 09:49:21 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> On Sat, Jan 18, 2025 at 05:09:59PM +0000, David Laight wrote:
> > On Sat, 18 Jan 2025 08:13:06 -0800
> > Guenter Roeck <linux@roeck-us.net> wrote:
> >   
> > > Hi,
> > > 
> > > On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote:  
> > > > Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity
> > > > check of the bounds in clamp().
> > > > Gives better error coverage and one less expansion of the arguments.
> > > > 
> > > > Signed-off-by: David Laight <david.laight@aculab.com>    
> > > 
> > > This patch triggers a build error when trying to build parisc:allmodconfig.
> > > See error message and bisect log below.
> > > 
> > > I don't think there is anything wrong with the patch. The underlying
> > > problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which
> > > tries to build the affected file even though CONFIG_DRM_I915 is not
> > > enabled/supported on parisc.  
> > 
> > This has appeared before.
> > Any idea which inlined copy of scale() is causing the problem.
> > On the face of it they all look ok.
> > 
> > If you can reproduce it maybe try commenting out some of the calls.
> >   
> 
> See diff below. All three changes are needed.
> No idea why the compiler would know that the values are invalid.

Maybe it isn't even an inlining issue.
Perhaps that compiler just doesn't like the function ?
What happens without the 'static' (and an extra prototype)?

	David

> 
> Guenter
> 
> ---
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index fc1e517e074a..3b2c8bdfcf8d 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> @@ -76,10 +76,14 @@ static u32 clamp_user_to_hw(struct intel_connector *connector,
>  static u32 scale_hw_to_user(struct intel_connector *connector,
>  			    u32 hw_level, u32 user_max)
>  {
> +#if 0
>  	struct intel_panel *panel = &connector->panel;
>  
>  	return scale(hw_level, panel->backlight.min, panel->backlight.max,
>  		     0, user_max);
> +#else
> +	return 0;
> +#endif
>  }
>  
>  u32 intel_backlight_invert_pwm_level(struct intel_connector *connector, u32 val)
> @@ -119,8 +123,10 @@ u32 intel_backlight_level_to_pwm(struct intel_connector *connector, u32 val)
>  	drm_WARN_ON_ONCE(&i915->drm,
>  			 panel->backlight.max == 0 || panel->backlight.pwm_level_max == 0);
>  
> +#if 0
>  	val = scale(val, panel->backlight.min, panel->backlight.max,
>  		    panel->backlight.pwm_level_min, panel->backlight.pwm_level_max);
> +#endif
>  
>  	return intel_backlight_invert_pwm_level(connector, val);
>  }
> @@ -138,8 +144,12 @@ u32 intel_backlight_level_from_pwm(struct intel_connector *connector, u32 val)
>  	     intel_has_quirk(display, QUIRK_INVERT_BRIGHTNESS)))
>  		val = panel->backlight.pwm_level_max - (val - panel->backlight.pwm_level_min);
>  
> +#if 0
>  	return scale(val, panel->backlight.pwm_level_min, panel->backlight.pwm_level_max,
>  		     panel->backlight.min, panel->backlight.max);
> +#else
> +	return 0;
> +#endif
>  }
>  
>  static u32 lpt_get_backlight(struct intel_connector *connector, enum pipe unused)

Re: Buiild error in i915/xe

[Guenter Roeck]

On 1/18/25 10:09, David Laight wrote:
> On Sat, 18 Jan 2025 09:49:21 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On Sat, Jan 18, 2025 at 05:09:59PM +0000, David Laight wrote:
>>> On Sat, 18 Jan 2025 08:13:06 -0800
>>> Guenter Roeck <linux@roeck-us.net> wrote:
>>>    
>>>> Hi,
>>>>
>>>> On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote:
>>>>> Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity
>>>>> check of the bounds in clamp().
>>>>> Gives better error coverage and one less expansion of the arguments.
>>>>>
>>>>> Signed-off-by: David Laight <david.laight@aculab.com>
>>>>
>>>> This patch triggers a build error when trying to build parisc:allmodconfig.
>>>> See error message and bisect log below.
>>>>
>>>> I don't think there is anything wrong with the patch. The underlying
>>>> problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which
>>>> tries to build the affected file even though CONFIG_DRM_I915 is not
>>>> enabled/supported on parisc.
>>>
>>> This has appeared before.
>>> Any idea which inlined copy of scale() is causing the problem.
>>> On the face of it they all look ok.
>>>
>>> If you can reproduce it maybe try commenting out some of the calls.
>>>    
>>
>> See diff below. All three changes are needed.
>> No idea why the compiler would know that the values are invalid.
> 
> Maybe it isn't even an inlining issue.
> Perhaps that compiler just doesn't like the function ?
> What happens without the 'static' (and an extra prototype)?
> 


You mean like that ?

-static u32 scale(u32 source_val,
+
+u32 scale(u32 source_val,
+                u32 source_min, u32 source_max,
+                u32 target_min, u32 target_max);
+
+u32 scale(u32 source_val,
                  u32 source_min, u32 source_max,
                  u32 target_min, u32 target_max)

It doesn't help. Worse, after that change the error is still reported
even with the #if 0 elsewhere.

Guenter

Re: Buiild error in i915/xe

[David Laight]

On Sat, 18 Jan 2025 10:36:11 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> On 1/18/25 10:09, David Laight wrote:
> > On Sat, 18 Jan 2025 09:49:21 -0800
> > Guenter Roeck <linux@roeck-us.net> wrote:
> >   
> >> On Sat, Jan 18, 2025 at 05:09:59PM +0000, David Laight wrote:  
> >>> On Sat, 18 Jan 2025 08:13:06 -0800
> >>> Guenter Roeck <linux@roeck-us.net> wrote:
> >>>      
> >>>> Hi,
> >>>>
> >>>> On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote:  
> >>>>> Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity
> >>>>> check of the bounds in clamp().
> >>>>> Gives better error coverage and one less expansion of the arguments.
> >>>>>
> >>>>> Signed-off-by: David Laight <david.laight@aculab.com>  
> >>>>
> >>>> This patch triggers a build error when trying to build parisc:allmodconfig.
> >>>> See error message and bisect log below.
> >>>>
> >>>> I don't think there is anything wrong with the patch. The underlying
> >>>> problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which
> >>>> tries to build the affected file even though CONFIG_DRM_I915 is not
> >>>> enabled/supported on parisc.  
> >>>
> >>> This has appeared before.
> >>> Any idea which inlined copy of scale() is causing the problem.
> >>> On the face of it they all look ok.
> >>>
> >>> If you can reproduce it maybe try commenting out some of the calls.
> >>>      
> >>
> >> See diff below. All three changes are needed.
> >> No idea why the compiler would know that the values are invalid.  
> > 
> > Maybe it isn't even an inlining issue.
> > Perhaps that compiler just doesn't like the function ?
> > What happens without the 'static' (and an extra prototype)?
> >   
> 
> 
> You mean like that ?
> 
> -static u32 scale(u32 source_val,
> +
> +u32 scale(u32 source_val,
> +                u32 source_min, u32 source_max,
> +                u32 target_min, u32 target_max);
> +
> +u32 scale(u32 source_val,
>                   u32 source_min, u32 source_max,
>                   u32 target_min, u32 target_max)
> 
> It doesn't help. Worse, after that change the error is still reported
> even with the #if 0 elsewhere.

Yes - that means the compiler is 'objecting' to the scale() function itself.
(Without any regard for its callers.)
Which should make it easy to reproduce outside the kernel build.

I think Mat had a successful build with a different (older?) version of gcc for
parisc.

There must be something odd causing the problem - there will be other clamp()
calls in the build that don't generate the error.

Remember that lack of the error messages requires the compiler optimise away
some code - so if the optimisation is skipped the call could be generated
and the warning output (even if the call is optimised away later).

Perhaps there is some obscure interaction with the WARN() statements?

I don't have the required compiler (neither does godbolt).

	David 

> 
> Guenter
> 

Re: Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[Linus Torvalds]

On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:
>
> No idea why the compiler would know that the values are invalid.

It's not that the compiler knows tat they are invalid, but I bet what
happens is in scale() (and possibly other places that do similar
checks), which does this:

        WARN_ON(source_min > source_max);
        ...
        source_val = clamp(source_val, source_min, source_max);

and the compiler notices that the ordering comparison in the first
WARN_ON() is the same as the one in clamp(), so it basically converts
the logic to

        if (source_min > source_max) {
                WARN(..);
                /* Do the clamp() knowing that source_min > source_max */
                source_val = clamp(source_val, source_min, source_max);
        } else {
                /* Do the clamp knowing that source_min <= source_max */
                source_val = clamp(source_val, source_min, source_max);
        }

(obviously I dropped the other WARN_ON in the conversion, it wasn't
relevant for this case).

And now that first clamp() case is done with source_min > source_max,
and it triggers that build error because that's invalid.

So the condition is not statically true in the *source* code, but in
the "I have moved code around to combine tests" case it now *is*
statically true as far as the compiler is concerned.

              Linus

Re: Buiild error in i915/xe

[Guenter Roeck]

On 1/18/25 13:18, David Laight wrote:
> On Sat, 18 Jan 2025 10:36:11 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On 1/18/25 10:09, David Laight wrote:
>>> On Sat, 18 Jan 2025 09:49:21 -0800
>>> Guenter Roeck <linux@roeck-us.net> wrote:
>>>    
>>>> On Sat, Jan 18, 2025 at 05:09:59PM +0000, David Laight wrote:
>>>>> On Sat, 18 Jan 2025 08:13:06 -0800
>>>>> Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>       
>>>>>> Hi,
>>>>>>
>>>>>> On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote:
>>>>>>> Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity
>>>>>>> check of the bounds in clamp().
>>>>>>> Gives better error coverage and one less expansion of the arguments.
>>>>>>>
>>>>>>> Signed-off-by: David Laight <david.laight@aculab.com>
>>>>>>
>>>>>> This patch triggers a build error when trying to build parisc:allmodconfig.
>>>>>> See error message and bisect log below.
>>>>>>
>>>>>> I don't think there is anything wrong with the patch. The underlying
>>>>>> problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which
>>>>>> tries to build the affected file even though CONFIG_DRM_I915 is not
>>>>>> enabled/supported on parisc.
>>>>>
>>>>> This has appeared before.
>>>>> Any idea which inlined copy of scale() is causing the problem.
>>>>> On the face of it they all look ok.
>>>>>
>>>>> If you can reproduce it maybe try commenting out some of the calls.
>>>>>       
>>>>
>>>> See diff below. All three changes are needed.
>>>> No idea why the compiler would know that the values are invalid.
>>>
>>> Maybe it isn't even an inlining issue.
>>> Perhaps that compiler just doesn't like the function ?
>>> What happens without the 'static' (and an extra prototype)?
>>>    
>>
>>
>> You mean like that ?
>>
>> -static u32 scale(u32 source_val,
>> +
>> +u32 scale(u32 source_val,
>> +                u32 source_min, u32 source_max,
>> +                u32 target_min, u32 target_max);
>> +
>> +u32 scale(u32 source_val,
>>                    u32 source_min, u32 source_max,
>>                    u32 target_min, u32 target_max)
>>
>> It doesn't help. Worse, after that change the error is still reported
>> even with the #if 0 elsewhere.
> 
> Yes - that means the compiler is 'objecting' to the scale() function itself.
> (Without any regard for its callers.)
> Which should make it easy to reproduce outside the kernel build.
> 
> I think Mat had a successful build with a different (older?) version of gcc for
> parisc.
> 
> There must be something odd causing the problem - there will be other clamp()
> calls in the build that don't generate the error.
> 
> Remember that lack of the error messages requires the compiler optimise away
> some code - so if the optimisation is skipped the call could be generated
> and the warning output (even if the call is optimised away later).
> 
> Perhaps there is some obscure interaction with the WARN() statements?
> 
> I don't have the required compiler (neither does godbolt).
> 

Oh man - that was a good hint. Turns out I can only reproduce the problem with
gcc 13.2 and 13.3. gcc 10.3, 11.4, and 12.4 do not generate the error.

And, yes, I can "fix" the problem with

-       WARN_ON(source_min > source_max);
+       // WARN_ON(source_min > source_max);

Any idea what to do ? Should I just scrap builds with gcc 13.x ?

Thanks,
Guenter

Re: Buiild error in i915/xe

[Guenter Roeck]

On 1/18/25 13:21, Linus Torvalds wrote:
> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> No idea why the compiler would know that the values are invalid.
> 
> It's not that the compiler knows tat they are invalid, but I bet what
> happens is in scale() (and possibly other places that do similar
> checks), which does this:
> 
>          WARN_ON(source_min > source_max);
>          ...
>          source_val = clamp(source_val, source_min, source_max);
> 
> and the compiler notices that the ordering comparison in the first
> WARN_ON() is the same as the one in clamp(), so it basically converts
> the logic to
> 
>          if (source_min > source_max) {
>                  WARN(..);
>                  /* Do the clamp() knowing that source_min > source_max */
>                  source_val = clamp(source_val, source_min, source_max);
>          } else {
>                  /* Do the clamp knowing that source_min <= source_max */
>                  source_val = clamp(source_val, source_min, source_max);
>          }
> 
> (obviously I dropped the other WARN_ON in the conversion, it wasn't
> relevant for this case).
> 
> And now that first clamp() case is done with source_min > source_max,
> and it triggers that build error because that's invalid.
> 
> So the condition is not statically true in the *source* code, but in
> the "I have moved code around to combine tests" case it now *is*
> statically true as far as the compiler is concerned.
> 

Yes, turns out I can reproduce the problem by adding WARN_ON() ahead
of similar clamp() calls (see below). However, I can only reproduce it
with gcc 13.3 for parisc. I don't see the problem with other cross compilers
(I tried arm, powerpc, and loongarch64). Compilers are weird :-(.

I am not sure what to do here. That kind of problem seems difficult
to avoid, and I am sure we will hit it again elsewhere. Should I declare
gcc 13.x off limits for parisc builds ?

Guenter

---
diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c
index 505c562a5daa..71c0da31a9d2 100644
--- a/drivers/input/mousedev.c
+++ b/drivers/input/mousedev.c
@@ -179,6 +179,7 @@ static void mousedev_abs_event(struct input_dev *dev, struct mousedev *mousedev,
                 if (size == 0)
                         size = xres ? : 1;

+               WARN_ON(min > max);
                 value = clamp(value, min, max);

                 mousedev->packet.x = ((value - min) * xres) / size;

Re: Buiild error in i915/xe

[Linus Torvalds]

On Sat, 18 Jan 2025 at 13:59, Guenter Roeck <linux@roeck-us.net> wrote:
>
> I am not sure what to do here. That kind of problem seems difficult
> to avoid, and I am sure we will hit it again elsewhere. Should I declare
> gcc 13.x off limits for parisc builds ?

No, I'm sure it can happen on other architectures too.

I think the only thing that makes parisc trigger this is that its
WARN_ON() is slightly different from others, and uses the
"__ret_warn_on" a few more times, and that just happens to make the
compiler decide to simplify all those tests.

Or something like that.

           Linus

Re: Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[David Laight]

On Sat, 18 Jan 2025 13:21:39 -0800
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > No idea why the compiler would know that the values are invalid.  
> 
> It's not that the compiler knows tat they are invalid, but I bet what
> happens is in scale() (and possibly other places that do similar
> checks), which does this:
> 
>         WARN_ON(source_min > source_max);
>         ...
>         source_val = clamp(source_val, source_min, source_max);
> 
> and the compiler notices that the ordering comparison in the first
> WARN_ON() is the same as the one in clamp(), so it basically converts
> the logic to
> 
>         if (source_min > source_max) {
>                 WARN(..);
>                 /* Do the clamp() knowing that source_min > source_max */
>                 source_val = clamp(source_val, source_min, source_max);
>         } else {
>                 /* Do the clamp knowing that source_min <= source_max */
>                 source_val = clamp(source_val, source_min, source_max);
>         }
> 
> (obviously I dropped the other WARN_ON in the conversion, it wasn't
> relevant for this case).
> 
> And now that first clamp() case is done with source_min > source_max,
> and it triggers that build error because that's invalid.
> 
> So the condition is not statically true in the *source* code, but in
> the "I have moved code around to combine tests" case it now *is*
> statically true as far as the compiler is concerned.

Well spotted :-)

One option would be to move the WARN_ON() below the clamp() and
add an OPTIMISER_HIDE_VAR(source_max) between them.

Or do something more sensible than the WARN().
Perhaps return target_min on any such errors?

	David

> 
>               Linus
> 

Re: Buiild error in i915/xe

[Guenter Roeck]

On 1/18/25 14:11, David Laight wrote:
> On Sat, 18 Jan 2025 13:21:39 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
>> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> No idea why the compiler would know that the values are invalid.
>>
>> It's not that the compiler knows tat they are invalid, but I bet what
>> happens is in scale() (and possibly other places that do similar
>> checks), which does this:
>>
>>          WARN_ON(source_min > source_max);
>>          ...
>>          source_val = clamp(source_val, source_min, source_max);
>>
>> and the compiler notices that the ordering comparison in the first
>> WARN_ON() is the same as the one in clamp(), so it basically converts
>> the logic to
>>
>>          if (source_min > source_max) {
>>                  WARN(..);
>>                  /* Do the clamp() knowing that source_min > source_max */
>>                  source_val = clamp(source_val, source_min, source_max);
>>          } else {
>>                  /* Do the clamp knowing that source_min <= source_max */
>>                  source_val = clamp(source_val, source_min, source_max);
>>          }
>>
>> (obviously I dropped the other WARN_ON in the conversion, it wasn't
>> relevant for this case).
>>
>> And now that first clamp() case is done with source_min > source_max,
>> and it triggers that build error because that's invalid.
>>
>> So the condition is not statically true in the *source* code, but in
>> the "I have moved code around to combine tests" case it now *is*
>> statically true as far as the compiler is concerned.
> 
> Well spotted :-)
> 
> One option would be to move the WARN_ON() below the clamp() and
> add an OPTIMISER_HIDE_VAR(source_max) between them.
> 
> Or do something more sensible than the WARN().
> Perhaps return target_min on any such errors?
> 

This helps:

-       WARN_ON(source_min > source_max);
-       WARN_ON(target_min > target_max);
-
         /* defensive */
         source_val = clamp(source_val, source_min, source_max);

+       WARN_ON(source_min > source_max);
+       WARN_ON(target_min > target_max);

Guenter

Re: Buiild error in i915/xe (was: [PATCH next 4/7] minmax.h: Use BUILD_BUG_ON_MSG() for the lo < hi test in clamp())

[Pedro Falcato]

On Sat, Jan 18, 2025 at 10:11 PM David Laight
<david.laight.linux@gmail.com> wrote:
>
> On Sat, 18 Jan 2025 13:21:39 -0800
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > No idea why the compiler would know that the values are invalid.
> >
> > It's not that the compiler knows tat they are invalid, but I bet what
> > happens is in scale() (and possibly other places that do similar
> > checks), which does this:
> >
> >         WARN_ON(source_min > source_max);
> >         ...
> >         source_val = clamp(source_val, source_min, source_max);
> >
> > and the compiler notices that the ordering comparison in the first
> > WARN_ON() is the same as the one in clamp(), so it basically converts
> > the logic to
> >
> >         if (source_min > source_max) {
> >                 WARN(..);
> >                 /* Do the clamp() knowing that source_min > source_max */
> >                 source_val = clamp(source_val, source_min, source_max);
> >         } else {
> >                 /* Do the clamp knowing that source_min <= source_max */
> >                 source_val = clamp(source_val, source_min, source_max);
> >         }
> >
> > (obviously I dropped the other WARN_ON in the conversion, it wasn't
> > relevant for this case).
> >
> > And now that first clamp() case is done with source_min > source_max,
> > and it triggers that build error because that's invalid.
> >
> > So the condition is not statically true in the *source* code, but in
> > the "I have moved code around to combine tests" case it now *is*
> > statically true as far as the compiler is concerned.
>
> Well spotted :-)
>
> One option would be to move the WARN_ON() below the clamp() and
> add an OPTIMISER_HIDE_VAR(source_max) between them.
>
> Or do something more sensible than the WARN().

Given the awful problems we've found with these macros (clamp, min,
max, etc), maybe the best option is to not play these awful games in
general?

These macros seem footgunned to hell just as a way to try to
compensate for C's lack of language features.

-- 
Pedro

Re: Buiild error in i915/xe

[David Laight]

On Sat, 18 Jan 2025 14:58:48 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> On 1/18/25 14:11, David Laight wrote:
> > On Sat, 18 Jan 2025 13:21:39 -0800
> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >   
> >> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:  
> >>>
> >>> No idea why the compiler would know that the values are invalid.  
> >>
> >> It's not that the compiler knows tat they are invalid, but I bet what
> >> happens is in scale() (and possibly other places that do similar
> >> checks), which does this:
> >>
> >>          WARN_ON(source_min > source_max);
> >>          ...
> >>          source_val = clamp(source_val, source_min, source_max);
> >>
> >> and the compiler notices that the ordering comparison in the first
> >> WARN_ON() is the same as the one in clamp(), so it basically converts
> >> the logic to
> >>
> >>          if (source_min > source_max) {
> >>                  WARN(..);
> >>                  /* Do the clamp() knowing that source_min > source_max */
> >>                  source_val = clamp(source_val, source_min, source_max);
> >>          } else {
> >>                  /* Do the clamp knowing that source_min <= source_max */
> >>                  source_val = clamp(source_val, source_min, source_max);
> >>          }
> >>
> >> (obviously I dropped the other WARN_ON in the conversion, it wasn't
> >> relevant for this case).
> >>
> >> And now that first clamp() case is done with source_min > source_max,
> >> and it triggers that build error because that's invalid.
> >>
> >> So the condition is not statically true in the *source* code, but in
> >> the "I have moved code around to combine tests" case it now *is*
> >> statically true as far as the compiler is concerned.  
> > 
> > Well spotted :-)
> > 
> > One option would be to move the WARN_ON() below the clamp() and
> > add an OPTIMISER_HIDE_VAR(source_max) between them.
> > 
> > Or do something more sensible than the WARN().
> > Perhaps return target_min on any such errors?
> >   
> 
> This helps:
> 
> -       WARN_ON(source_min > source_max);
> -       WARN_ON(target_min > target_max);
> -
>          /* defensive */
>          source_val = clamp(source_val, source_min, source_max);
> 
> +       WARN_ON(source_min > source_max);
> +       WARN_ON(target_min > target_max);

That is a 'quick fix' ...

Much better would be to replace the WARN() with (say):
	if (target_min >= target_max)
		return target_min;
	if (source_min >= source_max)
		return target_min + (target_max - target_min)/2;
So that the return values are actually in range (in as much as one is defined).
Note that the >= cpmparisons also remove a divide by zero.

	David

> 
> Guenter
> 

Re: Buiild error in i915/xe

[Jani Nikula]

On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:
> On Sat, 18 Jan 2025 14:58:48 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
>
>> On 1/18/25 14:11, David Laight wrote:
>> > On Sat, 18 Jan 2025 13:21:39 -0800
>> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
>> >   
>> >> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:  
>> >>>
>> >>> No idea why the compiler would know that the values are invalid.  
>> >>
>> >> It's not that the compiler knows tat they are invalid, but I bet what
>> >> happens is in scale() (and possibly other places that do similar
>> >> checks), which does this:
>> >>
>> >>          WARN_ON(source_min > source_max);
>> >>          ...
>> >>          source_val = clamp(source_val, source_min, source_max);
>> >>
>> >> and the compiler notices that the ordering comparison in the first
>> >> WARN_ON() is the same as the one in clamp(), so it basically converts
>> >> the logic to
>> >>
>> >>          if (source_min > source_max) {
>> >>                  WARN(..);
>> >>                  /* Do the clamp() knowing that source_min > source_max */
>> >>                  source_val = clamp(source_val, source_min, source_max);
>> >>          } else {
>> >>                  /* Do the clamp knowing that source_min <= source_max */
>> >>                  source_val = clamp(source_val, source_min, source_max);
>> >>          }
>> >>
>> >> (obviously I dropped the other WARN_ON in the conversion, it wasn't
>> >> relevant for this case).
>> >>
>> >> And now that first clamp() case is done with source_min > source_max,
>> >> and it triggers that build error because that's invalid.
>> >>
>> >> So the condition is not statically true in the *source* code, but in
>> >> the "I have moved code around to combine tests" case it now *is*
>> >> statically true as far as the compiler is concerned.  
>> > 
>> > Well spotted :-)
>> > 
>> > One option would be to move the WARN_ON() below the clamp() and
>> > add an OPTIMISER_HIDE_VAR(source_max) between them.
>> > 
>> > Or do something more sensible than the WARN().
>> > Perhaps return target_min on any such errors?
>> >   
>> 
>> This helps:
>> 
>> -       WARN_ON(source_min > source_max);
>> -       WARN_ON(target_min > target_max);
>> -
>>          /* defensive */
>>          source_val = clamp(source_val, source_min, source_max);
>> 
>> +       WARN_ON(source_min > source_max);
>> +       WARN_ON(target_min > target_max);
>
> That is a 'quick fix' ...
>
> Much better would be to replace the WARN() with (say):
> 	if (target_min >= target_max)
> 		return target_min;
> 	if (source_min >= source_max)
> 		return target_min + (target_max - target_min)/2;
> So that the return values are actually in range (in as much as one is defined).
> Note that the >= cpmparisons also remove a divide by zero.

I want the loud and early warnings for clear bugs instead of
"gracefully" silencing the errors only to be found through debugging
user reports.

BR,
Jani.



-- 
Jani Nikula, Intel

Re: Buiild error in i915/xe

[David Laight]

On Mon, 20 Jan 2025 12:48:11 +0200
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:
> > On Sat, 18 Jan 2025 14:58:48 -0800
> > Guenter Roeck <linux@roeck-us.net> wrote:
> >  
> >> On 1/18/25 14:11, David Laight wrote:  
> >> > On Sat, 18 Jan 2025 13:21:39 -0800
> >> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >> >     
> >> >> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:    
> >> >>>
> >> >>> No idea why the compiler would know that the values are invalid.    
> >> >>
> >> >> It's not that the compiler knows tat they are invalid, but I bet what
> >> >> happens is in scale() (and possibly other places that do similar
> >> >> checks), which does this:
> >> >>
> >> >>          WARN_ON(source_min > source_max);
> >> >>          ...
> >> >>          source_val = clamp(source_val, source_min, source_max);
> >> >>
> >> >> and the compiler notices that the ordering comparison in the first
> >> >> WARN_ON() is the same as the one in clamp(), so it basically converts
> >> >> the logic to
> >> >>
> >> >>          if (source_min > source_max) {
> >> >>                  WARN(..);
> >> >>                  /* Do the clamp() knowing that source_min > source_max */
> >> >>                  source_val = clamp(source_val, source_min, source_max);
> >> >>          } else {
> >> >>                  /* Do the clamp knowing that source_min <= source_max */
> >> >>                  source_val = clamp(source_val, source_min, source_max);
> >> >>          }
> >> >>
> >> >> (obviously I dropped the other WARN_ON in the conversion, it wasn't
> >> >> relevant for this case).
> >> >>
> >> >> And now that first clamp() case is done with source_min > source_max,
> >> >> and it triggers that build error because that's invalid.
> >> >>
> >> >> So the condition is not statically true in the *source* code, but in
> >> >> the "I have moved code around to combine tests" case it now *is*
> >> >> statically true as far as the compiler is concerned.    
> >> > 
> >> > Well spotted :-)
> >> > 
> >> > One option would be to move the WARN_ON() below the clamp() and
> >> > add an OPTIMISER_HIDE_VAR(source_max) between them.
> >> > 
> >> > Or do something more sensible than the WARN().
> >> > Perhaps return target_min on any such errors?
> >> >     
> >> 
> >> This helps:
> >> 
> >> -       WARN_ON(source_min > source_max);
> >> -       WARN_ON(target_min > target_max);
> >> -
> >>          /* defensive */
> >>          source_val = clamp(source_val, source_min, source_max);
> >> 
> >> +       WARN_ON(source_min > source_max);
> >> +       WARN_ON(target_min > target_max);  
> >
> > That is a 'quick fix' ...
> >
> > Much better would be to replace the WARN() with (say):
> > 	if (target_min >= target_max)
> > 		return target_min;
> > 	if (source_min >= source_max)
> > 		return target_min + (target_max - target_min)/2;
> > So that the return values are actually in range (in as much as one is defined).
> > Note that the >= cpmparisons also remove a divide by zero.  
> 
> I want the loud and early warnings for clear bugs instead of
> "gracefully" silencing the errors only to be found through debugging
> user reports.

A user isn't going to notice a WARN() - not until you tell them to look for it.
In any case even if you output a message you really want to return a 'sane'
value, who knows what effect a very out of range value is going to have.

	David

Re: Buiild error in i915/xe

[Jani Nikula]

On Mon, 20 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:
> On Mon, 20 Jan 2025 12:48:11 +0200
> Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
>> On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:
>> > On Sat, 18 Jan 2025 14:58:48 -0800
>> > Guenter Roeck <linux@roeck-us.net> wrote:
>> >  
>> >> On 1/18/25 14:11, David Laight wrote:  
>> >> > On Sat, 18 Jan 2025 13:21:39 -0800
>> >> > Linus Torvalds <torvalds@linux-foundation.org> wrote:
>> >> >     
>> >> >> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:    
>> >> >>>
>> >> >>> No idea why the compiler would know that the values are invalid.    
>> >> >>
>> >> >> It's not that the compiler knows tat they are invalid, but I bet what
>> >> >> happens is in scale() (and possibly other places that do similar
>> >> >> checks), which does this:
>> >> >>
>> >> >>          WARN_ON(source_min > source_max);
>> >> >>          ...
>> >> >>          source_val = clamp(source_val, source_min, source_max);
>> >> >>
>> >> >> and the compiler notices that the ordering comparison in the first
>> >> >> WARN_ON() is the same as the one in clamp(), so it basically converts
>> >> >> the logic to
>> >> >>
>> >> >>          if (source_min > source_max) {
>> >> >>                  WARN(..);
>> >> >>                  /* Do the clamp() knowing that source_min > source_max */
>> >> >>                  source_val = clamp(source_val, source_min, source_max);
>> >> >>          } else {
>> >> >>                  /* Do the clamp knowing that source_min <= source_max */
>> >> >>                  source_val = clamp(source_val, source_min, source_max);
>> >> >>          }
>> >> >>
>> >> >> (obviously I dropped the other WARN_ON in the conversion, it wasn't
>> >> >> relevant for this case).
>> >> >>
>> >> >> And now that first clamp() case is done with source_min > source_max,
>> >> >> and it triggers that build error because that's invalid.
>> >> >>
>> >> >> So the condition is not statically true in the *source* code, but in
>> >> >> the "I have moved code around to combine tests" case it now *is*
>> >> >> statically true as far as the compiler is concerned.    
>> >> > 
>> >> > Well spotted :-)
>> >> > 
>> >> > One option would be to move the WARN_ON() below the clamp() and
>> >> > add an OPTIMISER_HIDE_VAR(source_max) between them.
>> >> > 
>> >> > Or do something more sensible than the WARN().
>> >> > Perhaps return target_min on any such errors?
>> >> >     
>> >> 
>> >> This helps:
>> >> 
>> >> -       WARN_ON(source_min > source_max);
>> >> -       WARN_ON(target_min > target_max);
>> >> -
>> >>          /* defensive */
>> >>          source_val = clamp(source_val, source_min, source_max);
>> >> 
>> >> +       WARN_ON(source_min > source_max);
>> >> +       WARN_ON(target_min > target_max);  
>> >
>> > That is a 'quick fix' ...
>> >
>> > Much better would be to replace the WARN() with (say):
>> > 	if (target_min >= target_max)
>> > 		return target_min;
>> > 	if (source_min >= source_max)
>> > 		return target_min + (target_max - target_min)/2;
>> > So that the return values are actually in range (in as much as one is defined).
>> > Note that the >= cpmparisons also remove a divide by zero.  
>> 
>> I want the loud and early warnings for clear bugs instead of
>> "gracefully" silencing the errors only to be found through debugging
>> user reports.
>
> A user isn't going to notice a WARN() - not until you tell them to look for it.
> In any case even if you output a message you really want to return a 'sane'
> value, who knows what effect a very out of range value is going to have.

The point is, we'll catch the WARN in CI before it goes out to users.

BR,
Jani.

>
> 	David
>
>

-- 
Jani Nikula, Intel

Re: Buiild error in i915/xe

[Guenter Roeck]

On 1/20/25 03:21, Jani Nikula wrote:
> On Mon, 20 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:
>> On Mon, 20 Jan 2025 12:48:11 +0200
>> Jani Nikula <jani.nikula@linux.intel.com> wrote:
>>
>>> On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:
>>>> On Sat, 18 Jan 2025 14:58:48 -0800
>>>> Guenter Roeck <linux@roeck-us.net> wrote:
>>>>   
>>>>> On 1/18/25 14:11, David Laight wrote:
>>>>>> On Sat, 18 Jan 2025 13:21:39 -0800
>>>>>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>>>>>      
>>>>>>> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>>>
>>>>>>>> No idea why the compiler would know that the values are invalid.
>>>>>>>
>>>>>>> It's not that the compiler knows tat they are invalid, but I bet what
>>>>>>> happens is in scale() (and possibly other places that do similar
>>>>>>> checks), which does this:
>>>>>>>
>>>>>>>           WARN_ON(source_min > source_max);
>>>>>>>           ...
>>>>>>>           source_val = clamp(source_val, source_min, source_max);
>>>>>>>
>>>>>>> and the compiler notices that the ordering comparison in the first
>>>>>>> WARN_ON() is the same as the one in clamp(), so it basically converts
>>>>>>> the logic to
>>>>>>>
>>>>>>>           if (source_min > source_max) {
>>>>>>>                   WARN(..);
>>>>>>>                   /* Do the clamp() knowing that source_min > source_max */
>>>>>>>                   source_val = clamp(source_val, source_min, source_max);
>>>>>>>           } else {
>>>>>>>                   /* Do the clamp knowing that source_min <= source_max */
>>>>>>>                   source_val = clamp(source_val, source_min, source_max);
>>>>>>>           }
>>>>>>>
>>>>>>> (obviously I dropped the other WARN_ON in the conversion, it wasn't
>>>>>>> relevant for this case).
>>>>>>>
>>>>>>> And now that first clamp() case is done with source_min > source_max,
>>>>>>> and it triggers that build error because that's invalid.
>>>>>>>
>>>>>>> So the condition is not statically true in the *source* code, but in
>>>>>>> the "I have moved code around to combine tests" case it now *is*
>>>>>>> statically true as far as the compiler is concerned.
>>>>>>
>>>>>> Well spotted :-)
>>>>>>
>>>>>> One option would be to move the WARN_ON() below the clamp() and
>>>>>> add an OPTIMISER_HIDE_VAR(source_max) between them.
>>>>>>
>>>>>> Or do something more sensible than the WARN().
>>>>>> Perhaps return target_min on any such errors?
>>>>>>      
>>>>>
>>>>> This helps:
>>>>>
>>>>> -       WARN_ON(source_min > source_max);
>>>>> -       WARN_ON(target_min > target_max);
>>>>> -
>>>>>           /* defensive */
>>>>>           source_val = clamp(source_val, source_min, source_max);
>>>>>
>>>>> +       WARN_ON(source_min > source_max);
>>>>> +       WARN_ON(target_min > target_max);
>>>>
>>>> That is a 'quick fix' ...
>>>>
>>>> Much better would be to replace the WARN() with (say):
>>>> 	if (target_min >= target_max)
>>>> 		return target_min;
>>>> 	if (source_min >= source_max)
>>>> 		return target_min + (target_max - target_min)/2;
>>>> So that the return values are actually in range (in as much as one is defined).
>>>> Note that the >= cpmparisons also remove a divide by zero.
>>>
>>> I want the loud and early warnings for clear bugs instead of
>>> "gracefully" silencing the errors only to be found through debugging
>>> user reports.
>>
>> A user isn't going to notice a WARN() - not until you tell them to look for it.
>> In any case even if you output a message you really want to return a 'sane'
>> value, who knows what effect a very out of range value is going to have.
> 
> The point is, we'll catch the WARN in CI before it goes out to users.
> 

It isn't going to catch the divide by 0 error, and it obviously doesn't
catch the build problem on parisc with gcc 13.x because the CI isn't
testing it.

How about disabling DRM_XE on architectures where it isn't supported,
matching DRM_I915 ?

Thanks,
Guenter

Re: Buiild error in i915/xe

[David Laight]

On Mon, 20 Jan 2025 06:15:30 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> On 1/20/25 03:21, Jani Nikula wrote:
> > On Mon, 20 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:  
> >> On Mon, 20 Jan 2025 12:48:11 +0200
> >> Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >>  
> >>> On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:  
> >>>> On Sat, 18 Jan 2025 14:58:48 -0800
> >>>> Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>     
> >>>>> On 1/18/25 14:11, David Laight wrote:  
> >>>>>> On Sat, 18 Jan 2025 13:21:39 -0800
> >>>>>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >>>>>>        
> >>>>>>> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:  
> >>>>>>>>
> >>>>>>>> No idea why the compiler would know that the values are invalid.  
> >>>>>>>
> >>>>>>> It's not that the compiler knows tat they are invalid, but I bet what
> >>>>>>> happens is in scale() (and possibly other places that do similar
> >>>>>>> checks), which does this:
> >>>>>>>
> >>>>>>>           WARN_ON(source_min > source_max);
> >>>>>>>           ...
> >>>>>>>           source_val = clamp(source_val, source_min, source_max);
> >>>>>>>
> >>>>>>> and the compiler notices that the ordering comparison in the first
> >>>>>>> WARN_ON() is the same as the one in clamp(), so it basically converts
> >>>>>>> the logic to
> >>>>>>>
> >>>>>>>           if (source_min > source_max) {
> >>>>>>>                   WARN(..);
> >>>>>>>                   /* Do the clamp() knowing that source_min > source_max */
> >>>>>>>                   source_val = clamp(source_val, source_min, source_max);
> >>>>>>>           } else {
> >>>>>>>                   /* Do the clamp knowing that source_min <= source_max */
> >>>>>>>                   source_val = clamp(source_val, source_min, source_max);
> >>>>>>>           }
> >>>>>>>
> >>>>>>> (obviously I dropped the other WARN_ON in the conversion, it wasn't
> >>>>>>> relevant for this case).
> >>>>>>>
> >>>>>>> And now that first clamp() case is done with source_min > source_max,
> >>>>>>> and it triggers that build error because that's invalid.
> >>>>>>>
> >>>>>>> So the condition is not statically true in the *source* code, but in
> >>>>>>> the "I have moved code around to combine tests" case it now *is*
> >>>>>>> statically true as far as the compiler is concerned.  
> >>>>>>
> >>>>>> Well spotted :-)
> >>>>>>
> >>>>>> One option would be to move the WARN_ON() below the clamp() and
> >>>>>> add an OPTIMISER_HIDE_VAR(source_max) between them.
> >>>>>>
> >>>>>> Or do something more sensible than the WARN().
> >>>>>> Perhaps return target_min on any such errors?
> >>>>>>        
> >>>>>
> >>>>> This helps:
> >>>>>
> >>>>> -       WARN_ON(source_min > source_max);
> >>>>> -       WARN_ON(target_min > target_max);
> >>>>> -
> >>>>>           /* defensive */
> >>>>>           source_val = clamp(source_val, source_min, source_max);
> >>>>>
> >>>>> +       WARN_ON(source_min > source_max);
> >>>>> +       WARN_ON(target_min > target_max);  
> >>>>
> >>>> That is a 'quick fix' ...
> >>>>
> >>>> Much better would be to replace the WARN() with (say):
> >>>> 	if (target_min >= target_max)
> >>>> 		return target_min;
> >>>> 	if (source_min >= source_max)
> >>>> 		return target_min + (target_max - target_min)/2;
> >>>> So that the return values are actually in range (in as much as one is defined).
> >>>> Note that the >= cpmparisons also remove a divide by zero.  
> >>>
> >>> I want the loud and early warnings for clear bugs instead of
> >>> "gracefully" silencing the errors only to be found through debugging
> >>> user reports.  
> >>
> >> A user isn't going to notice a WARN() - not until you tell them to look for it.
> >> In any case even if you output a message you really want to return a 'sane'
> >> value, who knows what effect a very out of range value is going to have.  
> > 
> > The point is, we'll catch the WARN in CI before it goes out to users.
> >   
> 
> It isn't going to catch the divide by 0 error, and it obviously doesn't
> catch the build problem on parisc with gcc 13.x because the CI isn't
> testing it.
> 
> How about disabling DRM_XE on architectures where it isn't supported,
> matching DRM_I915 ?

That'll just bite back later.
As Linus spotted the compiler is just 'optimising' some code paths.
It could happen for any architecture including x64.
The repeated tests are basically slightly odd, although you might only
expect them to be present in debug builds.

An alternative would be to replace the clamp() with:
	if (source_val <= source_min)
		return target_min;
	if (source_val >= source_max)
		return target_max;

	David

> 
> Thanks,
> Guenter
> 

Re: Buiild error in i915/xe

[Andy Shevchenko]

On Mon, Jan 20, 2025 at 06:41:43PM +0000, David Laight wrote:
> On Mon, 20 Jan 2025 06:15:30 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
> > On 1/20/25 03:21, Jani Nikula wrote:
> > > On Mon, 20 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:  
> > >> On Mon, 20 Jan 2025 12:48:11 +0200
> > >> Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > >>> On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote:  
> > >>>> On Sat, 18 Jan 2025 14:58:48 -0800
> > >>>> Guenter Roeck <linux@roeck-us.net> wrote:
> > >>>>> On 1/18/25 14:11, David Laight wrote:  
> > >>>>>> On Sat, 18 Jan 2025 13:21:39 -0800
> > >>>>>> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > >>>>>>> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote:  
> > >>>>>>>>
> > >>>>>>>> No idea why the compiler would know that the values are invalid.  
> > >>>>>>>
> > >>>>>>> It's not that the compiler knows tat they are invalid, but I bet what
> > >>>>>>> happens is in scale() (and possibly other places that do similar
> > >>>>>>> checks), which does this:
> > >>>>>>>
> > >>>>>>>           WARN_ON(source_min > source_max);
> > >>>>>>>           ...
> > >>>>>>>           source_val = clamp(source_val, source_min, source_max);
> > >>>>>>>
> > >>>>>>> and the compiler notices that the ordering comparison in the first
> > >>>>>>> WARN_ON() is the same as the one in clamp(), so it basically converts
> > >>>>>>> the logic to
> > >>>>>>>
> > >>>>>>>           if (source_min > source_max) {
> > >>>>>>>                   WARN(..);
> > >>>>>>>                   /* Do the clamp() knowing that source_min > source_max */
> > >>>>>>>                   source_val = clamp(source_val, source_min, source_max);
> > >>>>>>>           } else {
> > >>>>>>>                   /* Do the clamp knowing that source_min <= source_max */
> > >>>>>>>                   source_val = clamp(source_val, source_min, source_max);
> > >>>>>>>           }
> > >>>>>>>
> > >>>>>>> (obviously I dropped the other WARN_ON in the conversion, it wasn't
> > >>>>>>> relevant for this case).
> > >>>>>>>
> > >>>>>>> And now that first clamp() case is done with source_min > source_max,
> > >>>>>>> and it triggers that build error because that's invalid.
> > >>>>>>>
> > >>>>>>> So the condition is not statically true in the *source* code, but in
> > >>>>>>> the "I have moved code around to combine tests" case it now *is*
> > >>>>>>> statically true as far as the compiler is concerned.  
> > >>>>>>
> > >>>>>> Well spotted :-)
> > >>>>>>
> > >>>>>> One option would be to move the WARN_ON() below the clamp() and
> > >>>>>> add an OPTIMISER_HIDE_VAR(source_max) between them.
> > >>>>>>
> > >>>>>> Or do something more sensible than the WARN().
> > >>>>>> Perhaps return target_min on any such errors?
> > >>>>>>        
> > >>>>>
> > >>>>> This helps:
> > >>>>>
> > >>>>> -       WARN_ON(source_min > source_max);
> > >>>>> -       WARN_ON(target_min > target_max);
> > >>>>> -
> > >>>>>           /* defensive */
> > >>>>>           source_val = clamp(source_val, source_min, source_max);
> > >>>>>
> > >>>>> +       WARN_ON(source_min > source_max);
> > >>>>> +       WARN_ON(target_min > target_max);  
> > >>>>
> > >>>> That is a 'quick fix' ...
> > >>>>
> > >>>> Much better would be to replace the WARN() with (say):
> > >>>> 	if (target_min >= target_max)
> > >>>> 		return target_min;
> > >>>> 	if (source_min >= source_max)
> > >>>> 		return target_min + (target_max - target_min)/2;
> > >>>> So that the return values are actually in range (in as much as one is defined).
> > >>>> Note that the >= cpmparisons also remove a divide by zero.  
> > >>>
> > >>> I want the loud and early warnings for clear bugs instead of
> > >>> "gracefully" silencing the errors only to be found through debugging
> > >>> user reports.  
> > >>
> > >> A user isn't going to notice a WARN() - not until you tell them to look for it.
> > >> In any case even if you output a message you really want to return a 'sane'
> > >> value, who knows what effect a very out of range value is going to have.  
> > > 
> > > The point is, we'll catch the WARN in CI before it goes out to users.
> > 
> > It isn't going to catch the divide by 0 error, and it obviously doesn't
> > catch the build problem on parisc with gcc 13.x because the CI isn't
> > testing it.
> > 
> > How about disabling DRM_XE on architectures where it isn't supported,
> > matching DRM_I915 ?
> 
> That'll just bite back later.
> As Linus spotted the compiler is just 'optimising' some code paths.
> It could happen for any architecture including x64.
> The repeated tests are basically slightly odd, although you might only
> expect them to be present in debug builds.
> 
> An alternative would be to replace the clamp() with:
> 	if (source_val <= source_min)
> 		return target_min;
> 	if (source_val >= source_max)
> 		return target_max;

Excuse me if I am missing something, but clamp() has a warning inside it, correct?
Why do wee need an additional warning on top of that?

P.S. However, I agree that ideally clamp() should work independently on the
caller to use WARN*() or other similar stuff.

-- 
With Best Regards,
Andy Shevchenko

Re: Buiild error in i915/xe

[Linus Torvalds]

On Mon, 20 Jan 2025 at 10:55, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Excuse me if I am missing something, but clamp() has a warning inside it, correct?
> Why do we need an additional warning on top of that?

Note: the warning in clamp() only finds compile-time obvious wrong uses.

It's really meant to notice the trivial case where you clam with
constants and just got the order wrong, so you do something silly like

    res = clamp(in, 15, 1);

but it does also end up catching slightly more complex things where
the compiler can figure out the range of the clamping.

The build problem then comes from the compiler doing various *other*
code movem,ent and optimization too, and - like in this case - finds
an error path where the clamping is done "wrong".

I think the real issue in the i915 driver is that it does that
WARN_ON(), but then it just happily continues anyway.

So if the i915 driver instead did

        if (WARN_ON(..)) return invalid value;

none of this would ever have happened.

                Linus

Re: Buiild error in i915/xe

[Guenter Roeck]

On 1/20/25 11:14, Linus Torvalds wrote:
> On Mon, 20 Jan 2025 at 10:55, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>>
>> Excuse me if I am missing something, but clamp() has a warning inside it, correct?
>> Why do we need an additional warning on top of that?
> 
> Note: the warning in clamp() only finds compile-time obvious wrong uses.
> 
> It's really meant to notice the trivial case where you clam with
> constants and just got the order wrong, so you do something silly like
> 
>      res = clamp(in, 15, 1);
> 
> but it does also end up catching slightly more complex things where
> the compiler can figure out the range of the clamping.
> 
> The build problem then comes from the compiler doing various *other*
> code movem,ent and optimization too, and - like in this case - finds
> an error path where the clamping is done "wrong".
> 
> I think the real issue in the i915 driver is that it does that
> WARN_ON(), but then it just happily continues anyway.
> 
> So if the i915 driver instead did
> 
>          if (WARN_ON(..)) return invalid value;
> 
> none of this would ever have happened.
> 

I'll take a stab and send a patch combining your and David's suggestions.

Guenter