[PATCH v7 0/4] Introduce the for_each_set_clump macro

[PATCH v7 0/4] Introduce the for_each_set_clump macro

[Syed Nayyar Waris]

Hello Linus,

Since this patchset primarily affects GPIO drivers, would you like
to pick it up through your GPIO tree?

This patchset introduces a new generic version of for_each_set_clump. 
The previous version of for_each_set_clump8 used a fixed size 8-bit
clump, but the new generic version can work with clump of any size but
less than or equal to BITS_PER_LONG. The patchset utilizes the new macro 
in several GPIO drivers.

The earlier 8-bit for_each_set_clump8 facilitated a
for-loop syntax that iterates over a memory region entire groups of set
bits at a time.

For example, suppose you would like to iterate over a 32-bit integer 8
bits at a time, skipping over 8-bit groups with no set bit, where
XXXXXXXX represents the current 8-bit group:

    Example:        10111110 00000000 11111111 00110011
    First loop:     10111110 00000000 11111111 XXXXXXXX
    Second loop:    10111110 00000000 XXXXXXXX 00110011
    Third loop:     XXXXXXXX 00000000 11111111 00110011

Each iteration of the loop returns the next 8-bit group that has at
least one set bit.

But with the new for_each_set_clump the clump size can be different from 8 bits.
Moreover, the clump can be split at word boundary in situations where word 
size is not multiple of clump size. Following are examples showing the working 
of new macro for clump sizes of 24 bits and 6 bits.

Example 1:
clump size: 24 bits, Number of clumps (or ports): 10
bitmap stores the bit information from where successive clumps are retrieved.

     /* bitmap memory region */
        0x00aa0000ff000000;  /* Most significant bits */
        0xaaaaaa0000ff0000;
        0x000000aa000000aa;
        0xbbbbabcdeffedcba;  /* Least significant bits */

Different iterations of for_each_set_clump:-
'offset' is the bit position and 'clump' is the 24 bit clump from the
above bitmap.
Iteration first:        offset: 0 clump: 0xfedcba
Iteration second:       offset: 24 clump: 0xabcdef
Iteration third:        offset: 48 clump: 0xaabbbb
Iteration fourth:       offset: 96 clump: 0xaa
Iteration fifth:        offset: 144 clump: 0xff
Iteration sixth:        offset: 168 clump: 0xaaaaaa
Iteration seventh:      offset: 216 clump: 0xff
Loop breaks because in the end the remaining bits (0x00aa) size was less
than clump size of 24 bits.

In above example it can be seen that in iteration third, the 24 bit clump
that was retrieved was split between bitmap[0] and bitmap[1]. This example 
also shows that 24 bit zeroes if present in between, were skipped (preserving
the previous for_each_set_macro8 behaviour). 

Example 2:
clump size = 6 bits, Number of clumps (or ports) = 3.

     /* bitmap memory region */
        0x00aa0000ff000000;  /* Most significant bits */
        0xaaaaaa0000ff0000;
        0x0f00000000000000;
        0x0000000000000ac0;  /* Least significant bits */

Different iterations of for_each_set_clump:
'offset' is the bit position and 'clump' is the 6 bit clump from the
above bitmap.
Iteration first:        offset: 6 clump: 0x2b
Loop breaks because 6 * 3 = 18 bits traversed in bitmap.
Here 6 * 3 is clump size * no. of clumps.

Changes in v7:
 - [Patch 2/4]: Minor changes: Use macro 'DECLARE_BITMAP()' and split 'struct'
   definition and test data.

Changes in v6:
 - [Patch 2/4]: Make 'for loop' inside test_for_each_set_clump more
   succinct.

Changes in v5:
 - [Patch 4/4]: Minor change: Hardcode value for better code readability.

Changes in v4:
 - [Patch 2/4]: Use 'for' loop in test function of for_each_set_clump.
 - [Patch 3/4]: Minor change: Inline value for better code readability.
 - [Patch 4/4]: Minor change: Inline value for better code readability.

Changes in v3:
 - [Patch 3/4]: Change datatype of some variables from u64 to unsigned long
   in function thunderx_gpio_set_multiple.

CHanges in v2:
 - [Patch 2/4]: Unify different tests for 'for_each_set_clump'. Pass test data as
   function parameters.
 - [Patch 2/4]: Remove unnecessary bitmap_zero calls.

Syed Nayyar Waris (4):
  bitops: Introduce the the for_each_set_clump macro
  lib/test_bitmap.c: Add for_each_set_clump test cases
  gpio: thunderx: Utilize for_each_set_clump macro
  gpio: xilinx: Utilize for_each_set_clump macro

 drivers/gpio/gpio-thunderx.c      |  11 ++-
 drivers/gpio/gpio-xilinx.c        |  62 ++++++-------
 include/asm-generic/bitops/find.h |  19 ++++
 include/linux/bitmap.h            |  61 +++++++++++++
 include/linux/bitops.h            |  13 +++
 lib/find_bit.c                    |  14 +++
 lib/test_bitmap.c                 | 144 ++++++++++++++++++++++++++++++
 7 files changed, 290 insertions(+), 34 deletions(-)


base-commit: b9bbe6ed63b2b9f2c9ee5cbd0f2c946a2723f4ce
-- 
2.26.2

[PATCH v7 0/4] Introduce the for_each_set_clump macro

[Syed Nayyar Waris]

Hello Linus,

Since this patchset primarily affects GPIO drivers, would you like
to pick it up through your GPIO tree?

This patchset introduces a new generic version of for_each_set_clump. 
The previous version of for_each_set_clump8 used a fixed size 8-bit
clump, but the new generic version can work with clump of any size but
less than or equal to BITS_PER_LONG. The patchset utilizes the new macro 
in several GPIO drivers.

The earlier 8-bit for_each_set_clump8 facilitated a
for-loop syntax that iterates over a memory region entire groups of set
bits at a time.

For example, suppose you would like to iterate over a 32-bit integer 8
bits at a time, skipping over 8-bit groups with no set bit, where
XXXXXXXX represents the current 8-bit group:

    Example:        10111110 00000000 11111111 00110011
    First loop:     10111110 00000000 11111111 XXXXXXXX
    Second loop:    10111110 00000000 XXXXXXXX 00110011
    Third loop:     XXXXXXXX 00000000 11111111 00110011

Each iteration of the loop returns the next 8-bit group that has at
least one set bit.

But with the new for_each_set_clump the clump size can be different from 8 bits.
Moreover, the clump can be split at word boundary in situations where word 
size is not multiple of clump size. Following are examples showing the working 
of new macro for clump sizes of 24 bits and 6 bits.

Example 1:
clump size: 24 bits, Number of clumps (or ports): 10
bitmap stores the bit information from where successive clumps are retrieved.

     /* bitmap memory region */
        0x00aa0000ff000000;  /* Most significant bits */
        0xaaaaaa0000ff0000;
        0x000000aa000000aa;
        0xbbbbabcdeffedcba;  /* Least significant bits */

Different iterations of for_each_set_clump:-
'offset' is the bit position and 'clump' is the 24 bit clump from the
above bitmap.
Iteration first:        offset: 0 clump: 0xfedcba
Iteration second:       offset: 24 clump: 0xabcdef
Iteration third:        offset: 48 clump: 0xaabbbb
Iteration fourth:       offset: 96 clump: 0xaa
Iteration fifth:        offset: 144 clump: 0xff
Iteration sixth:        offset: 168 clump: 0xaaaaaa
Iteration seventh:      offset: 216 clump: 0xff
Loop breaks because in the end the remaining bits (0x00aa) size was less
than clump size of 24 bits.

In above example it can be seen that in iteration third, the 24 bit clump
that was retrieved was split between bitmap[0] and bitmap[1]. This example 
also shows that 24 bit zeroes if present in between, were skipped (preserving
the previous for_each_set_macro8 behaviour). 

Example 2:
clump size = 6 bits, Number of clumps (or ports) = 3.

     /* bitmap memory region */
        0x00aa0000ff000000;  /* Most significant bits */
        0xaaaaaa0000ff0000;
        0x0f00000000000000;
        0x0000000000000ac0;  /* Least significant bits */

Different iterations of for_each_set_clump:
'offset' is the bit position and 'clump' is the 6 bit clump from the
above bitmap.
Iteration first:        offset: 6 clump: 0x2b
Loop breaks because 6 * 3 = 18 bits traversed in bitmap.
Here 6 * 3 is clump size * no. of clumps.

Changes in v7:
 - [Patch 2/4]: Minor changes: Use macro 'DECLARE_BITMAP()' and split 'struct'
   definition and test data.

Changes in v6:
 - [Patch 2/4]: Make 'for loop' inside test_for_each_set_clump more
   succinct.

Changes in v5:
 - [Patch 4/4]: Minor change: Hardcode value for better code readability.

Changes in v4:
 - [Patch 2/4]: Use 'for' loop in test function of for_each_set_clump.
 - [Patch 3/4]: Minor change: Inline value for better code readability.
 - [Patch 4/4]: Minor change: Inline value for better code readability.

Changes in v3:
 - [Patch 3/4]: Change datatype of some variables from u64 to unsigned long
   in function thunderx_gpio_set_multiple.

CHanges in v2:
 - [Patch 2/4]: Unify different tests for 'for_each_set_clump'. Pass test data as
   function parameters.
 - [Patch 2/4]: Remove unnecessary bitmap_zero calls.

Syed Nayyar Waris (4):
  bitops: Introduce the the for_each_set_clump macro
  lib/test_bitmap.c: Add for_each_set_clump test cases
  gpio: thunderx: Utilize for_each_set_clump macro
  gpio: xilinx: Utilize for_each_set_clump macro

 drivers/gpio/gpio-thunderx.c      |  11 ++-
 drivers/gpio/gpio-xilinx.c        |  62 ++++++-------
 include/asm-generic/bitops/find.h |  19 ++++
 include/linux/bitmap.h            |  61 +++++++++++++
 include/linux/bitops.h            |  13 +++
 lib/find_bit.c                    |  14 +++
 lib/test_bitmap.c                 | 144 ++++++++++++++++++++++++++++++
 7 files changed, 290 insertions(+), 34 deletions(-)


base-commit: b9bbe6ed63b2b9f2c9ee5cbd0f2c946a2723f4ce
-- 
2.26.2

[PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Syed Nayyar Waris]

This macro iterates for each group of bits (clump) with set bits,
within a bitmap memory region. For each iteration, "start" is set to
the bit offset of the found clump, while the respective clump value is
stored to the location pointed by "clump". Additionally, the
bitmap_get_value and bitmap_set_value functions are introduced to
respectively get and set a value of n-bits in a bitmap memory region.
The n-bits can have any size less than or equal to BITS_PER_LONG.
Moreover, during setting value of n-bit in bitmap, if a situation arise
that the width of next n-bit is exceeding the word boundary, then it
will divide itself such that some portion of it is stored in that word,
while the remaining portion is stored in the next higher word. Similar
situation occurs while retrieving value of n-bits from bitmap.

Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Syed Nayyar Waris <syednwaris@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
Changes in v7:
 - No change.

Changes in v6:
 - No change.

Changes in v5:
 - No change.

Changes in v4:
 - No change.

Changes in v3:
 - No change.

Changes in v2:
 - No change.

 include/asm-generic/bitops/find.h | 19 ++++++++++
 include/linux/bitmap.h            | 61 +++++++++++++++++++++++++++++++
 include/linux/bitops.h            | 13 +++++++
 lib/find_bit.c                    | 14 +++++++
 4 files changed, 107 insertions(+)

diff --git a/include/asm-generic/bitops/find.h b/include/asm-generic/bitops/find.h
index 9fdf21302fdf..4e6600759455 100644
--- a/include/asm-generic/bitops/find.h
+++ b/include/asm-generic/bitops/find.h
@@ -97,4 +97,23 @@ extern unsigned long find_next_clump8(unsigned long *clump,
 #define find_first_clump8(clump, bits, size) \
 	find_next_clump8((clump), (bits), (size), 0)
 
+/**
+ * find_next_clump - find next clump with set bits in a memory region
+ * @clump: location to store copy of found clump
+ * @addr: address to base the search on
+ * @size: bitmap size in number of bits
+ * @offset: bit offset at which to start searching
+ * @clump_size: clump size in bits
+ *
+ * Returns the bit offset for the next set clump; the found clump value is
+ * copied to the location pointed by @clump. If no bits are set, returns @size.
+ */
+extern unsigned long find_next_clump(unsigned long *clump,
+				      const unsigned long *addr,
+				      unsigned long size, unsigned long offset,
+				      unsigned long clump_size);
+
+#define find_first_clump(clump, bits, size, clump_size) \
+	find_next_clump((clump), (bits), (size), 0, (clump_size))
+
 #endif /*_ASM_GENERIC_BITOPS_FIND_H_ */
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 99058eb81042..7ab2c65fc964 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -75,7 +75,11 @@
  *  bitmap_from_arr32(dst, buf, nbits)          Copy nbits from u32[] buf to dst
  *  bitmap_to_arr32(buf, src, nbits)            Copy nbits from buf to u32[] dst
  *  bitmap_get_value8(map, start)               Get 8bit value from map at start
+ *  bitmap_get_value(map, start, nbits)		Get bit value of size
+ *						'nbits' from map at start
  *  bitmap_set_value8(map, value, start)        Set 8bit value to map at start
+ *  bitmap_set_value(map, value, start, nbits)	Set bit value of size 'nbits'
+ *						of map at start
  *
  * Note, bitmap_zero() and bitmap_fill() operate over the region of
  * unsigned longs, that is, bits behind bitmap till the unsigned long
@@ -563,6 +567,34 @@ static inline unsigned long bitmap_get_value8(const unsigned long *map,
 	return (map[index] >> offset) & 0xFF;
 }
 
+/**
+ * bitmap_get_value - get a value of n-bits from the memory region
+ * @map: address to the bitmap memory region
+ * @start: bit offset of the n-bit value
+ * @nbits: size of value in bits
+ *
+ * Returns value of nbits located at the @start bit offset within the @map
+ * memory region.
+ */
+static inline unsigned long bitmap_get_value(const unsigned long *map,
+					      unsigned long start,
+					      unsigned long nbits)
+{
+	const size_t index = BIT_WORD(start);
+	const unsigned long offset = start % BITS_PER_LONG;
+	const unsigned long ceiling = roundup(start + 1, BITS_PER_LONG);
+	const unsigned long space = ceiling - start;
+	unsigned long value_low, value_high;
+
+	if (space >= nbits)
+		return (map[index] >> offset) & GENMASK(nbits - 1, 0);
+	else {
+		value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
+		value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
+		return (value_low >> offset) | (value_high << space);
+	}
+}
+
 /**
  * bitmap_set_value8 - set an 8-bit value within a memory region
  * @map: address to the bitmap memory region
@@ -579,6 +611,35 @@ static inline void bitmap_set_value8(unsigned long *map, unsigned long value,
 	map[index] |= value << offset;
 }
 
+/**
+ * bitmap_set_value - set n-bit value within a memory region
+ * @map: address to the bitmap memory region
+ * @value: value of nbits
+ * @start: bit offset of the n-bit value
+ * @nbits: size of value in bits
+ */
+static inline void bitmap_set_value(unsigned long *map,
+				    unsigned long value,
+				    unsigned long start, unsigned long nbits)
+{
+	const size_t index = BIT_WORD(start);
+	const unsigned long offset = start % BITS_PER_LONG;
+	const unsigned long ceiling = roundup(start + 1, BITS_PER_LONG);
+	const unsigned long space = ceiling - start;
+
+	value &= GENMASK(nbits - 1, 0);
+
+	if (space >= nbits) {
+		map[index] &= ~(GENMASK(nbits + offset - 1, offset));
+		map[index] |= value << offset;
+	} else {
+		map[index] &= ~BITMAP_FIRST_WORD_MASK(start);
+		map[index] |= value << offset;
+		map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
+		map[index + 1] |= (value >> space);
+	}
+}
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __LINUX_BITMAP_H */
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 9acf654f0b19..41c2d9ce63e7 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -62,6 +62,19 @@ extern unsigned long __sw_hweight64(__u64 w);
 	     (start) < (size); \
 	     (start) = find_next_clump8(&(clump), (bits), (size), (start) + 8))
 
+/**
+ * for_each_set_clump - iterate over bitmap for each clump with set bits
+ * @start: bit offset to start search and to store the current iteration offset
+ * @clump: location to store copy of current 8-bit clump
+ * @bits: bitmap address to base the search on
+ * @size: bitmap size in number of bits
+ * @clump_size: clump size in bits
+ */
+#define for_each_set_clump(start, clump, bits, size, clump_size) \
+	for ((start) = find_first_clump(&(clump), (bits), (size), (clump_size)); \
+	     (start) < (size); \
+	     (start) = find_next_clump(&(clump), (bits), (size), (start) + (clump_size), (clump_size)))
+
 static inline int get_bitmask_order(unsigned int count)
 {
 	int order;
diff --git a/lib/find_bit.c b/lib/find_bit.c
index 49f875f1baf7..1341bd39b32a 100644
--- a/lib/find_bit.c
+++ b/lib/find_bit.c
@@ -190,3 +190,17 @@ unsigned long find_next_clump8(unsigned long *clump, const unsigned long *addr,
 	return offset;
 }
 EXPORT_SYMBOL(find_next_clump8);
+
+unsigned long find_next_clump(unsigned long *clump, const unsigned long *addr,
+			       unsigned long size, unsigned long offset,
+			       unsigned long clump_size)
+{
+	offset = find_next_bit(addr, size, offset);
+	if (offset == size)
+		return size;
+
+	offset = rounddown(offset, clump_size);
+	*clump = bitmap_get_value(addr, offset, clump_size);
+	return offset;
+}
+EXPORT_SYMBOL(find_next_clump);
-- 
2.26.2

[PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Syed Nayyar Waris]

This macro iterates for each group of bits (clump) with set bits,
within a bitmap memory region. For each iteration, "start" is set to
the bit offset of the found clump, while the respective clump value is
stored to the location pointed by "clump". Additionally, the
bitmap_get_value and bitmap_set_value functions are introduced to
respectively get and set a value of n-bits in a bitmap memory region.
The n-bits can have any size less than or equal to BITS_PER_LONG.
Moreover, during setting value of n-bit in bitmap, if a situation arise
that the width of next n-bit is exceeding the word boundary, then it
will divide itself such that some portion of it is stored in that word,
while the remaining portion is stored in the next higher word. Similar
situation occurs while retrieving value of n-bits from bitmap.

Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Syed Nayyar Waris <syednwaris@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
---
Changes in v7:
 - No change.

Changes in v6:
 - No change.

Changes in v5:
 - No change.

Changes in v4:
 - No change.

Changes in v3:
 - No change.

Changes in v2:
 - No change.

 include/asm-generic/bitops/find.h | 19 ++++++++++
 include/linux/bitmap.h            | 61 +++++++++++++++++++++++++++++++
 include/linux/bitops.h            | 13 +++++++
 lib/find_bit.c                    | 14 +++++++
 4 files changed, 107 insertions(+)

diff --git a/include/asm-generic/bitops/find.h b/include/asm-generic/bitops/find.h
index 9fdf21302fdf..4e6600759455 100644
--- a/include/asm-generic/bitops/find.h
+++ b/include/asm-generic/bitops/find.h
@@ -97,4 +97,23 @@ extern unsigned long find_next_clump8(unsigned long *clump,
 #define find_first_clump8(clump, bits, size) \
 	find_next_clump8((clump), (bits), (size), 0)
 
+/**
+ * find_next_clump - find next clump with set bits in a memory region
+ * @clump: location to store copy of found clump
+ * @addr: address to base the search on
+ * @size: bitmap size in number of bits
+ * @offset: bit offset at which to start searching
+ * @clump_size: clump size in bits
+ *
+ * Returns the bit offset for the next set clump; the found clump value is
+ * copied to the location pointed by @clump. If no bits are set, returns @size.
+ */
+extern unsigned long find_next_clump(unsigned long *clump,
+				      const unsigned long *addr,
+				      unsigned long size, unsigned long offset,
+				      unsigned long clump_size);
+
+#define find_first_clump(clump, bits, size, clump_size) \
+	find_next_clump((clump), (bits), (size), 0, (clump_size))
+
 #endif /*_ASM_GENERIC_BITOPS_FIND_H_ */
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 99058eb81042..7ab2c65fc964 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -75,7 +75,11 @@
  *  bitmap_from_arr32(dst, buf, nbits)          Copy nbits from u32[] buf to dst
  *  bitmap_to_arr32(buf, src, nbits)            Copy nbits from buf to u32[] dst
  *  bitmap_get_value8(map, start)               Get 8bit value from map at start
+ *  bitmap_get_value(map, start, nbits)		Get bit value of size
+ *						'nbits' from map at start
  *  bitmap_set_value8(map, value, start)        Set 8bit value to map at start
+ *  bitmap_set_value(map, value, start, nbits)	Set bit value of size 'nbits'
+ *						of map at start
  *
  * Note, bitmap_zero() and bitmap_fill() operate over the region of
  * unsigned longs, that is, bits behind bitmap till the unsigned long
@@ -563,6 +567,34 @@ static inline unsigned long bitmap_get_value8(const unsigned long *map,
 	return (map[index] >> offset) & 0xFF;
 }
 
+/**
+ * bitmap_get_value - get a value of n-bits from the memory region
+ * @map: address to the bitmap memory region
+ * @start: bit offset of the n-bit value
+ * @nbits: size of value in bits
+ *
+ * Returns value of nbits located at the @start bit offset within the @map
+ * memory region.
+ */
+static inline unsigned long bitmap_get_value(const unsigned long *map,
+					      unsigned long start,
+					      unsigned long nbits)
+{
+	const size_t index = BIT_WORD(start);
+	const unsigned long offset = start % BITS_PER_LONG;
+	const unsigned long ceiling = roundup(start + 1, BITS_PER_LONG);
+	const unsigned long space = ceiling - start;
+	unsigned long value_low, value_high;
+
+	if (space >= nbits)
+		return (map[index] >> offset) & GENMASK(nbits - 1, 0);
+	else {
+		value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
+		value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
+		return (value_low >> offset) | (value_high << space);
+	}
+}
+
 /**
  * bitmap_set_value8 - set an 8-bit value within a memory region
  * @map: address to the bitmap memory region
@@ -579,6 +611,35 @@ static inline void bitmap_set_value8(unsigned long *map, unsigned long value,
 	map[index] |= value << offset;
 }
 
+/**
+ * bitmap_set_value - set n-bit value within a memory region
+ * @map: address to the bitmap memory region
+ * @value: value of nbits
+ * @start: bit offset of the n-bit value
+ * @nbits: size of value in bits
+ */
+static inline void bitmap_set_value(unsigned long *map,
+				    unsigned long value,
+				    unsigned long start, unsigned long nbits)
+{
+	const size_t index = BIT_WORD(start);
+	const unsigned long offset = start % BITS_PER_LONG;
+	const unsigned long ceiling = roundup(start + 1, BITS_PER_LONG);
+	const unsigned long space = ceiling - start;
+
+	value &= GENMASK(nbits - 1, 0);
+
+	if (space >= nbits) {
+		map[index] &= ~(GENMASK(nbits + offset - 1, offset));
+		map[index] |= value << offset;
+	} else {
+		map[index] &= ~BITMAP_FIRST_WORD_MASK(start);
+		map[index] |= value << offset;
+		map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + nbits);
+		map[index + 1] |= (value >> space);
+	}
+}
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __LINUX_BITMAP_H */
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 9acf654f0b19..41c2d9ce63e7 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -62,6 +62,19 @@ extern unsigned long __sw_hweight64(__u64 w);
 	     (start) < (size); \
 	     (start) = find_next_clump8(&(clump), (bits), (size), (start) + 8))
 
+/**
+ * for_each_set_clump - iterate over bitmap for each clump with set bits
+ * @start: bit offset to start search and to store the current iteration offset
+ * @clump: location to store copy of current 8-bit clump
+ * @bits: bitmap address to base the search on
+ * @size: bitmap size in number of bits
+ * @clump_size: clump size in bits
+ */
+#define for_each_set_clump(start, clump, bits, size, clump_size) \
+	for ((start) = find_first_clump(&(clump), (bits), (size), (clump_size)); \
+	     (start) < (size); \
+	     (start) = find_next_clump(&(clump), (bits), (size), (start) + (clump_size), (clump_size)))
+
 static inline int get_bitmask_order(unsigned int count)
 {
 	int order;
diff --git a/lib/find_bit.c b/lib/find_bit.c
index 49f875f1baf7..1341bd39b32a 100644
--- a/lib/find_bit.c
+++ b/lib/find_bit.c
@@ -190,3 +190,17 @@ unsigned long find_next_clump8(unsigned long *clump, const unsigned long *addr,
 	return offset;
 }
 EXPORT_SYMBOL(find_next_clump8);
+
+unsigned long find_next_clump(unsigned long *clump, const unsigned long *addr,
+			       unsigned long size, unsigned long offset,
+			       unsigned long clump_size)
+{
+	offset = find_next_bit(addr, size, offset);
+	if (offset == size)
+		return size;
+
+	offset = rounddown(offset, clump_size);
+	*clump = bitmap_get_value(addr, offset, clump_size);
+	return offset;
+}
+EXPORT_SYMBOL(find_next_clump);
-- 
2.26.2

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 19019 bytes --]

Hi Syed,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on b9bbe6ed63b2b9f2c9ee5cbd0f2c946a2723f4ce]

url:    https://github.com/0day-ci/linux/commits/Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20200524-130931
base:    b9bbe6ed63b2b9f2c9ee5cbd0f2c946a2723f4ce
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

In file included from include/asm-generic/atomic-instrumented.h:20:0,
from arch/x86/include/asm/atomic.h:265,
from include/linux/atomic.h:7,
from include/linux/crypto.h:15,
from arch/x86/kernel/asm-offsets.c:9:
include/linux/bitmap.h: In function 'bitmap_get_value':
include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
>> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
return (map[index] >> offset) & GENMASK(nbits - 1, 0);
^~~~~~~
include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
>> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
return (map[index] >> offset) & GENMASK(nbits - 1, 0);
^~~~~~~
include/linux/bitmap.h: In function 'bitmap_set_value':
include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
value &= GENMASK(nbits - 1, 0);
^~~~~~~
include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
value &= GENMASK(nbits - 1, 0);
^~~~~~~
--
In file included from include/linux/bits.h:23:0,
from include/linux/bitops.h:5,
from include/linux/kernel.h:12,
from include/asm-generic/bug.h:19,
from arch/x86/include/asm/bug.h:83,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/gfp.h:5,
from arch/x86/mm/init.c:1:
include/linux/bitmap.h: In function 'bitmap_get_value':
include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
>> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
return (map[index] >> offset) & GENMASK(nbits - 1, 0);
^~~~~~~
include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
>> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
return (map[index] >> offset) & GENMASK(nbits - 1, 0);
^~~~~~~
include/linux/bitmap.h: In function 'bitmap_set_value':
include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
value &= GENMASK(nbits - 1, 0);
^~~~~~~
include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
value &= GENMASK(nbits - 1, 0);
^~~~~~~
arch/x86/mm/init.c: At top level:
arch/x86/mm/init.c:469:21: warning: no previous prototype for 'init_memory_mapping' [-Wmissing-prototypes]
unsigned long __ref init_memory_mapping(unsigned long start,
^~~~~~~~~~~~~~~~~~~
arch/x86/mm/init.c:711:13: warning: no previous prototype for 'poking_init' [-Wmissing-prototypes]
void __init poking_init(void)
^~~~~~~~~~~
arch/x86/mm/init.c:860:13: warning: no previous prototype for 'mem_encrypt_free_decrypted_mem' [-Wmissing-prototypes]
void __weak mem_encrypt_free_decrypted_mem(void) { }
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
--
In file included from include/linux/bits.h:23:0,
from include/linux/bitops.h:5,
from include/linux/kernel.h:12,
from include/asm-generic/bug.h:19,
from arch/x86/include/asm/bug.h:83,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/mm.h:9,
from include/linux/memblock.h:13,
from arch/x86/mm/ioremap.c:10:
include/linux/bitmap.h: In function 'bitmap_get_value':
include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
>> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
return (map[index] >> offset) & GENMASK(nbits - 1, 0);
^~~~~~~
include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
>> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
return (map[index] >> offset) & GENMASK(nbits - 1, 0);
^~~~~~~
include/linux/bitmap.h: In function 'bitmap_set_value':
include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
value &= GENMASK(nbits - 1, 0);
^~~~~~~
include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
value &= GENMASK(nbits - 1, 0);
^~~~~~~
arch/x86/mm/ioremap.c: At top level:
arch/x86/mm/ioremap.c:484:12: warning: no previous prototype for 'arch_ioremap_p4d_supported' [-Wmissing-prototypes]
int __init arch_ioremap_p4d_supported(void)
^~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/mm/ioremap.c:489:12: warning: no previous prototype for 'arch_ioremap_pud_supported' [-Wmissing-prototypes]
int __init arch_ioremap_pud_supported(void)
^~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/mm/ioremap.c:498:12: warning: no previous prototype for 'arch_ioremap_pmd_supported' [-Wmissing-prototypes]
int __init arch_ioremap_pmd_supported(void)
^~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/mm/ioremap.c:737:17: warning: no previous prototype for 'early_memremap_pgprot_adjust' [-Wmissing-prototypes]
pgprot_t __init early_memremap_pgprot_adjust(resource_size_t phys_addr,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
--
In file included from include/linux/bits.h:23:0,
from include/linux/bitops.h:5,
from include/linux/kernel.h:12,
from arch/x86/include/asm/percpu.h:45,
from arch/x86/include/asm/current.h:6,
from include/linux/sched.h:12,
from include/linux/uaccess.h:5,
from arch/x86/mm/extable.c:3:
include/linux/bitmap.h: In function 'bitmap_get_value':
include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
>> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
return (map[index] >> offset) & GENMASK(nbits - 1, 0);
^~~~~~~
include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
>> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
return (map[index] >> offset) & GENMASK(nbits - 1, 0);
^~~~~~~
include/linux/bitmap.h: In function 'bitmap_set_value':
include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
value &= GENMASK(nbits - 1, 0);
^~~~~~~
include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
value &= GENMASK(nbits - 1, 0);
^~~~~~~
arch/x86/mm/extable.c: At top level:
arch/x86/mm/extable.c:26:16: warning: no previous prototype for 'ex_handler_default' [-Wmissing-prototypes]
__visible bool ex_handler_default(const struct exception_table_entry *fixup,
^~~~~~~~~~~~~~~~~~
arch/x86/mm/extable.c:36:16: warning: no previous prototype for 'ex_handler_fault' [-Wmissing-prototypes]
__visible bool ex_handler_fault(const struct exception_table_entry *fixup,
^~~~~~~~~~~~~~~~
arch/x86/mm/extable.c:57:16: warning: no previous prototype for 'ex_handler_fprestore' [-Wmissing-prototypes]
__visible bool ex_handler_fprestore(const struct exception_table_entry *fixup,
^~~~~~~~~~~~~~~~~~~~
arch/x86/mm/extable.c:72:16: warning: no previous prototype for 'ex_handler_uaccess' [-Wmissing-prototypes]
__visible bool ex_handler_uaccess(const struct exception_table_entry *fixup,
^~~~~~~~~~~~~~~~~~
arch/x86/mm/extable.c:83:16: warning: no previous prototype for 'ex_handler_rdmsr_unsafe' [-Wmissing-prototypes]
__visible bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
^~~~~~~~~~~~~~~~~~~~~~~
arch/x86/mm/extable.c:100:16: warning: no previous prototype for 'ex_handler_wrmsr_unsafe' [-Wmissing-prototypes]
__visible bool ex_handler_wrmsr_unsafe(const struct exception_table_entry *fixup,
^~~~~~~~~~~~~~~~~~~~~~~
arch/x86/mm/extable.c:116:16: warning: no previous prototype for 'ex_handler_clear_fs' [-Wmissing-prototypes]
__visible bool ex_handler_clear_fs(const struct exception_table_entry *fixup,
^~~~~~~~~~~~~~~~~~~
--
In file included from include/linux/bits.h:23:0,
from include/linux/bitops.h:5,
from include/linux/kernel.h:12,
from include/asm-generic/bug.h:19,
from arch/x86/include/asm/bug.h:83,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/mm.h:9,
from arch/x86/mm/mmap.c:15:
include/linux/bitmap.h: In function 'bitmap_get_value':
include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
>> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
return (map[index] >> offset) & GENMASK(nbits - 1, 0);
^~~~~~~
include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
>> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
return (map[index] >> offset) & GENMASK(nbits - 1, 0);
^~~~~~~
include/linux/bitmap.h: In function 'bitmap_set_value':
include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
value &= GENMASK(nbits - 1, 0);
^~~~~~~
include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
^~~~~~~~~~~~~~~~~~~
include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
value &= GENMASK(nbits - 1, 0);
^~~~~~~
arch/x86/mm/mmap.c: At top level:
arch/x86/mm/mmap.c:75:15: warning: no previous prototype for 'arch_mmap_rnd' [-Wmissing-prototypes]
unsigned long arch_mmap_rnd(void)
^~~~~~~~~~~~~
arch/x86/mm/mmap.c:216:5: warning: no previous prototype for 'valid_phys_addr_range' [-Wmissing-prototypes]
int valid_phys_addr_range(phys_addr_t addr, size_t count)
^~~~~~~~~~~~~~~~~~~~~
arch/x86/mm/mmap.c:222:5: warning: no previous prototype for 'valid_mmap_phys_addr_range' [-Wmissing-prototypes]
int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
^~~~~~~~~~~~~~~~~~~~~~~~~~
..

vim +/GENMASK +590 include/linux/bitmap.h

   569	
   570	/**
   571	 * bitmap_get_value - get a value of n-bits from the memory region
   572	 * @map: address to the bitmap memory region
   573	 * @start: bit offset of the n-bit value
   574	 * @nbits: size of value in bits
   575	 *
   576	 * Returns value of nbits located at the @start bit offset within the @map
   577	 * memory region.
   578	 */
   579	static inline unsigned long bitmap_get_value(const unsigned long *map,
   580						      unsigned long start,
   581						      unsigned long nbits)
   582	{
   583		const size_t index = BIT_WORD(start);
   584		const unsigned long offset = start % BITS_PER_LONG;
   585		const unsigned long ceiling = roundup(start + 1, BITS_PER_LONG);
   586		const unsigned long space = ceiling - start;
   587		unsigned long value_low, value_high;
   588	
   589		if (space >= nbits)
 > 590			return (map[index] >> offset) & GENMASK(nbits - 1, 0);
   591		else {
   592			value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
   593			value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
   594			return (value_low >> offset) | (value_high << space);
   595		}
   596	}
   597	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7245 bytes --]

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Syed Nayyar Waris]

On Sun, May 24, 2020 at 8:15 PM kbuild test robot <lkp@intel.com> wrote:
>
> Hi Syed,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on b9bbe6ed63b2b9f2c9ee5cbd0f2c946a2723f4ce]
>
> url:    https://github.com/0day-ci/linux/commits/Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20200524-130931
> base:    b9bbe6ed63b2b9f2c9ee5cbd0f2c946a2723f4ce
> config: i386-tinyconfig (attached as .config)
> compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
> reproduce (this is a W=1 build):
>         # save the attached .config to linux build tree
>         make ARCH=i386
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>, old ones prefixed by <<):
>
> In file included from include/asm-generic/atomic-instrumented.h:20:0,
> from arch/x86/include/asm/atomic.h:265,
> from include/linux/atomic.h:7,
> from include/linux/crypto.h:15,
> from arch/x86/kernel/asm-offsets.c:9:
> include/linux/bitmap.h: In function 'bitmap_get_value':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> >> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
> return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> ^~~~~~~
> include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> >> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
> return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> ^~~~~~~
> include/linux/bitmap.h: In function 'bitmap_set_value':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
> value &= GENMASK(nbits - 1, 0);
> ^~~~~~~
> include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
> value &= GENMASK(nbits - 1, 0);
> ^~~~~~~
> --
> In file included from include/linux/bits.h:23:0,
> from include/linux/bitops.h:5,
> from include/linux/kernel.h:12,
> from include/asm-generic/bug.h:19,
> from arch/x86/include/asm/bug.h:83,
> from include/linux/bug.h:5,
> from include/linux/mmdebug.h:5,
> from include/linux/gfp.h:5,
> from arch/x86/mm/init.c:1:
> include/linux/bitmap.h: In function 'bitmap_get_value':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> >> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
> return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> ^~~~~~~
> include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> >> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
> return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> ^~~~~~~
> include/linux/bitmap.h: In function 'bitmap_set_value':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
> value &= GENMASK(nbits - 1, 0);
> ^~~~~~~
> include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
> value &= GENMASK(nbits - 1, 0);
> ^~~~~~~
> arch/x86/mm/init.c: At top level:
> arch/x86/mm/init.c:469:21: warning: no previous prototype for 'init_memory_mapping' [-Wmissing-prototypes]
> unsigned long __ref init_memory_mapping(unsigned long start,
> ^~~~~~~~~~~~~~~~~~~
> arch/x86/mm/init.c:711:13: warning: no previous prototype for 'poking_init' [-Wmissing-prototypes]
> void __init poking_init(void)
> ^~~~~~~~~~~
> arch/x86/mm/init.c:860:13: warning: no previous prototype for 'mem_encrypt_free_decrypted_mem' [-Wmissing-prototypes]
> void __weak mem_encrypt_free_decrypted_mem(void) { }
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> --
> In file included from include/linux/bits.h:23:0,
> from include/linux/bitops.h:5,
> from include/linux/kernel.h:12,
> from include/asm-generic/bug.h:19,
> from arch/x86/include/asm/bug.h:83,
> from include/linux/bug.h:5,
> from include/linux/mmdebug.h:5,
> from include/linux/mm.h:9,
> from include/linux/memblock.h:13,
> from arch/x86/mm/ioremap.c:10:
> include/linux/bitmap.h: In function 'bitmap_get_value':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> >> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
> return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> ^~~~~~~
> include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> >> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
> return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> ^~~~~~~
> include/linux/bitmap.h: In function 'bitmap_set_value':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
> value &= GENMASK(nbits - 1, 0);
> ^~~~~~~
> include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
> value &= GENMASK(nbits - 1, 0);
> ^~~~~~~
> arch/x86/mm/ioremap.c: At top level:
> arch/x86/mm/ioremap.c:484:12: warning: no previous prototype for 'arch_ioremap_p4d_supported' [-Wmissing-prototypes]
> int __init arch_ioremap_p4d_supported(void)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/mm/ioremap.c:489:12: warning: no previous prototype for 'arch_ioremap_pud_supported' [-Wmissing-prototypes]
> int __init arch_ioremap_pud_supported(void)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/mm/ioremap.c:498:12: warning: no previous prototype for 'arch_ioremap_pmd_supported' [-Wmissing-prototypes]
> int __init arch_ioremap_pmd_supported(void)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/mm/ioremap.c:737:17: warning: no previous prototype for 'early_memremap_pgprot_adjust' [-Wmissing-prototypes]
> pgprot_t __init early_memremap_pgprot_adjust(resource_size_t phys_addr,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> --
> In file included from include/linux/bits.h:23:0,
> from include/linux/bitops.h:5,
> from include/linux/kernel.h:12,
> from arch/x86/include/asm/percpu.h:45,
> from arch/x86/include/asm/current.h:6,
> from include/linux/sched.h:12,
> from include/linux/uaccess.h:5,
> from arch/x86/mm/extable.c:3:
> include/linux/bitmap.h: In function 'bitmap_get_value':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> >> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
> return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> ^~~~~~~
> include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> >> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
> return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> ^~~~~~~
> include/linux/bitmap.h: In function 'bitmap_set_value':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
> value &= GENMASK(nbits - 1, 0);
> ^~~~~~~
> include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
> value &= GENMASK(nbits - 1, 0);
> ^~~~~~~
> arch/x86/mm/extable.c: At top level:
> arch/x86/mm/extable.c:26:16: warning: no previous prototype for 'ex_handler_default' [-Wmissing-prototypes]
> __visible bool ex_handler_default(const struct exception_table_entry *fixup,
> ^~~~~~~~~~~~~~~~~~
> arch/x86/mm/extable.c:36:16: warning: no previous prototype for 'ex_handler_fault' [-Wmissing-prototypes]
> __visible bool ex_handler_fault(const struct exception_table_entry *fixup,
> ^~~~~~~~~~~~~~~~
> arch/x86/mm/extable.c:57:16: warning: no previous prototype for 'ex_handler_fprestore' [-Wmissing-prototypes]
> __visible bool ex_handler_fprestore(const struct exception_table_entry *fixup,
> ^~~~~~~~~~~~~~~~~~~~
> arch/x86/mm/extable.c:72:16: warning: no previous prototype for 'ex_handler_uaccess' [-Wmissing-prototypes]
> __visible bool ex_handler_uaccess(const struct exception_table_entry *fixup,
> ^~~~~~~~~~~~~~~~~~
> arch/x86/mm/extable.c:83:16: warning: no previous prototype for 'ex_handler_rdmsr_unsafe' [-Wmissing-prototypes]
> __visible bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
> ^~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/mm/extable.c:100:16: warning: no previous prototype for 'ex_handler_wrmsr_unsafe' [-Wmissing-prototypes]
> __visible bool ex_handler_wrmsr_unsafe(const struct exception_table_entry *fixup,
> ^~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/mm/extable.c:116:16: warning: no previous prototype for 'ex_handler_clear_fs' [-Wmissing-prototypes]
> __visible bool ex_handler_clear_fs(const struct exception_table_entry *fixup,
> ^~~~~~~~~~~~~~~~~~~
> --
> In file included from include/linux/bits.h:23:0,
> from include/linux/bitops.h:5,
> from include/linux/kernel.h:12,
> from include/asm-generic/bug.h:19,
> from arch/x86/include/asm/bug.h:83,
> from include/linux/bug.h:5,
> from include/linux/mmdebug.h:5,
> from include/linux/mm.h:9,
> from arch/x86/mm/mmap.c:15:
> include/linux/bitmap.h: In function 'bitmap_get_value':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> >> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
> return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> ^~~~~~~
> include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> >> include/linux/bitmap.h:590:35: note: in expansion of macro 'GENMASK'
> return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> ^~~~~~~
> include/linux/bitmap.h: In function 'bitmap_set_value':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
> value &= GENMASK(nbits - 1, 0);
> ^~~~~~~
> include/linux/bits.h:26:40: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> ^~~~~~~~~~~~~~~~~~~
> include/linux/bitmap.h:630:11: note: in expansion of macro 'GENMASK'
> value &= GENMASK(nbits - 1, 0);
> ^~~~~~~
> arch/x86/mm/mmap.c: At top level:
> arch/x86/mm/mmap.c:75:15: warning: no previous prototype for 'arch_mmap_rnd' [-Wmissing-prototypes]
> unsigned long arch_mmap_rnd(void)
> ^~~~~~~~~~~~~
> arch/x86/mm/mmap.c:216:5: warning: no previous prototype for 'valid_phys_addr_range' [-Wmissing-prototypes]
> int valid_phys_addr_range(phys_addr_t addr, size_t count)
> ^~~~~~~~~~~~~~~~~~~~~
> arch/x86/mm/mmap.c:222:5: warning: no previous prototype for 'valid_mmap_phys_addr_range' [-Wmissing-prototypes]
> int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
> ^~~~~~~~~~~~~~~~~~~~~~~~~~
> ..
>
> vim +/GENMASK +590 include/linux/bitmap.h
>
>    569
>    570  /**
>    571   * bitmap_get_value - get a value of n-bits from the memory region
>    572   * @map: address to the bitmap memory region
>    573   * @start: bit offset of the n-bit value
>    574   * @nbits: size of value in bits
>    575   *
>    576   * Returns value of nbits located at the @start bit offset within the @map
>    577   * memory region.
>    578   */
>    579  static inline unsigned long bitmap_get_value(const unsigned long *map,
>    580                                                unsigned long start,
>    581                                                unsigned long nbits)
>    582  {
>    583          const size_t index = BIT_WORD(start);
>    584          const unsigned long offset = start % BITS_PER_LONG;
>    585          const unsigned long ceiling = roundup(start + 1, BITS_PER_LONG);
>    586          const unsigned long space = ceiling - start;
>    587          unsigned long value_low, value_high;
>    588
>    589          if (space >= nbits)
>  > 590                  return (map[index] >> offset) & GENMASK(nbits - 1, 0);
>    591          else {
>    592                  value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
>    593                  value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
>    594                  return (value_low >> offset) | (value_high << space);
>    595          }
>    596  }
>    597
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Hi William, Andy and All,

Regarding the above compilation warnings. All the warnings are because
of GENMASK usage in my patch.
The warnings are coming because of sanity checks present for 'GENMASK'
macro in include/linux/bits.h.

Taking the example statement (in my patch) where compilation warning
is getting reported:
return (map[index] >> offset) & GENMASK(nbits - 1, 0);

'nbits' is of type 'unsigned long'.
In above, the sanity check is comparing '0' with unsigned value. And
unsigned value can't be less than '0' ever, hence the warning.
But this warning will occur whenever there will be '0' as one of the
'argument' and an unsigned variable as another 'argument' for GENMASK.

This warning is getting cleared if I cast the 'nbits' to 'long'.

Let me know if I should submit a next patch with the casts applied.
What do you guys think?

Regards
Syed Nayyar Waris

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Andy Shevchenko]

On Fri, May 29, 2020 at 11:38:18PM +0530, Syed Nayyar Waris wrote:
> On Sun, May 24, 2020 at 8:15 PM kbuild test robot <lkp@intel.com> wrote:

...

> >    579  static inline unsigned long bitmap_get_value(const unsigned long *map,
> >    580                                                unsigned long start,
> >    581                                                unsigned long nbits)
> >    582  {
> >    583          const size_t index = BIT_WORD(start);
> >    584          const unsigned long offset = start % BITS_PER_LONG;
> >    585          const unsigned long ceiling = roundup(start + 1, BITS_PER_LONG);
> >    586          const unsigned long space = ceiling - start;
> >    587          unsigned long value_low, value_high;
> >    588
> >    589          if (space >= nbits)
> >  > 590                  return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> >    591          else {
> >    592                  value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
> >    593                  value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
> >    594                  return (value_low >> offset) | (value_high << space);
> >    595          }
> >    596  }

> Regarding the above compilation warnings. All the warnings are because
> of GENMASK usage in my patch.
> The warnings are coming because of sanity checks present for 'GENMASK'
> macro in include/linux/bits.h.
> 
> Taking the example statement (in my patch) where compilation warning
> is getting reported:
> return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> 
> 'nbits' is of type 'unsigned long'.
> In above, the sanity check is comparing '0' with unsigned value. And
> unsigned value can't be less than '0' ever, hence the warning.
> But this warning will occur whenever there will be '0' as one of the
> 'argument' and an unsigned variable as another 'argument' for GENMASK.
> 
> This warning is getting cleared if I cast the 'nbits' to 'long'.
> 
> Let me know if I should submit a next patch with the casts applied.
> What do you guys think?

Proper fix is to fix GENMASK(), but allowed workaround is to use
	(BIT(nbits) - 1)
instead.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Syed Nayyar Waris]

On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, May 29, 2020 at 11:38:18PM +0530, Syed Nayyar Waris wrote:
> > On Sun, May 24, 2020 at 8:15 PM kbuild test robot <lkp@intel.com> wrote:
>
> ...
>
> > >    579  static inline unsigned long bitmap_get_value(const unsigned long *map,
> > >    580                                                unsigned long start,
> > >    581                                                unsigned long nbits)
> > >    582  {
> > >    583          const size_t index = BIT_WORD(start);
> > >    584          const unsigned long offset = start % BITS_PER_LONG;
> > >    585          const unsigned long ceiling = roundup(start + 1, BITS_PER_LONG);
> > >    586          const unsigned long space = ceiling - start;
> > >    587          unsigned long value_low, value_high;
> > >    588
> > >    589          if (space >= nbits)
> > >  > 590                  return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> > >    591          else {
> > >    592                  value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
> > >    593                  value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
> > >    594                  return (value_low >> offset) | (value_high << space);
> > >    595          }
> > >    596  }
>
> > Regarding the above compilation warnings. All the warnings are because
> > of GENMASK usage in my patch.
> > The warnings are coming because of sanity checks present for 'GENMASK'
> > macro in include/linux/bits.h.
> >
> > Taking the example statement (in my patch) where compilation warning
> > is getting reported:
> > return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> >
> > 'nbits' is of type 'unsigned long'.
> > In above, the sanity check is comparing '0' with unsigned value. And
> > unsigned value can't be less than '0' ever, hence the warning.
> > But this warning will occur whenever there will be '0' as one of the
> > 'argument' and an unsigned variable as another 'argument' for GENMASK.
> >
> > This warning is getting cleared if I cast the 'nbits' to 'long'.
> >
> > Let me know if I should submit a next patch with the casts applied.
> > What do you guys think?
>
> Proper fix is to fix GENMASK(), but allowed workaround is to use
>         (BIT(nbits) - 1)
> instead.
>
> --
> With Best Regards,
> Andy Shevchenko
>

Hi Andy. Thank You for your comment.

When I used BIT macro (earlier), I had faced a problem. I want to tell
you about that.

Inside functions 'bitmap_set_value' and 'bitmap_get_value' when nbits (or
clump size) is BITS_PER_LONG, unexpected calculation happens.

Explanation:
Actually when nbits (clump size) is 64 (BITS_PER_LONG is 64 on my computer),
(BIT(nbits) - 1)
gives a value of zero and when this zero is ANDed with any value, it
makes it full zero. This is unexpected and incorrect calculation happening.

What actually happens is in the macro expansion of BIT(64), that is 1
<< 64, the '1' overflows from leftmost bit position (most significant
bit) and re-enters at the rightmost bit position (least significant
bit), therefore 1 << 64 becomes '0x1', and when another '1' is
subtracted from this, the final result becomes 0.

Since this macro is being used in both bitmap_get_value and
bitmap_set_value functions, it will give unexpected results when nbits or clump
size is BITS_PER_LONG (32 or 64 depending on arch).

William also knows about this issue:

"This is undefined behavior in the C standard (section 6.5.7 in the N1124)"

Andy, William,
Let me know what do you think ?

Regards
Syed Nayyar Waris

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 4435 bytes --]

On Sat, May 30, 2020 at 01:32:44AM +0530, Syed Nayyar Waris wrote:
> On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Fri, May 29, 2020 at 11:38:18PM +0530, Syed Nayyar Waris wrote:
> > > On Sun, May 24, 2020 at 8:15 PM kbuild test robot <lkp@intel.com> wrote:
> >
> > ...
> >
> > > >    579  static inline unsigned long bitmap_get_value(const unsigned long *map,
> > > >    580                                                unsigned long start,
> > > >    581                                                unsigned long nbits)
> > > >    582  {
> > > >    583          const size_t index = BIT_WORD(start);
> > > >    584          const unsigned long offset = start % BITS_PER_LONG;
> > > >    585          const unsigned long ceiling = roundup(start + 1, BITS_PER_LONG);
> > > >    586          const unsigned long space = ceiling - start;
> > > >    587          unsigned long value_low, value_high;
> > > >    588
> > > >    589          if (space >= nbits)
> > > >  > 590                  return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> > > >    591          else {
> > > >    592                  value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
> > > >    593                  value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
> > > >    594                  return (value_low >> offset) | (value_high << space);
> > > >    595          }
> > > >    596  }
> >
> > > Regarding the above compilation warnings. All the warnings are because
> > > of GENMASK usage in my patch.
> > > The warnings are coming because of sanity checks present for 'GENMASK'
> > > macro in include/linux/bits.h.
> > >
> > > Taking the example statement (in my patch) where compilation warning
> > > is getting reported:
> > > return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> > >
> > > 'nbits' is of type 'unsigned long'.
> > > In above, the sanity check is comparing '0' with unsigned value. And
> > > unsigned value can't be less than '0' ever, hence the warning.
> > > But this warning will occur whenever there will be '0' as one of the
> > > 'argument' and an unsigned variable as another 'argument' for GENMASK.
> > >
> > > This warning is getting cleared if I cast the 'nbits' to 'long'.
> > >
> > > Let me know if I should submit a next patch with the casts applied.
> > > What do you guys think?
> >
> > Proper fix is to fix GENMASK(), but allowed workaround is to use
> >         (BIT(nbits) - 1)
> > instead.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> 
> Hi Andy. Thank You for your comment.
> 
> When I used BIT macro (earlier), I had faced a problem. I want to tell
> you about that.
> 
> Inside functions 'bitmap_set_value' and 'bitmap_get_value' when nbits (or
> clump size) is BITS_PER_LONG, unexpected calculation happens.
> 
> Explanation:
> Actually when nbits (clump size) is 64 (BITS_PER_LONG is 64 on my computer),
> (BIT(nbits) - 1)
> gives a value of zero and when this zero is ANDed with any value, it
> makes it full zero. This is unexpected and incorrect calculation happening.
> 
> What actually happens is in the macro expansion of BIT(64), that is 1
> << 64, the '1' overflows from leftmost bit position (most significant
> bit) and re-enters at the rightmost bit position (least significant
> bit), therefore 1 << 64 becomes '0x1', and when another '1' is
> subtracted from this, the final result becomes 0.
> 
> Since this macro is being used in both bitmap_get_value and
> bitmap_set_value functions, it will give unexpected results when nbits or clump
> size is BITS_PER_LONG (32 or 64 depending on arch).
> 
> William also knows about this issue:
> 
> "This is undefined behavior in the C standard (section 6.5.7 in the N1124)"
> 
> Andy, William,
> Let me know what do you think ?
> 
> Regards
> Syed Nayyar Waris

We can't use BIT here because nbits could be equal to BITS_PER_LONG in
some cases. Casting to long should be fine because the nbits will never
be greater than BITS_PER_LONG, so long should be safe to use.

However, I agree with Andy that the proper solution is to fix GENMASK so
that this warning does not come up. What's the actual line of code in
the GENMASK macro that is throwing this warning? I'd like to understand
better the logic of this sanity check.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Syed Nayyar Waris]

On Sat, May 30, 2020 at 3:01 AM William Breathitt Gray
<vilhelm.gray@gmail.com> wrote:
>
> On Sat, May 30, 2020 at 01:32:44AM +0530, Syed Nayyar Waris wrote:
> > On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Fri, May 29, 2020 at 11:38:18PM +0530, Syed Nayyar Waris wrote:
> > > > On Sun, May 24, 2020 at 8:15 PM kbuild test robot <lkp@intel.com> wrote:
> > >
> > > ...
> > >
> > > > >    579  static inline unsigned long bitmap_get_value(const unsigned long *map,
> > > > >    580                                                unsigned long start,
> > > > >    581                                                unsigned long nbits)
> > > > >    582  {
> > > > >    583          const size_t index = BIT_WORD(start);
> > > > >    584          const unsigned long offset = start % BITS_PER_LONG;
> > > > >    585          const unsigned long ceiling = roundup(start + 1, BITS_PER_LONG);
> > > > >    586          const unsigned long space = ceiling - start;
> > > > >    587          unsigned long value_low, value_high;
> > > > >    588
> > > > >    589          if (space >= nbits)
> > > > >  > 590                  return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> > > > >    591          else {
> > > > >    592                  value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
> > > > >    593                  value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
> > > > >    594                  return (value_low >> offset) | (value_high << space);
> > > > >    595          }
> > > > >    596  }
> > >
> > > > Regarding the above compilation warnings. All the warnings are because
> > > > of GENMASK usage in my patch.
> > > > The warnings are coming because of sanity checks present for 'GENMASK'
> > > > macro in include/linux/bits.h.
> > > >
> > > > Taking the example statement (in my patch) where compilation warning
> > > > is getting reported:
> > > > return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> > > >
> > > > 'nbits' is of type 'unsigned long'.
> > > > In above, the sanity check is comparing '0' with unsigned value. And
> > > > unsigned value can't be less than '0' ever, hence the warning.
> > > > But this warning will occur whenever there will be '0' as one of the
> > > > 'argument' and an unsigned variable as another 'argument' for GENMASK.
> > > >
> > > > This warning is getting cleared if I cast the 'nbits' to 'long'.
> > > >
> > > > Let me know if I should submit a next patch with the casts applied.
> > > > What do you guys think?
> > >
> > > Proper fix is to fix GENMASK(), but allowed workaround is to use
> > >         (BIT(nbits) - 1)
> > > instead.
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> > >
> >
> > Hi Andy. Thank You for your comment.
> >
> > When I used BIT macro (earlier), I had faced a problem. I want to tell
> > you about that.
> >
> > Inside functions 'bitmap_set_value' and 'bitmap_get_value' when nbits (or
> > clump size) is BITS_PER_LONG, unexpected calculation happens.
> >
> > Explanation:
> > Actually when nbits (clump size) is 64 (BITS_PER_LONG is 64 on my computer),
> > (BIT(nbits) - 1)
> > gives a value of zero and when this zero is ANDed with any value, it
> > makes it full zero. This is unexpected and incorrect calculation happening.
> >
> > What actually happens is in the macro expansion of BIT(64), that is 1
> > << 64, the '1' overflows from leftmost bit position (most significant
> > bit) and re-enters at the rightmost bit position (least significant
> > bit), therefore 1 << 64 becomes '0x1', and when another '1' is
> > subtracted from this, the final result becomes 0.
> >
> > Since this macro is being used in both bitmap_get_value and
> > bitmap_set_value functions, it will give unexpected results when nbits or clump
> > size is BITS_PER_LONG (32 or 64 depending on arch).
> >
> > William also knows about this issue:
> >
> > "This is undefined behavior in the C standard (section 6.5.7 in the N1124)"
> >
> > Andy, William,
> > Let me know what do you think ?
> >
> > Regards
> > Syed Nayyar Waris
>
> We can't use BIT here because nbits could be equal to BITS_PER_LONG in
> some cases. Casting to long should be fine because the nbits will never
> be greater than BITS_PER_LONG, so long should be safe to use.
>
> However, I agree with Andy that the proper solution is to fix GENMASK so
> that this warning does not come up. What's the actual line of code in
> the GENMASK macro that is throwing this warning? I'd like to understand
> better the logic of this sanity check.
>
> William Breathitt Gray

Here is the code snippet:

#define GENMASK_INPUT_CHECK(h, l) \
        (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
        __builtin_constant_p((l) > (h)), (l) > (h), 0)))

Above you can see the comparisons are being done in the last line.
And because of these comparisons, those compilation warnings are generated.

For full code related to GENMASK macro:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/tree/include/linux/bits.h

Yes I too agree, I can work on GENMASK.

Regards
Syed Nayyar Waris

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Andy Shevchenko]

On Fri, May 29, 2020 at 11:07 PM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, May 29, 2020 at 11:38:18PM +0530, Syed Nayyar Waris wrote:
> > > On Sun, May 24, 2020 at 8:15 PM kbuild test robot <lkp@intel.com> wrote:

...

> > > Taking the example statement (in my patch) where compilation warning
> > > is getting reported:
> > > return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> > >
> > > 'nbits' is of type 'unsigned long'.
> > > In above, the sanity check is comparing '0' with unsigned value. And
> > > unsigned value can't be less than '0' ever, hence the warning.
> > > But this warning will occur whenever there will be '0' as one of the
> > > 'argument' and an unsigned variable as another 'argument' for GENMASK.

> > Proper fix is to fix GENMASK(), but allowed workaround is to use
> >         (BIT(nbits) - 1)
> > instead.

> When I used BIT macro (earlier), I had faced a problem. I want to tell
> you about that.
>
> Inside functions 'bitmap_set_value' and 'bitmap_get_value' when nbits (or
> clump size) is BITS_PER_LONG, unexpected calculation happens.
>
> Explanation:
> Actually when nbits (clump size) is 64 (BITS_PER_LONG is 64 on my computer),
> (BIT(nbits) - 1)
> gives a value of zero and when this zero is ANDed with any value, it
> makes it full zero. This is unexpected and incorrect calculation happening.
>
> What actually happens is in the macro expansion of BIT(64), that is 1
> << 64, the '1' overflows from leftmost bit position (most significant
> bit) and re-enters at the rightmost bit position (least significant
> bit), therefore 1 << 64 becomes '0x1', and when another '1' is
> subtracted from this, the final result becomes 0.
>
> Since this macro is being used in both bitmap_get_value and
> bitmap_set_value functions, it will give unexpected results when nbits or clump
> size is BITS_PER_LONG (32 or 64 depending on arch).

I see, something like
https://elixir.bootlin.com/linux/latest/source/include/linux/dma-mapping.h#L139
should be done.
But yes, let's try to fix GENMASK().

So, if we modify the following

  #define GENMASK_INPUT_CHECK(h, l) \
    (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
    __builtin_constant_p((l) > (h)), (l) > (h), 0)))

to be

  #define GENMASK_INPUT_CHECK(h, l) \
    (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
    __builtin_constant_p((l) > (h)), (l) ? (l) > (h) : 0, 0)))

would it work?

> William also knows about this issue:
> "This is undefined behavior in the C standard (section 6.5.7 in the N1124)"

I think it is about 6.5.7.3  here, 1U << 31 (or 63) is okay.

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Andy Shevchenko]

On Fri, May 29, 2020 at 11:07 PM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, May 29, 2020 at 11:38:18PM +0530, Syed Nayyar Waris wrote:
> > > On Sun, May 24, 2020 at 8:15 PM kbuild test robot <lkp@intel.com> wrote:

...

> > > Taking the example statement (in my patch) where compilation warning
> > > is getting reported:
> > > return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> > >
> > > 'nbits' is of type 'unsigned long'.
> > > In above, the sanity check is comparing '0' with unsigned value. And
> > > unsigned value can't be less than '0' ever, hence the warning.
> > > But this warning will occur whenever there will be '0' as one of the
> > > 'argument' and an unsigned variable as another 'argument' for GENMASK.

> > Proper fix is to fix GENMASK(), but allowed workaround is to use
> >         (BIT(nbits) - 1)
> > instead.

> When I used BIT macro (earlier), I had faced a problem. I want to tell
> you about that.
>
> Inside functions 'bitmap_set_value' and 'bitmap_get_value' when nbits (or
> clump size) is BITS_PER_LONG, unexpected calculation happens.
>
> Explanation:
> Actually when nbits (clump size) is 64 (BITS_PER_LONG is 64 on my computer),
> (BIT(nbits) - 1)
> gives a value of zero and when this zero is ANDed with any value, it
> makes it full zero. This is unexpected and incorrect calculation happening.
>
> What actually happens is in the macro expansion of BIT(64), that is 1
> << 64, the '1' overflows from leftmost bit position (most significant
> bit) and re-enters at the rightmost bit position (least significant
> bit), therefore 1 << 64 becomes '0x1', and when another '1' is
> subtracted from this, the final result becomes 0.
>
> Since this macro is being used in both bitmap_get_value and
> bitmap_set_value functions, it will give unexpected results when nbits or clump
> size is BITS_PER_LONG (32 or 64 depending on arch).

I see, something like
https://elixir.bootlin.com/linux/latest/source/include/linux/dma-mapping.h#L139
should be done.
But yes, let's try to fix GENMASK().

So, if we modify the following

  #define GENMASK_INPUT_CHECK(h, l) \
    (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
    __builtin_constant_p((l) > (h)), (l) > (h), 0)))

to be

  #define GENMASK_INPUT_CHECK(h, l) \
    (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
    __builtin_constant_p((l) > (h)), (l) ? (l) > (h) : 0, 0)))

would it work?

> William also knows about this issue:
> "This is undefined behavior in the C standard (section 6.5.7 in the N1124)"

I think it is about 6.5.7.3  here, 1U << 31 (or 63) is okay.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Andy Shevchenko]

On Sat, May 30, 2020 at 12:42 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, May 29, 2020 at 11:07 PM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> > On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:

...

> > Explanation:
> > Actually when nbits (clump size) is 64 (BITS_PER_LONG is 64 on my computer),
> > (BIT(nbits) - 1)
> > gives a value of zero and when this zero is ANDed with any value, it
> > makes it full zero. This is unexpected and incorrect calculation happening.
> >
> > What actually happens is in the macro expansion of BIT(64), that is 1
> > << 64, the '1' overflows from leftmost bit position (most significant
> > bit) and re-enters at the rightmost bit position (least significant
> > bit), therefore 1 << 64 becomes '0x1', and when another '1' is
> > subtracted from this, the final result becomes 0.
> >
> > Since this macro is being used in both bitmap_get_value and
> > bitmap_set_value functions, it will give unexpected results when nbits or clump
> > size is BITS_PER_LONG (32 or 64 depending on arch).
>
> I see, something like
> https://elixir.bootlin.com/linux/latest/source/include/linux/dma-mapping.h#L139
> should be done.
> But yes, let's try to fix GENMASK().
>
> So, if we modify the following
>
>   #define GENMASK_INPUT_CHECK(h, l) \
>     (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
>     __builtin_constant_p((l) > (h)), (l) > (h), 0)))
>
> to be
>
>   #define GENMASK_INPUT_CHECK(h, l) \
>     (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
>     __builtin_constant_p((l) > (h)), (l) ? (l) > (h) : 0, 0)))
>
> would it work?

Actually it needs an amendment for signed types...

(l) ? (l) > (h) : !((h) > 0)

...but today is Friday night, so, mistakes are warranted :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Syed Nayyar Waris]

On Sat, May 30, 2020 at 3:13 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, May 29, 2020 at 11:07 PM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> > On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, May 29, 2020 at 11:38:18PM +0530, Syed Nayyar Waris wrote:
> > > > On Sun, May 24, 2020 at 8:15 PM kbuild test robot <lkp@intel.com> wrote:
>
> ...
>
> > > > Taking the example statement (in my patch) where compilation warning
> > > > is getting reported:
> > > > return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> > > >
> > > > 'nbits' is of type 'unsigned long'.
> > > > In above, the sanity check is comparing '0' with unsigned value. And
> > > > unsigned value can't be less than '0' ever, hence the warning.
> > > > But this warning will occur whenever there will be '0' as one of the
> > > > 'argument' and an unsigned variable as another 'argument' for GENMASK.
>
> > > Proper fix is to fix GENMASK(), but allowed workaround is to use
> > >         (BIT(nbits) - 1)
> > > instead.
>
> > When I used BIT macro (earlier), I had faced a problem. I want to tell
> > you about that.
> >
> > Inside functions 'bitmap_set_value' and 'bitmap_get_value' when nbits (or
> > clump size) is BITS_PER_LONG, unexpected calculation happens.
> >
> > Explanation:
> > Actually when nbits (clump size) is 64 (BITS_PER_LONG is 64 on my computer),
> > (BIT(nbits) - 1)
> > gives a value of zero and when this zero is ANDed with any value, it
> > makes it full zero. This is unexpected and incorrect calculation happening.
> >
> > What actually happens is in the macro expansion of BIT(64), that is 1
> > << 64, the '1' overflows from leftmost bit position (most significant
> > bit) and re-enters at the rightmost bit position (least significant
> > bit), therefore 1 << 64 becomes '0x1', and when another '1' is
> > subtracted from this, the final result becomes 0.
> >
> > Since this macro is being used in both bitmap_get_value and
> > bitmap_set_value functions, it will give unexpected results when nbits or clump
> > size is BITS_PER_LONG (32 or 64 depending on arch).
>
> I see, something like
> https://elixir.bootlin.com/linux/latest/source/include/linux/dma-mapping.h#L139
> should be done.
> But yes, let's try to fix GENMASK().
>
> So, if we modify the following
>
>   #define GENMASK_INPUT_CHECK(h, l) \
>     (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
>     __builtin_constant_p((l) > (h)), (l) > (h), 0)))
>
> to be
>
>   #define GENMASK_INPUT_CHECK(h, l) \
>     (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
>     __builtin_constant_p((l) > (h)), (l) ? (l) > (h) : 0, 0)))
>
> would it work?

Sorry Andy it is not working. Actually the warning will be thrown,
whenever there will be comparison between 'h' and 'l'. If one of them
is '0' and the other is unsigned variable.
In above, still there is comparison being done between 'h' and 'l', so
the warning is getting thrown.

>
> > William also knows about this issue:
> > "This is undefined behavior in the C standard (section 6.5.7 in the N1124)"
>
> I think it is about 6.5.7.3  here, 1U << 31 (or 63) is okay.

Actually for:
(BIT(nbits) - 1)
When nbits will be BITS_PER_LONG it will be 1U << 32 (or 64). Isn't it ?
The expression,
BIT(64) - 1
can become unexpectedly zero (incorrectly).

>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Andy Shevchenko]

On Sat, May 30, 2020 at 1:11 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> On Sat, May 30, 2020 at 3:13 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Fri, May 29, 2020 at 11:07 PM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> > > On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, May 29, 2020 at 11:38:18PM +0530, Syed Nayyar Waris wrote:
> > > > > On Sun, May 24, 2020 at 8:15 PM kbuild test robot <lkp@intel.com> wrote:
> >
> > ...
> >
> > > > > Taking the example statement (in my patch) where compilation warning
> > > > > is getting reported:
> > > > > return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> > > > >
> > > > > 'nbits' is of type 'unsigned long'.
> > > > > In above, the sanity check is comparing '0' with unsigned value. And
> > > > > unsigned value can't be less than '0' ever, hence the warning.
> > > > > But this warning will occur whenever there will be '0' as one of the
> > > > > 'argument' and an unsigned variable as another 'argument' for GENMASK.
> >
> > > > Proper fix is to fix GENMASK(), but allowed workaround is to use
> > > >         (BIT(nbits) - 1)
> > > > instead.
> >
> > > When I used BIT macro (earlier), I had faced a problem. I want to tell
> > > you about that.
> > >
> > > Inside functions 'bitmap_set_value' and 'bitmap_get_value' when nbits (or
> > > clump size) is BITS_PER_LONG, unexpected calculation happens.
> > >
> > > Explanation:
> > > Actually when nbits (clump size) is 64 (BITS_PER_LONG is 64 on my computer),
> > > (BIT(nbits) - 1)
> > > gives a value of zero and when this zero is ANDed with any value, it
> > > makes it full zero. This is unexpected and incorrect calculation happening.
> > >
> > > What actually happens is in the macro expansion of BIT(64), that is 1
> > > << 64, the '1' overflows from leftmost bit position (most significant
> > > bit) and re-enters at the rightmost bit position (least significant
> > > bit), therefore 1 << 64 becomes '0x1', and when another '1' is
> > > subtracted from this, the final result becomes 0.
> > >
> > > Since this macro is being used in both bitmap_get_value and
> > > bitmap_set_value functions, it will give unexpected results when nbits or clump
> > > size is BITS_PER_LONG (32 or 64 depending on arch).
> >
> > I see, something like
> > https://elixir.bootlin.com/linux/latest/source/include/linux/dma-mapping.h#L139
> > should be done.
> > But yes, let's try to fix GENMASK().
> >
> > So, if we modify the following
> >
> >   #define GENMASK_INPUT_CHECK(h, l) \
> >     (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> >     __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> >
> > to be
> >
> >   #define GENMASK_INPUT_CHECK(h, l) \
> >     (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> >     __builtin_constant_p((l) > (h)), (l) ? (l) > (h) : 0, 0)))
> >
> > would it work?
>
> Sorry Andy it is not working. Actually the warning will be thrown,
> whenever there will be comparison between 'h' and 'l'. If one of them
> is '0' and the other is unsigned variable.
> In above, still there is comparison being done between 'h' and 'l', so
> the warning is getting thrown.

Ah, okay

what about (l) && ((l) > (h)) ?

> > > William also knows about this issue:
> > > "This is undefined behavior in the C standard (section 6.5.7 in the N1124)"
> >
> > I think it is about 6.5.7.3  here, 1U << 31 (or 63) is okay.
>
> Actually for:
> (BIT(nbits) - 1)
> When nbits will be BITS_PER_LONG it will be 1U << 32 (or 64). Isn't it ?
> The expression,
> BIT(64) - 1
> can become unexpectedly zero (incorrectly).

Yes, that's why I pointed out to the paragraph. It's about right
operand to be "great than or equal to" the size of type of left
operand.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Syed Nayyar Waris]

On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sat, May 30, 2020 at 1:11 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> > On Sat, May 30, 2020 at 3:13 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Fri, May 29, 2020 at 11:07 PM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> > > > On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Fri, May 29, 2020 at 11:38:18PM +0530, Syed Nayyar Waris wrote:
> > > > > > On Sun, May 24, 2020 at 8:15 PM kbuild test robot <lkp@intel.com> wrote:
> > >
> > > ...
> > >
> > > > > > Taking the example statement (in my patch) where compilation warning
> > > > > > is getting reported:
> > > > > > return (map[index] >> offset) & GENMASK(nbits - 1, 0);
> > > > > >
> > > > > > 'nbits' is of type 'unsigned long'.
> > > > > > In above, the sanity check is comparing '0' with unsigned value. And
> > > > > > unsigned value can't be less than '0' ever, hence the warning.
> > > > > > But this warning will occur whenever there will be '0' as one of the
> > > > > > 'argument' and an unsigned variable as another 'argument' for GENMASK.
> > >
> > > > > Proper fix is to fix GENMASK(), but allowed workaround is to use
> > > > >         (BIT(nbits) - 1)
> > > > > instead.
> > >
> > > > When I used BIT macro (earlier), I had faced a problem. I want to tell
> > > > you about that.
> > > >
> > > > Inside functions 'bitmap_set_value' and 'bitmap_get_value' when nbits (or
> > > > clump size) is BITS_PER_LONG, unexpected calculation happens.
> > > >
> > > > Explanation:
> > > > Actually when nbits (clump size) is 64 (BITS_PER_LONG is 64 on my computer),
> > > > (BIT(nbits) - 1)
> > > > gives a value of zero and when this zero is ANDed with any value, it
> > > > makes it full zero. This is unexpected and incorrect calculation happening.
> > > >
> > > > What actually happens is in the macro expansion of BIT(64), that is 1
> > > > << 64, the '1' overflows from leftmost bit position (most significant
> > > > bit) and re-enters at the rightmost bit position (least significant
> > > > bit), therefore 1 << 64 becomes '0x1', and when another '1' is
> > > > subtracted from this, the final result becomes 0.
> > > >
> > > > Since this macro is being used in both bitmap_get_value and
> > > > bitmap_set_value functions, it will give unexpected results when nbits or clump
> > > > size is BITS_PER_LONG (32 or 64 depending on arch).
> > >
> > > I see, something like
> > > https://elixir.bootlin.com/linux/latest/source/include/linux/dma-mapping.h#L139
> > > should be done.
> > > But yes, let's try to fix GENMASK().
> > >
> > > So, if we modify the following
> > >
> > >   #define GENMASK_INPUT_CHECK(h, l) \
> > >     (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > >     __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > >
> > > to be
> > >
> > >   #define GENMASK_INPUT_CHECK(h, l) \
> > >     (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > >     __builtin_constant_p((l) > (h)), (l) ? (l) > (h) : 0, 0)))
> > >
> > > would it work?
> >
> > Sorry Andy it is not working. Actually the warning will be thrown,
> > whenever there will be comparison between 'h' and 'l'. If one of them
> > is '0' and the other is unsigned variable.
> > In above, still there is comparison being done between 'h' and 'l', so
> > the warning is getting thrown.
>
> Ah, okay
>
> what about (l) && ((l) > (h)) ?

When I finally changed:
__builtin_constant_p((l) > (h)), (l) > (h), 0)))
to:
__builtin_constant_p((l) && ((l) > (h))), (l) ? (l) > (h) : 0, 0)))

It is still throwing same compilation error at the same location where
comparison is being done between 'l' and 'h'.

Actually the short-circuit logic is not happening. For:
(l) && ((l) > (h))
Even if 'l' is zero, it still proceeds to compare 'l' and 'h' , that
is '((l) > (h))' is checked.
I think it is happening because '__builtin_constant_p' will check the
complete argument: (l) && ((l) > (h)),
'__builtin_constant_p' checks whether the argument is compile time
constant or not, so therefore, it will evaluate the WHOLE argument,
that is (including) the comparison operation.
https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html

I am still investigating more on this. Let me know if you have any suggestions.

>
> > > > William also knows about this issue:
> > > > "This is undefined behavior in the C standard (section 6.5.7 in the N1124)"
> > >
> > > I think it is about 6.5.7.3  here, 1U << 31 (or 63) is okay.
> >
> > Actually for:
> > (BIT(nbits) - 1)
> > When nbits will be BITS_PER_LONG it will be 1U << 32 (or 64). Isn't it ?
> > The expression,
> > BIT(64) - 1
> > can become unexpectedly zero (incorrectly).
>
> Yes, that's why I pointed out to the paragraph. It's about right
> operand to be "great than or equal to" the size of type of left
> operand.
>

Thank You. I understand now. :-)

Regards
Syed Nayyar Waris

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Andy Shevchenko]

On Sat, May 30, 2020 at 11:45 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:

...

> I am still investigating more on this. Let me know if you have any suggestions.

As far as I understand the start pointers are implementations of abs()
macro followed by min()/max().
I think in the latter case it's actually something which might help here.

Sorry, right now I have no time to dive deeper.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Andy Shevchenko]

On Sat, May 30, 2020 at 11:45 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:

...

> I am still investigating more on this. Let me know if you have any suggestions.

As far as I understand the start pointers are implementations of abs()
macro followed by min()/max().
I think in the latter case it's actually something which might help here.

Sorry, right now I have no time to dive deeper.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Syed Nayyar Waris]

On Sat, May 30, 2020 at 2:50 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sat, May 30, 2020 at 11:45 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> > On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
>
> ...
>
> > I am still investigating more on this. Let me know if you have any suggestions.
>
> As far as I understand the start pointers are implementations of abs()
> macro followed by min()/max().
> I think in the latter case it's actually something which might help here.
>
> Sorry, right now I have no time to dive deeper.

No Problem. Thank you for sharing your initial pointers.

By the way, as I was working on it I found a way to avoid comparison
with '0' in '__builtin_constant_p'. And because of this, no
compilation warnings are getting produced.

Change the following:

#define GENMASK_INPUT_CHECK(h, l) \
        (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
                __builtin_constant_p((l) > (h)), (l) > (h), 0)))


To this:

#if (l) == 0
#define GENMASK_INPUT_CHECK(h, l)  0
#elif
#define GENMASK_INPUT_CHECK(h, l) \
        (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
                __builtin_constant_p((l) > (h)), (l) > (h), 0)))
#endif

I have verified that this works. Basically this just avoids the sanity
check when the 'lower' bound 'l' is zero. Let me know if it looks
fine.

Regarding min, max macro that you suggested I am also looking further into it.

Regards
Syed Nayyar Waris

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Andy Shevchenko]

On Sun, May 31, 2020 at 4:11 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> On Sat, May 30, 2020 at 2:50 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sat, May 30, 2020 at 11:45 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> > > On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:

...

> #if (l) == 0
> #define GENMASK_INPUT_CHECK(h, l)  0
> #elif
> #define GENMASK_INPUT_CHECK(h, l) \
>         (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
>                 __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> #endif
>
> I have verified that this works. Basically this just avoids the sanity
> check when the 'lower' bound 'l' is zero. Let me know if it looks
> fine.

Unfortunately, it's not enough. We need to take care about the following cases
1) h or l negative;
2) h == 0, if l == 0, I dunno what is this. it's basically either 0 or warning;
3) l == 0;
4) h and l > 0.

Now, on top of that (since it's a macro) we have to keep in mind that
h and l can be signed and / or unsigned types.
And macro shall work for all 4 cases (by type signedess).

> Regarding min, max macro that you suggested I am also looking further into it.

Since this has been introduced in v5.7 and not only your code is
affected by this I think we need to ping original author either to fix
or revert.

So, I Cc'ed to the author and reviewers, because they probably know
better why that had been done in the first place and breaking existing
code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Rikard Falkeborn]

+ Emil who was working on a patch for this

On Sun, May 31, 2020 at 02:00:45PM +0300, Andy Shevchenko wrote:
> On Sun, May 31, 2020 at 4:11 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> > On Sat, May 30, 2020 at 2:50 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Sat, May 30, 2020 at 11:45 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> > > > On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> 
> ...
> 
Sorry about that, it seems it's only triggered by gcc-9, that's why I
missed it.

> > #if (l) == 0
> > #define GENMASK_INPUT_CHECK(h, l)  0
> > #elif
> > #define GENMASK_INPUT_CHECK(h, l) \
> >         (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> >                 __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > #endif
> >
> > I have verified that this works. Basically this just avoids the sanity
> > check when the 'lower' bound 'l' is zero. Let me know if it looks
> > fine.

I don't understand how you mean this? You can't use l before you have
defined GENMASK_INPUT_CHECK to take l as input? Am I missing something?

How about the following (with an added comment about why the casts are
necessary):

diff --git a/include/linux/bits.h b/include/linux/bits.h
index 4671fbf28842..5fdb9909fbff 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -23,7 +23,7 @@
 #include <linux/build_bug.h>
 #define GENMASK_INPUT_CHECK(h, l) \
        (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
-               __builtin_constant_p((l) > (h)), (l) > (h), 0)))
+               __builtin_constant_p((int)(l) > (int)(h)), (int)(l) > (int)(h), 0)))
 #else
 /*
  * BUILD_BUG_ON_ZERO is not available in h files included from asm files,

I can send a proper patch if this is ok.
> 
> Unfortunately, it's not enough. We need to take care about the following cases

The __GENMASK macro is only valid for values of h and l between 0 and 63
(or 31, if unsigned long is 32 bits). Negative values or values >=
sizeof(unsigned long) (or unsigned long long for GENMASK_ULL) result in
compiler warnings (-Wshift-count-negative or -Wshift-count-overflow). So
when I wrote the GENMASK_INPUT_CHECK macro, the intention was to catch
cases where l and h were swapped and let the existing compiler warnings
catch negative or too large values.

> 1) h or l negative;

Any of these cases will trigger a compiler warning (h negative triggers 
Wshift-count-overflow, l negative triggers Wshift-count-negative).

> 2) h == 0, if l == 0, I dunno what is this. it's basically either 0 or warning;

h == l == 0 is a complicated way of saying 1 (or BIT(0)). l negative
triggers compiler warning.

> 3) l == 0;

if h is negative, compiler warning (see 1). If h == 0, see 2. If h is
positive, there is no error in GENMASK_INPUT_CHECK.

> 4) h and l > 0.

The comparisson works as intended.

> 
> Now, on top of that (since it's a macro) we have to keep in mind that
> h and l can be signed and / or unsigned types.
> And macro shall work for all 4 cases (by type signedess).

If we cast to int, we don't need to worry about the signedness. If
someone enters a value that can't be cast to int, there will still
be a compiler warning about shift out of range.

Rikard

> > Regarding min, max macro that you suggested I am also looking further into it.
> 
> Since this has been introduced in v5.7 and not only your code is
> affected by this I think we need to ping original author either to fix
> or revert.
> 
> So, I Cc'ed to the author and reviewers, because they probably know
> better why that had been done in the first place and breaking existing
> code.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Syed Nayyar Waris]

On Mon, Jun 1, 2020 at 4:07 AM Rikard Falkeborn
<rikard.falkeborn@gmail.com> wrote:
>
> + Emil who was working on a patch for this
>
> On Sun, May 31, 2020 at 02:00:45PM +0300, Andy Shevchenko wrote:
> > On Sun, May 31, 2020 at 4:11 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> > > On Sat, May 30, 2020 at 2:50 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Sat, May 30, 2020 at 11:45 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> > > > > On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com> wrote:
> >
> > ...
> >
> Sorry about that, it seems it's only triggered by gcc-9, that's why I
> missed it.
>
> > > #if (l) == 0
> > > #define GENMASK_INPUT_CHECK(h, l)  0
> > > #elif
> > > #define GENMASK_INPUT_CHECK(h, l) \
> > >         (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > >                 __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > > #endif
> > >
> > > I have verified that this works. Basically this just avoids the sanity
> > > check when the 'lower' bound 'l' is zero. Let me know if it looks
> > > fine.
>
> I don't understand how you mean this? You can't use l before you have
> defined GENMASK_INPUT_CHECK to take l as input? Am I missing something?

Excuse me for the incorrect code snippet that I shared (above). Kindly
ignore it. I realise the error in it.

>
> How about the following (with an added comment about why the casts are
> necessary):
>
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 4671fbf28842..5fdb9909fbff 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -23,7 +23,7 @@
>  #include <linux/build_bug.h>
>  #define GENMASK_INPUT_CHECK(h, l) \
>         (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> -               __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> +               __builtin_constant_p((int)(l) > (int)(h)), (int)(l) > (int)(h), 0)))
>  #else
>  /*
>   * BUILD_BUG_ON_ZERO is not available in h files included from asm files,

Yes, the above fix is working fine. Now the compilation warning is not
getting reported.

Regards
Syed Nayyar Waris

>
> I can send a proper patch if this is ok.
> >
> > Unfortunately, it's not enough. We need to take care about the following cases
>
> The __GENMASK macro is only valid for values of h and l between 0 and 63
> (or 31, if unsigned long is 32 bits). Negative values or values >=
> sizeof(unsigned long) (or unsigned long long for GENMASK_ULL) result in
> compiler warnings (-Wshift-count-negative or -Wshift-count-overflow). So
> when I wrote the GENMASK_INPUT_CHECK macro, the intention was to catch
> cases where l and h were swapped and let the existing compiler warnings
> catch negative or too large values.
>
> > 1) h or l negative;
>
> Any of these cases will trigger a compiler warning (h negative triggers
> Wshift-count-overflow, l negative triggers Wshift-count-negative).
>
> > 2) h == 0, if l == 0, I dunno what is this. it's basically either 0 or warning;
>
> h == l == 0 is a complicated way of saying 1 (or BIT(0)). l negative
> triggers compiler warning.
>
> > 3) l == 0;
>
> if h is negative, compiler warning (see 1). If h == 0, see 2. If h is
> positive, there is no error in GENMASK_INPUT_CHECK.
>
> > 4) h and l > 0.
>
> The comparisson works as intended.
>
> >
> > Now, on top of that (since it's a macro) we have to keep in mind that
> > h and l can be signed and / or unsigned types.
> > And macro shall work for all 4 cases (by type signedess).
>
> If we cast to int, we don't need to worry about the signedness. If
> someone enters a value that can't be cast to int, there will still
> be a compiler warning about shift out of range.
>
> Rikard
>
> > > Regarding min, max macro that you suggested I am also looking further into it.
> >
> > Since this has been introduced in v5.7 and not only your code is
> > affected by this I think we need to ping original author either to fix
> > or revert.
> >
> > So, I Cc'ed to the author and reviewers, because they probably know
> > better why that had been done in the first place and breaking existing
> > code.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Andy Shevchenko]

On Mon, Jun 01, 2020 at 12:37:16AM +0200, Rikard Falkeborn wrote:
> + Emil who was working on a patch for this
> 
> On Sun, May 31, 2020 at 02:00:45PM +0300, Andy Shevchenko wrote:
> > On Sun, May 31, 2020 at 4:11 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> > > On Sat, May 30, 2020 at 2:50 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Sat, May 30, 2020 at 11:45 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> > > > > On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com> wrote:
> > 
> > ...
> > 
> Sorry about that, it seems it's only triggered by gcc-9, that's why I
> missed it.

I guess every compiler (more or less recent) will warn here.
(Sorry, there is a cut in the thread, the problem is with comparison unsigned
 type(s) to 0).

> > > #if (l) == 0
> > > #define GENMASK_INPUT_CHECK(h, l)  0
> > > #elif
> > > #define GENMASK_INPUT_CHECK(h, l) \
> > >         (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > >                 __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > > #endif
> > >
> > > I have verified that this works. Basically this just avoids the sanity
> > > check when the 'lower' bound 'l' is zero. Let me know if it looks
> > > fine.
> 
> I don't understand how you mean this? You can't use l before you have
> defined GENMASK_INPUT_CHECK to take l as input? Am I missing something?
> 
> How about the following (with an added comment about why the casts are
> necessary):
> 
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 4671fbf28842..5fdb9909fbff 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -23,7 +23,7 @@
>  #include <linux/build_bug.h>
>  #define GENMASK_INPUT_CHECK(h, l) \
>         (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> -               __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> +               __builtin_constant_p((int)(l) > (int)(h)), (int)(l) > (int)(h), 0)))
>  #else
>  /*
>   * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> 
> I can send a proper patch if this is ok.
> > 
> > Unfortunately, it's not enough. We need to take care about the following cases
> 
> The __GENMASK macro is only valid for values of h and l between 0 and 63
> (or 31, if unsigned long is 32 bits). Negative values or values >=
> sizeof(unsigned long) (or unsigned long long for GENMASK_ULL) result in
> compiler warnings (-Wshift-count-negative or -Wshift-count-overflow). So
> when I wrote the GENMASK_INPUT_CHECK macro, the intention was to catch
> cases where l and h were swapped and let the existing compiler warnings
> catch negative or too large values.

GENAMSK sometimes is used with non-constant arguments that's why your check
made a regression.

What I described below are the cases to consider w/o what should we do. What
you answered is the same what I implied. So, we are on the same page here.

> > 1) h or l negative;
> 
> Any of these cases will trigger a compiler warning (h negative triggers 
> Wshift-count-overflow, l negative triggers Wshift-count-negative).
> 
> > 2) h == 0, if l == 0, I dunno what is this. it's basically either 0 or warning;
> 
> h == l == 0 is a complicated way of saying 1 (or BIT(0)). l negative
> triggers compiler warning.

Oh, yes GENMASK(h, l), when h==l==0 should be equivalent to BIT(0) with no
warning given.

> > 3) l == 0;
> 
> if h is negative, compiler warning (see 1). If h == 0, see 2. If h is
> positive, there is no error in GENMASK_INPUT_CHECK.
> 
> > 4) h and l > 0.
> 
> The comparisson works as intended.

> > Now, on top of that (since it's a macro) we have to keep in mind that
> > h and l can be signed and / or unsigned types.
> > And macro shall work for all 4 cases (by type signedess).
> 
> If we cast to int, we don't need to worry about the signedness. If
> someone enters a value that can't be cast to int, there will still
> be a compiler warning about shift out of range.

If the argument unsigned long long will it be the warning (it should not)?

> > > Regarding min, max macro that you suggested I am also looking further into it.
> > 
> > Since this has been introduced in v5.7 and not only your code is
> > affected by this I think we need to ping original author either to fix
> > or revert.
> > 
> > So, I Cc'ed to the author and reviewers, because they probably know
> > better why that had been done in the first place and breaking existing
> > code.

Please, when you do something there, add a test case to test_bitops.c.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Rikard Falkeborn]

On Mon, Jun 01, 2020 at 11:33:30AM +0300, Andy Shevchenko wrote:
> On Mon, Jun 01, 2020 at 12:37:16AM +0200, Rikard Falkeborn wrote:
> > + Emil who was working on a patch for this
> > 
> > On Sun, May 31, 2020 at 02:00:45PM +0300, Andy Shevchenko wrote:
> > > On Sun, May 31, 2020 at 4:11 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> > > > On Sat, May 30, 2020 at 2:50 PM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:
> > > > > On Sat, May 30, 2020 at 11:45 AM Syed Nayyar Waris <syednwaris@gmail.com> wrote:
> > > > > > On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
> > > > > > <andy.shevchenko@gmail.com> wrote:
> > > 
> > > ...
> > > 
> > Sorry about that, it seems it's only triggered by gcc-9, that's why I
> > missed it.
> 
> I guess every compiler (more or less recent) will warn here.
> (Sorry, there is a cut in the thread, the problem is with comparison unsigned
>  type(s) to 0).
> 
> > > > #if (l) == 0
> > > > #define GENMASK_INPUT_CHECK(h, l)  0
> > > > #elif
> > > > #define GENMASK_INPUT_CHECK(h, l) \
> > > >         (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > > >                 __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > > > #endif
> > > >
> > > > I have verified that this works. Basically this just avoids the sanity
> > > > check when the 'lower' bound 'l' is zero. Let me know if it looks
> > > > fine.
> > 
> > I don't understand how you mean this? You can't use l before you have
> > defined GENMASK_INPUT_CHECK to take l as input? Am I missing something?
> > 
> > How about the following (with an added comment about why the casts are
> > necessary):
> > 
> > diff --git a/include/linux/bits.h b/include/linux/bits.h
> > index 4671fbf28842..5fdb9909fbff 100644
> > --- a/include/linux/bits.h
> > +++ b/include/linux/bits.h
> > @@ -23,7 +23,7 @@
> >  #include <linux/build_bug.h>
> >  #define GENMASK_INPUT_CHECK(h, l) \
> >         (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > -               __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > +               __builtin_constant_p((int)(l) > (int)(h)), (int)(l) > (int)(h), 0)))
> >  #else
> >  /*
> >   * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> > 
> > I can send a proper patch if this is ok.
> > > 
> > > Unfortunately, it's not enough. We need to take care about the following cases
> > 
> > The __GENMASK macro is only valid for values of h and l between 0 and 63
> > (or 31, if unsigned long is 32 bits). Negative values or values >=
> > sizeof(unsigned long) (or unsigned long long for GENMASK_ULL) result in
> > compiler warnings (-Wshift-count-negative or -Wshift-count-overflow). So
> > when I wrote the GENMASK_INPUT_CHECK macro, the intention was to catch
> > cases where l and h were swapped and let the existing compiler warnings
> > catch negative or too large values.
> 
> GENAMSK sometimes is used with non-constant arguments that's why your check
> made a regression.
> 
> What I described below are the cases to consider w/o what should we do. What
> you answered is the same what I implied. So, we are on the same page here.
> 
> > > 1) h or l negative;
> > 
> > Any of these cases will trigger a compiler warning (h negative triggers 
> > Wshift-count-overflow, l negative triggers Wshift-count-negative).
> > 
> > > 2) h == 0, if l == 0, I dunno what is this. it's basically either 0 or warning;
> > 
> > h == l == 0 is a complicated way of saying 1 (or BIT(0)). l negative
> > triggers compiler warning.
> 
> Oh, yes GENMASK(h, l), when h==l==0 should be equivalent to BIT(0) with no
> warning given.
> 
> > > 3) l == 0;
> > 
> > if h is negative, compiler warning (see 1). If h == 0, see 2. If h is
> > positive, there is no error in GENMASK_INPUT_CHECK.
> > 
> > > 4) h and l > 0.
> > 
> > The comparisson works as intended.
> 
> > > Now, on top of that (since it's a macro) we have to keep in mind that
> > > h and l can be signed and / or unsigned types.
> > > And macro shall work for all 4 cases (by type signedess).
> > 
> > If we cast to int, we don't need to worry about the signedness. If
> > someone enters a value that can't be cast to int, there will still
> > be a compiler warning about shift out of range.
> 
> If the argument unsigned long long will it be the warning (it should not)?

No, there should be no warning there.

The inputs to GENMASK() needs to be between 0 and 31 (or 63 depending on the
size of unsigned long). For any other values, there will be undefined behaviour,
since the operands to the shifts in __GENMASK will be too large (or negative).

Rikard

> 
> > > > Regarding min, max macro that you suggested I am also looking further into it.
> > > 
> > > Since this has been introduced in v5.7 and not only your code is
> > > affected by this I think we need to ping original author either to fix
> > > or revert.
> > > 
> > > So, I Cc'ed to the author and reviewers, because they probably know
> > > better why that had been done in the first place and breaking existing
> > > code.
> 
> Please, when you do something there, add a test case to test_bitops.c.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Andy Shevchenko]

On Tue, Jun 2, 2020 at 10:01 PM Rikard Falkeborn
<rikard.falkeborn@gmail.com> wrote:
> On Mon, Jun 01, 2020 at 11:33:30AM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 01, 2020 at 12:37:16AM +0200, Rikard Falkeborn wrote:
> > > On Sun, May 31, 2020 at 02:00:45PM +0300, Andy Shevchenko wrote:

...

> > > If we cast to int, we don't need to worry about the signedness. If
> > > someone enters a value that can't be cast to int, there will still
> > > be a compiler warning about shift out of range.
> >
> > If the argument unsigned long long will it be the warning (it should not)?
>
> No, there should be no warning there.
>
> The inputs to GENMASK() needs to be between 0 and 31 (or 63 depending on the
> size of unsigned long). For any other values, there will be undefined behaviour,
> since the operands to the shifts in __GENMASK will be too large (or negative).

What I'm implying here that argument may be not constant, and compiler
can't know their values at hand.
So, in the following snippet

foo(unsigned long long x)
{
  u32 y;
  y = GENMASK(x, 0);
}

when you cast x to int wouldn't be a warning of possible value
reduction (even if we know that it won't be higher than 63/31)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Rikard Falkeborn]

On Wed, Jun 03, 2020 at 11:49:37AM +0300, Andy Shevchenko wrote:
> On Tue, Jun 2, 2020 at 10:01 PM Rikard Falkeborn
> <rikard.falkeborn@gmail.com> wrote:
> > On Mon, Jun 01, 2020 at 11:33:30AM +0300, Andy Shevchenko wrote:
> > > On Mon, Jun 01, 2020 at 12:37:16AM +0200, Rikard Falkeborn wrote:
> > > > On Sun, May 31, 2020 at 02:00:45PM +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > > > If we cast to int, we don't need to worry about the signedness. If
> > > > someone enters a value that can't be cast to int, there will still
> > > > be a compiler warning about shift out of range.
> > >
> > > If the argument unsigned long long will it be the warning (it should not)?
> >
> > No, there should be no warning there.
> >
> > The inputs to GENMASK() needs to be between 0 and 31 (or 63 depending on the
> > size of unsigned long). For any other values, there will be undefined behaviour,
> > since the operands to the shifts in __GENMASK will be too large (or negative).
> 
> What I'm implying here that argument may be not constant, and compiler
> can't know their values at hand.
> So, in the following snippet
> 
> foo(unsigned long long x)
> {
>   u32 y;
>   y = GENMASK(x, 0);
> }
> 
> when you cast x to int wouldn't be a warning of possible value
> reduction (even if we know that it won't be higher than 63/31)?

Got it, no I was unable to trigger any warnings like that (but I still
can't reproduce to original warning, so take that with a grain of salt).
I'd be very surprised if compilers warned for explicit casts but  I'll
send a proper patch soon to let the build robot try it.

Rikard
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Andy Shevchenko]

On Thu, Jun 4, 2020 at 12:53 AM Rikard Falkeborn
<rikard.falkeborn@gmail.com> wrote:
> On Wed, Jun 03, 2020 at 11:49:37AM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 2, 2020 at 10:01 PM Rikard Falkeborn
> > <rikard.falkeborn@gmail.com> wrote:

...

> I'd be very surprised if compilers warned for explicit casts but  I'll
> send a proper patch soon to let the build robot try it.

I noticed that you should have received kbuild bot report about a
driver where it appears.

You patch broke all cases where (l) = 0 and (h) is type of unsigned
(not a const from compiler point of view).


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 1/4] bitops: Introduce the the for_each_set_clump macro

[Andy Shevchenko]

On Thu, Jun 4, 2020 at 12:58 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Jun 4, 2020 at 12:53 AM Rikard Falkeborn
> <rikard.falkeborn@gmail.com> wrote:
> > On Wed, Jun 03, 2020 at 11:49:37AM +0300, Andy Shevchenko wrote:
> > > On Tue, Jun 2, 2020 at 10:01 PM Rikard Falkeborn
> > > <rikard.falkeborn@gmail.com> wrote:
>
> ...
>
> > I'd be very surprised if compilers warned for explicit casts but  I'll
> > send a proper patch soon to let the build robot try it.
>
> I noticed that you should have received kbuild bot report about a
> driver where it appears.
>
> You patch broke all cases where (l) = 0 and (h) is type of unsigned
> (not a const from compiler point of view).

I will ask to revert for rc1 if there will be no fix.


-- 
With Best Regards,
Andy Shevchenko

[PATCH] linux/bits.h: fix unsigned less than zero warnings

[Rikard Falkeborn]

When calling the GENMASK and GENMASK_ULL macros with zero lower bit and
an unsigned unknown high bit, some gcc versions warn due to the
comparisons of the high and low bit in GENMASK_INPUT_CHECK.

To silence the warnings, cast the inputs to int before doing the
comparisons. The only valid inputs to GENMASK() and GENMASK_ULL() are
are 0 to 31 or 63. Anything outside this is undefined due to the shifts
in GENMASK()/GENMASK_ULL(). Therefore, casting the inputs to int do not
change the values for valid known inputs. For unknown values, the check
does not change anything since it's a compile-time check only.

As an example of the warning, kindly reported by the kbuild test robot:

from drivers/mfd/atmel-smc.c:11:
drivers/mfd/atmel-smc.c: In function 'atmel_smc_cs_encode_ncycles':
include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
26 |   __builtin_constant_p((l) > (h)), (l) > (h), 0)))
|                            ^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
|                                                              ^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
39 |  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
|   ^~~~~~~~~~~~~~~~~~~
>> drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro 'GENMASK'
49 |  unsigned int lsbmask = GENMASK(msbpos - 1, 0);
|                         ^~~~~~~

Fixes: 295bcca84916 ("linux/bits.h: add compile time sanity check of GENMASK inputs")
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Emil Velikov <emil.l.velikov@gmail.com>
Reported-by: Syed Nayyar Waris <syednwaris@gmail.com>
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
 include/linux/bits.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index 4671fbf28842..293d1ee71a48 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -21,9 +21,10 @@
 #if !defined(__ASSEMBLY__) && \
 	(!defined(CONFIG_CC_IS_GCC) || CONFIG_GCC_VERSION >= 49000)
 #include <linux/build_bug.h>
+/* Avoid Wtype-limits warnings by casting the inputs to int */
 #define GENMASK_INPUT_CHECK(h, l) \
 	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
-		__builtin_constant_p((l) > (h)), (l) > (h), 0)))
+		__builtin_constant_p((int)(l) > (int)(h)), (int)(l) > (int)(h), 0)))
 #else
 /*
  * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
-- 
2.27.0

[PATCH] linux/bits.h: fix unsigned less than zero warnings

[Rikard Falkeborn]

When calling the GENMASK and GENMASK_ULL macros with zero lower bit and
an unsigned unknown high bit, some gcc versions warn due to the
comparisons of the high and low bit in GENMASK_INPUT_CHECK.

To silence the warnings, cast the inputs to int before doing the
comparisons. The only valid inputs to GENMASK() and GENMASK_ULL() are
are 0 to 31 or 63. Anything outside this is undefined due to the shifts
in GENMASK()/GENMASK_ULL(). Therefore, casting the inputs to int do not
change the values for valid known inputs. For unknown values, the check
does not change anything since it's a compile-time check only.

As an example of the warning, kindly reported by the kbuild test robot:

from drivers/mfd/atmel-smc.c:11:
drivers/mfd/atmel-smc.c: In function 'atmel_smc_cs_encode_ncycles':
include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
26 |   __builtin_constant_p((l) > (h)), (l) > (h), 0)))
|                            ^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
|                                                              ^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
39 |  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
|   ^~~~~~~~~~~~~~~~~~~
>> drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro 'GENMASK'
49 |  unsigned int lsbmask = GENMASK(msbpos - 1, 0);
|                         ^~~~~~~

Fixes: 295bcca84916 ("linux/bits.h: add compile time sanity check of GENMASK inputs")
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Emil Velikov <emil.l.velikov@gmail.com>
Reported-by: Syed Nayyar Waris <syednwaris@gmail.com>
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
 include/linux/bits.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index 4671fbf28842..293d1ee71a48 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -21,9 +21,10 @@
 #if !defined(__ASSEMBLY__) && \
 	(!defined(CONFIG_CC_IS_GCC) || CONFIG_GCC_VERSION >= 49000)
 #include <linux/build_bug.h>
+/* Avoid Wtype-limits warnings by casting the inputs to int */
 #define GENMASK_INPUT_CHECK(h, l) \
 	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
-		__builtin_constant_p((l) > (h)), (l) > (h), 0)))
+		__builtin_constant_p((int)(l) > (int)(h)), (int)(l) > (int)(h), 0)))
 #else
 /*
  * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
-- 
2.27.0

Re: [PATCH] linux/bits.h: fix unsigned less than zero warnings

[Andy Shevchenko]

On Thu, Jun 4, 2020 at 1:03 AM Rikard Falkeborn
<rikard.falkeborn@gmail.com> wrote:
>
> When calling the GENMASK and GENMASK_ULL macros with zero lower bit and
> an unsigned unknown high bit, some gcc versions warn due to the
> comparisons of the high and low bit in GENMASK_INPUT_CHECK.
>
> To silence the warnings, cast the inputs to int before doing the
> comparisons. The only valid inputs to GENMASK() and GENMASK_ULL() are
> are 0 to 31 or 63. Anything outside this is undefined due to the shifts
> in GENMASK()/GENMASK_ULL(). Therefore, casting the inputs to int do not
> change the values for valid known inputs. For unknown values, the check
> does not change anything since it's a compile-time check only.
>
> As an example of the warning, kindly reported by the kbuild test robot:
>
> from drivers/mfd/atmel-smc.c:11:
> drivers/mfd/atmel-smc.c: In function 'atmel_smc_cs_encode_ncycles':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> 26 |   __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> |                            ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> 16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> |                                                              ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> 39 |  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> |   ^~~~~~~~~~~~~~~~~~~
> >> drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro 'GENMASK'
> 49 |  unsigned int lsbmask = GENMASK(msbpos - 1, 0);
> |                         ^~~~~~~
>

Thank you for the patch!

I think there is still a possibility to improve (as I mentioned there
are test cases that are absent right now).
What if we will have unsigned long value 0x100000001? Would it be 1
after casting?

Maybe cast to (long) or (long long) more appropriate?

Please, add test cases.

> Fixes: 295bcca84916 ("linux/bits.h: add compile time sanity check of GENMASK inputs")
> Reported-by: kbuild test robot <lkp@intel.com>
> Reported-by: Emil Velikov <emil.l.velikov@gmail.com>
> Reported-by: Syed Nayyar Waris <syednwaris@gmail.com>
> Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
> ---
>  include/linux/bits.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 4671fbf28842..293d1ee71a48 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -21,9 +21,10 @@
>  #if !defined(__ASSEMBLY__) && \
>         (!defined(CONFIG_CC_IS_GCC) || CONFIG_GCC_VERSION >= 49000)
>  #include <linux/build_bug.h>
> +/* Avoid Wtype-limits warnings by casting the inputs to int */
>  #define GENMASK_INPUT_CHECK(h, l) \
>         (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> -               __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> +               __builtin_constant_p((int)(l) > (int)(h)), (int)(l) > (int)(h), 0)))
>  #else
>  /*
>   * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> --
> 2.27.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] linux/bits.h: fix unsigned less than zero warnings

[Joe Perches]

On Thu, 2020-06-04 at 09:41 +0300, Andy Shevchenko wrote:
> I think there is still a possibility to improve (as I mentioned there
> are test cases that are absent right now).
> What if we will have unsigned long value 0x100000001? Would it be 1
> after casting?
> 
> Maybe cast to (long) or (long long) more appropriate?

Another good mechanism would be to compile-time check the use
of constants in BITS and BITS_ULL and verify that:

range of BITS is:
	>= 0 && < (BITS_PER_BYTE * sizeof(unsigned int))
range of BITS_ULL is:
	>= 0 && < (BITS_PER_BYTE * sizeof(unsigned long long))

There would be duplication similar to the GENMASK_INPUT_CHECK
macros.

Re: [PATCH] linux/bits.h: fix unsigned less than zero warnings

[Rikard Falkeborn]

On Thu, Jun 04, 2020 at 09:41:29AM +0300, Andy Shevchenko wrote:
> On Thu, Jun 4, 2020 at 1:03 AM Rikard Falkeborn
> <rikard.falkeborn@gmail.com> wrote:
> >
> > When calling the GENMASK and GENMASK_ULL macros with zero lower bit and
> > an unsigned unknown high bit, some gcc versions warn due to the
> > comparisons of the high and low bit in GENMASK_INPUT_CHECK.
> >
> > To silence the warnings, cast the inputs to int before doing the
> > comparisons. The only valid inputs to GENMASK() and GENMASK_ULL() are
> > are 0 to 31 or 63. Anything outside this is undefined due to the shifts
> > in GENMASK()/GENMASK_ULL(). Therefore, casting the inputs to int do not
> > change the values for valid known inputs. For unknown values, the check
> > does not change anything since it's a compile-time check only.
> >
> > As an example of the warning, kindly reported by the kbuild test robot:
> >
> > from drivers/mfd/atmel-smc.c:11:
> > drivers/mfd/atmel-smc.c: In function 'atmel_smc_cs_encode_ncycles':
> > include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> > 26 |   __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > |                            ^
> > include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> > 16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> > |                                                              ^
> > include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > 39 |  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > |   ^~~~~~~~~~~~~~~~~~~
> > >> drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro 'GENMASK'
> > 49 |  unsigned int lsbmask = GENMASK(msbpos - 1, 0);
> > |                         ^~~~~~~
> >
> 
> Thank you for the patch!
> 
> I think there is still a possibility to improve (as I mentioned there
> are test cases that are absent right now).
> What if we will have unsigned long value 0x100000001? Would it be 1
> after casting?
> 
> Maybe cast to (long) or (long long) more appropriate?

It you're entering 0x10000001 you're going to get a compiler warning
since it's going to be shifted out of range, when I wrote the check I
figured that should be enough, but perhaps I was wrong?

How about requiring both l and h bit to be constant? (that's
arguably the way I should have written in the first place). That's going
to remove the warnings for GENMASK(x, 0). It will not work as expected
when mixing negative and unsigned input, like GENMASK(-1, 0u) is not
going to fail the build while GENMASK(1u, -1) will. For GENMASK(-1, 0u),
you will get a compiler warning due to the shifts in GENMASK.

If we need to handle the negative inputs as well. I think I'd prefer to add
explicit checks for negative values over the casting. A macro for that
can probably be reused in other places as well.

> 
> Please, add test cases.

Will do.

Rikard
> 
> > Fixes: 295bcca84916 ("linux/bits.h: add compile time sanity check of GENMASK inputs")
> > Reported-by: kbuild test robot <lkp@intel.com>
> > Reported-by: Emil Velikov <emil.l.velikov@gmail.com>
> > Reported-by: Syed Nayyar Waris <syednwaris@gmail.com>
> > Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
> > ---
> >  include/linux/bits.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/bits.h b/include/linux/bits.h
> > index 4671fbf28842..293d1ee71a48 100644
> > --- a/include/linux/bits.h
> > +++ b/include/linux/bits.h
> > @@ -21,9 +21,10 @@
> >  #if !defined(__ASSEMBLY__) && \
> >         (!defined(CONFIG_CC_IS_GCC) || CONFIG_GCC_VERSION >= 49000)
> >  #include <linux/build_bug.h>
> > +/* Avoid Wtype-limits warnings by casting the inputs to int */
> >  #define GENMASK_INPUT_CHECK(h, l) \
> >         (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > -               __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> > +               __builtin_constant_p((int)(l) > (int)(h)), (int)(l) > (int)(h), 0)))
> >  #else
> >  /*
> >   * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> > --
> > 2.27.0
> >
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

[PATCH v2 1/2] linux/bits.h: fix unsigned less than zero warnings

[Rikard Falkeborn]

When calling the GENMASK and GENMASK_ULL macros with zero lower bit and
an unsigned unknown high bit, some gcc versions warn due to the
comparisons of the high and low bit in GENMASK_INPUT_CHECK.

To silence the warnings, only perform the check if both inputs are
known. This does not trigger any warnings, from the Wtype-limits help:

	Warn if a comparison is always true or always false due to the
	limited range of the data type, but do not warn for constant
	expressions.

As an example of the warning, kindly reported by the kbuild test robot:

from drivers/mfd/atmel-smc.c:11:
drivers/mfd/atmel-smc.c: In function 'atmel_smc_cs_encode_ncycles':
include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
26 |   __builtin_constant_p((l) > (h)), (l) > (h), 0)))
|                            ^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
|                                                              ^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
39 |  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
|   ^~~~~~~~~~~~~~~~~~~
>> drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro 'GENMASK'
49 |  unsigned int lsbmask = GENMASK(msbpos - 1, 0);
|                         ^~~~~~~

Fixes: 295bcca84916 ("linux/bits.h: add compile time sanity check of GENMASK inputs")
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Emil Velikov <emil.l.velikov@gmail.com>
Reported-by: Syed Nayyar Waris <syednwaris@gmail.com>
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
v1->v2
* Change to require both high and low bit to be constant expressions
  instead of introducing somewhat arbitrary casts

 include/linux/bits.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index 4671fbf28842..35ca3f5d11a0 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -23,7 +23,8 @@
 #include <linux/build_bug.h>
 #define GENMASK_INPUT_CHECK(h, l) \
 	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
-		__builtin_constant_p((l) > (h)), (l) > (h), 0)))
+		__builtin_constant_p(l) && __builtin_constant_p(h), \
+		(l) > (h), 0)))
 #else
 /*
  * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
-- 
2.27.0

[PATCH v2 2/2] bits: Add tests of GENMASK

[Rikard Falkeborn]

Add tests of GENMASK and GENMASK_ULL.

A few test cases that should fail compilation are provided under ifdef.

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
v1-v2
* New patch. First time I wrote a KUnittest so may be room for
  improvements...

 lib/Kconfig.debug | 11 +++++++
 lib/Makefile      |  1 +
 lib/test_bits.c   | 73 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+)
 create mode 100644 lib/test_bits.c

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 9bd4eb7f5ec1..1b28ef6bb081 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2170,6 +2170,17 @@ config LINEAR_RANGES_TEST
 
 	  If unsure, say N.
 
+config BITS_TEST
+	tristate "KUnit test for bits.h"
+	depends on KUNIT
+	help
+	  This builds the bits unit test.
+	  Tests the logic of macros defined in bits.h.
+	  For more information on KUnit and unit tests in general please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+	  If unsure, say N.
+
 config TEST_UDELAY
 	tristate "udelay test driver"
 	help
diff --git a/lib/Makefile b/lib/Makefile
index 32f19b4d1d2a..77673af9dd11 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -314,3 +314,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o
 # KUnit tests
 obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
 obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
+obj-$(CONFIG_BITS_TEST) += test_bits.o
diff --git a/lib/test_bits.c b/lib/test_bits.c
new file mode 100644
index 000000000000..5bd7a0ef0a3b
--- /dev/null
+++ b/lib/test_bits.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test cases for functions and macrso in bits.h
+ */
+
+#include <kunit/test.h>
+#include <linux/bits.h>
+
+
+void genmask_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 1ul, GENMASK(0, 0));
+	KUNIT_EXPECT_EQ(test, 3ul, GENMASK(1, 0));
+	KUNIT_EXPECT_EQ(test, 6ul, GENMASK(2, 1));
+	KUNIT_EXPECT_EQ(test, 0xFFFFFFFFul, GENMASK(31, 0));
+
+#ifdef TEST_BITS_COMPILE
+	/* these should fail compilation */
+	GENMASK(0, 1);
+	GENMASK(0, 10);
+	GENMASK(9, 10);
+#endif
+
+
+}
+
+void genmask_ull_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 1ull, GENMASK_ULL(0, 0));
+	KUNIT_EXPECT_EQ(test, 3ull, GENMASK_ULL(1, 0));
+	KUNIT_EXPECT_EQ(test, 0x000000ffffe00000ull, GENMASK_ULL(39, 21));
+	KUNIT_EXPECT_EQ(test, 0xffffffffffffffffull, GENMASK_ULL(63, 0));
+
+#ifdef TEST_BITS_COMPILE
+	/* these should fail compilation */
+	GENMASK_ULL(0, 1);
+	GENMASK_ULL(0, 10);
+	GENMASK_ULL(9, 10);
+#endif
+}
+
+void genmask_input_check_test(struct kunit *test)
+{
+	unsigned int x, y;
+	int z, w;
+
+	/* Unknown input */
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(x, 0));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(0, x));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(x, y));
+
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(z, 0));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(0, z));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(z, w));
+
+	/* Valid input */
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(1, 1));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(39, 21));
+}
+
+
+static struct kunit_case bits_test_cases[] = {
+	KUNIT_CASE(genmask_test),
+	KUNIT_CASE(genmask_ull_test),
+	KUNIT_CASE(genmask_input_check_test),
+	{}
+};
+
+static struct kunit_suite bits_test_suite = {
+	.name = "bits-test",
+	.test_cases = bits_test_cases,
+};
+kunit_test_suite(bits_test_suite);
-- 
2.27.0

Re: [PATCH v2 2/2] bits: Add tests of GENMASK

[Geert Uytterhoeven]

Hi Richard,

Thanks for your patch!

On Sun, Jun 7, 2020 at 10:35 PM Rikard Falkeborn
<rikard.falkeborn@gmail.com> wrote:
> Add tests of GENMASK and GENMASK_ULL.
>
> A few test cases that should fail compilation are provided under ifdef.

It doesn't hurt to mention the name of the #ifdef here.

> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>

> --- /dev/null
> +++ b/lib/test_bits.c
> @@ -0,0 +1,73 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Test cases for functions and macrso in bits.h
> + */
> +
> +#include <kunit/test.h>
> +#include <linux/bits.h>
> +
> +
> +void genmask_test(struct kunit *test)
> +{
> +       KUNIT_EXPECT_EQ(test, 1ul, GENMASK(0, 0));
> +       KUNIT_EXPECT_EQ(test, 3ul, GENMASK(1, 0));
> +       KUNIT_EXPECT_EQ(test, 6ul, GENMASK(2, 1));
> +       KUNIT_EXPECT_EQ(test, 0xFFFFFFFFul, GENMASK(31, 0));
> +
> +#ifdef TEST_BITS_COMPILE

"#ifdef TEST_GENMASK_FAILURES"?

> +       /* these should fail compilation */
> +       GENMASK(0, 1);
> +       GENMASK(0, 10);
> +       GENMASK(9, 10);
> +#endif

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] bits: Add tests of GENMASK

[Rikard Falkeborn]

On Mon, Jun 08, 2020 at 09:33:05AM +0200, Geert Uytterhoeven wrote:
> Hi Richard,
> 
> Thanks for your patch!
> 
> On Sun, Jun 7, 2020 at 10:35 PM Rikard Falkeborn
> <rikard.falkeborn@gmail.com> wrote:
> > Add tests of GENMASK and GENMASK_ULL.
> >
> > A few test cases that should fail compilation are provided under ifdef.
> 
> It doesn't hurt to mention the name of the #ifdef here.
> 
> > Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
> 
> > --- /dev/null
> > +++ b/lib/test_bits.c
> > @@ -0,0 +1,73 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Test cases for functions and macrso in bits.h
> > + */
> > +
> > +#include <kunit/test.h>
> > +#include <linux/bits.h>
> > +
> > +
> > +void genmask_test(struct kunit *test)
> > +{
> > +       KUNIT_EXPECT_EQ(test, 1ul, GENMASK(0, 0));
> > +       KUNIT_EXPECT_EQ(test, 3ul, GENMASK(1, 0));
> > +       KUNIT_EXPECT_EQ(test, 6ul, GENMASK(2, 1));
> > +       KUNIT_EXPECT_EQ(test, 0xFFFFFFFFul, GENMASK(31, 0));
> > +
> > +#ifdef TEST_BITS_COMPILE
> 
> "#ifdef TEST_GENMASK_FAILURES"?

Much better! I'll update that (and add the ifdef to the commit message)
for v3.

Thanks

Rikard
> 
> > +       /* these should fail compilation */
> > +       GENMASK(0, 1);
> > +       GENMASK(0, 10);
> > +       GENMASK(9, 10);
> > +#endif
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

[PATCH v3 1/2] linux/bits.h: fix unsigned less than zero warnings

[Rikard Falkeborn]

When calling the GENMASK and GENMASK_ULL macros with zero lower bit and
an unsigned unknown high bit, some gcc versions warn due to the
comparisons of the high and low bit in GENMASK_INPUT_CHECK.

To silence the warnings, only perform the check if both inputs are
known. This does not trigger any warnings, from the Wtype-limits help:

	Warn if a comparison is always true or always false due to the
	limited range of the data type, but do not warn for constant
	expressions.

As an example of the warning, kindly reported by the kbuild test robot:

from drivers/mfd/atmel-smc.c:11:
drivers/mfd/atmel-smc.c: In function 'atmel_smc_cs_encode_ncycles':
include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
26 |   __builtin_constant_p((l) > (h)), (l) > (h), 0)))
|                            ^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
|                                                              ^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
39 |  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
|   ^~~~~~~~~~~~~~~~~~~
>> drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro 'GENMASK'
49 |  unsigned int lsbmask = GENMASK(msbpos - 1, 0);
|                         ^~~~~~~

Fixes: 295bcca84916 ("linux/bits.h: add compile time sanity check of GENMASK inputs")
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Emil Velikov <emil.l.velikov@gmail.com>
Reported-by: Syed Nayyar Waris <syednwaris@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
v2-v3
Added Andys Reviewed-by.

v1->v2
Change to require both high and low bit to be constant expressions
instead of introducing somewhat arbitrary casts

 include/linux/bits.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index 4671fbf28842..35ca3f5d11a0 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -23,7 +23,8 @@
 #include <linux/build_bug.h>
 #define GENMASK_INPUT_CHECK(h, l) \
 	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
-		__builtin_constant_p((l) > (h)), (l) > (h), 0)))
+		__builtin_constant_p(l) && __builtin_constant_p(h), \
+		(l) > (h), 0)))
 #else
 /*
  * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
-- 
2.27.0

[PATCH v3 2/2] bits: Add tests of GENMASK

[Rikard Falkeborn]

Add tests of GENMASK and GENMASK_ULL.

A few test cases that should fail compilation are provided
under #ifdef TEST_GENMASK_FAILURES

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
I did not move it to test_bitops.c, because I think it makes more sense
that test_bitops.c tests bitops.h and test_bits.c tests bits.h, but if
you disagree, I can move it.

v2-v3
Updated commit message and ifdef after suggestion fron Geert. Also fixed
a typo in the description of the file.

v1-v2
New patch.

 lib/Kconfig.debug | 11 +++++++
 lib/Makefile      |  1 +
 lib/test_bits.c   | 73 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+)
 create mode 100644 lib/test_bits.c

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 333e878d8af9..9557cb570fb9 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2182,6 +2182,17 @@ config LINEAR_RANGES_TEST
 
 	  If unsure, say N.
 
+config BITS_TEST
+	tristate "KUnit test for bits.h"
+	depends on KUNIT
+	help
+	  This builds the bits unit test.
+	  Tests the logic of macros defined in bits.h.
+	  For more information on KUnit and unit tests in general please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+	  If unsure, say N.
+
 config TEST_UDELAY
 	tristate "udelay test driver"
 	help
diff --git a/lib/Makefile b/lib/Makefile
index 315516fa4ef4..2ce9892e3e63 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -314,3 +314,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o
 # KUnit tests
 obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
 obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
+obj-$(CONFIG_BITS_TEST) += test_bits.o
diff --git a/lib/test_bits.c b/lib/test_bits.c
new file mode 100644
index 000000000000..e2fcf24463bf
--- /dev/null
+++ b/lib/test_bits.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test cases for functions and macros in bits.h
+ */
+
+#include <kunit/test.h>
+#include <linux/bits.h>
+
+
+void genmask_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 1ul, GENMASK(0, 0));
+	KUNIT_EXPECT_EQ(test, 3ul, GENMASK(1, 0));
+	KUNIT_EXPECT_EQ(test, 6ul, GENMASK(2, 1));
+	KUNIT_EXPECT_EQ(test, 0xFFFFFFFFul, GENMASK(31, 0));
+
+#ifdef TEST_GENMASK_FAILURES
+	/* these should fail compilation */
+	GENMASK(0, 1);
+	GENMASK(0, 10);
+	GENMASK(9, 10);
+#endif
+
+
+}
+
+void genmask_ull_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 1ull, GENMASK_ULL(0, 0));
+	KUNIT_EXPECT_EQ(test, 3ull, GENMASK_ULL(1, 0));
+	KUNIT_EXPECT_EQ(test, 0x000000ffffe00000ull, GENMASK_ULL(39, 21));
+	KUNIT_EXPECT_EQ(test, 0xffffffffffffffffull, GENMASK_ULL(63, 0));
+
+#ifdef TEST_GENMASK_FAILURES
+	/* these should fail compilation */
+	GENMASK_ULL(0, 1);
+	GENMASK_ULL(0, 10);
+	GENMASK_ULL(9, 10);
+#endif
+}
+
+void genmask_input_check_test(struct kunit *test)
+{
+	unsigned int x, y;
+	int z, w;
+
+	/* Unknown input */
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(x, 0));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(0, x));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(x, y));
+
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(z, 0));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(0, z));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(z, w));
+
+	/* Valid input */
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(1, 1));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(39, 21));
+}
+
+
+static struct kunit_case bits_test_cases[] = {
+	KUNIT_CASE(genmask_test),
+	KUNIT_CASE(genmask_ull_test),
+	KUNIT_CASE(genmask_input_check_test),
+	{}
+};
+
+static struct kunit_suite bits_test_suite = {
+	.name = "bits-test",
+	.test_cases = bits_test_cases,
+};
+kunit_test_suite(bits_test_suite);
-- 
2.27.0

Re: [PATCH v3 2/2] bits: Add tests of GENMASK

[Andy Shevchenko]

On Tue, Jun 9, 2020 at 1:18 AM Rikard Falkeborn
<rikard.falkeborn@gmail.com> wrote:
>
> Add tests of GENMASK and GENMASK_ULL.
>
> A few test cases that should fail compilation are provided
> under #ifdef TEST_GENMASK_FAILURES
>

LGTM, thanks!
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
> ---
> I did not move it to test_bitops.c, because I think it makes more sense
> that test_bitops.c tests bitops.h and test_bits.c tests bits.h, but if
> you disagree, I can move it.

We could do it later and actually other way around, since you are
using KUnit, while the test_bitops.h doesn't.

>
> v2-v3
> Updated commit message and ifdef after suggestion fron Geert. Also fixed
> a typo in the description of the file.
>
> v1-v2
> New patch.
>
>  lib/Kconfig.debug | 11 +++++++
>  lib/Makefile      |  1 +
>  lib/test_bits.c   | 73 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 85 insertions(+)
>  create mode 100644 lib/test_bits.c
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 333e878d8af9..9557cb570fb9 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2182,6 +2182,17 @@ config LINEAR_RANGES_TEST
>
>           If unsure, say N.
>
> +config BITS_TEST
> +       tristate "KUnit test for bits.h"
> +       depends on KUNIT
> +       help
> +         This builds the bits unit test.
> +         Tests the logic of macros defined in bits.h.
> +         For more information on KUnit and unit tests in general please refer
> +         to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> +         If unsure, say N.
> +
>  config TEST_UDELAY
>         tristate "udelay test driver"
>         help
> diff --git a/lib/Makefile b/lib/Makefile
> index 315516fa4ef4..2ce9892e3e63 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -314,3 +314,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o
>  # KUnit tests
>  obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
>  obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
> +obj-$(CONFIG_BITS_TEST) += test_bits.o
> diff --git a/lib/test_bits.c b/lib/test_bits.c
> new file mode 100644
> index 000000000000..e2fcf24463bf
> --- /dev/null
> +++ b/lib/test_bits.c
> @@ -0,0 +1,73 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Test cases for functions and macros in bits.h
> + */
> +
> +#include <kunit/test.h>
> +#include <linux/bits.h>
> +
> +
> +void genmask_test(struct kunit *test)
> +{
> +       KUNIT_EXPECT_EQ(test, 1ul, GENMASK(0, 0));
> +       KUNIT_EXPECT_EQ(test, 3ul, GENMASK(1, 0));
> +       KUNIT_EXPECT_EQ(test, 6ul, GENMASK(2, 1));
> +       KUNIT_EXPECT_EQ(test, 0xFFFFFFFFul, GENMASK(31, 0));
> +
> +#ifdef TEST_GENMASK_FAILURES
> +       /* these should fail compilation */
> +       GENMASK(0, 1);
> +       GENMASK(0, 10);
> +       GENMASK(9, 10);
> +#endif
> +
> +
> +}
> +
> +void genmask_ull_test(struct kunit *test)
> +{
> +       KUNIT_EXPECT_EQ(test, 1ull, GENMASK_ULL(0, 0));
> +       KUNIT_EXPECT_EQ(test, 3ull, GENMASK_ULL(1, 0));
> +       KUNIT_EXPECT_EQ(test, 0x000000ffffe00000ull, GENMASK_ULL(39, 21));
> +       KUNIT_EXPECT_EQ(test, 0xffffffffffffffffull, GENMASK_ULL(63, 0));
> +
> +#ifdef TEST_GENMASK_FAILURES
> +       /* these should fail compilation */
> +       GENMASK_ULL(0, 1);
> +       GENMASK_ULL(0, 10);
> +       GENMASK_ULL(9, 10);
> +#endif
> +}
> +
> +void genmask_input_check_test(struct kunit *test)
> +{
> +       unsigned int x, y;
> +       int z, w;
> +
> +       /* Unknown input */
> +       KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(x, 0));
> +       KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(0, x));
> +       KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(x, y));
> +
> +       KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(z, 0));
> +       KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(0, z));
> +       KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(z, w));
> +
> +       /* Valid input */
> +       KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(1, 1));
> +       KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(39, 21));
> +}
> +
> +
> +static struct kunit_case bits_test_cases[] = {
> +       KUNIT_CASE(genmask_test),
> +       KUNIT_CASE(genmask_ull_test),
> +       KUNIT_CASE(genmask_input_check_test),
> +       {}
> +};
> +
> +static struct kunit_suite bits_test_suite = {
> +       .name = "bits-test",
> +       .test_cases = bits_test_cases,
> +};
> +kunit_test_suite(bits_test_suite);
> --
> 2.27.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] bits: Add tests of GENMASK

[Andrew Morton]

On Tue,  9 Jun 2020 00:18:23 +0200 Rikard Falkeborn <rikard.falkeborn@gmail.com> wrote:

> Add tests of GENMASK and GENMASK_ULL.
> 
> A few test cases that should fail compilation are provided
> under #ifdef TEST_GENMASK_FAILURES

WARNING: modpost: missing MODULE_LICENSE() in lib/test_bits.o


Could you please send a fix?

[PATCH v4 1/2] linux/bits.h: fix unsigned less than zero warnings

[Rikard Falkeborn]

When calling the GENMASK and GENMASK_ULL macros with zero lower bit and
an unsigned unknown high bit, some gcc versions warn due to the
comparisons of the high and low bit in GENMASK_INPUT_CHECK.

To silence the warnings, only perform the check if both inputs are
known. This does not trigger any warnings, from the Wtype-limits help:

	Warn if a comparison is always true or always false due to the
	limited range of the data type, but do not warn for constant
	expressions.

As an example of the warning, kindly reported by the kbuild test robot:

from drivers/mfd/atmel-smc.c:11:
drivers/mfd/atmel-smc.c: In function 'atmel_smc_cs_encode_ncycles':
include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
26 |   __builtin_constant_p((l) > (h)), (l) > (h), 0)))
|                            ^
include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
|                                                              ^
include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
39 |  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
|   ^~~~~~~~~~~~~~~~~~~
>> drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro 'GENMASK'
49 |  unsigned int lsbmask = GENMASK(msbpos - 1, 0);
|                         ^~~~~~~

Fixes: 295bcca84916 ("linux/bits.h: add compile time sanity check of GENMASK inputs")
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Emil Velikov <emil.l.velikov@gmail.com>
Reported-by: Syed Nayyar Waris <syednwaris@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
v3-v4
Added Emils Reviewed-by.

v2-v3
Added Andys Reviewed-by.

v1->v2
Change to require both high and low bit to be constant expressions
instead of introducing somewhat arbitrary casts

 include/linux/bits.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index 4671fbf28842..35ca3f5d11a0 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -23,7 +23,8 @@
 #include <linux/build_bug.h>
 #define GENMASK_INPUT_CHECK(h, l) \
 	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
-		__builtin_constant_p((l) > (h)), (l) > (h), 0)))
+		__builtin_constant_p(l) && __builtin_constant_p(h), \
+		(l) > (h), 0)))
 #else
 /*
  * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
-- 
2.27.0

[PATCH v4 2/2] bits: Add tests of GENMASK

[Rikard Falkeborn]

Add tests of GENMASK and GENMASK_ULL.

A few test cases that should fail compilation are provided
under #ifdef TEST_GENMASK_FAILURES

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
Sorry about the missing MODULE_LICENSE. I assume you just will drop v3
and use this instead, or should I have sent a patch with only
MODULE_LICENSE added?

v3-v4
Added missing MODULE_LICENSE.

v2-v3
Updated commit message and ifdef after suggestion fron Geert. Also fixed
a typo in the description of the file.

v1-v2
New patch.


 lib/Kconfig.debug | 11 +++++++
 lib/Makefile      |  1 +
 lib/test_bits.c   | 75 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 lib/test_bits.c

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index d74ac0fd6b2d..628097773b13 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2186,6 +2186,17 @@ config LINEAR_RANGES_TEST
 
 	  If unsure, say N.
 
+config BITS_TEST
+	tristate "KUnit test for bits.h"
+	depends on KUNIT
+	help
+	  This builds the bits unit test.
+	  Tests the logic of macros defined in bits.h.
+	  For more information on KUnit and unit tests in general please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+	  If unsure, say N.
+
 config TEST_UDELAY
 	tristate "udelay test driver"
 	help
diff --git a/lib/Makefile b/lib/Makefile
index b1c42c10073b..d157f6c980f7 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -318,3 +318,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o
 # KUnit tests
 obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
 obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
+obj-$(CONFIG_BITS_TEST) += test_bits.o
diff --git a/lib/test_bits.c b/lib/test_bits.c
new file mode 100644
index 000000000000..89e0ea83511f
--- /dev/null
+++ b/lib/test_bits.c
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test cases for functions and macros in bits.h
+ */
+
+#include <kunit/test.h>
+#include <linux/bits.h>
+
+
+void genmask_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 1ul, GENMASK(0, 0));
+	KUNIT_EXPECT_EQ(test, 3ul, GENMASK(1, 0));
+	KUNIT_EXPECT_EQ(test, 6ul, GENMASK(2, 1));
+	KUNIT_EXPECT_EQ(test, 0xFFFFFFFFul, GENMASK(31, 0));
+
+#ifdef TEST_GENMASK_FAILURES
+	/* these should fail compilation */
+	GENMASK(0, 1);
+	GENMASK(0, 10);
+	GENMASK(9, 10);
+#endif
+
+
+}
+
+void genmask_ull_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 1ull, GENMASK_ULL(0, 0));
+	KUNIT_EXPECT_EQ(test, 3ull, GENMASK_ULL(1, 0));
+	KUNIT_EXPECT_EQ(test, 0x000000ffffe00000ull, GENMASK_ULL(39, 21));
+	KUNIT_EXPECT_EQ(test, 0xffffffffffffffffull, GENMASK_ULL(63, 0));
+
+#ifdef TEST_GENMASK_FAILURES
+	/* these should fail compilation */
+	GENMASK_ULL(0, 1);
+	GENMASK_ULL(0, 10);
+	GENMASK_ULL(9, 10);
+#endif
+}
+
+void genmask_input_check_test(struct kunit *test)
+{
+	unsigned int x, y;
+	int z, w;
+
+	/* Unknown input */
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(x, 0));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(0, x));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(x, y));
+
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(z, 0));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(0, z));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(z, w));
+
+	/* Valid input */
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(1, 1));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(39, 21));
+}
+
+
+static struct kunit_case bits_test_cases[] = {
+	KUNIT_CASE(genmask_test),
+	KUNIT_CASE(genmask_ull_test),
+	KUNIT_CASE(genmask_input_check_test),
+	{}
+};
+
+static struct kunit_suite bits_test_suite = {
+	.name = "bits-test",
+	.test_cases = bits_test_cases,
+};
+kunit_test_suite(bits_test_suite);
+
+MODULE_LICENSE("GPL");
-- 
2.27.0

[PATCH v4 2/2] bits: Add tests of GENMASK

[Rikard Falkeborn]

Add tests of GENMASK and GENMASK_ULL.

A few test cases that should fail compilation are provided
under #ifdef TEST_GENMASK_FAILURES

Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
Sorry about the missing MODULE_LICENSE. I assume you just will drop v3
and use this instead, or should I have sent a patch with only
MODULE_LICENSE added?

v3-v4
Added missing MODULE_LICENSE.

v2-v3
Updated commit message and ifdef after suggestion fron Geert. Also fixed
a typo in the description of the file.

v1-v2
New patch.


 lib/Kconfig.debug | 11 +++++++
 lib/Makefile      |  1 +
 lib/test_bits.c   | 75 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+)
 create mode 100644 lib/test_bits.c

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index d74ac0fd6b2d..628097773b13 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2186,6 +2186,17 @@ config LINEAR_RANGES_TEST
 
 	  If unsure, say N.
 
+config BITS_TEST
+	tristate "KUnit test for bits.h"
+	depends on KUNIT
+	help
+	  This builds the bits unit test.
+	  Tests the logic of macros defined in bits.h.
+	  For more information on KUnit and unit tests in general please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+	  If unsure, say N.
+
 config TEST_UDELAY
 	tristate "udelay test driver"
 	help
diff --git a/lib/Makefile b/lib/Makefile
index b1c42c10073b..d157f6c980f7 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -318,3 +318,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o
 # KUnit tests
 obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
 obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
+obj-$(CONFIG_BITS_TEST) += test_bits.o
diff --git a/lib/test_bits.c b/lib/test_bits.c
new file mode 100644
index 000000000000..89e0ea83511f
--- /dev/null
+++ b/lib/test_bits.c
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Test cases for functions and macros in bits.h
+ */
+
+#include <kunit/test.h>
+#include <linux/bits.h>
+
+
+void genmask_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 1ul, GENMASK(0, 0));
+	KUNIT_EXPECT_EQ(test, 3ul, GENMASK(1, 0));
+	KUNIT_EXPECT_EQ(test, 6ul, GENMASK(2, 1));
+	KUNIT_EXPECT_EQ(test, 0xFFFFFFFFul, GENMASK(31, 0));
+
+#ifdef TEST_GENMASK_FAILURES
+	/* these should fail compilation */
+	GENMASK(0, 1);
+	GENMASK(0, 10);
+	GENMASK(9, 10);
+#endif
+
+
+}
+
+void genmask_ull_test(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 1ull, GENMASK_ULL(0, 0));
+	KUNIT_EXPECT_EQ(test, 3ull, GENMASK_ULL(1, 0));
+	KUNIT_EXPECT_EQ(test, 0x000000ffffe00000ull, GENMASK_ULL(39, 21));
+	KUNIT_EXPECT_EQ(test, 0xffffffffffffffffull, GENMASK_ULL(63, 0));
+
+#ifdef TEST_GENMASK_FAILURES
+	/* these should fail compilation */
+	GENMASK_ULL(0, 1);
+	GENMASK_ULL(0, 10);
+	GENMASK_ULL(9, 10);
+#endif
+}
+
+void genmask_input_check_test(struct kunit *test)
+{
+	unsigned int x, y;
+	int z, w;
+
+	/* Unknown input */
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(x, 0));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(0, x));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(x, y));
+
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(z, 0));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(0, z));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(z, w));
+
+	/* Valid input */
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(1, 1));
+	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(39, 21));
+}
+
+
+static struct kunit_case bits_test_cases[] = {
+	KUNIT_CASE(genmask_test),
+	KUNIT_CASE(genmask_ull_test),
+	KUNIT_CASE(genmask_input_check_test),
+	{}
+};
+
+static struct kunit_suite bits_test_suite = {
+	.name = "bits-test",
+	.test_cases = bits_test_cases,
+};
+kunit_test_suite(bits_test_suite);
+
+MODULE_LICENSE("GPL");
-- 
2.27.0

Re: [PATCH v4 2/2] bits: Add tests of GENMASK

[Nico Pache]

Hey,

Where is TEST_GENMASK_FAILURES being defined? This fails when compiling this

test as a module.

Am I missing something here?

Cheers!

-- Nico

On 6/21/20 1:42 AM, Rikard Falkeborn wrote:
> [Snip...] 
> +#ifdef TEST_GENMASK_FAILURES
> +	/* these should fail compilation */
> +	GENMASK(0, 1);
> +	GENMASK(0, 10);
> +	GENMASK(9, 10);
> +#endif
> [Snap..] 

Re: [PATCH v4 2/2] bits: Add tests of GENMASK

[Nico Pache]

I was missing something... This was not my issue.

Sorry for the noise!

-- Nico

On 4/22/21 3:40 PM, Nico Pache wrote:
> Hey,
>
> Where is TEST_GENMASK_FAILURES being defined? This fails when compiling this
>
> test as a module.
>
> Am I missing something here?
>
> Cheers!
>
> -- Nico
>
> On 6/21/20 1:42 AM, Rikard Falkeborn wrote:
>> [Snip...] 
>> +#ifdef TEST_GENMASK_FAILURES
>> +	/* these should fail compilation */
>> +	GENMASK(0, 1);
>> +	GENMASK(0, 10);
>> +	GENMASK(9, 10);
>> +#endif
>> [Snap..] 
>

Re: [PATCH v4 1/2] linux/bits.h: fix unsigned less than zero warnings

[Herbert Xu]

Rikard Falkeborn <rikard.falkeborn@gmail.com> wrote:
> When calling the GENMASK and GENMASK_ULL macros with zero lower bit and
> an unsigned unknown high bit, some gcc versions warn due to the
> comparisons of the high and low bit in GENMASK_INPUT_CHECK.
> 
> To silence the warnings, only perform the check if both inputs are
> known. This does not trigger any warnings, from the Wtype-limits help:
> 
>        Warn if a comparison is always true or always false due to the
>        limited range of the data type, but do not warn for constant
>        expressions.
> 
> As an example of the warning, kindly reported by the kbuild test robot:
> 
> from drivers/mfd/atmel-smc.c:11:
> drivers/mfd/atmel-smc.c: In function 'atmel_smc_cs_encode_ncycles':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> 26 |   __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> |                            ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> 16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> |                                                              ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> 39 |  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> |   ^~~~~~~~~~~~~~~~~~~
>>> drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro 'GENMASK'
> 49 |  unsigned int lsbmask = GENMASK(msbpos - 1, 0);
> |                         ^~~~~~~
> 
> Fixes: 295bcca84916 ("linux/bits.h: add compile time sanity check of GENMASK inputs")
> Reported-by: kbuild test robot <lkp@intel.com>
> Reported-by: Emil Velikov <emil.l.velikov@gmail.com>
> Reported-by: Syed Nayyar Waris <syednwaris@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
> Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
> ---
> v3-v4
> Added Emils Reviewed-by.

I'm still getting the same warning even with the patch, for example:

  CC [M]  drivers/crypto/inside-secure/safexcel.o
  CHECK   ../drivers/crypto/inside-secure/safexcel_hash.c
In file included from ../include/linux/bits.h:23,
                 from ../include/linux/bitops.h:5,
                 from ../include/linux/kernel.h:12,
                 from ../include/linux/clk.h:13,
                 from ../drivers/crypto/inside-secure/safexcel.c:8:
../drivers/crypto/inside-secure/safexcel.c: In function \u2018safexcel_hw_init\u2019:
../include/linux/bits.h:27:7: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
   (l) > (h), 0)))
       ^
../include/linux/build_bug.h:16:62: note: in definition of macro \u2018BUILD_BUG_ON_ZERO\u2019
 #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
                                                              ^
../include/linux/bits.h:40:3: note: in expansion of macro \u2018GENMASK_INPUT_CHECK\u2019
  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
   ^~~~~~~~~~~~~~~~~~~
../drivers/crypto/inside-secure/safexcel.c:649:11: note: in expansion of macro \u2018GENMASK\u2019
           GENMASK(priv->config.rings - 1, 0),
           ^~~~~~~
../include/linux/bits.h:27:7: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
   (l) > (h), 0)))
       ^
../include/linux/build_bug.h:16:62: note: in definition of macro \u2018BUILD_BUG_ON_ZERO\u2019
 #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
                                                              ^
../include/linux/bits.h:40:3: note: in expansion of macro \u2018GENMASK_INPUT_CHECK\u2019
  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
   ^~~~~~~~~~~~~~~~~~~
../drivers/crypto/inside-secure/safexcel.c:757:35: note: in expansion of macro \u2018GENMASK\u2019
   writel(EIP197_DxE_THR_CTRL_EN | GENMASK(priv->config.rings - 1, 0),
                                   ^~~~~~~
../include/linux/bits.h:27:7: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
   (l) > (h), 0)))
       ^
../include/linux/build_bug.h:16:62: note: in definition of macro \u2018BUILD_BUG_ON_ZERO\u2019
 #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
                                                              ^
../include/linux/bits.h:40:3: note: in expansion of macro \u2018GENMASK_INPUT_CHECK\u2019
  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
   ^~~~~~~~~~~~~~~~~~~
../drivers/crypto/inside-secure/safexcel.c:761:35: note: in expansion of macro \u2018GENMASK\u2019
   writel(EIP197_DxE_THR_CTRL_EN | GENMASK(priv->config.rings - 1, 0),
                                   ^~~~~~~

This happens when only l is const but h isn't.

Can we please just revert the original patch and work this out in
the next release?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v4 1/2] linux/bits.h: fix unsigned less than zero warnings

[Herbert Xu]

Rikard Falkeborn <rikard.falkeborn@gmail.com> wrote:
> When calling the GENMASK and GENMASK_ULL macros with zero lower bit and
> an unsigned unknown high bit, some gcc versions warn due to the
> comparisons of the high and low bit in GENMASK_INPUT_CHECK.
> 
> To silence the warnings, only perform the check if both inputs are
> known. This does not trigger any warnings, from the Wtype-limits help:
> 
>        Warn if a comparison is always true or always false due to the
>        limited range of the data type, but do not warn for constant
>        expressions.
> 
> As an example of the warning, kindly reported by the kbuild test robot:
> 
> from drivers/mfd/atmel-smc.c:11:
> drivers/mfd/atmel-smc.c: In function 'atmel_smc_cs_encode_ncycles':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> 26 |   __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> |                            ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> 16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> |                                                              ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> 39 |  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> |   ^~~~~~~~~~~~~~~~~~~
>>> drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro 'GENMASK'
> 49 |  unsigned int lsbmask = GENMASK(msbpos - 1, 0);
> |                         ^~~~~~~
> 
> Fixes: 295bcca84916 ("linux/bits.h: add compile time sanity check of GENMASK inputs")
> Reported-by: kbuild test robot <lkp@intel.com>
> Reported-by: Emil Velikov <emil.l.velikov@gmail.com>
> Reported-by: Syed Nayyar Waris <syednwaris@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
> Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
> ---
> v3-v4
> Added Emils Reviewed-by.

I'm still getting the same warning even with the patch, for example:

  CC [M]  drivers/crypto/inside-secure/safexcel.o
  CHECK   ../drivers/crypto/inside-secure/safexcel_hash.c
In file included from ../include/linux/bits.h:23,
                 from ../include/linux/bitops.h:5,
                 from ../include/linux/kernel.h:12,
                 from ../include/linux/clk.h:13,
                 from ../drivers/crypto/inside-secure/safexcel.c:8:
../drivers/crypto/inside-secure/safexcel.c: In function \u2018safexcel_hw_init\u2019:
../include/linux/bits.h:27:7: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
   (l) > (h), 0)))
       ^
../include/linux/build_bug.h:16:62: note: in definition of macro \u2018BUILD_BUG_ON_ZERO\u2019
 #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
                                                              ^
../include/linux/bits.h:40:3: note: in expansion of macro \u2018GENMASK_INPUT_CHECK\u2019
  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
   ^~~~~~~~~~~~~~~~~~~
../drivers/crypto/inside-secure/safexcel.c:649:11: note: in expansion of macro \u2018GENMASK\u2019
           GENMASK(priv->config.rings - 1, 0),
           ^~~~~~~
../include/linux/bits.h:27:7: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
   (l) > (h), 0)))
       ^
../include/linux/build_bug.h:16:62: note: in definition of macro \u2018BUILD_BUG_ON_ZERO\u2019
 #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
                                                              ^
../include/linux/bits.h:40:3: note: in expansion of macro \u2018GENMASK_INPUT_CHECK\u2019
  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
   ^~~~~~~~~~~~~~~~~~~
../drivers/crypto/inside-secure/safexcel.c:757:35: note: in expansion of macro \u2018GENMASK\u2019
   writel(EIP197_DxE_THR_CTRL_EN | GENMASK(priv->config.rings - 1, 0),
                                   ^~~~~~~
../include/linux/bits.h:27:7: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
   (l) > (h), 0)))
       ^
../include/linux/build_bug.h:16:62: note: in definition of macro \u2018BUILD_BUG_ON_ZERO\u2019
 #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
                                                              ^
../include/linux/bits.h:40:3: note: in expansion of macro \u2018GENMASK_INPUT_CHECK\u2019
  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
   ^~~~~~~~~~~~~~~~~~~
../drivers/crypto/inside-secure/safexcel.c:761:35: note: in expansion of macro \u2018GENMASK\u2019
   writel(EIP197_DxE_THR_CTRL_EN | GENMASK(priv->config.rings - 1, 0),
                                   ^~~~~~~

This happens when only l is const but h isn't.

Can we please just revert the original patch and work this out in
the next release?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v4 1/2] linux/bits.h: fix unsigned less than zero warnings

[Linus Torvalds]

On Thu, Jul 9, 2020 at 5:30 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> I'm still getting the same warning even with the patch, for example:

Are you building with W=1?

There's a patch to move that broken -Wtype-limits thing to W=2.

    https://lore.kernel.org/lkml/20200708190756.16810-1-rikard.falkeborn@gmail.com/

does that work for you?

              Linus

Re: [PATCH v4 1/2] linux/bits.h: fix unsigned less than zero warnings

[Herbert Xu]

On Thu, Jul 09, 2020 at 11:13:31AM -0700, Linus Torvalds wrote:
> On Thu, Jul 9, 2020 at 5:30 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >
> > I'm still getting the same warning even with the patch, for example:
> 
> Are you building with W=1?
> 
> There's a patch to move that broken -Wtype-limits thing to W=2.
> 
>     https://lore.kernel.org/lkml/20200708190756.16810-1-rikard.falkeborn@gmail.com/
> 
> does that work for you?

Yes that should work too.  Thanks!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3 1/2] linux/bits.h: fix unsigned less than zero warnings

[Emil Velikov]

Hi Rikard,

On Mon, 8 Jun 2020 at 23:18, Rikard Falkeborn
<rikard.falkeborn@gmail.com> wrote:
>
> When calling the GENMASK and GENMASK_ULL macros with zero lower bit and
> an unsigned unknown high bit, some gcc versions warn due to the
> comparisons of the high and low bit in GENMASK_INPUT_CHECK.
>
> To silence the warnings, only perform the check if both inputs are
> known. This does not trigger any warnings, from the Wtype-limits help:
>
>         Warn if a comparison is always true or always false due to the
>         limited range of the data type, but do not warn for constant
>         expressions.
>
> As an example of the warning, kindly reported by the kbuild test robot:
>
> from drivers/mfd/atmel-smc.c:11:
> drivers/mfd/atmel-smc.c: In function 'atmel_smc_cs_encode_ncycles':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> 26 |   __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> |                            ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> 16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> |                                                              ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> 39 |  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> |   ^~~~~~~~~~~~~~~~~~~
> >> drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro 'GENMASK'
> 49 |  unsigned int lsbmask = GENMASK(msbpos - 1, 0);
> |                         ^~~~~~~
>
> Fixes: 295bcca84916 ("linux/bits.h: add compile time sanity check of GENMASK inputs")
> Reported-by: kbuild test robot <lkp@intel.com>
> Reported-by: Emil Velikov <emil.l.velikov@gmail.com>
> Reported-by: Syed Nayyar Waris <syednwaris@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>

This version is far better than my approach. Fwiw:

Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

Thanks
Emil

Re: [PATCH v2 2/2] bits: Add tests of GENMASK

[Andy Shevchenko]

On Sun, Jun 7, 2020 at 11:34 PM Rikard Falkeborn
<rikard.falkeborn@gmail.com> wrote:
>
> Add tests of GENMASK and GENMASK_ULL.
>
> A few test cases that should fail compilation are provided under ifdef.
>

Thank you very much!

> * New patch. First time I wrote a KUnittest so may be room for
>   improvements...

Have you considered to unify them with existing test_bitops.h?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] bits: Add tests of GENMASK

[Andy Shevchenko]

On Mon, Jun 8, 2020 at 11:08 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sun, Jun 7, 2020 at 11:34 PM Rikard Falkeborn
> <rikard.falkeborn@gmail.com> wrote:
> >
> > Add tests of GENMASK and GENMASK_ULL.
> >
> > A few test cases that should fail compilation are provided under ifdef.
> >
>
> Thank you very much!
>
> > * New patch. First time I wrote a KUnittest so may be room for
> >   improvements...
>
> Have you considered to unify them with existing test_bitops.h?

test_bitops.c, of course.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] bits: Add tests of GENMASK

[Rikard Falkeborn]

On Mon, Jun 08, 2020 at 11:08:04AM +0300, Andy Shevchenko wrote:
> On Sun, Jun 7, 2020 at 11:34 PM Rikard Falkeborn
> <rikard.falkeborn@gmail.com> wrote:
> >
> > Add tests of GENMASK and GENMASK_ULL.
> >
> > A few test cases that should fail compilation are provided under ifdef.
> >
> 
> Thank you very much!
> 
> > * New patch. First time I wrote a KUnittest so may be room for
> >   improvements...
> 
> Have you considered to unify them with existing test_bitops.h?

test_bitops.c seems to be tests for macros/functions in bitops.h, so I
figured it would make more sense to add tests of bits.h in test_bits.c.
But I don't have a strong opinion about it. If you prefer, I'll move
them to test_bitops.c.

Rikard
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2 1/2] linux/bits.h: fix unsigned less than zero warnings

[Andy Shevchenko]

On Sun, Jun 7, 2020 at 11:34 PM Rikard Falkeborn
<rikard.falkeborn@gmail.com> wrote:
>
> When calling the GENMASK and GENMASK_ULL macros with zero lower bit and
> an unsigned unknown high bit, some gcc versions warn due to the
> comparisons of the high and low bit in GENMASK_INPUT_CHECK.
>
> To silence the warnings, only perform the check if both inputs are
> known. This does not trigger any warnings, from the Wtype-limits help:
>
>         Warn if a comparison is always true or always false due to the
>         limited range of the data type, but do not warn for constant
>         expressions.
>
> As an example of the warning, kindly reported by the kbuild test robot:
>
> from drivers/mfd/atmel-smc.c:11:
> drivers/mfd/atmel-smc.c: In function 'atmel_smc_cs_encode_ncycles':
> include/linux/bits.h:26:28: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
> 26 |   __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> |                            ^
> include/linux/build_bug.h:16:62: note: in definition of macro 'BUILD_BUG_ON_ZERO'
> 16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> |                                                              ^
> include/linux/bits.h:39:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> 39 |  (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> |   ^~~~~~~~~~~~~~~~~~~
> >> drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro 'GENMASK'
> 49 |  unsigned int lsbmask = GENMASK(msbpos - 1, 0);
> |                         ^~~~~~~

It's much better, than previous variant, thanks!
FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Fixes: 295bcca84916 ("linux/bits.h: add compile time sanity check of GENMASK inputs")
> Reported-by: kbuild test robot <lkp@intel.com>
> Reported-by: Emil Velikov <emil.l.velikov@gmail.com>
> Reported-by: Syed Nayyar Waris <syednwaris@gmail.com>
> Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
> ---
> v1->v2
> * Change to require both high and low bit to be constant expressions
>   instead of introducing somewhat arbitrary casts
>
>  include/linux/bits.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index 4671fbf28842..35ca3f5d11a0 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -23,7 +23,8 @@
>  #include <linux/build_bug.h>
>  #define GENMASK_INPUT_CHECK(h, l) \
>         (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> -               __builtin_constant_p((l) > (h)), (l) > (h), 0)))
> +               __builtin_constant_p(l) && __builtin_constant_p(h), \
> +               (l) > (h), 0)))
>  #else
>  /*
>   * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> --
> 2.27.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 0/4] Introduce the for_each_set_clump macro

[Bartosz Golaszewski]

niedz., 24 maj 2020 o 07:00 Syed Nayyar Waris <syednwaris@gmail.com> napisał(a):
>
> Hello Linus,
>
> Since this patchset primarily affects GPIO drivers, would you like
> to pick it up through your GPIO tree?
>
> This patchset introduces a new generic version of for_each_set_clump.
> The previous version of for_each_set_clump8 used a fixed size 8-bit
> clump, but the new generic version can work with clump of any size but
> less than or equal to BITS_PER_LONG. The patchset utilizes the new macro
> in several GPIO drivers.
>
> The earlier 8-bit for_each_set_clump8 facilitated a
> for-loop syntax that iterates over a memory region entire groups of set
> bits at a time.
>

The GPIO part looks good to me. Linus: how do we go about merging it
given the bitops dependency?

Bart

Re: [PATCH v7 0/4] Introduce the for_each_set_clump macro

[Bartosz Golaszewski]

niedz., 24 maj 2020 o 07:00 Syed Nayyar Waris <syednwaris@gmail.com> napisał(a):
>
> Hello Linus,
>
> Since this patchset primarily affects GPIO drivers, would you like
> to pick it up through your GPIO tree?
>
> This patchset introduces a new generic version of for_each_set_clump.
> The previous version of for_each_set_clump8 used a fixed size 8-bit
> clump, but the new generic version can work with clump of any size but
> less than or equal to BITS_PER_LONG. The patchset utilizes the new macro
> in several GPIO drivers.
>
> The earlier 8-bit for_each_set_clump8 facilitated a
> for-loop syntax that iterates over a memory region entire groups of set
> bits at a time.
>

The GPIO part looks good to me. Linus: how do we go about merging it
given the bitops dependency?

Bart

Re: [PATCH v7 0/4] Introduce the for_each_set_clump macro

[Syed Nayyar Waris]

On Mon, May 25, 2020 at 3:06 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> niedz., 24 maj 2020 o 07:00 Syed Nayyar Waris <syednwaris@gmail.com> napisał(a):
> >
> > Hello Linus,
> >
> > Since this patchset primarily affects GPIO drivers, would you like
> > to pick it up through your GPIO tree?
> >
> > This patchset introduces a new generic version of for_each_set_clump.
> > The previous version of for_each_set_clump8 used a fixed size 8-bit
> > clump, but the new generic version can work with clump of any size but
> > less than or equal to BITS_PER_LONG. The patchset utilizes the new macro
> > in several GPIO drivers.
> >
> > The earlier 8-bit for_each_set_clump8 facilitated a
> > for-loop syntax that iterates over a memory region entire groups of set
> > bits at a time.
> >
>
> The GPIO part looks good to me. Linus: how do we go about merging it
> given the bitops dependency?
>
> Bart

A minor change has been done in patch [2/4] to fix compilation warning.
Kindly refer patchset v8 in future.

Thanks
Syed Nayyar Waris