[PATCH v5 0/7] bits: Fixed-type GENMASK()/BIT()

[PATCH v5 0/7] bits: Fixed-type GENMASK()/BIT()

[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 an 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_t(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_t(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.

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 a defconfig)

--
2.43.0

---
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                 |  77 ++++++++++++++++++-------
 lib/test_bits.c                      |  47 +++++++++++++++
 4 files changed, 113 insertions(+), 120 deletions(-)
---
base-commit: 0312e94abe484b9ee58c32d2f8ba177e04955b35
change-id: 20250228-fixed-type-genmasks-8d1a555f34e8

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

[PATCH v5 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:

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

diff --git a/include/linux/bits.h b/include/linux/bits.h
index 14fd0ca9a6cd17339dd2f69e449558312a8a001b..4819cbe7bd48fbae796fc6005c9f92b1c93a034c 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,12 @@
  */
 #define GENMASK_U128(h, l) \
 	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
-#endif
+
+#else /* defined(__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

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

[Andy Shevchenko]

On Thu, Mar 06, 2025 at 08:29:52PM +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.
> - */

I believe this comment is still valid...

> +#else /* defined(__ASSEMBLY__) */


...here.

Otherwise justify its removal in the commit message.

> +#define GENMASK(h, l)		__GENMASK(h, l)
> +#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)
> +
> +#endif /* !defined(__ASSEMBLY__) */

-- 
With Best Regards,
Andy Shevchenko

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

[Vincent Mailhol]

On 06/03/2025 at 22:05, Andy Shevchenko wrote:
> On Thu, Mar 06, 2025 at 08:29:52PM +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.
>> - */
> 
> I believe this comment is still valid...
> 
>> +#else /* defined(__ASSEMBLY__) */
> 
> 
> ...here.
> 
> Otherwise justify its removal in the commit message.

OK. I will restore the comment in v6, but will move it to the #else
branch, like this:

  #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__) */

>> +#define GENMASK(h, l)		__GENMASK(h, l)
>> +#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)
>> +
>> +#endif /* !defined(__ASSEMBLY__) */
> 

Yours sincerely,
Vincent Mailhol

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

[Andy Shevchenko]

On Fri, Mar 07, 2025 at 12:07:45AM +0900, Vincent Mailhol wrote:
> On 06/03/2025 at 22:05, Andy Shevchenko wrote:
> > On Thu, Mar 06, 2025 at 08:29:52PM +0900, Vincent Mailhol via B4 Relay wrote:
> >> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

...

> >> -/*
> >> - * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> >> - * disable the input check if that is the case.
> >> - */
> > 
> > I believe this comment is still valid...
> > 
> >> +#else /* defined(__ASSEMBLY__) */
> > 
> > 
> > ...here.
> > 
> > Otherwise justify its removal in the commit message.
> 
> OK. I will restore the comment in v6, but will move it to the #else
> branch,

Isn't it what I suggested? :-)

> like this:

>   #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__) */
> 
> >> +#define GENMASK(h, l)		__GENMASK(h, l)
> >> +#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)
> >> +
> >> +#endif /* !defined(__ASSEMBLY__) */

-- 
With Best Regards,
Andy Shevchenko

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

[Lucas De Marchi]

On Thu, Mar 06, 2025 at 08:29:52PM +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.
>
>Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>---
>Changelog:
>
>  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 | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
>diff --git a/include/linux/bits.h b/include/linux/bits.h
>index 14fd0ca9a6cd17339dd2f69e449558312a8a001b..4819cbe7bd48fbae796fc6005c9f92b1c93a034c 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.
>- */

it seems we now have 1 inconsistency that we comment why
GENMASK_U128() is not available in asm, but we don't comment why
GENMASK_INPUT_CHECK() is not available there. Maybe move this comment on
top of GENMASK_INPUT_CHECK().

Anyway,

	Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks for picking up this series.

Lucas De Marchi

>-#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,12 @@
>  */
> #define GENMASK_U128(h, l) \
> 	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
>-#endif
>+
>+#else /* defined(__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
>
>

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

[Vincent Mailhol]

On 06/03/2025 at 23:34, Lucas De Marchi wrote:
> On Thu, Mar 06, 2025 at 08:29:52PM +0900, Vincent Mailhol via B4 Relay
> wrote:

(...)

> it seems we now have 1 inconsistency that we comment why
> GENMASK_U128() is not available in asm, but we don't comment why
> GENMASK_INPUT_CHECK() is not available there. Maybe move this comment on
> top of GENMASK_INPUT_CHECK().

I will restore the comment in v6 and put it next to the asm definition,
c.f. my reply to Andy.

> Anyway,
> 
>     Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

Is this only valid for the first patch or for the full series? If this
is for the full series, would you mind replying to the cover letter with
your review tag?

> thanks for picking up this series.

You are welcome!

> Lucas De Marchi

(...)

Yours sincerely,
Vincent Mailhol

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

[Lucas De Marchi]

On Fri, Mar 07, 2025 at 02:10:28AM +0900, Vincent Mailhol wrote:
>On 06/03/2025 at 23:34, Lucas De Marchi wrote:
>> On Thu, Mar 06, 2025 at 08:29:52PM +0900, Vincent Mailhol via B4 Relay
>> wrote:
>
>(...)
>
>> it seems we now have 1 inconsistency that we comment why
>> GENMASK_U128() is not available in asm, but we don't comment why
>> GENMASK_INPUT_CHECK() is not available there. Maybe move this comment on
>> top of GENMASK_INPUT_CHECK().
>
>I will restore the comment in v6 and put it next to the asm definition,
>c.f. my reply to Andy.
>
>> Anyway,
>>
>>     Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
>
>Is this only valid for the first patch or for the full series? If this
>is for the full series, would you mind replying to the cover letter with
>your review tag?

only for this patch. I'm the author of some of the patches and also add
my s-o-b in others. I'm not sure what b4 is going to do with those - it
would be weird if it added a r-b on my own patch.

Now I added r-b in some and comments in another.

thanks
Lucas De Marchi

>
>> thanks for picking up this series.
>
>You are welcome!
>
>> Lucas De Marchi
>
>(...)
>
>Yours sincerely,
>Vincent Mailhol
>

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

[Vincent Mailhol]

On 13/03/2025 at 13:16, Lucas De Marchi wrote:
> On Fri, Mar 07, 2025 at 02:10:28AM +0900, Vincent Mailhol wrote:
>> On 06/03/2025 at 23:34, Lucas De Marchi wrote:
>>> On Thu, Mar 06, 2025 at 08:29:52PM +0900, Vincent Mailhol via B4 Relay
>>> wrote:
>>
>> (...)
>>
>>> it seems we now have 1 inconsistency that we comment why
>>> GENMASK_U128() is not available in asm, but we don't comment why
>>> GENMASK_INPUT_CHECK() is not available there. Maybe move this comment on
>>> top of GENMASK_INPUT_CHECK().
>>
>> I will restore the comment in v6 and put it next to the asm definition,
>> c.f. my reply to Andy.
>>
>>> Anyway,
>>>
>>>     Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>
>> Is this only valid for the first patch or for the full series? If this
>> is for the full series, would you mind replying to the cover letter with
>> your review tag?
> 
> only for this patch. I'm the author of some of the patches and also add
> my s-o-b in others. I'm not sure what b4 is going to do with those - it
> would be weird if it added a r-b on my own patch.

Because I added some modification since, I think it wouldn't be so
problematic. But it also makes sense to just add your review on the new
patches. So let do as you suggested.

> Now I added r-b in some and comments in another.

Thanks! I applied the tags locally and answered your comment.


Yours sincerely,
Vincent Mailhol

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

[David Laight]

On Thu, 06 Mar 2025 20:29:52 +0900
Vincent Mailhol via B4 Relay <devnull+mailhol.vincent.wanadoo.fr@kernel.org> 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.
...
> +#else /* defined(__ASSEMBLY__) */
> +
> +#define GENMASK(h, l)		__GENMASK(h, l)
> +#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)

What do those actually expand to now?
As I've said a few times both UL(0) and ULL(0) are just (0) for __ASSEMBLY__
so the expansions of __GENMASK() and __GENMASK_ULL() contained the
same numeric constants.
This means they should be generating the same values.
I don't know the correct 'sizeof (int_type)' for the shift right of ~0.
My suspicion is that a 32bit assembler used 32bit signed integers and a
64bit one 64bit signed integers (but a 32bit asm on a 64bit host might
be 64bit).
So the asm versions need to avoid the right shift and only do left shifts.

Which probably means they need to be enirely separate from the C versions.
And then the C ones can have all the ULL() removed.

	David

> +
> +#endif /* !defined(__ASSEMBLY__) */
>  
>  #endif	/* __LINUX_BITS_H */
> 

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

[Vincent Mailhol]

On 07/03/2025 at 04:23, David Laight wrote:
> On Thu, 06 Mar 2025 20:29:52 +0900
> Vincent Mailhol via B4 Relay <devnull+mailhol.vincent.wanadoo.fr@kernel.org> 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.
> ...
>> +#else /* defined(__ASSEMBLY__) */
>> +
>> +#define GENMASK(h, l)		__GENMASK(h, l)
>> +#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)
> 
> What do those actually expand to now?
> As I've said a few times both UL(0) and ULL(0) are just (0) for __ASSEMBLY__
> so the expansions of __GENMASK() and __GENMASK_ULL() contained the
> same numeric constants.

Indeed, in asm, the UL(0) and ULL(0) expands to the same thing: 0.

But the two macros still expand to something different on 32 bits
architectures:

  * __GENMASK:

      (((~(0)) << (l)) & (~(0) >> (32 - 1 - (h))))

  * __GENMASK_ULL:

      (((~(0)) << (l)) & (~(0) >> (64 - 1 - (h))))

On 64 bits architecture these are the same.

> This means they should be generating the same values.
> I don't know the correct 'sizeof (int_type)' for the shift right of ~0.
> My suspicion is that a 32bit assembler used 32bit signed integers and a
> 64bit one 64bit signed integers (but a 32bit asm on a 64bit host might
> be 64bit).
> So the asm versions need to avoid the right shift and only do left shifts.
> 
> Which probably means they need to be enirely separate from the C versions.
> And then the C ones can have all the ULL() removed.

In this v5, I already have the ULL() removed from the non-uapi C
version. And we are left with two distinct variants:

  - the uapi C & asm
  - the non-uapi C (including fix width)

For the uapi ones, I do not think we can modify it without a risk of
breaking some random userland. At least, this is not a risk I will take.
And if we have to keep the __GENMASK() and __GENMASK_ULL(), then I would
rather just reuse these for the asm variant instead of splitting further
more and finding ourselves with three variants:

  - the uapi C
  - the asm
  - the non-uapi C (including fix width)

If __GENMASK() and __GENMASK_ULL() were not in the uapi, I would have
agreed with you.

If you believe that the risk of modifying the uapi GENMASK*() is low
enough, then you can submit a patch. But I will definitely not touch
these myself.


Yours sincerely,
Vincent Mailhol

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

[David Laight]

On Fri, 7 Mar 2025 18:58:08 +0900
Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:

> On 07/03/2025 at 04:23, David Laight wrote:
> > On Thu, 06 Mar 2025 20:29:52 +0900
> > Vincent Mailhol via B4 Relay <devnull+mailhol.vincent.wanadoo.fr@kernel.org> 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.  
> > ...  
> >> +#else /* defined(__ASSEMBLY__) */
> >> +
> >> +#define GENMASK(h, l)		__GENMASK(h, l)
> >> +#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)  
> > 
> > What do those actually expand to now?
> > As I've said a few times both UL(0) and ULL(0) are just (0) for __ASSEMBLY__
> > so the expansions of __GENMASK() and __GENMASK_ULL() contained the
> > same numeric constants.  
> 
> Indeed, in asm, the UL(0) and ULL(0) expands to the same thing: 0.
> 
> But the two macros still expand to something different on 32 bits
> architectures:
> 
>   * __GENMASK:
> 
>       (((~(0)) << (l)) & (~(0) >> (32 - 1 - (h))))
> 
>   * __GENMASK_ULL:
> 
>       (((~(0)) << (l)) & (~(0) >> (64 - 1 - (h))))
> 
> On 64 bits architecture these are the same.

If the assembler is naive and uses the cpu shift instruction for the >>
then a lot of cpu (including all x86 since the 286) mask off the high bits.
So __GENMASK_ULL() may well generate the expected pattern - provided it
is 32bits wide.

> 
> > This means they should be generating the same values.
> > I don't know the correct 'sizeof (int_type)' for the shift right of ~0.
> > My suspicion is that a 32bit assembler used 32bit signed integers and a
> > 64bit one 64bit signed integers (but a 32bit asm on a 64bit host might
> > be 64bit).
> > So the asm versions need to avoid the right shift and only do left shifts.
> > 
> > Which probably means they need to be enirely separate from the C versions.
> > And then the C ones can have all the ULL() removed.  
> 
> In this v5, I already have the ULL() removed from the non-uapi C
> version. And we are left with two distinct variants:
> 
>   - the uapi C & asm
>   - the non-uapi C (including fix width)
> 
> For the uapi ones, I do not think we can modify it without a risk of
> breaking some random userland. At least, this is not a risk I will take.
> And if we have to keep the __GENMASK() and __GENMASK_ULL(), then I would
> rather just reuse these for the asm variant instead of splitting further
> more and finding ourselves with three variants:
> 
>   - the uapi C
>   - the asm
>   - the non-uapi C (including fix width)
> 
> If __GENMASK() and __GENMASK_ULL() were not in the uapi, I would have
> agreed with you.
> 
> If you believe that the risk of modifying the uapi GENMASK*() is low
> enough, then you can submit a patch. But I will definitely not touch
> these myself.

I don't think you'll break userspace by stopping the uapi .h working
for asm files (with __ASSEMBLER__ defined).

But someone else might have a different opinion.

> 
> 
> Yours sincerely,
> Vincent Mailhol
> 

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

[Vincent Mailhol]

On 07/03/2025 at 22:27, David Laight wrote:
> On Fri, 7 Mar 2025 18:58:08 +0900
> Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
> 
>> On 07/03/2025 at 04:23, David Laight wrote:
>>> On Thu, 06 Mar 2025 20:29:52 +0900
>>> Vincent Mailhol via B4 Relay <devnull+mailhol.vincent.wanadoo.fr@kernel.org> wrote:
>>>   
>>>> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

(...)

>>>> +#define GENMASK(h, l)		__GENMASK(h, l)
>>>> +#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)  
>>>
>>> What do those actually expand to now?
>>> As I've said a few times both UL(0) and ULL(0) are just (0) for __ASSEMBLY__
>>> so the expansions of __GENMASK() and __GENMASK_ULL() contained the
>>> same numeric constants.  
>>
>> Indeed, in asm, the UL(0) and ULL(0) expands to the same thing: 0.
>>
>> But the two macros still expand to something different on 32 bits
>> architectures:
>>
>>   * __GENMASK:
>>
>>       (((~(0)) << (l)) & (~(0) >> (32 - 1 - (h))))
>>
>>   * __GENMASK_ULL:
>>
>>       (((~(0)) << (l)) & (~(0) >> (64 - 1 - (h))))
>>
>> On 64 bits architecture these are the same.
> 
> If the assembler is naive and uses the cpu shift instruction for the >>
> then a lot of cpu (including all x86 since the 286) mask off the high bits.
> So __GENMASK_ULL() may well generate the expected pattern - provided it
> is 32bits wide.

"If", "may", that's still a lot of conditionals in your sentence :)

I do not have enough knowledge to prove or disprove this, but what I am
sure of is that this uncertainty tells me that this is not something I
want to touch myself.

I picked up this stalled fixed width patch series and re-spinned it
because I had confidence here. I do not want to extend this series with
some asm clean-up which looks uncertain to me. I am not telling you are
wrong but just that I will happily delegate this to whoever has more
confidence than me!

>>> This means they should be generating the same values.
>>> I don't know the correct 'sizeof (int_type)' for the shift right of ~0.
>>> My suspicion is that a 32bit assembler used 32bit signed integers and a
>>> 64bit one 64bit signed integers (but a 32bit asm on a 64bit host might
>>> be 64bit).
>>> So the asm versions need to avoid the right shift and only do left shifts.
>>>
>>> Which probably means they need to be enirely separate from the C versions.
>>> And then the C ones can have all the ULL() removed.  
>>
>> In this v5, I already have the ULL() removed from the non-uapi C
>> version. And we are left with two distinct variants:
>>
>>   - the uapi C & asm
>>   - the non-uapi C (including fix width)
>>
>> For the uapi ones, I do not think we can modify it without a risk of
>> breaking some random userland. At least, this is not a risk I will take.
>> And if we have to keep the __GENMASK() and __GENMASK_ULL(), then I would
>> rather just reuse these for the asm variant instead of splitting further
>> more and finding ourselves with three variants:
>>
>>   - the uapi C
>>   - the asm
>>   - the non-uapi C (including fix width)
>>
>> If __GENMASK() and __GENMASK_ULL() were not in the uapi, I would have
>> agreed with you.
>>
>> If you believe that the risk of modifying the uapi GENMASK*() is low
>> enough, then you can submit a patch. But I will definitely not touch
>> these myself.
> 
> I don't think you'll break userspace by stopping the uapi .h working
> for asm files (with __ASSEMBLER__ defined).
> 
> But someone else might have a different opinion.

How do you know that there isn't someone somewhere who is using the uapi
__GENMASK*() in their userland asm code? Wouldn't such code break?

You may argue that the uapi is not meant to be used that way, but at the
end, we are not supposed to make assumptions on how the uapi code will
be used. Once it is exposed, if something break because the uapi was not
used in the intended way, it is still the kernel fault, not the userland.


Yours sincerely,
Vincent Mailhol

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

[David Laight]

On Fri, 7 Mar 2025 18:58:08 +0900
Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:

> On 07/03/2025 at 04:23, David Laight wrote:
> > On Thu, 06 Mar 2025 20:29:52 +0900
> > Vincent Mailhol via B4 Relay <devnull+mailhol.vincent.wanadoo.fr@kernel.org> 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.  
> > ...  
> >> +#else /* defined(__ASSEMBLY__) */
> >> +
> >> +#define GENMASK(h, l)		__GENMASK(h, l)
> >> +#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)  
> > 
> > What do those actually expand to now?
> > As I've said a few times both UL(0) and ULL(0) are just (0) for __ASSEMBLY__
> > so the expansions of __GENMASK() and __GENMASK_ULL() contained the
> > same numeric constants.  
> 
> Indeed, in asm, the UL(0) and ULL(0) expands to the same thing: 0.
> 
> But the two macros still expand to something different on 32 bits
> architectures:
> 
>   * __GENMASK:
> 
>       (((~(0)) << (l)) & (~(0) >> (32 - 1 - (h))))
> 
>   * __GENMASK_ULL:
> 
>       (((~(0)) << (l)) & (~(0) >> (64 - 1 - (h))))
> 
> On 64 bits architecture these are the same.

I've just fed those into godbolt (https://www.godbolt.org/z/Ter6WE9qE) as:
int fi(void)
{
    int v;
    asm("mov $(((~(0)) << (8)) & (~(0) >> (32 - 1 - (15)))),%0": "=r" (v));
    return v -(((~(0u)) << (8)) & (~(0u) >> (32 - 1 - (15))));
}

gas warns:
<source>:4: Warning: 0xffffffffff00 shortened to 0xffffff00

unsigned long long fll(void)
{
    unsigned long long v;
    asm("mov $(((~(0)) << (8)) & (~(0) >> (64 - 1 - (15)))),%0": "=r" (v));
    return v -(((~(0ull)) << (8)) & (~(0ull) >> (64 - 1 - (15))));
}

(for other architectures you'll need to change the opcode)

For x86 and x86-32 the assembler seems to be doing 64bit maths with unsigned
right shifts - so the second function (with the 64 in it) generates 0xff00.
I doubt a 32bit only assembler does 64bit maths, but the '>> 48' above
might get masked to a '>> 16' by the cpu and generate the correct result.

So __GENMASK() is likely to be broken for any assembler that supports 64bits
when generating 32bit code.
__GENMASK_ULL() works (assuming all have unsigned >>) on 64bit assemblers
(even when generating 32bit code). It may work on some 32bit assemblers.

Since most uses in the header files will be GENMASK() I doubt (hope) no
asm code actually uses the values!
The headers assemble - but that is about all that can be said.

Bags of worms :-)

	David

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

[David Laight]

On Sun, 9 Mar 2025 01:58:53 +0000
David Laight <david.laight.linux@gmail.com> wrote:

> On Fri, 7 Mar 2025 18:58:08 +0900
> Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
> 
> > On 07/03/2025 at 04:23, David Laight wrote:  
> > > On Thu, 06 Mar 2025 20:29:52 +0900
> > > Vincent Mailhol via B4 Relay <devnull+mailhol.vincent.wanadoo.fr@kernel.org> 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.    
> > > ...    
> > >> +#else /* defined(__ASSEMBLY__) */
> > >> +
> > >> +#define GENMASK(h, l)		__GENMASK(h, l)
> > >> +#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)    
> > > 
> > > What do those actually expand to now?
> > > As I've said a few times both UL(0) and ULL(0) are just (0) for __ASSEMBLY__
> > > so the expansions of __GENMASK() and __GENMASK_ULL() contained the
> > > same numeric constants.    
> > 
> > Indeed, in asm, the UL(0) and ULL(0) expands to the same thing: 0.
> > 
> > But the two macros still expand to something different on 32 bits
> > architectures:
> > 
> >   * __GENMASK:
> > 
> >       (((~(0)) << (l)) & (~(0) >> (32 - 1 - (h))))
> > 
> >   * __GENMASK_ULL:
> > 
> >       (((~(0)) << (l)) & (~(0) >> (64 - 1 - (h))))
> > 
> > On 64 bits architecture these are the same.  
> 
> I've just fed those into godbolt (https://www.godbolt.org/z/Ter6WE9qE) as:
> int fi(void)
> {
>     int v;
>     asm("mov $(((~(0)) << (8)) & (~(0) >> (32 - 1 - (15)))),%0": "=r" (v));
>     return v -(((~(0u)) << (8)) & (~(0u) >> (32 - 1 - (15))));
> }
> 
> gas warns:
> <source>:4: Warning: 0xffffffffff00 shortened to 0xffffff00
> 
> unsigned long long fll(void)
> {
>     unsigned long long v;
>     asm("mov $(((~(0)) << (8)) & (~(0) >> (64 - 1 - (15)))),%0": "=r" (v));
>     return v -(((~(0ull)) << (8)) & (~(0ull) >> (64 - 1 - (15))));
> }
> 
> (for other architectures you'll need to change the opcode)
> 
> For x86 and x86-32 the assembler seems to be doing 64bit maths with unsigned
> right shifts - so the second function (with the 64 in it) generates 0xff00.
> I doubt a 32bit only assembler does 64bit maths, but the '>> 48' above
> might get masked to a '>> 16' by the cpu and generate the correct result.
> 
> So __GENMASK() is likely to be broken for any assembler that supports 64bits
> when generating 32bit code.
> __GENMASK_ULL() works (assuming all have unsigned >>) on 64bit assemblers
> (even when generating 32bit code). It may work on some 32bit assemblers.

I've remembered my 'pi' has a 32bit userspace (on a 64bit kernel).
I quick test of "mov %0,#(...)" and bits 11..8 gives the correct output
for size '32' but the error message:
/tmp/ccPB7bWh.s:26: Warning: shift count out of range (56 is not between 0 and 31)
with size '64'.

Assuming that part of the gnu assembler is consistent across architectures
you can't use either GENMASK in asm for 32bit architectures.

Any change (probably including removing the asm support for the uapi) isn't
going to make things worse!

	David

> 
> Since most uses in the header files will be GENMASK() I doubt (hope) no
> asm code actually uses the values!
> The headers assemble - but that is about all that can be said.
> 
> Bags of worms :-)
> 
> 	David
> 

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

[Vincent Mailhol]

On 09/03/2025 at 19:23, David Laight wrote:
> On Sun, 9 Mar 2025 01:58:53 +0000
> David Laight <david.laight.linux@gmail.com> wrote:
> 
>> On Fri, 7 Mar 2025 18:58:08 +0900
>> Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
>>
>>> On 07/03/2025 at 04:23, David Laight wrote:  
>>>> On Thu, 06 Mar 2025 20:29:52 +0900
>>>> Vincent Mailhol via B4 Relay <devnull+mailhol.vincent.wanadoo.fr@kernel.org> 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.    
>>>> ...    
>>>>> +#else /* defined(__ASSEMBLY__) */
>>>>> +
>>>>> +#define GENMASK(h, l)		__GENMASK(h, l)
>>>>> +#define GENMASK_ULL(h, l)	__GENMASK_ULL(h, l)    
>>>>
>>>> What do those actually expand to now?
>>>> As I've said a few times both UL(0) and ULL(0) are just (0) for __ASSEMBLY__
>>>> so the expansions of __GENMASK() and __GENMASK_ULL() contained the
>>>> same numeric constants.    
>>>
>>> Indeed, in asm, the UL(0) and ULL(0) expands to the same thing: 0.
>>>
>>> But the two macros still expand to something different on 32 bits
>>> architectures:
>>>
>>>   * __GENMASK:
>>>
>>>       (((~(0)) << (l)) & (~(0) >> (32 - 1 - (h))))
>>>
>>>   * __GENMASK_ULL:
>>>
>>>       (((~(0)) << (l)) & (~(0) >> (64 - 1 - (h))))
>>>
>>> On 64 bits architecture these are the same.  
>>
>> I've just fed those into godbolt (https://www.godbolt.org/z/Ter6WE9qE) as:
>> int fi(void)
>> {
>>     int v;
>>     asm("mov $(((~(0)) << (8)) & (~(0) >> (32 - 1 - (15)))),%0": "=r" (v));
>>     return v -(((~(0u)) << (8)) & (~(0u) >> (32 - 1 - (15))));
>> }
>>
>> gas warns:
>> <source>:4: Warning: 0xffffffffff00 shortened to 0xffffff00
>>
>> unsigned long long fll(void)
>> {
>>     unsigned long long v;
>>     asm("mov $(((~(0)) << (8)) & (~(0) >> (64 - 1 - (15)))),%0": "=r" (v));
>>     return v -(((~(0ull)) << (8)) & (~(0ull) >> (64 - 1 - (15))));
>> }
>>
>> (for other architectures you'll need to change the opcode)
>>
>> For x86 and x86-32 the assembler seems to be doing 64bit maths with unsigned
>> right shifts - so the second function (with the 64 in it) generates 0xff00.
>> I doubt a 32bit only assembler does 64bit maths, but the '>> 48' above
>> might get masked to a '>> 16' by the cpu and generate the correct result.
>>
>> So __GENMASK() is likely to be broken for any assembler that supports 64bits
>> when generating 32bit code.
>> __GENMASK_ULL() works (assuming all have unsigned >>) on 64bit assemblers
>> (even when generating 32bit code). It may work on some 32bit assemblers.>
> I've remembered my 'pi' has a 32bit userspace (on a 64bit kernel).
> I quick test of "mov %0,#(...)" and bits 11..8 gives the correct output
> for size '32' but the error message:
> /tmp/ccPB7bWh.s:26: Warning: shift count out of range (56 is not between 0 and 31)
> with size '64'.
> 
> Assuming that part of the gnu assembler is consistent across architectures
> you can't use either GENMASK in asm for 32bit architectures.
> 
> Any change (probably including removing the asm support for the uapi) isn't
> going to make things worse!
> 
> 	David
> 
>>
>> Since most uses in the header files will be GENMASK() I doubt (hope) no
>> asm code actually uses the values!

After reading your message, I wanted to understand where these
GENMASK*() were used in asm code. It happens that most of the
architectures simply don't use it!

I see these are using in aarch64, but that's about it.

I couldn't find any use of neither GENMASK() nor GENMASK_ULL() in x86,
arm 32 bits, m68k or powerpc asm code (I did not check the other
architectures).

aarch64 uses both the long and long long variants, but this being a 64
bits architecture, these are the same. So OK.

So, this goes into the same direction as you: I guess that the fact that
no one noticed the issue is that no one uses this on a 32 bits arch,
probably for historical reasons, i.e. the asm code was written before
the introduction of the GENMASK() macros.

>> The headers assemble - but that is about all that can be said.
>>
>> Bags of worms :-)

+1 (I will not open that bag)

I think that the asm and non asm variant should have used a different
implementation from the begining. By wanting to have a single variant
that fit both the asm and the C code, we have a complex result that is
hard to understand and maintain (c.f. the bug which you pointed out
which have been unnoticed for ever).

But now that it is in the uapi, I am not sure of what is the best
approach. I sincerely hope that we can just modify it with no userland
impact.

Well, just to make it clear, I am not touching the asm part. I
acknowledge the issue, but I do not think that I have the skill to fix it.


Yours sincerely,
Vincent Mailhol

[PATCH v5 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:

  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   | 47 +++++++++++++++++++++++++++++++----------------
 2 files changed, 31 insertions(+), 17 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 4819cbe7bd48fbae796fc6005c9f92b1c93a034c..74219521a56e2639ccff7fdc899d6805ee355a0c 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__)
 
+/*
+ * 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.
+ */
+
 #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)))
 
-#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
+ * 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_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(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_U128(h, l) \
-	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
+#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

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

[Andy Shevchenko]

On Thu, Mar 06, 2025 at 08:29:53PM +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)))))
>         |                           ^~

Code LGTM, but just to be sure: you prepared your series using histogram
diff algo, right?

-- 
With Best Regards,
Andy Shevchenko

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

[Vincent Mailhol]

On 06/03/2025 à 22:08, Andy Shevchenko wrote:
> On Thu, Mar 06, 2025 at 08:29:53PM +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)))))
>>         |                           ^~
> 
> Code LGTM

Does this mean I get your Reviewed-by tag? Or will you wait the v6 to
formally give it?

> but just to be sure: you prepared your series using histogram
> diff algo, right?

No, I never used the histogram diff. My git config is extremely boring.
Mostly vanilla.

I remember that Linus even commented on this:

https://lore.kernel.org/all/CAHk-=wiUxm-NZ1si8dXWVTTJ9n3c+1SRTC0V+Lk7hOE4bDVwJQ@mail.gmail.com/

But he made it clear this was *not* a requirement, so I just left the
diff algorithm to the default. Or did I miss any communication that
contributors should now use histogram diff?

Regardless, I do not mind activating it. I just did a:

  git config diff.algorithm histogram

The v6 will have histogram diffs.


Yours sincerely,
Vincent Mailhol

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

[Andy Shevchenko]

On Fri, Mar 07, 2025 at 01:08:01AM +0900, Vincent Mailhol wrote:
> On 06/03/2025 à 22:08, Andy Shevchenko wrote:
> > On Thu, Mar 06, 2025 at 08:29:53PM +0900, Vincent Mailhol via B4 Relay wrote:

...

> Does this mean I get your Reviewed-by tag? Or will you wait the v6 to
> formally give it?

I'll wait for v6.

> > but just to be sure: you prepared your series using histogram
> > diff algo, right?
> 
> No, I never used the histogram diff. My git config is extremely boring.
> Mostly vanilla.
> 
> I remember that Linus even commented on this:
> 
> https://lore.kernel.org/all/CAHk-=wiUxm-NZ1si8dXWVTTJ9n3c+1SRTC0V+Lk7hOE4bDVwJQ@mail.gmail.com/
> 
> But he made it clear this was *not* a requirement, so I just left the
> diff algorithm to the default. Or did I miss any communication that
> contributors should now use histogram diff?

Use your common sense, i.e. look at the result and evaluate which one is more
readable. My guts are telling me that histogram will be slightly better here.

> Regardless, I do not mind activating it. I just did a:
> 
>   git config diff.algorithm histogram
> 
> The v6 will have histogram diffs.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 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 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:

  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 74219521a56e2639ccff7fdc899d6805ee355a0c..f95e7815cb18636cc47ac17ef66d1bd6668e6819 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__) */
 
 #define GENMASK(h, l)		__GENMASK(h, l)

-- 
2.45.3

[PATCH v5 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:

  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.
- */
-#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.45.3

[PATCH v5 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.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 lib/test_bits.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/lib/test_bits.c b/lib/test_bits.c
index c7b38d91e1f16d42b7ca92e62fbd6c19b37e76a0..dc93ded9fdb201e0d44b3c1cd71e233fd62258a5 100644
--- a/lib/test_bits.c
+++ b/lib/test_bits.c
@@ -7,6 +7,22 @@
 #include <linux/bits.h>
 
 
+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)
 {
 	KUNIT_EXPECT_EQ(test, 1ul, GENMASK(0, 0));
@@ -93,6 +109,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

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

[Lucas De Marchi]

On Thu, Mar 06, 2025 at 08:29:56PM +0900, Vincent Mailhol via B4 Relay wrote:
>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.
>
>Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>---
> lib/test_bits.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
>diff --git a/lib/test_bits.c b/lib/test_bits.c
>index c7b38d91e1f16d42b7ca92e62fbd6c19b37e76a0..dc93ded9fdb201e0d44b3c1cd71e233fd62258a5 100644
>--- a/lib/test_bits.c
>+++ b/lib/test_bits.c
>@@ -7,6 +7,22 @@
> #include <linux/bits.h>
>
>
>+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));

why are you dropping the ones for TEST_GENMASK_FAILURES ?

>+}
>+
>+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));

ditto

thanks
Lucas De Marchi

>+	KUNIT_EXPECT_EQ(test, 0xffffffffffffffffull, __GENMASK_ULL(63, 0));
>+}
>+
> static void genmask_test(struct kunit *test)
> {
> 	KUNIT_EXPECT_EQ(test, 1ul, GENMASK(0, 0));
>@@ -93,6 +109,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
>
>

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

[Vincent Mailhol]

On 13/03/2025 at 13:13, Lucas De Marchi wrote:
> On Thu, Mar 06, 2025 at 08:29:56PM +0900, Vincent Mailhol via B4 Relay
> wrote:
>> 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.
>>
>> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>> ---
>> lib/test_bits.c | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/lib/test_bits.c b/lib/test_bits.c
>> index
>> c7b38d91e1f16d42b7ca92e62fbd6c19b37e76a0..dc93ded9fdb201e0d44b3c1cd71e233fd62258a5 100644
>> --- a/lib/test_bits.c
>> +++ b/lib/test_bits.c
>> @@ -7,6 +7,22 @@
>> #include <linux/bits.h>
>>
>>
>> +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));
> 
> why are you dropping the ones for TEST_GENMASK_FAILURES ?

Because the __GENMASK() and the __GENMASK_ULL() do not use
GENMASK_INPUT_CHECK(), it is not possible to have those
TEST_GENMASK_FAILURES negative test cases here.

I will add one sentence to the commit message to explain this.

>> +}
>> +
>> +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));
> 
> ditto
> 
> thanks
> Lucas De Marchi
> 
>> +    KUNIT_EXPECT_EQ(test, 0xffffffffffffffffull, __GENMASK_ULL(63, 0));
>> +}
>> +
>> static void genmask_test(struct kunit *test)
>> {
>>     KUNIT_EXPECT_EQ(test, 1ul, GENMASK(0, 0));
>> @@ -93,6 +109,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),


Yours sincerely,
Vincent Mailhol

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

[Lucas De Marchi]

On Thu, Mar 13, 2025 at 03:00:34PM +0900, Vincent Mailhol wrote:
>On 13/03/2025 at 13:13, Lucas De Marchi wrote:
>> On Thu, Mar 06, 2025 at 08:29:56PM +0900, Vincent Mailhol via B4 Relay
>> wrote:
>>> 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.
>>>
>>> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>> ---
>>> lib/test_bits.c | 18 ++++++++++++++++++
>>> 1 file changed, 18 insertions(+)
>>>
>>> diff --git a/lib/test_bits.c b/lib/test_bits.c
>>> index
>>> c7b38d91e1f16d42b7ca92e62fbd6c19b37e76a0..dc93ded9fdb201e0d44b3c1cd71e233fd62258a5 100644
>>> --- a/lib/test_bits.c
>>> +++ b/lib/test_bits.c
>>> @@ -7,6 +7,22 @@
>>> #include <linux/bits.h>
>>>
>>>
>>> +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));
>>
>> why are you dropping the ones for TEST_GENMASK_FAILURES ?
>
>Because the __GENMASK() and the __GENMASK_ULL() do not use
>GENMASK_INPUT_CHECK(), it is not possible to have those
>TEST_GENMASK_FAILURES negative test cases here.
>
>I will add one sentence to the commit message to explain this.

ok, makes sense.

Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi

[PATCH v5 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:

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

diff --git a/lib/test_bits.c b/lib/test_bits.c
index dc93ded9fdb201e0d44b3c1cd71e233fd62258a5..91968227687bb11b7d1361b153c27eb851c6c1c2 100644
--- a/lib/test_bits.c
+++ b/lib/test_bits.c
@@ -5,7 +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);
 
 static void __genmask_test(struct kunit *test)
 {
@@ -30,11 +39,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 v5 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

  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 91968227687bb11b7d1361b153c27eb851c6c1c2..72984fae7b815031bb6eb2892c772ffcc409cf78 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 v5 7/7] test_bits: add tests for BIT_U*()

[Andy Shevchenko]

On Thu, Mar 06, 2025 at 08:29:58PM +0900, Vincent Mailhol via B4 Relay wrote:
> 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.

Still would be good to have a small assembly test case for GENMASK*() as they
went split and it will be a good regression test in case somebody decides to
unify both without much thinking..

-- 
With Best Regards,
Andy Shevchenko

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

[Vincent Mailhol]

On 06/03/2025 at 22:11, Andy Shevchenko wrote:
> On Thu, Mar 06, 2025 at 08:29:58PM +0900, Vincent Mailhol via B4 Relay wrote:
>> 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.
> 
> Still would be good to have a small assembly test case for GENMASK*() as they
> went split and it will be a good regression test in case somebody decides to
> unify both without much thinking..

Let me confirm that I correctly understood your ask. Would something
like this meet your expectations?

diff --git a/lib/test_bits.c b/lib/test_bits.c
index 72984fae7b81..869b291587e6 100644
--- a/lib/test_bits.c
+++ b/lib/test_bits.c
@@ -136,6 +136,29 @@ static void genmask_input_check_test(struct kunit
*test)
 	KUNIT_EXPECT_EQ(test, 0, GENMASK_INPUT_CHECK(127, 0));
 }

+#undef __LINUX_BITS_H
+#undef GENMASK
+#undef GENMASK_ULL
+#define __ASSEMBLY__
+#include <linux/bits.h>
+static void asm_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 asm_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));
+}
+#undef __ASSEMBLY__
+#undef GENMASK
+#undef GENMASK_ULL

 static struct kunit_case bits_test_cases[] = {
        KUNIT_CASE(__genmask_test),
@@ -144,6 +167,8 @@ static struct kunit_case bits_test_cases[] = {
 	KUNIT_CASE(genmask_ull_test),
 	KUNIT_CASE(genmask_u128_test),
 	KUNIT_CASE(genmask_input_check_test),
+	KUNIT_CASE(asm_genmask_test),
+	KUNIT_CASE(asm_genmask_ull_test),
 	{}
 };


Yours sincerely,
Vincent Mailhol

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

[Andy Shevchenko]

On Fri, Mar 07, 2025 at 01:08:15AM +0900, Vincent Mailhol wrote:
> On 06/03/2025 at 22:11, Andy Shevchenko wrote:
> > On Thu, Mar 06, 2025 at 08:29:58PM +0900, Vincent Mailhol via B4 Relay wrote:
> >> 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.
> > 
> > Still would be good to have a small assembly test case for GENMASK*() as they
> > went split and it will be a good regression test in case somebody decides to
> > unify both without much thinking..
> 
> Let me confirm that I correctly understood your ask. Would something
> like this meet your expectations?

I believe it should be written in asm.

-- 
With Best Regards,
Andy Shevchenko

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

[Vincent Mailhol]

On 07/03/2025 at 02:55, Andy Shevchenko wrote:
> On Fri, Mar 07, 2025 at 01:08:15AM +0900, Vincent Mailhol wrote:
>> On 06/03/2025 at 22:11, Andy Shevchenko wrote:
>>> On Thu, Mar 06, 2025 at 08:29:58PM +0900, Vincent Mailhol via B4 Relay wrote:
>>>> 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.
>>>
>>> Still would be good to have a small assembly test case for GENMASK*() as they
>>> went split and it will be a good regression test in case somebody decides to
>>> unify both without much thinking..
>>
>> Let me confirm that I correctly understood your ask. Would something
>> like this meet your expectations?
> 
> I believe it should be written in asm.

I am not confident enough in my assembly skills to submit asm patches to
the kernel. So, I would rather take a pass on that one.

Regardless, if somebody decides to unify both without much thinking as
you said, I am fully confident that the patch will get Nack-ed right
away. So, I do not have any concerns.


Yours sincerely,
Vincent Mailhol

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

[Andy Shevchenko]

On Fri, Mar 07, 2025 at 07:11:42PM +0900, Vincent Mailhol wrote:
> On 07/03/2025 at 02:55, Andy Shevchenko wrote:
> > On Fri, Mar 07, 2025 at 01:08:15AM +0900, Vincent Mailhol wrote:
> >> On 06/03/2025 at 22:11, Andy Shevchenko wrote:
> >>> On Thu, Mar 06, 2025 at 08:29:58PM +0900, Vincent Mailhol via B4 Relay wrote:
> >>>> 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.
> >>>
> >>> Still would be good to have a small assembly test case for GENMASK*() as they
> >>> went split and it will be a good regression test in case somebody decides to
> >>> unify both without much thinking..
> >>
> >> Let me confirm that I correctly understood your ask. Would something
> >> like this meet your expectations?
> > 
> > I believe it should be written in asm.
> 
> I am not confident enough in my assembly skills to submit asm patches to
> the kernel. So, I would rather take a pass on that one.
> 
> Regardless, if somebody decides to unify both without much thinking as
> you said, I am fully confident that the patch will get Nack-ed right

As I said above "would be good", if you think it's not feasible by you, perhaps
a comment (FIXME: ?) in the Kunit test cases that we lack of / need an asm test
as well.

-- 
With Best Regards,
Andy Shevchenko

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

[Vincent Mailhol]

On 08/03/2025 at 01:07, Andy Shevchenko wrote:
> On Fri, Mar 07, 2025 at 07:11:42PM +0900, Vincent Mailhol wrote:
>> On 07/03/2025 at 02:55, Andy Shevchenko wrote:
>>> On Fri, Mar 07, 2025 at 01:08:15AM +0900, Vincent Mailhol wrote:
>>>> On 06/03/2025 at 22:11, Andy Shevchenko wrote:
>>>>> On Thu, Mar 06, 2025 at 08:29:58PM +0900, Vincent Mailhol via B4 Relay wrote:
>>>>>> 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.
>>>>>
>>>>> Still would be good to have a small assembly test case for GENMASK*() as they
>>>>> went split and it will be a good regression test in case somebody decides to
>>>>> unify both without much thinking..
>>>>
>>>> Let me confirm that I correctly understood your ask. Would something
>>>> like this meet your expectations?
>>>
>>> I believe it should be written in asm.
>>
>> I am not confident enough in my assembly skills to submit asm patches to
>> the kernel. So, I would rather take a pass on that one.
>>
>> Regardless, if somebody decides to unify both without much thinking as
>> you said, I am fully confident that the patch will get Nack-ed right
> 
> As I said above "would be good", if you think it's not feasible by you, perhaps
> a comment (FIXME: ?) in the Kunit test cases that we lack of / need an asm test
> as well.

Ack. I will add a FIXME message in v6.


Yours sincerely,
Vincent Mailhol

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

[Lucas De Marchi]

On Thu, Mar 06, 2025 at 08:29:58PM +0900, Vincent Mailhol via B4 Relay wrote:
>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>

Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>

thanks
Lucas De Marchi

Re: [PATCH v5 0/7] bits: Fixed-type GENMASK()/BIT()

[Andy Shevchenko]

On Thu, Mar 06, 2025 at 08:29:51PM +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 an 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_t(t, w, h, l)			\

I thought we agreed on renaming...

>   	(((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_t(unsigned long, __BITS_PER_LONG, h, l)

Ditto.

> 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.
> 
> 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 a defconfig)

What defconfig? x86_64_defconfig?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 0/7] bits: Fixed-type GENMASK()/BIT()

[Vincent Mailhol]

On 06/03/2025 at 22:02, Andy Shevchenko wrote:
> On Thu, Mar 06, 2025 at 08:29:51PM +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 an 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_t(t, w, h, l)			\
> 
> I thought we agreed on renaming...
> 
>>   	(((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_t(unsigned long, __BITS_PER_LONG, h, l)
> 
> Ditto.

I forgot to update the cover letter, my bad…

>> 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.
>>
>> 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 a defconfig)
> 
> What defconfig? x86_64_defconfig?

Yes, x86_64 defconfig.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v5 0/7] bits: Fixed-type GENMASK()/BIT()

[Andy Shevchenko]

On Thu, Mar 06, 2025 at 08:29:51PM +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 version LGTM, just a couple of comments that you may find
in the individual replies.

-- 
With Best Regards,
Andy Shevchenko