[RFC PATCH 0/2] Helper to isolate least-significant bit

[RFC PATCH 0/2] Helper to isolate least-significant bit

[Petr Tesarik]

Isolation of the least significant bit can be achieved with 3 basic
ALU operations which are already open-coded in various places in the
kernel.

However, since other places less efficient constructs, for example
`1UL << ffs(x)`, I assume the trick is known only to some authors, and
it's worth adding a helper to promote its use.

Petr Tesarik (2):
  bits: introduce ffs_val()
  treewide, bits: use ffs_val() where it is open-coded

 arch/alpha/include/asm/bitops.h              |  2 +-
 arch/alpha/kernel/core_cia.c                 |  2 +-
 arch/alpha/kernel/smp.c                      |  2 +-
 arch/arc/include/asm/bitops.h                |  2 +-
 arch/m68k/include/asm/bitops.h               | 12 +++++------
 arch/mips/dec/ecc-berr.c                     |  2 +-
 arch/mips/include/asm/bitops.h               |  4 ++--
 arch/mips/pci/pci-malta.c                    |  4 ++--
 arch/powerpc/include/asm/bitops.h            |  4 ++--
 arch/powerpc/kvm/e500_mmu_host.c             |  2 +-
 arch/powerpc/lib/sstep.c                     |  2 +-
 arch/powerpc/xmon/ppc-dis.c                  |  3 ++-
 arch/powerpc/xmon/ppc-opc.c                  |  6 +++---
 arch/s390/include/asm/bitops.h               |  2 +-
 arch/xtensa/include/asm/bitops.h             |  6 +++---
 arch/xtensa/kernel/traps.c                   |  2 +-
 drivers/gpu/drm/gma500/psb_intel_sdvo.c      |  2 +-
 drivers/iommu/dma-iommu.c                    |  2 +-
 drivers/net/ethernet/netronome/nfp/bpf/jit.c |  2 +-
 drivers/net/usb/cdc_ncm.c                    |  4 ++--
 include/asm-generic/div64.h                  |  4 ++--
 include/linux/bitfield.h                     |  3 ++-
 include/linux/bitops.h                       |  1 +
 include/linux/ffs_val.h                      | 21 ++++++++++++++++++++
 include/linux/log2.h                         |  2 +-
 include/linux/min_heap.h                     |  5 +++--
 lib/math/gcd.c                               |  4 ++--
 lib/sort.c                                   |  3 ++-
 net/bluetooth/mgmt.c                         |  2 +-
 net/netfilter/nft_set_pipapo.c               |  2 +-
 30 files changed, 70 insertions(+), 44 deletions(-)
 create mode 100644 include/linux/ffs_val.h

-- 
2.52.0

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

[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.
+ *
+ * Returns:
+ * least significant non-zero bit, 0 if all bits are zero
+ */
+#define ffs_val(x)			\
+({					\
+	const typeof(x) val__ = (x);	\
+	val__ & -val__;			\
+})
+
+#endif /* _ASM_LINUX_FFS_VAL_H_ */
-- 
2.52.0

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

[Arnd Bergmann]

On Fri, Jan 9, 2026, at 17:37, Petr Tesarik wrote:

> + * Returns:
> + * least significant non-zero bit, 0 if all bits are zero
> + */
> +#define ffs_val(x)			\
> +({					\
> +	const typeof(x) val__ = (x);	\
> +	val__ & -val__;			\
> +})

This looks good to me, but I'd suggest using 'const auto val__'
instead of typeof(), to reduce expanding complex arguments twice.

    Arnd

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

[David Laight]

On Fri, 09 Jan 2026 18:01:02 +0100
"Arnd Bergmann" <arnd@arndb.de> wrote:

> On Fri, Jan 9, 2026, at 17:37, Petr Tesarik wrote:
> 
> > + * Returns:
> > + * least significant non-zero bit, 0 if all bits are zero
> > + */
> > +#define ffs_val(x)			\
> > +({					\
> > +	const typeof(x) val__ = (x);	\
> > +	val__ & -val__;			\
> > +})  
> 
> This looks good to me, but I'd suggest using 'const auto val__'
> instead of typeof(), to reduce expanding complex arguments twice.

It is more usual to just use a single _ prefix and the same name.
I wouldn't bother with 'const' either, maybe:
#define ffs_val(val) ({  \
	auto _val = val; \
	_val & -_val;    \
})

However it isn't necessarily better than using __ffs().
FIELD_PREP(mask, val) (for non-constant mask) can be (mask & -mask) * val
or val << __ffs(mask).
So the 'ffs' version is fewer instructions (assuming non-zero mask).
The timings for bsf/bsr are similar to those for imul on intel cpu,
mul wins on zen3 and bsf/bsr on zen4.
On balance the __ffs() version is actually likely to be faster.

Other architectures may fair better or worse.
Clearly you don't want to use __ffs() unless it is a single instruction.

Of course, for FIELD_GET(reg, mask) you'd need reg/(mask & -mask)
and the cost of the integer division is far more than __ffs().
(But that will give you a compile-time error if mask is a constant zero
and the compiler will convert the divide to a shift.)

And for 64bit calculations on 32bit 'all bets are off'.
Even the shift left might be problematic.

	David

> 
>     Arnd
> 

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

[Yury Norov]

On Fri, Jan 09, 2026 at 05:37:56PM +0100, Petr Tesarik wrote:
> 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>.

Nice catch. Thanks!

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

No need for a separate header. Just put int straight in bitops.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

By definition, the value of 1st set bit is 1, just like any other set
bit. :)

> + * @x: the value to search
> + *
> + * Unlike ffs(), which returns a bit position, ffs_val() returns the bit
> + * value itself.
> + *
> + * Returns:
> + * least significant non-zero bit, 0 if all bits are zero
> + */
> +#define ffs_val(x)			\
> +({					\
> +	const typeof(x) val__ = (x);	\

const auto? Also, are you sure it works OK with unsigned types? No
warnings? Maybe add a test?

> +	val__ & -val__;			\
> +})

This macro returns in fact a mask containing LSB only, so I'd suggest
to choose a name like lsb_mask().

This is also a replacement of BIT(ffs()), GENMASK(ffs(), 0) constructions.
Can you check the kernel, and convert those patterns too? I found at least
one in drivers/clk/nxp/clk-lpc32xx.c:lpc32xx_clk_div_quirk().

Thanks,
Yury

> +
> +#endif /* _ASM_LINUX_FFS_VAL_H_ */
> -- 
> 2.52.0

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

[Petr Tesarik]

On Fri, 9 Jan 2026 12:16:06 -0500
Yury Norov <ynorov@nvidia.com> wrote:

> On Fri, Jan 09, 2026 at 05:37:56PM +0100, Petr Tesarik wrote:
> > 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>.  
> 
> Nice catch. Thanks!

It's been on my TODO list for months, but my first attempt failed
because of a coccinelle bug, and then I never got to it again.

> > 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  
> 
> No need for a separate header. Just put int straight in bitops.h.

Well, <linux/bitops.h> is a bit heavy, so I was afraid of spoiling
build times if I include it from <asm-generic/div64.h>, but if you say
it's fine, yes, why not, let's put it into bitops.h somewhere before
#include <asm/bitops.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  
> 
> By definition, the value of 1st set bit is 1, just like any other set
> bit. :)

I'm struggling with suitable wording. The trouble is that "find first
set bit" is generally understood as find the bit _position_. Maybe I
should say _isolate_ the bit, or something like that.

> > + * @x: the value to search
> > + *
> > + * Unlike ffs(), which returns a bit position, ffs_val() returns the bit
> > + * value itself.
> > + *
> > + * Returns:
> > + * least significant non-zero bit, 0 if all bits are zero
> > + */
> > +#define ffs_val(x)			\
> > +({					\
> > +	const typeof(x) val__ = (x);	\  
> 
> const auto? Also, are you sure it works OK with unsigned types? No
> warnings? Maybe add a test?

The "const auto" is good idea. Regarding unsigned types, indeed, the
result of applying unary minus to any non-zero value is out of bounds
of an unsigned type. However, the C standard has this much to say:
"C’s unsigned integer types are ‘‘modulo’’ in the LIA−1 sense in that
overflows or out-of-bounds results silently wrap."

Besides, this patch series does not change anything, it merely puts the
arithmetic inside a macro.

> > +	val__ & -val__;			\
> > +})  
> 
> This macro returns in fact a mask containing LSB only, so I'd suggest
> to choose a name like lsb_mask().

Mask is a terrible word, because it doesn't say if the masked bit is
set or clear. Even if I limit myself to the Linux kernel, it's used for
both in different contexts.

What about isolate_lsb()?

The only issue is that LSB may also refer to least-significant BYTE. :-(

> This is also a replacement of BIT(ffs()), GENMASK(ffs(), 0) constructions.
> Can you check the kernel, and convert those patterns too? I found at least
> one in drivers/clk/nxp/clk-lpc32xx.c:lpc32xx_clk_div_quirk().

Yes, there's a lot of places under drivers/ that can benefit from this
macro. I didn't want to spam everyone with this RFC, as we iron out the
details. I'm even unsure about the correct process to get such a change
into the kernel.

Thanks for the review and suggestions!

Petr T

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

[Yury Norov]

> > No need for a separate header. Just put int straight in bitops.h.
> 
> Well, <linux/bitops.h> is a bit heavy, so I was afraid of spoiling
> build times if I include it from <asm-generic/div64.h>, but if you say
> it's fine, yes, why not, let's put it into bitops.h somewhere before
> #include <asm/bitops.h>.

Unless you have strong performance numbers, let's keep it in the
bitops.h
 
> > > +/**
> > > + * ffs_val - find the value of the first set bit  
> > 
> > By definition, the value of 1st set bit is 1, just like any other set
> > bit. :)
> 
> I'm struggling with suitable wording. The trouble is that "find first
> set bit" is generally understood as find the bit _position_. Maybe I
> should say _isolate_ the bit, or something like that.
> 
> > > + * @x: the value to search
> > > + *
> > > + * Unlike ffs(), which returns a bit position, ffs_val() returns the bit
> > > + * value itself.
> > > + *
> > > + * Returns:
> > > + * least significant non-zero bit, 0 if all bits are zero
> > > + */
> > > +#define ffs_val(x)			\
> > > +({					\
> > > +	const typeof(x) val__ = (x);	\  
> > 
> > const auto? Also, are you sure it works OK with unsigned types? No
> > warnings? Maybe add a test?
> 
> The "const auto" is good idea. Regarding unsigned types, indeed, the
> result of applying unary minus to any non-zero value is out of bounds
> of an unsigned type. However, the C standard has this much to say:
> "C’s unsigned integer types are ‘‘modulo’’ in the LIA−1 sense in that
> overflows or out-of-bounds results silently wrap."
> 
> Besides, this patch series does not change anything, it merely puts the
> arithmetic inside a macro.
> 
> > > +	val__ & -val__;			\
> > > +})  
> > 
> > This macro returns in fact a mask containing LSB only, so I'd suggest
> > to choose a name like lsb_mask().
> 
> Mask is a terrible word, because it doesn't say if the masked bit is
> set or clear. Even if I limit myself to the Linux kernel, it's used for
> both in different contexts.
> 
> What about isolate_lsb()?

In bitops world, something_mask() has a very clear meaning. Consider
GENMASK(), BITMAP_FIRST_BYTE_MASK(), __BF_FIELD_CHECK_MASK(), and so
on.

lsb_mask(), or LSB_MASK() if you prefer, is just right.

> The only issue is that LSB may also refer to least-significant BYTE. :-(

It will never mean byte because it hosts in bitops.h, not byteops.h
 
> > This is also a replacement of BIT(ffs()), GENMASK(ffs(), 0) constructions.
> > Can you check the kernel, and convert those patterns too? I found at least
> > one in drivers/clk/nxp/clk-lpc32xx.c:lpc32xx_clk_div_quirk().
> 
> Yes, there's a lot of places under drivers/ that can benefit from this
> macro. I didn't want to spam everyone with this RFC, as we iron out the
> details. I'm even unsure about the correct process to get such a change
> into the kernel.

If you don't want to fix every driver, it's OK. But please keep core
kernel clean.

> 
> Thanks for the review and suggestions!
> 
> Petr T

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

[Petr Tesarik]

On Fri, 9 Jan 2026 13:26:30 -0500
Yury Norov <ynorov@nvidia.com> wrote:

> > > No need for a separate header. Just put int straight in bitops.h.  
> > 
> > Well, <linux/bitops.h> is a bit heavy, so I was afraid of spoiling
> > build times if I include it from <asm-generic/div64.h>, but if you say
> > it's fine, yes, why not, let's put it into bitops.h somewhere before
> > #include <asm/bitops.h>.  
> 
> Unless you have strong performance numbers, let's keep it in the
> bitops.h

OK.

> > > > +/**
> > > > + * ffs_val - find the value of the first set bit    
> > > 
> > > By definition, the value of 1st set bit is 1, just like any other set
> > > bit. :)  
> > 
> > I'm struggling with suitable wording. The trouble is that "find first
> > set bit" is generally understood as find the bit _position_. Maybe I
> > should say _isolate_ the bit, or something like that.
> >   
> > > > + * @x: the value to search
> > > > + *
> > > > + * Unlike ffs(), which returns a bit position, ffs_val() returns the bit
> > > > + * value itself.
> > > > + *
> > > > + * Returns:
> > > > + * least significant non-zero bit, 0 if all bits are zero
> > > > + */
> > > > +#define ffs_val(x)			\
> > > > +({					\
> > > > +	const typeof(x) val__ = (x);	\    
> > > 
> > > const auto? Also, are you sure it works OK with unsigned types? No
> > > warnings? Maybe add a test?  
> > 
> > The "const auto" is good idea. Regarding unsigned types, indeed, the
> > result of applying unary minus to any non-zero value is out of bounds
> > of an unsigned type. However, the C standard has this much to say:
> > "C’s unsigned integer types are ‘‘modulo’’ in the LIA−1 sense in that
> > overflows or out-of-bounds results silently wrap."
> > 
> > Besides, this patch series does not change anything, it merely puts the
> > arithmetic inside a macro.
> >   
> > > > +	val__ & -val__;			\
> > > > +})    
> > > 
> > > This macro returns in fact a mask containing LSB only, so I'd suggest
> > > to choose a name like lsb_mask().  
> > 
> > Mask is a terrible word, because it doesn't say if the masked bit is
> > set or clear. Even if I limit myself to the Linux kernel, it's used for
> > both in different contexts.
> > 
> > What about isolate_lsb()?  
> 
> In bitops world, something_mask() has a very clear meaning. Consider
> GENMASK(), BITMAP_FIRST_BYTE_MASK(), __BF_FIELD_CHECK_MASK(), and so
> on.
> 
> lsb_mask(), or LSB_MASK() if you prefer, is just right.

I assume you mean BITMAP_FIRST_WORD_MASK(), not btrfs-specific
BITMAP_FIRST_BYTE_MASK(), defined in fs/btrfs/extent_io.h.

But then it nicely illustrate exactly what I mean:

  BIT_MASK(4) = 0x0000000000000010
  GENMASK(4, 0) = 0x000000000000001f
  BITMAP_FIRST_WORD_MASK(4) == 0xfffffffffffffff0

> > The only issue is that LSB may also refer to least-significant BYTE. :-(  
> 
> It will never mean byte because it hosts in bitops.h, not byteops.h

I'm afraid that identifiers from various sources end up next to each
other and they do not carry tags where each of them came from...

> > > This is also a replacement of BIT(ffs()), GENMASK(ffs(), 0) constructions.
> > > Can you check the kernel, and convert those patterns too? I found at least
> > > one in drivers/clk/nxp/clk-lpc32xx.c:lpc32xx_clk_div_quirk().  
> > 
> > Yes, there's a lot of places under drivers/ that can benefit from this
> > macro. I didn't want to spam everyone with this RFC, as we iron out the
> > details. I'm even unsure about the correct process to get such a change
> > into the kernel.  
> 
> If you don't want to fix every driver, it's OK. But please keep core
> kernel clean.

Yes, getting acks from all maintainers will take a lot of time even
then.

Again, thank you for all the tips!

Petr T

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

[Yury Norov]

On Fri, Jan 09, 2026 at 08:27:11PM +0100, Petr Tesarik wrote:
> On Fri, 9 Jan 2026 13:26:30 -0500
> Yury Norov <ynorov@nvidia.com> wrote:

...

> > In bitops world, something_mask() has a very clear meaning. Consider
> > GENMASK(), BITMAP_FIRST_BYTE_MASK(), __BF_FIELD_CHECK_MASK(), and so
> > on.
> > 
> > lsb_mask(), or LSB_MASK() if you prefer, is just right.
> 
> I assume you mean BITMAP_FIRST_WORD_MASK(), not btrfs-specific
> BITMAP_FIRST_BYTE_MASK(), defined in fs/btrfs/extent_io.h.
> 
> But then it nicely illustrate exactly what I mean:

They all have a consistent meaning:

>   BIT_MASK(4) = 0x0000000000000010

A mask constructed from bit #4.

>   GENMASK(4, 0) = 0x000000000000001f

A mask constructed from bits #0..4.

>   BITMAP_FIRST_WORD_MASK(4) == 0xfffffffffffffff0

A mask constructed from all bits starting #4.

Don't see any misalignment here.
 
> > > The only issue is that LSB may also refer to least-significant BYTE. :-(  
> > 
> > It will never mean byte because it hosts in bitops.h, not byteops.h
> 
> I'm afraid that identifiers from various sources end up next to each
> other and they do not carry tags where each of them came from...

You can name it lsb_bitmask() if you want, either way works. But to me
shorter is better.

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

[Arnd Bergmann]

On Fri, Jan 9, 2026, at 18:46, Petr Tesarik wrote:
> On Fri, 9 Jan 2026 12:16:06 -0500 Yury Norov <ynorov@nvidia.com> wrote:
>> > 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  
>> 
>> No need for a separate header. Just put int straight in bitops.h.
>
> Well, <linux/bitops.h> is a bit heavy, so I was afraid of spoiling
> build times if I include it from <asm-generic/div64.h>, but if you say
> it's fine, yes, why not, let's put it into bitops.h somewhere before
> #include <asm/bitops.h>.

A single macro definition makes zero difference for build times.

What makes linux/bitops.h expensive is the 419kb of preprocessed
source code that result from the generated atomics in asm/bitops.h
that get included everywhere.

    Arnd

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

[David Laight]

On Fri,  9 Jan 2026 17:37:56 +0100
Petr Tesarik <ptesarik@suse.com> wrote:

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

I'm not sure whether ffs_val(x) is actually more readable than an
open-coded (x & -x).
If you don't know what either means you have to look it up or work
it out.
The latter just requires a bit of thought, the former searching through
the source tree for the correct header and then believing the comment
or, again, working out what it does.

That said, I'm not objecting to adding it, but the churn of changing
existing code is probably not worth the effort.

I'd also define it as x & (~x + 1) - which makes it a lot more obvious
why it is correct, the compiler will convert it to a signed negate.

Also, as I pointed out earlier, many modern cpu have an instruction
for ffs(). While x & -x is usualy better than 1u << __ffs(x); the same
is not true for y * (x & -x) and y << __ffs(x).
In particular on Zen4/5 bsf (used for __ffs) has a latency of 1 but the
multiply has a latency of 3.
Intel mainstream x86 cpu all have latency 3 for both imul and bsf.

There should be #define definitions of is_power_of_2_or_zero() !(x + (x-1))
and is_power_of_2() (!x && is_power_of_2_or_zero(x)) in the same header.
But there is only an inline is_power_of_2(unsigned long) in log.h.

	David

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

[Thomas Zimmermann]

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

> + *
> + * 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.

Best regards
Thomas

> +})
> +
> +#endif /* _ASM_LINUX_FFS_VAL_H_ */

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

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

[Petr Tesarik]

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

I think I found the best wording only when composing the cover letter:
isolate the least significant set bit. I should have gone back and
adjusted the patches, too.

> > + *
> > + * 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.

It works fine for all numbers: -LONG_MIN is LONG_MIN, and that is
incidentally the number with only the least significant bit set.

But this is all moot. After reading some other replies, I'm not even
sure, this helper adds any value. I'm going to start over by converting
occurrences of x & -x with the most suitable existing helper, and see
if there's still some value then.

Last but not least, if there's no good self-explanatory name for this
operation, then I'm told that open-coding "x & -x" is in fact easier to
read. Then again, this is the opinion of people who coded back in the
1980s, and my general feeling is that bit operations are generally less
well understood by people born after 2000...

Glad to spark a bikeshedding flamewar, anyway. /s

Petr T

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

[David Laight]

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_ */  
> 

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

[Maciej W. Rozycki]

On Mon, 12 Jan 2026, David Laight wrote:

> OTOH I'm pretty sure most modern C code assumes two's compliment binary
> numbers (and NULL being the all-zero bit pattern).

 FWIW ISO C23 mandates the two's complement integer sign representation.  
Other sign representations are no longer allowed.

  Maciej

[RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[Petr Tesarik]

Replace the open-coded bit-twiddling trick to get the least significant bit
with a call to ffs_val().

This patch has been generated by the following coccinelle script:

  @
  depends on !(file in "include/asm-generic/bitops/ffs_val.h")
	  && !(file in "tools/")
  @
  expression E;
  @@

  - ((E) & (-(E)))
  + ffs_val(E)

Include <linux/ffs_val.h> where the result did not build.

Signed-off-by: Petr Tesarik <ptesarik@suse.com>
---
 arch/alpha/include/asm/bitops.h              |  2 +-
 arch/alpha/kernel/core_cia.c                 |  2 +-
 arch/alpha/kernel/smp.c                      |  2 +-
 arch/arc/include/asm/bitops.h                |  2 +-
 arch/m68k/include/asm/bitops.h               | 12 ++++++------
 arch/mips/dec/ecc-berr.c                     |  2 +-
 arch/mips/include/asm/bitops.h               |  4 ++--
 arch/mips/pci/pci-malta.c                    |  4 ++--
 arch/powerpc/include/asm/bitops.h            |  4 ++--
 arch/powerpc/kvm/e500_mmu_host.c             |  2 +-
 arch/powerpc/lib/sstep.c                     |  2 +-
 arch/powerpc/xmon/ppc-dis.c                  |  3 ++-
 arch/powerpc/xmon/ppc-opc.c                  |  6 +++---
 arch/s390/include/asm/bitops.h               |  2 +-
 arch/xtensa/include/asm/bitops.h             |  6 +++---
 arch/xtensa/kernel/traps.c                   |  2 +-
 drivers/gpu/drm/gma500/psb_intel_sdvo.c      |  2 +-
 drivers/iommu/dma-iommu.c                    |  2 +-
 drivers/net/ethernet/netronome/nfp/bpf/jit.c |  2 +-
 drivers/net/usb/cdc_ncm.c                    |  4 ++--
 include/asm-generic/div64.h                  |  4 ++--
 include/linux/bitfield.h                     |  3 ++-
 include/linux/log2.h                         |  2 +-
 include/linux/min_heap.h                     |  5 +++--
 lib/math/gcd.c                               |  4 ++--
 lib/sort.c                                   |  3 ++-
 net/bluetooth/mgmt.c                         |  2 +-
 net/netfilter/nft_set_pipapo.c               |  2 +-
 28 files changed, 48 insertions(+), 44 deletions(-)

diff --git a/arch/alpha/include/asm/bitops.h b/arch/alpha/include/asm/bitops.h
index 76e4343c090f7..590e25862192f 100644
--- a/arch/alpha/include/asm/bitops.h
+++ b/arch/alpha/include/asm/bitops.h
@@ -317,7 +317,7 @@ static inline unsigned long ffz_b(unsigned long x)
 {
 	unsigned long sum, x1, x2, x4;
 
-	x = ~x & -~x;		/* set first 0 bit, clear others */
+	x = ffs_val(~x);	/* set first 0 bit, clear others */
 	x1 = x & 0xAA;
 	x2 = x & 0xCC;
 	x4 = x & 0xF0;
diff --git a/arch/alpha/kernel/core_cia.c b/arch/alpha/kernel/core_cia.c
index 6e577228e175d..b3c273c13c104 100644
--- a/arch/alpha/kernel/core_cia.c
+++ b/arch/alpha/kernel/core_cia.c
@@ -1035,7 +1035,7 @@ cia_decode_ecc_error(struct el_CIA_sysdata_mcheck *cia, const char *msg)
 	cia_decode_mem_error(cia, msg);
 
 	syn = cia->cia_syn & 0xff;
-	if (syn == (syn & -syn)) {
+	if (syn == ffs_val(syn)) {
 		fmt = KERN_CRIT "  ECC syndrome %#x -- check bit %d\n";
 		i = ffs(syn) - 1;
 	} else {
diff --git a/arch/alpha/kernel/smp.c b/arch/alpha/kernel/smp.c
index ed06367ece574..2f3809c015b99 100644
--- a/arch/alpha/kernel/smp.c
+++ b/arch/alpha/kernel/smp.c
@@ -525,7 +525,7 @@ handle_ipi(struct pt_regs *regs)
 	  do {
 		unsigned long which;
 
-		which = ops & -ops;
+		which = ffs_val(ops);
 		ops &= ~which;
 		which = __ffs(which);
 
diff --git a/arch/arc/include/asm/bitops.h b/arch/arc/include/asm/bitops.h
index df894235fdbc6..def870bd7e4c7 100644
--- a/arch/arc/include/asm/bitops.h
+++ b/arch/arc/include/asm/bitops.h
@@ -94,7 +94,7 @@ static inline __attribute__ ((const)) unsigned long __fls(unsigned long x)
  * ffs = Find First Set in word (LSB to MSB)
  * @result: [1-32], 0 if all 0's
  */
-#define ffs(x)	({ unsigned long __t = (x); fls(__t & -__t); })
+#define ffs(x)	({ unsigned long __t = (x); fls(ffs_val(__t)); })
 
 /*
  * __ffs: Similar to ffs, but zero based (0-31)
diff --git a/arch/m68k/include/asm/bitops.h b/arch/m68k/include/asm/bitops.h
index e9639e48c6c3c..a67f08b8a7541 100644
--- a/arch/m68k/include/asm/bitops.h
+++ b/arch/m68k/include/asm/bitops.h
@@ -368,7 +368,7 @@ static inline unsigned long find_first_zero_bit(const unsigned long *vaddr,
 	}
 
 	__asm__ __volatile__ ("bfffo %1{#0,#0},%0"
-			      : "=d" (res) : "d" (num & -num));
+			      : "=d" (res) : "d" (ffs_val(num)));
 	res ^= 31;
 out:
 	res += ((long)p - (long)vaddr - 4) * 8;
@@ -392,7 +392,7 @@ static inline unsigned long find_next_zero_bit(const unsigned long *vaddr,
 
 		/* Look for zero in first longword */
 		__asm__ __volatile__ ("bfffo %1{#0,#0},%0"
-				      : "=d" (res) : "d" (num & -num));
+				      : "=d" (res) : "d" (ffs_val(num)));
 		if (res < 32) {
 			offset += res ^ 31;
 			return offset < size ? offset : size;
@@ -425,7 +425,7 @@ static inline unsigned long find_first_bit(const unsigned long *vaddr,
 	}
 
 	__asm__ __volatile__ ("bfffo %1{#0,#0},%0"
-			      : "=d" (res) : "d" (num & -num));
+			      : "=d" (res) : "d" (ffs_val(num)));
 	res ^= 31;
 out:
 	res += ((long)p - (long)vaddr - 4) * 8;
@@ -449,7 +449,7 @@ static inline unsigned long find_next_bit(const unsigned long *vaddr,
 
 		/* Look for one in first longword */
 		__asm__ __volatile__ ("bfffo %1{#0,#0},%0"
-				      : "=d" (res) : "d" (num & -num));
+				      : "=d" (res) : "d" (ffs_val(num)));
 		if (res < 32) {
 			offset += res ^ 31;
 			return offset < size ? offset : size;
@@ -473,7 +473,7 @@ static inline unsigned long __attribute_const__ ffz(unsigned long word)
 	int res;
 
 	__asm__ __volatile__ ("bfffo %1{#0,#0},%0"
-			      : "=d" (res) : "d" (~word & -~word));
+			      : "=d" (res) : "d" (ffs_val(~word)));
 	return res ^ 31;
 }
 
@@ -527,7 +527,7 @@ static inline __attribute_const__ int ffs(int x)
 
 	__asm__ ("bfffo %1{#0:#0},%0"
 		: "=d" (cnt)
-		: "dm" (x & -x));
+		: "dm" (ffs_val(x)));
 	return 32 - cnt;
 }
 
diff --git a/arch/mips/dec/ecc-berr.c b/arch/mips/dec/ecc-berr.c
index 1eb356fdd8323..8934b8b1cf375 100644
--- a/arch/mips/dec/ecc-berr.c
+++ b/arch/mips/dec/ecc-berr.c
@@ -153,7 +153,7 @@ static int dec_ecc_be_backend(struct pt_regs *regs, int is_fixup, int invoker)
 			/* Ack now, now we've rewritten (or not). */
 			dec_ecc_be_ack();
 
-			if (syn && syn == (syn & -syn)) {
+			if (syn && syn == ffs_val(syn)) {
 				if (syn == 0x01) {
 					fmt = KERN_ALERT "%s"
 					      "%#04x -- %s bit error "
diff --git a/arch/mips/include/asm/bitops.h b/arch/mips/include/asm/bitops.h
index 42f88452c920f..6e71999618876 100644
--- a/arch/mips/include/asm/bitops.h
+++ b/arch/mips/include/asm/bitops.h
@@ -395,7 +395,7 @@ static __always_inline __attribute_const__ unsigned long __fls(unsigned long wor
  */
 static __always_inline __attribute_const__ unsigned long __ffs(unsigned long word)
 {
-	return __fls(word & -word);
+	return __fls(ffs_val(word));
 }
 
 /*
@@ -463,7 +463,7 @@ static inline __attribute_const__ int ffs(int word)
 	if (!word)
 		return 0;
 
-	return fls(word & -word);
+	return fls(ffs_val(word));
 }
 
 #include <asm-generic/bitops/ffz.h>
diff --git a/arch/mips/pci/pci-malta.c b/arch/mips/pci/pci-malta.c
index 2e35aeba45bc1..502f99f46eb75 100644
--- a/arch/mips/pci/pci-malta.c
+++ b/arch/mips/pci/pci-malta.c
@@ -117,7 +117,7 @@ void __init mips_pcibios_init(void)
 		mask = ~(start ^ end);
 		/* We don't support remapping with a discontiguous mask.  */
 		BUG_ON((start & GT_PCI_HD_MSK) != (map & GT_PCI_HD_MSK) &&
-		       mask != ~((mask & -mask) - 1));
+		       mask != ~(ffs_val(mask) - 1));
 		gt64120_mem_resource.start = start;
 		gt64120_mem_resource.end = end;
 		gt64120_controller.mem_offset = (start & mask) - (map & mask);
@@ -134,7 +134,7 @@ void __init mips_pcibios_init(void)
 		mask = ~(start ^ end);
 		/* We don't support remapping with a discontiguous mask.  */
 		BUG_ON((start & GT_PCI_HD_MSK) != (map & GT_PCI_HD_MSK) &&
-		       mask != ~((mask & -mask) - 1));
+		       mask != ~(ffs_val(mask) - 1));
 		gt64120_io_resource.start = map & mask;
 		gt64120_io_resource.end = (map & mask) | ~mask;
 		gt64120_controller.io_offset = 0;
diff --git a/arch/powerpc/include/asm/bitops.h b/arch/powerpc/include/asm/bitops.h
index 0d0470cd5ac31..571700bb41e46 100644
--- a/arch/powerpc/include/asm/bitops.h
+++ b/arch/powerpc/include/asm/bitops.h
@@ -87,8 +87,8 @@ static __always_inline bool is_rlwinm_mask_valid(unsigned long x)
 	if (!x)
 		return false;
 	if (x & 1)
-		x = ~x;	// make the mask non-wrapping
-	x += x & -x;	// adding the low set bit results in at most one bit set
+		x = ~x;		// make the mask non-wrapping
+	x += ffs_val(x);	// adding the low set bit results in at most one bit set
 
 	return !(x & (x - 1));
 }
diff --git a/arch/powerpc/kvm/e500_mmu_host.c b/arch/powerpc/kvm/e500_mmu_host.c
index 06caf8bbbe2b7..00ae88b3369fd 100644
--- a/arch/powerpc/kvm/e500_mmu_host.c
+++ b/arch/powerpc/kvm/e500_mmu_host.c
@@ -205,7 +205,7 @@ void inval_gtlbe_on_host(struct kvmppc_vcpu_e500 *vcpu_e500, int tlbsel,
 
 		local_irq_save(flags);
 		while (tmp) {
-			hw_tlb_indx = __ilog2_u64(tmp & -tmp);
+			hw_tlb_indx = __ilog2_u64(ffs_val(tmp));
 			mtspr(SPRN_MAS0,
 			      MAS0_TLBSEL(1) |
 			      MAS0_ESEL(to_htlb1_esel(hw_tlb_indx)));
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index ac3ee19531d8a..2a8a7e007dbd2 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -236,7 +236,7 @@ static nokprobe_inline unsigned long mlsd_8lsd_ea(unsigned int instr,
 static nokprobe_inline unsigned long max_align(unsigned long x)
 {
 	x |= sizeof(unsigned long);
-	return x & -x;		/* isolates rightmost bit */
+	return ffs_val(x);		/* isolates rightmost bit */
 }
 
 static nokprobe_inline unsigned long byterev_2(unsigned long x)
diff --git a/arch/powerpc/xmon/ppc-dis.c b/arch/powerpc/xmon/ppc-dis.c
index af105e1bc3fca..13b31964686ad 100644
--- a/arch/powerpc/xmon/ppc-dis.c
+++ b/arch/powerpc/xmon/ppc-dis.c
@@ -9,6 +9,7 @@ This file is part of GDB, GAS, and the GNU binutils.
 
 #include <asm/cputable.h>
 #include <asm/cpu_has_feature.h>
+#include <linux/ffs_val.h>
 #include "nonstdio.h"
 #include "ansidecl.h"
 #include "ppc.h"
@@ -44,7 +45,7 @@ operand_value_powerpc (const struct powerpc_operand *operand,
 	  unsigned long top = operand->bitm;
 	  /* top & -top gives the rightmost 1 bit, so this
 	     fills in any trailing zeros.  */
-	  top |= (top & -top) - 1;
+	  top |= ffs_val(top) - 1;
 	  top &= ~(top >> 1);
 	  value = (value ^ top) - top;
 	}
diff --git a/arch/powerpc/xmon/ppc-opc.c b/arch/powerpc/xmon/ppc-opc.c
index de9b4236728c4..23d845439fa11 100644
--- a/arch/powerpc/xmon/ppc-opc.c
+++ b/arch/powerpc/xmon/ppc-opc.c
@@ -1417,7 +1417,7 @@ insert_fxm (unsigned long insn,
      one bit of the mask field is set.  */
   if ((insn & (1 << 20)) != 0)
     {
-      if (value == 0 || (value & -value) != value)
+      if (value == 0 || ffs_val(value) != value)
 	{
 	  *errmsg = _("invalid mask field");
 	  value = 0;
@@ -1430,7 +1430,7 @@ insert_fxm (unsigned long insn,
      new form unless -mpower4 has been given, or -many and the two
      operand form of mfcr was used.  */
   else if (value > 0
-	   && (value & -value) == value
+	   && ffs_val(value) == value
 	   && ((dialect & PPC_OPCODE_POWER4) != 0
 	       || ((dialect & PPC_OPCODE_ANY) != 0
 		   && (insn & (0x3ff << 1)) == 19 << 1)))
@@ -1460,7 +1460,7 @@ extract_fxm (unsigned long insn,
   if ((insn & (1 << 20)) != 0)
     {
       /* Exactly one bit of MASK should be set.  */
-      if (mask == 0 || (mask & -mask) != mask)
+      if (mask == 0 || ffs_val(mask) != mask)
 	*invalid = 1;
     }
 
diff --git a/arch/s390/include/asm/bitops.h b/arch/s390/include/asm/bitops.h
index 5f10074665b0c..dadd8a5bd5428 100644
--- a/arch/s390/include/asm/bitops.h
+++ b/arch/s390/include/asm/bitops.h
@@ -193,7 +193,7 @@ static __always_inline __flatten __attribute_const__ int ffs(int word)
 {
 	unsigned int val = (unsigned int)word;
 
-	return BITS_PER_LONG - __flogr(-val & val);
+	return BITS_PER_LONG - __flogr(ffs_val(val));
 }
 
 #else /* CONFIG_CC_HAS_BUILTIN_FFS */
diff --git a/arch/xtensa/include/asm/bitops.h b/arch/xtensa/include/asm/bitops.h
index f7390b6761e1b..44e0debde1b3d 100644
--- a/arch/xtensa/include/asm/bitops.h
+++ b/arch/xtensa/include/asm/bitops.h
@@ -39,7 +39,7 @@ static inline unsigned long __cntlz (unsigned long x)
 
 static inline int __attribute_const__ ffz(unsigned long x)
 {
-	return 31 - __cntlz(~x & -~x);
+	return 31 - __cntlz(ffs_val(~x));
 }
 
 /*
@@ -48,7 +48,7 @@ static inline int __attribute_const__ ffz(unsigned long x)
 
 static inline __attribute_const__ unsigned long __ffs(unsigned long x)
 {
-	return 31 - __cntlz(x & -x);
+	return 31 - __cntlz(ffs_val(x));
 }
 
 /*
@@ -59,7 +59,7 @@ static inline __attribute_const__ unsigned long __ffs(unsigned long x)
 
 static inline __attribute_const__ int ffs(unsigned long x)
 {
-	return 32 - __cntlz(x & -x);
+	return 32 - __cntlz(ffs_val(x));
 }
 
 /*
diff --git a/arch/xtensa/kernel/traps.c b/arch/xtensa/kernel/traps.c
index 44c07c4e0833a..7ab71b21581f5 100644
--- a/arch/xtensa/kernel/traps.c
+++ b/arch/xtensa/kernel/traps.c
@@ -309,7 +309,7 @@ static void do_interrupt(struct pt_regs *regs)
 			break;
 
 		/* clear lowest pending irq in the unhandled mask */
-		unhandled ^= (int_at_level & -int_at_level);
+		unhandled ^= ffs_val(int_at_level);
 		do_IRQ(__ffs(int_at_level), regs);
 	}
 
diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
index 553e7c7d9bb83..56d3fa0bd0334 100644
--- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c
+++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
@@ -1226,7 +1226,7 @@ psb_intel_sdvo_multifunc_encoder(struct psb_intel_sdvo *psb_intel_sdvo)
 {
 	/* Is there more than one type of output? */
 	int caps = psb_intel_sdvo->caps.output_flags & 0xf;
-	return caps & -caps;
+	return ffs_val(caps);
 }
 
 static struct edid *
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index c92088855450a..a0211b51d8043 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -943,7 +943,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
 	    iommu_deferred_attach(dev, domain))
 		return NULL;
 
-	min_size = alloc_sizes & -alloc_sizes;
+	min_size = ffs_val(alloc_sizes);
 	if (min_size < PAGE_SIZE) {
 		min_size = PAGE_SIZE;
 		alloc_sizes |= PAGE_SIZE;
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/jit.c b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
index 3a02eef58cc6d..25e0905a61721 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/jit.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
@@ -1565,7 +1565,7 @@ static int wrp_div_imm(struct nfp_prog *nfp_prog, u8 dst, u64 imm)
 	rvalue = reciprocal_value_adv(imm, 32);
 	exp = rvalue.exp;
 	if (rvalue.is_wide_m && !(imm & 1)) {
-		pre_shift = fls(imm & -imm) - 1;
+		pre_shift = fls(ffs_val(imm)) - 1;
 		rvalue = reciprocal_value_adv(imm >> pre_shift, 32 - pre_shift);
 	} else {
 		pre_shift = 0;
diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 5d123df0a866b..e132f222a026a 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -668,7 +668,7 @@ static void cdc_ncm_fix_modulus(struct usbnet *dev)
 	val = ctx->tx_ndp_modulus;
 
 	if ((val < USB_CDC_NCM_NDP_ALIGN_MIN_SIZE) ||
-	    (val != ((-val) & val)) || (val >= ctx->tx_max)) {
+	    (val != ffs_val(val)) || (val >= ctx->tx_max)) {
 		dev_dbg(&dev->intf->dev, "Using default alignment: 4 bytes\n");
 		ctx->tx_ndp_modulus = USB_CDC_NCM_NDP_ALIGN_MIN_SIZE;
 	}
@@ -682,7 +682,7 @@ static void cdc_ncm_fix_modulus(struct usbnet *dev)
 	val = ctx->tx_modulus;
 
 	if ((val < USB_CDC_NCM_NDP_ALIGN_MIN_SIZE) ||
-	    (val != ((-val) & val)) || (val >= ctx->tx_max)) {
+	    (val != ffs_val(val)) || (val >= ctx->tx_max)) {
 		dev_dbg(&dev->intf->dev, "Using default transmit modulus: 4 bytes\n");
 		ctx->tx_modulus = USB_CDC_NCM_NDP_ALIGN_MIN_SIZE;
 	}
diff --git a/include/asm-generic/div64.h b/include/asm-generic/div64.h
index 25e7b4b58dcf5..d56208be887de 100644
--- a/include/asm-generic/div64.h
+++ b/include/asm-generic/div64.h
@@ -111,8 +111,8 @@
 	}								\
 									\
 	/* Reduce m / p to help avoid overflow handling later. */	\
-	___p /= (___m & -___m);						\
-	___m /= (___m & -___m);						\
+	___p /= ffs_val(___m);						\
+	___m /= ffs_val(___m);						\
 									\
 	/*								\
 	 * Perform (m_bias + m * n) / (1 << 64).			\
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 126dc5b380afa..26fbb6cfc0719 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -8,6 +8,7 @@
 #define _LINUX_BITFIELD_H
 
 #include <linux/build_bug.h>
+#include <linux/ffs_val.h>
 #include <linux/typecheck.h>
 #include <asm/byteorder.h>
 
@@ -202,7 +203,7 @@ static __always_inline u64 field_multiplier(u64 field)
 {
 	if ((field | (field - 1)) & ((field | (field - 1)) + 1))
 		__bad_mask();
-	return field & -field;
+	return ffs_val(field);
 }
 static __always_inline u64 field_mask(u64 field)
 {
diff --git a/include/linux/log2.h b/include/linux/log2.h
index 2eac3fc9303d6..7615e513885e9 100644
--- a/include/linux/log2.h
+++ b/include/linux/log2.h
@@ -266,7 +266,7 @@ int __bits_per(unsigned long n)
 static inline __attribute__((const))
 unsigned int max_pow_of_two_factor(unsigned int n)
 {
-	return n & -n;
+	return ffs_val(n);
 }
 
 #endif /* _LINUX_LOG2_H */
diff --git a/include/linux/min_heap.h b/include/linux/min_heap.h
index 79ddc0adbf2bf..e914919470c88 100644
--- a/include/linux/min_heap.h
+++ b/include/linux/min_heap.h
@@ -3,6 +3,7 @@
 #define _LINUX_MIN_HEAP_H
 
 #include <linux/bug.h>
+#include <linux/ffs_val.h>
 #include <linux/string.h>
 #include <linux/types.h>
 
@@ -257,7 +258,7 @@ static __always_inline
 void __min_heap_sift_down_inline(min_heap_char *heap, size_t pos, size_t elem_size,
 				 const struct min_heap_callbacks *func, void *args)
 {
-	const unsigned long lsbit = elem_size & -elem_size;
+	const unsigned long lsbit = ffs_val(elem_size);
 	void *data = heap->data;
 	void (*swp)(void *lhs, void *rhs, void *args) = func->swp;
 	/* pre-scale counters for performance */
@@ -297,7 +298,7 @@ static __always_inline
 void __min_heap_sift_up_inline(min_heap_char *heap, size_t elem_size, size_t idx,
 			       const struct min_heap_callbacks *func, void *args)
 {
-	const unsigned long lsbit = elem_size & -elem_size;
+	const unsigned long lsbit = ffs_val(elem_size);
 	void *data = heap->data;
 	void (*swp)(void *lhs, void *rhs, void *args) = func->swp;
 	/* pre-scale counters for performance */
diff --git a/lib/math/gcd.c b/lib/math/gcd.c
index 62efca6787aef..3706c84e68bc1 100644
--- a/lib/math/gcd.c
+++ b/lib/math/gcd.c
@@ -23,12 +23,12 @@ static unsigned long binary_gcd(unsigned long a, unsigned long b)
 
 	b >>= __ffs(b);
 	if (b == 1)
-		return r & -r;
+		return ffs_val(r);
 
 	for (;;) {
 		a >>= __ffs(a);
 		if (a == 1)
-			return r & -r;
+			return ffs_val(r);
 		if (a == b)
 			return a << __ffs(r);
 
diff --git a/lib/sort.c b/lib/sort.c
index 52363995ccc5c..2489aaaa21a06 100644
--- a/lib/sort.c
+++ b/lib/sort.c
@@ -12,6 +12,7 @@
 
 #include <linux/types.h>
 #include <linux/export.h>
+#include <linux/ffs_val.h>
 #include <linux/sort.h>
 
 /**
@@ -196,7 +197,7 @@ static void __sort_r(void *base, size_t num, size_t size,
 {
 	/* pre-scale counters for performance */
 	size_t n = num * size, a = (num/2) * size;
-	const unsigned int lsbit = size & -size;  /* Used to find parent */
+	const unsigned int lsbit = ffs_val(size);  /* Used to find parent */
 	size_t shift = 0;
 
 	if (!a)		/* num < 2 || size == 0 */
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 5be9b8c919490..2b13998fdb0fe 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -8664,7 +8664,7 @@ static bool requested_adv_flags_are_valid(struct hci_dev *hdev, u32 adv_flags)
 	supported_flags = get_supported_adv_flags(hdev);
 	phy_flags = adv_flags & MGMT_ADV_FLAG_SEC_MASK;
 	if (adv_flags & ~supported_flags ||
-	    ((phy_flags && (phy_flags ^ (phy_flags & -phy_flags)))))
+	    ((phy_flags && (phy_flags ^ ffs_val(phy_flags)))))
 		return false;
 
 	return true;
diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
index 112fe46788b6f..f93f509cc3fbf 100644
--- a/net/netfilter/nft_set_pipapo.c
+++ b/net/netfilter/nft_set_pipapo.c
@@ -370,7 +370,7 @@ int pipapo_refill(unsigned long *map, unsigned int len, unsigned int rules,
 	for (k = 0; k < len; k++) {
 		bitset = map[k];
 		while (bitset) {
-			unsigned long t = bitset & -bitset;
+			unsigned long t = ffs_val(bitset);
 			int r = __builtin_ctzl(bitset);
 			int i = k * BITS_PER_LONG + r;
 
-- 
2.52.0

Re: [RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[Yury Norov]

On Fri, Jan 09, 2026 at 05:37:57PM +0100, Petr Tesarik wrote:
> Replace the open-coded bit-twiddling trick to get the least significant bit
> with a call to ffs_val().
> 
> This patch has been generated by the following coccinelle script:
> 
>   @
>   depends on !(file in "include/asm-generic/bitops/ffs_val.h")
> 	  && !(file in "tools/")
>   @
>   expression E;
>   @@
> 
>   - ((E) & (-(E)))
>   + ffs_val(E)
> 
> Include <linux/ffs_val.h> where the result did not build.
> 
> Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> ---
>  arch/alpha/include/asm/bitops.h              |  2 +-
>  arch/alpha/kernel/core_cia.c                 |  2 +-
>  arch/alpha/kernel/smp.c                      |  2 +-
>  arch/arc/include/asm/bitops.h                |  2 +-
>  arch/m68k/include/asm/bitops.h               | 12 ++++++------
>  arch/mips/dec/ecc-berr.c                     |  2 +-
>  arch/mips/include/asm/bitops.h               |  4 ++--
>  arch/mips/pci/pci-malta.c                    |  4 ++--
>  arch/powerpc/include/asm/bitops.h            |  4 ++--
>  arch/powerpc/kvm/e500_mmu_host.c             |  2 +-
>  arch/powerpc/lib/sstep.c                     |  2 +-
>  arch/powerpc/xmon/ppc-dis.c                  |  3 ++-
>  arch/powerpc/xmon/ppc-opc.c                  |  6 +++---
>  arch/s390/include/asm/bitops.h               |  2 +-
>  arch/xtensa/include/asm/bitops.h             |  6 +++---
>  arch/xtensa/kernel/traps.c                   |  2 +-
>  drivers/gpu/drm/gma500/psb_intel_sdvo.c      |  2 +-
>  drivers/iommu/dma-iommu.c                    |  2 +-
>  drivers/net/ethernet/netronome/nfp/bpf/jit.c |  2 +-
>  drivers/net/usb/cdc_ncm.c                    |  4 ++--
>  include/asm-generic/div64.h                  |  4 ++--
>  include/linux/bitfield.h                     |  3 ++-
>  include/linux/log2.h                         |  2 +-
>  include/linux/min_heap.h                     |  5 +++--
>  lib/math/gcd.c                               |  4 ++--
>  lib/sort.c                                   |  3 ++-
>  net/bluetooth/mgmt.c                         |  2 +-
>  net/netfilter/nft_set_pipapo.c               |  2 +-
>  28 files changed, 48 insertions(+), 44 deletions(-)
> 
> diff --git a/arch/alpha/include/asm/bitops.h b/arch/alpha/include/asm/bitops.h
> index 76e4343c090f7..590e25862192f 100644
> --- a/arch/alpha/include/asm/bitops.h
> +++ b/arch/alpha/include/asm/bitops.h
> @@ -317,7 +317,7 @@ static inline unsigned long ffz_b(unsigned long x)
>  {
>  	unsigned long sum, x1, x2, x4;
>  
> -	x = ~x & -~x;		/* set first 0 bit, clear others */
> +	x = ffs_val(~x);	/* set first 0 bit, clear others */
>  	x1 = x & 0xAA;
>  	x2 = x & 0xCC;
>  	x4 = x & 0xF0;
> diff --git a/arch/alpha/kernel/core_cia.c b/arch/alpha/kernel/core_cia.c
> index 6e577228e175d..b3c273c13c104 100644
> --- a/arch/alpha/kernel/core_cia.c
> +++ b/arch/alpha/kernel/core_cia.c
> @@ -1035,7 +1035,7 @@ cia_decode_ecc_error(struct el_CIA_sysdata_mcheck *cia, const char *msg)
>  	cia_decode_mem_error(cia, msg);
>  
>  	syn = cia->cia_syn & 0xff;
> -	if (syn == (syn & -syn)) {
> +	if (syn == ffs_val(syn)) {
>  		fmt = KERN_CRIT "  ECC syndrome %#x -- check bit %d\n";
>  		i = ffs(syn) - 1;
>  	} else {
> diff --git a/arch/alpha/kernel/smp.c b/arch/alpha/kernel/smp.c
> index ed06367ece574..2f3809c015b99 100644
> --- a/arch/alpha/kernel/smp.c
> +++ b/arch/alpha/kernel/smp.c
> @@ -525,7 +525,7 @@ handle_ipi(struct pt_regs *regs)
>  	  do {
>  		unsigned long which;
>  
> -		which = ops & -ops;
> +		which = ffs_val(ops);
>  		ops &= ~which;
>  		which = __ffs(which);

I guess, this should be:

        which = __ffs(ops);
        ops &= ops - 1;

Check how fns() works. And to that extend, you can make another
helper: clear_lsb().

> diff --git a/arch/arc/include/asm/bitops.h b/arch/arc/include/asm/bitops.h
> index df894235fdbc6..def870bd7e4c7 100644
> --- a/arch/arc/include/asm/bitops.h
> +++ b/arch/arc/include/asm/bitops.h
> @@ -94,7 +94,7 @@ static inline __attribute__ ((const)) unsigned long __fls(unsigned long x)
>   * ffs = Find First Set in word (LSB to MSB)
>   * @result: [1-32], 0 if all 0's
>   */
> -#define ffs(x)	({ unsigned long __t = (x); fls(__t & -__t); })
> +#define ffs(x)	({ unsigned long __t = (x); fls(ffs_val(__t)); })
>  
>  /*
>   * __ffs: Similar to ffs, but zero based (0-31)
> diff --git a/arch/m68k/include/asm/bitops.h b/arch/m68k/include/asm/bitops.h
> index e9639e48c6c3c..a67f08b8a7541 100644
> --- a/arch/m68k/include/asm/bitops.h
> +++ b/arch/m68k/include/asm/bitops.h
> @@ -368,7 +368,7 @@ static inline unsigned long find_first_zero_bit(const unsigned long *vaddr,
>  	}
>  
>  	__asm__ __volatile__ ("bfffo %1{#0,#0},%0"
> -			      : "=d" (res) : "d" (num & -num));
> +			      : "=d" (res) : "d" (ffs_val(num)));
>  	res ^= 31;
>  out:
>  	res += ((long)p - (long)vaddr - 4) * 8;
> @@ -392,7 +392,7 @@ static inline unsigned long find_next_zero_bit(const unsigned long *vaddr,
>  
>  		/* Look for zero in first longword */
>  		__asm__ __volatile__ ("bfffo %1{#0,#0},%0"
> -				      : "=d" (res) : "d" (num & -num));
> +				      : "=d" (res) : "d" (ffs_val(num)));
>  		if (res < 32) {
>  			offset += res ^ 31;
>  			return offset < size ? offset : size;
> @@ -425,7 +425,7 @@ static inline unsigned long find_first_bit(const unsigned long *vaddr,
>  	}
>  
>  	__asm__ __volatile__ ("bfffo %1{#0,#0},%0"
> -			      : "=d" (res) : "d" (num & -num));
> +			      : "=d" (res) : "d" (ffs_val(num)));
>  	res ^= 31;
>  out:
>  	res += ((long)p - (long)vaddr - 4) * 8;
> @@ -449,7 +449,7 @@ static inline unsigned long find_next_bit(const unsigned long *vaddr,
>  
>  		/* Look for one in first longword */
>  		__asm__ __volatile__ ("bfffo %1{#0,#0},%0"
> -				      : "=d" (res) : "d" (num & -num));
> +				      : "=d" (res) : "d" (ffs_val(num)));
>  		if (res < 32) {
>  			offset += res ^ 31;
>  			return offset < size ? offset : size;
> @@ -473,7 +473,7 @@ static inline unsigned long __attribute_const__ ffz(unsigned long word)
>  	int res;
>  
>  	__asm__ __volatile__ ("bfffo %1{#0,#0},%0"
> -			      : "=d" (res) : "d" (~word & -~word));
> +			      : "=d" (res) : "d" (ffs_val(~word)));

This should be ffz_val(word). But if you like fsb_mask(), maybe
fzb_mask()?

>  	return res ^ 31;
>  }
>  
> @@ -527,7 +527,7 @@ static inline __attribute_const__ int ffs(int x)
>  
>  	__asm__ ("bfffo %1{#0:#0},%0"
>  		: "=d" (cnt)
> -		: "dm" (x & -x));
> +		: "dm" (ffs_val(x)));
>  	return 32 - cnt;
>  }
>  
> diff --git a/arch/mips/dec/ecc-berr.c b/arch/mips/dec/ecc-berr.c
> index 1eb356fdd8323..8934b8b1cf375 100644
> --- a/arch/mips/dec/ecc-berr.c
> +++ b/arch/mips/dec/ecc-berr.c
> @@ -153,7 +153,7 @@ static int dec_ecc_be_backend(struct pt_regs *regs, int is_fixup, int invoker)
>  			/* Ack now, now we've rewritten (or not). */
>  			dec_ecc_be_ack();
>  
> -			if (syn && syn == (syn & -syn)) {
> +			if (syn && syn == ffs_val(syn)) {

It opencodes is_power_of_2().

>  				if (syn == 0x01) {
>  					fmt = KERN_ALERT "%s"
>  					      "%#04x -- %s bit error "
> diff --git a/arch/mips/include/asm/bitops.h b/arch/mips/include/asm/bitops.h
> index 42f88452c920f..6e71999618876 100644
> --- a/arch/mips/include/asm/bitops.h
> +++ b/arch/mips/include/asm/bitops.h
> @@ -395,7 +395,7 @@ static __always_inline __attribute_const__ unsigned long __fls(unsigned long wor
>   */
>  static __always_inline __attribute_const__ unsigned long __ffs(unsigned long word)
>  {
> -	return __fls(word & -word);
> +	return __fls(ffs_val(word));
>  }
>  
>  /*
> @@ -463,7 +463,7 @@ static inline __attribute_const__ int ffs(int word)
>  	if (!word)
>  		return 0;
>  
> -	return fls(word & -word);
> +	return fls(ffs_val(word));
>  }
>  
>  #include <asm-generic/bitops/ffz.h>
> diff --git a/arch/mips/pci/pci-malta.c b/arch/mips/pci/pci-malta.c
> index 2e35aeba45bc1..502f99f46eb75 100644
> --- a/arch/mips/pci/pci-malta.c
> +++ b/arch/mips/pci/pci-malta.c
> @@ -117,7 +117,7 @@ void __init mips_pcibios_init(void)
>  		mask = ~(start ^ end);
>  		/* We don't support remapping with a discontiguous mask.  */
>  		BUG_ON((start & GT_PCI_HD_MSK) != (map & GT_PCI_HD_MSK) &&
> -		       mask != ~((mask & -mask) - 1));
> +		       mask != ~(ffs_val(mask) - 1));
>  		gt64120_mem_resource.start = start;
>  		gt64120_mem_resource.end = end;
>  		gt64120_controller.mem_offset = (start & mask) - (map & mask);
> @@ -134,7 +134,7 @@ void __init mips_pcibios_init(void)
>  		mask = ~(start ^ end);
>  		/* We don't support remapping with a discontiguous mask.  */
>  		BUG_ON((start & GT_PCI_HD_MSK) != (map & GT_PCI_HD_MSK) &&
> -		       mask != ~((mask & -mask) - 1));
> +		       mask != ~(ffs_val(mask) - 1));
>  		gt64120_io_resource.start = map & mask;
>  		gt64120_io_resource.end = (map & mask) | ~mask;
>  		gt64120_controller.io_offset = 0;
> diff --git a/arch/powerpc/include/asm/bitops.h b/arch/powerpc/include/asm/bitops.h
> index 0d0470cd5ac31..571700bb41e46 100644
> --- a/arch/powerpc/include/asm/bitops.h
> +++ b/arch/powerpc/include/asm/bitops.h
> @@ -87,8 +87,8 @@ static __always_inline bool is_rlwinm_mask_valid(unsigned long x)
>  	if (!x)
>  		return false;
>  	if (x & 1)
> -		x = ~x;	// make the mask non-wrapping
> -	x += x & -x;	// adding the low set bit results in at most one bit set
> +		x = ~x;		// make the mask non-wrapping
> +	x += ffs_val(x);	// adding the low set bit results in at most one bit set
>  
>  	return !(x & (x - 1));
>  }
> diff --git a/arch/powerpc/kvm/e500_mmu_host.c b/arch/powerpc/kvm/e500_mmu_host.c
> index 06caf8bbbe2b7..00ae88b3369fd 100644
> --- a/arch/powerpc/kvm/e500_mmu_host.c
> +++ b/arch/powerpc/kvm/e500_mmu_host.c
> @@ -205,7 +205,7 @@ void inval_gtlbe_on_host(struct kvmppc_vcpu_e500 *vcpu_e500, int tlbsel,
>  
>  		local_irq_save(flags);
>  		while (tmp) {
> -			hw_tlb_indx = __ilog2_u64(tmp & -tmp);
> +			hw_tlb_indx = __ilog2_u64(ffs_val(tmp));

Is it the same as __ffs64(tmp)?

>  			mtspr(SPRN_MAS0,
>  			      MAS0_TLBSEL(1) |
>  			      MAS0_ESEL(to_htlb1_esel(hw_tlb_indx)));
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index ac3ee19531d8a..2a8a7e007dbd2 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -236,7 +236,7 @@ static nokprobe_inline unsigned long mlsd_8lsd_ea(unsigned int instr,
>  static nokprobe_inline unsigned long max_align(unsigned long x)
>  {
>  	x |= sizeof(unsigned long);
> -	return x & -x;		/* isolates rightmost bit */
> +	return ffs_val(x);		/* isolates rightmost bit */
>  }
>  
>  static nokprobe_inline unsigned long byterev_2(unsigned long x)
> diff --git a/arch/powerpc/xmon/ppc-dis.c b/arch/powerpc/xmon/ppc-dis.c
> index af105e1bc3fca..13b31964686ad 100644
> --- a/arch/powerpc/xmon/ppc-dis.c
> +++ b/arch/powerpc/xmon/ppc-dis.c
> @@ -9,6 +9,7 @@ This file is part of GDB, GAS, and the GNU binutils.
>  
>  #include <asm/cputable.h>
>  #include <asm/cpu_has_feature.h>
> +#include <linux/ffs_val.h>
>  #include "nonstdio.h"
>  #include "ansidecl.h"
>  #include "ppc.h"
> @@ -44,7 +45,7 @@ operand_value_powerpc (const struct powerpc_operand *operand,
>  	  unsigned long top = operand->bitm;
>  	  /* top & -top gives the rightmost 1 bit, so this
>  	     fills in any trailing zeros.  */
> -	  top |= (top & -top) - 1;
> +	  top |= ffs_val(top) - 1;

Yeah, to me here it would be more verbose if:
          
          top |= lsb_mask(top) - 1;

>  	  top &= ~(top >> 1);
>  	  value = (value ^ top) - top;
>  	}
> diff --git a/arch/powerpc/xmon/ppc-opc.c b/arch/powerpc/xmon/ppc-opc.c
> index de9b4236728c4..23d845439fa11 100644
> --- a/arch/powerpc/xmon/ppc-opc.c
> +++ b/arch/powerpc/xmon/ppc-opc.c
> @@ -1417,7 +1417,7 @@ insert_fxm (unsigned long insn,
>       one bit of the mask field is set.  */
>    if ((insn & (1 << 20)) != 0)
>      {
> -      if (value == 0 || (value & -value) != value)
> +      if (value == 0 || ffs_val(value) != value)

Again, opencodes is_power_of_2()

>  	{
>  	  *errmsg = _("invalid mask field");
>  	  value = 0;
> @@ -1430,7 +1430,7 @@ insert_fxm (unsigned long insn,
>       new form unless -mpower4 has been given, or -many and the two
>       operand form of mfcr was used.  */
>    else if (value > 0
> -	   && (value & -value) == value
> +	   && ffs_val(value) == value
>  	   && ((dialect & PPC_OPCODE_POWER4) != 0
>  	       || ((dialect & PPC_OPCODE_ANY) != 0
>  		   && (insn & (0x3ff << 1)) == 19 << 1)))
> @@ -1460,7 +1460,7 @@ extract_fxm (unsigned long insn,
>    if ((insn & (1 << 20)) != 0)
>      {
>        /* Exactly one bit of MASK should be set.  */
> -      if (mask == 0 || (mask & -mask) != mask)
> +      if (mask == 0 || ffs_val(mask) != mask)

And again.

--

OK, let me stop here.

Can you please instead of mechanical rework check each case
individually and find the best replacement to highlight the intention?

Can you also split this patch to a series according to subsystems, so
that corresponding maintainers will have better access to their changes?

Thanks,
Yury

>  	*invalid = 1;
>      }
>  
> diff --git a/arch/s390/include/asm/bitops.h b/arch/s390/include/asm/bitops.h
> index 5f10074665b0c..dadd8a5bd5428 100644
> --- a/arch/s390/include/asm/bitops.h
> +++ b/arch/s390/include/asm/bitops.h
> @@ -193,7 +193,7 @@ static __always_inline __flatten __attribute_const__ int ffs(int word)
>  {
>  	unsigned int val = (unsigned int)word;
>  
> -	return BITS_PER_LONG - __flogr(-val & val);
> +	return BITS_PER_LONG - __flogr(ffs_val(val));
>  }
>  
>  #else /* CONFIG_CC_HAS_BUILTIN_FFS */
> diff --git a/arch/xtensa/include/asm/bitops.h b/arch/xtensa/include/asm/bitops.h
> index f7390b6761e1b..44e0debde1b3d 100644
> --- a/arch/xtensa/include/asm/bitops.h
> +++ b/arch/xtensa/include/asm/bitops.h
> @@ -39,7 +39,7 @@ static inline unsigned long __cntlz (unsigned long x)
>  
>  static inline int __attribute_const__ ffz(unsigned long x)
>  {
> -	return 31 - __cntlz(~x & -~x);
> +	return 31 - __cntlz(ffs_val(~x));
>  }
>  
>  /*
> @@ -48,7 +48,7 @@ static inline int __attribute_const__ ffz(unsigned long x)
>  
>  static inline __attribute_const__ unsigned long __ffs(unsigned long x)
>  {
> -	return 31 - __cntlz(x & -x);
> +	return 31 - __cntlz(ffs_val(x));
>  }
>  
>  /*
> @@ -59,7 +59,7 @@ static inline __attribute_const__ unsigned long __ffs(unsigned long x)
>  
>  static inline __attribute_const__ int ffs(unsigned long x)
>  {
> -	return 32 - __cntlz(x & -x);
> +	return 32 - __cntlz(ffs_val(x));
>  }
>  
>  /*
> diff --git a/arch/xtensa/kernel/traps.c b/arch/xtensa/kernel/traps.c
> index 44c07c4e0833a..7ab71b21581f5 100644
> --- a/arch/xtensa/kernel/traps.c
> +++ b/arch/xtensa/kernel/traps.c
> @@ -309,7 +309,7 @@ static void do_interrupt(struct pt_regs *regs)
>  			break;
>  
>  		/* clear lowest pending irq in the unhandled mask */
> -		unhandled ^= (int_at_level & -int_at_level);
> +		unhandled ^= ffs_val(int_at_level);
>  		do_IRQ(__ffs(int_at_level), regs);
>  	}
>  
> diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
> index 553e7c7d9bb83..56d3fa0bd0334 100644
> --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c
> +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
> @@ -1226,7 +1226,7 @@ psb_intel_sdvo_multifunc_encoder(struct psb_intel_sdvo *psb_intel_sdvo)
>  {
>  	/* Is there more than one type of output? */
>  	int caps = psb_intel_sdvo->caps.output_flags & 0xf;
> -	return caps & -caps;
> +	return ffs_val(caps);
>  }
>  
>  static struct edid *
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index c92088855450a..a0211b51d8043 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -943,7 +943,7 @@ static struct page **__iommu_dma_alloc_noncontiguous(struct device *dev,
>  	    iommu_deferred_attach(dev, domain))
>  		return NULL;
>  
> -	min_size = alloc_sizes & -alloc_sizes;
> +	min_size = ffs_val(alloc_sizes);
>  	if (min_size < PAGE_SIZE) {
>  		min_size = PAGE_SIZE;
>  		alloc_sizes |= PAGE_SIZE;
> diff --git a/drivers/net/ethernet/netronome/nfp/bpf/jit.c b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
> index 3a02eef58cc6d..25e0905a61721 100644
> --- a/drivers/net/ethernet/netronome/nfp/bpf/jit.c
> +++ b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
> @@ -1565,7 +1565,7 @@ static int wrp_div_imm(struct nfp_prog *nfp_prog, u8 dst, u64 imm)
>  	rvalue = reciprocal_value_adv(imm, 32);
>  	exp = rvalue.exp;
>  	if (rvalue.is_wide_m && !(imm & 1)) {
> -		pre_shift = fls(imm & -imm) - 1;
> +		pre_shift = fls(ffs_val(imm)) - 1;
>  		rvalue = reciprocal_value_adv(imm >> pre_shift, 32 - pre_shift);
>  	} else {
>  		pre_shift = 0;
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 5d123df0a866b..e132f222a026a 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -668,7 +668,7 @@ static void cdc_ncm_fix_modulus(struct usbnet *dev)
>  	val = ctx->tx_ndp_modulus;
>  
>  	if ((val < USB_CDC_NCM_NDP_ALIGN_MIN_SIZE) ||
> -	    (val != ((-val) & val)) || (val >= ctx->tx_max)) {
> +	    (val != ffs_val(val)) || (val >= ctx->tx_max)) {
>  		dev_dbg(&dev->intf->dev, "Using default alignment: 4 bytes\n");
>  		ctx->tx_ndp_modulus = USB_CDC_NCM_NDP_ALIGN_MIN_SIZE;
>  	}
> @@ -682,7 +682,7 @@ static void cdc_ncm_fix_modulus(struct usbnet *dev)
>  	val = ctx->tx_modulus;
>  
>  	if ((val < USB_CDC_NCM_NDP_ALIGN_MIN_SIZE) ||
> -	    (val != ((-val) & val)) || (val >= ctx->tx_max)) {
> +	    (val != ffs_val(val)) || (val >= ctx->tx_max)) {
>  		dev_dbg(&dev->intf->dev, "Using default transmit modulus: 4 bytes\n");
>  		ctx->tx_modulus = USB_CDC_NCM_NDP_ALIGN_MIN_SIZE;
>  	}
> diff --git a/include/asm-generic/div64.h b/include/asm-generic/div64.h
> index 25e7b4b58dcf5..d56208be887de 100644
> --- a/include/asm-generic/div64.h
> +++ b/include/asm-generic/div64.h
> @@ -111,8 +111,8 @@
>  	}								\
>  									\
>  	/* Reduce m / p to help avoid overflow handling later. */	\
> -	___p /= (___m & -___m);						\
> -	___m /= (___m & -___m);						\
> +	___p /= ffs_val(___m);						\
> +	___m /= ffs_val(___m);						\
>  									\
>  	/*								\
>  	 * Perform (m_bias + m * n) / (1 << 64).			\
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 126dc5b380afa..26fbb6cfc0719 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -8,6 +8,7 @@
>  #define _LINUX_BITFIELD_H
>  
>  #include <linux/build_bug.h>
> +#include <linux/ffs_val.h>
>  #include <linux/typecheck.h>
>  #include <asm/byteorder.h>
>  
> @@ -202,7 +203,7 @@ static __always_inline u64 field_multiplier(u64 field)
>  {
>  	if ((field | (field - 1)) & ((field | (field - 1)) + 1))
>  		__bad_mask();
> -	return field & -field;
> +	return ffs_val(field);
>  }
>  static __always_inline u64 field_mask(u64 field)
>  {
> diff --git a/include/linux/log2.h b/include/linux/log2.h
> index 2eac3fc9303d6..7615e513885e9 100644
> --- a/include/linux/log2.h
> +++ b/include/linux/log2.h
> @@ -266,7 +266,7 @@ int __bits_per(unsigned long n)
>  static inline __attribute__((const))
>  unsigned int max_pow_of_two_factor(unsigned int n)
>  {
> -	return n & -n;
> +	return ffs_val(n);
>  }
>  
>  #endif /* _LINUX_LOG2_H */
> diff --git a/include/linux/min_heap.h b/include/linux/min_heap.h
> index 79ddc0adbf2bf..e914919470c88 100644
> --- a/include/linux/min_heap.h
> +++ b/include/linux/min_heap.h
> @@ -3,6 +3,7 @@
>  #define _LINUX_MIN_HEAP_H
>  
>  #include <linux/bug.h>
> +#include <linux/ffs_val.h>
>  #include <linux/string.h>
>  #include <linux/types.h>
>  
> @@ -257,7 +258,7 @@ static __always_inline
>  void __min_heap_sift_down_inline(min_heap_char *heap, size_t pos, size_t elem_size,
>  				 const struct min_heap_callbacks *func, void *args)
>  {
> -	const unsigned long lsbit = elem_size & -elem_size;
> +	const unsigned long lsbit = ffs_val(elem_size);
>  	void *data = heap->data;
>  	void (*swp)(void *lhs, void *rhs, void *args) = func->swp;
>  	/* pre-scale counters for performance */
> @@ -297,7 +298,7 @@ static __always_inline
>  void __min_heap_sift_up_inline(min_heap_char *heap, size_t elem_size, size_t idx,
>  			       const struct min_heap_callbacks *func, void *args)
>  {
> -	const unsigned long lsbit = elem_size & -elem_size;
> +	const unsigned long lsbit = ffs_val(elem_size);
>  	void *data = heap->data;
>  	void (*swp)(void *lhs, void *rhs, void *args) = func->swp;
>  	/* pre-scale counters for performance */
> diff --git a/lib/math/gcd.c b/lib/math/gcd.c
> index 62efca6787aef..3706c84e68bc1 100644
> --- a/lib/math/gcd.c
> +++ b/lib/math/gcd.c
> @@ -23,12 +23,12 @@ static unsigned long binary_gcd(unsigned long a, unsigned long b)
>  
>  	b >>= __ffs(b);
>  	if (b == 1)
> -		return r & -r;
> +		return ffs_val(r);
>  
>  	for (;;) {
>  		a >>= __ffs(a);
>  		if (a == 1)
> -			return r & -r;
> +			return ffs_val(r);
>  		if (a == b)
>  			return a << __ffs(r);
>  
> diff --git a/lib/sort.c b/lib/sort.c
> index 52363995ccc5c..2489aaaa21a06 100644
> --- a/lib/sort.c
> +++ b/lib/sort.c
> @@ -12,6 +12,7 @@
>  
>  #include <linux/types.h>
>  #include <linux/export.h>
> +#include <linux/ffs_val.h>
>  #include <linux/sort.h>
>  
>  /**
> @@ -196,7 +197,7 @@ static void __sort_r(void *base, size_t num, size_t size,
>  {
>  	/* pre-scale counters for performance */
>  	size_t n = num * size, a = (num/2) * size;
> -	const unsigned int lsbit = size & -size;  /* Used to find parent */
> +	const unsigned int lsbit = ffs_val(size);  /* Used to find parent */
>  	size_t shift = 0;
>  
>  	if (!a)		/* num < 2 || size == 0 */
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 5be9b8c919490..2b13998fdb0fe 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -8664,7 +8664,7 @@ static bool requested_adv_flags_are_valid(struct hci_dev *hdev, u32 adv_flags)
>  	supported_flags = get_supported_adv_flags(hdev);
>  	phy_flags = adv_flags & MGMT_ADV_FLAG_SEC_MASK;
>  	if (adv_flags & ~supported_flags ||
> -	    ((phy_flags && (phy_flags ^ (phy_flags & -phy_flags)))))
> +	    ((phy_flags && (phy_flags ^ ffs_val(phy_flags)))))
>  		return false;
>  
>  	return true;
> diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
> index 112fe46788b6f..f93f509cc3fbf 100644
> --- a/net/netfilter/nft_set_pipapo.c
> +++ b/net/netfilter/nft_set_pipapo.c
> @@ -370,7 +370,7 @@ int pipapo_refill(unsigned long *map, unsigned int len, unsigned int rules,
>  	for (k = 0; k < len; k++) {
>  		bitset = map[k];
>  		while (bitset) {
> -			unsigned long t = bitset & -bitset;
> +			unsigned long t = ffs_val(bitset);
>  			int r = __builtin_ctzl(bitset);
>  			int i = k * BITS_PER_LONG + r;
>  
> -- 
> 2.52.0

Re: [RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[Petr Tesarik]

On Fri, 9 Jan 2026 13:19:25 -0500
Yury Norov <ynorov@nvidia.com> wrote:

>[...]
> --
> 
> OK, let me stop here.
> 
> Can you please instead of mechanical rework check each case
> individually and find the best replacement to highlight the intention?
> 
> Can you also split this patch to a series according to subsystems, so
> that corresponding maintainers will have better access to their changes?

Well, then it makes more sense to start by identifying the intention in
every place and getting them fixed one by one. Whatever remains should
be converted to use the new macro.

In fact, I can do both in parallel, converting the places where the
result of ffs() is immediately used as a shift count.

Then no tree-wide change is necessary, as long as we can first get the
helper (under whichever name) into bitops.h. But it will be rejected
without an in-tree user. I somehow feel caught by Catch XXII.

Advice welcome. Non-ironically.

Petr T

Re: [RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[Yury Norov]

On Fri, Jan 09, 2026 at 08:32:50PM +0100, Petr Tesarik wrote:
> On Fri, 9 Jan 2026 13:19:25 -0500
> Yury Norov <ynorov@nvidia.com> wrote:
> 
> >[...]
> > --
> > 
> > OK, let me stop here.
> > 
> > Can you please instead of mechanical rework check each case
> > individually and find the best replacement to highlight the intention?
> > 
> > Can you also split this patch to a series according to subsystems, so
> > that corresponding maintainers will have better access to their changes?
> 
> Well, then it makes more sense to start by identifying the intention in
> every place and getting them fixed one by one. Whatever remains should
> be converted to use the new macro.
> 
> In fact, I can do both in parallel, converting the places where the
> result of ffs() is immediately used as a shift count.
> 
> Then no tree-wide change is necessary, as long as we can first get the
> helper (under whichever name) into bitops.h. But it will be rejected
> without an in-tree user. I somehow feel caught by Catch XXII.
> 
> Advice welcome. Non-ironically.

Build your series like this:

1. Switch all 'a & -a' chunks to the existing helpers where possible;
2. Introduce the new macro;
3. Convert the rest of the codebase to using the macro;

In cover-letter mention that patches from #1 may be taken individually
or together with the rest of the series at maintainers' discretion, and
#2 and 3 must be merged at once, for example with my branch.

Then depending on feedback, I'll take your series as is, or will take
only explicitly acked patches. In 2nd case, I will ask you to resend
non-acked part for better visibility, and then decide either take it
or drop.

Thanks,
Yury

Re: [RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[Maciej W. Rozycki]

On Fri, 9 Jan 2026, Yury Norov wrote:

> > diff --git a/arch/alpha/kernel/smp.c b/arch/alpha/kernel/smp.c
> > index ed06367ece574..2f3809c015b99 100644
> > --- a/arch/alpha/kernel/smp.c
> > +++ b/arch/alpha/kernel/smp.c
> > @@ -525,7 +525,7 @@ handle_ipi(struct pt_regs *regs)
> >  	  do {
> >  		unsigned long which;
> >  
> > -		which = ops & -ops;
> > +		which = ffs_val(ops);
> >  		ops &= ~which;
> >  		which = __ffs(which);
> 
> I guess, this should be:
> 
>         which = __ffs(ops);
>         ops &= ops - 1;

 I think it's the wrong operation order.

> > diff --git a/arch/mips/dec/ecc-berr.c b/arch/mips/dec/ecc-berr.c
> > index 1eb356fdd8323..8934b8b1cf375 100644
> > --- a/arch/mips/dec/ecc-berr.c
> > +++ b/arch/mips/dec/ecc-berr.c
> > @@ -153,7 +153,7 @@ static int dec_ecc_be_backend(struct pt_regs *regs, int is_fixup, int invoker)
> >  			/* Ack now, now we've rewritten (or not). */
> >  			dec_ecc_be_ack();
> >  
> > -			if (syn && syn == (syn & -syn)) {
> > +			if (syn && syn == ffs_val(syn)) {
> 
> It opencodes is_power_of_2().

 Correct and it also predates that helper's existence by ~4 years.  I'll 
be happy to see this converted as the intent will be obviously clearer.

  Maciej

Re: [RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[David Laight]

On Sat, 10 Jan 2026 10:36:07 +0000 (GMT)
"Maciej W. Rozycki" <macro@orcam.me.uk> wrote:

> On Fri, 9 Jan 2026, Yury Norov wrote:
> 
> > > diff --git a/arch/alpha/kernel/smp.c b/arch/alpha/kernel/smp.c
> > > index ed06367ece574..2f3809c015b99 100644
> > > --- a/arch/alpha/kernel/smp.c
> > > +++ b/arch/alpha/kernel/smp.c
> > > @@ -525,7 +525,7 @@ handle_ipi(struct pt_regs *regs)
> > >  	  do {
> > >  		unsigned long which;
> > >  
> > > -		which = ops & -ops;
> > > +		which = ffs_val(ops);
> > >  		ops &= ~which;
> > >  		which = __ffs(which);  
> > 
> > I guess, this should be:
> > 
> >         which = __ffs(ops);
> >         ops &= ops - 1;  
> 
>  I think it's the wrong operation order.

If you are going to do the __ffs() do it first.
		which = __ffs(ops);
		ops &= ~(1u << which);

OTOH what follows is:
		switch (which) {
		case IPI_RESCHEDULE:
so changing to 'case 1u << IPI_RESCHEDULE: would save the ffs().
But the entire loop is pointless, three tests, the first being:
	if (op & (1u << IPI_RESCHEDULE))
		scheduler_ipi();
would be far less code.

But probably pointless churn - no one cares about alpha any more :-)

> 
> > > diff --git a/arch/mips/dec/ecc-berr.c b/arch/mips/dec/ecc-berr.c
> > > index 1eb356fdd8323..8934b8b1cf375 100644
> > > --- a/arch/mips/dec/ecc-berr.c
> > > +++ b/arch/mips/dec/ecc-berr.c
> > > @@ -153,7 +153,7 @@ static int dec_ecc_be_backend(struct pt_regs *regs, int is_fixup, int invoker)
> > >  			/* Ack now, now we've rewritten (or not). */
> > >  			dec_ecc_be_ack();
> > >  
> > > -			if (syn && syn == (syn & -syn)) {
> > > +			if (syn && syn == ffs_val(syn)) {  
> > 
> > It opencodes is_power_of_2().

Badly...

	David

> 
>  Correct and it also predates that helper's existence by ~4 years.  I'll 
> be happy to see this converted as the intent will be obviously clearer.
> 
>   Maciej
> 

Re: [RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[Maciej W. Rozycki]

On Sat, 10 Jan 2026, David Laight wrote:

> > > > diff --git a/arch/mips/dec/ecc-berr.c b/arch/mips/dec/ecc-berr.c
> > > > index 1eb356fdd8323..8934b8b1cf375 100644
> > > > --- a/arch/mips/dec/ecc-berr.c
> > > > +++ b/arch/mips/dec/ecc-berr.c
> > > > @@ -153,7 +153,7 @@ static int dec_ecc_be_backend(struct pt_regs *regs, int is_fixup, int invoker)
> > > >  			/* Ack now, now we've rewritten (or not). */
> > > >  			dec_ecc_be_ack();
> > > >  
> > > > -			if (syn && syn == (syn & -syn)) {
> > > > +			if (syn && syn == ffs_val(syn)) {  
> > > 
> > > It opencodes is_power_of_2().
> 
> Badly...

 It's 4 MIPS instructions and exactly the same machine code produced as 
compiler output from `is_power_of_2'.  If you know of a better alternative 
I'll be happy to get enlightened, however it's academic anyway, as it's 
the ECC memory error handler.  If you get here, then you have a bigger 
issue than a couple of extra instructions executed per kernel log entry 
produced.  If you indeed can get rid of them in the first place, that is.

  Maciej

Re: [RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[David Laight]

On Sun, 11 Jan 2026 03:15:22 +0000 (GMT)
"Maciej W. Rozycki" <macro@orcam.me.uk> wrote:

> On Sat, 10 Jan 2026, David Laight wrote:
> 
> > > > > diff --git a/arch/mips/dec/ecc-berr.c b/arch/mips/dec/ecc-berr.c
> > > > > index 1eb356fdd8323..8934b8b1cf375 100644
> > > > > --- a/arch/mips/dec/ecc-berr.c
> > > > > +++ b/arch/mips/dec/ecc-berr.c
> > > > > @@ -153,7 +153,7 @@ static int dec_ecc_be_backend(struct pt_regs *regs, int is_fixup, int invoker)
> > > > >  			/* Ack now, now we've rewritten (or not). */
> > > > >  			dec_ecc_be_ack();
> > > > >  
> > > > > -			if (syn && syn == (syn & -syn)) {
> > > > > +			if (syn && syn == ffs_val(syn)) {    
> > > > 
> > > > It opencodes is_power_of_2().  
> > 
> > Badly...  
> 
>  It's 4 MIPS instructions and exactly the same machine code produced as 
> compiler output from `is_power_of_2'.

The compiler must be detecting that x == (x & -x) is the same as
(x & (x - 1)) == 0.
Although for MIPS the former is negate, and, beq(x,y) - so 3 instructions.
On x86 it is negate, and, cmp, beq(zero flag) - one extra instruction.
(The 4th MIPS instruction will be beq(syn,0).)

So maybe s/Badly/In an unusual way/

> If you know of a better alternative

Doing is_power_of_2() with only one conditional branch would be a gain.
But I've not thought about it hard enough to find one.
I suspect there may not be one, but all these 'tricks' come from the 1970s
(or earlier) and don't allow for the behaviour of modern cpu with multiple
execution units and long pipelines.
(Although MIPS is likely to be relatively sane.)

	David

> I'll be happy to get enlightened, however it's academic anyway, as it's 
> the ECC memory error handler.  If you get here, then you have a bigger 
> issue than a couple of extra instructions executed per kernel log entry 
> produced.  If you indeed can get rid of them in the first place, that is.
> 
>   Maciej
> 

Re: [RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[Maciej W. Rozycki]

On Sun, 11 Jan 2026, David Laight wrote:

> >  It's 4 MIPS instructions and exactly the same machine code produced as 
> > compiler output from `is_power_of_2'.
> 
> The compiler must be detecting that x == (x & -x) is the same as
> (x & (x - 1)) == 0.

 There are 3 basic operations either way, so unless a CPU has an odd 
instruction that combines any, there's no way to reduce the instruction 
count.

> Although for MIPS the former is negate, and, beq(x,y) - so 3 instructions.
> On x86 it is negate, and, cmp, beq(zero flag) - one extra instruction.
> (The 4th MIPS instruction will be beq(syn,0).)

 This code only ever runs on R2000/R3000 and R4000/R4400 processors, on 
which the cost of an untaken branch is nil.  The cost of a taken branch on 
the former ones is nil too, with their 4-stage pipelines, and while there 
is a fixed penalty on the latter ones, the compiler should be able to 
arrange code here such that at most one branch is taken anyway.

> So maybe s/Badly/In an unusual way/

 What's unusual in this way of determining that exactly one bit is set in 
a word?  It's as good as any I suppose.

> > If you know of a better alternative
> 
> Doing is_power_of_2() with only one conditional branch would be a gain.
> But I've not thought about it hard enough to find one.

 Sure you can, either of:

	(x != 0) & (x == (x & -x))

and:

	(x != 0) & ((x & (x - 1)) == 0)

will do, but it's 5 instructions on MIPS, so no gain here.

 If you have a population count instruction such as CTPOP on EV67 Alpha, 
you can do it in two instructions, such as:

	ctpop	$16, $0
	cmpeq	$0, 1, $0

so there seems to be room for improvement here, but GCC stubbornly refuses 
to produce this instruction sequence for this code:

bool is_power_of_2(unsigned long n)
{
	return __builtin_popcountl(n) == 1;
}

apparently owing to no insn cost set for the POPCOUNT RTX in the backend 
causing the middle end to pick up some default.  Instead GCC comes up with 
what can be coded in C as:

bool is_power_of_2(unsigned long n)
{
	return n - 1 < (n ^ (n - 1));
}

which seems an interesting alternative that produces 3 instructions only 
across Alpha, MIPS and RISC-V targets.  It's 5 instructions on POWER and 
x86-64 vs 8 and 9 respectively with our current implementation.  There are 
no branches produced here, although an inline expansion will likely end 
with one.

 Overall it seems a worthwhile improvement even if still an instruction 
longer than necessary for EV67 Alpha (and also for POWER, which also has 
a population count operation, and which GCC does pick up in this case).

 FWIW on VAX, notorious for the lack of conditional set operations, it's 7 
instructions and 1 branch, down from 14 and 2 respectively.

> I suspect there may not be one, but all these 'tricks' come from the 1970s
> (or earlier) and don't allow for the behaviour of modern cpu with multiple
> execution units and long pipelines.

 As we can see you never know.  I've sent a code update proposal.

 The use of the population count operation can be considered separately, 
but we'd have to be careful here as the quality of code produced by the 
intrinsic varies, e.g. with pre-EV67 Alpha, RISC-V and VAX (!) GCC emits 
code equivalent to my proposal, but with MIPS or x86-64 it resorts to a 
libcall, so a guarding condition would be required.  So I'd leave it for 
another occasion.

 Thanks for sparking this discussion overall!

  Maciej

Re: [RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[David Laight]

On Sun, 11 Jan 2026 21:22:03 +0000 (GMT)
"Maciej W. Rozycki" <macro@orcam.me.uk> wrote:

> On Sun, 11 Jan 2026, David Laight wrote:
> 
> > >  It's 4 MIPS instructions and exactly the same machine code produced as 
> > > compiler output from `is_power_of_2'.  
> > 
> > The compiler must be detecting that x == (x & -x) is the same as
> > (x & (x - 1)) == 0.  
> 
>  There are 3 basic operations either way, so unless a CPU has an odd 
> instruction that combines any, there's no way to reduce the instruction 
> count.
> 
> > Although for MIPS the former is negate, and, beq(x,y) - so 3 instructions.
> > On x86 it is negate, and, cmp, beq(zero flag) - one extra instruction.
> > (The 4th MIPS instruction will be beq(syn,0).)  
> 
>  This code only ever runs on R2000/R3000 and R4000/R4400 processors, on 
> which the cost of an untaken branch is nil.  The cost of a taken branch on 
> the former ones is nil too, with their 4-stage pipelines, and while there 
> is a fixed penalty on the latter ones, the compiler should be able to 
> arrange code here such that at most one branch is taken anyway.
> 
> > So maybe s/Badly/In an unusual way/  
> 
>  What's unusual in this way of determining that exactly one bit is set in 
> a word?  It's as good as any I suppose.
> 
> > > If you know of a better alternative  
> > 
> > Doing is_power_of_2() with only one conditional branch would be a gain.
> > But I've not thought about it hard enough to find one.  
> 
>  Sure you can, either of:
> 
> 	(x != 0) & (x == (x & -x))
> 
> and:
> 
> 	(x != 0) & ((x & (x - 1)) == 0)
> 
> will do, but it's 5 instructions on MIPS, so no gain here.
> 
>  If you have a population count instruction such as CTPOP on EV67 Alpha, 
> you can do it in two instructions, such as:
> 
> 	ctpop	$16, $0
> 	cmpeq	$0, 1, $0
> 
> so there seems to be room for improvement here, but GCC stubbornly refuses 
> to produce this instruction sequence for this code:
> 
> bool is_power_of_2(unsigned long n)
> {
> 	return __builtin_popcountl(n) == 1;
> }
> 
> apparently owing to no insn cost set for the POPCOUNT RTX in the backend 
> causing the middle end to pick up some default.  Instead GCC comes up with 
> what can be coded in C as:
> 
> bool is_power_of_2(unsigned long n)
> {
> 	return n - 1 < (n ^ (n - 1));
> }

Not seen that one - and not thought of popcnt, far too new for me.
(I learnt asm for a pdp11 first...)

> which seems an interesting alternative that produces 3 instructions only 
> across Alpha, MIPS and RISC-V targets.  It's 5 instructions on POWER and 
> x86-64 vs 8 and 9 respectively with our current implementation.  There are 
> no branches produced here, although an inline expansion will likely end 
> with one.

I'm seeing:
is_power_of_2:
        leaq    -1(%rdi), %rax
        xorq    %rax, %rdi
        cmpq    %rdi, %rax
        setb    %al
        retq
on x86-64 - which is really 3 instructions.

While using the popcnt instruction would reduce it by one, it will only
be faster on amd zen cpu, on intel cpu popcnt has a latency of 3 and only
executes on p1 (according to Agner).

> 
>  Overall it seems a worthwhile improvement even if still an instruction 
> longer than necessary for EV67 Alpha (and also for POWER, which also has 
> a population count operation, and which GCC does pick up in this case).
> 
>  FWIW on VAX, notorious for the lack of conditional set operations, it's 7 
> instructions and 1 branch, down from 14 and 2 respectively.

I worked for ICL through the 1980s so missed out on VAX.
Picked up 8085, x86, sparc32 and m68k.

> > I suspect there may not be one, but all these 'tricks' come from the 1970s
> > (or earlier) and don't allow for the behaviour of modern cpu with multiple
> > execution units and long pipelines.  
> 
>  As we can see you never know.  I've sent a code update proposal.

I'd suggest moving is_power_of_2() to bits.h as:
#define is_power_of_2(n) ({ auto _n = n; _n - 1 < _n ^ (_n - 1); })
and add:
#define is_zero_or_power_of_2(n) ({ auto _n = n; _n & (_n - 1) != 0; })

> 
>  The use of the population count operation can be considered separately, 
> but we'd have to be careful here as the quality of code produced by the 
> intrinsic varies, e.g. with pre-EV67 Alpha, RISC-V and VAX (!) GCC emits 
> code equivalent to my proposal, but with MIPS or x86-64 it resorts to a 
> libcall, so a guarding condition would be required.  So I'd leave it for 
> another occasion.

I'd guess that many of those popcnt instructions aren't actually as fast
as the dec/xor pair.
But the libcall is just plain stupid.
I think that happens elsewhere - which is one reason __ffs() is x86 asm.

> 
>  Thanks for sparking this discussion overall!

Nice wet Sunday bikeshed...

	David

> 
>   Maciej
> 

Re: [RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[Maciej W. Rozycki]

On Sun, 11 Jan 2026, David Laight wrote:

> >  If you have a population count instruction such as CTPOP on EV67 Alpha, 
> > you can do it in two instructions, such as:
> > 
> > 	ctpop	$16, $0
> > 	cmpeq	$0, 1, $0
> > 
> > so there seems to be room for improvement here, but GCC stubbornly refuses 
> > to produce this instruction sequence for this code:
> > 
> > bool is_power_of_2(unsigned long n)
> > {
> > 	return __builtin_popcountl(n) == 1;
> > }
> > 
> > apparently owing to no insn cost set for the POPCOUNT RTX in the backend 
> > causing the middle end to pick up some default.  Instead GCC comes up with 
> > what can be coded in C as:
> > 
> > bool is_power_of_2(unsigned long n)
> > {
> > 	return n - 1 < (n ^ (n - 1));
> > }
> 
> Not seen that one - and not thought of popcnt, far too new for me.
> (I learnt asm for a pdp11 first...)

 It does show up as an RTL operation in the compiler; it's a good way to 
discover stuff.

> > which seems an interesting alternative that produces 3 instructions only 
> > across Alpha, MIPS and RISC-V targets.  It's 5 instructions on POWER and 
> > x86-64 vs 8 and 9 respectively with our current implementation.  There are 
> > no branches produced here, although an inline expansion will likely end 
> > with one.
> 
> I'm seeing:
> is_power_of_2:
>         leaq    -1(%rdi), %rax
>         xorq    %rax, %rdi
>         cmpq    %rdi, %rax
>         setb    %al
>         retq
> on x86-64 - which is really 3 instructions.

 Ack, this is `unsigned long' vs `bool' return type.  I'm just too used to 
RISC psABIs.  FWIW I have this:

	leaq	-1(%rdi), %rax
	xorq	%rax, %rdi
	cmpq	%rdi, %rax
	setb	%al
	movzbl	%al, %eax
	ret

in the former case and I take it `bool' is 8-bit on x86.

> While using the popcnt instruction would reduce it by one, it will only
> be faster on amd zen cpu, on intel cpu popcnt has a latency of 3 and only
> executes on p1 (according to Agner).

 Ack.  I've lost track of x86 after ~Pentium Pro.

> > > I suspect there may not be one, but all these 'tricks' come from the 1970s
> > > (or earlier) and don't allow for the behaviour of modern cpu with multiple
> > > execution units and long pipelines.  
> > 
> >  As we can see you never know.  I've sent a code update proposal.
> 
> I'd suggest moving is_power_of_2() to bits.h as:
> #define is_power_of_2(n) ({ auto _n = n; _n - 1 < _n ^ (_n - 1); })
> and add:
> #define is_zero_or_power_of_2(n) ({ auto _n = n; _n & (_n - 1) != 0; })

 I'll leave it as an exercise for someone else.

> >  The use of the population count operation can be considered separately, 
> > but we'd have to be careful here as the quality of code produced by the 
> > intrinsic varies, e.g. with pre-EV67 Alpha, RISC-V and VAX (!) GCC emits 
> > code equivalent to my proposal, but with MIPS or x86-64 it resorts to a 
> > libcall, so a guarding condition would be required.  So I'd leave it for 
> > another occasion.
> 
> I'd guess that many of those popcnt instructions aren't actually as fast
> as the dec/xor pair.

 With EV67 Alpha and POWER being proper RISC architectures I'd expect the 
instruction to be implemented using a dedicated logic gate network.  After 
all the population count operation is just an n-operand addition of 1-bit 
inputs, so not a big deal.  The propagation delay is probably on the same 
order of magnitude as with a 2-operand n-bit adder, so the operation ought 
to fit in a single pipeline stage.

 I suppose it's just a matter of whether you want to dedicate a piece of
silicon just for this somewhat exotic operation, and in the old days the 
answer would be no for a general-purpose CPU owing to manufacturing cost 
and die size limitation, while nowadays with process shrinkage and power 
consumption reduction it may have become worthwhile.

> But the libcall is just plain stupid.

 It could just be that it's a recent addition as my compilation of GCC for 
x86-64 and MIPS turns out to be version 11 and 13 respectively, while I 
have version 15 for the remaining targets.  Indeed when I tried now GCC 14 
that I have at hand for another MIPS configuration the libcall is gone, so 
the optimisation must have been added in that version.

 Of course you still need a libcall for a plain `__builtin_popcountl' call 
where there's no suitable hardware instruction available.

  Maciej

Re: [RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[David Laight]

On Mon, 12 Jan 2026 11:21:59 +0000 (GMT)
"Maciej W. Rozycki" <macro@orcam.me.uk> wrote:

> On Sun, 11 Jan 2026, David Laight wrote:
> 
> > >  If you have a population count instruction such as CTPOP on EV67 Alpha, 
> > > you can do it in two instructions, such as:
> > > 
> > > 	ctpop	$16, $0
> > > 	cmpeq	$0, 1, $0
> > > 
> > > so there seems to be room for improvement here, but GCC stubbornly refuses 
> > > to produce this instruction sequence for this code:
> > > 
> > > bool is_power_of_2(unsigned long n)
> > > {
> > > 	return __builtin_popcountl(n) == 1;
> > > }
> > > 
> > > apparently owing to no insn cost set for the POPCOUNT RTX in the backend 
> > > causing the middle end to pick up some default.  Instead GCC comes up with 
> > > what can be coded in C as:
> > > 
> > > bool is_power_of_2(unsigned long n)
> > > {
> > > 	return n - 1 < (n ^ (n - 1));
> > > }  
> > 
> > Not seen that one - and not thought of popcnt, far too new for me.
> > (I learnt asm for a pdp11 first...)  
> 
>  It does show up as an RTL operation in the compiler; it's a good way to 
> discover stuff.
> 
> > > which seems an interesting alternative that produces 3 instructions only 
> > > across Alpha, MIPS and RISC-V targets.  It's 5 instructions on POWER and 
> > > x86-64 vs 8 and 9 respectively with our current implementation.  There are 
> > > no branches produced here, although an inline expansion will likely end 
> > > with one.  
> > 
> > I'm seeing:
> > is_power_of_2:
> >         leaq    -1(%rdi), %rax
> >         xorq    %rax, %rdi
> >         cmpq    %rdi, %rax
> >         setb    %al
> >         retq
> > on x86-64 - which is really 3 instructions.  
> 
>  Ack, this is `unsigned long' vs `bool' return type.  I'm just too used to 
> RISC psABIs.  FWIW I have this:
> 
> 	leaq	-1(%rdi), %rax
> 	xorq	%rax, %rdi
> 	cmpq	%rdi, %rax
> 	setb	%al
> 	movzbl	%al, %eax
> 	ret
> 
> in the former case and I take it `bool' is 8-bit on x86.

That was _Bool on godbolt, it you return 'int' the extra 'zero extend'
is needed. SETcc was added for 386 (I still forget about it) and someone
assumed you wouldn't want the 'word' variant.
(It isn't as though they couldn't have allocated another row of the 0F
opcode to it.)
I sort of hate 'bool' types, something sane has to happen for 'a & b'
when the bit patterns are unexpected - and it ought to be well defined.
The traditional C where 'a > b' generates 0 or 1 is fine.

> > While using the popcnt instruction would reduce it by one, it will only
> > be faster on amd zen cpu, on intel cpu popcnt has a latency of 3 and only
> > executes on p1 (according to Agner).  
> 
>  Ack.  I've lost track of x86 after ~Pentium Pro.

Those were the days :-)
I do remember something from one of the Intel manuals of that period about
not intermixing data with code because of the possibility of the cpu
speculatively executing the data following a branch and if the data happened
to be a floating point trig function it had to wait for it to finish.
SLS isn't a modern thing.

> > > > I suspect there may not be one, but all these 'tricks' come from the 1970s
> > > > (or earlier) and don't allow for the behaviour of modern cpu with multiple
> > > > execution units and long pipelines.    
> > > 
> > >  As we can see you never know.  I've sent a code update proposal.  
> > 
> > I'd suggest moving is_power_of_2() to bits.h as:
> > #define is_power_of_2(n) ({ auto _n = n; _n - 1 < _n ^ (_n - 1); })
> > and add:
> > #define is_zero_or_power_of_2(n) ({ auto _n = n; _n & (_n - 1) != 0; })  
> 
>  I'll leave it as an exercise for someone else.

I might add it to the patchset I'm writing - mostly bits.h

> > >  The use of the population count operation can be considered separately, 
> > > but we'd have to be careful here as the quality of code produced by the 
> > > intrinsic varies, e.g. with pre-EV67 Alpha, RISC-V and VAX (!) GCC emits 
> > > code equivalent to my proposal, but with MIPS or x86-64 it resorts to a 
> > > libcall, so a guarding condition would be required.  So I'd leave it for 
> > > another occasion.  
> > 
> > I'd guess that many of those popcnt instructions aren't actually as fast
> > as the dec/xor pair.  
> 
>  With EV67 Alpha and POWER being proper RISC architectures I'd expect the 
> instruction to be implemented using a dedicated logic gate network.  After 
> all the population count operation is just an n-operand addition of 1-bit 
> inputs, so not a big deal.  The propagation delay is probably on the same 
> order of magnitude as with a 2-operand n-bit adder, so the operation ought 
> to fit in a single pipeline stage.

There are some tricks that can be done to reduce the propagation delay
of a 64bit add from ~64 (for the ripple carry) to nearer 16 (add each byte
with and without a carry input and then use a mux to select the correct
output). That may not work for popcnt.

Also I suspect that multiply isn't single clock, so multicycle instructions
have to be supported.
Throw silicon at multiply and you can reduce it to full-adders with a carry-
chain the length of the result, not sure you can do much better.

For my sins I re-implemented the Nios-II cpu a couple of years ago (because
Intel had deprecated it), the single 64bit add needed at the end of multiply
(done by fgpa DSP blocks) made it impossible to feed the product straight
back into the ALU for the next cycle.
My version is about the same size, 1 clock faster in a few places (like
predicted taken branches and the stall if a multiply result (and I think
memory reads) is needed in the next-but-one instruction.
FMax is a bit lower, but we were constrained by the PCIe interface to 67.5MHz
and it was fast enough.

	David

> 
>  I suppose it's just a matter of whether you want to dedicate a piece of
> silicon just for this somewhat exotic operation, and in the old days the 
> answer would be no for a general-purpose CPU owing to manufacturing cost 
> and die size limitation, while nowadays with process shrinkage and power 
> consumption reduction it may have become worthwhile.
> 
> > But the libcall is just plain stupid.  
> 
>  It could just be that it's a recent addition as my compilation of GCC for 
> x86-64 and MIPS turns out to be version 11 and 13 respectively, while I 
> have version 15 for the remaining targets.  Indeed when I tried now GCC 14 
> that I have at hand for another MIPS configuration the libcall is gone, so 
> the optimisation must have been added in that version.
> 
>  Of course you still need a libcall for a plain `__builtin_popcountl' call 
> where there's no suitable hardware instruction available.
> 
>   Maciej
> 

Re: [RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[Kuan-Wei Chiu]

On Sat, Jan 10, 2026 at 10:36:07AM +0000, Maciej W. Rozycki wrote:
> On Fri, 9 Jan 2026, Yury Norov wrote:
> 
> > > diff --git a/arch/alpha/kernel/smp.c b/arch/alpha/kernel/smp.c
> > > index ed06367ece574..2f3809c015b99 100644
> > > --- a/arch/alpha/kernel/smp.c
> > > +++ b/arch/alpha/kernel/smp.c
> > > @@ -525,7 +525,7 @@ handle_ipi(struct pt_regs *regs)
> > >  	  do {
> > >  		unsigned long which;
> > >  
> > > -		which = ops & -ops;
> > > +		which = ffs_val(ops);
> > >  		ops &= ~which;
> > >  		which = __ffs(which);
> > 
> > I guess, this should be:
> > 
> >         which = __ffs(ops);
> >         ops &= ops - 1;
> 
>  I think it's the wrong operation order.
> 
> > > diff --git a/arch/mips/dec/ecc-berr.c b/arch/mips/dec/ecc-berr.c
> > > index 1eb356fdd8323..8934b8b1cf375 100644
> > > --- a/arch/mips/dec/ecc-berr.c
> > > +++ b/arch/mips/dec/ecc-berr.c
> > > @@ -153,7 +153,7 @@ static int dec_ecc_be_backend(struct pt_regs *regs, int is_fixup, int invoker)
> > >  			/* Ack now, now we've rewritten (or not). */
> > >  			dec_ecc_be_ack();
> > >  
> > > -			if (syn && syn == (syn & -syn)) {
> > > +			if (syn && syn == ffs_val(syn)) {
> > 
> > It opencodes is_power_of_2().
> 
>  Correct and it also predates that helper's existence by ~4 years.  I'll 
> be happy to see this converted as the intent will be obviously clearer.
> 
>   Maciej
> 

Hi all,

(Apologies for the off-topic question regarding email delivery)

I noticed Maciej's reply only after seeing David's subsequent response.
For some reason, this specific email from Maciej never reached my gmail
inbox, nor did it appear in my spam folder. It seems gmail may have
silently dropped it.

I have retrieved this raw message from the lore.kernel.org web
interface to reply here.

Since there are other gmail users on this thread (e.g., David, whom I
assume received it successfully given the reply), I am wondering if
anyone else using gmail experienced similar issues with this message?

If anyone has insights on why this might be happening or how to adjust
settings to prevent this, please let me know. I want to ensure I don't
inadvertently miss any comments or appear unresponsive in the future.

Regards,
Kuan-Wei

Re: [RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[David Laight]

On Sun, 11 Jan 2026 00:42:56 +0800
Kuan-Wei Chiu <visitorckw@gmail.com> wrote:

...
> Since there are other gmail users on this thread (e.g., David, whom I
> assume received it successfully given the reply), I am wondering if
> anyone else using gmail experienced similar issues with this message?

Me 'David' ?
I'm actually subscribed to the lists on a different email provider.
I used to use gmail, but it bounces, spams and just plain discards
far too many messages to reliably receive lkml and friends.
I post from the gmail address to avoid leaking the other address to spammers.

	David

Re: [RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[Kuan-Wei Chiu]

On Sat, Jan 10, 2026 at 10:23:02PM +0000, David Laight wrote:
> On Sun, 11 Jan 2026 00:42:56 +0800
> Kuan-Wei Chiu <visitorckw@gmail.com> wrote:
> 
> ...
> > Since there are other gmail users on this thread (e.g., David, whom I
> > assume received it successfully given the reply), I am wondering if
> > anyone else using gmail experienced similar issues with this message?
> 
> Me 'David' ?
> I'm actually subscribed to the lists on a different email provider.
> I used to use gmail, but it bounces, spams and just plain discards
> far too many messages to reliably receive lkml and friends.
> I post from the gmail address to avoid leaking the other address to spammers.
> 
Ah, I misunderstood. I thought you were receiving on gmail as well.

I actually just missed another reply from Maciej (again, nothing in
spam). It appears gmail has decided to silently drop messages from him
for some reason.

I'm a bit surprised to hear gmail described as generally unsuitable. I
have used this account for Linux and several other open source projects
for the past three years without issues until now.

Also, a quick check shows:

$ grep "gmail.com" MAINTAINERS | wc -l
534

Given that over 500 maintainers and reviewers use gmail, it implies it
is widely relied upon. I even recall Linus mentioning issues with gmail
spam filters regarding PRs in the past t (though I am not sure of the
exact association between linux-foundation.org and gmail), suggesting
that even at that level, gmail is part of the workflow.

TBH, if these emails were simply going to spam, it wouldn't be an issue
as I check that folder regularly. The problem here is that Maciej's
emails aren't even in spam.

That said, if direct emails continue to vanish like this, I may indeed
have to start looking for an alternative. (sigh...)

Regards,
Kuan-Wei

Re: [RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[Maciej W. Rozycki]

On Sun, 11 Jan 2026, Kuan-Wei Chiu wrote:

> > > Since there are other gmail users on this thread (e.g., David, whom I
> > > assume received it successfully given the reply), I am wondering if
> > > anyone else using gmail experienced similar issues with this message?
> > 
> > Me 'David' ?
> > I'm actually subscribed to the lists on a different email provider.
> > I used to use gmail, but it bounces, spams and just plain discards
> > far too many messages to reliably receive lkml and friends.
> > I post from the gmail address to avoid leaking the other address to spammers.
> > 
> Ah, I misunderstood. I thought you were receiving on gmail as well.
> 
> I actually just missed another reply from Maciej (again, nothing in
> spam). It appears gmail has decided to silently drop messages from him
> for some reason.

 Not at all silent, GMail rejects any e-mail sent here and I get a bounce 
every time.  It has been like this for a while now.  I'm not going to do 
anything about it, my SMTP service complies with the relevant RFC AFAIK 
and GMail is the only recipient showing any issues.

 FWIW I chose to host my e-mail myself for the very reason to avoid any
odd limitations imposed by professional hosting services and have the 
choice to use my own tools to process e-mail.  I have no direct influence 
on other people's choices.  I'll appreciate your understanding.

  Maciej

Re: [RFC PATCH 0/2] Helper to isolate least-significant bit

[Kuan-Wei Chiu]

Hi Petr,

On Fri, Jan 09, 2026 at 05:41:34PM +0100, Petr Tesarik wrote:
> Isolation of the least significant bit can be achieved with 3 basic
> ALU operations which are already open-coded in various places in the
> kernel.
> 
> However, since other places less efficient constructs, for example
> `1UL << ffs(x)`, I assume the trick is known only to some authors, and
> it's worth adding a helper to promote its use.

Just out of curiosity, are there any existing users employing 1 <<
ffs(x) (or other inefficient variants) in performance-critical
hotpaths?

From a quick grep, I only found one instance in drivers/clk/ti/mux.c
matching the 1 << ffs(x) pattern. However, this doesn't appear to be a
bottleneck since it is followed by ti_clk_ll_ops->clk_writel(...). The
latency of the MMIO write would likely overshadow the savings of a few
ALU cycles.

Additionally, it seems that patch #2 focuses on replacing the x & -x
implementation with the new API, rather than converting inefficient
constructs like 1 << ffs(x) to use ffs_val().

Regards,
Kuan-Wei

> 
> Petr Tesarik (2):
>   bits: introduce ffs_val()
>   treewide, bits: use ffs_val() where it is open-coded
> 
>  arch/alpha/include/asm/bitops.h              |  2 +-
>  arch/alpha/kernel/core_cia.c                 |  2 +-
>  arch/alpha/kernel/smp.c                      |  2 +-
>  arch/arc/include/asm/bitops.h                |  2 +-
>  arch/m68k/include/asm/bitops.h               | 12 +++++------
>  arch/mips/dec/ecc-berr.c                     |  2 +-
>  arch/mips/include/asm/bitops.h               |  4 ++--
>  arch/mips/pci/pci-malta.c                    |  4 ++--
>  arch/powerpc/include/asm/bitops.h            |  4 ++--
>  arch/powerpc/kvm/e500_mmu_host.c             |  2 +-
>  arch/powerpc/lib/sstep.c                     |  2 +-
>  arch/powerpc/xmon/ppc-dis.c                  |  3 ++-
>  arch/powerpc/xmon/ppc-opc.c                  |  6 +++---
>  arch/s390/include/asm/bitops.h               |  2 +-
>  arch/xtensa/include/asm/bitops.h             |  6 +++---
>  arch/xtensa/kernel/traps.c                   |  2 +-
>  drivers/gpu/drm/gma500/psb_intel_sdvo.c      |  2 +-
>  drivers/iommu/dma-iommu.c                    |  2 +-
>  drivers/net/ethernet/netronome/nfp/bpf/jit.c |  2 +-
>  drivers/net/usb/cdc_ncm.c                    |  4 ++--
>  include/asm-generic/div64.h                  |  4 ++--
>  include/linux/bitfield.h                     |  3 ++-
>  include/linux/bitops.h                       |  1 +
>  include/linux/ffs_val.h                      | 21 ++++++++++++++++++++
>  include/linux/log2.h                         |  2 +-
>  include/linux/min_heap.h                     |  5 +++--
>  lib/math/gcd.c                               |  4 ++--
>  lib/sort.c                                   |  3 ++-
>  net/bluetooth/mgmt.c                         |  2 +-
>  net/netfilter/nft_set_pipapo.c               |  2 +-
>  30 files changed, 70 insertions(+), 44 deletions(-)
>  create mode 100644 include/linux/ffs_val.h
> 
> -- 
> 2.52.0
> 

Re: [RFC PATCH 0/2] Helper to isolate least-significant bit

[Petr Tesarik]

On Sat, 10 Jan 2026 01:20:59 +0800
Kuan-Wei Chiu <visitorckw@gmail.com> wrote:

> Hi Petr,
> 
> On Fri, Jan 09, 2026 at 05:41:34PM +0100, Petr Tesarik wrote:
> > Isolation of the least significant bit can be achieved with 3 basic
> > ALU operations which are already open-coded in various places in the
> > kernel.
> > 
> > However, since other places less efficient constructs, for example
> > `1UL << ffs(x)`, I assume the trick is known only to some authors, and
> > it's worth adding a helper to promote its use.  
> 
> Just out of curiosity, are there any existing users employing 1 <<
> ffs(x) (or other inefficient variants) in performance-critical
> hotpaths?
>
> From a quick grep, I only found one instance in drivers/clk/ti/mux.c
> matching the 1 << ffs(x) pattern. However, this doesn't appear to be a
> bottleneck since it is followed by ti_clk_ll_ops->clk_writel(...). The
> latency of the MMIO write would likely overshadow the savings of a few
> ALU cycles.

Most expressions are a bit more complex, like this one in
page_cache_async_ra():

  align = 1UL << min(ra->order, ffs(max_pages) - 1);

Or split across multiple lines, like this one in sata_down_spd_limit():

  bit = ffs(mask) - 1;
  mask = 1 << bit;

I agree that there is most likely no measurable performance win. The
resulting machine code merely looks quite silly on architectures
without an instruction to do ffs() and a little bit silly on
architectures where the instruction has a slightly different semantics
(bit position numbering and/or handling of zero value).

> Additionally, it seems that patch #2 focuses on replacing the x & -x
> implementation with the new API, rather than converting inefficient
> constructs like 1 << ffs(x) to use ffs_val().

This is correct. This patch series merely introduces the helper. Patch
2/2 was created mechanically. I haven't sent any follow-up yet to
replace inefficient use, because that would require more effort, and I'm
trying to get some early feedback first (that's why the series is
tagged RFC).

If we agree that it's worth the effort, and I split the patches by
subsystem, it may become a rather long series.

Petr T

Re: [RFC PATCH 0/2] Helper to isolate least-significant bit

[Andrew Cooper]

> diff --git a/arch/alpha/kernel/core_cia.c b/arch/alpha/kernel/core_cia.c
> index 6e577228e175d..b3c273c13c104 100644
> --- a/arch/alpha/kernel/core_cia.c
> +++ b/arch/alpha/kernel/core_cia.c
> @@ -1035,7 +1035,7 @@ cia_decode_ecc_error(struct el_CIA_sysdata_mcheck *cia, const char *msg)
>      cia_decode_mem_error(cia, msg);
>  
>      syn = cia->cia_syn & 0xff;
> -    if (syn == (syn & -syn)) {
> +    if (syn == ffs_val(syn)) {
>          fmt = KERN_CRIT "  ECC syndrome %#x -- check bit %d\n";
>          i = ffs(syn) - 1;
>      } else {

This is a check for multiple bits set, which has a simpler expression to
calculate.  (the ffs() in context shows that syn isn't expected to be 0.)

In Xen I added this helper while tiding things up, and it's applicable
to the more common pattern of checking hweight() of a bitmap against 1.

/*
 * Calculate if a value has two or more bits set.  Always use this in
 * preference to an expression of the form 'hweight(x) > 1'.
 */
#define multiple_bits_set(x)                    \
    ({                                          \
        typeof(x) _v = (x);                     \
        (_v & (_v - 1)) != 0;                   \
    })


~Andrew

[RFC PATCH 0/2] Helper to isolate least-significant bit

[Petr Tesarik]

Isolation of the least significant bit can be achieved with 3 basic
ALU operations which are already open-coded in various places in the
kernel.

However, since other places less efficient constructs, for example
`1UL << ffs(x)`, I assume the trick is known only to some authors, and
it's worth adding a helper to promote its use.

Petr Tesarik (2):
  bits: introduce ffs_val()
  treewide, bits: use ffs_val() where it is open-coded

 arch/alpha/include/asm/bitops.h              |  2 +-
 arch/alpha/kernel/core_cia.c                 |  2 +-
 arch/alpha/kernel/smp.c                      |  2 +-
 arch/arc/include/asm/bitops.h                |  2 +-
 arch/m68k/include/asm/bitops.h               | 12 +++++------
 arch/mips/dec/ecc-berr.c                     |  2 +-
 arch/mips/include/asm/bitops.h               |  4 ++--
 arch/mips/pci/pci-malta.c                    |  4 ++--
 arch/powerpc/include/asm/bitops.h            |  4 ++--
 arch/powerpc/kvm/e500_mmu_host.c             |  2 +-
 arch/powerpc/lib/sstep.c                     |  2 +-
 arch/powerpc/xmon/ppc-dis.c                  |  3 ++-
 arch/powerpc/xmon/ppc-opc.c                  |  6 +++---
 arch/s390/include/asm/bitops.h               |  2 +-
 arch/xtensa/include/asm/bitops.h             |  6 +++---
 arch/xtensa/kernel/traps.c                   |  2 +-
 drivers/gpu/drm/gma500/psb_intel_sdvo.c      |  2 +-
 drivers/iommu/dma-iommu.c                    |  2 +-
 drivers/net/ethernet/netronome/nfp/bpf/jit.c |  2 +-
 drivers/net/usb/cdc_ncm.c                    |  4 ++--
 include/asm-generic/div64.h                  |  4 ++--
 include/linux/bitfield.h                     |  3 ++-
 include/linux/bitops.h                       |  1 +
 include/linux/ffs_val.h                      | 21 ++++++++++++++++++++
 include/linux/log2.h                         |  2 +-
 include/linux/min_heap.h                     |  5 +++--
 lib/math/gcd.c                               |  4 ++--
 lib/sort.c                                   |  3 ++-
 net/bluetooth/mgmt.c                         |  2 +-
 net/netfilter/nft_set_pipapo.c               |  2 +-
 30 files changed, 70 insertions(+), 44 deletions(-)
 create mode 100644 include/linux/ffs_val.h

-- 
2.52.0

Re: [RFC PATCH 0/2] Helper to isolate least-significant bit

[Geert Uytterhoeven]

Hi Petr,

On Fri, 9 Jan 2026 at 17:38, Petr Tesarik <ptesarik@suse.com> wrote:
> Isolation of the least significant bit can be achieved with 3 basic

least significant _set_ bit

> ALU operations which are already open-coded in various places in the
> kernel.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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