Introduce the for_each_set_clump macro

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))
^~~~~~~~~~~~~~~~~~~

Quoted 1 line

Quoted 1 line

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))
^~~~~~~~~~~~~~~~~~~

Quoted 1 line

Quoted 1 line

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))
^~~~~~~~~~~~~~~~~~~

Quoted 1 line

Quoted 1 line

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))
^~~~~~~~~~~~~~~~~~~

Quoted 1 line

Quoted 1 line

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))
^~~~~~~~~~~~~~~~~~~

Quoted 1 line

Quoted 1 line

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))
^~~~~~~~~~~~~~~~~~~

Quoted 1 line

Quoted 1 line

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))
^~~~~~~~~~~~~~~~~~~

Quoted 1 line

Quoted 1 line

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))
^~~~~~~~~~~~~~~~~~~

Quoted 1 line

Quoted 1 line

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))
^~~~~~~~~~~~~~~~~~~

Quoted 1 line

Quoted 1 line

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))
^~~~~~~~~~~~~~~~~~~

Quoted 1 line

Quoted 1 line

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)

Quoted 1 line

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]

On Sun, May 24, 2020 at 8:15 PM kbuild test robot <[email protected]> wrote:

Quoted 11 lines

   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)

Quoted 7 lines

Quoted 1 line

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?

Quoted 1 line · Syed Nayyar Waris

On Sun, May 24, 2020 at 8:15 PM kbuild test robot <[email protected]> wrote:

...

Quoted 34 lines

Quoted 11 lines

   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)

Quoted 7 lines

Quoted 1 line

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.

Quoted 4 lines

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

Quoted 45 lines · Andy Shevchenko

Quoted 1 line · Syed Nayyar Waris

On Sun, May 24, 2020 at 8:15 PM kbuild test robot <[email protected]> wrote:

...

Quoted 34 lines

Quoted 11 lines

   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)

Quoted 7 lines

Quoted 1 line

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.

Quoted 4 lines

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?

Quoted 8 lines

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

Quoted 82 lines · Syed Nayyar Waris

Quoted 45 lines · Andy Shevchenko

Quoted 1 line · Syed Nayyar Waris

On Sun, May 24, 2020 at 8:15 PM kbuild test robot <[email protected]> wrote:

...

Quoted 34 lines

Quoted 11 lines

   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)

Quoted 7 lines

Quoted 1 line

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.

Quoted 4 lines

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?

Quoted 8 lines

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:

Quoted 7 lines

"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

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Syed Nayyar Waris

On Sun, May 24, 2020 at 8:15 PM kbuild test robot <[email protected]> wrote:

Quoted 10 lines

Quoted 9 lines

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.

Quoted 3 lines

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

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

Quoted 2 lines · Syed Nayyar Waris

On Sat, May 30, 2020 at 12:08 AM Andy Shevchenko
<[email protected]> wrote:

Quoted 12 lines

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.

Quoted 3 lines

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?

Quoted 4 lines · Syed Nayyar Waris

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Syed Nayyar Waris

On Sun, May 24, 2020 at 8:15 PM kbuild test robot <[email protected]> wrote:

...

Quoted 32 lines

Quoted 10 lines

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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?

Quoted 3 lines

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

Quoted 61 lines · Andy Shevchenko

Quoted 4 lines · Syed Nayyar Waris

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Syed Nayyar Waris

On Sun, May 24, 2020 at 8:15 PM kbuild test robot <[email protected]> wrote:

...

Quoted 32 lines

Quoted 10 lines

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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

Quoted 1 line

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.

Quoted 4 lines

Quoted 2 lines

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

Quoted 63 lines · Syed Nayyar Waris

Quoted 61 lines · Andy Shevchenko

Quoted 4 lines · Syed Nayyar Waris

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Syed Nayyar Waris

On Sun, May 24, 2020 at 8:15 PM kbuild test robot <[email protected]> wrote:

...

Quoted 32 lines

Quoted 10 lines

Quoted 9 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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

Quoted 7 lines

Quoted 1 line

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)) ?

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

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

Quoted 8 lines

Quoted 1 line

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.

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.

Quoted 2 lines · Syed Nayyar Waris

On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
<[email protected]> wrote:

...

Quoted 1 line

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.

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Syed Nayyar Waris

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.

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

Quoted 5 lines · Syed Nayyar Waris

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Syed Nayyar Waris

On Sat, May 30, 2020 at 3:49 AM Andy Shevchenko
<[email protected]> wrote:

...

Quoted 11 lines

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

Quoted 1 line

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.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

+ Emil who was working on a patch for this

Quoted 9 lines · Andy Shevchenko

Quoted 5 lines · Syed Nayyar Waris

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Syed Nayyar Waris

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.

Quoted 8 lines

Quoted 8 lines

#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

Quoted 3 lines

Quoted 3 lines

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):

include/linux/bits.h +0 -0

index 4671fbf28842..5fdb9909fbff 100644

I can send a proper patch if this is ok.

Quoted 2 lines

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.

Quoted 1 line

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

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 1 line

4) h and l > 0.

The comparisson works as intended.

Quoted 4 lines

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

Quoted 13 lines

Quoted 1 line

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

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Syed Nayyar Waris

On Sat, May 30, 2020 at 2:50 PM Andy Shevchenko
<[email protected]> wrote:

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines · Syed Nayyar Waris

Quoted 2 lines

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.

Quoted 11 lines

Quoted 11 lines

#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):

include/linux/bits.h +1 -1

index 4671fbf28842..5fdb9909fbff 100644
@@ -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.

Quoted 2 lines

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.

Quoted 1 line

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

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 1 line

4) h and l > 0.

The comparisson works as intended.

Quoted 3 lines

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.

Quoted 9 lines

Quoted 1 line

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.

Quoted 6 lines · Rikard Falkeborn

+ Emil who was working on a patch for this

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Syed Nayyar Waris

On Sat, May 30, 2020 at 2:50 PM Andy Shevchenko
<[email protected]> wrote:

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines · Syed Nayyar Waris

Quoted 2 lines

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

Quoted 36 lines

Quoted 11 lines

Quoted 11 lines

#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):

include/linux/bits.h +1 -1

index 4671fbf28842..5fdb9909fbff 100644
@@ -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.

Quoted 2 lines

Unfortunately, it's not enough. We need to take care about the following cases

Quoted 7 lines

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.

Quoted 9 lines

Quoted 1 line

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

Quoted 1 line

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.

Quoted 13 lines

Quoted 1 line

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.

Quoted 1 line

4) h and l > 0.

The comparisson works as intended.

Quoted 3 lines

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

Quoted 3 lines

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)?

Quoted 10 lines

Quoted 10 lines

Quoted 2 lines

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.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Rikard Falkeborn

On Sun, May 31, 2020 at 02:00:45PM +0300, Andy Shevchenko wrote:

Quoted 5 lines

Quoted 3 lines

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

Quoted 3 lines · Rikard Falkeborn

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Rikard Falkeborn

On Sun, May 31, 2020 at 02:00:45PM +0300, Andy Shevchenko wrote:

...

Quoted 11 lines

Quoted 5 lines

Quoted 3 lines

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)?

Signature · 3 lines

-- 
With Best Regards,
Andy Shevchenko

Quoted 2 lines · Andy Shevchenko

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.

Quoted 3 lines · Rikard Falkeborn

Quoted 2 lines · Andy Shevchenko

On Tue, Jun 2, 2020 at 10:01 PM Rikard Falkeborn
<[email protected]> wrote:

...

Quoted 2 lines

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

Quoted 1 line

drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro 'GENMASK'

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))
| ^~~~~~~~~~~~~~~~~~~

Quoted 1 line

Quoted 1 line

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

include/linux/bits.h +2 -2

index 4671fbf28842..293d1ee71a48 100644
@@ -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

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?

Quoted 25 lines · Rikard Falkeborn

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

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

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

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

Quoted 4 lines

Quoted 1 line

Quoted 1 line

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.

Quoted 29 lines

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

include/linux/bits.h +2 -2

index 4671fbf28842..293d1ee71a48 100644
@@ -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

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Quoted 1 line

drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro 'GENMASK'

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]>

lib/test_bits.c new file +7 -0

@@ -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>

Hi Richard,

Thanks for your patch!

Quoted 3 lines · Rikard Falkeborn

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.

Quoted 17 lines

Suggested-by Andy Shevchenko <[email protected]>

Signed-off-by Rikard Falkeborn <[email protected]>

lib/test_bits.c new file +11 -0

@@ -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)
+{

lib/test_bits.c line 17

Quoted 6 lines

lib/test_bits.c new file +6 -0

+       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"?

lib/test_bits.c line 22

Quoted 6 lines

lib/test_bits.c new file +5 -0

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

Gr{oetje,eeting}s,

Geert

Signature · 5 lines

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

Quoted 1 line

drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro 'GENMASK'

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

lib/Kconfig.debug +11 -0

index 333e878d8af9..9557cb570fb9 100644
@@ -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

lib/Makefile +1 -0

index 315516fa4ef4..2ce9892e3e63 100644
@@ -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

lib/test_bits.c new file +73 -1

new file mode 100644
index 000000000000..e2fcf24463bf
@@ -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

Add tests of GENMASK and GENMASK_ULL.

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

Quoted 1 line

drivers/mfd/atmel-smc.c:49:25: note: in expansion of macro 'GENMASK'

[Snip...]

lib/test_bits.c new file +6 -0

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

[Snap..]

Hey,

Where is TEST_GENMASK_FAILURES being defined? This fails when compiling this

test as a module.

Am I missing something here?

Cheers!

-- Nico

Quoted 8 lines · Rikard Falkeborn

[Snip...]

lib/test_bits.c new file +6 -0

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

[Snap..]

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))
| ^~~~~~~~~~~~~~~~~~~

Quoted 1 line

Quoted 1 line

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:

Quoted 2 lines · Herbert Xu

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?

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))
| ^~~~~~~~~~~~~~~~~~~

Quoted 1 line

Quoted 1 line

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]>

Add tests of GENMASK and GENMASK_ULL.

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

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

Quoted 5 lines · Rikard Falkeborn

Add tests of GENMASK and GENMASK_ULL.

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

Thank you very much!

Quoted 2 lines

* 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?

Quoted 5 lines · Rikard Falkeborn

Add tests of GENMASK and GENMASK_ULL.

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

Thank you very much!

Quoted 2 lines

* 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?

Signature · 3 lines

-- 
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, 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))
| ^~~~~~~~~~~~~~~~~~~

Quoted 1 line

Quoted 1 line

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

include/linux/bits.h +2 -2

index 4671fbf28842..35ca3f5d11a0 100644
@@ -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

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 <<):

Quoted 1 line

Quoted 1 line

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:

--

Quoted 1 line

Quoted 1 line

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:

--

Quoted 1 line

Quoted 1 line

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:

--

Quoted 1 line

Quoted 1 line

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]

Quoted 20 lines · kbuild test robot

Quoted 1 line

Quoted 1 line

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:

--

Quoted 1 line

Quoted 1 line

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:

--

Quoted 1 line

Quoted 1 line

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:

Quoted 6 lines

--

Quoted 1 line

Quoted 1 line

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.

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 !

Quoted 22 lines · Syed Nayyar Waris

Quoted 20 lines · kbuild test robot

Quoted 1 line

Quoted 1 line

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:

--

Quoted 1 line

Quoted 1 line

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:

--

Quoted 1 line

Quoted 1 line

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:

Quoted 8 lines

Quoted 6 lines

--

Quoted 1 line

Quoted 1 line

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.

Quoted 5 lines

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

Quoted 41 lines · Andy Shevchenko

Quoted 22 lines · Syed Nayyar Waris

Quoted 20 lines · kbuild test robot

Quoted 1 line

Quoted 1 line

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:

--

Quoted 1 line

Quoted 1 line

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:

--

Quoted 1 line

Quoted 1 line

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:

Quoted 8 lines

Quoted 6 lines

--

Quoted 1 line

Quoted 1 line

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.

Quoted 5 lines

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 !

Quoted 6 lines

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

(quoted diff) +1 -1

@@ -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.

Quoted 34 lines · Andy Shevchenko

Quoted 21 lines · Syed Nayyar Waris

Quoted 20 lines · kbuild test robot

Quoted 1 line

Quoted 1 line

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:

--

Quoted 1 line

Quoted 1 line

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:

--

Quoted 1 line

Quoted 1 line

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:

Quoted 7 lines

Quoted 6 lines

--

Quoted 1 line

Quoted 1 line

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.

Quoted 3 lines

I ran the command:
make W=1 C=1 ARCH=x86_64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' lib/

Quoted 8 lines

Quoted 2 lines

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

(quoted diff) +1 -1

@@ -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

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.

niedz., 24 maj 2020 o 07:00 Syed Nayyar Waris <[email protected]> napisał(a):

Quoted 12 lines

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.

Quoted 4 lines

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