Re: [PATCH treewide v3 2/4] bitfield: Add non-constant field_{prep,get}() helpers
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: 2025-10-17 16:00:58
Also in:
linux-arm-kernel, linux-clk, linux-crypto, linux-gpio, linux-iio, linux-renesas-soc, linux-sound, lkml
Hi Jakub, On Fri, 17 Oct 2025 at 17:19, Jakub Kicinski [off-list ref] wrote:
On Fri, 14 Feb 2025 14:55:51 +0100 Geert Uytterhoeven wrote:quoted
The existing FIELD_{GET,PREP}() macros are limited to compile-time constants. However, it is very common to prepare or extract bitfield elements where the bitfield mask is not a compile-time constant. To avoid this limitation, the AT91 clock driver and several other drivers already have their own non-const field_{prep,get}() macros. Make them available for general use by consolidating them in <linux/bitfield.h>, and improve them slightly: 1. Avoid evaluating macro parameters more than once, 2. Replace "ffs() - 1" by "__ffs()", 3. Support 64-bit use on 32-bit architectures. This is deliberately not merged into the existing FIELD_{GET,PREP}() macros, as people expressed the desire to keep stricter variants for increased safety, or for performance critical paths.We already have helpers for this, please just don't know they exist :/ The "const" version of the helpers are specifically defined to work on masks generated with BIT() and GENMASK(). If the mask is not constant we should expect it to have a well defined width. I strongly prefer that we do this instead and convert the users to the fixed-width version: ---->8---------------- Subject: bitfield: open code the fixed-width non-const helpers so that people see them There is a number of useful helpers defined in bitfield.h but they are mostly invisible to the reader because they are all generated by macros. Open code the 32b versions (which are most commonly used) to give developers a chance to discover them. Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Thanks, but this is more or less the same code which you suggested before [1], and to which I just replied[2] after looking at the generated assembler output on various architectures.
quoted hunk ↗ jump to hunk
@@ -188,6 +193,81 @@ static __always_inline u64 field_mask(u64 field) return field / field_multiplier(field); } #define field_max(field) ((typeof(field))field_mask(field)) + +/** + * u32_encode_bits() - prepare a u32 bitfield element (non-const) + * @v: value to put in the field + * @field: shifted mask defining the field's length and position + * + * Equivalent of FIELD_PREP() for u32, field does not have to be constant. + * + * Note that the helper is available for other field widths (generated below). + */ +static __always_inline __u32 u32_encode_bits(u32 v, u32 field) +{ + if (__builtin_constant_p(v) && (v & ~field_mask(field))) + __field_overflow(); + return ((v & field_mask(field)) * field_multiplier(field));
Unfortunately gcc emits actual divisions or __*div*() calls, and multiplications in the non-constant case. So I don't think this is suitable as-is.
+}
[1] https://lore.kernel.org/all/20250214073402.0129e259@kernel.org (local) [2] https://lore.kernel.org/all/CAMuHMdU+0HGG22FbO3wNmXtbUm9RhTopYrGghF6UrkFu-iww2A@mail.gmail.com (local) 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