* [PATCH v2 1/3] bits: split the definition of the asm and non-asm GENMASK*()
2025-06-09 2:45 [PATCH v2 0/3] bits: Split asm and non-asm GENMASK*() and unify definitions Vincent Mailhol via B4 Relay
@ 2025-06-09 2:45 ` Vincent Mailhol via B4 Relay
2025-06-09 2:45 ` [PATCH v2 2/3] bits: unify the " Vincent Mailhol via B4 Relay
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Vincent Mailhol via B4 Relay @ 2025-06-09 2:45 UTC (permalink / raw)
To: Yury Norov, Lucas De Marchi, Rasmus Villemoes, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, David Airlie,
Simona Vetter, Andrew Morton
Cc: linux-kernel, intel-gfx, dri-devel, Andi Shyti, David Laight,
Andy Shevchenko, Catalin Marinas, Anshuman Khandual,
linux-arm-kernel, Vincent Mailhol
From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
In an upcoming change, the non-asm GENMASK*() will all be unified to
depend on GENMASK_TYPE() which indirectly depend on sizeof(), something
not available in asm.
Instead of adding further complexity to GENMASK_TYPE() to make it work
for both asm and non asm, just split the definition of the two variants.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
Changelog:
v1 -> v2:
- No changes
This patch previously belonged to this series:
bits: Fixed-type GENMASK_U*() and BIT_U*()
Link: https://lore.kernel.org/r/20250308-fixed-type-genmasks-v6-0-f59315e73c29@wanadoo.fr
Below changelog contains the history from the previous series.
Changelog:
v6 (previous series) -> v1 (new series):
- Do not rephrase the comment saying that BUILD_BUG_ON() is not
available in asm code. Leave it as it is.
- Add Lucas reviewed-by tag.
v5 -> v6:
- Restore the comment saying that BUILD_BUG_ON() is not available in asm
code.
v4 -> v5:
- Use tab indentations instead of single space to separate the
macro name from its body.
v3 -> v4:
- New patch in the series
---
include/linux/bits.h | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/include/linux/bits.h b/include/linux/bits.h
index 7ad0562191153471dac729b0020cdb1c9d3049fc..13dbc8adc70ee0a624b509cb6cb9f807459e0899 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -35,6 +35,11 @@
#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))
+
/*
* Generate a mask for the specified type @t. Additional checks are made to
* guarantee the value returned fits in that type, relying on
@@ -79,15 +84,11 @@
* BUILD_BUG_ON_ZERO is not available in h files included from asm files,
* disable the input check if that is the case.
*/
-#define GENMASK_INPUT_CHECK(h, l) 0
+#define GENMASK(h, l) __GENMASK(h, l)
+#define GENMASK_ULL(h, l) __GENMASK_ULL(h, l)
#endif /* !defined(__ASSEMBLY__) */
-#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))
-
#if !defined(__ASSEMBLY__)
/*
* Missing asm support
--
2.49.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 2/3] bits: unify the non-asm GENMASK*()
2025-06-09 2:45 [PATCH v2 0/3] bits: Split asm and non-asm GENMASK*() and unify definitions Vincent Mailhol via B4 Relay
2025-06-09 2:45 ` [PATCH v2 1/3] bits: split the definition of the asm and non-asm GENMASK*() Vincent Mailhol via B4 Relay
@ 2025-06-09 2:45 ` Vincent Mailhol via B4 Relay
2025-06-09 2:45 ` [PATCH v2 3/3] test_bits: add tests for __GENMASK() and __GENMASK_ULL() Vincent Mailhol via B4 Relay
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Vincent Mailhol via B4 Relay @ 2025-06-09 2:45 UTC (permalink / raw)
To: Yury Norov, Lucas De Marchi, Rasmus Villemoes, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, David Airlie,
Simona Vetter, Andrew Morton
Cc: linux-kernel, intel-gfx, dri-devel, Andi Shyti, David Laight,
Andy Shevchenko, Catalin Marinas, Anshuman Khandual,
linux-arm-kernel, Vincent Mailhol
From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
The newly introduced GENMASK_TYPE() macro can also be used to generate
the pre-existing non-asm GENMASK*() variants.
Apply GENMASK_TYPE() to GENMASK(), GENMASK_ULL() and GENMASK_U128().
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Changelog:
v1 -> v2:
- No changes
---
include/linux/bits.h | 26 ++++----------------------
1 file changed, 4 insertions(+), 22 deletions(-)
diff --git a/include/linux/bits.h b/include/linux/bits.h
index 13dbc8adc70ee0a624b509cb6cb9f807459e0899..a40cc861b3a7c60fec5184d7eb94fa15aafbbc06 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -2,10 +2,8 @@
#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)
@@ -35,11 +33,6 @@
#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))
-
/*
* Generate a mask for the specified type @t. Additional checks are made to
* guarantee the value returned fits in that type, relying on
@@ -55,10 +48,14 @@
(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)
/*
* Fixed-type variants of BIT(), with additional checks like GENMASK_TYPE(). The
@@ -89,19 +86,4 @@
#endif /* !defined(__ASSEMBLY__) */
-#if !defined(__ASSEMBLY__)
-/*
- * 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.
- */
-#define GENMASK_U128(h, l) \
- (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
-#endif
-
#endif /* __LINUX_BITS_H */
--
2.49.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 3/3] test_bits: add tests for __GENMASK() and __GENMASK_ULL()
2025-06-09 2:45 [PATCH v2 0/3] bits: Split asm and non-asm GENMASK*() and unify definitions Vincent Mailhol via B4 Relay
2025-06-09 2:45 ` [PATCH v2 1/3] bits: split the definition of the asm and non-asm GENMASK*() Vincent Mailhol via B4 Relay
2025-06-09 2:45 ` [PATCH v2 2/3] bits: unify the " Vincent Mailhol via B4 Relay
@ 2025-06-09 2:45 ` Vincent Mailhol via B4 Relay
2025-06-30 14:07 ` [PATCH v2 0/3] bits: Split asm and non-asm GENMASK*() and unify definitions Vincent Mailhol
2025-07-07 16:17 ` Yury Norov
4 siblings, 0 replies; 7+ messages in thread
From: Vincent Mailhol via B4 Relay @ 2025-06-09 2:45 UTC (permalink / raw)
To: Yury Norov, Lucas De Marchi, Rasmus Villemoes, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, David Airlie,
Simona Vetter, Andrew Morton
Cc: linux-kernel, intel-gfx, dri-devel, Andi Shyti, David Laight,
Andy Shevchenko, Catalin Marinas, Anshuman Khandual,
linux-arm-kernel, Vincent Mailhol
From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
The definitions of GENMASK() and GENMASK_ULL() do not depend any more
on __GENMASK() and __GENMASK_ULL(). Duplicate the existing unit tests
so that __GENMASK{,ULL}() are still covered.
Because __GENMASK() and __GENMASK_ULL() do use GENMASK_INPUT_CHECK(),
drop the TEST_GENMASK_FAILURES negative tests.
It would be good to have a small assembly test case for GENMASK*() in
case somebody decides to unify both in the future. However, I lack
expertise in assembly to do so. Instead add a FIXME message to
highlight the absence of the asm unit test.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
Changelog:
v1 -> v2:
- No changes
This patch previously belonged to this series:
bits: Fixed-type GENMASK_U*() and BIT_U*()
Link: https://lore.kernel.org/r/20250308-fixed-type-genmasks-v6-0-f59315e73c29@wanadoo.fr
Below changelog contains the history from the previous series.
Changelog:
v6 (previous series) -> v1 (new series):
- Add a message in the commit description to explain why the
negative tests are dropped.
- Add Lucas's Reviewed-by tag.
v5 -> v6:
- Add a FIXME message to highlight the absence of the asm
GENMASK*() unit tests.
v4 -> v5:
- No changes.
v3 -> v4:
- New patch.
---
lib/tests/test_bits.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/lib/tests/test_bits.c b/lib/tests/test_bits.c
index 47325b41515fde2c3ed434ed6f4094925c98886b..ab88e50d2edfa2b011f07d50460ac8ea6ff99923 100644
--- a/lib/tests/test_bits.c
+++ b/lib/tests/test_bits.c
@@ -26,6 +26,23 @@ static_assert(assert_type(u16, GENMASK_U16(15, 0)) == U16_MAX);
static_assert(assert_type(u32, GENMASK_U32(31, 0)) == U32_MAX);
static_assert(assert_type(u64, GENMASK_U64(63, 0)) == U64_MAX);
+/* FIXME: add a test case written in asm for GENMASK() and GENMASK_ULL() */
+
+static void __genmask_test(struct kunit *test)
+{
+ KUNIT_EXPECT_EQ(test, 1ul, __GENMASK(0, 0));
+ KUNIT_EXPECT_EQ(test, 3ul, __GENMASK(1, 0));
+ KUNIT_EXPECT_EQ(test, 6ul, __GENMASK(2, 1));
+ KUNIT_EXPECT_EQ(test, 0xFFFFFFFFul, __GENMASK(31, 0));
+}
+
+static void __genmask_ull_test(struct kunit *test)
+{
+ KUNIT_EXPECT_EQ(test, 1ull, __GENMASK_ULL(0, 0));
+ KUNIT_EXPECT_EQ(test, 3ull, __GENMASK_ULL(1, 0));
+ KUNIT_EXPECT_EQ(test, 0x000000ffffe00000ull, __GENMASK_ULL(39, 21));
+ KUNIT_EXPECT_EQ(test, 0xffffffffffffffffull, __GENMASK_ULL(63, 0));
+}
static void genmask_test(struct kunit *test)
{
@@ -123,6 +140,8 @@ static void genmask_input_check_test(struct kunit *test)
static struct kunit_case bits_test_cases[] = {
+ KUNIT_CASE(__genmask_test),
+ KUNIT_CASE(__genmask_ull_test),
KUNIT_CASE(genmask_test),
KUNIT_CASE(genmask_ull_test),
KUNIT_CASE(genmask_u128_test),
--
2.49.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 0/3] bits: Split asm and non-asm GENMASK*() and unify definitions
2025-06-09 2:45 [PATCH v2 0/3] bits: Split asm and non-asm GENMASK*() and unify definitions Vincent Mailhol via B4 Relay
` (2 preceding siblings ...)
2025-06-09 2:45 ` [PATCH v2 3/3] test_bits: add tests for __GENMASK() and __GENMASK_ULL() Vincent Mailhol via B4 Relay
@ 2025-06-30 14:07 ` Vincent Mailhol
2025-06-30 22:20 ` Yury Norov
2025-07-07 16:17 ` Yury Norov
4 siblings, 1 reply; 7+ messages in thread
From: Vincent Mailhol @ 2025-06-30 14:07 UTC (permalink / raw)
To: Yury Norov
Cc: linux-kernel, intel-gfx, dri-devel, Andi Shyti, David Laight,
Andy Shevchenko, Catalin Marinas, Anshuman Khandual,
linux-arm-kernel, Lucas De Marchi, Rasmus Villemoes, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, David Airlie,
Simona Vetter, Andrew Morton
Hi Yury,
On 09/06/2025 at 11:45, Vincent Mailhol wrote:
> This is a subset of below series:
>
> bits: Fixed-type GENMASK_U*() and BIT_U*()
> Link: https://lore.kernel.org/r/20250308-fixed-type-genmasks-v6-0-f59315e73c29@wanadoo.fr
>
> Yury suggested to split the above series in two steps:
>
> #1 Introduce the new fixed type GENMASK_U*() (already merged upstream)
> #2 Consolidate the existing GENMASK*()
>
> This new series is the resulting step #2 following the split.
>
> And thus, this series consolidate all the non-asm GENMASK*() so that
> they now all depend on GENMASK_TYPE() which was introduced in step #1.
>
> To do so, I had to split the definition of the asm and non-asm
> GENMASK(). I think this is controversial. So I initially implemented a
> first draft in which both the asm and non-asm version would rely on
> the same helper macro, i.e. adding this:
>
> #define __GENMASK_TYPE(t, w, h, l) \
> (((t)~_ULL(0) << (l)) & \
> ((t)~_ULL(0) >> (w - 1 - (h))))
>
> to uapi/bits.h. And then, the different GENMASK()s would look like
> this:
>
> #define __GENMASK(h, l) __GENMASK_TYPE(unsigned long, __BITS_PER_LONG, h, l)
>
> and so on.
>
> I implemented it, and the final result looked quite ugly. Not only do
> we need to manually provide the width each time, the biggest concern
> is that adding this to the uapi is asking for trouble. Who knows how
> people are going to use this? And once it is in the uapi, there is
> virtually no way back.
>
> Adding to this, that macro can not even be generalised to u128
> integers, whereas after the split, it can.
>
> And so, after implementing both, the asm seems way cleaner than the
> non-asm split and is, I think, the best compromise.
>
> Aside from the split, the asm's GENMASK() and GENMASK_ULL() are left
> untouched. While there are some strong incentives to also simplify
> these as pointed by David Laight in this thread:
>
> https://lore.kernel.org/all/20250309102312.4ff08576@pumpkin/
>
> this series deliberately limit its scope to the non-asm variants.
>
> Here are the bloat-o-meter stats:
>
> $ ./scripts/bloat-o-meter vmlinux_before.o vmlinux_after.o
> add/remove: 0/0 grow/shrink: 4/2 up/down: 5/-9 (-4)
> Function old new delta
> intel_psr_invalidate 352 354 +2
> mst_stream_compute_config 1589 1590 +1
> intel_psr_flush 707 708 +1
> intel_dp_compute_link_config 1338 1339 +1
> intel_drrs_activate 398 395 -3
> cfg80211_inform_bss_data 5137 5131 -6
> Total: Before=23333846, After=23333842, chg -0.00%
>
> (done with GCC 12.4.1 on an x86_64 defconfig)
>
> --
> 2.43.0
>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
I didn't hear back from you on this series. Are you still interested in this
cleanup or should I just abandon it?
Note that now that the GENMASK_U*() are upstream, I am done. I think that it
will be better with this clean-up, but I do not mind if we keep it as it.
Just let me know what you think.
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 0/3] bits: Split asm and non-asm GENMASK*() and unify definitions
2025-06-30 14:07 ` [PATCH v2 0/3] bits: Split asm and non-asm GENMASK*() and unify definitions Vincent Mailhol
@ 2025-06-30 22:20 ` Yury Norov
0 siblings, 0 replies; 7+ messages in thread
From: Yury Norov @ 2025-06-30 22:20 UTC (permalink / raw)
To: Vincent Mailhol
Cc: linux-kernel, intel-gfx, dri-devel, Andi Shyti, David Laight,
Andy Shevchenko, Catalin Marinas, Anshuman Khandual,
linux-arm-kernel, Lucas De Marchi, Rasmus Villemoes, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, David Airlie,
Simona Vetter, Andrew Morton
On Mon, Jun 30, 2025 at 11:07:43PM +0900, Vincent Mailhol wrote:
> Hi Yury,
...
> I didn't hear back from you on this series. Are you still interested in this
> cleanup or should I just abandon it?
>
> Note that now that the GENMASK_U*() are upstream, I am done. I think that it
> will be better with this clean-up, but I do not mind if we keep it as it.
>
> Just let me know what you think.
Hi Vincent,
Sorry for delay and thank you for pinging me on it.
I'll take a look on it at the weekend.
Thanks,
Yury
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/3] bits: Split asm and non-asm GENMASK*() and unify definitions
2025-06-09 2:45 [PATCH v2 0/3] bits: Split asm and non-asm GENMASK*() and unify definitions Vincent Mailhol via B4 Relay
` (3 preceding siblings ...)
2025-06-30 14:07 ` [PATCH v2 0/3] bits: Split asm and non-asm GENMASK*() and unify definitions Vincent Mailhol
@ 2025-07-07 16:17 ` Yury Norov
4 siblings, 0 replies; 7+ messages in thread
From: Yury Norov @ 2025-07-07 16:17 UTC (permalink / raw)
To: mailhol.vincent
Cc: Lucas De Marchi, Rasmus Villemoes, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, Tvrtko Ursulin, David Airlie, Simona Vetter,
Andrew Morton, linux-kernel, intel-gfx, dri-devel, Andi Shyti,
David Laight, Andy Shevchenko, Catalin Marinas, Anshuman Khandual,
linux-arm-kernel
On Mon, Jun 09, 2025 at 11:45:44AM +0900, Vincent Mailhol via B4 Relay wrote:
> This is a subset of below series:
>
> bits: Fixed-type GENMASK_U*() and BIT_U*()
> Link: https://lore.kernel.org/r/20250308-fixed-type-genmasks-v6-0-f59315e73c29@wanadoo.fr
>
> Yury suggested to split the above series in two steps:
>
> #1 Introduce the new fixed type GENMASK_U*() (already merged upstream)
> #2 Consolidate the existing GENMASK*()
>
> This new series is the resulting step #2 following the split.
>
> And thus, this series consolidate all the non-asm GENMASK*() so that
> they now all depend on GENMASK_TYPE() which was introduced in step #1.
>
> To do so, I had to split the definition of the asm and non-asm
> GENMASK(). I think this is controversial. So I initially implemented a
> first draft in which both the asm and non-asm version would rely on
> the same helper macro, i.e. adding this:
>
> #define __GENMASK_TYPE(t, w, h, l) \
> (((t)~_ULL(0) << (l)) & \
> ((t)~_ULL(0) >> (w - 1 - (h))))
>
> to uapi/bits.h. And then, the different GENMASK()s would look like
> this:
>
> #define __GENMASK(h, l) __GENMASK_TYPE(unsigned long, __BITS_PER_LONG, h, l)
>
> and so on.
>
> I implemented it, and the final result looked quite ugly. Not only do
> we need to manually provide the width each time, the biggest concern
> is that adding this to the uapi is asking for trouble. Who knows how
> people are going to use this? And once it is in the uapi, there is
> virtually no way back.
>
> Adding to this, that macro can not even be generalised to u128
> integers, whereas after the split, it can.
>
> And so, after implementing both, the asm seems way cleaner than the
> non-asm split and is, I think, the best compromise.
>
> Aside from the split, the asm's GENMASK() and GENMASK_ULL() are left
> untouched. While there are some strong incentives to also simplify
> these as pointed by David Laight in this thread:
>
> https://lore.kernel.org/all/20250309102312.4ff08576@pumpkin/
>
> this series deliberately limit its scope to the non-asm variants.
>
> Here are the bloat-o-meter stats:
>
> $ ./scripts/bloat-o-meter vmlinux_before.o vmlinux_after.o
> add/remove: 0/0 grow/shrink: 4/2 up/down: 5/-9 (-4)
> Function old new delta
> intel_psr_invalidate 352 354 +2
> mst_stream_compute_config 1589 1590 +1
> intel_psr_flush 707 708 +1
> intel_dp_compute_link_config 1338 1339 +1
> intel_drrs_activate 398 395 -3
> cfg80211_inform_bss_data 5137 5131 -6
> Total: Before=23333846, After=23333842, chg -0.00%
>
> (done with GCC 12.4.1 on an x86_64 defconfig)
So, I'm still concerned about that split for C and asm implementations.
But seemingly nobody else does, and after all it's a nice consolidation.
I've moved this in bitmap-for-next for testing. Thank you Vincent for
your work.
Thanks,
Yury
^ permalink raw reply [flat|nested] 7+ messages in thread