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

[Yury Norov <ynorov@nvidia.com>]

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