Message-ID: <20260109184614.7b3b9bb3@mordecai>
Date: Fri, 9 Jan 2026 18:46:14 +0100
From: Petr Tesarik <ptesarik@...e.com>
To: Yury Norov <ynorov@...dia.com>
Cc: Yury Norov <yury.norov@...il.com>, Rasmus Villemoes
 <linux@...musvillemoes.dk>, Richard Henderson
 <richard.henderson@...aro.org>, Matt Turner <mattst88@...il.com>, Magnus
 Lindholm <linmag7@...il.com>, Vineet Gupta <vgupta@...nel.org>, Geert
 Uytterhoeven <geert@...ux-m68k.org>, "Maciej W. Rozycki"
 <macro@...am.me.uk>, Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
 Madhavan Srinivasan <maddy@...ux.ibm.com>, Michael Ellerman
 <mpe@...erman.id.au>, Heiko Carstens <hca@...ux.ibm.com>, Vasily Gorbik
 <gor@...ux.ibm.com>, Alexander Gordeev <agordeev@...ux.ibm.com>, Chris
 Zankel <chris@...kel.net>, Max Filippov <jcmvbkbc@...il.com>, Patrik
 Jakobsson <patrik.r.jakobsson@...il.com>, Maarten Lankhorst
 <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
 Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>, Robin Murphy <robin.murphy@....com>, Joerg
 Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>, Jakub Kicinski
 <kuba@...nel.org>, Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
 <pabeni@...hat.com>, Oliver Neukum <oliver@...kum.org>, Arnd Bergmann
 <arnd@...db.de>, Kuan-Wei Chiu <visitorckw@...il.com>, Andrew Morton
 <akpm@...ux-foundation.org>, Marcel Holtmann <marcel@...tmann.org>, Johan
 Hedberg <johan.hedberg@...il.com>, Luiz Augusto von Dentz
 <luiz.dentz@...il.com>, Pablo Neira Ayuso <pablo@...filter.org>, Florian
 Westphal <fw@...len.de>, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/2] bits: introduce ffs_val()

On Fri, 9 Jan 2026 12:16:06 -0500
Yury Norov <ynorov@...dia.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@...e.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

Powered by blists - more mailing lists