From: Yury Norov <yury.norov@gmail.com>
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@gmail.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
Co-developed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
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)
+
+#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.3On Sat, Mar 08, 2025 at 01:48:49AM +0900, Vincent Mailhol via B4 Relay wrote: > From: Yury Norov <yury.norov@gmail.com> > > 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@gmail.com> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > Acked-by: Jani Nikula <jani.nikula@intel.com> > Co-developed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > --- > 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 >
On 19/03/2025 at 01:45, Yury Norov wrote:
> On Sat, Mar 08, 2025 at 01:48:49AM +0900, Vincent Mailhol via B4 Relay wrote:
>> From: Yury Norov <yury.norov@gmail.com>
(...)
>> +#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...
Ack. I started drafting the split. The two series would look like:
[Series #1] bits: Fixed-type GENMASK_U*() and BIT_U*()
- bits: introduce fixed-type GENMASK_U*()
- bits: introduce fixed-type BIT_U*()
- drm/i915: Convert REG_GENMASK*() to fixed-width GENMASK_U*()
- test_bits: add tests for GENMASK_U*()
- test_bits: add tests for BIT_U*()
[Series #2] bits: Split asm and non-asm GENMASK*() and unify definitions
- bits: split the definition of the asm and non-asm GENMASK*()
- bits: unify the non-asm GENMASK*()
- test_bits: add tests for __GENMASK() and __GENMASK_ULL()
Series #1 will leave GENMASK(), GENMASK_ULL() and GENMASK_128()
untouched. The final result after the Series #2 will be the exact same
code as of now.
I am thinking of sending the two series at the same time, and then, you
can decide what is the good timing to merge these (and eventually, start
a separate discussion on the second series).
Does this work for you?
On a side note, it did a lot of modifications to your original patch
which introduced the GENMASK_U*(). It is OK to tag myself as author and
you as co-author or do you still prefer to stay as the main author? Let
me know!
Yours sincerely,
Vincent Mailhol
On Wed, Mar 19, 2025 at 02:39:03PM +0900, Vincent Mailhol wrote: > On 19/03/2025 at 01:45, Yury Norov wrote: > > On Sat, Mar 08, 2025 at 01:48:49AM +0900, Vincent Mailhol via B4 Relay wrote: > >> From: Yury Norov <yury.norov@gmail.com> > > (...) > > >> +#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... > > Ack. I started drafting the split. The two series would look like: > > [Series #1] bits: Fixed-type GENMASK_U*() and BIT_U*() > - bits: introduce fixed-type GENMASK_U*() > - bits: introduce fixed-type BIT_U*() > - drm/i915: Convert REG_GENMASK*() to fixed-width GENMASK_U*() > - test_bits: add tests for GENMASK_U*() > - test_bits: add tests for BIT_U*() > > [Series #2] bits: Split asm and non-asm GENMASK*() and unify definitions > - bits: split the definition of the asm and non-asm GENMASK*() > - bits: unify the non-asm GENMASK*() > - test_bits: add tests for __GENMASK() and __GENMASK_ULL() > > > Series #1 will leave GENMASK(), GENMASK_ULL() and GENMASK_128() > untouched. The final result after the Series #2 will be the exact same > code as of now. > > I am thinking of sending the two series at the same time, and then, you > can decide what is the good timing to merge these (and eventually, start > a separate discussion on the second series). > > Does this work for you? Yes. > On a side note, it did a lot of modifications to your original patch > which introduced the GENMASK_U*(). It is OK to tag myself as author and > you as co-author or do you still prefer to stay as the main author? Let > me know! Yes, I'm OK.
On Sat, Mar 08, 2025 at 01:48:49AM +0900, Vincent Mailhol via B4 Relay wrote: > From: Yury Norov <yury.norov@gmail.com> > > 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))))) > | ^~ ... > /* > * Missing asm support > * > + * GENMASK_U*() depends on BITS_PER_TYPE() which relies on sizeof(), s/depends/depend/ (we are already referring to a plural) > + * 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. > */ -- With Best Regards, Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.