diff mbox series

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

Message ID 8f69f4deac014f558bab186444bac2e8@AcuMS.aculab.com (mailing list archive)
State New
Headers show
Series minmax.h: Cleanups and minor optimisations | expand

Commit Message

David Laight Nov. 18, 2024, 7:15 p.m. UTC
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(-)

Comments

kernel test robot Nov. 22, 2024, 8:20 p.m. UTC | #1
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
kernel test robot Nov. 28, 2024, 3:05 p.m. UTC | #2
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
David Laight Nov. 28, 2024, 3:52 p.m. UTC | #3
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)
diff mbox series

Patch

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().