Message-Id: <9578ECFC-6C59-40E3-9340-A426E8D2328A@collabora.com>
Date: Mon, 16 Jun 2025 11:14:58 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>,
 Danilo Krummrich <dakr@...nel.org>,
 linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v6] rust: kernel: add support for bits/genmask macros

Hi,

> On 14 Jun 2025, at 10:38, Alexandre Courbot <acourbot@...dia.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

Powered by blists - more mailing lists