On Wed, Jun 21, 2023 at 07:20:59PM -0700, Yury Norov wrote:
Hi Lucas, all!
(Thanks, Andy, for pointing to this thread.)
On Mon, May 08, 2023 at 10:14:02PM -0700, Lucas De Marchi wrote:
Add GENMASK_U32(), GENMASK_U16() and GENMASK_U8() macros to create
masks for fixed-width types and also the corresponding BIT_U32(),
BIT_U16() and BIT_U8().
Can you split BIT() and GENMASK() material to separate patches?
All of those depend on a new "U" suffix added to the integer constant.
Due to naming clashes it's better to call the macro U32. Since C doesn't
have a proper suffix for short and char types, the U16 and U18 variants
just use U32 with one additional check in the BIT_* macros to make
sure the compiler gives an error when the those types overflow.
I feel like I don't understand the sentence...
maybe it was a digression of the integer constants
The BIT_U16() and BIT_U8() need the help of GENMASK_INPUT_CHECK(),
as otherwise they would allow an invalid bit to be passed. Hence
implement them in include/linux/bits.h rather than together with
the other BIT* variants.
I don't think it's a good way to go because BIT() belongs to a more basic
level than GENMASK(). Not mentioning possible header dependency issues.
If you need to test against tighter numeric region, I'd suggest to
do the same trick as GENMASK_INPUT_CHECK() does, but in uapi/linux/const.h
directly. Something like:
#define _U8(x) (CONST_GT(U8_MAX, x) + _AC(x, U))
The following test file is is used to test this:
$ cat mask.c
#include <linux/types.h>
#include <linux/bits.h>
static const u32 a = GENMASK_U32(31, 0);
static const u16 b = GENMASK_U16(15, 0);
static const u8 c = GENMASK_U8(7, 0);
static const u32 x = BIT_U32(31);
static const u16 y = BIT_U16(15);
static const u8 z = BIT_U8(7);
#if FAIL
static const u32 a2 = GENMASK_U32(32, 0);
static const u16 b2 = GENMASK_U16(16, 0);
static const u8 c2 = GENMASK_U8(8, 0);
static const u32 x2 = BIT_U32(32);
static const u16 y2 = BIT_U16(16);
static const u8 z2 = BIT_U8(8);
#endif
Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
---
include/linux/bits.h | 22 ++++++++++++++++++++++
include/uapi/linux/const.h | 2 ++
include/vdso/const.h | 1 +
3 files changed, 25 insertions(+)
diff --git a/include/linux/bits.h b/include/linux/bits.h
index 7c0cf5031abe..ff4786c99b8c 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -42,4 +42,26 @@
#define GENMASK_ULL(h, l) \
(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
+#define __GENMASK_U32(h, l) \
+ (((~U32(0)) - (U32(1) << (l)) + 1) & \
+ (~U32(0) >> (32 - 1 - (h))))
+#define GENMASK_U32(h, l) \
+ (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U32(h, l))
+
+#define __GENMASK_U16(h, l) \
+ ((U32(0xffff) - (U32(1) << (l)) + 1) & \
+ (U32(0xffff) >> (16 - 1 - (h))))
+#define GENMASK_U16(h, l) \
+ (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U16(h, l))
+
+#define __GENMASK_U8(h, l) \
+ (((U32(0xff)) - (U32(1) << (l)) + 1) & \
+ (U32(0xff) >> (8 - 1 - (h))))
+#define GENMASK_U8(h, l) \
+ (GENMASK_INPUT_CHECK(h, l) + __GENMASK_U8(h, l))
[...]
I see nothing wrong with fixed-wight versions of GENMASK if it helps
people to write safer code. Can you please in commit message mention
the exact patch(es) that added a bug related to GENMASK() misuse? It
would be easier to advocate the purpose of new API with that in mind.
Regarding implementation - we should avoid copy-pasting in cases
like this. Below is the patch that I boot-tested for x86_64 and
compile-tested for arm64.
It looks less opencoded, and maybe Andy will be less skeptical about
this approach because of less maintenance burden. Please take it if
you like for v2.
Thanks,
Yury
From 39c5b35075df67e7d88644470ca78a3486367c02 Mon Sep 17 00:00:00 2001
From: Yury Norov <yury.norov@xxxxxxxxx>
Date: Wed, 21 Jun 2023 15:27:29 -0700
Subject: [PATCH] bits: introduce fixed-type genmasks
Generalize __GENMASK() to support different types, and implement
fixed-types versions of GENMASK() based on it.
Signed-off-by: Yury Norov <yury.norov@xxxxxxxxx>
---
include/linux/bitops.h | 1 -
include/linux/bits.h | 22 ++++++++++++----------
2 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 2ba557e067fe..1db50c69cfdb 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -15,7 +15,6 @@
# define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n)))
#endif
-#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 7c0cf5031abe..cb94128171b2 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -6,6 +6,8 @@
#include <vdso/bits.h>
#include <asm/bitsperlong.h>
+#define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE)
+
#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))
@@ -30,16 +32,16 @@
#define GENMASK_INPUT_CHECK(h, l) 0
#endif
-#define __GENMASK(h, l) \
- (((~UL(0)) - (UL(1) << (l)) + 1) & \
- (~UL(0) >> (BITS_PER_LONG - 1 - (h))))
-#define GENMASK(h, l) \
- (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
+#define __GENMASK(t, h, l) \
+ (GENMASK_INPUT_CHECK(h, l) + \
+ (((t)~0ULL - ((t)(1) << (l)) + 1) & \
+ ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))
yeah... forcing the use of ull and then casting to the type is simpler
and does the job. Checked that it does not break the build if h is
greater than the type and it works
../include/linux/bits.h:40:20: error: right shift count >= width of type [-Werror=shift-count-overflow]
40 | ((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h)))))
| ^~
However this new version does increase the size. Using i915 module
to test:
$ size build64/drivers/gpu/drm/i915/i915.ko*
text data bss dec hex filename
4355676 213473 7048 4576197 45d3c5 build64/drivers/gpu/drm/i915/i915.ko
4361052 213505 7048 4581605 45e8e5 build64/drivers/gpu/drm/i915/i915.ko.new
Lucas De Marchi
-#define __GENMASK_ULL(h, l) \
- (((~ULL(0)) - (ULL(1) << (l)) + 1) & \
- (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h))))
-#define GENMASK_ULL(h, l) \
- (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
+#define GENMASK(h, l) __GENMASK(unsigned long, h, l)
+#define GENMASK_ULL(h, l) __GENMASK(unsigned long long, h, l)
+#define GENMASK_U8(h, l) __GENMASK(u8, h, l)
+#define GENMASK_U16(h, l) __GENMASK(u16, h, l)
+#define GENMASK_U32(h, l) __GENMASK(u32, h, l)
+#define GENMASK_U64(h, l) __GENMASK(u64, h, l)
#endif /* __LINUX_BITS_H */
--
2.39.2
