On Sat, Mar 08, 2025 at 01:48:49AM +0900, Vincent Mailhol via B4 Relay wrote: > From: Yury Norov <yury.norov@xxxxxxxxx> > > Add GENMASK_TYPE() which generalizes __GENMASK() to support different > types, and implement fixed-types versions of GENMASK() based on it. > The fixed-type version allows more strict checks to the min/max values > accepted, which is useful for defining registers like implemented by > i915 and xe drivers with their REG_GENMASK*() macros. > > The strict checks rely on shift-count-overflow compiler check to fail > the build if a number outside of the range allowed is passed. > Example: > > #define FOO_MASK GENMASK_U32(33, 4) > > will generate a warning like: > > include/linux/bits.h:51:27: error: right shift count >= width of type [-Werror=shift-count-overflow] > 51 | type_max(t) >> (BITS_PER_TYPE(t) - 1 - (h))))) > | ^~ > > Signed-off-by: Yury Norov <yury.norov@xxxxxxxxx> > Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx> > Acked-by: Jani Nikula <jani.nikula@xxxxxxxxx> > Co-developed-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > --- > Changelog: > > v5 -> v6: > > - No changes. > > v4 -> v5: > > - Rename GENMASK_t() to GENMASK_TYPE(). > > - Fix typo in patch description. > > - Use tab indentations instead of single space to separate the > macro name from its body. > > - s/__GENMASK_U*()/GENMASK_U*()/g in the comment. > > - Add a tag to credit myself as Co-developer. Keep Yury as the > main author. > > - Modify GENMASK_TYPE() to match the changes made to __GENMASK() > in: https://github.com/norov/linux/commit/1e7933a575ed > > - Replace (t)~_ULL(0) with type_max(t). This is OK because > GENMASK_TYPE() is not available in asm. > > - linux/const.h and asm/bitsperlong.h are not used anymore. Remove > them. > > - Apply GENMASK_TYPE() to GENMASK_U128(). > > - Remove the unsigned int cast for the U8 and U16 variants. Cast > to the target type instead. Do that cast directly in > GENMASK_TYPE(). > > v3 -> v4: > > - The v3 is one year old. Meanwhile people started using > __GENMASK() directly. So instead of generalizing __GENMASK() to > support different types, add a new GENMASK_t(). > > - replace ~0ULL by ~_ULL(0). Otherwise, GENMASK_t() would fail in > asm code. > > - Make GENMASK_U8() and GENMASK_U16() return an unsigned int. In > v3, due to the integer promotion rules, these were returning a > signed integer. By casting these to unsigned int, at least the > signedness is kept. > --- > include/linux/bitops.h | 1 - > include/linux/bits.h | 55 ++++++++++++++++++++++++++++++++------------------ > 2 files changed, 35 insertions(+), 21 deletions(-) > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > index c1cb53cf2f0f8662ed3e324578f74330e63f935d..9be2d50da09a417966b3d11c84092bb2f4cd0bef 100644 > --- a/include/linux/bitops.h > +++ b/include/linux/bitops.h > @@ -8,7 +8,6 @@ > > #include <uapi/linux/kernel.h> > > -#define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE) > #define BITS_TO_LONGS(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(long)) > #define BITS_TO_U64(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u64)) > #define BITS_TO_U32(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32)) > diff --git a/include/linux/bits.h b/include/linux/bits.h > index 9c1c7ce0bba6bb09490d891904c143a5394fd512..b690611c769be61ab2b5ced43c8302ba5693308b 100644 > --- a/include/linux/bits.h > +++ b/include/linux/bits.h > @@ -2,16 +2,15 @@ > #ifndef __LINUX_BITS_H > #define __LINUX_BITS_H > > -#include <linux/const.h> > #include <vdso/bits.h> > #include <uapi/linux/bits.h> > -#include <asm/bitsperlong.h> > > #define BIT_MASK(nr) (UL(1) << ((nr) % BITS_PER_LONG)) > #define BIT_WORD(nr) ((nr) / BITS_PER_LONG) > #define BIT_ULL_MASK(nr) (ULL(1) << ((nr) % BITS_PER_LONG_LONG)) > #define BIT_ULL_WORD(nr) ((nr) / BITS_PER_LONG_LONG) > #define BITS_PER_BYTE 8 > +#define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE) > > /* > * Create a contiguous bitmask starting at bit position @l and ending at > @@ -20,28 +19,44 @@ > */ > #if !defined(__ASSEMBLY__) > > -#include <linux/build_bug.h> > -#include <linux/compiler.h> > - > -#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h))) > - > -#define GENMASK(h, l) \ > - (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l)) > -#define GENMASK_ULL(h, l) \ > - (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l)) > - > /* > * Missing asm support > * > - * __GENMASK_U128() depends on _BIT128() which would not work > - * in the asm code, as it shifts an 'unsigned __int128' data > - * type instead of direct representation of 128 bit constants > - * such as long and unsigned long. The fundamental problem is > - * that a 128 bit constant will get silently truncated by the > - * gcc compiler. > + * GENMASK_U*() depends on BITS_PER_TYPE() which relies on sizeof(), > + * something not available in asm. Nethertheless, fixed width integers > + * is a C concept. Assembly code can rely on the long and long long > + * versions instead. > */ > -#define GENMASK_U128(h, l) \ > - (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l)) > + > +#include <linux/build_bug.h> > +#include <linux/compiler.h> > +#include <linux/overflow.h> > + > +#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h))) > + > +/* > + * Generate a mask for the specified type @t. Additional checks are made to > + * guarantee the value returned fits in that type, relying on > + * shift-count-overflow compiler check to detect incompatible arguments. > + * For example, all these create build errors or warnings: > + * > + * - GENMASK(15, 20): wrong argument order > + * - GENMASK(72, 15): doesn't fit unsigned long > + * - GENMASK_U32(33, 15): doesn't fit in a u32 > + */ > +#define GENMASK_TYPE(t, h, l) \ > + ((t)(GENMASK_INPUT_CHECK(h, l) + \ > + (type_max(t) << (l) & \ > + type_max(t) >> (BITS_PER_TYPE(t) - 1 - (h))))) > + > +#define GENMASK(h, l) GENMASK_TYPE(unsigned long, h, l) > +#define GENMASK_ULL(h, l) GENMASK_TYPE(unsigned long long, h, l) I like everything except this part. We switch GENMASK() from a well tested implementation, including an asm code, and we split uapi and non-uapi users, with no functionality changes. Unification is a solid point, however. Let's make it a 2-step procedure? Adding fixed-width GENMASKs is a non-questionable improvement. Switching an existing API from one implementation to another should be a separate patch, and probably even a separate series. And we should be very clear that __GENMASK() is uapi-only thing from now. If we decide to switch GENMASK() in a separate series, we'll have some extra time to think about unification... > +#define GENMASK_U8(h, l) GENMASK_TYPE(u8, h, l) > +#define GENMASK_U16(h, l) GENMASK_TYPE(u16, h, l) > +#define GENMASK_U32(h, l) GENMASK_TYPE(u32, h, l) > +#define GENMASK_U64(h, l) GENMASK_TYPE(u64, h, l) > +#define GENMASK_U128(h, l) GENMASK_TYPE(u128, h, l) > > #else /* defined(__ASSEMBLY__) */ > > > -- > 2.45.3 >
