linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v9] rust: kernel: add support for bits/genmask macros
@ 2025-07-14 23:29 Daniel Almeida
  2025-07-16 18:38 ` Danilo Krummrich
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Daniel Almeida @ 2025-07-14 23:29 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich
  Cc: linux-kernel, rust-for-linux, Alexandre Courbot, 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.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
Changes in v9:
- Rebased on rust-next
- Changed the examples to what Alex suggested (Alex)
- Placed them in a single code block (Alex)
- Fixed a minor typo in the docs, because a..=a is allowed (Alex)
- Added Alex's r-b.
- Link to v8: https://lore.kernel.org/rust-for-linux/20250708-topic-panthor-rs-genmask-v8-1-fb4256744c9b@collabora.com/

Changes in v8:
Thanks, Alex {
  - Added examples to genmask_checked_*
  - Removed the term "compile-checked" from the _checked variants
  - Add "or is out of the representable range for the type"
  - Change to "if start > end" in genmask_checked_* (as n..=n is valid)
  - Removed the extra check to $ty::BITS (which is already checked by
    the bit macro)
  - Changed the build_assert condition to "start <= end" in genmask_*
  - Added examples for min and max, i.e. 0..=0 and 0..=$ty::BITS-1
  - Separated hex values using underscore where applicable
}
- Link to v7: https://lore.kernel.org/r/20250623-topic-panthor-rs-genmask-v7-1-9f986951e7b5@collabora.com

Changes in v7:
- Rebase on top of latest rust-next
- Use RangeInclusive
- Fix formatting
- Fix checks of start/end to account for RangeInclusive
- Use the paste macro to simplify the implementation
- Get rid of the _unbounded versions (they were not const fns anyway, so
  users can just write it themselves)
- Change the examples to account for RangeInclusive
- Link to v6: https://lore.kernel.org/r/20250610-topic-panthor-rs-genmask-v6-1-50fa1a981bc1@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 | 223 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs  |   1 +
 2 files changed, 224 insertions(+)

diff --git a/rust/kernel/bits.rs b/rust/kernel/bits.rs
new file mode 100644
index 0000000000000000000000000000000000000000..a15cb4fc0fa93a309d73e822bfd5eb469f2077ef
--- /dev/null
+++ b/rust/kernel/bits.rs
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Bit manipulation macros.
+//!
+//! C header: [`include/linux/bits.h`](srctree/include/linux/bits.h)
+
+use crate::prelude::*;
+use core::ops::RangeInclusive;
+use macros::paste;
+
+macro_rules! impl_bit_fn {
+    (
+        $ty:ty
+    ) => {
+        paste! {
+            /// 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_bit_ $ty>](n: u32) -> Option<$ty> {
+                (1 as $ty).checked_shl(n)
+            }
+
+            /// 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 [<bit_ $ty>](n: u32) -> $ty {
+                build_assert!(n < <$ty>::BITS);
+                (1 as $ty) << n
+            }
+        }
+    };
+}
+
+impl_bit_fn!(u64);
+impl_bit_fn!(u32);
+impl_bit_fn!(u16);
+impl_bit_fn!(u8);
+
+macro_rules! impl_genmask_fn {
+    (
+        $ty:ty,
+        $(#[$genmask_checked_ex:meta])*,
+        $(#[$genmask_ex:meta])*
+    ) => {
+        paste! {
+            /// Creates a 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 the end or if the range is outside of the
+            /// representable range for the type.
+            $(#[$genmask_checked_ex])*
+            #[inline]
+            pub fn [<genmask_checked_ $ty>](range: RangeInclusive<u32>) -> Option<$ty> {
+                let start = *range.start();
+                let end = *range.end();
+
+                if start > end {
+                    return None;
+                }
+
+                let high = [<checked_bit_ $ty>](end)?;
+                let low = [<checked_bit_ $ty>](start)?;
+                Some((high | (high - 1)) & !(low - 1))
+            }
+
+            /// 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_ $ty>](range: RangeInclusive<u32>) -> $ty {
+                let start = *range.start();
+                let end = *range.end();
+
+                build_assert!(start <= end);
+
+                let high = [<bit_ $ty>](end);
+                let low = [<bit_ $ty>](start);
+                (high | (high - 1)) & !(low - 1)
+            }
+        }
+    };
+}
+
+impl_genmask_fn!(
+    u64,
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::bits::genmask_checked_u64;
+    ///
+    /// assert_eq!(genmask_checked_u64(0..=0), Some(0b1));
+    ///
+    /// assert_eq!(genmask_checked_u64(0..=63), Some(u64::MAX));
+    ///
+    /// assert_eq!(genmask_checked_u64(21..=39), Some(0x0000_00ff_ffe0_0000));
+    ///
+    /// // `80` is out of the supported bit range.
+    /// assert_eq!(genmask_checked_u64(21..=80), None);
+    ///
+    /// // Invalid range where the start is bigger than the end.
+    /// assert_eq!(genmask_checked_u64(15..=8), None);
+    /// ```
+    ,
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::bits::genmask_u64;
+    ///
+    /// assert_eq!(genmask_u64(21..=39), 0x0000_00ff_ffe0_0000);
+    ///
+    /// assert_eq!(genmask_u64(0..=0), 0b1);
+    ///
+    /// assert_eq!(genmask_u64(0..=63), u64::MAX);
+    /// ```
+);
+
+impl_genmask_fn!(
+    u32,
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::bits::genmask_checked_u32;
+    ///
+    /// assert_eq!(genmask_checked_u32(0..=0), Some(0b1));
+    ///
+    /// assert_eq!(genmask_checked_u32(0..=31), Some(u32::MAX));
+    ///
+    /// assert_eq!(genmask_checked_u32(21..=31), Some(0xffe0_0000));
+    ///
+    /// // `40` is out of the supported bit range.
+    /// assert_eq!(genmask_checked_u32(21..=40), None);
+    ///
+    /// // Invalid range where the start is bigger than the end.
+    /// assert_eq!(genmask_checked_u32(15..=8), None);
+    /// ```
+    ,
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::bits::genmask_u32;
+    ///
+    /// assert_eq!(genmask_u32(21..=31), 0xffe0_0000);
+    ///
+    /// assert_eq!(genmask_u32(0..=0), 0b1);
+    ///
+    /// assert_eq!(genmask_u32(0..=31), u32::MAX);
+    /// ```
+);
+
+impl_genmask_fn!(
+    u16,
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::bits::genmask_checked_u16;
+    ///
+    /// assert_eq!(genmask_checked_u16(0..=0), Some(0b1));
+    ///
+    /// assert_eq!(genmask_checked_u16(0..=15), Some(u16::MAX));
+    ///
+    /// assert_eq!(genmask_checked_u16(6..=15), Some(0xffc0));
+    ///
+    /// // `20` is out of the supported bit range.
+    /// assert_eq!(genmask_checked_u16(6..=20), None);
+    ///
+    /// // Invalid range where the start is bigger than the end.
+    /// assert_eq!(genmask_checked_u16(10..=5), None);
+    /// ```
+    ,
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::bits::genmask_u16;
+    ///
+    /// assert_eq!(genmask_u16(6..=15), 0xffc0);
+    ///
+    /// assert_eq!(genmask_u16(0..=0), 0b1);
+    ///
+    /// assert_eq!(genmask_u16(0..=15), u16::MAX);
+    /// ```
+);
+
+impl_genmask_fn!(
+    u8,
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::bits::genmask_checked_u8;
+    ///
+    /// assert_eq!(genmask_checked_u8(0..=0), Some(0b1));
+    ///
+    /// assert_eq!(genmask_checked_u8(0..=7), Some(u8::MAX));
+    ///
+    /// assert_eq!(genmask_checked_u8(6..=7), Some(0xc0));
+    ///
+    /// // `10` is out of the supported bit range.
+    /// assert_eq!(genmask_checked_u8(6..=10), None);
+    ///
+    /// // Invalid range where the start is bigger than the end.
+    /// assert_eq!(genmask_checked_u8(5..=2), None);
+    /// ```
+    ,
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::bits::genmask_u8;
+    ///
+    /// assert_eq!(genmask_u8(6..=7), 0xc0);
+    ///
+    /// assert_eq!(genmask_u8(0..=0), 0b1);
+    ///
+    /// assert_eq!(genmask_u8(0..=7), u8::MAX);
+    /// ```
+);
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6b4774b2b1c37f4da1866e993be6230bc6715841..1bb294de8cb000120a0d04f61d6f7262fbc9f600 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -54,6 +54,7 @@
 pub mod alloc;
 #[cfg(CONFIG_AUXILIARY_BUS)]
 pub mod auxiliary;
+pub mod bits;
 #[cfg(CONFIG_BLOCK)]
 pub mod block;
 #[doc(hidden)]

---
base-commit: a68a6bef0e75fb9e5aea1399d8538f4e3584dab1
change-id: 20250714-topics-tyr-genmask2-1d7a0ba01e85

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v9] rust: kernel: add support for bits/genmask macros
  2025-07-14 23:29 [PATCH v9] rust: kernel: add support for bits/genmask macros Daniel Almeida
@ 2025-07-16 18:38 ` Danilo Krummrich
  2025-07-16 19:11   ` Daniel Almeida
  2025-07-16 20:07 ` Danilo Krummrich
  2025-07-19 22:17 ` Miguel Ojeda
  2 siblings, 1 reply; 10+ messages in thread
From: Danilo Krummrich @ 2025-07-16 18:38 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, linux-kernel, rust-for-linux, Alexandre Courbot

On Tue Jul 15, 2025 at 1:29 AM CEST, Daniel Almeida wrote:
> +macro_rules! impl_genmask_fn {
> +    (
> +        $ty:ty,
> +        $(#[$genmask_checked_ex:meta])*,
> +        $(#[$genmask_ex:meta])*
> +    ) => {
> +        paste! {
> +            /// Creates a 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 the end or if the range is outside of the
> +            /// representable range for the type.
> +            $(#[$genmask_checked_ex])*
> +            #[inline]
> +            pub fn [<genmask_checked_ $ty>](range: RangeInclusive<u32>) -> Option<$ty> {
> +                let start = *range.start();
> +                let end = *range.end();
> +
> +                if start > end {
> +                    return None;
> +                }
> +
> +                let high = [<checked_bit_ $ty>](end)?;
> +                let low = [<checked_bit_ $ty>](start)?;
> +                Some((high | (high - 1)) & !(low - 1))
> +            }
> +
> +            /// 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_ $ty>](range: RangeInclusive<u32>) -> $ty {
> +                let start = *range.start();
> +                let end = *range.end();
> +
> +                build_assert!(start <= end);
> +
> +                let high = [<bit_ $ty>](end);
> +                let low = [<bit_ $ty>](start);
> +                (high | (high - 1)) & !(low - 1)
> +            }
> +        }
> +    };
> +}

Just for reference, I asked some questions regarding this code in [1].

Additional to "Why does genmask_u64(0..=100) compile?" I would expect the
corresponding genmask_checked_u64() call to return None.

[1] https://lore.kernel.org/lkml/DBDP0BJW9VAZ.5KRU4V4288R8@kernel.org/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v9] rust: kernel: add support for bits/genmask macros
  2025-07-16 18:38 ` Danilo Krummrich
@ 2025-07-16 19:11   ` Daniel Almeida
  2025-07-16 19:18     ` Daniel Almeida
  2025-07-16 19:32     ` Danilo Krummrich
  0 siblings, 2 replies; 10+ messages in thread
From: Daniel Almeida @ 2025-07-16 19:11 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, linux-kernel, rust-for-linux, Alexandre Courbot

Hi Danilo,

> 
> Just for reference, I asked some questions regarding this code in [1].
> 
> Additional to "Why does genmask_u64(0..=100) compile?" I would expect the
> corresponding genmask_checked_u64() call to return None.
> 
> [1] https://lore.kernel.org/lkml/DBDP0BJW9VAZ.5KRU4V4288R8@kernel.org/
> 

Let’s transfer this discussion to this patch.

> I also quickly tried genmask and I have a few questions:
> 
>  (1) Why does genmask not use a const generic? I think this makes it more
>      obvious that it's only intended to be used from const context.

I guess none of us thought about it, since the current version also works.

> 
>  (2) Why is there no build_assert() when the range exceeds the number of bits
>      of the target type? I would expect genmask_u64(0..100) to fail.

Doesn’t it?

There is a build_assert in the underlying bit implementation. It was redundant
to have it both in bit_* and in genmask, because genmask calls bit().

In your example, bit_u64(100) hits that assert, and so it shouldn't compile.

Note that I can't add a test for this because "compile_fail" tests are not
supported yet, but there is a test for the checked version, i.e.:

+    /// // `80` is out of the supported bit range.
+    /// assert_eq!(genmask_checked_u64(21..=80), None);

> Additional to "Why does genmask_u64(0..=100) compile?" I would expect the
> corresponding genmask_checked_u64() call to return None.

The test above proves that it returns None.

> 
>  (3) OOC, why did you choose u32 as argument type?

No reason. i32 is the default integer type and signed integers don’t make
sense here, so I chose u32.

Also, we can trivially promote integers to wider types,  but the other way
around is not true. So my reasoning was that if you had u8, or u16s you could
trivially get u32s using into(), but if you had u32s and e.g. genmask_u16
required u16s, you'd have to resort to try_into() or `as`, which is annoying.

In any case, feel free to suggest anything else, I think.

— Daniel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v9] rust: kernel: add support for bits/genmask macros
  2025-07-16 19:11   ` Daniel Almeida
@ 2025-07-16 19:18     ` Daniel Almeida
  2025-07-16 19:32     ` Danilo Krummrich
  1 sibling, 0 replies; 10+ messages in thread
From: Daniel Almeida @ 2025-07-16 19:18 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, linux-kernel, rust-for-linux, Alexandre Courbot


> 
>> 
>> (3) OOC, why did you choose u32 as argument type?
> 
> No reason. i32 is the default integer type and signed integers don’t make
> sense here, so I chose u32.
> 
> Also, we can trivially promote integers to wider types,  but the other way
> around is not true. So my reasoning was that if you had u8, or u16s you could
> trivially get u32s using into(), but if you had u32s and e.g. genmask_u16
> required u16s, you'd have to resort to try_into() or `as`, which is annoying.
> 
> In any case, feel free to suggest anything else, I think.
> 
> — Daniel

I guess that, using the logic above, one could ask "why not u64 then?".

64bit variables are less commonly used, so this would be the inverse
problem, i.e.: we'd see way too many uses of into().

— Daniel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v9] rust: kernel: add support for bits/genmask macros
  2025-07-16 19:11   ` Daniel Almeida
  2025-07-16 19:18     ` Daniel Almeida
@ 2025-07-16 19:32     ` Danilo Krummrich
  2025-07-16 19:44       ` Daniel Almeida
  1 sibling, 1 reply; 10+ messages in thread
From: Danilo Krummrich @ 2025-07-16 19:32 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, linux-kernel, rust-for-linux, Alexandre Courbot

On Wed Jul 16, 2025 at 9:11 PM CEST, Daniel Almeida wrote:
> Let’s transfer this discussion to this patch.
>
>> I also quickly tried genmask and I have a few questions:
>> 
>>  (1) Why does genmask not use a const generic? I think this makes it more
>>      obvious that it's only intended to be used from const context.
>
> I guess none of us thought about it, since the current version also works.

I think using a const generic would be a bit better for the mentioned reason.

>> 
>>  (2) Why is there no build_assert() when the range exceeds the number of bits
>>      of the target type? I would expect genmask_u64(0..100) to fail.
>
> Doesn’t it?
>
> There is a build_assert in the underlying bit implementation. It was redundant
> to have it both in bit_* and in genmask, because genmask calls bit().
>
> In your example, bit_u64(100) hits that assert, and so it shouldn't compile.

Indeed, and it also works, except from doc-tests for some reason, which is what
I tried real quick. :)

>>  (3) OOC, why did you choose u32 as argument type?
>
> No reason. i32 is the default integer type and signed integers don’t make
> sense here, so I chose u32.
>
> Also, we can trivially promote integers to wider types,  but the other way
> around is not true. So my reasoning was that if you had u8, or u16s you could
> trivially get u32s using into(), but if you had u32s and e.g. genmask_u16
> required u16s, you'd have to resort to try_into() or `as`, which is annoying.
>
> In any case, feel free to suggest anything else, I think.

I feel like usize would be a better fit, but not a strong opinion.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v9] rust: kernel: add support for bits/genmask macros
  2025-07-16 19:32     ` Danilo Krummrich
@ 2025-07-16 19:44       ` Daniel Almeida
  2025-07-16 19:49         ` Danilo Krummrich
  0 siblings, 1 reply; 10+ messages in thread
From: Daniel Almeida @ 2025-07-16 19:44 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, linux-kernel, rust-for-linux, Alexandre Courbot



> On 16 Jul 2025, at 16:32, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Wed Jul 16, 2025 at 9:11 PM CEST, Daniel Almeida wrote:
>> Let’s transfer this discussion to this patch.
>> 
>>> I also quickly tried genmask and I have a few questions:
>>> 
>>> (1) Why does genmask not use a const generic? I think this makes it more
>>>     obvious that it's only intended to be used from const context.
>> 
>> I guess none of us thought about it, since the current version also works.
> 
> I think using a const generic would be a bit better for the mentioned reason.

Btw, how does monomorphization work here? Would we have to codegen all the
versions? Also, I don't think that you can take a range as a const generic
argument, i.e., I don't recall ever seeing this syntax:

genmask_u64::<0..=63>();

So we'd have to go back to taking two arguments, and it's less ergonomic than
a..=b (which was discussed a bit at length during the previous iterations).
Also it would stand out against the non-const (i.e. checked) versions, which would
still take a range as argument.

> 
>>> 
>>> (2) Why is there no build_assert() when the range exceeds the number of bits
>>>     of the target type? I would expect genmask_u64(0..100) to fail.
>> 
>> Doesn’t it?
>> 
>> There is a build_assert in the underlying bit implementation. It was redundant
>> to have it both in bit_* and in genmask, because genmask calls bit().
>> 
>> In your example, bit_u64(100) hits that assert, and so it shouldn't compile.
> 
> Indeed, and it also works, except from doc-tests for some reason, which is what
> I tried real quick. :)
> 

Wait, this was a bit confusing :)
You’re confirming that it doesn’t compile, correct?

> I feel like usize would be a better fit, but not a strong opinion.

I guess this is the same problem as u64: drivers will usually have either
i32s/u32s and this would require a cast.

— Daniel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v9] rust: kernel: add support for bits/genmask macros
  2025-07-16 19:44       ` Daniel Almeida
@ 2025-07-16 19:49         ` Danilo Krummrich
  2025-07-16 20:06           ` Danilo Krummrich
  0 siblings, 1 reply; 10+ messages in thread
From: Danilo Krummrich @ 2025-07-16 19:49 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, linux-kernel, rust-for-linux, Alexandre Courbot

On Wed Jul 16, 2025 at 9:44 PM CEST, Daniel Almeida wrote:
>
>
>> On 16 Jul 2025, at 16:32, Danilo Krummrich <dakr@kernel.org> wrote:
>> 
>> On Wed Jul 16, 2025 at 9:11 PM CEST, Daniel Almeida wrote:
>>> Let’s transfer this discussion to this patch.
>>> 
>>>> I also quickly tried genmask and I have a few questions:
>>>> 
>>>> (1) Why does genmask not use a const generic? I think this makes it more
>>>>     obvious that it's only intended to be used from const context.
>>> 
>>> I guess none of us thought about it, since the current version also works.
>> 
>> I think using a const generic would be a bit better for the mentioned reason.
>
> Btw, how does monomorphization work here? Would we have to codegen all the
> versions? Also, I don't think that you can take a range as a const generic
> argument, i.e., I don't recall ever seeing this syntax:
>
> genmask_u64::<0..=63>();

Ah, of course, it's RangeInclusive, just scratch the const generic proposal. :)

>>>> (2) Why is there no build_assert() when the range exceeds the number of bits
>>>>     of the target type? I would expect genmask_u64(0..100) to fail.
>>> 
>>> Doesn’t it?
>>> 
>>> There is a build_assert in the underlying bit implementation. It was redundant
>>> to have it both in bit_* and in genmask, because genmask calls bit().
>>> 
>>> In your example, bit_u64(100) hits that assert, and so it shouldn't compile.
>> 
>> Indeed, and it also works, except from doc-tests for some reason, which is what
>> I tried real quick. :)
>> 
>
> Wait, this was a bit confusing :)
> You’re confirming that it doesn’t compile, correct?

Yes, except for in doc-tests for some reason.

>> I feel like usize would be a better fit, but not a strong opinion.
>
> I guess this is the same problem as u64: drivers will usually have either
> i32s/u32s and this would require a cast.

I don't understand this argument? We usualy use usize for such things, no?

>
> — Daniel


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v9] rust: kernel: add support for bits/genmask macros
  2025-07-16 19:49         ` Danilo Krummrich
@ 2025-07-16 20:06           ` Danilo Krummrich
  0 siblings, 0 replies; 10+ messages in thread
From: Danilo Krummrich @ 2025-07-16 20:06 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, linux-kernel, rust-for-linux, Alexandre Courbot

On Wed Jul 16, 2025 at 9:49 PM CEST, Danilo Krummrich wrote:
> On Wed Jul 16, 2025 at 9:44 PM CEST, Daniel Almeida wrote:
>> I guess this is the same problem as u64: drivers will usually have either
>> i32s/u32s and this would require a cast.
>
> I don't understand this argument? We usualy use usize for such things, no?

I already thought in terms of the register!() macro, which deals with target
types for us anyways. Without that, you're right, u32 is better.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v9] rust: kernel: add support for bits/genmask macros
  2025-07-14 23:29 [PATCH v9] rust: kernel: add support for bits/genmask macros Daniel Almeida
  2025-07-16 18:38 ` Danilo Krummrich
@ 2025-07-16 20:07 ` Danilo Krummrich
  2025-07-19 22:17 ` Miguel Ojeda
  2 siblings, 0 replies; 10+ messages in thread
From: Danilo Krummrich @ 2025-07-16 20:07 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, linux-kernel, rust-for-linux, Alexandre Courbot

On Tue Jul 15, 2025 at 1:29 AM CEST, 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.

Reviewed-by: Danilo Krummrich <dakr@kernel.org>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v9] rust: kernel: add support for bits/genmask macros
  2025-07-14 23:29 [PATCH v9] rust: kernel: add support for bits/genmask macros Daniel Almeida
  2025-07-16 18:38 ` Danilo Krummrich
  2025-07-16 20:07 ` Danilo Krummrich
@ 2025-07-19 22:17 ` Miguel Ojeda
  2 siblings, 0 replies; 10+ messages in thread
From: Miguel Ojeda @ 2025-07-19 22:17 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, linux-kernel, rust-for-linux,
	Alexandre Courbot

On Tue, Jul 15, 2025 at 1:30 AM Daniel Almeida
<daniel.almeida@collabora.com> 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.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>

Applied to `rust-next` -- thanks everyone!

    [ `expect`ed Clippy warning in doctests, hid single `use`, grouped
      examples. Reworded title. - Miguel ]

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-07-19 22:17 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-14 23:29 [PATCH v9] rust: kernel: add support for bits/genmask macros Daniel Almeida
2025-07-16 18:38 ` Danilo Krummrich
2025-07-16 19:11   ` Daniel Almeida
2025-07-16 19:18     ` Daniel Almeida
2025-07-16 19:32     ` Danilo Krummrich
2025-07-16 19:44       ` Daniel Almeida
2025-07-16 19:49         ` Danilo Krummrich
2025-07-16 20:06           ` Danilo Krummrich
2025-07-16 20:07 ` Danilo Krummrich
2025-07-19 22:17 ` Miguel Ojeda

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).