Re: [RFC PATCH 1/2] bits: introduce ffs_val()

[David Laight <david.laight.linux@gmail.com>]

On Mon, 12 Jan 2026 09:15:41 +0100
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Hi Petr
> 
> Am 09.01.26 um 17:37 schrieb Petr Tesarik:
> > Introduce a macro that can efficiently extract the least significant
> > non-zero bit from a value.
> >
> > Interestingly, this bit-twiddling trick is open-coded in some places, but
> > it also appears to be little known, leading to various inefficient
> > implementations in other places. Let's make it part of the standard bitops
> > arsenal.
> >
> > Define the macro in a separate header file included from <linux/bitops.h>,
> > to allow using it in very low-level header files that may not want to
> > include all of <linux/bitops.h>.
> >
> > Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> > ---
> >   MAINTAINERS             |  1 +
> >   include/linux/bitops.h  |  1 +
> >   include/linux/ffs_val.h | 21 +++++++++++++++++++++
> >   3 files changed, 23 insertions(+)
> >   create mode 100644 include/linux/ffs_val.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a0dd762f5648b..8f15c76a67ea2 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -4466,6 +4466,7 @@ F:	arch/*/lib/bitops.c
> >   F:	include/asm-generic/bitops
> >   F:	include/asm-generic/bitops.h
> >   F:	include/linux/bitops.h
> > +F:	include/linux/ffs_val.h
> >   F:	lib/hweight.c
> >   F:	lib/test_bitops.c
> >   F:	tools/*/bitops*
> > diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> > index ea7898cc59039..209f0c3e07b9e 100644
> > --- a/include/linux/bitops.h
> > +++ b/include/linux/bitops.h
> > @@ -4,6 +4,7 @@
> >   
> >   #include <asm/types.h>
> >   #include <linux/bits.h>
> > +#include <linux/ffs_val.h>
> >   #include <linux/typecheck.h>
> >   
> >   #include <uapi/linux/kernel.h>
> > diff --git a/include/linux/ffs_val.h b/include/linux/ffs_val.h
> > new file mode 100644
> > index 0000000000000..193ec86d2b53b
> > --- /dev/null
> > +++ b/include/linux/ffs_val.h
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _ASM_LINUX_FFS_VAL_H_
> > +#define _ASM_LINUX_FFS_VAL_H_
> > +
> > +/**
> > + * ffs_val - find the value of the first set bit
> > + * @x: the value to search
> > + *
> > + * Unlike ffs(), which returns a bit position, ffs_val() returns the bit
> > + * value itself.  
> 
> This sentence was confusing me at first, because the individual bit's 
> value is always '1'. Maybe say something more descriptive, such as 
> 'ffs_val returns the value resulting from that bit's position.'

An example would clarify things massively.
Perhaps ffs_val(0x40100) is 0x100.

> > + *
> > + * Returns:
> > + * least significant non-zero bit, 0 if all bits are zero  
> 
> Same here.
> 
> > + */
> > +#define ffs_val(x)			\
> > +({					\
> > +	const typeof(x) val__ = (x);	\
> > +	val__ & -val__;			\  
> 
> Is this construct supposed to work with signed integers and/or negative 
> numbers? I assume that two's complement can be expected nowadays, but 
> for LONG_MIN it returns zero AFAICT. The documentation should mention 
> these cases.

Writing as 'x & (~x + 1)' makes it pretty obvious that it is always valid.
For two's compliment '-x == ~x + 1' and the compiler will do the transformation.
That expression would also be valid for one's compliment and sign overpunch
systems - but the result for negative values might be unexpected!

OTOH I'm pretty sure most modern C code assumes two's compliment binary
numbers (and NULL being the all-zero bit pattern).
You'd have grief trying to compile C for an ICL System-25 - decimal/BCD
numbers with sign overpunch and addresses binary_page * 10000 + BCD_offset.
(Still in use in the late 1980s.)

	David


> 
> Best regards
> Thomas
> 
> > +})
> > +
> > +#endif /* _ASM_LINUX_FFS_VAL_H_ */  
>