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

[Petr Tesarik <ptesarik@suse.com>]

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