ove the implementation of REG_GENMASK/REG_BIT to a more appropriate
place to be shared by i915, xe and possibly other parts of the kernel.
For now this re-defines the old macros. In future we may start using the
new macros directly, but that's a more intrusive search-and-replace.
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.
v1: https://lore.kernel.org/intel-xe/[email protected]/
v2: https://lore.kernel.org/intel-xe/[email protected]
Lucas De Marchi (2):
bits: Introduce fixed-type BIT
drm/i915: Convert REG_GENMASK* to fixed-width GENMASK_*
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 | 51 ++++++++++---
3 files changed, 51 insertions(+), 109 deletions(-)
--
2.43.0
Implement fixed-type BIT() to help drivers add stricter checks, like was
done for GENMASK.
Signed-off-by: Lucas De Marchi <[email protected]>
Acked-by: Jani Nikula <[email protected]>
---
include/linux/bits.h | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/include/linux/bits.h b/include/linux/bits.h
index bd56f32de44e..811846ce110e 100644
--- a/include/linux/bits.h
+++ b/include/linux/bits.h
@@ -24,12 +24,16 @@
#define GENMASK_INPUT_CHECK(h, l) \
(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
__is_constexpr((l) > (h)), (l) > (h), 0)))
+#define BIT_INPUT_CHECK(type, b) \
+ ((BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
+ __is_constexpr(b), (b) >= BITS_PER_TYPE(type), 0))))
#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
+#define BIT_INPUT_CHECK(type, b) 0
#endif
/*
@@ -54,4 +58,17 @@
#define GENMASK_U32(h, l) __GENMASK(u32, h, l)
#define GENMASK_U64(h, l) __GENMASK(u64, h, l)
+/*
+ * Fixed-type variants of BIT(), with additional checks like __GENMASK(). The
+ * following examples generate compiler warnings due to shift-count-overflow:
+ *
+ * - BIT_U8(8)
+ * - BIT_U32(-1)
+ * - BIT_U32(40)
+ */
+#define BIT_U8(b) ((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b)))
+#define BIT_U16(b) ((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b)))
+#define BIT_U32(b) ((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b)))
+#define BIT_U64(b) ((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b)))
+
#endif /* __LINUX_BITS_H */
--
2.43.0
Now that include/linux/bits.h implements fixed-width GENMASK_*, 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 <[email protected]>
Acked-by: Jani Nikula <[email protected]>
---
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 a685db1e815d..52f99eb96f86 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.
- */
-#define REG_BIT(__n) \
- ((u32)(BIT(__n) + \
- BUILD_BUG_ON_ZERO(__is_constexpr(__n) && \
- ((__n) < 0 || (__n) > 31))))
-
-/**
- * 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.
+/*
+ * Wrappers over the generic BIT_* and GENMASK_* implementations,
+ * for compatibility reasons with previous implementation
*/
-#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)))))
+#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_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.43.0
On Wed, 07 Feb 2024, Lucas De Marchi <[email protected]> wrote:
> Now that include/linux/bits.h implements fixed-width GENMASK_*, 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 <[email protected]>
> Acked-by: Jani Nikula <[email protected]>
Thanks for doing this!
This is fine to merge via whichever tree you find best. The i915 parts
have very little conflict potential, there haven't been changes here in
a while.
BR,
Jani.
--
Jani Nikula, Intel
Hi Lucas,
looks good, just one idea...
..
> +#define BIT_U8(b) ((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b)))
> +#define BIT_U16(b) ((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b)))
> +#define BIT_U32(b) ((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b)))
> +#define BIT_U64(b) ((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b)))
considering that BIT defines are always referred to unsigned
types, I would just call them
#define BIT8
#define BIT16
#define BIT32
#define BIT64
what do you think?
Andi
Hi Lucas,
..
> +#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)
I was hoping to use directly GENMASK_U*() functions.
Andi
On Thu, Feb 08, 2024 at 09:04:45PM +0100, Andi Shyti wrote:
>Hi Lucas,
>
>looks good, just one idea...
>
>...
>
>> +#define BIT_U8(b) ((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b)))
>> +#define BIT_U16(b) ((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b)))
>> +#define BIT_U32(b) ((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b)))
>> +#define BIT_U64(b) ((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b)))
>
>considering that BIT defines are always referred to unsigned
>types, I would just call them
>
>#define BIT8
>#define BIT16
>#define BIT32
>#define BIT64
>
>what do you think?
it will clash with defines from other headers and not match the ones for
GENMASK so I prefer it the other way.
thanks
Lucas De Marchi
>
>Andi
On Thu, Feb 08, 2024 at 09:07:32PM +0100, Andi Shyti wrote:
>Hi Lucas,
>
>...
>
>> +#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)
>
>I was hoping to use directly GENMASK_U*() functions.
this would be something to be done on top on each drivers' trees, not
something to apply here.
Lucas De Marchi
>
>Andi
On Thu, 08 Feb 2024, Lucas De Marchi <[email protected]> wrote:
> On Thu, Feb 08, 2024 at 09:04:45PM +0100, Andi Shyti wrote:
>>Hi Lucas,
>>
>>looks good, just one idea...
>>
>>...
>>
>>> +#define BIT_U8(b) ((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b)))
>>> +#define BIT_U16(b) ((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b)))
>>> +#define BIT_U32(b) ((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b)))
>>> +#define BIT_U64(b) ((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b)))
>>
>>considering that BIT defines are always referred to unsigned
>>types, I would just call them
>>
>>#define BIT8
>>#define BIT16
>>#define BIT32
>>#define BIT64
>>
>>what do you think?
>
> it will clash with defines from other headers and not match the ones for
> GENMASK so I prefer it the other way.
Agreed.
--
Jani Nikula, Intel
On Wed, Feb 07, 2024 at 11:45:20PM -0800, Lucas De Marchi wrote:
> Implement fixed-type BIT() to help drivers add stricter checks, like was
> done for GENMASK.
>
> Signed-off-by: Lucas De Marchi <[email protected]>
> Acked-by: Jani Nikula <[email protected]>
So I get v1 from Jan.23 in my mailbox, and this one is v3. Did I miss
a v2? Anyways, please bear my reviewed-by from v1 for this patch.
Thanks,
Yury
> ---
> include/linux/bits.h | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/include/linux/bits.h b/include/linux/bits.h
> index bd56f32de44e..811846ce110e 100644
> --- a/include/linux/bits.h
> +++ b/include/linux/bits.h
> @@ -24,12 +24,16 @@
> #define GENMASK_INPUT_CHECK(h, l) \
> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> __is_constexpr((l) > (h)), (l) > (h), 0)))
> +#define BIT_INPUT_CHECK(type, b) \
> + ((BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> + __is_constexpr(b), (b) >= BITS_PER_TYPE(type), 0))))
> #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
> +#define BIT_INPUT_CHECK(type, b) 0
> #endif
>
> /*
> @@ -54,4 +58,17 @@
> #define GENMASK_U32(h, l) __GENMASK(u32, h, l)
> #define GENMASK_U64(h, l) __GENMASK(u64, h, l)
>
> +/*
> + * Fixed-type variants of BIT(), with additional checks like __GENMASK(). The
> + * following examples generate compiler warnings due to shift-count-overflow:
> + *
> + * - BIT_U8(8)
> + * - BIT_U32(-1)
> + * - BIT_U32(40)
> + */
> +#define BIT_U8(b) ((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b)))
> +#define BIT_U16(b) ((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b)))
> +#define BIT_U32(b) ((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b)))
> +#define BIT_U64(b) ((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b)))
> +
> #endif /* __LINUX_BITS_H */
> --
> 2.43.0
..
> >> +#define BIT_U8(b) ((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b)))
> >> +#define BIT_U16(b) ((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b)))
> >> +#define BIT_U32(b) ((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b)))
> >> +#define BIT_U64(b) ((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b)))
> >
> >considering that BIT defines are always referred to unsigned
> >types, I would just call them
Except that pretty much as soon as you breath on them
the u8 and u16 types get converted to int.
If you want them to be an unsigned type then you need
to cast them to (unsigned int).
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Fri, Feb 09, 2024 at 08:53:25AM -0800, Yury Norov wrote:
>On Wed, Feb 07, 2024 at 11:45:20PM -0800, Lucas De Marchi wrote:
>> Implement fixed-type BIT() to help drivers add stricter checks, like was
>> done for GENMASK.
>>
>> Signed-off-by: Lucas De Marchi <[email protected]>
>> Acked-by: Jani Nikula <[email protected]>
>
>So I get v1 from Jan.23 in my mailbox, and this one is v3. Did I miss
>a v2? Anyways, please bear my reviewed-by from v1 for this patch.
Jan 23 was actually the v2 and I missed the subject prefix.
My understanding was that you were going to apply this through some
bitmap tree, but checking MAINTAINERS now it seems there's no git tree
associated. So I will just add your r-b and merge this through
drm-xe.
thanks
Lucas De Marchi
>
>Thanks,
>Yury
>
>> ---
>> include/linux/bits.h | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/include/linux/bits.h b/include/linux/bits.h
>> index bd56f32de44e..811846ce110e 100644
>> --- a/include/linux/bits.h
>> +++ b/include/linux/bits.h
>> @@ -24,12 +24,16 @@
>> #define GENMASK_INPUT_CHECK(h, l) \
>> (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
>> __is_constexpr((l) > (h)), (l) > (h), 0)))
>> +#define BIT_INPUT_CHECK(type, b) \
>> + ((BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
>> + __is_constexpr(b), (b) >= BITS_PER_TYPE(type), 0))))
>> #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
>> +#define BIT_INPUT_CHECK(type, b) 0
>> #endif
>>
>> /*
>> @@ -54,4 +58,17 @@
>> #define GENMASK_U32(h, l) __GENMASK(u32, h, l)
>> #define GENMASK_U64(h, l) __GENMASK(u64, h, l)
>>
>> +/*
>> + * Fixed-type variants of BIT(), with additional checks like __GENMASK(). The
>> + * following examples generate compiler warnings due to shift-count-overflow:
>> + *
>> + * - BIT_U8(8)
>> + * - BIT_U32(-1)
>> + * - BIT_U32(40)
>> + */
>> +#define BIT_U8(b) ((u8)(BIT_INPUT_CHECK(u8, b) + BIT(b)))
>> +#define BIT_U16(b) ((u16)(BIT_INPUT_CHECK(u16, b) + BIT(b)))
>> +#define BIT_U32(b) ((u32)(BIT_INPUT_CHECK(u32, b) + BIT(b)))
>> +#define BIT_U64(b) ((u64)(BIT_INPUT_CHECK(u64, b) + BIT(b)))
>> +
>> #endif /* __LINUX_BITS_H */
>> --
>> 2.43.0
On Mon, Feb 19, 2024 at 11:13:57PM -0600, Lucas De Marchi wrote:
> On Fri, Feb 09, 2024 at 08:53:25AM -0800, Yury Norov wrote:
> > On Wed, Feb 07, 2024 at 11:45:20PM -0800, Lucas De Marchi wrote:
> > > Implement fixed-type BIT() to help drivers add stricter checks, like was
> > > done for GENMASK.
> > >
> > > Signed-off-by: Lucas De Marchi <[email protected]>
> > > Acked-by: Jani Nikula <[email protected]>
> >
> > So I get v1 from Jan.23 in my mailbox, and this one is v3. Did I miss
> > a v2? Anyways, please bear my reviewed-by from v1 for this patch.
>
> Jan 23 was actually the v2 and I missed the subject prefix.
>
> My understanding was that you were going to apply this through some
> bitmap tree, but checking MAINTAINERS now it seems there's no git tree
> associated. So I will just add your r-b and merge this through
> drm-xe.
I've got a bitmap-related branch. I can move this series in there if
you prefer. At your discretion.
https://github.com/norov/linux/tree/bitmap_for_next
Thanks,
Yury
On Thu, Feb 22, 2024 at 06:51:52AM -0800, Yury Norov wrote:
>On Mon, Feb 19, 2024 at 11:13:57PM -0600, Lucas De Marchi wrote:
>> On Fri, Feb 09, 2024 at 08:53:25AM -0800, Yury Norov wrote:
>> > On Wed, Feb 07, 2024 at 11:45:20PM -0800, Lucas De Marchi wrote:
>> > > Implement fixed-type BIT() to help drivers add stricter checks, like was
>> > > done for GENMASK.
>> > >
>> > > Signed-off-by: Lucas De Marchi <[email protected]>
>> > > Acked-by: Jani Nikula <[email protected]>
>> >
>> > So I get v1 from Jan.23 in my mailbox, and this one is v3. Did I miss
>> > a v2? Anyways, please bear my reviewed-by from v1 for this patch.
>>
>> Jan 23 was actually the v2 and I missed the subject prefix.
>>
>> My understanding was that you were going to apply this through some
>> bitmap tree, but checking MAINTAINERS now it seems there's no git tree
>> associated. So I will just add your r-b and merge this through
>> drm-xe.
>
>I've got a bitmap-related branch. I can move this series in there if
>you prefer. At your discretion.
>
>https://github.com/norov/linux/tree/bitmap_for_next
yeah, sounds good.
thanks
Lucas De Marchi
>
>Thanks,
>Yury