[v7,0/5] bits: Fixed-type GENMASK_U*() and BIT_U*()

Message

Vincent Mailhol March 22, 2025, 9:23 a.m. UTC

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/[e-mail]

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. For this
reason, a new GENMASK_TYPE() is introduced instead and the uapi
__GENMASK() is left untouched.

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. So
the GENMASK_U*() are only visible to the non-asm code. For asm, the
long and long long variants seems sufficient.

This series does not modify the actual GENMASK(), GENMASK_ULL() and
GENMASK_U128(). A consolidation of the existing genmasks will be
proposed later on in a separate series.

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: 0/0 up/down: 0/0 (0)
  Function                                     old     new   delta
  Total: Before=22723481, After=22723481, chg +0.00%

(done with GCC 12.4.1 on an x86_64 defconfig)

--
2.43.0

---
Changes from v6:

  - Split the series in two: this series leave any existing GENMASK*()
    unmodified. The consolidation will be done in a separate series.

  - Collect some Reviewed-by tag.

  - Address miscellaneous nitpick on the code comments and the line
    wrapping (details in each patch).

  - Link to v6: https://lore.kernel.org/r/[e-mail]

Changes from v5:

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

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

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

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

  - Use git's histogram diff algorithm

  - Link to v5: https://lore.kernel.org/r/[e-mail]

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/[e-mail]

  - 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/[e-mail]

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/[e-mail]

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/[e-mail]

Link to v1: https://lore.kernel.org/intel-xe/[e-mail]

---
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 (2):
      bits: introduce fixed-type GENMASK_U*()
      test_bits: add tests for BIT_U*()

 drivers/gpu/drm/i915/i915_reg_defs.h | 108 ++++-------------------------------
 include/linux/bitops.h               |   1 -
 include/linux/bits.h                 |  57 +++++++++++++++++-
 lib/test_bits.c                      |  30 ++++++++++
 4 files changed, 96 insertions(+), 100 deletions(-)
---
base-commit: e3f42c436d7e0cb432935fe3ae275dd8d9b60f71
change-id: 20250228-fixed-type-genmasks-8d1a555f34e8

Best regards,

Comments

Yury Norov March 24, 2025, 2:28 p.m. UTC | #1

On Sat, Mar 22, 2025 at 06:23:11PM +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:

You say this, and then typecast both BIT and GENMASK. This may confuse
readers. Maybe add few words about promotion rules in C standard, or
just drop this note entirely? Doesn't require new submission, of
course.

>   GENMASK_U16(16, 0)
> 
> will raise a build bug.
> 
> This series is a continuation of:
> 
>   https://lore.kernel.org/intel-xe/[e-mail]
> 
> 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. For this
> reason, a new GENMASK_TYPE() is introduced instead and the uapi
> __GENMASK() is left untouched.
> 
> 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. So
> the GENMASK_U*() are only visible to the non-asm code. For asm, the
> long and long long variants seems sufficient.
> 
> This series does not modify the actual GENMASK(), GENMASK_ULL() and
> GENMASK_U128(). A consolidation of the existing genmasks will be
> proposed later on in a separate series.
> 
> 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: 0/0 up/down: 0/0 (0)
>   Function                                     old     new   delta
>   Total: Before=22723481, After=22723481, chg +0.00%
> 
> (done with GCC 12.4.1 on an x86_64 defconfig)
> 
> --
> 2.43.0
> 
> ---
> Changes from v6:
> 
>   - Split the series in two: this series leave any existing GENMASK*()
>     unmodified. The consolidation will be done in a separate series.
> 
>   - Collect some Reviewed-by tag.
> 
>   - Address miscellaneous nitpick on the code comments and the line
>     wrapping (details in each patch).
> 
>   - Link to v6: https://lore.kernel.org/r/[e-mail]
> 
> Changes from v5:
> 
>   - Update the cover letter message. I was still refering to
>     GENMASK_t() instead of GENMASK_TYPE().
> 
>   - Add a comment in the cover letter to explain that a common
>     GENMASK_TYPE() for C and asm wouldn't allow to generate the u128
>     variant.
> 
>   - Restore the comment saying that BUILD_BUG_ON() is not available in
>     asm code.
> 
>   - Add a FIXME message to highlight the absence of the asm GENMASK*()
>     unit tests.
> 
>   - Use git's histogram diff algorithm
> 
>   - Link to v5: https://lore.kernel.org/r/[e-mail]
> 
> 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/[e-mail]
> 
>   - 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/[e-mail]
> 
> 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/[e-mail]
> 
> 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/[e-mail]
> 
> Link to v1: https://lore.kernel.org/intel-xe/[e-mail]
> 
> ---
> 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 (2):
>       bits: introduce fixed-type GENMASK_U*()
>       test_bits: add tests for BIT_U*()
> 
>  drivers/gpu/drm/i915/i915_reg_defs.h | 108 ++++-------------------------------
>  include/linux/bitops.h               |   1 -
>  include/linux/bits.h                 |  57 +++++++++++++++++-
>  lib/test_bits.c                      |  30 ++++++++++
>  4 files changed, 96 insertions(+), 100 deletions(-)
> ---
> base-commit: e3f42c436d7e0cb432935fe3ae275dd8d9b60f71
> change-id: 20250228-fixed-type-genmasks-8d1a555f34e8
> 
> Best regards,
> -- 
> Vincent Mailhol <[e-mail]>
>

Vincent Mailhol March 24, 2025, 4:23 p.m. UTC | #2

On 24/03/2025 at 23:28, Yury Norov wrote:
> On Sat, Mar 22, 2025 at 06:23:11PM +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:
> 
> You say this, and then typecast both BIT and GENMASK. This may confuse
> readers. Maybe add few words about promotion rules in C standard, or
> just drop this note entirely? Doesn't require new submission, of
> course.

I do not want to into this level of details in the cover letter, so I
will remove. Instead, I can add below paragraph to the "bits: introduce
fixed-type GENMASK_U*()" patch:

  The result is casted to the corresponding fixed width type. For
  example, GENMASK_U8() returns an u8. Note that because of the C
  promotion rules, GENMASK_U8() and GENMASK_U16() will immediately be
  promoted to int if used in an expression. Regardless, the main goal is
  not to get the correct type, but rather to enforce more checks at
  compile time.

I staged this change in the v8 together with the other nitpicks from
Andy. If you want that v8, let me know, it is ready. If you are happy
enough with the v7 (and if it doesn't receive more comments), then go
with it!


Yours sincerely,
Vincent Mailhol

Yury Norov March 25, 2025, 3:23 p.m. UTC | #3

On Tue, Mar 25, 2025 at 01:23:22AM +0900, Vincent Mailhol wrote:
> On 24/03/2025 at 23:28, Yury Norov wrote:
> > On Sat, Mar 22, 2025 at 06:23:11PM +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:
> > 
> > You say this, and then typecast both BIT and GENMASK. This may confuse
> > readers. Maybe add few words about promotion rules in C standard, or
> > just drop this note entirely? Doesn't require new submission, of
> > course.
> 
> I do not want to into this level of details in the cover letter, so I
> will remove. Instead, I can add below paragraph to the "bits: introduce
> fixed-type GENMASK_U*()" patch:
> 
>   The result is casted to the corresponding fixed width type. For
>   example, GENMASK_U8() returns an u8. Note that because of the C
>   promotion rules, GENMASK_U8() and GENMASK_U16() will immediately be
>   promoted to int if used in an expression. Regardless, the main goal is
>   not to get the correct type, but rather to enforce more checks at
>   compile time.
> 
> I staged this change in the v8 together with the other nitpicks from
> Andy. If you want that v8, let me know, it is ready. If you are happy
> enough with the v7 (and if it doesn't receive more comments), then go
> with it!

This series doesn't apply on 6.15-rc1 because test_bits.c has moved to
lib/tests. Can you please rebase your v8 and submit? I see no other
issues to merge it in bitmap-for-next.

Thanks,
Yury

Vincent Mailhol March 25, 2025, 4:13 p.m. UTC | #4

On 26/03/2025 at 00:23, Yury Norov wrote:
> On Tue, Mar 25, 2025 at 01:23:22AM +0900, Vincent Mailhol wrote:

(...)

> This series doesn't apply on 6.15-rc1 because test_bits.c has moved to
> lib/tests. Can you please rebase your v8 and submit? I see no other
> issues to merge it in bitmap-for-next.

git was smart enough to rebase everything automatically!

Here is the v8 (which includes the other few nitpicks from you and Andy):

https://lore.kernel.org/all/[e-mail]/

Do you also want me to rebase the other series which consolidates the
GENMASK(), GENMASK_ULL() and GENMASK_U128() now? Or should I wait a while?


Yours sincerely,
Vincent Mailhol

Yury Norov March 25, 2025, 4:30 p.m. UTC | #5

On Wed, Mar 26, 2025 at 01:13:28AM +0900, Vincent Mailhol wrote:
> On 26/03/2025 at 00:23, Yury Norov wrote:
> > On Tue, Mar 25, 2025 at 01:23:22AM +0900, Vincent Mailhol wrote:
> 
> (...)
> 
> > This series doesn't apply on 6.15-rc1 because test_bits.c has moved to
> > lib/tests. Can you please rebase your v8 and submit? I see no other
> > issues to merge it in bitmap-for-next.
> 
> git was smart enough to rebase everything automatically!
> 
> Here is the v8 (which includes the other few nitpicks from you and Andy):
> 
> https://lore.kernel.org/all/[e-mail]/
> 
> Do you also want me to rebase the other series which consolidates the
> GENMASK(), GENMASK_ULL() and GENMASK_U128() now? Or should I wait a while?

Let's wait for feedback, especially from ARM folks.