This seems to be causing lots of ARM breakage:
lib/find_next_bit.c:183: error: implicit declaration of function '__fls'
Whoever's responsible, please fix this ASAP so I can see whether my fixes
I merged on Monday fix the previous set of ARM build errors in linux-next.
Thanks.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Wed, 3 Dec 2008 19:29:05 +0000
Russell King <[email protected]> wrote:
> This seems to be causing lots of ARM breakage:
>
> lib/find_next_bit.c:183: error: implicit declaration of function '__fls'
>
> Whoever's responsible,
git-blame?
> please fix this ASAP so I can see whether my fixes
> I merged on Monday fix the previous set of ARM build errors in linux-next.
commit b032dfc80921daa9c957810fb2e2ff253aaf2ac4
Author: Rusty Russell <[email protected]>
Date: Wed Dec 3 11:16:59 2008 +1100
bitmap:find_last_bit
On Thursday 04 December 2008 07:11:09 Andrew Morton wrote:
> On Wed, 3 Dec 2008 19:29:05 +0000
>
> Russell King <[email protected]> wrote:
> > This seems to be causing lots of ARM breakage:
> >
> > lib/find_next_bit.c:183: error: implicit declaration of function '__fls'
> >
> > Whoever's responsible,
>
> git-blame?
It's me. Turns out sparc, avr32 and arm all don't define __fls in their
asm/bitops.h, and I'm the first one to use it in generic code.
But as I prepared this patch, I note that the armv5 __fls/fls is wrong:
/* Implement fls() in C so that 64-bit args are suitably truncated */
static inline int fls(int x)
{
return __fls(x);
}
__fls(x) returns a bit number (0-31). fls() returns 0 or bitnumber+1.
(Yes, classic useless kerneldoc documentation doesn't actually *say*
this clearly).
But here's the linux-next fix:
arm: define __fls for pre v5 ARM
Signed-off-by: Rusty Russell <[email protected]>
diff --git a/arch/arm/include/asm/bitops.h b/arch/arm/include/asm/bitops.h
--- a/arch/arm/include/asm/bitops.h
+++ b/arch/arm/include/asm/bitops.h
@@ -239,6 +239,7 @@ extern int _find_next_bit_be(const unsig
#include <asm-generic/bitops/ffz.h>
#include <asm-generic/bitops/__ffs.h>
#include <asm-generic/bitops/fls.h>
+#include <asm-generic/bitops/__fls.h>
#include <asm-generic/bitops/ffs.h>
#else
Rusty Russell wrote:
> On Thursday 04 December 2008 07:11:09 Andrew Morton wrote:
>> On Wed, 3 Dec 2008 19:29:05 +0000
>>
>> Russell King <[email protected]> wrote:
>>> This seems to be causing lots of ARM breakage:
>>>
>>> lib/find_next_bit.c:183: error: implicit declaration of function '__fls'
>>>
>>> Whoever's responsible,
>> git-blame?
>
> It's me. Turns out sparc, avr32 and arm all don't define __fls in their
> asm/bitops.h, and I'm the first one to use it in generic code.
>
> But as I prepared this patch, I note that the armv5 __fls/fls is wrong:
>
> /* Implement fls() in C so that 64-bit args are suitably truncated */
> static inline int fls(int x)
> {
> return __fls(x);
> }
>
> __fls(x) returns a bit number (0-31). fls() returns 0 or bitnumber+1.
>
> (Yes, classic useless kerneldoc documentation doesn't actually *say*
> this clearly).
oh fud. That's not a fault of kernel-doc, just of whoever wrote it.
It's only as good as someone makes it.
> But here's the linux-next fix:
>
> arm: define __fls for pre v5 ARM
>
> Signed-off-by: Rusty Russell <[email protected]>
>
> diff --git a/arch/arm/include/asm/bitops.h b/arch/arm/include/asm/bitops.h
> --- a/arch/arm/include/asm/bitops.h
> +++ b/arch/arm/include/asm/bitops.h
> @@ -239,6 +239,7 @@ extern int _find_next_bit_be(const unsig
> #include <asm-generic/bitops/ffz.h>
> #include <asm-generic/bitops/__ffs.h>
> #include <asm-generic/bitops/fls.h>
> +#include <asm-generic/bitops/__fls.h>
> #include <asm-generic/bitops/ffs.h>
>
> #else
On Wed, 03 Dec 2008 15:37:44 -0800
Randy Dunlap <[email protected]> wrote:
> > (Yes, classic useless kerneldoc documentation doesn't actually *say*
> > this clearly).
>
> oh fud. That's not a fault of kernel-doc, just of whoever wrote it.
> It's only as good as someone makes it.
For some reason, the act of typing in some kerneldoc makes people's
brains turn off. Perhaps it's because "oh, I am supposed to type some
documentation here" instead of "gee, I think this code is unclear,
let's clarify that".
From: Randy Dunlap <[email protected]>
Date: Wed, 03 Dec 2008 15:37:44 -0800
> Rusty Russell wrote:
> > On Thursday 04 December 2008 07:11:09 Andrew Morton wrote:
> >> On Wed, 3 Dec 2008 19:29:05 +0000
> >>
> >> Russell King <[email protected]> wrote:
> >>> This seems to be causing lots of ARM breakage:
> >>>
> >>> lib/find_next_bit.c:183: error: implicit declaration of function '__fls'
> >>>
> >>> Whoever's responsible,
> >> git-blame?
> >
> > It's me. Turns out sparc, avr32 and arm all don't define __fls in their
> > asm/bitops.h, and I'm the first one to use it in generic code.
> >
> > But as I prepared this patch, I note that the armv5 __fls/fls is wrong:
> >
> > /* Implement fls() in C so that 64-bit args are suitably truncated */
> > static inline int fls(int x)
> > {
> > return __fls(x);
> > }
> >
> > __fls(x) returns a bit number (0-31). fls() returns 0 or bitnumber+1.
> >
> > (Yes, classic useless kerneldoc documentation doesn't actually *say*
> > this clearly).
>
> oh fud. That's not a fault of kernel-doc, just of whoever wrote it.
> It's only as good as someone makes it.
That's true, but it is not fud to say that kerneldoc is only any good
if people keep it accurate and up to date, and this is what I think
Rusty is upset about.
I personally don't like kerneldoc at all, because the truth is that
people will work on fixing bugs and other userful things before
keeping kerneldoc up to date.
And that's the basic fact which cannot be denied.
I wish it could work, but it doesn't across the board. So unless
we have dedicated monkeys scouring over every single patch that
goes into the tree and doing the necessary kerneldoc updates,
kerneldoc will be chronically wrong somewhere.
That leads to confusion and lost developer time. Because if the
kerneldoc bits are wrong, it's worthless.
> For some reason, the act of typing in some kerneldoc makes people's
> brains turn off. Perhaps it's because "oh, I am supposed to type some
> documentation here" instead of "gee, I think this code is unclear,
> let's clarify that".
Well prior to the kerneldoc stuff they didn't type anything at all to
clarify or document. Also kerneldoc is meant for documenting the APIs not
so much the internals of functions. When used sensibly it can be very
useful, especially if it documents API locking rules.
Alan
David Miller wrote:
> From: Randy Dunlap <[email protected]>
> Date: Wed, 03 Dec 2008 15:37:44 -0800
>
>> Rusty Russell wrote:
>>> On Thursday 04 December 2008 07:11:09 Andrew Morton wrote:
>>>> On Wed, 3 Dec 2008 19:29:05 +0000
>>>>
>>>> Russell King <[email protected]> wrote:
>>>>> This seems to be causing lots of ARM breakage:
>>>>>
>>>>> lib/find_next_bit.c:183: error: implicit declaration of function '__fls'
>>>>>
>>>>> Whoever's responsible,
>>>> git-blame?
>>> It's me. Turns out sparc, avr32 and arm all don't define __fls in their
>>> asm/bitops.h, and I'm the first one to use it in generic code.
>>>
>>> But as I prepared this patch, I note that the armv5 __fls/fls is wrong:
>>>
>>> /* Implement fls() in C so that 64-bit args are suitably truncated */
>>> static inline int fls(int x)
>>> {
>>> return __fls(x);
>>> }
>>>
>>> __fls(x) returns a bit number (0-31). fls() returns 0 or bitnumber+1.
>>>
>>> (Yes, classic useless kerneldoc documentation doesn't actually *say*
>>> this clearly).
>>
>> oh fud. That's not a fault of kernel-doc, just of whoever wrote it.
>> It's only as good as someone makes it.
>
> That's true, but it is not fud to say that kerneldoc is only any good
> if people keep it accurate and up to date, and this is what I think
> Rusty is upset about.
>
> I personally don't like kerneldoc at all, because the truth is that
> people will work on fixing bugs and other userful things before
> keeping kerneldoc up to date.
>
> And that's the basic fact which cannot be denied.
>
> I wish it could work, but it doesn't across the board. So unless
> we have dedicated monkeys scouring over every single patch that
> goes into the tree and doing the necessary kerneldoc updates,
> kerneldoc will be chronically wrong somewhere.
>
> That leads to confusion and lost developer time. Because if the
> kerneldoc bits are wrong, it's worthless.
That's all independent of kernel-doc. I.e., if someone just used
plain comments and they still were not being updated (typical),
then it's the same problem. And yes, I agree, the wrong documentation
is bad and a time sink.
~Randy
On Thu, Dec 04, 2008 at 09:52:44AM +1030, Rusty Russell wrote:
> On Thursday 04 December 2008 07:11:09 Andrew Morton wrote:
> > On Wed, 3 Dec 2008 19:29:05 +0000
> >
> > Russell King <[email protected]> wrote:
> > > This seems to be causing lots of ARM breakage:
> > >
> > > lib/find_next_bit.c:183: error: implicit declaration of function '__fls'
> > >
> > > Whoever's responsible,
> >
> > git-blame?
>
> It's me. Turns out sparc, avr32 and arm all don't define __fls in their
> asm/bitops.h, and I'm the first one to use it in generic code.
>
> But as I prepared this patch, I note that the armv5 __fls/fls is wrong:
__fls is wrong.
>
> /* Implement fls() in C so that 64-bit args are suitably truncated */
> static inline int fls(int x)
> {
> return __fls(x);
> }
>
> __fls(x) returns a bit number (0-31). fls() returns 0 or bitnumber+1.
The 'clz' instruction returns 32 for a zero input, or (31 - most significant
set bit) - which seems to work for fls() but not __fls().
Sending to Nicolas.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Thursday 04 December 2008 10:07:44 Randy Dunlap wrote:
> Rusty Russell wrote:
> > (Yes, classic useless kerneldoc documentation doesn't actually *say*
> > this clearly).
>
> oh fud. That's not a fault of kernel-doc, just of whoever wrote it.
> It's only as good as someone makes it.
Sorry that this came out wrong. kernel-doc provides structure, but it can't
provide content. And authors seem unable to think from the POV of someone
*using* the API.
With some work, I tracked it back to Stephen Hemminger for this comment in
12d9c8420b9daa1da3d9e090640fb24bcd0deba2. It's since been fixed and moved,
but it's still:
* __fls: find last set bit in word
* @word: The word to search
*
* Undefined if no set bit exists, so code should check against 0 first.
Which would be *fine* if fls() didn't have such confusing bit numbering and
the exact same one-line description.
Thanks,
Rusty.
On Wed, Dec 3, 2008 at 18:22, Rusty Russell wrote:
> On Thursday 04 December 2008 07:11:09 Andrew Morton wrote:
>> On Wed, 3 Dec 2008 19:29:05 +0000
>>
>> Russell King <[email protected]> wrote:
>> > This seems to be causing lots of ARM breakage:
>> >
>> > lib/find_next_bit.c:183: error: implicit declaration of function '__fls'
>> >
>> > Whoever's responsible,
>>
>> git-blame?
>
> It's me. Turns out sparc, avr32 and arm all don't define __fls in their
> asm/bitops.h, and I'm the first one to use it in generic code.
the Blackfin port also does not ... you going to post a change for
that since the build breaks for Blackfin atm too ?
-mike
On Thursday 04 December 2008 12:03:57 Mike Frysinger wrote:
> On Wed, Dec 3, 2008 at 18:22, Rusty Russell wrote:
> > On Thursday 04 December 2008 07:11:09 Andrew Morton wrote:
> >> On Wed, 3 Dec 2008 19:29:05 +0000
> >>
> >> Russell King <[email protected]> wrote:
> >> > This seems to be causing lots of ARM breakage:
> >> >
> >> > lib/find_next_bit.c:183: error: implicit declaration of function
> >> > '__fls'
> >> >
> >> > Whoever's responsible,
> >>
> >> git-blame?
> >
> > It's me. Turns out sparc, avr32 and arm all don't define __fls in their
> > asm/bitops.h, and I'm the first one to use it in generic code.
>
> the Blackfin port also does not ... you going to post a change for
> that since the build breaks for Blackfin atm too ?
> -mike
Sure, why not join the party!
(Hmm, maybe I should change that list to a shorter list of archs which
*do* define __fls?)
blackfin: define __fls
Like fls, but can't be handed 0 and returns the bit number.
(I broke this arch in linux-next by using __fls in generic code).
Signed-off-by: Rusty Russell <[email protected]>
diff --git a/arch/blackfin/include/asm/bitops.h b/arch/blackfin/include/asm/bitops.h
--- a/arch/blackfin/include/asm/bitops.h
+++ b/arch/blackfin/include/asm/bitops.h
@@ -213,6 +213,7 @@ static __inline__ int __test_bit(int nr,
#endif /* __KERNEL__ */
#include <asm-generic/bitops/fls.h>
+#include <asm-generic/bitops/__fls.h>
#include <asm-generic/bitops/fls64.h>
#endif /* _BLACKFIN_BITOPS_H */
On Thu, 4 Dec 2008 11:57:14 +1030
Rusty Russell <[email protected]> wrote:
> On Thursday 04 December 2008 10:07:44 Randy Dunlap wrote:
> > Rusty Russell wrote:
> > > (Yes, classic useless kerneldoc documentation doesn't actually *say*
> > > this clearly).
> >
> > oh fud. That's not a fault of kernel-doc, just of whoever wrote it.
> > It's only as good as someone makes it.
>
> Sorry that this came out wrong. kernel-doc provides structure, but it can't
> provide content. And authors seem unable to think from the POV of someone
> *using* the API.
>
> With some work, I tracked it back to Stephen Hemminger for this comment in
> 12d9c8420b9daa1da3d9e090640fb24bcd0deba2. It's since been fixed and moved,
> but it's still:
>
> * __fls: find last set bit in word
> * @word: The word to search
> *
> * Undefined if no set bit exists, so code should check against 0 first.
>
> Which would be *fine* if fls() didn't have such confusing bit numbering and
> the exact same one-line description.
>
> Thanks,
> Rusty.
I think the idea was that fls was supposed to match ffs which had stupid
bit numbering inherited from BSD. and __ffs and __fls were same
but undefined if word is 0 so that they could just be one line asm.
On Thu, 4 Dec 2008, Russell King wrote:
> On Thu, Dec 04, 2008 at 09:52:44AM +1030, Rusty Russell wrote:
> > On Thursday 04 December 2008 07:11:09 Andrew Morton wrote:
> > > On Wed, 3 Dec 2008 19:29:05 +0000
> > >
> > > Russell King <[email protected]> wrote:
> > > > This seems to be causing lots of ARM breakage:
> > > >
> > > > lib/find_next_bit.c:183: error: implicit declaration of function '__fls'
> > > >
> > > > Whoever's responsible,
> > >
> > > git-blame?
> >
> > It's me. Turns out sparc, avr32 and arm all don't define __fls in their
> > asm/bitops.h, and I'm the first one to use it in generic code.
> >
> > But as I prepared this patch, I note that the armv5 __fls/fls is wrong:
>
> __fls is wrong.
__fls used to _not_ exist at all on ARM until commit 0c65f459ce.
> > /* Implement fls() in C so that 64-bit args are suitably truncated */
> > static inline int fls(int x)
> > {
> > return __fls(x);
> > }
> >
> > __fls(x) returns a bit number (0-31). fls() returns 0 or bitnumber+1.
>
> The 'clz' instruction returns 32 for a zero input, or (31 - most significant
> set bit) - which seems to work for fls() but not __fls().
... and it looks like the person who introduced the commit above didn't
take into account the fact that __fls() already had another semantic in
the kernel.
> Sending to Nicolas.
I queued a fix addressing both issues for RMK to merge:
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=5339/1
Nicolas
On Wed, Dec 3, 2008 at 21:15, Rusty Russell wrote:
> On Thursday 04 December 2008 12:03:57 Mike Frysinger wrote:
>> On Wed, Dec 3, 2008 at 18:22, Rusty Russell wrote:
>> > On Thursday 04 December 2008 07:11:09 Andrew Morton wrote:
>> >> On Wed, 3 Dec 2008 19:29:05 +0000
>> >>
>> >> Russell King <[email protected]> wrote:
>> >> > This seems to be causing lots of ARM breakage:
>> >> >
>> >> > lib/find_next_bit.c:183: error: implicit declaration of function
>> >> > '__fls'
>> >> >
>> >> > Whoever's responsible,
>> >>
>> >> git-blame?
>> >
>> > It's me. Turns out sparc, avr32 and arm all don't define __fls in their
>> > asm/bitops.h, and I'm the first one to use it in generic code.
>>
>> the Blackfin port also does not ... you going to post a change for
>> that since the build breaks for Blackfin atm too ?
>
> Sure, why not join the party!
>
> (Hmm, maybe I should change that list to a shorter list of archs which
> *do* define __fls?)
>
> blackfin: define __fls
>
> Like fls, but can't be handed 0 and returns the bit number.
>
> (I broke this arch in linux-next by using __fls in generic code).
>
> Signed-off-by: Rusty Russell <[email protected]>
cheers
Acked-by: Mike Frysinger <[email protected]>
-mike
On Wed, 3 Dec 2008, Andrew Morton wrote:
> On Wed, 3 Dec 2008 19:29:05 +0000
> Russell King <[email protected]> wrote:
>
> > This seems to be causing lots of ARM breakage:
> >
> > lib/find_next_bit.c:183: error: implicit declaration of function '__fls'
> >
> > Whoever's responsible,
>
> git-blame?
>
> > please fix this ASAP so I can see whether my fixes
> > I merged on Monday fix the previous set of ARM build errors in linux-next.
>
> commit b032dfc80921daa9c957810fb2e2ff253aaf2ac4
> Author: Rusty Russell <[email protected]>
> Date: Wed Dec 3 11:16:59 2008 +1100
>
> bitmap:find_last_bit
Also broke m68k (a bit more hidden due to other build failures).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thursday 04 December 2008 19:48:06 Geert Uytterhoeven wrote:
> Also broke m68k (a bit more hidden due to other build failures).
And m68knommu has similar issues.
Here's what I have for m68k for tomorrow's linux-next. Please steal.
m68k: define __fls
Like fls, but can't be handed 0 and returns the bit number.
(I broke this arch in linux-next by using __fls in generic code).
Signed-off-by: Rusty Russell <[email protected]>
diff --git a/include/asm-m68k/bitops.h b/include/asm-m68k/bitops.h
--- a/include/asm-m68k/bitops.h
+++ b/include/asm-m68k/bitops.h
@@ -315,6 +315,11 @@ static inline int fls(int x)
return 32 - cnt;
}
+static inline int __fls(int x)
+{
+ return fls(x) - 1;
+}
+
#include <asm-generic/bitops/fls64.h>
#include <asm-generic/bitops/sched.h>
#include <asm-generic/bitops/hweight.h>
On Fri, 5 Dec 2008, Rusty Russell wrote:
> On Thursday 04 December 2008 19:48:06 Geert Uytterhoeven wrote:
> > Also broke m68k (a bit more hidden due to other build failures).
>
> And m68knommu has similar issues.
>
> Here's what I have for m68k for tomorrow's linux-next. Please steal.
>
> m68k: define __fls
>
> Like fls, but can't be handed 0 and returns the bit number.
>
> (I broke this arch in linux-next by using __fls in generic code).
>
> Signed-off-by: Rusty Russell <[email protected]>
>
> diff --git a/include/asm-m68k/bitops.h b/include/asm-m68k/bitops.h
> --- a/include/asm-m68k/bitops.h
> +++ b/include/asm-m68k/bitops.h
> @@ -315,6 +315,11 @@ static inline int fls(int x)
> return 32 - cnt;
> }
>
> +static inline int __fls(int x)
^^^ ^^^
Other implementations take `unsigned long' and return `unsigned long'...
> +{
> + return fls(x) - 1;
> +}
> +
On Thu, 4 Dec 2008, Rusty Russell wrote:
> Like fls, but can't be handed 0 and returns the bit number.
>
> (I added find_last_bit() to bitmap.h which broke archs which didn't
> define this)
>
> Signed-off-by: Rusty Russell <[email protected]>
> ---
> arch/avr32/include/asm/bitops.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/avr32/include/asm/bitops.h b/arch/avr32/include/asm/bitops.h
> --- a/arch/avr32/include/asm/bitops.h
> +++ b/arch/avr32/include/asm/bitops.h
> @@ -263,6 +263,11 @@ static inline int fls(unsigned long word
> return 32 - result;
> }
>
> +static inline int __fls(unsigned long word)
^^^ ^^^^^^^^^^^^^
> +{
> + return fls(word) - 1;
> +}
> +
... but this one uses mixed types?
What are the official semantics of __fls()?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Monday 08 December 2008 08:20:59 Geert Uytterhoeven wrote:
> On Fri, 5 Dec 2008, Rusty Russell wrote:
> > +static inline int __fls(int x)
> ^^^ ^^^
> Other implementations take `unsigned long' and return `unsigned long'...
It's all over the place, actually. 32 bit archs are especially loose.
I've been toying with the idea of a boottime testsuite for all the
bitops to see who gets them wrong.
> > +static inline int __fls(unsigned long word)
> ^^^ ^^^^^^^^^^^^^
> > +{
> > + return fls(word) - 1;
> > +}
> > +
>
> ... but this one uses mixed types?
I cut and pasted. I thought you were 32 bit, so doesn't matter?
> What are the official semantics of __fls()?
Find last bit set in the word, undefined if word is 0. Returns 0
to BITS_PER_LONG-1.
Cheers,
Rusty.