Introduce the for_each_set_clump macro
Subsystems (7)
cover Introduce the for_each_set_clump macro
Hello Linus,
Since this patchset primarily affects GPIO drivers, would you like to pick it up through your GPIO tree?
(Note: Patchset resent with the new macro and relevant functions shifted to a new header clump_bits.h [Linus Torvalds])
Michal,
What do you think of [PATCH 5/5]? Is the conditional check needed? And also does returning -EINVAL look good?
This patchset introduces a new generic version of for_each_set_clump. The previous version of for_each_set_clump8 used a fixed size 8-bit clump, but the new generic version can work with clump of any size but less than or equal to BITS_PER_LONG. The patchset utilizes the new macro in several GPIO drivers.
The earlier 8-bit for_each_set_clump8 facilitated a
for-loop syntax that iterates over a memory region entire groups of set bits at a time.
For example, suppose you would like to iterate over a 32-bit integer 8 bits at a time, skipping over 8-bit groups with no set bit, where XXXXXXXX represents the current 8-bit group:
Example: 10111110 00000000 11111111 00110011
First loop: 10111110 00000000 11111111 XXXXXXXX
Second loop: 10111110 00000000 XXXXXXXX 00110011
Third loop: XXXXXXXX 00000000 11111111 00110011Each iteration of the loop returns the next 8-bit group that has at least one set bit.
But with the new for_each_set_clump the clump size can be different from 8 bits. Moreover, the clump can be split at word boundary in situations where word size is not multiple of clump size. Following are examples showing the working of new macro for clump sizes of 24 bits and 6 bits.
Example 1:
clump size: 24 bits, Number of clumps (or ports): 10
bitmap stores the bit information from where successive clumps are retrieved.
/* bitmap memory region */
0x00aa0000ff000000; /* Most significant bits */
0xaaaaaa0000ff0000;
0x000000aa000000aa;
0xbbbbabcdeffedcba; /* Least significant bits */Different iterations of for_each_set_clump:-
'offset' is the bit position and 'clump' is the 24 bit clump from the above bitmap.
Iteration first: offset: 0 clump: 0xfedcba Iteration second: offset: 24 clump: 0xabcdef Iteration third: offset: 48 clump: 0xaabbbb Iteration fourth: offset: 96 clump: 0xaa Iteration fifth: offset: 144 clump: 0xff Iteration sixth: offset: 168 clump: 0xaaaaaa Iteration seventh: offset: 216 clump: 0xff
Loop breaks because in the end the remaining bits (0x00aa) size was less than clump size of 24 bits.
In above example it can be seen that in iteration third, the 24 bit clump that was retrieved was split between bitmap[0] and bitmap[1]. This example also shows that 24 bit zeroes if present in between, were skipped (preserving the previous for_each_set_macro8 behaviour).
Example 2:
clump size = 6 bits, Number of clumps (or ports) = 3.
/* bitmap memory region */
0x00aa0000ff000000; /* Most significant bits */
0xaaaaaa0000ff0000;
0x0f00000000000000;
0x0000000000000ac0; /* Least significant bits */Different iterations of for_each_set_clump:
'offset' is the bit position and 'clump' is the 6 bit clump from the above bitmap.
Iteration first: offset: 6 clump: 0x2b
Loop breaks because 6 * 3 = 18 bits traversed in bitmap. Here 6 * 3 is clump size * no. of clumps.
GCC gives warning in bitmap_set_value(): https://godbolt.org/z/rjx34r
Add explicit check to see if the value being written into the bitmap does not fall outside the bitmap.
The situation that it is falling outside would never be possible in the code because the boundaries are required to be correct before the function is called. The responsibility is on the caller for ensuring the boundaries are correct.
The code change is simply to silence the GCC warning messages because GCC is not aware that the boundaries have already been checked. As such, we're better off using __builtin_unreachable() here because we can avoid the latency of the conditional check entirely.
Syed Nayyar Waris (5):
clump_bits: Introduce the for_each_set_clump macro lib/test_bitmap.c: Add for_each_set_clump test cases
gpio: thunderx: Utilize for_each_set_clump macro
gpio: xilinx: Utilize generic bitmap_get_value and _set_value
gpio: xilinx: Add extra check if sum of widths exceed 64
drivers/gpio/clump_bits.h | 101 ++++++++++++++++++++++++ drivers/gpio/gpio-thunderx.c | 12 ++- drivers/gpio/gpio-xilinx.c | 72 ++++++++++-------- lib/test_bitmap.c | 144 +++++++++++++++++++++++++++++++++++ 4 files changed, 292 insertions(+), 37 deletions(-) create mode 100644 drivers/gpio/clump_bits.h
base-commit: bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb
Signature · 1 line
2.29.0
PATCH 1/5 clump_bits: Introduce the for_each_set_clump macro
This macro iterates for each group of bits (clump) with set bits, within a bitmap memory region. For each iteration, "start" is set to the bit offset of the found clump, while the respective clump value is stored to the location pointed by "clump". Additionally, the bitmap_get_value() and bitmap_set_value() functions are introduced to respectively get and set a value of n-bits in a bitmap memory region. The n-bits can have any size from 1 to BITS_PER_LONG. size less than 1 or more than BITS_PER_LONG causes undefined behaviour. Moreover, during setting value of n-bit in bitmap, if a situation arise that the width of next n-bit is exceeding the word boundary, then it will divide itself such that some portion of it is stored in that word, while the remaining portion is stored in the next higher word. Similar situation occurs while retrieving the value from bitmap.
GCC gives warning in bitmap_set_value(): https://godbolt.org/z/rjx34r
Add explicit check to see if the value being written into the bitmap does not fall outside the bitmap.
The situation that it is falling outside would never be possible in the code because the boundaries are required to be correct before the function is called. The responsibility is on the caller for ensuring the boundaries are correct.
The code change is simply to silence the GCC warning messages because GCC is not aware that the boundaries have already been checked. As such, we're better off using __builtin_unreachable() here because we can avoid the latency of the conditional check entirely.
--- drivers/gpio/clump_bits.h | 101 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 drivers/gpio/clump_bits.h
drivers/gpio/clump_bits.h new file +101 -0
@@ -0,0 +1,101 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef __CLUMP_BITS_H +#define __CLUMP_BITS_H + +/** +* find_next_clump - find next clump with set bits in a memory region +* @clump: location to store copy of found clump +* @addr: address to base the search on +* @size: bitmap size in number of bits +* @offset: bit offset at which to start searching +* @clump_size: clump size in bits +* +* Returns the bit offset for the next set clump; the found clump value is +* copied to the location pointed by @clump. If no bits are set, returns @size. +*/ +extern unsigned long find_next_clump (unsigned long *clump, +const unsigned long *addr, +unsigned long size,unsigned long offset, +unsigned long clump_size); + +#define find_first_clump (clump, bits, size, clump_size) \ + find_next_clump((clump), (bits), (size), 0, (clump_size)) + +/** +* bitmap_get_value - get a value of n-bits from the memory region +* @map: address to the bitmap memory region +* @start: bit offset of the n-bit value +* @nbits: size of value in bits (must be between 1 and BITS_PER_LONG inclusive). +* +* Returns value of nbits located at the @start bit offset within the @map +* memory region. +*/ +static inline unsigned long bitmap_get_value (const unsigned long *map, +unsigned long start, +unsigned long nbits) +{ +const size_t index =BIT_WORD (start); +const unsigned long offset = start %BITS_PER_LONG ; +const unsigned long ceiling =round_up (start +1 ,BITS_PER_LONG ); +const unsigned long space = ceiling - start; +unsigned long value_low, value_high; + +if (space >= nbits) +return (map[index] >> offset) &GENMASK (nbits -1 ,0 ); +else { + value_low = map[index] &BITMAP_FIRST_WORD_MASK (start); + value_high = map[index +1 ] &BITMAP_LAST_WORD_MASK (start + nbits); +return (value_low >> offset) | (value_high << space); + } +} + +/** +* bitmap_set_value - set value within a memory region +* @map: address to the bitmap memory region +* @nbits: size of map in bits +* @value: value of clump +* @value_width: size of value in bits (must be between 1 and BITS_PER_LONG inclusive) +* @start: bit offset of the value +*/ +static inline void bitmap_set_value (unsigned long *map,unsigned long nbits, +unsigned long value,unsigned long value_width, +unsigned long start) +{ +const unsigned long index =BIT_WORD (start); +const unsigned long length =BIT_WORD (nbits); +const unsigned long offset = start %BITS_PER_LONG ; +const unsigned long ceiling =round_up (start +1 ,BITS_PER_LONG ); +const unsigned long space = ceiling - start; + + value &=GENMASK (value_width -1 ,0 ); + +if (space >= value_width) { + map[index] &= ~(GENMASK (value_width -1 ,0 ) << offset); + map[index] |= value << offset; + }else { + map[index +0 ] &= ~BITMAP_FIRST_WORD_MASK (start); + map[index +0 ] |= value << offset; + +if (index +1 >= length) +__builtin_unreachable (); + + map[index +1 ] &= ~BITMAP_LAST_WORD_MASK (start + value_width); + map[index +1 ] |= value >> space; + } +} + +/** +* for_each_set_clump - iterate over bitmap for each clump with set bits +* @start: bit offset to start search and to store the current iteration offset +* @clump: location to store copy of current 8-bit clump +* @bits: bitmap address to base the search on +* @size: bitmap size in number of bits +* @clump_size: clump size in bits +*/ +#define for_each_set_clump (start, clump, bits, size, clump_size) \ + for ((start) = find_first_clump(&(clump), (bits), (size), (clump_size)); \ + (start) < (size); \ + (start) = find_next_clump(&(clump), (bits), (size), (start) + (clump_size), (clump_size))) + +#endif /* __CLUMP_BITS_H */
Signature · 1 line
2.29.0
Re: [PATCH 1/5] clump_bits: Introduce the for_each_set_clump macro
Quoted 15 lines · Syed Nayyar Waris
This macro iterates for each group of bits (clump) with set bits, within a bitmap memory region. For each iteration, "start" is set to the bit offset of the found clump, while the respective clump value is stored to the location pointed by "clump". Additionally, the
bitmap_get_value()andbitmap_set_value()functions are introduced to respectively get and set a value of n-bits in a bitmap memory region. The n-bits can have any size from 1 toBITS_PER_LONG. size less than 1 or more thanBITS_PER_LONGcauses undefined behaviour. Moreover, during setting value of n-bit in bitmap, if a situation arise that the width of next n-bit is exceeding the word boundary, then it will divide itself such that some portion of it is stored in that word, while the remaining portion is stored in the next higher word. Similar situation occurs while retrieving the value from bitmap.
Quoted 11 lines
GCC gives warning in
bitmap_set_value(): https://godbolt.org/z/rjx34r
Add explicit check to see if the value being written into the bitmap does not fall outside the bitmap.
The situation that it is falling outside would never be possible in the code because the boundaries are required to be correct before the function is called. The responsibility is on the caller for ensuring the boundaries are correct.
The code change is simply to silence the GCC warning messages because GCC is not aware that the boundaries have already been checked. As such, we're better off using__builtin_unreachable()here because we can avoid the latency of the conditional check entirely.
Didn't the __builtin_unreachable() end up leading to an objtool warning about incorrect stack frames for the code path that leads into the undefined behavior? I thought I saw a message from the 0day build bot about that and didn't expect to see it again after that.
Can you actually measure any performance difference compared to BUG_ON() that avoids the undefined behavior? Practically all CPUs from the past 20 years have branch predictors that should completely hide measurable overhead from this.
Arnd
Re: [PATCH 1/5] clump_bits: Introduce the for_each_set_clump macro
Quoted 33 lines · Arnd Bergmann
Quoted 15 lines · Syed Nayyar Waris
This macro iterates for each group of bits (clump) with set bits, within a bitmap memory region. For each iteration, "start" is set to the bit offset of the found clump, while the respective clump value is stored to the location pointed by "clump". Additionally, the
bitmap_get_value()andbitmap_set_value()functions are introduced to respectively get and set a value of n-bits in a bitmap memory region. The n-bits can have any size from 1 toBITS_PER_LONG. size less than 1 or more thanBITS_PER_LONGcauses undefined behaviour. Moreover, during setting value of n-bit in bitmap, if a situation arise that the width of next n-bit is exceeding the word boundary, then it will divide itself such that some portion of it is stored in that word, while the remaining portion is stored in the next higher word. Similar situation occurs while retrieving the value from bitmap.Quoted 11 lines
GCC gives warning in
bitmap_set_value(): https://godbolt.org/z/rjx34r
Add explicit check to see if the value being written into the bitmap does not fall outside the bitmap.
The situation that it is falling outside would never be possible in the code because the boundaries are required to be correct before the function is called. The responsibility is on the caller for ensuring the boundaries are correct.
The code change is simply to silence the GCC warning messages because GCC is not aware that the boundaries have already been checked. As such, we're better off using__builtin_unreachable()here because we can avoid the latency of the conditional check entirely.Didn't the
__builtin_unreachable()end up leading to an objtool warning about incorrect stack frames for the code path that leads into the undefined behavior? I thought I saw a message from the 0day build bot about that and didn't expect to see it again after that.
Quoted 6 lines
Can you actually measure any performance difference compared to
BUG_ON()that avoids the undefined behavior? Practically all CPUs from the past 20 years have branch predictors that should completely hide measurable overhead from this.Arnd
When I initially recommended using __builtin_unreachable(), I was anticipating the use of bitmap_set_value() in kernel at large -- so the possible performance hit from a conditional check was a concern for me. However, now that we're restricting the scope of bitmap_set_value() to only the GPIO subsystem, such optimization is no longer a major concern I feel: gpio-xilinx is the only driver utilizing bitmap_set_value() -- and we know it won't be called in a loop -- so whatever hypothetical performance hit there might be is inconsequential in the end.
Instead, we should focus on code clarity now. I believe it makes sense given the new scope of this function to revert back to the earlier suggestion of passing in and checking the boundary explicitly, and to remove the __builtin_unreachable() call for now. If bitmap_set_value() becomes available to the rest of the kernel in the future, we can reconsider whether or not to use __builtin_unreachable().
William Breathitt Gray
PATCH 2/5 lib/test_bitmap.c: Add for_each_set_clump test cases
In reply to Syed Nayyar WarisThe introduction of the generic for_each_set_clump macro need test cases to verify the implementation. This patch adds test cases for scenarios in which clump sizes are 8 bits, 24 bits, 30 bits and 6 bits. The cases contain situations where clump is getting split at the word boundary and also when zeroes are present in the start and middle of bitmap.
--- lib/test_bitmap.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 144 insertions(+)
lib/test_bitmap.c +144 -0
@@ -13,6 +13,7 @@#include <linux/slab.h> #include <linux/string.h> #include <linux/uaccess.h> +#include <../drivers/gpio/clump_bits.h> #include "../tools/testing/selftests/kselftest_module.h"
@@ -155,6 +156,37 @@ static bool __init __check_eq_clump8(const char *srcfile, unsigned int line,return true; } +static bool __init __check_eq_clump (const char *srcfile,unsigned int line, +const unsigned int offset, +const unsigned int size, +const unsigned long *const clump_exp, +const unsigned long *const clump, +const unsigned long clump_size) +{ +unsigned long exp; + +if (offset >= size) { +pr_warn ("[%s:%u] bit offset for clump out-of-bounds: expected less than %u, got %u\n" , + srcfile, line, size, offset); +return false; + } + + exp = clump_exp[offset / clump_size]; +if (!exp) { +pr_warn ("[%s:%u] bit offset for zero clump: expected nonzero clump, got bit offset %u with clump value 0" , + srcfile, line, offset); +return false; + } + +if (*clump != exp) { +pr_warn ("[%s:%u] expected clump value of 0x%lX, got clump value of 0x%lX" , + srcfile, line, exp, *clump); +return false; + } + +return true; +} +#define __expect_eq (suffix, ...) \ ({ \ int result = 0; \
@@ -172,6 +204,7 @@ static bool __init __check_eq_clump8(const char *srcfile, unsigned int line,#define expect_eq_pbl (...) __expect_eq(pbl, ##__VA_ARGS__)#define expect_eq_u32_array (...) __expect_eq(u32_array, ##__VA_ARGS__)#define expect_eq_clump8 (...) __expect_eq(clump8, ##__VA_ARGS__) +#define expect_eq_clump (...) __expect_eq(clump, ##__VA_ARGS__)static void __init test_zero_clear (void ) {
@@ -530,6 +563,28 @@ static void noinline __init test_mem_optimisations(void) } } +static const unsigned long clump_bitmap_data[]__initconst = { +0x38000201 , +0x05ff0f38 , +0xeffedcba , +0xbbbbabcd , +0x000000aa , +0x000000aa , +0x00ff0000 , +0xaaaaaa00 , +0xff000000 , +0x00aa0000 , +0x00000000 , +0x00000000 , +0x00000000 , +0x0f000000 , +0x00ff0000 , +0xaaaaaa00 , +0xff000000 , +0x00aa0000 , +0x00000ac0 , +}; +static const unsigned char clump_exp[]__initconst = {0x01 ,/* 1 bit set */ 0x02 ,/* non-edge 1 bit set */
@@ -541,6 +596,94 @@ static const unsigned char clump_exp[] __initconst = { 0x05, /* non-adjacent 2 bits set */ }; +static const unsigned long clump_exp1[]__initconst = { +0x01 ,/* 1 bit set */ +0x02 ,/* non-edge 1 bit set */ +0x00 ,/* zero bits set */ +0x38 ,/* 3 bits set across 4-bit boundary */ +0x38 ,/* Repeated clump */ +0x0F ,/* 4 bits set */ +0xFF ,/* all bits set */ +0x05 ,/* non-adjacent 2 bits set */ +}; + +static const unsigned long clump_exp2[]__initconst = { +0xfedcba ,/* 24 bits */ +0xabcdef , +0xaabbbb ,/* Clump split between 2 words */ +0x000000 ,/* zeroes in between */ +0x0000aa , +0x000000 , +0x0000ff , +0xaaaaaa , +0x000000 , +0x0000ff , +}; + +static const unsigned long clump_exp3[]__initconst = { +0x00000000 ,/* starting with 0s*/ +0x00000000 ,/* All 0s */ +0x00000000 , +0x00000000 , +0x3f00000f ,/* Non zero set */ +0x2aa80003 , +0x00000aaa , +0x00003fc0 , +}; + +static const unsigned long clump_exp4[]__initconst = { +0x00 , +0x2b , +}; + +struct clump_test_data_params { + DECLARE_BITMAP(data, 256); +unsigned long count; +unsigned long offset; +unsigned long limit; +unsigned long clump_size; +unsigned long const *exp; +}; + +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}, + {{0 },1 ,18 ,18 ,6 , clump_exp4} }; + +static void __init prepare_test_data (unsigned int index) +{ +int i; +unsigned long width =0 ; + +for (i =0 ; i < clump_test_data[index].count; i++) { +bitmap_set_value (clump_test_data[index].data,256 , + clump_bitmap_data[(clump_test_data[index].offset)++],32 , width); + width +=32 ; + } +} + +static void __init execute_for_each_set_clump_test (unsigned int index) +{ +unsigned long start, clump; + +for_each_set_clump (start, clump, clump_test_data[index].data, + clump_test_data[index].limit, + clump_test_data[index].clump_size) +expect_eq_clump (start, clump_test_data[index].limit, clump_test_data[index].exp, + &clump, clump_test_data[index].clump_size); +} + +static void __init test_for_each_set_clump (void ) +{ +unsigned int i; + +for (i =0 ; i <ARRAY_SIZE (clump_test_data); i++) { +prepare_test_data (i); +execute_for_each_set_clump_test (i); + } +} +static void __init test_for_each_set_clump8 (void ) {#define CLUMP_EXP_NUMBITS 64
@@ -631,6 +774,7 @@ static void __init selftest(void)test_bitmap_parselist ();test_mem_optimisations (); test_for_each_set_clump8(); + test_for_each_set_clump();test_bitmap_cut (); }
Signature · 1 line
2.29.0
kernel test robot
build botRe: [PATCH 2/5] lib/test_bitmap.c: Add for_each_set_clump test cases
I love your patch! Yet something to improve:
Hi Syed,
I love your patch! Yet something to improve:
[auto build test ERROR on bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb]
url: https://github.com/0day-ci/linux/commits/Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20201226-144926 base: bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb config: arm-randconfig-r024-20201226 (attached as .config) compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project cee1e7d14f4628d6174b33640d502bff3b54ae45)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/0day-ci/linux/commit/18673b78ae76b6db62549f983d83455b18d31865
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20201226-144926 git checkout 18673b78ae76b6db62549f983d83455b18d31865
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
All errors (new ones prefixed by >>):
>> ld.lld: error: undefined symbol: find_next_clump >>> referenced by test_bitmap.c >>> test_bitmap.o:(execute_for_each_set_clump_test) in archive lib/built-in.a >>> referenced by test_bitmap.c >>> test_bitmap.o:(execute_for_each_set_clump_test) in archive lib/built-in.a >>> did you mean: find_next_clump8 >>> defined in: lib/built-in.a(find_bit.o)
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
kernel test robot
build botRe: [PATCH 2/5] lib/test_bitmap.c: Add for_each_set_clump test cases
In reply to Syed Nayyar WarisI love your patch! Yet something to improve:
Hi Syed,
I love your patch! Yet something to improve:
[auto build test ERROR on bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb]
url: https://github.com/0day-ci/linux/commits/Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20201226-144926 base: bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb config: i386-randconfig-c001-20201226 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/18673b78ae76b6db62549f983d83455b18d31865 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20201226-144926 git checkout 18673b78ae76b6db62549f983d83455b18d31865 # save the attached .config to linux build tree make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "find_next_clump" [lib/test_bitmap.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
kernel test robot
build botRe: [PATCH 2/5] lib/test_bitmap.c: Add for_each_set_clump test cases
In reply to Syed Nayyar WarisI love your patch! Yet something to improve:
Hi Syed,
I love your patch! Yet something to improve:
[auto build test ERROR on bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb]
url: https://github.com/0day-ci/linux/commits/Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20201226-144926 base: bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb config: arm-allyesconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/18673b78ae76b6db62549f983d83455b18d31865
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20201226-144926 git checkout 18673b78ae76b6db62549f983d83455b18d31865
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
All errors (new ones prefixed by >>):
arm-linux-gnueabi-ld: lib/test_bitmap.o: in function `test_bitmap_init':
test_bitmap.c:(.init.text+0x1f24): undefined reference to `find_next_clump'
>> arm-linux-gnueabi-ld: test_bitmap.c:(.init.text+0x2038): undefined reference to `find_next_clump'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
A message that is not in the archive (sent off-list or not yet received): CAHp75VcSsfDKY3w4ufZktXzRB=GiObAV6voPfmeAHcbdwX0uqg@mail.gmail.com
Re: [PATCH 2/5] lib/test_bitmap.c: Add for_each_set_clump test cases
Quoted 13 lines · Andy Shevchenko
Quoted 7 lines · Syed Nayyar Waris
The introduction of the generic
for_each_set_clumpmacro need test cases to verify the implementation. This patch adds test cases for scenarios in which clump sizes are 8 bits, 24 bits, 30 bits and 6 bits. The cases contain situations where clump is getting split at the word boundary and also when zeroes are present in the start and middle of bitmap.
Quoted 1 line
You have to split it to a separate test under
drivers/gpio, because now it has no sense to be like this.
Hi Andy,
How do I split it into separate test under drivers/gpio ? I have thought of making a test_clump_bits.c file in drivers/gpio. But how do I integrate this test file so that tests are executed at runtime? Similar to tests in lib/test_bitmap.c ?
I believe I need to make changes in config files so that tests in test_clump_bits.c ( in drivers/gpio ) are executed at runtime. Could you please provide some steps on how to do that. Thank You !
Regards
Syed Nayyar Waris
Re: [PATCH 2/5] lib/test_bitmap.c: Add for_each_set_clump test cases
Quoted 25 lines · Syed Nayyar Waris
Quoted 13 lines · Andy Shevchenko
Quoted 7 lines · Syed Nayyar Waris
The introduction of the generic
for_each_set_clumpmacro need test cases to verify the implementation. This patch adds test cases for scenarios in which clump sizes are 8 bits, 24 bits, 30 bits and 6 bits. The cases contain situations where clump is getting split at the word boundary and also when zeroes are present in the start and middle of bitmap.Quoted 1 line
You have to split it to a separate test under
drivers/gpio, because now it has no sense to be like this.Hi Andy,
How do I split it into separate test under
drivers/gpio? I have thought of making atest_clump_bits.cfile indrivers/gpio. But how do I integrate this test file so that tests are executed at runtime? Similar to tests inlib/test_bitmap.c?
Quoted 6 lines
I believe I need to make changes in config files so that tests in
test_clump_bits.c( indrivers/gpio) are executed at runtime. Could you please provide some steps on how to do that. Thank You !Regards
Syed Nayyar Waris
Hi Andy, could you please help me on the above. Thanks !
Regards
Syed Nayyar Waris
PATCH 3/5 gpio: thunderx: Utilize for_each_set_clump macro
In reply to Syed Nayyar WarisThis patch reimplements the thunderx_gpio_set_multiple function in drivers/gpio/gpio-thunderx.c to use the new for_each_set_clump macro. Instead of looping for each bank in thunderx_gpio_set_multiple function, now we can skip bank which is not set and save cycles.
--- drivers/gpio/gpio-thunderx.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/gpio/gpio-thunderx.c +8 -4
@@ -16,6 +16,7 @@#include <linux/pci.h> #include <linux/spinlock.h> #include <asm-generic/msi.h> +#include <../drivers/gpio/clump_bits.h> #define GPIO_RX_DAT 0x0
@@ -275,12 +276,15 @@ static void thunderx_gpio_set_multiple(struct gpio_chip *chip, unsigned long *bits) {int bank; -u64 set_bits, clear_bits; +unsigned long set_bits, clear_bits, gpio_mask; +unsigned long offset; +struct thunderx_gpio *txgpio =gpiochip_get_data (chip); -for (bank =0 ; bank <= chip->ngpio /64 ; bank++) { - set_bits = bits[bank] & mask[bank]; - clear_bits = ~bits[bank] & mask[bank]; +for_each_set_clump (offset, gpio_mask, mask, chip->ngpio,64 ) { + bank = offset /64 ; + set_bits = bits[bank] & gpio_mask; + clear_bits = ~bits[bank] & gpio_mask;writeq (set_bits, txgpio->register_base + (bank *GPIO_2ND_BANK ) +GPIO_TX_SET );writeq (clear_bits, txgpio->register_base + (bank *GPIO_2ND_BANK ) +GPIO_TX_CLR ); }
Signature · 1 line
2.29.0
kernel test robot
build botRe: [PATCH 3/5] gpio: thunderx: Utilize for_each_set_clump macro
I love your patch! Yet something to improve:
Hi Syed,
I love your patch! Yet something to improve:
[auto build test ERROR on bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb]
url: https://github.com/0day-ci/linux/commits/Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20201226-144926 base: bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb config: x86_64-allyesconfig (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/ceadf04c7029acf863c15e72133acfbec3601aba git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20201226-144926 git checkout ceadf04c7029acf863c15e72133acfbec3601aba # save the attached .config to linux build tree make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
All errors (new ones prefixed by >>):
ld: lib/test_bitmap.o: in function `test_bitmap_init':
test_bitmap.c:(.init.text+0x193a): undefined reference to `find_next_clump'
ld: test_bitmap.c:(.init.text+0x1aa8): undefined reference to `find_next_clump'
ld: drivers/gpio/gpio-thunderx.o: in function `thunderx_gpio_set_multiple':
gpio-thunderx.c:(.text+0x7b9): undefined reference to `find_next_clump'
>> ld: gpio-thunderx.c:(.text+0x88e): undefined reference to `find_next_clump'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
kernel test robot
build botRe: [PATCH 3/5] gpio: thunderx: Utilize for_each_set_clump macro
In reply to Syed Nayyar WarisI love your patch! Yet something to improve:
Hi Syed,
I love your patch! Yet something to improve:
[auto build test ERROR on bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb]
url: https://github.com/0day-ci/linux/commits/Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20201226-144926 base: bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb config: s390-allyesconfig (attached as .config) compiler: s390-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/ceadf04c7029acf863c15e72133acfbec3601aba
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20201226-144926 git checkout ceadf04c7029acf863c15e72133acfbec3601aba
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=s390
If you fix the issue, kindly add following tag as appropriate
All errors (new ones prefixed by >>):
s390-linux-ld: lib/test_bitmap.o: in function `test_bitmap_init':
test_bitmap.c:(.init.text+0x25fe): undefined reference to `find_next_clump'
s390-linux-ld: test_bitmap.c:(.init.text+0x27f0): undefined reference to `find_next_clump'
s390-linux-ld: drivers/gpio/gpio-thunderx.o: in function `thunderx_gpio_set_multiple':
>> gpio-thunderx.c:(.text+0x734): undefined reference to `find_next_clump' >> s390-linux-ld: gpio-thunderx.c:(.text+0x850): undefined reference to `find_next_clump'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
PATCH 4/5 gpio: xilinx: Utilize generic bitmap_get_value and _set_value
In reply to Syed Nayyar WarisThis patch reimplements the xgpio_set_multiple() function in drivers/gpio/gpio-xilinx.c to use the new generic functions:bitmap_get_value() and bitmap_set_value(). The code is now simpler to read and understand. Moreover, instead of looping for each bit in xgpio_set_multiple() function, now we can check each channel at a time and save cycles.
--- drivers/gpio/gpio-xilinx.c | 66 +++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 33 deletions(-)
drivers/gpio/gpio-xilinx.c +33 -33
@@ -14,6 +14,7 @@#include <linux/io.h> #include <linux/gpio/driver.h> #include <linux/slab.h> +#include <../drivers/gpio/clump_bits.h> /* Register Offset Definitions */ #define XGPIO_DATA_OFFSET (0x0)/* Data register */
@@ -138,37 +139,37 @@ static void xgpio_set_multiple(struct gpio_chip *gc, unsigned long *mask, {unsigned long flags;struct xgpio_instance *chip =gpiochip_get_data (gc); -int index =xgpio_index (chip,0 ); -int offset, i; - -spin_lock_irqsave (&chip->gpio_lock[index], flags); - -/* Write to GPIO signals */ -for (i =0 ; i < gc->ngpio; i++) { -if (*mask ==0 ) -break ; -/* Once finished with an index write it out to the register */ -if (index !=xgpio_index (chip, i)) { -xgpio_writereg (chip->regs +XGPIO_DATA_OFFSET + - index *XGPIO_CHANNEL_OFFSET , - chip->gpio_state[index]); -spin_unlock_irqrestore (&chip->gpio_lock[index], flags); - index =xgpio_index (chip, i); -spin_lock_irqsave (&chip->gpio_lock[index], flags); - } -if (__test_and_clear_bit (i, mask)) { - offset =xgpio_offset (chip, i); -if (test_bit (i, bits)) - chip->gpio_state[index] |=BIT (offset); -else - chip->gpio_state[index] &= ~BIT (offset); - } - } - -xgpio_writereg (chip->regs +XGPIO_DATA_OFFSET + - index *XGPIO_CHANNEL_OFFSET , chip->gpio_state[index]); - -spin_unlock_irqrestore (&chip->gpio_lock[index], flags); +u32 *const state = chip->gpio_state; +unsigned int *const width = chip->gpio_width; + +DECLARE_BITMAP (old,64 ); +DECLARE_BITMAP (new,64 ); +DECLARE_BITMAP (changed,64 ); + +spin_lock_irqsave (&chip->gpio_lock[0 ], flags); +spin_lock (&chip->gpio_lock[1 ]); + +bitmap_set_value (old,64 , state[0 ], width[0 ],0 ); +bitmap_set_value (old,64 , state[1 ], width[1 ], width[0 ]); +bitmap_replace (new, old, bits, mask, gc->ngpio); + +bitmap_set_value (old,64 , state[0 ],32 ,0 ); +bitmap_set_value (old,64 , state[1 ],32 ,32 ); + state[0 ] =bitmap_get_value (new,0 , width[0 ]); + state[1 ] =bitmap_get_value (new, width[0 ], width[1 ]); +bitmap_set_value (new,64 , state[0 ],32 ,0 ); +bitmap_set_value (new,64 , state[1 ],32 ,32 ); +bitmap_xor (changed, old, new,64 ); + +if (((u32 *)changed)[0 ]) +xgpio_writereg (chip->regs +XGPIO_DATA_OFFSET , + state[0 ]); +if (((u32 *)changed)[1 ]) +xgpio_writereg (chip->regs +XGPIO_DATA_OFFSET + +XGPIO_CHANNEL_OFFSET , state[1 ]); + +spin_unlock (&chip->gpio_lock[1 ]); +spin_unlock_irqrestore (&chip->gpio_lock[0 ], flags); }/**
@@ -292,6 +293,7 @@ static int xgpio_probe(struct platform_device *pdev) chip->gpio_width[0 ] =32 ;spin_lock_init (&chip->gpio_lock[0 ]); +spin_lock_init (&chip->gpio_lock[1 ]);if (of_property_read_u32 (np,"xlnx,is-dual" , &is_dual)) is_dual =0 ;
@@ -313,8 +315,6 @@ static int xgpio_probe(struct platform_device *pdev)if (of_property_read_u32 (np,"xlnx,gpio2-width" , &chip->gpio_width[1 ])) chip->gpio_width[1 ] =32 ; - -spin_lock_init (&chip->gpio_lock[1 ]); } chip->gc.base =-1 ;
Signature · 1 line
2.29.0
Re: [PATCH 4/5] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
Quoted 11 lines · Syed Nayyar Waris
This patch reimplements the
xgpio_set_multiple()function indrivers/gpio/gpio-xilinx.cto use the new generic functions:bitmap_get_value()andbitmap_set_value(). The code is now simpler to read and understand. Moreover, instead of looping for each bit inxgpio_set_multiple()function, now we can check each channel at a time and save cycles.Cc William Breathitt Gray <[email protected]>Cc Bartosz Golaszewski <[email protected]>Cc Michal Simek <[email protected]>Signed-off-by Syed Nayyar Waris <[email protected]>
(...)
Quoted 1 line
+#include <
../drivers/gpio/clump_bits.h>
What is this?
Isn't a simple
#include "clump_bits.h"
enough?
We need an ACK from the Xilinx people that they think this actually improves the readability and maintainability of their driver.
Yours,
Linus Walleij
Re: [PATCH 4/5] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
Hi, +Srinivas,
Quoted 22 lines · Linus Walleij
Quoted 11 lines · Syed Nayyar Waris
This patch reimplements the
xgpio_set_multiple()function indrivers/gpio/gpio-xilinx.cto use the new generic functions:bitmap_get_value()andbitmap_set_value(). The code is now simpler to read and understand. Moreover, instead of looping for each bit inxgpio_set_multiple()function, now we can check each channel at a time and save cycles.Cc William Breathitt Gray <[email protected]>Cc Bartosz Golaszewski <[email protected]>Cc Michal Simek <[email protected]>Signed-off-by Syed Nayyar Waris <[email protected]>(...)
Quoted 1 line
+#include <
../drivers/gpio/clump_bits.h>What is this?
Isn't a simple
Quoted 7 lines
#include "
clump_bits.h"enough?
We need an ACK from the Xilinx people that they think this actually improves the readability and maintainability of their driver.
Srinivas is going to send some patches against this driver. That's why please take a look if both of these changes are fitting together.
Thanks,
Michal
kernel test robot
build botRe: [PATCH 4/5] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
In reply to Syed Nayyar WarisI love your patch! Perhaps something to improve:
Hi Syed,
I love your patch! Perhaps something to improve:
[auto build test WARNING on bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb]
url: https://github.com/0day-ci/linux/commits/Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20201226-144926 base: bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb
:::::: branch date: 8 hours ago
:::::: commit date: 8 hours ago
config: mips-randconfig-s032-20201226 (attached as .config)
compiler: mips64-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.3-184-g1b896707-dirty
# https://github.com/0day-ci/linux/commit/668f2f620cce361abead957de41153c84fbed6c9
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20201226-144926 git checkout 668f2f620cce361abead957de41153c84fbed6c9
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=mips
If you fix the issue, kindly add following tag as appropriate
"sparse warnings: (new ones prefixed by >>)"
command-line: note: in included file:
builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_ACQUIRE redefined builtin:0:0: sparse: this was the original definition builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_SEQ_CST redefined builtin:0:0: sparse: this was the original definition builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_ACQ_REL redefined builtin:0:0: sparse: this was the original definition builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_RELEASE redefined builtin:0:0: sparse: this was the original definition drivers/gpio/gpio-xilinx.c: note: in included file:
>> include/../drivers/gpio/clump_bits.h:48:30: sparse: sparse: invalid access past the end of 'new' (8 8)
include/../drivers/gpio/clump_bits.h:49:63: sparse: sparse: shift too big (64) for type unsigned long
vim +/new +48 include/../drivers/gpio/clump_bits.h
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 5
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 6 /**
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 7 * find_next_clump - find next clump with set bits in a memory region
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 8 * @clump: location to store copy of found clump
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 9 * @addr: address to base the search on
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 10 * @size: bitmap size in number of bits
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 11 * @offset: bit offset at which to start searching
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 12 * @clump_size: clump size in bits
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 13 *
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 14 * Returns the bit offset for the next set clump; the found clump value is
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 15 * copied to the location pointed by @clump. If no bits are set, returns @size.
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 16 */
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 17 extern unsigned long find_next_clump(unsigned long *clump,
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 18 const unsigned long *addr,
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 19 unsigned long size, unsigned long offset,
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 20 unsigned long clump_size);
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 21
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 22 #define find_first_clump(clump, bits, size, clump_size) \
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 23 find_next_clump((clump), (bits), (size), 0, (clump_size))
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 24
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 25 /**
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 26 * bitmap_get_value - get a value of n-bits from the memory region
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 27 * @map: address to the bitmap memory region
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 28 * @start: bit offset of the n-bit value
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 29 * @nbits: size of value in bits (must be between 1 and BITS_PER_LONG inclusive).
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 30 *
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 31 * Returns value of nbits located at the @start bit offset within the @map
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 32 * memory region.
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 33 */
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 34 static inline unsigned long bitmap_get_value(const unsigned long *map,
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 35 unsigned long start,
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 36 unsigned long nbits)
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 37 {
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 38 const size_t index = BIT_WORD(start);
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 39 const unsigned long offset = start % BITS_PER_LONG;
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 40 const unsigned long ceiling = round_up(start + 1, BITS_PER_LONG);
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 41 const unsigned long space = ceiling - start;
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 42 unsigned long value_low, value_high;
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 43
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 44 if (space >= nbits)
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 45 return (map[index] >> offset) & GENMASK(nbits - 1, 0);
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 46 else {
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 47 value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 @48 value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 49 return (value_low >> offset) | (value_high << space);
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 50 }
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 51 }d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 52
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
_______________________________________________
kbuild mailing list -- kbuild(a)lists.01.org
To unsubscribe send an email to [email protected]
kernel test robot
build botRe: [PATCH 4/5] gpio: xilinx: Utilize generic bitmap_get_value and _set_value
In reply to Syed Nayyar WarisI love your patch! Perhaps something to improve:
Hi Syed,
I love your patch! Perhaps something to improve:
[auto build test WARNING on bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb]
url: https://github.com/0day-ci/linux/commits/Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20201226-144926 base: bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb
:::::: branch date: 9 hours ago
:::::: commit date: 9 hours ago
config: i386-randconfig-s002-20201226 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.3-184-g1b896707-dirty
# https://github.com/0day-ci/linux/commit/668f2f620cce361abead957de41153c84fbed6c9
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20201226-144926
git checkout 668f2f620cce361abead957de41153c84fbed6c9
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 If you fix the issue, kindly add following tag as appropriate
"sparse warnings: (new ones prefixed by >>)"
drivers/gpio/gpio-xilinx.c: note: in included file:
>> include/../drivers/gpio/clump_bits.h:84:44: sparse: sparse: shift too big (32) for type unsigned long
include/../drivers/gpio/clump_bits.h:49:63: sparse: sparse: shift too big (32) for type unsigned long
vim +84 include/../drivers/gpio/clump_bits.h
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 52
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 53 /**
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 54 * bitmap_set_value - set value within a memory region
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 55 * @map: address to the bitmap memory region
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 56 * @nbits: size of map in bits
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 57 * @value: value of clump
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 58 * @value_width: size of value in bits (must be between 1 and BITS_PER_LONG inclusive)
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 59 * @start: bit offset of the value
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 60 */
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 61 static inline void bitmap_set_value(unsigned long *map, unsigned long nbits,
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 62 unsigned long value, unsigned long value_width,
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 63 unsigned long start)
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 64 {
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 65 const unsigned long index = BIT_WORD(start);
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 66 const unsigned long length = BIT_WORD(nbits);
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 67 const unsigned long offset = start % BITS_PER_LONG;
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 68 const unsigned long ceiling = round_up(start + 1, BITS_PER_LONG);
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 69 const unsigned long space = ceiling - start;
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 70
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 71 value &= GENMASK(value_width - 1, 0);
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 72
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 73 if (space >= value_width) {
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 74 map[index] &= ~(GENMASK(value_width - 1, 0) << offset);
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 75 map[index] |= value << offset;
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 76 } else {
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 77 map[index + 0] &= ~BITMAP_FIRST_WORD_MASK(start);
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 78 map[index + 0] |= value << offset;
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 79
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 80 if (index + 1 >= length)
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 81 __builtin_unreachable();
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 82
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 83 map[index + 1] &= ~BITMAP_LAST_WORD_MASK(start + value_width);
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 @84 map[index + 1] |= value >> space;
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 85 }
d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 86 }d14f564f0d6c963 Syed Nayyar Waris 2020-12-26 87
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
_______________________________________________
kbuild mailing list -- kbuild(a)lists.01.org
To unsubscribe send an email to kbuild-leave(a)lists.01.org
PATCH 5/5 gpio: xilinx: Add extra check if sum of widths exceed 64
In reply to Syed Nayyar WarisAdd extra check to see if sum of widths does not exceed 64. If it exceeds then return -EINVAL alongwith appropriate error message.
--- drivers/gpio/gpio-xilinx.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/gpio/gpio-xilinx.c +6 -0
@@ -319,6 +319,12 @@ static int xgpio_probe(struct platform_device *pdev) chip->gc.base =-1 ; chip->gc.ngpio = chip->gpio_width[0 ] + chip->gpio_width[1 ]; + +if (chip->gc.ngpio >64 ) { +dev_err (&pdev->dev,"invalid configuration: number of GPIO is greater than 64" ); +return -EINVAL ; + } + chip->gc.parent = &pdev->dev; chip->gc.direction_input = xgpio_dir_in; chip->gc.direction_output = xgpio_dir_out;
Signature · 1 line
2.29.0
kernel test robot
build botRe: [PATCH 5/5] gpio: xilinx: Add extra check if sum of widths exceed 64
I love your patch! Perhaps something to improve:
Hi Syed,
I love your patch! Perhaps something to improve:
[auto build test WARNING on bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb]
url: https://github.com/0day-ci/linux/commits/Syed-Nayyar-Waris/Introduce-the-for_each_set_clump-macro/20201226-144926 base: bbe2ba04c5a92a49db8a42c850a5a2f6481e47eb config: i386-randconfig-m021-20201226 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
smatch warnings:drivers/gpio/gpio-xilinx.c:325 xgpio_probe() warn: inconsistent indenting
vim +325 drivers/gpio/gpio-xilinx.c
259
260 /**
261 * xgpio_of_probe - Probe method for the GPIO device.
262 * @pdev: pointer to the platform device
263 *
264 * Return:
265 * It returns 0, if the driver is bound to the GPIO device, or
266 * a negative value if there is an error.
267 */
268 static int xgpio_probe(struct platform_device *pdev)
269 {
270 struct xgpio_instance *chip;
271 int status = 0;
272 struct device_node *np = pdev->dev.of_node;
273 u32 is_dual;
274
275 chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
276 if (!chip)
277 return -ENOMEM;
278
279 platform_set_drvdata(pdev, chip);
280
281 /* Update GPIO state shadow register with default value */
282 of_property_read_u32(np, "xlnx,dout-default", &chip->gpio_state[0]);
283
284 /* Update GPIO direction shadow register with default value */
285 if (of_property_read_u32(np, "xlnx,tri-default", &chip->gpio_dir[0]))
286 chip->gpio_dir[0] = 0xFFFFFFFF;
287
288 /*
289 * Check device node and parent device node for device width
290 * and assume default width of 32
291 */
292 if (of_property_read_u32(np, "xlnx,gpio-width", &chip->gpio_width[0]))
293 chip->gpio_width[0] = 32;
294
295 spin_lock_init(&chip->gpio_lock[0]);
296 spin_lock_init(&chip->gpio_lock[1]);
297
298 if (of_property_read_u32(np, "xlnx,is-dual", &is_dual))
299 is_dual = 0;
300
301 if (is_dual) {
302 /* Update GPIO state shadow register with default value */
303 of_property_read_u32(np, "xlnx,dout-default-2",
304 &chip->gpio_state[1]);
305
306 /* Update GPIO direction shadow register with default value */
307 if (of_property_read_u32(np, "xlnx,tri-default-2",
308 &chip->gpio_dir[1]))
309 chip->gpio_dir[1] = 0xFFFFFFFF;
310
311 /*
312 * Check device node and parent device node for device width
313 * and assume default width of 32
314 */
315 if (of_property_read_u32(np, "xlnx,gpio2-width",
316 &chip->gpio_width[1]))
317 chip->gpio_width[1] = 32;
318 }
319
320 chip->gc.base = -1;
321 chip->gc.ngpio = chip->gpio_width[0] + chip->gpio_width[1];
322
323 if (chip->gc.ngpio > 64) {
324 dev_err(&pdev->dev, "invalid configuration: number of GPIO is greater than 64");
> 325 return -EINVAL;
326 }
327
328 chip->gc.parent = &pdev->dev;
329 chip->gc.direction_input = xgpio_dir_in;
330 chip->gc.direction_output = xgpio_dir_out;
331 chip->gc.get = xgpio_get;
332 chip->gc.set = xgpio_set;
333 chip->gc.set_multiple = xgpio_set_multiple;
334
335 chip->gc.label = dev_name(&pdev->dev);
336
337 chip->regs = devm_platform_ioremap_resource(pdev, 0);
338 if (IS_ERR(chip->regs)) {
339 dev_err(&pdev->dev, "failed to ioremap memory resource\n");
340 return PTR_ERR(chip->regs);
341 }
342
343 xgpio_save_regs(chip);
344
345 status = devm_gpiochip_add_data(&pdev->dev, &chip->gc, chip);
346 if (status) {
347 dev_err(&pdev->dev, "failed to add GPIO chip\n");
348 return status;
349 }
350
351 return 0;
352 }
353
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
Re: [PATCH 5/5] gpio: xilinx: Add extra check if sum of widths exceed 64
In reply to Syed Nayyar WarisQuoted 5 lines · Syed Nayyar Waris
Add extra check to see if sum of widths does not exceed 64. If it exceeds then return
-EINVALalongwith appropriate error message.Cc Michal Simek <[email protected]>Signed-off-by Syed Nayyar Waris <[email protected]>
Hello Syed,
This change is independent from the rest of this patchset so I recommend dropping this patch and instead resubmitting it separately as an independent patch submission.
William Breathitt Gray
Quoted 24 lines
--- drivers/gpio/gpio-xilinx.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/gpio/gpio-xilinx.c +6
-0@@ -319,6 +319,12 @@ static int xgpio_probe(struct platform_device *pdev) chip->gc.base =-1 ; chip->gc.ngpio = chip->gpio_width[0 ] + chip->gpio_width[1 ]; + +if (chip->gc.ngpio >64 ) { +dev_err (&pdev->dev,"invalid configuration: number of GPIO is greater than 64" ); +return -EINVAL ; + } + chip->gc.parent = &pdev->dev; chip->gc.direction_input = xgpio_dir_in; chip->gc.direction_output = xgpio_dir_out;Signature · 1 line
2.29.0
Re: [PATCH 5/5] gpio: xilinx: Add extra check if sum of widths exceed 64
In reply to Syed Nayyar WarisQuoted 17 lines · Syed Nayyar Waris
Add extra check to see if sum of widths does not exceed 64. If it exceeds then return
-EINVALalongwith appropriate error message.Cc Michal Simek <[email protected]>Signed-off-by Syed Nayyar Waris <[email protected]>--- drivers/gpio/gpio-xilinx.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/gpio/gpio-xilinx.c +0
-0@@ -319,6 +319,12 @@ static int xgpio_probe(struct platform_device *pdev) chip->gc.base =-1 ;
drivers/gpio/gpio-xilinx.c static int xgpio_probe(struct platform_device *pdev) · line 330
Quoted 10 lines
drivers/gpio/gpio-xilinx.c +6
-0chip->gc.ngpio = chip->gpio_width[0 ] + chip->gpio_width[1 ]; + +if (chip->gc.ngpio >64 ) { +dev_err (&pdev->dev,"invalid configuration: number of GPIO is greater than 64" ); +return -EINVAL ; + } + chip->gc.parent = &pdev->dev; chip->gc.direction_input = xgpio_dir_in; chip->gc.direction_output = xgpio_dir_out;
Thanks,
Michal
Quoted 2 lines · Syed Nayyar Waris
Since this patchset primarily affects GPIO drivers, would you like to pick it up through your GPIO tree?
Actually Bartosz is handling the GPIO patches for v5.12. I tried to merge the patch series before but failed for various reasons.
Yours,
Linus Walleij
Quoted 6 lines · Linus Walleij
Quoted 2 lines · Syed Nayyar Waris
Since this patchset primarily affects GPIO drivers, would you like to pick it up through your GPIO tree?
Quoted 6 lines
Actually Bartosz is handling the GPIO patches for v5.12. I tried to merge the patch series before but failed for various reasons.
Yours,
Linus Walleij
My info on this is a bit outdated - didn't Linus Torvalds reject these patches from Andrew Morton's PR? Or am I confusing this series with something else?
Bart
Quoted 7 lines · Bartosz Golaszewski
Quoted 6 lines · Linus Walleij
Quoted 2 lines · Syed Nayyar Waris
Since this patchset primarily affects GPIO drivers, would you like to pick it up through your GPIO tree?
Quoted 7 lines
Quoted 3 lines
Actually Bartosz is handling the GPIO patches for v5.12. I tried to merge the patch series before but failed for various reasons.
My info on this is a bit outdated - didn't Linus Torvalds reject these patches from Andrew Morton's PR? Or am I confusing this series with something else?
Linus T. told that it can be done inside GPIO realm. This version tries (badly in my opinion) to achieve that.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 13 lines · Andy Shevchenko
Quoted 10 lines · Bartosz Golaszewski
Quoted 9 lines · Linus Walleij
Quoted 2 lines · Syed Nayyar Waris
Since this patchset primarily affects GPIO drivers, would you like to pick it up through your GPIO tree?
Actually Bartosz is handling the GPIO patches for v5.12. I tried to merge the patch series before but failed for various reasons.
Quoted 7 lines
Quoted 3 lines
My info on this is a bit outdated - didn't Linus Torvalds reject these patches from Andrew Morton's PR? Or am I confusing this series with something else?
Linus T. told that it can be done inside GPIO realm. This version tries (badly in my opinion) to achieve that.
I'm seeing William and Arnd have some unaddressed issues with patch 1 (with using __builtin_unreachable()).
Admittedly I didn't follow the previous iterations too much so I may miss some history behind it. Why do the first two patches go into lib if this is supposed to be gpiolib-only?
Bartosz
Quoted 23 lines · Bartosz Golaszewski
Quoted 13 lines · Andy Shevchenko
Quoted 10 lines · Bartosz Golaszewski
Quoted 9 lines · Linus Walleij
Quoted 2 lines · Syed Nayyar Waris
Since this patchset primarily affects GPIO drivers, would you like to pick it up through your GPIO tree?
Actually Bartosz is handling the GPIO patches for v5.12. I tried to merge the patch series before but failed for various reasons.
Quoted 7 lines
Quoted 3 lines
My info on this is a bit outdated - didn't Linus Torvalds reject these patches from Andrew Morton's PR? Or am I confusing this series with something else?
Linus T. told that it can be done inside GPIO realm. This version tries (badly in my opinion) to achieve that.
Quoted 8 lines
I'm seeing William and Arnd have some unaddressed issues with patch 1 (with using
__builtin_unreachable()).Admittedly I didn't follow the previous iterations too much so I may miss some history behind it. Why do the first two patches go into lib if this is supposed to be gpiolib-only?
Bartosz
This patchset originally start out as a replacement for bitmap_get_value8/bitmap_set_value8/for_each_set_clump8, which are used outside of the GPIO subsystem. Over the course of the revisions, the scope of this patchset was reduced down and now it's only affecting GPIO drivers.
You're right that this shouldn't be going into lib anymore because it's gpiolib-only now. I expect the next revision of this patchset Syed submits will address that.
William Breathitt Gray
Hi,
Quoted 11 lines · Syed Nayyar Waris
Hello Linus,
Since this patchset primarily affects GPIO drivers, would you like to pick it up through your GPIO tree?
(Note: Patchset resent with the new macro and relevant functions shifted to a new header
clump_bits.h[Linus Torvalds])Michal,
What do you think of [PATCH 5/5]? Is the conditional check needed? And also does returning-EINVALlook good?
As was said would be better to handle it out of this series. And I expect none is really describing fpga designs by hand and using DT generator for it. But I can't see any issue with checking that we are not exceeding certain limit.
Just keep in your mind that every bank has max 32 lines.
It means if you say bank0 40, bank1 10 which is in total 50 it will pass your condition in 5/5.
It means maybe checking every bank separately is better approach.
Thanks,
Michal