[PATCH v6] rust: kernel: add support for bits/genmask macros

[PATCH v6] rust: kernel: add support for bits/genmask macros

[Daniel Almeida]

In light of bindgen being unable to generate bindings for macros, and
owing to the widespread use of these macros in drivers, manually define
the bit and genmask C macros in Rust.

The *_checked version of the functions provide runtime checking while
the const version performs compile-time assertions on the arguments via
the build_assert!() macro.

Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
Changes in v6:
Thanks, Alex {
  - Added _checked and _unbounded versions of the functions
  - Implemented the functions through a macro
  - Changed the genmask logic to prevent over/underflow (hopefully)
  - Genmask now takes a range instead of "h" and "l" arguments
  - Made all functions #[inline]
  - +cc Alex directly
  - Removed all panics
}
- Link to v5: https://lore.kernel.org/r/20250326-topic-panthor-rs-genmask-v5-1-bfa6140214da@collabora.com

Changes in v5:
- Added versions for u16 and u8 in order to reduce the amount of casts
  for callers. This came up after discussing the issue with Alexandre
  Courbot in light of his "register" abstractions.
- Link to v4: https://lore.kernel.org/r/20250318-topic-panthor-rs-genmask-v4-1-35004fca6ac5@collabora.com

Changes in v4:
- Split bits into bits_u32 and bits_u64
- Added r-b's
- Rebased on top of rust-next
- Link to v3: https://lore.kernel.org/r/20250121-topic-panthor-rs-genmask-v3-1-5c3bdf21ce05@collabora.com

Changes in v3:
- Changed from declarative macro to const fn
- Added separate versions for u32 and u64
- Link to v2: https://lore.kernel.org/r/20241024-topic-panthor-rs-genmask-v2-1-85237c1f0cea@collabora.com

Changes in v2:

- Added ticks around `BIT`, and `h >=l` (Thanks, Benno).
- Decided to keep the arguments as `expr`, as I see no issues with that
- Added a proper example, with an assert_eq!() (Thanks, Benno)
- Fixed the condition h <= l, which should be h >= l.
- Checked that the assert for the condition above is described in the
  docs.
---
 rust/kernel/bits.rs | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs  |   1 +
 2 files changed, 169 insertions(+)

diff --git a/rust/kernel/bits.rs b/rust/kernel/bits.rs
new file mode 100644
index 0000000000000000000000000000000000000000..98065c8f7c94cfc3b076e041de190e942e1b4a9f
--- /dev/null
+++ b/rust/kernel/bits.rs
@@ -0,0 +1,168 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Bit manipulation macros.
+//!
+//! C header: [`include/linux/bits.h`](srctree/include/linux/bits.h)
+
+use crate::build_assert;
+use core::ops::Range;
+
+macro_rules! impl_bit_fn {
+    (
+        $checked_name:ident, $unbounded_name:ident, $const_name:ident, $ty:ty
+    ) => {
+        /// Computes `1 << n` if `n` is in bounds, i.e.: if `n` is smaller than
+        /// the maximum number of bits supported by the type.
+        ///
+        /// Returns [`None`] otherwise.
+        #[inline]
+        pub fn $checked_name(n: u32) -> Option<$ty> {
+            (1 as $ty) .checked_shl(n)
+        }
+
+        /// Computes `1 << n` if `n` is in bounds, i.e.: if `n` is smaller than
+        /// the maximum number of bits supported by the type.
+        ///
+        /// Returns `0` otherwise.
+        ///
+        /// This is a convenience, as [`Option::unwrap_or`] cannot be used in
+        /// const-context.
+        #[inline]
+        pub fn $unbounded_name(n: u32) -> $ty {
+            match $checked_name(n) {
+                Some(v) => v,
+                None => 0,
+            }
+        }
+
+        /// Computes `1 << n` by performing a compile-time assertion that `n` is
+        /// in bounds.
+        ///
+        /// This version is the default and should be used if `n` is known at
+        /// compile time.
+        #[inline]
+        pub const fn $const_name(n: u32) -> $ty {
+            build_assert!(n < <$ty>::BITS);
+            1 as $ty << n
+        }
+    };
+}
+
+impl_bit_fn!(checked_bit_u64, unbounded_bit_u64, bit_u64, u64);
+impl_bit_fn!(checked_bit_u32, unbounded_bit_u32, bit_u32, u32);
+impl_bit_fn!(checked_bit_u16, unbounded_bit_u16, bit_u16, u16);
+impl_bit_fn!(checked_bit_u8, unbounded_bit_u8, bit_u8, u8);
+
+macro_rules! impl_genmask_fn {
+    (
+        $ty:ty, $checked_bit:ident, $bit:ident, $genmask:ident, $genmask_checked:ident, $genmask_unbounded:ident,
+        $(#[$genmask_ex:meta])*
+    ) => {
+        /// Creates a compile-time contiguous bitmask for the given range by
+        /// validating the range at runtime.
+        ///
+        /// Returns [`None`] if the range is invalid, i.e.: if the start is
+        /// greater than or equal to the end.
+        #[inline]
+        pub fn $genmask_checked(range: Range<u32>) -> Option<$ty> {
+            if range.start >= range.end || range.end > <$ty>::BITS {
+                return None;
+            }
+            let high = $checked_bit(range.end)?;
+            let low = $checked_bit(range.start)?;
+            Some((high | (high - 1)) & !(low - 1))
+        }
+
+        /// Creates a compile-time contiguous bitmask for the given range by
+        /// validating the range at runtime.
+        ///
+        /// Returns `0` if the range is invalid, i.e.: if the start is greater
+        /// than or equal to the end.
+        #[inline]
+        pub fn $genmask_unbounded(range: Range<u32>) -> $ty {
+            match $genmask_checked(range) {
+                Some(v) => v,
+                None => 0,
+            }
+        }
+
+        /// Creates a compile-time contiguous bitmask for the given range by
+        /// performing a compile-time assertion that the range is valid.
+        ///
+        /// This version is the default and should be used if the range is known
+        /// at compile time.
+        $(#[$genmask_ex])*
+        #[inline]
+        pub const fn $genmask(range: Range<u32>) -> $ty {
+            build_assert!(range.start < range.end);
+            build_assert!(range.end <= <$ty>::BITS);
+            let high = $bit(range.end);
+            let low = $bit(range.start);
+            (high | (high - 1)) & !(low - 1)
+        }
+    };
+}
+
+impl_genmask_fn!(
+    u64,
+    checked_bit_u64,
+    bit_u64,
+    genmask_u64,
+    genmask_checked_u64,
+    genmask_unbounded_u64,
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::bits::genmask_u64;
+    /// let mask = genmask_u64(21..39);
+    /// assert_eq!(mask, 0x000000ffffe00000);
+    /// ```
+);
+
+impl_genmask_fn!(
+    u32,
+    checked_bit_u32,
+    bit_u32,
+    genmask_u32,
+    genmask_checked_u32,
+    genmask_unbounded_u32,
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::bits::genmask_u32;
+    /// let mask = genmask_u32(0..9);
+    /// assert_eq!(mask, 0x000003ff);
+    /// ```
+);
+
+impl_genmask_fn!(
+    u16,
+    checked_bit_u16,
+    bit_u16,
+    genmask_u16,
+    genmask_checked_u16,
+    genmask_unbounded_u16,
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::bits::genmask_u16;
+    /// let mask = genmask_u16(0..9);
+    /// assert_eq!(mask, 0x000003ff);
+    /// ```
+);
+
+impl_genmask_fn!(
+    u8,
+    checked_bit_u8,
+    bit_u8,
+    genmask_u8,
+    genmask_checked_u8,
+    genmask_unbounded_u8,
+    /// # Examples
+    ///
+    /// ```
+    /// # use kernel::bits::genmask_u8;
+    /// let mask = genmask_u8(0..7);
+    /// assert_eq!(mask, 0x000000ff);
+    /// ```
+);
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index c92497c7c655e8faefd85bb4a1d5b4cc696b8499..a90aaf7fe6755a5a42055b7b4008714fcafe6f6f 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -36,6 +36,7 @@
 pub use ffi;
 
 pub mod alloc;
+pub mod bits;
 #[cfg(CONFIG_BLOCK)]
 pub mod block;
 #[doc(hidden)]

---
base-commit: cf25bc61f8aecad9b0c45fe32697e35ea4b13378
change-id: 20241023-topic-panthor-rs-genmask-fabc573fef43

Best regards,
-- 
Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Miguel Ojeda]

On Tue, Jun 10, 2025 at 4:16 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> +use crate::build_assert;

You can include the prelude instead.

> +            (1 as $ty) .checked_shl(n)

Formatting.

> +        pub fn $unbounded_name(n: u32) -> $ty {

We may want to have a comment inside here to remind ourselves to
forward the call to the standard library one when available (1.87.0).

> +        /// Creates a compile-time contiguous bitmask for the given range by
> +        /// validating the range at runtime.

I may be confused by what you are trying to do here, but how are these
`checked` and `unbounded` ones compile-time?

Also, you can probably simplify the macro `impl` calls by removing
parameters by using `paste!`, e.g.

    let high = ::kernel::macros::paste!([<checked_bit_ $ty>])(range.end)?;

> base-commit: cf25bc61f8aecad9b0c45fe32697e35ea4b13378

This is a fairly old base now (the patch does not apply cleanly).

Thanks!

Cheers,
Miguel

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Daniel Almeida]

Hi Miguel,

> On 10 Jun 2025, at 15:08, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
> On Tue, Jun 10, 2025 at 4:16 PM Daniel Almeida
> <daniel.almeida@collabora.com> wrote:
>> 
>> +use crate::build_assert;
> 
> You can include the prelude instead.
> 
>> +            (1 as $ty) .checked_shl(n)
> 
> Formatting.
> 
>> +        pub fn $unbounded_name(n: u32) -> $ty {
> 
> We may want to have a comment inside here to remind ourselves to
> forward the call to the standard library one when available (1.87.0).
> 
>> +        /// Creates a compile-time contiguous bitmask for the given range by
>> +        /// validating the range at runtime.
> 
> I may be confused by what you are trying to do here, but how are these
> `checked` and `unbounded` ones compile-time?

This is wrong indeed, thanks for catching that.

> 
> Also, you can probably simplify the macro `impl` calls by removing
> parameters by using `paste!`, e.g.
> 
>    let high = ::kernel::macros::paste!([<checked_bit_ $ty>])(range.end)?;

I personally find paste’s syntax a bit hard to read, but sure, I have nothing against this.

> 
>> base-commit: cf25bc61f8aecad9b0c45fe32697e35ea4b13378
> 
> This is a fairly old base now (the patch does not apply cleanly).

Sorry about that. 

Anyways, let's see if others are happy with the direction this patch is now
taking. I will rebase on the next iteration :)

> 
> Thanks!
> 
> Cheers,
> Miguel
> 

— Daniel

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Alexandre Courbot]

Hi Daniel,

On Tue Jun 10, 2025 at 11:14 PM JST, Daniel Almeida wrote:
> In light of bindgen being unable to generate bindings for macros, and
> owing to the widespread use of these macros in drivers, manually define
> the bit and genmask C macros in Rust.
>
> The *_checked version of the functions provide runtime checking while
> the const version performs compile-time assertions on the arguments via
> the build_assert!() macro.

I think this is the right approach, I wish we could make the functions
generic but that doesn't appear to be currently possible (and it
wouldn't make their invocation shorter anyway).

I agree with Miguel's suggestion to use paste to shorten the macro
syntax, it would make it much easier to understand them (and most of the
naming could be harmonized in the macro itself instead of relying on the
invocations to use the same names).

>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> ---
> Changes in v6:
> Thanks, Alex {
>   - Added _checked and _unbounded versions of the functions
>   - Implemented the functions through a macro
>   - Changed the genmask logic to prevent over/underflow (hopefully)
>   - Genmask now takes a range instead of "h" and "l" arguments
>   - Made all functions #[inline]
>   - +cc Alex directly
>   - Removed all panics
> }
> - Link to v5: https://lore.kernel.org/r/20250326-topic-panthor-rs-genmask-v5-1-bfa6140214da@collabora.com
>
> Changes in v5:
> - Added versions for u16 and u8 in order to reduce the amount of casts
>   for callers. This came up after discussing the issue with Alexandre
>   Courbot in light of his "register" abstractions.
> - Link to v4: https://lore.kernel.org/r/20250318-topic-panthor-rs-genmask-v4-1-35004fca6ac5@collabora.com
>
> Changes in v4:
> - Split bits into bits_u32 and bits_u64
> - Added r-b's
> - Rebased on top of rust-next
> - Link to v3: https://lore.kernel.org/r/20250121-topic-panthor-rs-genmask-v3-1-5c3bdf21ce05@collabora.com
>
> Changes in v3:
> - Changed from declarative macro to const fn
> - Added separate versions for u32 and u64
> - Link to v2: https://lore.kernel.org/r/20241024-topic-panthor-rs-genmask-v2-1-85237c1f0cea@collabora.com
>
> Changes in v2:
>
> - Added ticks around `BIT`, and `h >=l` (Thanks, Benno).
> - Decided to keep the arguments as `expr`, as I see no issues with that
> - Added a proper example, with an assert_eq!() (Thanks, Benno)
> - Fixed the condition h <= l, which should be h >= l.
> - Checked that the assert for the condition above is described in the
>   docs.
> ---
>  rust/kernel/bits.rs | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs  |   1 +
>  2 files changed, 169 insertions(+)
>
> diff --git a/rust/kernel/bits.rs b/rust/kernel/bits.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..98065c8f7c94cfc3b076e041de190e942e1b4a9f
> --- /dev/null
> +++ b/rust/kernel/bits.rs
> @@ -0,0 +1,168 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Bit manipulation macros.
> +//!
> +//! C header: [`include/linux/bits.h`](srctree/include/linux/bits.h)
> +
> +use crate::build_assert;
> +use core::ops::Range;
> +
> +macro_rules! impl_bit_fn {
> +    (
> +        $checked_name:ident, $unbounded_name:ident, $const_name:ident, $ty:ty
> +    ) => {
> +        /// Computes `1 << n` if `n` is in bounds, i.e.: if `n` is smaller than
> +        /// the maximum number of bits supported by the type.
> +        ///
> +        /// Returns [`None`] otherwise.
> +        #[inline]
> +        pub fn $checked_name(n: u32) -> Option<$ty> {
> +            (1 as $ty) .checked_shl(n)
> +        }
> +
> +        /// Computes `1 << n` if `n` is in bounds, i.e.: if `n` is smaller than
> +        /// the maximum number of bits supported by the type.
> +        ///
> +        /// Returns `0` otherwise.
> +        ///
> +        /// This is a convenience, as [`Option::unwrap_or`] cannot be used in
> +        /// const-context.
> +        #[inline]
> +        pub fn $unbounded_name(n: u32) -> $ty {
> +            match $checked_name(n) {
> +                Some(v) => v,
> +                None => 0,
> +            }

This could more succintly be `$checked_name(n).unwrap_or(0)` (same
remark for `$genmask_unbounded` below).

> +        }
> +
> +        /// Computes `1 << n` by performing a compile-time assertion that `n` is
> +        /// in bounds.
> +        ///
> +        /// This version is the default and should be used if `n` is known at
> +        /// compile time.
> +        #[inline]
> +        pub const fn $const_name(n: u32) -> $ty {
> +            build_assert!(n < <$ty>::BITS);
> +            1 as $ty << n
> +        }
> +    };
> +}
> +
> +impl_bit_fn!(checked_bit_u64, unbounded_bit_u64, bit_u64, u64);
> +impl_bit_fn!(checked_bit_u32, unbounded_bit_u32, bit_u32, u32);
> +impl_bit_fn!(checked_bit_u16, unbounded_bit_u16, bit_u16, u16);
> +impl_bit_fn!(checked_bit_u8, unbounded_bit_u8, bit_u8, u8);
> +
> +macro_rules! impl_genmask_fn {
> +    (
> +        $ty:ty, $checked_bit:ident, $bit:ident, $genmask:ident, $genmask_checked:ident, $genmask_unbounded:ident,
> +        $(#[$genmask_ex:meta])*
> +    ) => {
> +        /// Creates a compile-time contiguous bitmask for the given range by
> +        /// validating the range at runtime.
> +        ///
> +        /// Returns [`None`] if the range is invalid, i.e.: if the start is
> +        /// greater than or equal to the end.
> +        #[inline]
> +        pub fn $genmask_checked(range: Range<u32>) -> Option<$ty> {
> +            if range.start >= range.end || range.end > <$ty>::BITS {
> +                return None;
> +            }

From this check I assumed that you interpret `range` as non-inclusive,
since `range.end == 32` is valid on u32...

> +            let high = $checked_bit(range.end)?;

... however IIUC `checked_bit` will return `None` here in such a case.
Should the argument be `range.end - 1`?

Your examples do seem to interpret the range as inclusive though, so
probably the check should be `|| range.end >= <$ty>::BITS`. But that
triggers the question, is it ok to use `Range` that way, when its
documentation specifically states that it is bounded exclusively above?
We could use `RangeInclusive` to match the semantics, which would
require us to write the ranges as `0..=7`. At least it is clear that the
upper bound is inclusive.

... or we make the methods generic against `RangeBounds` and allow both
`Range` and `RangeInclusive` to be used. But I'm concerned that callers
might use `0..1` thinking it is inclusive while it is not.

Thoughts?

> +            let low = $checked_bit(range.start)?;
> +            Some((high | (high - 1)) & !(low - 1))
> +        }
> +
> +        /// Creates a compile-time contiguous bitmask for the given range by
> +        /// validating the range at runtime.
> +        ///
> +        /// Returns `0` if the range is invalid, i.e.: if the start is greater
> +        /// than or equal to the end.
> +        #[inline]
> +        pub fn $genmask_unbounded(range: Range<u32>) -> $ty {
> +            match $genmask_checked(range) {
> +                Some(v) => v,
> +                None => 0,
> +            }

This could more succintly be `$genmask_checked(range).unwrap_or(0)`.

> +        }
> +
> +        /// Creates a compile-time contiguous bitmask for the given range by
> +        /// performing a compile-time assertion that the range is valid.
> +        ///
> +        /// This version is the default and should be used if the range is known
> +        /// at compile time.
> +        $(#[$genmask_ex])*
> +        #[inline]
> +        pub const fn $genmask(range: Range<u32>) -> $ty {
> +            build_assert!(range.start < range.end);
> +            build_assert!(range.end <= <$ty>::BITS);

I guess this check also needs to be fixed.

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Boqun Feng]

On Sat, Jun 14, 2025 at 10:38:11PM +0900, Alexandre Courbot wrote:
[...]
> > +macro_rules! impl_genmask_fn {
> > +    (
> > +        $ty:ty, $checked_bit:ident, $bit:ident, $genmask:ident, $genmask_checked:ident, $genmask_unbounded:ident,
> > +        $(#[$genmask_ex:meta])*
> > +    ) => {
> > +        /// Creates a compile-time contiguous bitmask for the given range by
> > +        /// validating the range at runtime.
> > +        ///
> > +        /// Returns [`None`] if the range is invalid, i.e.: if the start is
> > +        /// greater than or equal to the end.
> > +        #[inline]
> > +        pub fn $genmask_checked(range: Range<u32>) -> Option<$ty> {
> > +            if range.start >= range.end || range.end > <$ty>::BITS {
> > +                return None;
> > +            }
> 
> From this check I assumed that you interpret `range` as non-inclusive,
> since `range.end == 32` is valid on u32...
> 
> > +            let high = $checked_bit(range.end)?;
> 
> ... however IIUC `checked_bit` will return `None` here in such a case.
> Should the argument be `range.end - 1`?
> 
> Your examples do seem to interpret the range as inclusive though, so
> probably the check should be `|| range.end >= <$ty>::BITS`. But that
> triggers the question, is it ok to use `Range` that way, when its
> documentation specifically states that it is bounded exclusively above?
> We could use `RangeInclusive` to match the semantics, which would
> require us to write the ranges as `0..=7`. At least it is clear that the
> upper bound is inclusive.
> 
> ... or we make the methods generic against `RangeBounds` and allow both
> `Range` and `RangeInclusive` to be used. But I'm concerned that callers
> might use `0..1` thinking it is inclusive while it is not.
> 

I think generic over `RangeBounds` is a good idea, and we should
.is_emtpy() or .contains() instead of comparison + boolean operation
when possible. Seems we need a function to check whether one range
contains another range, which is not available currently?

I would not be worried about callers treating `0..1` as inclusive: this
is a Rust project anyway, we need to learn the correct semantics of
expressions eventually ;-)

Regards,
Boqun

> Thoughts?
> 
> > +            let low = $checked_bit(range.start)?;
> > +            Some((high | (high - 1)) & !(low - 1))
> > +        }
> > +
> > +        /// Creates a compile-time contiguous bitmask for the given range by
> > +        /// validating the range at runtime.
> > +        ///
> > +        /// Returns `0` if the range is invalid, i.e.: if the start is greater
> > +        /// than or equal to the end.
> > +        #[inline]
> > +        pub fn $genmask_unbounded(range: Range<u32>) -> $ty {
> > +            match $genmask_checked(range) {
> > +                Some(v) => v,
> > +                None => 0,
> > +            }
[...]

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Boqun Feng]

On Sat, Jun 14, 2025 at 08:06:04AM -0700, Boqun Feng wrote:
> On Sat, Jun 14, 2025 at 10:38:11PM +0900, Alexandre Courbot wrote:
> [...]
> > > +macro_rules! impl_genmask_fn {
> > > +    (
> > > +        $ty:ty, $checked_bit:ident, $bit:ident, $genmask:ident, $genmask_checked:ident, $genmask_unbounded:ident,
> > > +        $(#[$genmask_ex:meta])*
> > > +    ) => {
> > > +        /// Creates a compile-time contiguous bitmask for the given range by
> > > +        /// validating the range at runtime.
> > > +        ///
> > > +        /// Returns [`None`] if the range is invalid, i.e.: if the start is
> > > +        /// greater than or equal to the end.
> > > +        #[inline]
> > > +        pub fn $genmask_checked(range: Range<u32>) -> Option<$ty> {
> > > +            if range.start >= range.end || range.end > <$ty>::BITS {
> > > +                return None;
> > > +            }
> > 
> > From this check I assumed that you interpret `range` as non-inclusive,
> > since `range.end == 32` is valid on u32...
> > 
> > > +            let high = $checked_bit(range.end)?;
> > 
> > ... however IIUC `checked_bit` will return `None` here in such a case.
> > Should the argument be `range.end - 1`?
> > 
> > Your examples do seem to interpret the range as inclusive though, so
> > probably the check should be `|| range.end >= <$ty>::BITS`. But that
> > triggers the question, is it ok to use `Range` that way, when its
> > documentation specifically states that it is bounded exclusively above?
> > We could use `RangeInclusive` to match the semantics, which would
> > require us to write the ranges as `0..=7`. At least it is clear that the
> > upper bound is inclusive.
> > 
> > ... or we make the methods generic against `RangeBounds` and allow both
> > `Range` and `RangeInclusive` to be used. But I'm concerned that callers
> > might use `0..1` thinking it is inclusive while it is not.
> > 
> 
> I think generic over `RangeBounds` is a good idea, and we should
> .is_emtpy() or .contains() instead of comparison + boolean operation
> when possible. Seems we need a function to check whether one range

Ah.. from the other email, genmask_checked() needs to be const, then I
think we cannot use RangeBounds here? Because RangeBounds::start() is 
not a const function.

Regards,
Boqun

> contains another range, which is not available currently?
> 
> I would not be worried about callers treating `0..1` as inclusive: this
> is a Rust project anyway, we need to learn the correct semantics of
> expressions eventually ;-)
> 
> Regards,
> Boqun
> 
> > Thoughts?
> > 
> > > +            let low = $checked_bit(range.start)?;
> > > +            Some((high | (high - 1)) & !(low - 1))
> > > +        }
> > > +
> > > +        /// Creates a compile-time contiguous bitmask for the given range by
> > > +        /// validating the range at runtime.
> > > +        ///
> > > +        /// Returns `0` if the range is invalid, i.e.: if the start is greater
> > > +        /// than or equal to the end.
> > > +        #[inline]
> > > +        pub fn $genmask_unbounded(range: Range<u32>) -> $ty {
> > > +            match $genmask_checked(range) {
> > > +                Some(v) => v,
> > > +                None => 0,
> > > +            }
> [...]

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Boqun Feng]

On Sat, Jun 14, 2025 at 08:56:36AM -0700, Boqun Feng wrote:
> On Sat, Jun 14, 2025 at 08:06:04AM -0700, Boqun Feng wrote:
> > On Sat, Jun 14, 2025 at 10:38:11PM +0900, Alexandre Courbot wrote:
> > [...]
> > > > +macro_rules! impl_genmask_fn {
> > > > +    (
> > > > +        $ty:ty, $checked_bit:ident, $bit:ident, $genmask:ident, $genmask_checked:ident, $genmask_unbounded:ident,
> > > > +        $(#[$genmask_ex:meta])*
> > > > +    ) => {
> > > > +        /// Creates a compile-time contiguous bitmask for the given range by
> > > > +        /// validating the range at runtime.
> > > > +        ///
> > > > +        /// Returns [`None`] if the range is invalid, i.e.: if the start is
> > > > +        /// greater than or equal to the end.
> > > > +        #[inline]
> > > > +        pub fn $genmask_checked(range: Range<u32>) -> Option<$ty> {
> > > > +            if range.start >= range.end || range.end > <$ty>::BITS {
> > > > +                return None;
> > > > +            }
> > > 
> > > From this check I assumed that you interpret `range` as non-inclusive,
> > > since `range.end == 32` is valid on u32...
> > > 
> > > > +            let high = $checked_bit(range.end)?;
> > > 
> > > ... however IIUC `checked_bit` will return `None` here in such a case.
> > > Should the argument be `range.end - 1`?
> > > 
> > > Your examples do seem to interpret the range as inclusive though, so
> > > probably the check should be `|| range.end >= <$ty>::BITS`. But that
> > > triggers the question, is it ok to use `Range` that way, when its
> > > documentation specifically states that it is bounded exclusively above?
> > > We could use `RangeInclusive` to match the semantics, which would
> > > require us to write the ranges as `0..=7`. At least it is clear that the
> > > upper bound is inclusive.
> > > 
> > > ... or we make the methods generic against `RangeBounds` and allow both
> > > `Range` and `RangeInclusive` to be used. But I'm concerned that callers
> > > might use `0..1` thinking it is inclusive while it is not.
> > > 
> > 
> > I think generic over `RangeBounds` is a good idea, and we should
> > .is_emtpy() or .contains() instead of comparison + boolean operation
> > when possible. Seems we need a function to check whether one range
> 
> Ah.. from the other email, genmask_checked() needs to be const, then I
> think we cannot use RangeBounds here? Because RangeBounds::start() is 
> not a const function.
> 

Ignore this, seems these functions meant to be not const, just the
document is wrong.

Regards,
Boqun

> Regards,
> Boqun
> 
> > contains another range, which is not available currently?
> > 
> > I would not be worried about callers treating `0..1` as inclusive: this
> > is a Rust project anyway, we need to learn the correct semantics of
> > expressions eventually ;-)
> > 
> > Regards,
> > Boqun
> > 
> > > Thoughts?
> > > 
> > > > +            let low = $checked_bit(range.start)?;
> > > > +            Some((high | (high - 1)) & !(low - 1))
> > > > +        }
> > > > +
> > > > +        /// Creates a compile-time contiguous bitmask for the given range by
> > > > +        /// validating the range at runtime.
> > > > +        ///
> > > > +        /// Returns `0` if the range is invalid, i.e.: if the start is greater
> > > > +        /// than or equal to the end.
> > > > +        #[inline]
> > > > +        pub fn $genmask_unbounded(range: Range<u32>) -> $ty {
> > > > +            match $genmask_checked(range) {
> > > > +                Some(v) => v,
> > > > +                None => 0,
> > > > +            }
> > [...]

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Alexandre Courbot]

On Sun Jun 15, 2025 at 12:06 AM JST, Boqun Feng wrote:
> On Sat, Jun 14, 2025 at 10:38:11PM +0900, Alexandre Courbot wrote:
> [...]
>> > +macro_rules! impl_genmask_fn {
>> > +    (
>> > +        $ty:ty, $checked_bit:ident, $bit:ident, $genmask:ident, $genmask_checked:ident, $genmask_unbounded:ident,
>> > +        $(#[$genmask_ex:meta])*
>> > +    ) => {
>> > +        /// Creates a compile-time contiguous bitmask for the given range by
>> > +        /// validating the range at runtime.
>> > +        ///
>> > +        /// Returns [`None`] if the range is invalid, i.e.: if the start is
>> > +        /// greater than or equal to the end.
>> > +        #[inline]
>> > +        pub fn $genmask_checked(range: Range<u32>) -> Option<$ty> {
>> > +            if range.start >= range.end || range.end > <$ty>::BITS {
>> > +                return None;
>> > +            }
>> 
>> From this check I assumed that you interpret `range` as non-inclusive,
>> since `range.end == 32` is valid on u32...
>> 
>> > +            let high = $checked_bit(range.end)?;
>> 
>> ... however IIUC `checked_bit` will return `None` here in such a case.
>> Should the argument be `range.end - 1`?
>> 
>> Your examples do seem to interpret the range as inclusive though, so
>> probably the check should be `|| range.end >= <$ty>::BITS`. But that
>> triggers the question, is it ok to use `Range` that way, when its
>> documentation specifically states that it is bounded exclusively above?
>> We could use `RangeInclusive` to match the semantics, which would
>> require us to write the ranges as `0..=7`. At least it is clear that the
>> upper bound is inclusive.
>> 
>> ... or we make the methods generic against `RangeBounds` and allow both
>> `Range` and `RangeInclusive` to be used. But I'm concerned that callers
>> might use `0..1` thinking it is inclusive while it is not.
>> 
>
> I think generic over `RangeBounds` is a good idea, and we should
> .is_emtpy() or .contains() instead of comparison + boolean operation
> when possible. Seems we need a function to check whether one range
> contains another range, which is not available currently?
>
> I would not be worried about callers treating `0..1` as inclusive: this
> is a Rust project anyway, we need to learn the correct semantics of
> expressions eventually ;-)

Right, your comment made me realize that my concern could apply to all
uses of the ranges, including the basic and common slice access. So
following the expected semantics and offering the caller the option to
use an inclusive or non-inclusive range through `RangeBounds` indeed
sounds like the way to go.

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Daniel Almeida]

Hi,

> On 14 Jun 2025, at 10:38, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> Hi Daniel,
> 
> On Tue Jun 10, 2025 at 11:14 PM JST, Daniel Almeida wrote:
>> In light of bindgen being unable to generate bindings for macros, and
>> owing to the widespread use of these macros in drivers, manually define
>> the bit and genmask C macros in Rust.
>> 
>> The *_checked version of the functions provide runtime checking while
>> the const version performs compile-time assertions on the arguments via
>> the build_assert!() macro.
> 
> I think this is the right approach, I wish we could make the functions
> generic but that doesn't appear to be currently possible (and it
> wouldn't make their invocation shorter anyway).
> 
> I agree with Miguel's suggestion to use paste to shorten the macro
> syntax, it would make it much easier to understand them (and most of the
> naming could be harmonized in the macro itself instead of relying on the
> invocations to use the same names).
> 

OK.

[…]

>> 
>> diff --git a/rust/kernel/bits.rs b/rust/kernel/bits.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..98065c8f7c94cfc3b076e041de190e942e1b4a9f
>> --- /dev/null
>> +++ b/rust/kernel/bits.rs
>> @@ -0,0 +1,168 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Bit manipulation macros.
>> +//!
>> +//! C header: [`include/linux/bits.h`](srctree/include/linux/bits.h)
>> +
>> +use crate::build_assert;
>> +use core::ops::Range;
>> +
>> +macro_rules! impl_bit_fn {
>> +    (
>> +        $checked_name:ident, $unbounded_name:ident, $const_name:ident, $ty:ty
>> +    ) => {
>> +        /// Computes `1 << n` if `n` is in bounds, i.e.: if `n` is smaller than
>> +        /// the maximum number of bits supported by the type.
>> +        ///
>> +        /// Returns [`None`] otherwise.
>> +        #[inline]
>> +        pub fn $checked_name(n: u32) -> Option<$ty> {
>> +            (1 as $ty) .checked_shl(n)
>> +        }
>> +
>> +        /// Computes `1 << n` if `n` is in bounds, i.e.: if `n` is smaller than
>> +        /// the maximum number of bits supported by the type.
>> +        ///
>> +        /// Returns `0` otherwise.
>> +        ///
>> +        /// This is a convenience, as [`Option::unwrap_or`] cannot be used in
>> +        /// const-context.
>> +        #[inline]
>> +        pub fn $unbounded_name(n: u32) -> $ty {
>> +            match $checked_name(n) {
>> +                Some(v) => v,
>> +                None => 0,
>> +            }
> 
> This could more succintly be `$checked_name(n).unwrap_or(0)` (same
> remark for `$genmask_unbounded` below).
> 

Wait, I just realized that $unbounded_name is not ‘const fn’, so we don’t need this function at all?

Users can simply do `unwrap_or` on their own.

>> +        }
>> +
>> +        /// Computes `1 << n` by performing a compile-time assertion that `n` is
>> +        /// in bounds.
>> +        ///
>> +        /// This version is the default and should be used if `n` is known at
>> +        /// compile time.
>> +        #[inline]
>> +        pub const fn $const_name(n: u32) -> $ty {
>> +            build_assert!(n < <$ty>::BITS);
>> +            1 as $ty << n
>> +        }
>> +    };
>> +}
>> +
>> +impl_bit_fn!(checked_bit_u64, unbounded_bit_u64, bit_u64, u64);
>> +impl_bit_fn!(checked_bit_u32, unbounded_bit_u32, bit_u32, u32);
>> +impl_bit_fn!(checked_bit_u16, unbounded_bit_u16, bit_u16, u16);
>> +impl_bit_fn!(checked_bit_u8, unbounded_bit_u8, bit_u8, u8);
>> +
>> +macro_rules! impl_genmask_fn {
>> +    (
>> +        $ty:ty, $checked_bit:ident, $bit:ident, $genmask:ident, $genmask_checked:ident, $genmask_unbounded:ident,
>> +        $(#[$genmask_ex:meta])*
>> +    ) => {
>> +        /// Creates a compile-time contiguous bitmask for the given range by
>> +        /// validating the range at runtime.
>> +        ///
>> +        /// Returns [`None`] if the range is invalid, i.e.: if the start is
>> +        /// greater than or equal to the end.
>> +        #[inline]
>> +        pub fn $genmask_checked(range: Range<u32>) -> Option<$ty> {
>> +            if range.start >= range.end || range.end > <$ty>::BITS {
>> +                return None;
>> +            }
> 
> From this check I assumed that you interpret `range` as non-inclusive,
> since `range.end == 32` is valid on u32...
> 
>> +            let high = $checked_bit(range.end)?;
> 
> ... however IIUC `checked_bit` will return `None` here in such a case.
> Should the argument be `range.end - 1`?
> 
> Your examples do seem to interpret the range as inclusive though, so
> probably the check should be `|| range.end >= <$ty>::BITS`. But that
> triggers the question, is it ok to use `Range` that way, when its
> documentation specifically states that it is bounded exclusively above?
> We could use `RangeInclusive` to match the semantics, which would
> require us to write the ranges as `0..=7`. At least it is clear that the
> upper bound is inclusive.

Sorry, the idea was to indeed interpret a..b as inclusive. I specifically
thought we'd suprise a lot of people if we deviated from the way genmask works
in C. In other words, I assumed a lot of people would write a..b, when what
they meant is a..=b.

> 
> ... or we make the methods generic against `RangeBounds` and allow both
> `Range` and `RangeInclusive` to be used. But I'm concerned that callers
> might use `0..1` thinking it is inclusive while it is not.
> 
> Thoughts?

I don't think we can do what you suggested here. I assume that we'd have to
rely on [0] and friends, and these are not const fn, so they can’t be used in
the const version of genmask.

> 
>> +            let low = $checked_bit(range.start)?;
>> +            Some((high | (high - 1)) & !(low - 1))
>> +        }
>> +
>> +        /// Creates a compile-time contiguous bitmask for the given range by
>> +        /// validating the range at runtime.
>> +        ///
>> +        /// Returns `0` if the range is invalid, i.e.: if the start is greater
>> +        /// than or equal to the end.
>> +        #[inline]
>> +        pub fn $genmask_unbounded(range: Range<u32>) -> $ty {
>> +            match $genmask_checked(range) {
>> +                Some(v) => v,
>> +                None => 0,
>> +            }
> 
> This could more succintly be `$genmask_checked(range).unwrap_or(0)`.

See my comment above. Maybe this function is not needed at all?

> 
>> +        }
>> +
>> +        /// Creates a compile-time contiguous bitmask for the given range by
>> +        /// performing a compile-time assertion that the range is valid.
>> +        ///
>> +        /// This version is the default and should be used if the range is known
>> +        /// at compile time.
>> +        $(#[$genmask_ex])*
>> +        #[inline]
>> +        pub const fn $genmask(range: Range<u32>) -> $ty {
>> +            build_assert!(range.start < range.end);
>> +            build_assert!(range.end <= <$ty>::BITS);
> 
> I guess this check also needs to be fixed.

Ok.

— Daniel

[0]: https://doc.rust-lang.org/src/core/ops/range.rs.html#781

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Boqun Feng]

On Mon, Jun 16, 2025 at 11:14:58AM -0300, Daniel Almeida wrote:
> Hi,
> 
[...]
> >> +        }
> >> +
> >> +        /// Computes `1 << n` by performing a compile-time assertion that `n` is
> >> +        /// in bounds.
> >> +        ///
> >> +        /// This version is the default and should be used if `n` is known at
> >> +        /// compile time.
> >> +        #[inline]
> >> +        pub const fn $const_name(n: u32) -> $ty {
> >> +            build_assert!(n < <$ty>::BITS);
> >> +            1 as $ty << n
> >> +        }
> >> +    };
> >> +}
> >> +
> >> +impl_bit_fn!(checked_bit_u64, unbounded_bit_u64, bit_u64, u64);
> >> +impl_bit_fn!(checked_bit_u32, unbounded_bit_u32, bit_u32, u32);
> >> +impl_bit_fn!(checked_bit_u16, unbounded_bit_u16, bit_u16, u16);
> >> +impl_bit_fn!(checked_bit_u8, unbounded_bit_u8, bit_u8, u8);
> >> +
> >> +macro_rules! impl_genmask_fn {
> >> +    (
> >> +        $ty:ty, $checked_bit:ident, $bit:ident, $genmask:ident, $genmask_checked:ident, $genmask_unbounded:ident,
> >> +        $(#[$genmask_ex:meta])*
> >> +    ) => {
> >> +        /// Creates a compile-time contiguous bitmask for the given range by
> >> +        /// validating the range at runtime.
> >> +        ///
> >> +        /// Returns [`None`] if the range is invalid, i.e.: if the start is
> >> +        /// greater than or equal to the end.
> >> +        #[inline]
> >> +        pub fn $genmask_checked(range: Range<u32>) -> Option<$ty> {
> >> +            if range.start >= range.end || range.end > <$ty>::BITS {
> >> +                return None;
> >> +            }
> > 
> > From this check I assumed that you interpret `range` as non-inclusive,
> > since `range.end == 32` is valid on u32...
> > 
> >> +            let high = $checked_bit(range.end)?;
> > 
> > ... however IIUC `checked_bit` will return `None` here in such a case.
> > Should the argument be `range.end - 1`?
> > 
> > Your examples do seem to interpret the range as inclusive though, so
> > probably the check should be `|| range.end >= <$ty>::BITS`. But that
> > triggers the question, is it ok to use `Range` that way, when its
> > documentation specifically states that it is bounded exclusively above?
> > We could use `RangeInclusive` to match the semantics, which would
> > require us to write the ranges as `0..=7`. At least it is clear that the
> > upper bound is inclusive.
> 
> Sorry, the idea was to indeed interpret a..b as inclusive. I specifically

Please don't do this.

> thought we'd suprise a lot of people if we deviated from the way genmask works
> in C. In other words, I assumed a lot of people would write a..b, when what
> they meant is a..=b.
> 

We should tell/educate people to do the right thing, if a..b is not
inclusive in Rust, then we should treat them as non-inclusive in Rust
kernel code. Otherwise you create confusion for no reason. My assumption
is that most people will ask "what's the right way to do this" first
instead of replicating the old way.

Regards,
Boqun

> > 
[...]

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Daniel Almeida]

Hi Boqun,

> 
> We should tell/educate people to do the right thing, if a..b is not
> inclusive in Rust, then we should treat them as non-inclusive in Rust
> kernel code. Otherwise you create confusion for no reason. My assumption
> is that most people will ask "what's the right way to do this" first
> instead of replicating the old way.
> 
> Regards,
> Boqun
> 

This is just my opinion, of course:

I _hardly_ believe this will be the case. When people see genmask and two
numbers, they expect the range to be inclusive, full stop (at least IMHO). That's how it has
worked for decades, so it’s only natural to expect this behavior to transfer over.

However, I do understand and agree with your point, and I will change the
implementation here to comply. Perhaps we can use some markdown to alert users?

— Daniel

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Daniel Almeida]

> On 16 Jun 2025, at 11:42, Daniel Almeida <daniel.almeida@collabora.com> wrote:
> 
> Hi Boqun,
> 
>> 
>> We should tell/educate people to do the right thing, if a..b is not
>> inclusive in Rust, then we should treat them as non-inclusive in Rust
>> kernel code. Otherwise you create confusion for no reason. My assumption
>> is that most people will ask "what's the right way to do this" first
>> instead of replicating the old way.
>> 
>> Regards,
>> Boqun
>> 
> 
> This is just my opinion, of course:
> 
> I _hardly_ believe this will be the case. When people see genmask and two
> numbers, they expect the range to be inclusive, full stop (at least IMHO). That's how it has
> worked for decades, so it’s only natural to expect this behavior to transfer over.
> 
> However, I do understand and agree with your point, and I will change the
> implementation here to comply. Perhaps we can use some markdown to alert users?
> 
> — Daniel

Or better yet, perhaps we should only support a..=b.

— Daniel 

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Alexandre Courbot]

On Mon Jun 16, 2025 at 11:45 PM JST, Daniel Almeida wrote:
>
>
>> On 16 Jun 2025, at 11:42, Daniel Almeida <daniel.almeida@collabora.com> wrote:
>> 
>> Hi Boqun,
>> 
>>> 
>>> We should tell/educate people to do the right thing, if a..b is not
>>> inclusive in Rust, then we should treat them as non-inclusive in Rust
>>> kernel code. Otherwise you create confusion for no reason. My assumption
>>> is that most people will ask "what's the right way to do this" first
>>> instead of replicating the old way.
>>> 
>>> Regards,
>>> Boqun
>>> 
>> 
>> This is just my opinion, of course:
>> 
>> I _hardly_ believe this will be the case. When people see genmask and two
>> numbers, they expect the range to be inclusive, full stop (at least IMHO). That's how it has
>> worked for decades, so it’s only natural to expect this behavior to transfer over.
>> 
>> However, I do understand and agree with your point, and I will change the
>> implementation here to comply. Perhaps we can use some markdown to alert users?
>> 
>> — Daniel
>
> Or better yet, perhaps we should only support a..=b.

... or just drop the ranges and do as Daniel initially did, using two
arguments. But I agree with Boqun that we should not deviate from the
official interpretation of ranges if we use them - the fact that `Range`
is exclusive on its upper bound is documented and a property of the type
itself.

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Daniel Almeida]

Hi Alex,

> On 16 Jun 2025, at 11:52, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> On Mon Jun 16, 2025 at 11:45 PM JST, Daniel Almeida wrote:
>> 
>> 
>>> On 16 Jun 2025, at 11:42, Daniel Almeida <daniel.almeida@collabora.com> wrote:
>>> 
>>> Hi Boqun,
>>> 
>>>> 
>>>> We should tell/educate people to do the right thing, if a..b is not
>>>> inclusive in Rust, then we should treat them as non-inclusive in Rust
>>>> kernel code. Otherwise you create confusion for no reason. My assumption
>>>> is that most people will ask "what's the right way to do this" first
>>>> instead of replicating the old way.
>>>> 
>>>> Regards,
>>>> Boqun
>>>> 
>>> 
>>> This is just my opinion, of course:
>>> 
>>> I _hardly_ believe this will be the case. When people see genmask and two
>>> numbers, they expect the range to be inclusive, full stop (at least IMHO). That's how it has
>>> worked for decades, so it’s only natural to expect this behavior to transfer over.
>>> 
>>> However, I do understand and agree with your point, and I will change the
>>> implementation here to comply. Perhaps we can use some markdown to alert users?
>>> 
>>> — Daniel
>> 
>> Or better yet, perhaps we should only support a..=b.
> 
> ... or just drop the ranges and do as Daniel initially did, using two
> arguments. But I agree with Boqun that we should not deviate from the
> official interpretation of ranges if we use them - the fact that `Range`
> is exclusive on its upper bound is documented and a property of the type
> itself.

By the same token, I agree that we should use ranges instead of two arguments,
if said two arguments represent a range anyways. So my vote is for a..=b JFYI.

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Boqun Feng]

On Mon, Jun 16, 2025 at 11:45:21AM -0300, Daniel Almeida wrote:
> 
> 
> > On 16 Jun 2025, at 11:42, Daniel Almeida <daniel.almeida@collabora.com> wrote:
> > 
> > Hi Boqun,
> > 
> >> 
> >> We should tell/educate people to do the right thing, if a..b is not
> >> inclusive in Rust, then we should treat them as non-inclusive in Rust
> >> kernel code. Otherwise you create confusion for no reason. My assumption
> >> is that most people will ask "what's the right way to do this" first
> >> instead of replicating the old way.
> >> 
> >> Regards,
> >> Boqun
> >> 
> > 
> > This is just my opinion, of course:
> > 
> > I _hardly_ believe this will be the case. When people see genmask and two
> > numbers, they expect the range to be inclusive, full stop (at least IMHO). That's how it has
> > worked for decades, so it´s only natural to expect this behavior to transfer over.
> > 

Well, there are always users who don't read the manual, but we shouldn't
encourage that ;-) Technically kernel internal API is unstable, so use
before fully understanding the semantics is a user risk.

And if we provided non-inclusive range as inclusive, there would be
complains (probably from the same people) that:

    for_each_bit(genmask(a..b), |i| { do_sth(i); });

doesn't behave the same as:

    for i in a..b { do_sth(i); }

And we cannot always make them happy ;-)

> > However, I do understand and agree with your point, and I will change the
> > implementation here to comply. Perhaps we can use some markdown to alert users?
> > 
> > - Daniel
> 
> Or better yet, perhaps we should only support a..=b.
> 

Yes, given the const function factor for now, and I think eventually
most people will get themselves more familiar with Rust syntax.

Regards,
Boqun

> - Daniel 

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Alexandre Courbot]

On Mon Jun 16, 2025 at 11:56 PM JST, Daniel Almeida wrote:
> Hi Alex,
>
>> On 16 Jun 2025, at 11:52, Alexandre Courbot <acourbot@nvidia.com> wrote:
>> 
>> On Mon Jun 16, 2025 at 11:45 PM JST, Daniel Almeida wrote:
>>> 
>>> 
>>>> On 16 Jun 2025, at 11:42, Daniel Almeida <daniel.almeida@collabora.com> wrote:
>>>> 
>>>> Hi Boqun,
>>>> 
>>>>> 
>>>>> We should tell/educate people to do the right thing, if a..b is not
>>>>> inclusive in Rust, then we should treat them as non-inclusive in Rust
>>>>> kernel code. Otherwise you create confusion for no reason. My assumption
>>>>> is that most people will ask "what's the right way to do this" first
>>>>> instead of replicating the old way.
>>>>> 
>>>>> Regards,
>>>>> Boqun
>>>>> 
>>>> 
>>>> This is just my opinion, of course:
>>>> 
>>>> I _hardly_ believe this will be the case. When people see genmask and two
>>>> numbers, they expect the range to be inclusive, full stop (at least IMHO). That's how it has
>>>> worked for decades, so it’s only natural to expect this behavior to transfer over.
>>>> 
>>>> However, I do understand and agree with your point, and I will change the
>>>> implementation here to comply. Perhaps we can use some markdown to alert users?
>>>> 
>>>> — Daniel
>>> 
>>> Or better yet, perhaps we should only support a..=b.
>> 
>> ... or just drop the ranges and do as Daniel initially did, using two
>> arguments. But I agree with Boqun that we should not deviate from the
>> official interpretation of ranges if we use them - the fact that `Range`
>> is exclusive on its upper bound is documented and a property of the type
>> itself.
>
> By the same token, I agree that we should use ranges instead of two arguments,
> if said two arguments represent a range anyways. So my vote is for a..=b JFYI.

That works for me, it has the benefit of being absolutely clear that the
range is inclusive.

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Alexandre Courbot]

On Mon Jun 16, 2025 at 11:14 PM JST, Daniel Almeida wrote:
>>> +macro_rules! impl_bit_fn {
>>> +    (
>>> +        $checked_name:ident, $unbounded_name:ident, $const_name:ident, $ty:ty
>>> +    ) => {
>>> +        /// Computes `1 << n` if `n` is in bounds, i.e.: if `n` is smaller than
>>> +        /// the maximum number of bits supported by the type.
>>> +        ///
>>> +        /// Returns [`None`] otherwise.
>>> +        #[inline]
>>> +        pub fn $checked_name(n: u32) -> Option<$ty> {
>>> +            (1 as $ty) .checked_shl(n)
>>> +        }
>>> +
>>> +        /// Computes `1 << n` if `n` is in bounds, i.e.: if `n` is smaller than
>>> +        /// the maximum number of bits supported by the type.
>>> +        ///
>>> +        /// Returns `0` otherwise.
>>> +        ///
>>> +        /// This is a convenience, as [`Option::unwrap_or`] cannot be used in
>>> +        /// const-context.
>>> +        #[inline]
>>> +        pub fn $unbounded_name(n: u32) -> $ty {
>>> +            match $checked_name(n) {
>>> +                Some(v) => v,
>>> +                None => 0,
>>> +            }
>> 
>> This could more succintly be `$checked_name(n).unwrap_or(0)` (same
>> remark for `$genmask_unbounded` below).
>> 
>
> Wait, I just realized that $unbounded_name is not ‘const fn’, so we don’t need this function at all?
>
> Users can simply do `unwrap_or` on their own.

Agreed, we can probably drop this.

>> 
>> ... or we make the methods generic against `RangeBounds` and allow both
>> `Range` and `RangeInclusive` to be used. But I'm concerned that callers
>> might use `0..1` thinking it is inclusive while it is not.
>> 
>> Thoughts?
>
> I don't think we can do what you suggested here. I assume that we'd have to
> rely on [0] and friends, and these are not const fn, so they can’t be used in
> the const version of genmask.

You are right, this cannot be used here. It's not a big loss, limiting
the API to inclusive ranges as discussed on the other thread might
actually end up being safer than having two options.

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Joel Fernandes]

On Sat, Jun 14, 2025 at 09:05:41AM -0700, Boqun Feng wrote:
> On Sat, Jun 14, 2025 at 08:56:36AM -0700, Boqun Feng wrote:
> > On Sat, Jun 14, 2025 at 08:06:04AM -0700, Boqun Feng wrote:
> > > On Sat, Jun 14, 2025 at 10:38:11PM +0900, Alexandre Courbot wrote:
> > > [...]
> > > > > +macro_rules! impl_genmask_fn {
> > > > > +    (
> > > > > +        $ty:ty, $checked_bit:ident, $bit:ident, $genmask:ident, $genmask_checked:ident, $genmask_unbounded:ident,
> > > > > +        $(#[$genmask_ex:meta])*
> > > > > +    ) => {
> > > > > +        /// Creates a compile-time contiguous bitmask for the given range by
> > > > > +        /// validating the range at runtime.
> > > > > +        ///
> > > > > +        /// Returns [`None`] if the range is invalid, i.e.: if the start is
> > > > > +        /// greater than or equal to the end.
> > > > > +        #[inline]
> > > > > +        pub fn $genmask_checked(range: Range<u32>) -> Option<$ty> {
> > > > > +            if range.start >= range.end || range.end > <$ty>::BITS {
> > > > > +                return None;
> > > > > +            }
> > > > 
> > > > From this check I assumed that you interpret `range` as non-inclusive,
> > > > since `range.end == 32` is valid on u32...
> > > > 
> > > > > +            let high = $checked_bit(range.end)?;
> > > > 
> > > > ... however IIUC `checked_bit` will return `None` here in such a case.
> > > > Should the argument be `range.end - 1`?
> > > > 
> > > > Your examples do seem to interpret the range as inclusive though, so
> > > > probably the check should be `|| range.end >= <$ty>::BITS`. But that
> > > > triggers the question, is it ok to use `Range` that way, when its
> > > > documentation specifically states that it is bounded exclusively above?
> > > > We could use `RangeInclusive` to match the semantics, which would
> > > > require us to write the ranges as `0..=7`. At least it is clear that the
> > > > upper bound is inclusive.
> > > > 
> > > > ... or we make the methods generic against `RangeBounds` and allow both
> > > > `Range` and `RangeInclusive` to be used. But I'm concerned that callers
> > > > might use `0..1` thinking it is inclusive while it is not.

I think if they use 0..1 and consider it as inclusive, then they just need to
learn Rust. If they are writing Rust, then not knowing Rust is going to cause
them issues anyway. ;-)

> > > > 
> > > 
> > > I think generic over `RangeBounds` is a good idea, and we should
> > > .is_emtpy() or .contains() instead of comparison + boolean operation
> > > when possible. Seems we need a function to check whether one range

I am also of the opinion that RangeBounds is a good idea. I think it may come
down to both classes of devs, those who have used genmask before in C and
expect inclusivity, and those who are using it for the first time in Rust -
the latter may almost always want to use the non-inclusive syntax, no? 

thanks,

 - Joel

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Daniel Almeida]

Hi Joel,

> 
>>>>> 
>>>> 
>>>> I think generic over `RangeBounds` is a good idea, and we should
>>>> .is_emtpy() or .contains() instead of comparison + boolean operation
>>>> when possible. Seems we need a function to check whether one range
> 
> I am also of the opinion that RangeBounds is a good idea. I think it may come
> down to both classes of devs, those who have used genmask before in C and
> expect inclusivity, and those who are using it for the first time in Rust -
> the latter may almost always want to use the non-inclusive syntax, no? 
> 
> thanks,
> 
> - Joel


Can’t do that in a const fn, and we really want a const fn as the default.
Hence my suggestion to only support a..=b, which is both correct and explicit.

— Daniel

Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

[Joel Fernandes]

Hi Daniel,

On 6/20/2025 9:48 AM, Daniel Almeida wrote:
> Hi Joel,
> 
>>>>> I think generic over `RangeBounds` is a good idea, and we should
>>>>> .is_emtpy() or .contains() instead of comparison + boolean operation
>>>>> when possible. Seems we need a function to check whether one range
>> I am also of the opinion that RangeBounds is a good idea. I think it may come
>> down to both classes of devs, those who have used genmask before in C and
>> expect inclusivity, and those who are using it for the first time in Rust -
>> the latter may almost always want to use the non-inclusive syntax, no? 
>>
>> thanks,
>>
>> - Joel
> 
> Can’t do that in a const fn, and we really want a const fn as the default.
> Hence my suggestion to only support a..=b, which is both correct and explicit.

I haven't looked too deeply into the const issue, but the a..=b syntax is fine
with me.

thanks,

 - Joel