linux-kernel - Re: [RFC PATCH 1/2] bits: introduce ffs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aWFIVrH41EPgVmbw@yury>
Date: Fri, 9 Jan 2026 13:26:30 -0500
From: Yury Norov <ynorov@...dia.com>
To: Petr Tesarik <ptesarik@...e.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()

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