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
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 <[email protected]>
Signed-off-by: Syed Nayyar Waris <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: William Breathitt Gray <[email protected]>
---
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
The introduction of the generic for_each_set_clump macro need test
cases to verify the implementation. This patch adds test cases for
scenarios in which clump sizes are 8 bits, 24 bits, 30 bits and 6 bits.
The cases contain situations where clump is getting split at the word
boundary and also when zeroes are present in the start and middle of
bitmap.
Signed-off-by: Syed Nayyar Waris <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: William Breathitt Gray <[email protected]>
---
Changes in v7:
- Minor changes: Use macro 'DECLARE_BITMAP()' and split 'struct'
definition and test data.
Changes in v6:
- Make 'for loop' inside 'test_for_each_set_clump' more succinct.
Changes in v5:
- No change.
Changes in v4:
- Use 'for' loop in test function of 'for_each_set_clump'.
Changes in v3:
- No Change.
Changes in v2:
- Unify different tests for 'for_each_set_clump'. Pass test data as
function parameters.
- Remove unnecessary bitmap_zero calls.
lib/test_bitmap.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 144 insertions(+)
diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index 6b13150667f5..31b3cd920c93 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -155,6 +155,38 @@ static bool __init __check_eq_clump8(const char *srcfile, unsigned int line,
return true;
}
+static bool __init __check_eq_clump(const char *srcfile, unsigned int line,
+ const unsigned int offset,
+ const unsigned int size,
+ const unsigned long *const clump_exp,
+ const unsigned long *const clump,
+ const unsigned long clump_size)
+{
+ unsigned long exp;
+
+ if (offset >= size) {
+ pr_warn("[%s:%u] bit offset for clump out-of-bounds: expected less than %u, got %u\n",
+ srcfile, line, size, offset);
+ return false;
+ }
+
+ exp = clump_exp[offset / clump_size];
+ if (!exp) {
+ pr_warn("[%s:%u] bit offset for zero clump: expected nonzero clump, got bit offset %u with clump value 0",
+ srcfile, line, offset);
+ return false;
+ }
+
+ if (*clump != exp) {
+ pr_warn("[%s:%u] expected clump value of 0x%lX, got clump value of 0x%lX",
+ srcfile, line, exp, *clump);
+ return false;
+ }
+
+ return true;
+}
+
+
#define __expect_eq(suffix, ...) \
({ \
int result = 0; \
@@ -172,6 +204,7 @@ static bool __init __check_eq_clump8(const char *srcfile, unsigned int line,
#define expect_eq_pbl(...) __expect_eq(pbl, ##__VA_ARGS__)
#define expect_eq_u32_array(...) __expect_eq(u32_array, ##__VA_ARGS__)
#define expect_eq_clump8(...) __expect_eq(clump8, ##__VA_ARGS__)
+#define expect_eq_clump(...) __expect_eq(clump, ##__VA_ARGS__)
static void __init test_zero_clear(void)
{
@@ -577,6 +610,28 @@ static void noinline __init test_mem_optimisations(void)
}
}
+static const unsigned long clump_bitmap_data[] __initconst = {
+ 0x38000201,
+ 0x05ff0f38,
+ 0xeffedcba,
+ 0xbbbbabcd,
+ 0x000000aa,
+ 0x000000aa,
+ 0x00ff0000,
+ 0xaaaaaa00,
+ 0xff000000,
+ 0x00aa0000,
+ 0x00000000,
+ 0x00000000,
+ 0x00000000,
+ 0x0f000000,
+ 0x00ff0000,
+ 0xaaaaaa00,
+ 0xff000000,
+ 0x00aa0000,
+ 0x00000ac0,
+};
+
static const unsigned char clump_exp[] __initconst = {
0x01, /* 1 bit set */
0x02, /* non-edge 1 bit set */
@@ -588,6 +643,94 @@ static const unsigned char clump_exp[] __initconst = {
0x05, /* non-adjacent 2 bits set */
};
+static const unsigned long clump_exp1[] __initconst = {
+ 0x01, /* 1 bit set */
+ 0x02, /* non-edge 1 bit set */
+ 0x00, /* zero bits set */
+ 0x38, /* 3 bits set across 4-bit boundary */
+ 0x38, /* Repeated clump */
+ 0x0F, /* 4 bits set */
+ 0xFF, /* all bits set */
+ 0x05, /* non-adjacent 2 bits set */
+};
+
+static const unsigned long clump_exp2[] __initconst = {
+ 0xfedcba, /* 24 bits */
+ 0xabcdef,
+ 0xaabbbb, /* Clump split between 2 words */
+ 0x000000, /* zeroes in between */
+ 0x0000aa,
+ 0x000000,
+ 0x0000ff,
+ 0xaaaaaa,
+ 0x000000,
+ 0x0000ff,
+};
+
+static const unsigned long clump_exp3[] __initconst = {
+ 0x00000000, /* starting with 0s*/
+ 0x00000000, /* All 0s */
+ 0x00000000,
+ 0x00000000,
+ 0x3f00000f, /* Non zero set */
+ 0x2aa80003,
+ 0x00000aaa,
+ 0x00003fc0,
+};
+
+static const unsigned long clump_exp4[] __initconst = {
+ 0x00,
+ 0x2b,
+};
+
+struct clump_test_data_params {
+ DECLARE_BITMAP(data, 256);
+ unsigned long count;
+ unsigned long offset;
+ unsigned long limit;
+ unsigned long clump_size;
+ unsigned long const *exp;
+};
+
+struct clump_test_data_params clump_test_data[] = { {{0}, 2, 0, 64, 8, clump_exp1},
+ {{0}, 8, 2, 240, 24, clump_exp2},
+ {{0}, 8, 10, 240, 30, clump_exp3},
+ {{0}, 1, 18, 18, 6, clump_exp4} };
+
+static void __init prepare_test_data(unsigned int index)
+{
+ int i;
+ unsigned long width = 0;
+
+ for(i = 0; i < clump_test_data[index].count; i++)
+ {
+ bitmap_set_value(clump_test_data[index].data,
+ clump_bitmap_data[(clump_test_data[index].offset)++], width, 32);
+ width += 32;
+ }
+}
+
+static void __init execute_for_each_set_clump_test(unsigned int index)
+{
+ unsigned long start, clump;
+
+ for_each_set_clump(start, clump, clump_test_data[index].data,
+ clump_test_data[index].limit,
+ clump_test_data[index].clump_size)
+ expect_eq_clump(start, clump_test_data[index].limit, clump_test_data[index].exp,
+ &clump, clump_test_data[index].clump_size);
+}
+
+static void __init test_for_each_set_clump(void)
+{
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(clump_test_data); i++) {
+ prepare_test_data(i);
+ execute_for_each_set_clump_test(i);
+ }
+}
+
static void __init test_for_each_set_clump8(void)
{
#define CLUMP_EXP_NUMBITS 64
@@ -623,6 +766,7 @@ static void __init selftest(void)
test_bitmap_parselist_user();
test_mem_optimisations();
test_for_each_set_clump8();
+ test_for_each_set_clump();
}
KSTM_MODULE_LOADERS(test_bitmap);
--
2.26.2
This patch reimplements the thunderx_gpio_set_multiple function in
drivers/gpio/gpio-thunderx.c to use the new for_each_set_clump macro.
Instead of looping for each bank in thunderx_gpio_set_multiple
function, now we can skip bank which is not set and save cycles.
Cc: Robert Richter <[email protected]>
Cc: Bartosz Golaszewski <[email protected]>
Signed-off-by: Syed Nayyar Waris <[email protected]>
Signed-off-by: William Breathitt Gray <[email protected]>
---
Changes in v7:
- No change.
Changes in v6:
- No change.
Changes in v5:
- No change.
Changes in v4:
- Minor change: Inline value '64' in code for better code readability.
Changes in v3:
- Change datatype of some variables from u64 to unsigned long
in function thunderx_gpio_set_multiple.
Changes in v2:
- No change.
drivers/gpio/gpio-thunderx.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c
index 9f66deab46ea..58c9bb25a377 100644
--- a/drivers/gpio/gpio-thunderx.c
+++ b/drivers/gpio/gpio-thunderx.c
@@ -275,12 +275,15 @@ static void thunderx_gpio_set_multiple(struct gpio_chip *chip,
unsigned long *bits)
{
int bank;
- u64 set_bits, clear_bits;
+ unsigned long set_bits, clear_bits, gpio_mask;
+ unsigned long offset;
+
struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
- for (bank = 0; bank <= chip->ngpio / 64; bank++) {
- set_bits = bits[bank] & mask[bank];
- clear_bits = ~bits[bank] & mask[bank];
+ for_each_set_clump(offset, gpio_mask, mask, chip->ngpio, 64) {
+ bank = offset / 64;
+ set_bits = bits[bank] & gpio_mask;
+ clear_bits = ~bits[bank] & gpio_mask;
writeq(set_bits, txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_SET);
writeq(clear_bits, txgpio->register_base + (bank * GPIO_2ND_BANK) + GPIO_TX_CLR);
}
--
2.26.2
This patch reimplements the xgpio_set_multiple function in
drivers/gpio/gpio-xilinx.c to use the new for_each_set_clump macro.
Instead of looping for each bit in xgpio_set_multiple
function, now we can check each channel at a time and save cycles.
Cc: Bartosz Golaszewski <[email protected]>
Cc: Michal Simek <[email protected]>
Signed-off-by: Syed Nayyar Waris <[email protected]>
Signed-off-by: William Breathitt Gray <[email protected]>
---
Changes in v7:
- No change.
Changes in v6:
- No change.
Changes in v5:
- Minor change: Inline values '32' and '64' in code for better
code readability.
Changes in v4:
- Minor change: Inline values '32' and '64' in code for better
code readability.
Changes in v3:
- No change.
Changes in v2:
- No change.
drivers/gpio/gpio-xilinx.c | 62 ++++++++++++++++++++------------------
1 file changed, 32 insertions(+), 30 deletions(-)
diff --git a/drivers/gpio/gpio-xilinx.c b/drivers/gpio/gpio-xilinx.c
index 67f9f82e0db0..e81092dea27e 100644
--- a/drivers/gpio/gpio-xilinx.c
+++ b/drivers/gpio/gpio-xilinx.c
@@ -136,39 +136,41 @@ static void xgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask,
unsigned long *bits)
{
- unsigned long flags;
+ unsigned long flags[2];
struct xgpio_instance *chip = gpiochip_get_data(gc);
- int index = xgpio_index(chip, 0);
- int offset, i;
-
- spin_lock_irqsave(&chip->gpio_lock[index], flags);
-
- /* Write to GPIO signals */
- for (i = 0; i < gc->ngpio; i++) {
- if (*mask == 0)
- break;
- /* Once finished with an index write it out to the register */
- if (index != xgpio_index(chip, i)) {
- xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET +
- index * XGPIO_CHANNEL_OFFSET,
- chip->gpio_state[index]);
- spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
- index = xgpio_index(chip, i);
- spin_lock_irqsave(&chip->gpio_lock[index], flags);
- }
- if (__test_and_clear_bit(i, mask)) {
- offset = xgpio_offset(chip, i);
- if (test_bit(i, bits))
- chip->gpio_state[index] |= BIT(offset);
- else
- chip->gpio_state[index] &= ~BIT(offset);
- }
+ u32 *const state = chip->gpio_state;
+ unsigned int *const width = chip->gpio_width;
+ unsigned long offset, clump;
+ size_t index;
+
+ DECLARE_BITMAP(old, 64);
+ DECLARE_BITMAP(new, 64);
+ DECLARE_BITMAP(changed, 64);
+
+ spin_lock_irqsave(&chip->gpio_lock[0], flags[0]);
+ spin_lock_irqsave(&chip->gpio_lock[1], flags[1]);
+
+ bitmap_set_value(old, state[0], 0, width[0]);
+ bitmap_set_value(old, state[1], width[0], width[1]);
+ bitmap_replace(new, old, bits, mask, gc->ngpio);
+
+ bitmap_set_value(old, state[0], 0, 32);
+ bitmap_set_value(old, state[1], 32, 32);
+ state[0] = bitmap_get_value(new, 0, width[0]);
+ state[1] = bitmap_get_value(new, width[0], width[1]);
+ bitmap_set_value(new, state[0], 0, 32);
+ bitmap_set_value(new, state[1], 32, 32);
+ bitmap_xor(changed, old, new, 64);
+
+ for_each_set_clump(offset, clump, changed, 64, 32) {
+ index = offset / 32;
+ xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET +
+ index * XGPIO_CHANNEL_OFFSET,
+ state[index]);
}
- xgpio_writereg(chip->regs + XGPIO_DATA_OFFSET +
- index * XGPIO_CHANNEL_OFFSET, chip->gpio_state[index]);
-
- spin_unlock_irqrestore(&chip->gpio_lock[index], flags);
+ spin_unlock_irqrestore(&chip->gpio_lock[1], flags[1]);
+ spin_unlock_irqrestore(&chip->gpio_lock[0], flags[0]);
}
/**
--
2.26.2
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 <[email protected]>
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/[email protected]
niedz., 24 maj 2020 o 07:00 Syed Nayyar Waris <[email protected]> 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
On Sun, May 24, 2020 at 8:15 PM kbuild test robot <[email protected]> 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 <[email protected]>
>
> 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/[email protected]
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
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 <[email protected]> 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
On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
<[email protected]> 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 <[email protected]> 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
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
> <[email protected]> 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 <[email protected]> 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
On Fri, May 29, 2020 at 11:07 PM Syed Nayyar Waris <[email protected]> wrote:
> On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
> <[email protected]> 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 <[email protected]> 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
On Sat, May 30, 2020 at 3:01 AM William Breathitt Gray
<[email protected]> 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
> > <[email protected]> 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 <[email protected]> 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
On Sat, May 30, 2020 at 12:42 AM Andy Shevchenko
<[email protected]> wrote:
> On Fri, May 29, 2020 at 11:07 PM Syed Nayyar Waris <[email protected]> wrote:
> > On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
> > <[email protected]> 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
On Sat, May 30, 2020 at 3:13 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Fri, May 29, 2020 at 11:07 PM Syed Nayyar Waris <[email protected]> wrote:
> > On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
> > <[email protected]> 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 <[email protected]> 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
On Sat, May 30, 2020 at 1:11 AM Syed Nayyar Waris <[email protected]> wrote:
> On Sat, May 30, 2020 at 3:13 AM Andy Shevchenko
> <[email protected]> wrote:
> > On Fri, May 29, 2020 at 11:07 PM Syed Nayyar Waris <[email protected]> wrote:
> > > On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
> > > <[email protected]> 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 <[email protected]> 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
On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Sat, May 30, 2020 at 1:11 AM Syed Nayyar Waris <[email protected]> wrote:
> > On Sat, May 30, 2020 at 3:13 AM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Fri, May 29, 2020 at 11:07 PM Syed Nayyar Waris <[email protected]> wrote:
> > > > On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
> > > > <[email protected]> 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 <[email protected]> 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
On Sat, May 30, 2020 at 11:45 AM Syed Nayyar Waris <[email protected]> wrote:
> On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
> <[email protected]> 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
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: x86_64-randconfig-s021-20200529 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-243-gc100a7ab-dirty
# save the attached .config to linux build tree
make W=1 C=1 ARCH=x86_64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <[email protected]>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
>> WARNING: modpost: lib/test_bitmap.o(.data+0xe80): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp1
The variable clump_test_data references
the variable __initconst clump_exp1
If the reference is valid then annotate the
variable with or __refdata (see linux/init.h) or name the variable:
--
>> WARNING: modpost: lib/test_bitmap.o(.data+0xec8): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp2
The variable clump_test_data references
the variable __initconst clump_exp2
If the reference is valid then annotate the
variable with or __refdata (see linux/init.h) or name the variable:
--
>> WARNING: modpost: lib/test_bitmap.o(.data+0xf10): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp3
The variable clump_test_data references
the variable __initconst clump_exp3
If the reference is valid then annotate the
variable with or __refdata (see linux/init.h) or name the variable:
--
>> WARNING: modpost: lib/test_bitmap.o(.data+0xf58): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp4
The variable clump_test_data references
the variable __initconst clump_exp4
If the reference is valid then annotate the
variable with or __refdata (see linux/init.h) or name the variable:
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Sat, May 30, 2020 at 2:50 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Sat, May 30, 2020 at 11:45 AM Syed Nayyar Waris <[email protected]> wrote:
> > On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
> > <[email protected]> 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
On Sun, May 31, 2020 at 4:11 AM Syed Nayyar Waris <[email protected]> wrote:
> On Sat, May 30, 2020 at 2:50 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Sat, May 30, 2020 at 11:45 AM Syed Nayyar Waris <[email protected]> wrote:
> > > On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
> > > <[email protected]> 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
+ 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 <[email protected]> wrote:
> > On Sat, May 30, 2020 at 2:50 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Sat, May 30, 2020 at 11:45 AM Syed Nayyar Waris <[email protected]> wrote:
> > > > On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
> > > > <[email protected]> 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
On Mon, Jun 1, 2020 at 4:07 AM Rikard Falkeborn
<[email protected]> 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 <[email protected]> wrote:
> > > On Sat, May 30, 2020 at 2:50 PM Andy Shevchenko
> > > <[email protected]> wrote:
> > > > On Sat, May 30, 2020 at 11:45 AM Syed Nayyar Waris <[email protected]> wrote:
> > > > > On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
> > > > > <[email protected]> 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
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 <[email protected]> wrote:
> > > On Sat, May 30, 2020 at 2:50 PM Andy Shevchenko
> > > <[email protected]> wrote:
> > > > On Sat, May 30, 2020 at 11:45 AM Syed Nayyar Waris <[email protected]> wrote:
> > > > > On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
> > > > > <[email protected]> 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
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 <[email protected]> wrote:
> > > > On Sat, May 30, 2020 at 2:50 PM Andy Shevchenko
> > > > <[email protected]> wrote:
> > > > > On Sat, May 30, 2020 at 11:45 AM Syed Nayyar Waris <[email protected]> wrote:
> > > > > > On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
> > > > > > <[email protected]> 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
>
>
On Tue, Jun 2, 2020 at 10:01 PM Rikard Falkeborn
<[email protected]> 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
On Wed, Jun 03, 2020 at 11:49:37AM +0300, Andy Shevchenko wrote:
> On Tue, Jun 2, 2020 at 10:01 PM Rikard Falkeborn
> <[email protected]> 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
On Thu, Jun 4, 2020 at 12:53 AM Rikard Falkeborn
<[email protected]> 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
> > <[email protected]> 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
On Thu, Jun 4, 2020 at 12:58 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Jun 4, 2020 at 12:53 AM Rikard Falkeborn
> <[email protected]> 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
> > > <[email protected]> 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
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 <[email protected]>
Reported-by: Emil Velikov <[email protected]>
Reported-by: Syed Nayyar Waris <[email protected]>
Signed-off-by: Rikard Falkeborn <[email protected]>
---
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
On Thu, Jun 4, 2020 at 1:03 AM Rikard Falkeborn
<[email protected]> 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 <[email protected]>
> Reported-by: Emil Velikov <[email protected]>
> Reported-by: Syed Nayyar Waris <[email protected]>
> Signed-off-by: Rikard Falkeborn <[email protected]>
> ---
> 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
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.
On Sun, May 31, 2020 at 12:50 AM kbuild test robot <[email protected]> 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: x86_64-randconfig-s021-20200529 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
> reproduce:
> # apt-get install sparse
> # sparse version: v0.6.1-243-gc100a7ab-dirty
> # save the attached .config to linux build tree
> make W=1 C=1 ARCH=x86_64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kbuild test robot <[email protected]>
>
> All warnings (new ones prefixed by >>, old ones prefixed by <<):
>
> >> WARNING: modpost: lib/test_bitmap.o(.data+0xe80): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp1
> The variable clump_test_data references
> the variable __initconst clump_exp1
> If the reference is valid then annotate the
> variable with or __refdata (see linux/init.h) or name the variable:
>
> --
> >> WARNING: modpost: lib/test_bitmap.o(.data+0xec8): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp2
> The variable clump_test_data references
> the variable __initconst clump_exp2
> If the reference is valid then annotate the
> variable with or __refdata (see linux/init.h) or name the variable:
>
> --
> >> WARNING: modpost: lib/test_bitmap.o(.data+0xf10): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp3
> The variable clump_test_data references
> the variable __initconst clump_exp3
> If the reference is valid then annotate the
> variable with or __refdata (see linux/init.h) or name the variable:
>
> --
> >> WARNING: modpost: lib/test_bitmap.o(.data+0xf58): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp4
> The variable clump_test_data references
> the variable __initconst clump_exp4
> If the reference is valid then annotate the
> variable with or __refdata (see linux/init.h) or name the variable:
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/[email protected]
HI All,
Regarding the above compilation warning reported by bot. I think this is
different than GENMASK.
I am unable to reproduce the compilation warning.
I ran the command:
make W=1 C=1 ARCH=x86_64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' lib/
But the compilation warning didn't show up. Can anyone please point to me
what I am doing wrong here? How shall I reproduce the warning? Thanks !
Regards
Syed Nayyar Waris
On Thu, Jun 04, 2020 at 09:41:29AM +0300, Andy Shevchenko wrote:
> On Thu, Jun 4, 2020 at 1:03 AM Rikard Falkeborn
> <[email protected]> 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 <[email protected]>
> > Reported-by: Emil Velikov <[email protected]>
> > Reported-by: Syed Nayyar Waris <[email protected]>
> > Signed-off-by: Rikard Falkeborn <[email protected]>
> > ---
> > 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
On Fri, Jun 05, 2020 at 02:12:54AM +0530, Syed Nayyar Waris wrote:
> On Sun, May 31, 2020 at 12:50 AM kbuild test robot <[email protected]> wrote:
> > >> WARNING: modpost: lib/test_bitmap.o(.data+0xe80): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp1
> > The variable clump_test_data references
> > the variable __initconst clump_exp1
> > If the reference is valid then annotate the
> > variable with or __refdata (see linux/init.h) or name the variable:
> >
> > --
> > >> WARNING: modpost: lib/test_bitmap.o(.data+0xec8): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp2
> > The variable clump_test_data references
> > the variable __initconst clump_exp2
> > If the reference is valid then annotate the
> > variable with or __refdata (see linux/init.h) or name the variable:
> >
> > --
> > >> WARNING: modpost: lib/test_bitmap.o(.data+0xf10): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp3
> > The variable clump_test_data references
> > the variable __initconst clump_exp3
> > If the reference is valid then annotate the
> > variable with or __refdata (see linux/init.h) or name the variable:
> >
> > --
> > >> WARNING: modpost: lib/test_bitmap.o(.data+0xf58): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp4
> > The variable clump_test_data references
> > the variable __initconst clump_exp4
> > If the reference is valid then annotate the
> > variable with or __refdata (see linux/init.h) or name the variable:
> I am unable to reproduce the compilation warning.
You have to enable section mismatch checker.
> I ran the command:
> make W=1 C=1 ARCH=x86_64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' lib/
>
> But the compilation warning didn't show up. Can anyone please point to me
> what I am doing wrong here? How shall I reproduce the warning? Thanks !
You put some data into init section of the object, while you are trying to
access it from non-init one. It's easy-to-fix issue.
--
With Best Regards,
Andy Shevchenko
On Fri, Jun 5, 2020 at 5:54 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Fri, Jun 05, 2020 at 02:12:54AM +0530, Syed Nayyar Waris wrote:
> > On Sun, May 31, 2020 at 12:50 AM kbuild test robot <[email protected]> wrote:
>
> > > >> WARNING: modpost: lib/test_bitmap.o(.data+0xe80): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp1
> > > The variable clump_test_data references
> > > the variable __initconst clump_exp1
> > > If the reference is valid then annotate the
> > > variable with or __refdata (see linux/init.h) or name the variable:
> > >
> > > --
> > > >> WARNING: modpost: lib/test_bitmap.o(.data+0xec8): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp2
> > > The variable clump_test_data references
> > > the variable __initconst clump_exp2
> > > If the reference is valid then annotate the
> > > variable with or __refdata (see linux/init.h) or name the variable:
> > >
> > > --
> > > >> WARNING: modpost: lib/test_bitmap.o(.data+0xf10): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp3
> > > The variable clump_test_data references
> > > the variable __initconst clump_exp3
> > > If the reference is valid then annotate the
> > > variable with or __refdata (see linux/init.h) or name the variable:
> > >
> > > --
> > > >> WARNING: modpost: lib/test_bitmap.o(.data+0xf58): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp4
> > > The variable clump_test_data references
> > > the variable __initconst clump_exp4
> > > If the reference is valid then annotate the
> > > variable with or __refdata (see linux/init.h) or name the variable:
>
> > I am unable to reproduce the compilation warning.
>
> You have to enable section mismatch checker.
>
> > I ran the command:
> > make W=1 C=1 ARCH=x86_64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' lib/
> >
> > But the compilation warning didn't show up. Can anyone please point to me
> > what I am doing wrong here? How shall I reproduce the warning? Thanks !
>
> You put some data into init section of the object, while you are trying to
> access it from non-init one. It's easy-to-fix issue.
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks! I have made code changes for the above warning. Actually I am
still unable to reproduce the compilation warning. But I believe the
following code fix will fix the compilation warning:
In file lib/test_bitmap.c
@@ -692,7 +692,7 @@ struct clump_test_data_params {
unsigned long const *exp;
};
-struct clump_test_data_params clump_test_data[] =
+static struct clump_test_data_params clump_test_data[] __initdata =
{ {{0}, 2, 0, 64, 8, clump_exp1},
{{0}, 8, 2, 240, 24, clump_exp2},
{{0}, 8, 10, 240, 30, clump_exp3},
Let me know if I should submit a new patchset (v8) for
'for_each_set_clump' including above code fix.
Just to share how I attempted to reproduce the warning (but unsuccessful):
Step 1: Use the config file in attachment. Download, extract, rename
file to .config at the root of the tree.
Step 2: '$ make lib/'
No warning reproduced after above step 2.
Step 3: '$ make W=1 C=1 ARCH=x86_64 CF='-fdiagnostic-prefix
-D__CHECK_ENDIAN__' lib/'
After step 3 I got error in build:
scripts/kconfig/conf --syncconfig Kconfig
CHECK scripts/mod/empty.c
No such file: asan-globals=1
scripts/Makefile.build:266: recipe for target 'scripts/mod/empty.o' failed
make[1]: *** [scripts/mod/empty.o] Error 1
Makefile:1147: recipe for target 'prepare0' failed
make: *** [prepare0] Error 2
The command in above step 3 was mentioned in the bot mail.
Regards
Syed Nayyar Waris
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 <[email protected]>
Reported-by: Emil Velikov <[email protected]>
Reported-by: Syed Nayyar Waris <[email protected]>
Signed-off-by: Rikard Falkeborn <[email protected]>
---
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
Add tests of GENMASK and GENMASK_ULL.
A few test cases that should fail compilation are provided under ifdef.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Rikard Falkeborn <[email protected]>
---
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
Hi Richard,
Thanks for your patch!
On Sun, Jun 7, 2020 at 10:35 PM Rikard Falkeborn
<[email protected]> 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 <[email protected]>
> Signed-off-by: Rikard Falkeborn <[email protected]>
> --- /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 -- [email protected]
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
On Sun, Jun 7, 2020 at 11:34 PM Rikard Falkeborn
<[email protected]> 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
On Mon, Jun 8, 2020 at 11:08 AM Andy Shevchenko
<[email protected]> wrote:
> On Sun, Jun 7, 2020 at 11:34 PM Rikard Falkeborn
> <[email protected]> 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
On Sun, Jun 7, 2020 at 11:34 PM Rikard Falkeborn
<[email protected]> 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 <[email protected]>
> Fixes: 295bcca84916 ("linux/bits.h: add compile time sanity check of GENMASK inputs")
> Reported-by: kbuild test robot <[email protected]>
> Reported-by: Emil Velikov <[email protected]>
> Reported-by: Syed Nayyar Waris <[email protected]>
> Signed-off-by: Rikard Falkeborn <[email protected]>
> ---
> 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
On Sun, Jun 07, 2020 at 04:45:19AM +0530, Syed Nayyar Waris wrote:
> On Fri, Jun 5, 2020 at 5:54 PM Andy Shevchenko
> <[email protected]> wrote:
> >
> > On Fri, Jun 05, 2020 at 02:12:54AM +0530, Syed Nayyar Waris wrote:
> > > On Sun, May 31, 2020 at 12:50 AM kbuild test robot <[email protected]> wrote:
> >
> > > > >> WARNING: modpost: lib/test_bitmap.o(.data+0xe80): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp1
> > > > The variable clump_test_data references
> > > > the variable __initconst clump_exp1
> > > > If the reference is valid then annotate the
> > > > variable with or __refdata (see linux/init.h) or name the variable:
> > > >
> > > > --
> > > > >> WARNING: modpost: lib/test_bitmap.o(.data+0xec8): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp2
> > > > The variable clump_test_data references
> > > > the variable __initconst clump_exp2
> > > > If the reference is valid then annotate the
> > > > variable with or __refdata (see linux/init.h) or name the variable:
> > > >
> > > > --
> > > > >> WARNING: modpost: lib/test_bitmap.o(.data+0xf10): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp3
> > > > The variable clump_test_data references
> > > > the variable __initconst clump_exp3
> > > > If the reference is valid then annotate the
> > > > variable with or __refdata (see linux/init.h) or name the variable:
> > > >
> > > > --
> > > > >> WARNING: modpost: lib/test_bitmap.o(.data+0xf58): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp4
> > > > The variable clump_test_data references
> > > > the variable __initconst clump_exp4
> > > > If the reference is valid then annotate the
> > > > variable with or __refdata (see linux/init.h) or name the variable:
> >
> > > I am unable to reproduce the compilation warning.
> >
> > You have to enable section mismatch checker.
> >
> > > I ran the command:
> > > make W=1 C=1 ARCH=x86_64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' lib/
> > >
> > > But the compilation warning didn't show up. Can anyone please point to me
> > > what I am doing wrong here? How shall I reproduce the warning? Thanks !
> >
> > You put some data into init section of the object, while you are trying to
> > access it from non-init one. It's easy-to-fix issue.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
>
> Thanks! I have made code changes for the above warning. Actually I am
> still unable to reproduce the compilation warning. But I believe the
> following code fix will fix the compilation warning:
>
> In file lib/test_bitmap.c
>
> @@ -692,7 +692,7 @@ struct clump_test_data_params {
> unsigned long const *exp;
> };
>
> -struct clump_test_data_params clump_test_data[] =
> +static struct clump_test_data_params clump_test_data[] __initdata =
> { {{0}, 2, 0, 64, 8, clump_exp1},
> {{0}, 8, 2, 240, 24, clump_exp2},
> {{0}, 8, 10, 240, 30, clump_exp3},
>
>
>
> Let me know if I should submit a new patchset (v8) for
> 'for_each_set_clump' including above code fix.
>
> Just to share how I attempted to reproduce the warning (but unsuccessful):
>
> Step 1: Use the config file in attachment. Download, extract, rename
> file to .config at the root of the tree.
> Step 2: '$ make lib/'
> No warning reproduced after above step 2.
> Step 3: '$ make W=1 C=1 ARCH=x86_64 CF='-fdiagnostic-prefix
> -D__CHECK_ENDIAN__' lib/'
> After step 3 I got error in build:
> scripts/kconfig/conf --syncconfig Kconfig
> CHECK scripts/mod/empty.c
> No such file: asan-globals=1
> scripts/Makefile.build:266: recipe for target 'scripts/mod/empty.o' failed
> make[1]: *** [scripts/mod/empty.o] Error 1
> Makefile:1147: recipe for target 'prepare0' failed
> make: *** [prepare0] Error 2
>
> The command in above step 3 was mentioned in the bot mail.
You need to take their configuration as well.
--
With Best Regards,
Andy Shevchenko
On Mon, Jun 08, 2020 at 11:08:04AM +0300, Andy Shevchenko wrote:
> On Sun, Jun 7, 2020 at 11:34 PM Rikard Falkeborn
> <[email protected]> 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
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
> <[email protected]> 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 <[email protected]>
> > Signed-off-by: Rikard Falkeborn <[email protected]>
>
> > --- /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 -- [email protected]
>
> 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
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 <[email protected]>
Reported-by: Emil Velikov <[email protected]>
Reported-by: Syed Nayyar Waris <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Rikard Falkeborn <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Rikard Falkeborn <[email protected]>
---
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
On Tue, Jun 9, 2020 at 1:18 AM Rikard Falkeborn
<[email protected]> 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 <[email protected]>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Rikard Falkeborn <[email protected]>
> ---
> 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
On 6/7/20 7:15 AM, Syed Nayyar Waris wrote:
> On Fri, Jun 5, 2020 at 5:54 PM Andy Shevchenko
> <[email protected]> wrote:
>> On Fri, Jun 05, 2020 at 02:12:54AM +0530, Syed Nayyar Waris wrote:
>>> On Sun, May 31, 2020 at 12:50 AM kbuild test robot <[email protected]> wrote:
>>>>>> WARNING: modpost: lib/test_bitmap.o(.data+0xe80): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp1
>>>> The variable clump_test_data references
>>>> the variable __initconst clump_exp1
>>>> If the reference is valid then annotate the
>>>> variable with or __refdata (see linux/init.h) or name the variable:
>>>>
>>>> --
>>>>>> WARNING: modpost: lib/test_bitmap.o(.data+0xec8): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp2
>>>> The variable clump_test_data references
>>>> the variable __initconst clump_exp2
>>>> If the reference is valid then annotate the
>>>> variable with or __refdata (see linux/init.h) or name the variable:
>>>>
>>>> --
>>>>>> WARNING: modpost: lib/test_bitmap.o(.data+0xf10): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp3
>>>> The variable clump_test_data references
>>>> the variable __initconst clump_exp3
>>>> If the reference is valid then annotate the
>>>> variable with or __refdata (see linux/init.h) or name the variable:
>>>>
>>>> --
>>>>>> WARNING: modpost: lib/test_bitmap.o(.data+0xf58): Section mismatch in reference from the variable clump_test_data to the variable .init.rodata:clump_exp4
>>>> The variable clump_test_data references
>>>> the variable __initconst clump_exp4
>>>> If the reference is valid then annotate the
>>>> variable with or __refdata (see linux/init.h) or name the variable:
>>> I am unable to reproduce the compilation warning.
>> You have to enable section mismatch checker.
>>
>>> I ran the command:
>>> make W=1 C=1 ARCH=x86_64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' lib/
>>>
>>> But the compilation warning didn't show up. Can anyone please point to me
>>> what I am doing wrong here? How shall I reproduce the warning? Thanks !
>> You put some data into init section of the object, while you are trying to
>> access it from non-init one. It's easy-to-fix issue.
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
> Thanks! I have made code changes for the above warning. Actually I am
> still unable to reproduce the compilation warning. But I believe the
> following code fix will fix the compilation warning:
>
> In file lib/test_bitmap.c
>
> @@ -692,7 +692,7 @@ struct clump_test_data_params {
> unsigned long const *exp;
> };
>
> -struct clump_test_data_params clump_test_data[] =
> +static struct clump_test_data_params clump_test_data[] __initdata =
> { {{0}, 2, 0, 64, 8, clump_exp1},
> {{0}, 8, 2, 240, 24, clump_exp2},
> {{0}, 8, 10, 240, 30, clump_exp3},
>
>
>
> Let me know if I should submit a new patchset (v8) for
> 'for_each_set_clump' including above code fix.
>
> Just to share how I attempted to reproduce the warning (but unsuccessful):
>
> Step 1: Use the config file in attachment. Download, extract, rename
> file to .config at the root of the tree.
> Step 2: '$ make lib/'
> No warning reproduced after above step 2.
> Step 3: '$ make W=1 C=1 ARCH=x86_64 CF='-fdiagnostic-prefix
> -D__CHECK_ENDIAN__' lib/'
> After step 3 I got error in build:
> scripts/kconfig/conf --syncconfig Kconfig
> CHECK scripts/mod/empty.c
> No such file: asan-globals=1
> scripts/Makefile.build:266: recipe for target 'scripts/mod/empty.o' failed
> make[1]: *** [scripts/mod/empty.o] Error 1
> Makefile:1147: recipe for target 'prepare0' failed
> make: *** [prepare0] Error 2
>
> The command in above step 3 was mentioned in the bot mail.
>
> Regards
> Syed Nayyar Waris
>
Hi Syed Nayyar Waris,
We can reproduce the warning with the steps in original report,
you may need to build the whole kernel instead of the 'lib'.
Best Regards,
Rong Chen
On Mon, May 25, 2020 at 3:06 PM Bartosz Golaszewski
<[email protected]> wrote:
>
> niedz., 24 maj 2020 o 07:00 Syed Nayyar Waris <[email protected]> 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
Hi Rikard,
On Mon, 8 Jun 2020 at 23:18, Rikard Falkeborn
<[email protected]> 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 <[email protected]>
> Reported-by: Emil Velikov <[email protected]>
> Reported-by: Syed Nayyar Waris <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Rikard Falkeborn <[email protected]>
This version is far better than my approach. Fwiw:
Reviewed-by: Emil Velikov <[email protected]>
Thanks
Emil
On Tue, 9 Jun 2020 00:18:23 +0200 Rikard Falkeborn <[email protected]> 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?
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 <[email protected]>
Reported-by: Emil Velikov <[email protected]>
Reported-by: Syed Nayyar Waris <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Emil Velikov <[email protected]>
Signed-off-by: Rikard Falkeborn <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Rikard Falkeborn <[email protected]>
---
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
Rikard Falkeborn <[email protected]> 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 <[email protected]>
> Reported-by: Emil Velikov <[email protected]>
> Reported-by: Syed Nayyar Waris <[email protected]>
> Reviewed-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Emil Velikov <[email protected]>
> Signed-off-by: Rikard Falkeborn <[email protected]>
> ---
> 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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Thu, Jul 9, 2020 at 5:30 AM Herbert Xu <[email protected]> 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/[email protected]/
does that work for you?
Linus
On Thu, Jul 09, 2020 at 11:13:31AM -0700, Linus Torvalds wrote:
> On Thu, Jul 9, 2020 at 5:30 AM Herbert Xu <[email protected]> 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/[email protected]/
>
> does that work for you?
Yes that should work too. Thanks!
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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..]
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..]
>