Since "size" is an "unsigned int", the rvalue "size - 1" will still be
"unsigned int" according to the C standard (3.2.1.5 Usual arithmetic
conversions). Therefore, GENMASK(size - 1, 0) will always return 0UL. Those
are also caught by GCC (W=2):
./include/linux/find.h: In function 'find_first_bit':
./include/linux/bits.h:25:22: warning: comparison of unsigned expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((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:25:3: note: in expansion of macro '__is_constexpr'
25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
| ^~~~~~~~~~~~~~
./include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:119:31: note: in expansion of macro 'GENMASK'
119 | unsigned long val = *addr & GENMASK(size - 1, 0);
| ^~~~~~~
./include/linux/bits.h:25:34: warning: comparison of unsigned expression in '< 0' is always false [-Wtype-limits]
25 | __is_constexpr((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:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
| ^~~~~~~~~~~~~~~~~~~
./include/linux/find.h:119:31: note: in expansion of macro 'GENMASK'
119 | unsigned long val = *addr & GENMASK(size - 1, 0);
| ^~~~~~~
Signed-off-by: Qian Cai <[email protected]>
---
include/linux/find.h | 28 ++++++++--------------------
1 file changed, 8 insertions(+), 20 deletions(-)
diff --git a/include/linux/find.h b/include/linux/find.h
index 5bb6db213bcb..5ce2b17aea42 100644
--- a/include/linux/find.h
+++ b/include/linux/find.h
@@ -115,11 +115,8 @@ unsigned long find_next_zero_bit(const unsigned long *addr, unsigned long size,
static inline
unsigned long find_first_bit(const unsigned long *addr, unsigned long size)
{
- if (small_const_nbits(size)) {
- unsigned long val = *addr & GENMASK(size - 1, 0);
-
- return val ? __ffs(val) : size;
- }
+ if (small_const_nbits(size))
+ return size;
return _find_first_bit(addr, size);
}
@@ -140,11 +137,8 @@ unsigned long find_first_and_bit(const unsigned long *addr1,
const unsigned long *addr2,
unsigned long size)
{
- if (small_const_nbits(size)) {
- unsigned long val = *addr1 & *addr2 & GENMASK(size - 1, 0);
-
- return val ? __ffs(val) : size;
- }
+ if (small_const_nbits(size))
+ return size;
return _find_first_and_bit(addr1, addr2, size);
}
@@ -162,11 +156,8 @@ unsigned long find_first_and_bit(const unsigned long *addr1,
static inline
unsigned long find_first_zero_bit(const unsigned long *addr, unsigned long size)
{
- if (small_const_nbits(size)) {
- unsigned long val = *addr | ~GENMASK(size - 1, 0);
-
- return val == ~0UL ? size : ffz(val);
- }
+ if (small_const_nbits(size))
+ return size;
return _find_first_zero_bit(addr, size);
}
@@ -183,11 +174,8 @@ unsigned long find_first_zero_bit(const unsigned long *addr, unsigned long size)
static inline
unsigned long find_last_bit(const unsigned long *addr, unsigned long size)
{
- if (small_const_nbits(size)) {
- unsigned long val = *addr & GENMASK(size - 1, 0);
-
- return val ? __fls(val) : size;
- }
+ if (small_const_nbits(size))
+ return size;
return _find_last_bit(addr, size);
}
--
2.30.2
On Tue, Oct 26, 2021 at 10:41:08AM -0400, Qian Cai wrote:
> Since "size" is an "unsigned int", the rvalue "size - 1" will still be
> "unsigned int" according to the C standard (3.2.1.5 Usual arithmetic
> conversions). Therefore, GENMASK(size - 1, 0) will always return 0UL.
Huh?!
Have you run test_bitmap et al., btw?
--
With Best Regards,
Andy Shevchenko
On 10/26/21 11:10 AM, Andy Shevchenko wrote:
> On Tue, Oct 26, 2021 at 10:41:08AM -0400, Qian Cai wrote:
>> Since "size" is an "unsigned int", the rvalue "size - 1" will still be
>> "unsigned int" according to the C standard (3.2.1.5 Usual arithmetic
>> conversions). Therefore, GENMASK(size - 1, 0) will always return 0UL.
>
> Huh?!
>
> Have you run test_bitmap et al., btw?
Not yet. I'll setup a VM to run it soon.
On Tue, Oct 26, 2021 at 10:41:08AM -0400, Qian Cai wrote:
> Since "size" is an "unsigned int", the rvalue "size - 1" will still be
> "unsigned int" according to the C standard (3.2.1.5 Usual arithmetic
> conversions). Therefore, GENMASK(size - 1, 0) will always return 0UL. Those
> are also caught by GCC (W=2):
>
> ./include/linux/find.h: In function 'find_first_bit':
> ./include/linux/bits.h:25:22: warning: comparison of unsigned expression in '< 0' is always false [-Wtype-limits]
> 25 | __is_constexpr((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:25:3: note: in expansion of macro '__is_constexpr'
> 25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
> | ^~~~~~~~~~~~~~
> ./include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> 38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> | ^~~~~~~~~~~~~~~~~~~
> ./include/linux/find.h:119:31: note: in expansion of macro 'GENMASK'
> 119 | unsigned long val = *addr & GENMASK(size - 1, 0);
> | ^~~~~~~
> ./include/linux/bits.h:25:34: warning: comparison of unsigned expression in '< 0' is always false [-Wtype-limits]
> 25 | __is_constexpr((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:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> 38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> | ^~~~~~~~~~~~~~~~~~~
> ./include/linux/find.h:119:31: note: in expansion of macro 'GENMASK'
> 119 | unsigned long val = *addr & GENMASK(size - 1, 0);
> | ^~~~~~~
>
> Signed-off-by: Qian Cai <[email protected]>
> ---
> include/linux/find.h | 28 ++++++++--------------------
> 1 file changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/find.h b/include/linux/find.h
> index 5bb6db213bcb..5ce2b17aea42 100644
> --- a/include/linux/find.h
> +++ b/include/linux/find.h
> @@ -115,11 +115,8 @@ unsigned long find_next_zero_bit(const unsigned long *addr, unsigned long size,
> static inline
> unsigned long find_first_bit(const unsigned long *addr, unsigned long size)
> {
> - if (small_const_nbits(size)) {
> - unsigned long val = *addr & GENMASK(size - 1, 0);
> -
> - return val ? __ffs(val) : size;
> - }
> + if (small_const_nbits(size))
> + return size;
>
> return _find_first_bit(addr, size);
> }
[...]
Nice catch! I'm a bit concerned that small_const_nbits() will never
allow GENMASK() to be passed with size == 0, but the patch looks
good to me overall.
It's too late to merge it in 5.15, so I will add it in a spring merge
window - most probably in a bitmap branch for 5.17 or 5.18:
https://github.com/norov/linux/tree/bitmap-2022-Apr-01
Thanks,
Yury
On Tue, Oct 26, 2021 at 11:54:16AM -0700, Yury Norov wrote:
> On Tue, Oct 26, 2021 at 10:41:08AM -0400, Qian Cai wrote:
> > Since "size" is an "unsigned int", the rvalue "size - 1" will still be
> > "unsigned int" according to the C standard (3.2.1.5 Usual arithmetic
> > conversions). Therefore, GENMASK(size - 1, 0) will always return 0UL. Those
> > are also caught by GCC (W=2):
> >
> > ./include/linux/find.h: In function 'find_first_bit':
> > ./include/linux/bits.h:25:22: warning: comparison of unsigned expression in '< 0' is always false [-Wtype-limits]
> > 25 | __is_constexpr((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:25:3: note: in expansion of macro '__is_constexpr'
> > 25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
> > | ^~~~~~~~~~~~~~
> > ./include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > 38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > | ^~~~~~~~~~~~~~~~~~~
> > ./include/linux/find.h:119:31: note: in expansion of macro 'GENMASK'
> > 119 | unsigned long val = *addr & GENMASK(size - 1, 0);
> > | ^~~~~~~
> > ./include/linux/bits.h:25:34: warning: comparison of unsigned expression in '< 0' is always false [-Wtype-limits]
> > 25 | __is_constexpr((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:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > 38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > | ^~~~~~~~~~~~~~~~~~~
> > ./include/linux/find.h:119:31: note: in expansion of macro 'GENMASK'
> > 119 | unsigned long val = *addr & GENMASK(size - 1, 0);
> > | ^~~~~~~
> >
> > Signed-off-by: Qian Cai <[email protected]>
> > ---
> > include/linux/find.h | 28 ++++++++--------------------
> > 1 file changed, 8 insertions(+), 20 deletions(-)
> >
> > diff --git a/include/linux/find.h b/include/linux/find.h
> > index 5bb6db213bcb..5ce2b17aea42 100644
> > --- a/include/linux/find.h
> > +++ b/include/linux/find.h
> > @@ -115,11 +115,8 @@ unsigned long find_next_zero_bit(const unsigned long *addr, unsigned long size,
> > static inline
> > unsigned long find_first_bit(const unsigned long *addr, unsigned long size)
> > {
> > - if (small_const_nbits(size)) {
> > - unsigned long val = *addr & GENMASK(size - 1, 0);
> > -
> > - return val ? __ffs(val) : size;
> > - }
> > + if (small_const_nbits(size))
> > + return size;
> >
> > return _find_first_bit(addr, size);
> > }
>
> [...]
>
> Nice catch! I'm a bit concerned that small_const_nbits() will never
> allow GENMASK() to be passed with size == 0, but the patch looks
> good to me overall.
Can you explain to me how it is supposed to work?
For example,
x = 0xaa55;
size = 5;
printf("%lu\n", find_first_bit(&x, size));
In the resulting code we will always have 5 as the result,
but is it correct one?
--
With Best Regards,
Andy Shevchenko
On Tue, Oct 26, 2021 at 10:21:58PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 26, 2021 at 11:54:16AM -0700, Yury Norov wrote:
> > On Tue, Oct 26, 2021 at 10:41:08AM -0400, Qian Cai wrote:
> > > Since "size" is an "unsigned int", the rvalue "size - 1" will still be
> > > "unsigned int" according to the C standard (3.2.1.5 Usual arithmetic
> > > conversions). Therefore, GENMASK(size - 1, 0) will always return 0UL. Those
> > > are also caught by GCC (W=2):
> > >
> > > ./include/linux/find.h: In function 'find_first_bit':
> > > ./include/linux/bits.h:25:22: warning: comparison of unsigned expression in '< 0' is always false [-Wtype-limits]
> > > 25 | __is_constexpr((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:25:3: note: in expansion of macro '__is_constexpr'
> > > 25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
> > > | ^~~~~~~~~~~~~~
> > > ./include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > > 38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > > | ^~~~~~~~~~~~~~~~~~~
> > > ./include/linux/find.h:119:31: note: in expansion of macro 'GENMASK'
> > > 119 | unsigned long val = *addr & GENMASK(size - 1, 0);
> > > | ^~~~~~~
> > > ./include/linux/bits.h:25:34: warning: comparison of unsigned expression in '< 0' is always false [-Wtype-limits]
> > > 25 | __is_constexpr((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:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > > 38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > > | ^~~~~~~~~~~~~~~~~~~
> > > ./include/linux/find.h:119:31: note: in expansion of macro 'GENMASK'
> > > 119 | unsigned long val = *addr & GENMASK(size - 1, 0);
> > > | ^~~~~~~
> > >
> > > Signed-off-by: Qian Cai <[email protected]>
> > > ---
> > > include/linux/find.h | 28 ++++++++--------------------
> > > 1 file changed, 8 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/include/linux/find.h b/include/linux/find.h
> > > index 5bb6db213bcb..5ce2b17aea42 100644
> > > --- a/include/linux/find.h
> > > +++ b/include/linux/find.h
> > > @@ -115,11 +115,8 @@ unsigned long find_next_zero_bit(const unsigned long *addr, unsigned long size,
> > > static inline
> > > unsigned long find_first_bit(const unsigned long *addr, unsigned long size)
> > > {
> > > - if (small_const_nbits(size)) {
> > > - unsigned long val = *addr & GENMASK(size - 1, 0);
> > > -
> > > - return val ? __ffs(val) : size;
> > > - }
> > > + if (small_const_nbits(size))
> > > + return size;
> > >
> > > return _find_first_bit(addr, size);
> > > }
> >
> > [...]
> >
> > Nice catch! I'm a bit concerned that small_const_nbits() will never
> > allow GENMASK() to be passed with size == 0, but the patch looks
> > good to me overall.
>
> Can you explain to me how it is supposed to work?
>
> For example,
>
> x = 0xaa55;
> size = 5;
>
> printf("%lu\n", find_first_bit(&x, size));
>
> In the resulting code we will always have 5 as the result,
> but is it correct one?
I think it would work really bad and fail to load the kernel
for many systems, especially those with NR_CPUS == 64 or less.
That's why I think Apr 1 branch is a good place for it.
On 10/26/21 3:21 PM, Andy Shevchenko wrote:
> Can you explain to me how it is supposed to work?
>
> For example,
>
> x = 0xaa55;
> size = 5;
>
> printf("%lu\n", find_first_bit(&x, size));
>
> In the resulting code we will always have 5 as the result,
> but is it correct one?
Sorry, my bad. GENMASK(size - 1, 0) would just become __GENMASK(size -
1, 0) instead of 0. Let me revisit it and run some tests first.
+Cc: Greg (see below)
On Tue, Oct 26, 2021 at 12:28:48PM -0700, Yury Norov wrote:
> On Tue, Oct 26, 2021 at 10:21:58PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 26, 2021 at 11:54:16AM -0700, Yury Norov wrote:
> > > On Tue, Oct 26, 2021 at 10:41:08AM -0400, Qian Cai wrote:
> > > > Since "size" is an "unsigned int", the rvalue "size - 1" will still be
> > > > "unsigned int" according to the C standard (3.2.1.5 Usual arithmetic
> > > > conversions). Therefore, GENMASK(size - 1, 0) will always return 0UL. Those
> > > > are also caught by GCC (W=2):
> > > >
> > > > ./include/linux/find.h: In function 'find_first_bit':
> > > > ./include/linux/bits.h:25:22: warning: comparison of unsigned expression in '< 0' is always false [-Wtype-limits]
> > > > 25 | __is_constexpr((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:25:3: note: in expansion of macro '__is_constexpr'
> > > > 25 | __is_constexpr((l) > (h)), (l) > (h), 0)))
> > > > | ^~~~~~~~~~~~~~
> > > > ./include/linux/bits.h:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > > > 38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > > > | ^~~~~~~~~~~~~~~~~~~
> > > > ./include/linux/find.h:119:31: note: in expansion of macro 'GENMASK'
> > > > 119 | unsigned long val = *addr & GENMASK(size - 1, 0);
> > > > | ^~~~~~~
> > > > ./include/linux/bits.h:25:34: warning: comparison of unsigned expression in '< 0' is always false [-Wtype-limits]
> > > > 25 | __is_constexpr((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:38:3: note: in expansion of macro 'GENMASK_INPUT_CHECK'
> > > > 38 | (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > > > | ^~~~~~~~~~~~~~~~~~~
> > > > ./include/linux/find.h:119:31: note: in expansion of macro 'GENMASK'
> > > > 119 | unsigned long val = *addr & GENMASK(size - 1, 0);
> > > > | ^~~~~~~
> > > >
> > > > Signed-off-by: Qian Cai <[email protected]>
> > > > ---
> > > > include/linux/find.h | 28 ++++++++--------------------
> > > > 1 file changed, 8 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/include/linux/find.h b/include/linux/find.h
> > > > index 5bb6db213bcb..5ce2b17aea42 100644
> > > > --- a/include/linux/find.h
> > > > +++ b/include/linux/find.h
> > > > @@ -115,11 +115,8 @@ unsigned long find_next_zero_bit(const unsigned long *addr, unsigned long size,
> > > > static inline
> > > > unsigned long find_first_bit(const unsigned long *addr, unsigned long size)
> > > > {
> > > > - if (small_const_nbits(size)) {
> > > > - unsigned long val = *addr & GENMASK(size - 1, 0);
> > > > -
> > > > - return val ? __ffs(val) : size;
> > > > - }
> > > > + if (small_const_nbits(size))
> > > > + return size;
> > > >
> > > > return _find_first_bit(addr, size);
> > > > }
> > >
> > > [...]
> > >
> > > Nice catch! I'm a bit concerned that small_const_nbits() will never
> > > allow GENMASK() to be passed with size == 0, but the patch looks
> > > good to me overall.
> >
> > Can you explain to me how it is supposed to work?
> >
> > For example,
> >
> > x = 0xaa55;
> > size = 5;
> >
> > printf("%lu\n", find_first_bit(&x, size));
> >
> > In the resulting code we will always have 5 as the result,
> > but is it correct one?
>
> I think it would work really bad and fail to load the kernel
> for many systems, especially those with NR_CPUS == 64 or less.
>
> That's why I think Apr 1 branch is a good place for it.
Ah, I have got you. We are on the same page then.
Now, I have checked that email appearance in the upstream:
$ git log --oneline --author="[email protected]"
95cadae320be fortify: strlen: Avoid shadowing previous locals
94560f6156fe Revert "arm pl011 serial: support multi-irq request"
While first one perhaps okay, although it also refers to W=2,
I have now doubts if the "Revert" was really thought through
and not just yet another UMN-like experiment.
Greg, what do you think is the best course of actions here?
--
With Best Regards,
Andy Shevchenko
On 10/26/21 3:42 PM, Andy Shevchenko wrote:
> Now, I have checked that email appearance in the upstream:
>
> $ git log --oneline --author="[email protected]"
> 95cadae320be fortify: strlen: Avoid shadowing previous locals
> 94560f6156fe Revert "arm pl011 serial: support multi-irq request"
>
> While first one perhaps okay, although it also refers to W=2,
> I have now doubts if the "Revert" was really thought through
> and not just yet another UMN-like experiment.
>
> Greg, what do you think is the best course of actions here?
Perhaps, a little sympathy towards a stranger might get us a better
community. Feel free to audit my previous works.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=author&q=Qian+Cai
On Tue, Oct 26, 2021 at 03:33:48PM -0400, Qian Cai wrote:
> On 10/26/21 3:21 PM, Andy Shevchenko wrote:
> > Can you explain to me how it is supposed to work?
> >
> > For example,
> >
> > x = 0xaa55;
> > size = 5;
> >
> > printf("%lu\n", find_first_bit(&x, size));
> >
> > In the resulting code we will always have 5 as the result,
> > but is it correct one?
>
> Sorry, my bad. GENMASK(size - 1, 0) would just become __GENMASK(size -
> 1, 0) instead of 0. Let me revisit it and run some tests first.
And we do not want to have __GENMASK() in the code.
Btw, I found one lurking around. Will fix it.
--
With Best Regards,
Andy Shevchenko
On Tue, Oct 26, 2021 at 03:54:26PM -0400, Qian Cai wrote:
> On 10/26/21 3:42 PM, Andy Shevchenko wrote:
> > Now, I have checked that email appearance in the upstream:
> >
> > $ git log --oneline --author="[email protected]"
> > 95cadae320be fortify: strlen: Avoid shadowing previous locals
> > 94560f6156fe Revert "arm pl011 serial: support multi-irq request"
> >
> > While first one perhaps okay, although it also refers to W=2,
> > I have now doubts if the "Revert" was really thought through
> > and not just yet another UMN-like experiment.
> >
> > Greg, what do you think is the best course of actions here?
>
> Perhaps, a little sympathy towards a stranger might get us a better
> community.
Agree, but you see a problem here, W=2, for example, is high due to
a lot of non-sense (or very little sense) noise. The warning you got
is hidden on purpose. On top of that, the code has not been thought
through at all, despite missed test run. This is easy to catch.
What I expect from "a stranger" who is in doubt (obviously) how this
code works to ask and then decide how to act.
And on top of all these, we used to have UMN case which makes me
first think of bad experiments on the community (I really haven't
believed that this patch was sent consciously).
So, please be careful next time and better ask first before acting,
if in doubt.
> Feel free to audit my previous works.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=author&q=Qian+Cai
Yes, after above patch these all in doubt (to me personally), but
I have no time to revisit all of them, esp. they do not touched my
area of interests in the Linux kernel.
--
With Best Regards,
Andy Shevchenko
On 10/27/21 5:44 AM, Andy Shevchenko wrote:
> Agree, but you see a problem here, W=2, for example, is high due to
> a lot of non-sense (or very little sense) noise. The warning you got
> is hidden on purpose. On top of that, the code has not been thought
> through at all, despite missed test run. This is easy to catch.
>
> What I expect from "a stranger" who is in doubt (obviously) how this
> code works to ask and then decide how to act.
>
> And on top of all these, we used to have UMN case which makes me
> first think of bad experiments on the community (I really haven't
> believed that this patch was sent consciously).
>
> So, please be careful next time and better ask first before acting,
> if in doubt.
Yes, I am an idiot. My brain was short-circled for a moment to be
totally mistaken with "GENMASK() == 0" instead GENMASK_INPUT_CHECK(). I
am not sure how to avoid those short-circles again in the future though
apart from thinking about going to an office in the near future, so kids
at home would drive my less crazy. Of course, I knew the trick for
lib_test.c now which should help me in this subsystem in the future.
Thanks for your time, Andy.
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: ca95e676a1f3ea18bca11df611514ab582fec8f2 ("[PATCH] bitmap: simplify GENMASK(size - 1, 0) lines")
url: https://github.com/0day-ci/linux/commits/Qian-Cai/bitmap-simplify-GENMASK-size-1-0-lines/20211026-224815
in testcase: boot
on test machine: qemu-system-i386 -enable-kvm -cpu SandyBridge -smp 2 -m 4G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+---------------------------------------------+---------------+------------+
| | next-20211025 | ca95e676a1 |
+---------------------------------------------+---------------+------------+
| boot_successes | 11 | 0 |
| boot_failures | 0 | 12 |
| BUG:unable_to_handle_page_fault_for_address | 0 | 12 |
| Oops:#[##] | 0 | 12 |
| EIP:sparse_init_nid | 0 | 12 |
| Kernel_panic-not_syncing:Fatal_exception | 0 | 12 |
| Kernel_panic-not_syncing:Fatal_exception] | 0 | 12 |
+---------------------------------------------+---------------+------------+
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 0.177015][ T0] BUG: unable to handle page fault for address: 00001120
[ 0.177738][ T0] #PF: supervisor read access in kernel mode
[ 0.178348][ T0] #PF: error_code(0x0000) - not-present page
[ 0.178970][ T0] *pdpt = 0000000000000000 *pde = f000ff53f000ff53
[ 0.179768][ T0] Oops: 0000 [#1] PREEMPT SMP PTI
[ 0.180290][ T0] CPU: 0 PID: 0 Comm: swapper Not tainted 5.15.0-rc6-next-20211025-00001-gca95e676a1f3 #1
[ 0.181305][ T0] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
[ 0.182226][ T0] EIP: sparse_init_nid+0x21/0x1c2
[ 0.182721][ T0] Code: 93 77 81 ff 8b 5d fc c9 c3 e8 1d 53 eb fe 55 89 e5 57 89 c7 56 53 89 d3 31 d2 83 ec 10 89 4d ec 8b 0c bd 20 f0 15 d9 8b 75 08
<ff> b1 20 11 00 00 6a 00 89 f0 6a 00 c1 e0 07 6a 00 6a 00 6a 00 6a
[ 0.184741][ T0] EAX: 00000000 EBX: 00000000 ECX: 00000000 EDX: 00000000
[ 0.185446][ T0] ESI: 00000008 EDI: 00000000 EBP: d8d59ec8 ESP: d8d59eac
[ 0.186167][ T0] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00210086
[ 0.186949][ T0] CR0: 80050033 CR2: 00001120 CR3: 19262000 CR4: 000406b0
[ 0.187701][ T0] Call Trace:
[ 0.188037][ T0] sparse_init+0x1ad/0x1cc
[ 0.188494][ T0] paging_init+0x87/0x8f
[ 0.188923][ T0] native_pagetable_init+0xe0/0xe8
[ 0.189437][ T0] setup_arch+0xb2f/0xc39
[ 0.189900][ T0] start_kernel+0x60/0x576
[ 0.190365][ T0] ? set_intr_gate+0x47/0x5a
[ 0.190836][ T0] ? early_idt_handler_common+0x44/0x44
[ 0.191403][ T0] i386_start_kernel+0x48/0x4a
[ 0.191891][ T0] startup_32_smp+0x161/0x164
[ 0.192347][ T0] Modules linked in:
[ 0.192736][ T0] CR2: 0000000000001120
[ 0.193160][ T0] random: get_random_bytes called from print_oops_end_marker+0x2c/0x80 with crng_init=0
[ 0.193170][ T0] ---[ end trace 6f3fca0785832265 ]---
[ 0.194700][ T0] EIP: sparse_init_nid+0x21/0x1c2
[ 0.195205][ T0] Code: 93 77 81 ff 8b 5d fc c9 c3 e8 1d 53 eb fe 55 89 e5 57 89 c7 56 53 89 d3 31 d2 83 ec 10 89 4d ec 8b 0c bd 20 f0 15 d9 8b 75 08 <ff> b1 20 11 00 00 6a 00 89 f0 6a 00 c1 e0 07 6a 00 6a 00 6a 00 6a
[ 0.197244][ T0] EAX: 00000000 EBX: 00000000 ECX: 00000000 EDX: 00000000
[ 0.197955][ T0] ESI: 00000008 EDI: 00000000 EBP: d8d59ec8 ESP: d8d59eac
[ 0.198678][ T0] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00210086
[ 0.199470][ T0] CR0: 80050033 CR2: 00001120 CR3: 19262000 CR4: 000406b0
[ 0.200229][ T0] Kernel panic - not syncing: Fatal exception
[ 0.200865][ T0] ---[ end Kernel panic - not syncing: Fatal exception ]---
To reproduce:
# build kernel
cd linux
cp config-5.15.0-rc6-next-20211025-00001-gca95e676a1f3 .config
make HOSTCC=gcc-9 CC=gcc-9 ARCH=i386 olddefconfig prepare modules_prepare bzImage
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> job-script # job-script is attached in this email
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
---
0DAY/LKP+ Test Infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation
Thanks,
Oliver Sang