[PATCH v6 0/7] bits: Fixed-type GENMASK_U*() and BIT_U*()

[PATCH v6 0/7] bits: Fixed-type GENMASK_U*() and BIT_U*()

[Vincent Mailhol via B4 Relay]

Introduce some fixed width variant of the GENMASK() and the BIT()
macros in bits.h. Note that the main goal is not to get the correct
type, but rather to enforce more checks at compile time. For example:

  GENMASK_U16(16, 0)

will raise a build bug.

This series is a continuation of:

  https://lore.kernel.org/intel-xe/20240208074521.577076-1-lucas.demarchi@intel.com

from Lucas De Marchi. Above series is one year old. I really think
that this was a good idea and I do not want this series to die. So I
am volunteering to revive it.

Meanwhile, many changes occurred in bits.h. The most significant
change is that __GENMASK() was moved to the uapi headers.

In v4 and onward, I introduce one big change: split the definition of
the asm and non-asm GENMASK(). I think this is controversial.
Especially, Yury commented that he did not want such split. 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) - ((t)1 << (l)) + 1) &		\
  	 ((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 looks 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 generalized to u128
integers, whereas after the split, it is.

Finally, I do not think it makes sense to expose the fixed width
variants to the asm. The fixed width integers type are a C
concept. For asm, the long and long long variants seems sufficient.

And so, after implementing both, the asm and non-asm split seems way
more clean and I think this is the best compromise. Let me know what
you think :)

As requested, 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/-4 (1)
  Function                                     old     new   delta
  intel_psr_invalidate                         666     668      +2
  mst_stream_compute_config                   1652    1653      +1
  intel_psr_flush                              977     978      +1
  intel_dp_compute_link_config                1327    1328      +1
  cfg80211_inform_bss_data                    5109    5108      -1
  intel_drrs_activate                          379     376      -3
  Total: Before=22723481, After=22723482, chg +0.00%

(done with GCC 12.4.1 on an x86_64 defconfig)

--
2.43.0

---
Changes from v5:

  - Update the cover letter message. I was still refering to
    GENMASK_t() instead of GENMASK_TYPE().

  - Add a comment in the cover letter to explain that a common
    GENMASK_TYPE() for C and asm wouldn't allow to generate the u128
    variant.

  - Restore the comment saying that BUILD_BUG_ON() is not available in
    asm code.

  - Add a FIXME message to highlight the absence of the asm GENMASK*()
    unit tests.

  - Use git's histogram diff algorithm

  - Link to v5: https://lore.kernel.org/r/20250306-fixed-type-genmasks-v5-0-b443e9dcba63@wanadoo.fr

Changes from v4:

  - Rebase on https://github.com/norov/linux/tree/bitmap-for-next

  - Rename GENMASK_t() to GENMASK_TYPE()

  - First patch of v4 (the typo fix 'init128' -> 'int128') is removed
    because it was resent separately in:
    https://lore.kernel.org/all/20250305-fix_init128_typo-v1-1-cbe5b8e54e7d@wanadoo.fr

  - Replace the (t)~ULL(0) by type_max(t). This way, GENMASK_TYPE()
    can now be used to generate GENMASK_U128().

  - Get rid of the unsigned int cast for the U8 and U16 variants.

  - Add the BIT_TYPE() helper macro.

  - Link to v4: https://lore.kernel.org/r/20250305-fixed-type-genmasks-v4-0-1873dcdf6723@wanadoo.fr

Changes from v3:

  - Rebase on v6.14-rc5

  - Fix a typo in GENMASK_U128() comment.

  - Split the asm and non-asm definition of 

  - Replace ~0ULL by ~ULL(0)

  - Since v3, __GENMASK() was moved to the uapi and people started
    using directly. Introduce GENMASK_t() instead.

  - Link to v3: https://lore.kernel.org/intel-xe/20240208074521.577076-1-lucas.demarchi@intel.com

Changes from v2:

  - Document both in commit message and code about the strict type
    checking and give examples how it´d break with invalid params.

  - Link to v2: https://lore.kernel.org/intel-xe/20240124050205.3646390-1-lucas.demarchi@intel.com

Link to v1: https://lore.kernel.org/intel-xe/20230509051403.2748545-1-lucas.demarchi@intel.com

---
Lucas De Marchi (3):
      bits: introduce fixed-type BIT_U*()
      drm/i915: Convert REG_GENMASK*() to fixed-width GENMASK_U*()
      test_bits: add tests for GENMASK_U*()

Vincent Mailhol (3):
      bits: split the definition of the asm and non-asm GENMASK()
      test_bits: add tests for __GENMASK() and __GENMASK_ULL()
      test_bits: add tests for BIT_U*()

Yury Norov (1):
      bits: introduce fixed-type genmasks

 drivers/gpu/drm/i915/i915_reg_defs.h | 108 ++++-------------------------------
 include/linux/bitops.h               |   1 -
 include/linux/bits.h                 |  91 ++++++++++++++++++++---------
 lib/test_bits.c                      |  49 ++++++++++++++++
 4 files changed, 124 insertions(+), 125 deletions(-)
---
base-commit: 0312e94abe484b9ee58c32d2f8ba177e04955b35
change-id: 20250228-fixed-type-genmasks-8d1a555f34e8

Best regards,
-- 
Vincent Mailhol <mailhol.vincent@wanadoo.fr>

[PATCH v6 1/7] bits: split the definition of the asm and non-asm GENMASK()

[Vincent Mailhol via B4 Relay]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

In an upcoming change, GENMASK() and its friends will indirectly
depend on sizeof() which is not available in asm.

Instead of adding further complexity to __GENMASK() 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>
---
Changelog:

  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 | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index 14fd0ca9a6cd17339dd2f69e449558312a8a001b..9c1c7ce0bba6bb09490d891904c143a5394fd512 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -19,23 +19,17 @@
  * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
  */
 #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)))
-#else
-/*
- * 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
-#endif
 
 #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
  *
@@ -48,6 +42,16 @@
  */
 #define GENMASK_U128(h, l) \
 	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
-#endif
+
+#else /* defined(__ASSEMBLY__) */
+
+/*
+ * BUILD_BUG_ON_ZERO() is not available in h files included from asm files, so
+ * no input checks in assembly.
+ */
+#define GENMASK(h, l)		__GENMASK(h, l)
+#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)
+
+#endif /* !defined(__ASSEMBLY__) */
 
 #endif	/* __LINUX_BITS_H */

-- 
2.45.3

[PATCH v6 2/7] bits: introduce fixed-type genmasks

[Vincent Mailhol via B4 Relay]

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.3

[PATCH v6 3/7] bits: introduce fixed-type BIT_U*()

[Vincent Mailhol via B4 Relay]

From: Lucas De Marchi <lucas.demarchi@intel.com>

Implement fixed-type BIT_U*() to help drivers add stricter checks,
like it was done for GENMASK_U*().

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().

    - Use tab indentations instead of single space to separate the
      macro name from its body.

    - Add a global comment at the beginning of the file to explain why
      GENMASK_U*() and BIT_U*() are not available in asm.

    - Add a new BIT_TYPE() helper function, similar to GENMASK_TYPE().

    - Remove the unsigned int cast for the U8 and U16 variants. Move
      the cast to BIT_TYPE().

    - Rename the argument from BIT_U*(b) to BIT_U=(nr) for consistency
      with vdso/bits.h.

  v3 -> v4:

    - Use const_true() to simplify BIT_INPUT_CHECK().

    - Make BIT_U8() and BIT_U16() return an unsigned int instead of a
      u8 and u16. Because of the integer promotion rules in C, an u8
      or an u16 would become a signed integer as soon as these are
      used in any expression. By casting these to unsigned ints, at
      least the signedness is kept.

    - Put the cast next to the BIT() macro.

    - In BIT_U64(): use BIT_ULL() instead of BIT().
---
 include/linux/bits.h | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/include/linux/bits.h b/include/linux/bits.h
index b690611c769be61ab2b5ced43c8302ba5693308b..b234ef0394f133c8f11388fb6a4a5448d8ba9994 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -22,10 +22,10 @@
 /*
  * Missing asm support
  *
- * 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.
+ * GENMASK_U*() and BIT_U*() depend 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.
  */
 
 #include <linux/build_bug.h>
@@ -58,6 +58,24 @@
 #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
+ * following examples generate compiler warnings due to shift-count-overflow:
+ *
+ * - BIT_U8(8)
+ * - BIT_U32(-1)
+ * - BIT_U32(40)
+ */
+#define BIT_INPUT_CHECK(type, nr) \
+	BUILD_BUG_ON_ZERO(const_true((nr) >= BITS_PER_TYPE(type)))
+
+#define BIT_TYPE(type, nr) ((type)(BIT_INPUT_CHECK(type, nr) + BIT_ULL(nr)))
+
+#define BIT_U8(nr)	BIT_TYPE(u8, nr)
+#define BIT_U16(nr)	BIT_TYPE(u16, nr)
+#define BIT_U32(nr)	BIT_TYPE(u32, nr)
+#define BIT_U64(nr)	BIT_TYPE(u64, nr)
+
 #else /* defined(__ASSEMBLY__) */
 
 /*

-- 
2.45.3

[PATCH v6 4/7] drm/i915: Convert REG_GENMASK*() to fixed-width GENMASK_U*()

[Vincent Mailhol via B4 Relay]

From: Lucas De Marchi <lucas.demarchi@intel.com>

Now that include/linux/bits.h implements fixed-width GENMASK_U*(), use
them to implement the i915/xe specific macros. Converting each driver
to use the generic macros are left for later, when/if other
driver-specific macros are also generalized.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Changelog:

  v5 -> v6:

    - No changes.

  v4 -> v5:

    - Add braket to macro names in patch description,
      e.g. 'REG_GENMASK*' -> 'REG_GENMASK*()'

  v3 -> v4:

    - Remove the prefixes in macro parameters,
      e.g. 'REG_GENMASK(__high, __low)' -> 'REG_GENMASK(high, low)'
---
 drivers/gpu/drm/i915/i915_reg_defs.h | 108 ++++-------------------------------
 1 file changed, 11 insertions(+), 97 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h
index e251bcc0c89f5710125bc70f07851b2cb978c89c..39e5ed9511174b8757b9201bff735fa362651b34 100644
--- a/drivers/gpu/drm/i915/i915_reg_defs.h
+++ b/drivers/gpu/drm/i915/i915_reg_defs.h
@@ -9,76 +9,19 @@
 #include <linux/bitfield.h>
 #include <linux/bits.h>
 
-/**
- * REG_BIT() - Prepare a u32 bit value
- * @__n: 0-based bit number
- *
- * Local wrapper for BIT() to force u32, with compile time checks.
- *
- * @return: Value with bit @__n set.
+/*
+ * Wrappers over the generic BIT_* and GENMASK_* implementations,
+ * for compatibility reasons with previous implementation
  */
-#define REG_BIT(__n)							\
-	((u32)(BIT(__n) +						\
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&		\
-				 ((__n) < 0 || (__n) > 31))))
+#define REG_GENMASK(high, low)		GENMASK_U32(high, low)
+#define REG_GENMASK64(high, low)	GENMASK_U64(high, low)
+#define REG_GENMASK16(high, low)	GENMASK_U16(high, low)
+#define REG_GENMASK8(high, low)		GENMASK_U8(high, low)
 
-/**
- * REG_BIT8() - Prepare a u8 bit value
- * @__n: 0-based bit number
- *
- * Local wrapper for BIT() to force u8, with compile time checks.
- *
- * @return: Value with bit @__n set.
- */
-#define REG_BIT8(__n)                                                   \
-	((u8)(BIT(__n) +                                                \
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&         \
-				 ((__n) < 0 || (__n) > 7))))
-
-/**
- * REG_GENMASK() - Prepare a continuous u32 bitmask
- * @__high: 0-based high bit
- * @__low: 0-based low bit
- *
- * Local wrapper for GENMASK() to force u32, with compile time checks.
- *
- * @return: Continuous bitmask from @__high to @__low, inclusive.
- */
-#define REG_GENMASK(__high, __low)					\
-	((u32)(GENMASK(__high, __low) +					\
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&	\
-				 __is_constexpr(__low) &&		\
-				 ((__low) < 0 || (__high) > 31 || (__low) > (__high)))))
-
-/**
- * REG_GENMASK64() - Prepare a continuous u64 bitmask
- * @__high: 0-based high bit
- * @__low: 0-based low bit
- *
- * Local wrapper for GENMASK_ULL() to force u64, with compile time checks.
- *
- * @return: Continuous bitmask from @__high to @__low, inclusive.
- */
-#define REG_GENMASK64(__high, __low)					\
-	((u64)(GENMASK_ULL(__high, __low) +				\
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&		\
-				 __is_constexpr(__low) &&		\
-				 ((__low) < 0 || (__high) > 63 || (__low) > (__high)))))
-
-/**
- * REG_GENMASK8() - Prepare a continuous u8 bitmask
- * @__high: 0-based high bit
- * @__low: 0-based low bit
- *
- * Local wrapper for GENMASK() to force u8, with compile time checks.
- *
- * @return: Continuous bitmask from @__high to @__low, inclusive.
- */
-#define REG_GENMASK8(__high, __low)                                     \
-	((u8)(GENMASK(__high, __low) +                                  \
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&      \
-				 __is_constexpr(__low) &&               \
-				 ((__low) < 0 || (__high) > 7 || (__low) > (__high)))))
+#define REG_BIT(n)			BIT_U32(n)
+#define REG_BIT64(n)			BIT_U64(n)
+#define REG_BIT16(n)			BIT_U16(n)
+#define REG_BIT8(n)			BIT_U8(n)
 
 /*
  * Local integer constant expression version of is_power_of_2().
@@ -143,35 +86,6 @@
  */
 #define REG_FIELD_GET64(__mask, __val)	((u64)FIELD_GET(__mask, __val))
 
-/**
- * REG_BIT16() - Prepare a u16 bit value
- * @__n: 0-based bit number
- *
- * Local wrapper for BIT() to force u16, with compile time
- * checks.
- *
- * @return: Value with bit @__n set.
- */
-#define REG_BIT16(__n)                                                   \
-	((u16)(BIT(__n) +                                                \
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&         \
-				 ((__n) < 0 || (__n) > 15))))
-
-/**
- * REG_GENMASK16() - Prepare a continuous u8 bitmask
- * @__high: 0-based high bit
- * @__low: 0-based low bit
- *
- * Local wrapper for GENMASK() to force u16, with compile time
- * checks.
- *
- * @return: Continuous bitmask from @__high to @__low, inclusive.
- */
-#define REG_GENMASK16(__high, __low)                                     \
-	((u16)(GENMASK(__high, __low) +                                  \
-	       BUILD_BUG_ON_ZERO(__is_constexpr(__high) &&      \
-				 __is_constexpr(__low) &&               \
-				 ((__low) < 0 || (__high) > 15 || (__low) > (__high)))))
 
 /**
  * REG_FIELD_PREP16() - Prepare a u16 bitfield value

-- 
2.45.3

[PATCH v6 5/7] test_bits: add tests for __GENMASK() and __GENMASK_ULL()

[Vincent Mailhol via B4 Relay]

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}() is still covered.

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>
---
Changelog:

  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/test_bits.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/lib/test_bits.c b/lib/test_bits.c
index c7b38d91e1f16d42b7ca92e62fbd6c19b37e76a0..b0c29b01fb30cbcc759ac1e7106457b18bcd73a0 100644
--- a/lib/test_bits.c
+++ b/lib/test_bits.c
@@ -6,6 +6,23 @@
 #include <kunit/test.h>
 #include <linux/bits.h>
 
+/* 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)
 {
@@ -93,6 +110,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.45.3

[PATCH v6 6/7] test_bits: add tests for GENMASK_U*()

[Vincent Mailhol via B4 Relay]

From: Lucas De Marchi <lucas.demarchi@intel.com>

Add some additional tests in lib/test_bits.c to cover the
expected/non-expected values of the fixed-type GENMASK_U*() macros.

Also check that the result value matches the expected type. Since
those are known at build time, use static_assert() instead of normal
kunit tests.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Changelog:

  v5 -> v6:

    - No changes.

  v4 -> v5:

    - Revert v4 change. GENMASK_U8()/GENMASK_U16() are now back to
      u8/u16.

  v3 -> v4:

    - Adjust the type of GENMASK_U8()/GENMASK_U16() from u8/u16 to
      unsigned int.

    - Reorder the tests to match the order in which the macros are
      declared in bits.h.
---
 lib/test_bits.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/lib/test_bits.c b/lib/test_bits.c
index b0c29b01fb30cbcc759ac1e7106457b18bcd73a0..87112d1895194da33c0ffdf0a6fe6df43ce6b1e7 100644
--- a/lib/test_bits.c
+++ b/lib/test_bits.c
@@ -5,6 +5,16 @@
 
 #include <kunit/test.h>
 #include <linux/bits.h>
+#include <linux/types.h>
+
+#define assert_type(t, x) _Generic(x, t: x, default: 0)
+
+static_assert(assert_type(unsigned long, GENMASK(31, 0)) == U32_MAX);
+static_assert(assert_type(unsigned long long, GENMASK_ULL(63, 0)) == U64_MAX);
+static_assert(assert_type(u8, GENMASK_U8(7, 0)) == U8_MAX);
+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() */
 
@@ -31,11 +41,21 @@ static void genmask_test(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, 6ul, GENMASK(2, 1));
 	KUNIT_EXPECT_EQ(test, 0xFFFFFFFFul, GENMASK(31, 0));
 
+	KUNIT_EXPECT_EQ(test, 1u, GENMASK_U8(0, 0));
+	KUNIT_EXPECT_EQ(test, 3u, GENMASK_U16(1, 0));
+	KUNIT_EXPECT_EQ(test, 0x10000, GENMASK_U32(16, 16));
+
 #ifdef TEST_GENMASK_FAILURES
 	/* these should fail compilation */
 	GENMASK(0, 1);
 	GENMASK(0, 10);
 	GENMASK(9, 10);
+
+	GENMASK_U32(0, 31);
+	GENMASK_U64(64, 0);
+	GENMASK_U32(32, 0);
+	GENMASK_U16(16, 0);
+	GENMASK_U8(8, 0);
 #endif
 
 

-- 
2.45.3

[PATCH v6 7/7] test_bits: add tests for BIT_U*()

[Vincent Mailhol via B4 Relay]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

Add some additional tests in lib/test_bits.c to cover the expected
results of the fixed type BIT_U*() macros.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Changelog:

  v5 -> v6:

    - No changes.

  v4 -> v5:

    - BIT_U8()/BIT_U16() are now back to u8/u16.

  v3 -> v4:

    - New patch.
---
 lib/test_bits.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/test_bits.c b/lib/test_bits.c
index 87112d1895194da33c0ffdf0a6fe6df43ce6b1e7..ab88e50d2edfa2b011f07d50460ac8ea6ff99923 100644
--- a/lib/test_bits.c
+++ b/lib/test_bits.c
@@ -9,6 +9,16 @@
 
 #define assert_type(t, x) _Generic(x, t: x, default: 0)
 
+static_assert(assert_type(u8, BIT_U8(0)) == 1u);
+static_assert(assert_type(u16, BIT_U16(0)) == 1u);
+static_assert(assert_type(u32, BIT_U32(0)) == 1u);
+static_assert(assert_type(u64, BIT_U64(0)) == 1ull);
+
+static_assert(assert_type(u8, BIT_U8(7)) == 0x80u);
+static_assert(assert_type(u16, BIT_U16(15)) == 0x8000u);
+static_assert(assert_type(u32, BIT_U32(31)) == 0x80000000u);
+static_assert(assert_type(u64, BIT_U64(63)) == 0x8000000000000000ull);
+
 static_assert(assert_type(unsigned long, GENMASK(31, 0)) == U32_MAX);
 static_assert(assert_type(unsigned long long, GENMASK_ULL(63, 0)) == U64_MAX);
 static_assert(assert_type(u8, GENMASK_U8(7, 0)) == U8_MAX);

-- 
2.45.3

Re: [PATCH v6 0/7] bits: Fixed-type GENMASK_U*() and BIT_U*()

[Yury Norov]

No rush, please allow your reviewers a week or two before submitting
a new iteration unless you want to disregard the previous version for
some reason, of course. This will not get into the upcoming merge
window, anyways.

So, what should I do? Go through the v5 and all discussions in there,
or just jump on this?

On Sat, Mar 08, 2025 at 01:48:47AM +0900, Vincent Mailhol via B4 Relay wrote:
> Introduce some fixed width variant of the GENMASK() and the BIT()
> macros in bits.h. Note that the main goal is not to get the correct
> type, but rather to enforce more checks at compile time. For example:
> 
>   GENMASK_U16(16, 0)
> 
> will raise a build bug.
> 
> This series is a continuation of:
> 
>   https://lore.kernel.org/intel-xe/20240208074521.577076-1-lucas.demarchi@intel.com
> 
> from Lucas De Marchi. Above series is one year old. I really think
> that this was a good idea and I do not want this series to die. So I
> am volunteering to revive it.
> 
> Meanwhile, many changes occurred in bits.h. The most significant
> change is that __GENMASK() was moved to the uapi headers.
> 
> In v4 and onward, I introduce one big change: split the definition of
> the asm and non-asm GENMASK(). I think this is controversial.
> Especially, Yury commented that he did not want such split. 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) - ((t)1 << (l)) + 1) &		\
>   	 ((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 looks 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 generalized to u128
> integers, whereas after the split, it is.
> 
> Finally, I do not think it makes sense to expose the fixed width
> variants to the asm. The fixed width integers type are a C
> concept. For asm, the long and long long variants seems sufficient.
> 
> And so, after implementing both, the asm and non-asm split seems way
> more clean and I think this is the best compromise. Let me know what
> you think :)
> 
> As requested, 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/-4 (1)
>   Function                                     old     new   delta
>   intel_psr_invalidate                         666     668      +2
>   mst_stream_compute_config                   1652    1653      +1
>   intel_psr_flush                              977     978      +1
>   intel_dp_compute_link_config                1327    1328      +1
>   cfg80211_inform_bss_data                    5109    5108      -1
>   intel_drrs_activate                          379     376      -3
>   Total: Before=22723481, After=22723482, chg +0.00%
> 
> (done with GCC 12.4.1 on an x86_64 defconfig)
> 
> --
> 2.43.0
> 
> ---
> Changes from v5:
> 
>   - Update the cover letter message. I was still refering to
>     GENMASK_t() instead of GENMASK_TYPE().
> 
>   - Add a comment in the cover letter to explain that a common
>     GENMASK_TYPE() for C and asm wouldn't allow to generate the u128
>     variant.
> 
>   - Restore the comment saying that BUILD_BUG_ON() is not available in
>     asm code.
> 
>   - Add a FIXME message to highlight the absence of the asm GENMASK*()
>     unit tests.
> 
>   - Use git's histogram diff algorithm
> 
>   - Link to v5: https://lore.kernel.org/r/20250306-fixed-type-genmasks-v5-0-b443e9dcba63@wanadoo.fr
> 
> Changes from v4:
> 
>   - Rebase on https://github.com/norov/linux/tree/bitmap-for-next
> 
>   - Rename GENMASK_t() to GENMASK_TYPE()
> 
>   - First patch of v4 (the typo fix 'init128' -> 'int128') is removed
>     because it was resent separately in:
>     https://lore.kernel.org/all/20250305-fix_init128_typo-v1-1-cbe5b8e54e7d@wanadoo.fr
> 
>   - Replace the (t)~ULL(0) by type_max(t). This way, GENMASK_TYPE()
>     can now be used to generate GENMASK_U128().
> 
>   - Get rid of the unsigned int cast for the U8 and U16 variants.
> 
>   - Add the BIT_TYPE() helper macro.
> 
>   - Link to v4: https://lore.kernel.org/r/20250305-fixed-type-genmasks-v4-0-1873dcdf6723@wanadoo.fr
> 
> Changes from v3:
> 
>   - Rebase on v6.14-rc5
> 
>   - Fix a typo in GENMASK_U128() comment.
> 
>   - Split the asm and non-asm definition of 
> 
>   - Replace ~0ULL by ~ULL(0)
> 
>   - Since v3, __GENMASK() was moved to the uapi and people started
>     using directly. Introduce GENMASK_t() instead.
> 
>   - Link to v3: https://lore.kernel.org/intel-xe/20240208074521.577076-1-lucas.demarchi@intel.com
> 
> Changes from v2:
> 
>   - Document both in commit message and code about the strict type
>     checking and give examples how it´d break with invalid params.
> 
>   - Link to v2: https://lore.kernel.org/intel-xe/20240124050205.3646390-1-lucas.demarchi@intel.com
> 
> Link to v1: https://lore.kernel.org/intel-xe/20230509051403.2748545-1-lucas.demarchi@intel.com
> 
> ---
> Lucas De Marchi (3):
>       bits: introduce fixed-type BIT_U*()
>       drm/i915: Convert REG_GENMASK*() to fixed-width GENMASK_U*()
>       test_bits: add tests for GENMASK_U*()
> 
> Vincent Mailhol (3):
>       bits: split the definition of the asm and non-asm GENMASK()
>       test_bits: add tests for __GENMASK() and __GENMASK_ULL()
>       test_bits: add tests for BIT_U*()
> 
> Yury Norov (1):
>       bits: introduce fixed-type genmasks
> 
>  drivers/gpu/drm/i915/i915_reg_defs.h | 108 ++++-------------------------------
>  include/linux/bitops.h               |   1 -
>  include/linux/bits.h                 |  91 ++++++++++++++++++++---------
>  lib/test_bits.c                      |  49 ++++++++++++++++
>  4 files changed, 124 insertions(+), 125 deletions(-)
> ---
> base-commit: 0312e94abe484b9ee58c32d2f8ba177e04955b35
> change-id: 20250228-fixed-type-genmasks-8d1a555f34e8
> 
> Best regards,
> -- 
> Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 

Re: [PATCH v6 1/7] bits: split the definition of the asm and non-asm GENMASK()

[Andy Shevchenko]

On Sat, Mar 08, 2025 at 01:48:48AM +0900, Vincent Mailhol via B4 Relay wrote:
> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 
> In an upcoming change, GENMASK() and its friends will indirectly
> depend on sizeof() which is not available in asm.
> 
> Instead of adding further complexity to __GENMASK() to make it work
> for both asm and non asm, just split the definition of the two
> variants.

...

> -/*
> - * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> - * disable the input check if that is the case.
> - */

> +/*
> + * BUILD_BUG_ON_ZERO() is not available in h files included from asm files, so
> + * no input checks in assembly.
> + */

In case of a new version I would reformat this as

/*
 * BUILD_BUG_ON_ZERO() is not available in h files included from asm files,
 * so no input checks in assembly.
 */

It makes easier to review the changes and see that the first line is kept
the same.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 0/7] bits: Fixed-type GENMASK_U*() and BIT_U*()

[Andy Shevchenko]

On Fri, Mar 07, 2025 at 12:18:02PM -0500, Yury Norov wrote:
> No rush, please allow your reviewers a week or two before submitting
> a new iteration unless you want to disregard the previous version for
> some reason, of course. This will not get into the upcoming merge
> window, anyways.
> 
> So, what should I do? Go through the v5 and all discussions in there,
> or just jump on this?

There is also question to you. Are we going to leave with U128 variants or is
it subject to remove? If the latter, can you issue a formal patch?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 2/7] bits: introduce fixed-type genmasks

[Andy Shevchenko]

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

Re: [PATCH v6 3/7] bits: introduce fixed-type BIT_U*()

[Andy Shevchenko]

On Sat, Mar 08, 2025 at 01:48:50AM +0900, Vincent Mailhol via B4 Relay wrote:
> From: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> Implement fixed-type BIT_U*() to help drivers add stricter checks,
> like it was done for GENMASK_U*().

...

>  /*
>   * Missing asm support
>   *
> - * 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.
> + * GENMASK_U*() and BIT_U*() depend 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.
>   */

I don't like this hunk. You just introduced a message which is rewritten
completely in the immediate followup. Can you come up in a better text
here and/or there so it will give only + LoCs (or minimizes - to 1:ish)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 0/7] bits: Fixed-type GENMASK_U*() and BIT_U*()

[Yury Norov]

On Fri, Mar 07, 2025 at 07:43:57PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 07, 2025 at 12:18:02PM -0500, Yury Norov wrote:
> > No rush, please allow your reviewers a week or two before submitting
> > a new iteration unless you want to disregard the previous version for
> > some reason, of course. This will not get into the upcoming merge
> > window, anyways.
> > 
> > So, what should I do? Go through the v5 and all discussions in there,
> > or just jump on this?
> 
> There is also question to you. Are we going to leave with U128 variants or is
> it subject to remove? If the latter, can you issue a formal patch?

I asked Anshuman about it as he's the only person interested in it. Will wait
for a _usual_ few weeks for reply before making any conclusions. If you know
anyone relevant in ARM or everywhere else, feel free to loop them.

Re: [PATCH v6 3/7] bits: introduce fixed-type BIT_U*()

[Andy Shevchenko]

On Fri, Mar 07, 2025 at 07:48:01PM +0200, Andy Shevchenko wrote:
> On Sat, Mar 08, 2025 at 01:48:50AM +0900, Vincent Mailhol via B4 Relay wrote:

...

> >  /*
> >   * Missing asm support
> >   *
> > - * 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.
> > + * GENMASK_U*() and BIT_U*() depend 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.
> >   */
> 
> I don't like this hunk. You just introduced a message which is rewritten
> completely in the immediate followup. Can you come up in a better text
> here and/or there so it will give only + LoCs (or minimizes - to 1:ish)?

And also note, that using up to 90 characters in the comments most likely fine
here. At least I don't see a problem with that.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 0/7] bits: Fixed-type GENMASK_U*() and BIT_U*()

[Andy Shevchenko]

On Fri, Mar 07, 2025 at 12:48:36PM -0500, Yury Norov wrote:
> On Fri, Mar 07, 2025 at 07:43:57PM +0200, Andy Shevchenko wrote:
> > On Fri, Mar 07, 2025 at 12:18:02PM -0500, Yury Norov wrote:
> > > No rush, please allow your reviewers a week or two before submitting
> > > a new iteration unless you want to disregard the previous version for
> > > some reason, of course. This will not get into the upcoming merge
> > > window, anyways.
> > > 
> > > So, what should I do? Go through the v5 and all discussions in there,
> > > or just jump on this?
> > 
> > There is also question to you. Are we going to leave with U128 variants or is
> > it subject to remove? If the latter, can you issue a formal patch?
> 
> I asked Anshuman about it as he's the only person interested in it. Will wait
> for a _usual_ few weeks for reply before making any conclusions. If you know
> anyone relevant in ARM or everywhere else, feel free to loop them.

I see, yep, we still have time for that, let's wait a bit.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 4/7] drm/i915: Convert REG_GENMASK*() to fixed-width GENMASK_U*()

[Andy Shevchenko]

On Sat, Mar 08, 2025 at 01:48:51AM +0900, Vincent Mailhol via B4 Relay wrote:
> From: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> Now that include/linux/bits.h implements fixed-width GENMASK_U*(), use
> them to implement the i915/xe specific macros. Converting each driver
> to use the generic macros are left for later, when/if other
> driver-specific macros are also generalized.

...

> +/*
> + * Wrappers over the generic BIT_* and GENMASK_* implementations,

BIT_U*(), GENMASK_U*() as far as I can see.

Also "...generic fixed-width...".

> + * for compatibility reasons with previous implementation
>   */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 1/7] bits: split the definition of the asm and non-asm GENMASK()

[Vincent Mailhol]

On 08/03/2025 at 02:42, Andy Shevchenko wrote:
> On Sat, Mar 08, 2025 at 01:48:48AM +0900, Vincent Mailhol via B4 Relay wrote:
>> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>
>> In an upcoming change, GENMASK() and its friends will indirectly
>> depend on sizeof() which is not available in asm.
>>
>> Instead of adding further complexity to __GENMASK() to make it work
>> for both asm and non asm, just split the definition of the two
>> variants.
> 
> ...
> 
>> -/*
>> - * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
>> - * disable the input check if that is the case.
>> - */
> 
>> +/*
>> + * BUILD_BUG_ON_ZERO() is not available in h files included from asm files, so
>> + * no input checks in assembly.
>> + */
> 
> In case of a new version I would reformat this as
> 
> /*
>  * BUILD_BUG_ON_ZERO() is not available in h files included from asm files,
>  * so no input checks in assembly.
>  */
> 
> It makes easier to review the changes and see that the first line is kept
> the same.

Not fully convinced, but why not. I staged this change locally, it will
be in v7.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v6 3/7] bits: introduce fixed-type BIT_U*()

[Vincent Mailhol]

On 08/03/2025 at 02:49, Andy Shevchenko wrote:
> On Fri, Mar 07, 2025 at 07:48:01PM +0200, Andy Shevchenko wrote:
>> On Sat, Mar 08, 2025 at 01:48:50AM +0900, Vincent Mailhol via B4 Relay wrote:
> 
> ...
> 
>>>  /*
>>>   * Missing asm support
>>>   *
>>> - * 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.
>>> + * GENMASK_U*() and BIT_U*() depend 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.
>>>   */
>>
>> I don't like this hunk. You just introduced a message which is rewritten
>> completely in the immediate followup. Can you come up in a better text
>> here and/or there so it will give only + LoCs (or minimizes - to 1:ish)?

OK. I will add an artificial early new line in the previous patch so
that the diff is only one line.

> And also note, that using up to 90 characters in the comments most likely fine
> here. At least I don't see a problem with that.

I re-wrapped the text to the 80 column and it now fits on three lines.
90 column wouldn't reduce the line count, so I am keeping it to 80.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v6 4/7] drm/i915: Convert REG_GENMASK*() to fixed-width GENMASK_U*()

[Vincent Mailhol]

On 08/03/2025 at 02:54, Andy Shevchenko wrote:
> On Sat, Mar 08, 2025 at 01:48:51AM +0900, Vincent Mailhol via B4 Relay wrote:
>> From: Lucas De Marchi <lucas.demarchi@intel.com>
>>
>> Now that include/linux/bits.h implements fixed-width GENMASK_U*(), use
>> them to implement the i915/xe specific macros. Converting each driver
>> to use the generic macros are left for later, when/if other
>> driver-specific macros are also generalized.
> 
> ...
> 
>> +/*
>> + * Wrappers over the generic BIT_* and GENMASK_* implementations,
> 
> BIT_U*(), GENMASK_U*() as far as I can see.
> 
> Also "...generic fixed-width...".

Ack. I will address both in next version.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v6 0/7] bits: Fixed-type GENMASK_U*() and BIT_U*()

[Vincent Mailhol]

On 08/03/2025 at 02:50, Andy Shevchenko wrote:
> On Fri, Mar 07, 2025 at 12:48:36PM -0500, Yury Norov wrote:
>> On Fri, Mar 07, 2025 at 07:43:57PM +0200, Andy Shevchenko wrote:
>>> On Fri, Mar 07, 2025 at 12:18:02PM -0500, Yury Norov wrote:
>>>> No rush, please allow your reviewers a week or two before submitting
>>>> a new iteration unless you want to disregard the previous version for
>>>> some reason, of course. This will not get into the upcoming merge
>>>> window, anyways.

Ack. I was not expecting this to go into the next merge windows either.

Most of the feedback was not on the actual code but just on the naming,
the code comments or the patch descriptions. I normally wait longer on
the first version of a series but I tend to do kick re-spin when
addressing the nitpicks.

But message taken! I will wait a couple of weeks before the next iteration.

>>>> So, what should I do? Go through the v5 and all discussions in there,
>>>> or just jump on this?

The code is the same between v5 and v6.

There is this message from David in which he suggested to make some
changes to the uapi __GENMASK() and __GENMASK_ULL() and to which I
commented that I was not confident doing such changes:

  https://lore.kernel.org/all/20250306192331.2701a029@pumpkin/t/#u

Aside from the above, you wouldn't miss much by directly jumping on this v6.

>>> There is also question to you. Are we going to leave with U128 variants or is
>>> it subject to remove? If the latter, can you issue a formal patch?
>>
>> I asked Anshuman about it as he's the only person interested in it. Will wait
>> for a _usual_ few weeks for reply before making any conclusions. If you know
>> anyone relevant in ARM or everywhere else, feel free to loop them.
> 
> I see, yep, we still have time for that, let's wait a bit.

Ack. Andy, I already addressed your last comments in my local tree. I
will now wait for others' feedback.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v6 1/7] bits: split the definition of the asm and non-asm GENMASK()

[Yury Norov]

On Sat, Mar 08, 2025 at 06:10:25PM +0900, Vincent Mailhol wrote:
> On 08/03/2025 at 02:42, Andy Shevchenko wrote:
> > On Sat, Mar 08, 2025 at 01:48:48AM +0900, Vincent Mailhol via B4 Relay wrote:
> >> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >>
> >> In an upcoming change, GENMASK() and its friends will indirectly
> >> depend on sizeof() which is not available in asm.
> >>
> >> Instead of adding further complexity to __GENMASK() to make it work
> >> for both asm and non asm, just split the definition of the two
> >> variants.
> > 
> > ...
> > 
> >> -/*
> >> - * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> >> - * disable the input check if that is the case.
> >> - */
> > 
> >> +/*
> >> + * BUILD_BUG_ON_ZERO() is not available in h files included from asm files, so
> >> + * no input checks in assembly.
> >> + */
> > 
> > In case of a new version I would reformat this as
> > 
> > /*
> >  * BUILD_BUG_ON_ZERO() is not available in h files included from asm files,
> >  * so no input checks in assembly.
> >  */
> > 
> > It makes easier to review the changes and see that the first line is kept
> > the same.
> 
> Not fully convinced, but why not. I staged this change locally, it will
> be in v7.

I don't understand why this change is needed at all. The comment
states the same thing before and after, but the history gets wiped.
Maybe just don't touch it?

Re: [PATCH v6 1/7] bits: split the definition of the asm and non-asm GENMASK()

[Vincent Mailhol]

On 19/03/2025 at 01:06, Yury Norov wrote:
> On Sat, Mar 08, 2025 at 06:10:25PM +0900, Vincent Mailhol wrote:
>> On 08/03/2025 at 02:42, Andy Shevchenko wrote:
>>> On Sat, Mar 08, 2025 at 01:48:48AM +0900, Vincent Mailhol via B4 Relay wrote:
>>>> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>>>
>>>> In an upcoming change, GENMASK() and its friends will indirectly
>>>> depend on sizeof() which is not available in asm.
>>>>
>>>> Instead of adding further complexity to __GENMASK() to make it work
>>>> for both asm and non asm, just split the definition of the two
>>>> variants.
>>>
>>> ...
>>>
>>>> -/*
>>>> - * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
>>>> - * disable the input check if that is the case.
>>>> - */
>>>
>>>> +/*
>>>> + * BUILD_BUG_ON_ZERO() is not available in h files included from asm files, so
>>>> + * no input checks in assembly.
>>>> + */
>>>
>>> In case of a new version I would reformat this as
>>>
>>> /*
>>>  * BUILD_BUG_ON_ZERO() is not available in h files included from asm files,
>>>  * so no input checks in assembly.
>>>  */
>>>
>>> It makes easier to review the changes and see that the first line is kept
>>> the same.
>>
>> Not fully convinced, but why not. I staged this change locally, it will
>> be in v7.
> 
> I don't understand why this change is needed at all. The comment
> states the same thing before and after, but the history gets wiped.
> Maybe just don't touch it?

Ack. I will just move the text down and not rephrase.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v6 2/7] bits: introduce fixed-type genmasks

[Yury Norov]

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)))))
>         |                           ^~
> 
> 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
> 

Re: [PATCH v6 4/7] drm/i915: Convert REG_GENMASK*() to fixed-width GENMASK_U*()

[Yury Norov]

On Sat, Mar 08, 2025 at 01:48:51AM +0900, Vincent Mailhol via B4 Relay wrote:
> From: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> Now that include/linux/bits.h implements fixed-width GENMASK_U*(), use
> them to implement the i915/xe specific macros. Converting each driver
> to use the generic macros are left for later, when/if other
> driver-specific macros are also generalized.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> Changelog:
> 
>   v5 -> v6:
> 
>     - No changes.
> 
>   v4 -> v5:
> 
>     - Add braket to macro names in patch description,
>       e.g. 'REG_GENMASK*' -> 'REG_GENMASK*()'
> 
>   v3 -> v4:
> 
>     - Remove the prefixes in macro parameters,
>       e.g. 'REG_GENMASK(__high, __low)' -> 'REG_GENMASK(high, low)'
> ---
>  drivers/gpu/drm/i915/i915_reg_defs.h | 108 ++++-------------------------------
>  1 file changed, 11 insertions(+), 97 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h
> index e251bcc0c89f5710125bc70f07851b2cb978c89c..39e5ed9511174b8757b9201bff735fa362651b34 100644
> --- a/drivers/gpu/drm/i915/i915_reg_defs.h
> +++ b/drivers/gpu/drm/i915/i915_reg_defs.h
> @@ -9,76 +9,19 @@
>  #include <linux/bitfield.h>
>  #include <linux/bits.h>
>  
> -/**
> - * REG_BIT() - Prepare a u32 bit value
> - * @__n: 0-based bit number
> - *
> - * Local wrapper for BIT() to force u32, with compile time checks.
> - *
> - * @return: Value with bit @__n set.
> +/*
> + * Wrappers over the generic BIT_* and GENMASK_* implementations,
> + * for compatibility reasons with previous implementation
>   */
> -#define REG_BIT(__n)							\
> -	((u32)(BIT(__n) +						\
> -	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&		\
> -				 ((__n) < 0 || (__n) > 31))))
> +#define REG_GENMASK(high, low)		GENMASK_U32(high, low)
> +#define REG_GENMASK64(high, low)	GENMASK_U64(high, low)
> +#define REG_GENMASK16(high, low)	GENMASK_U16(high, low)
> +#define REG_GENMASK8(high, low)		GENMASK_U8(high, low)

Nit. Maybe just

 #define REG_GENMASK		GENMASK_U32

Re: [PATCH v6 4/7] drm/i915: Convert REG_GENMASK*() to fixed-width GENMASK_U*()

[Jani Nikula]

On Tue, 18 Mar 2025, Yury Norov <yury.norov@gmail.com> wrote:
> On Sat, Mar 08, 2025 at 01:48:51AM +0900, Vincent Mailhol via B4 Relay wrote:
>> From: Lucas De Marchi <lucas.demarchi@intel.com>
>> 
>> Now that include/linux/bits.h implements fixed-width GENMASK_U*(), use
>> them to implement the i915/xe specific macros. Converting each driver
>> to use the generic macros are left for later, when/if other
>> driver-specific macros are also generalized.
>> 
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>> ---
>> Changelog:
>> 
>>   v5 -> v6:
>> 
>>     - No changes.
>> 
>>   v4 -> v5:
>> 
>>     - Add braket to macro names in patch description,
>>       e.g. 'REG_GENMASK*' -> 'REG_GENMASK*()'
>> 
>>   v3 -> v4:
>> 
>>     - Remove the prefixes in macro parameters,
>>       e.g. 'REG_GENMASK(__high, __low)' -> 'REG_GENMASK(high, low)'
>> ---
>>  drivers/gpu/drm/i915/i915_reg_defs.h | 108 ++++-------------------------------
>>  1 file changed, 11 insertions(+), 97 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h
>> index e251bcc0c89f5710125bc70f07851b2cb978c89c..39e5ed9511174b8757b9201bff735fa362651b34 100644
>> --- a/drivers/gpu/drm/i915/i915_reg_defs.h
>> +++ b/drivers/gpu/drm/i915/i915_reg_defs.h
>> @@ -9,76 +9,19 @@
>>  #include <linux/bitfield.h>
>>  #include <linux/bits.h>
>>  
>> -/**
>> - * REG_BIT() - Prepare a u32 bit value
>> - * @__n: 0-based bit number
>> - *
>> - * Local wrapper for BIT() to force u32, with compile time checks.
>> - *
>> - * @return: Value with bit @__n set.
>> +/*
>> + * Wrappers over the generic BIT_* and GENMASK_* implementations,
>> + * for compatibility reasons with previous implementation
>>   */
>> -#define REG_BIT(__n)							\
>> -	((u32)(BIT(__n) +						\
>> -	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&		\
>> -				 ((__n) < 0 || (__n) > 31))))
>> +#define REG_GENMASK(high, low)		GENMASK_U32(high, low)
>> +#define REG_GENMASK64(high, low)	GENMASK_U64(high, low)
>> +#define REG_GENMASK16(high, low)	GENMASK_U16(high, low)
>> +#define REG_GENMASK8(high, low)		GENMASK_U8(high, low)
>
> Nit. Maybe just
>
>  #define REG_GENMASK		GENMASK_U32

Please just keep it as it is for clarity.

BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH v6 4/7] drm/i915: Convert REG_GENMASK*() to fixed-width GENMASK_U*()

[Vincent Mailhol]

On 19/03/2025 at 07:32, Jani Nikula wrote:
> On Tue, 18 Mar 2025, Yury Norov <yury.norov@gmail.com> wrote:
>> On Sat, Mar 08, 2025 at 01:48:51AM +0900, Vincent Mailhol via B4 Relay wrote:
>>> From: Lucas De Marchi <lucas.demarchi@intel.com>
>>>
>>> Now that include/linux/bits.h implements fixed-width GENMASK_U*(), use
>>> them to implement the i915/xe specific macros. Converting each driver
>>> to use the generic macros are left for later, when/if other
>>> driver-specific macros are also generalized.
>>>
>>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>> Acked-by: Jani Nikula <jani.nikula@intel.com>
>>> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>> ---
>>> Changelog:
>>>
>>>   v5 -> v6:
>>>
>>>     - No changes.
>>>
>>>   v4 -> v5:
>>>
>>>     - Add braket to macro names in patch description,
>>>       e.g. 'REG_GENMASK*' -> 'REG_GENMASK*()'
>>>
>>>   v3 -> v4:
>>>
>>>     - Remove the prefixes in macro parameters,
>>>       e.g. 'REG_GENMASK(__high, __low)' -> 'REG_GENMASK(high, low)'
>>> ---
>>>  drivers/gpu/drm/i915/i915_reg_defs.h | 108 ++++-------------------------------
>>>  1 file changed, 11 insertions(+), 97 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h
>>> index e251bcc0c89f5710125bc70f07851b2cb978c89c..39e5ed9511174b8757b9201bff735fa362651b34 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg_defs.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg_defs.h
>>> @@ -9,76 +9,19 @@
>>>  #include <linux/bitfield.h>
>>>  #include <linux/bits.h>
>>>  
>>> -/**
>>> - * REG_BIT() - Prepare a u32 bit value
>>> - * @__n: 0-based bit number
>>> - *
>>> - * Local wrapper for BIT() to force u32, with compile time checks.
>>> - *
>>> - * @return: Value with bit @__n set.
>>> +/*
>>> + * Wrappers over the generic BIT_* and GENMASK_* implementations,
>>> + * for compatibility reasons with previous implementation
>>>   */
>>> -#define REG_BIT(__n)							\
>>> -	((u32)(BIT(__n) +						\
>>> -	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&		\
>>> -				 ((__n) < 0 || (__n) > 31))))
>>> +#define REG_GENMASK(high, low)		GENMASK_U32(high, low)
>>> +#define REG_GENMASK64(high, low)	GENMASK_U64(high, low)
>>> +#define REG_GENMASK16(high, low)	GENMASK_U16(high, low)
>>> +#define REG_GENMASK8(high, low)		GENMASK_U8(high, low)
>>
>> Nit. Maybe just
>>
>>  #define REG_GENMASK		GENMASK_U32
> 
> Please just keep it as it is for clarity.

I also prefer when the argument is clearly displayed. It shows at first
glance that this is a function-like macro and reminds of the correct
order of the argument without having to look at the definitions in
bits.h. It also allows for people to grep "#define REG_GENMASK(" in
order to find the macro definition.

To be honest, I don't have a strong opinion either, but because Jani
also prefers it this way, I will keep as-is.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v6 2/7] bits: introduce fixed-type genmasks

[Vincent Mailhol]

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

Re: [PATCH v6 2/7] bits: introduce fixed-type genmasks

[Yury Norov]

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.

Re: [PATCH v6 4/7] drm/i915: Convert REG_GENMASK*() to fixed-width GENMASK_U*()

[Yury Norov]

On Wed, Mar 19, 2025 at 01:37:32PM +0900, Vincent Mailhol wrote:
> On 19/03/2025 at 07:32, Jani Nikula wrote:
> > On Tue, 18 Mar 2025, Yury Norov <yury.norov@gmail.com> wrote:
> >> On Sat, Mar 08, 2025 at 01:48:51AM +0900, Vincent Mailhol via B4 Relay wrote:
> >>> From: Lucas De Marchi <lucas.demarchi@intel.com>
> >>>
> >>> Now that include/linux/bits.h implements fixed-width GENMASK_U*(), use
> >>> them to implement the i915/xe specific macros. Converting each driver
> >>> to use the generic macros are left for later, when/if other
> >>> driver-specific macros are also generalized.
> >>>
> >>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> >>> Acked-by: Jani Nikula <jani.nikula@intel.com>
> >>> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >>> ---
> >>> Changelog:
> >>>
> >>>   v5 -> v6:
> >>>
> >>>     - No changes.
> >>>
> >>>   v4 -> v5:
> >>>
> >>>     - Add braket to macro names in patch description,
> >>>       e.g. 'REG_GENMASK*' -> 'REG_GENMASK*()'
> >>>
> >>>   v3 -> v4:
> >>>
> >>>     - Remove the prefixes in macro parameters,
> >>>       e.g. 'REG_GENMASK(__high, __low)' -> 'REG_GENMASK(high, low)'
> >>> ---
> >>>  drivers/gpu/drm/i915/i915_reg_defs.h | 108 ++++-------------------------------
> >>>  1 file changed, 11 insertions(+), 97 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_reg_defs.h b/drivers/gpu/drm/i915/i915_reg_defs.h
> >>> index e251bcc0c89f5710125bc70f07851b2cb978c89c..39e5ed9511174b8757b9201bff735fa362651b34 100644
> >>> --- a/drivers/gpu/drm/i915/i915_reg_defs.h
> >>> +++ b/drivers/gpu/drm/i915/i915_reg_defs.h
> >>> @@ -9,76 +9,19 @@
> >>>  #include <linux/bitfield.h>
> >>>  #include <linux/bits.h>
> >>>  
> >>> -/**
> >>> - * REG_BIT() - Prepare a u32 bit value
> >>> - * @__n: 0-based bit number
> >>> - *
> >>> - * Local wrapper for BIT() to force u32, with compile time checks.
> >>> - *
> >>> - * @return: Value with bit @__n set.
> >>> +/*
> >>> + * Wrappers over the generic BIT_* and GENMASK_* implementations,
> >>> + * for compatibility reasons with previous implementation
> >>>   */
> >>> -#define REG_BIT(__n)							\
> >>> -	((u32)(BIT(__n) +						\
> >>> -	       BUILD_BUG_ON_ZERO(__is_constexpr(__n) &&		\
> >>> -				 ((__n) < 0 || (__n) > 31))))
> >>> +#define REG_GENMASK(high, low)		GENMASK_U32(high, low)
> >>> +#define REG_GENMASK64(high, low)	GENMASK_U64(high, low)
> >>> +#define REG_GENMASK16(high, low)	GENMASK_U16(high, low)
> >>> +#define REG_GENMASK8(high, low)		GENMASK_U8(high, low)
> >>
> >> Nit. Maybe just
> >>
> >>  #define REG_GENMASK		GENMASK_U32
> > 
> > Please just keep it as it is for clarity.
> 
> I also prefer when the argument is clearly displayed. It shows at first
> glance that this is a function-like macro and reminds of the correct
> order of the argument without having to look at the definitions in
> bits.h. It also allows for people to grep "#define REG_GENMASK(" in
> order to find the macro definition.
> 
> To be honest, I don't have a strong opinion either, but because Jani
> also prefers it this way, I will keep as-is.

Please go with the original version. It was just a minor nitpick.