Message-Id: <DB1LPLOF0O0R.YQBJ0HBDUSKA@nvidia.com>
Date: Wed, 02 Jul 2025 22:25:02 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Daniel Almeida" <daniel.almeida@...labora.com>, "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>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "Danilo Krummrich" <dakr@...nel.org>,
 "Benno Lossin" <lossin@...nel.org>
Cc: <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v7] rust: kernel: add support for bits/genmask macros

Hi Daniel,

On Tue Jun 24, 2025 at 5:17 AM 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.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.com>

Overall this looks great ; I especially like how succint this has
become. I think I found a couple of last remaining issues, please check
below.

<snip>
> diff --git a/rust/kernel/bits.rs b/rust/kernel/bits.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..8db122b5db9565b76c14fc33e33058f9dac7bd75
> --- /dev/null
> +++ b/rust/kernel/bits.rs
> @@ -0,0 +1,133 @@
> +// 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_ex:meta])*
> +    ) => {
> +        paste! {
> +            /// Creates a compile-time contiguous bitmask for the given range by

This is the checked version, so IIUC the bitmask is not created at
compile-time?

> +            /// 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.

"... or is out of the representable range for $ty."

> +            #[inline]

Can we give examples for this function as well?

> +            pub fn [<genmask_checked_ $ty>](range: RangeInclusive<u32>) -> Option<$ty> {
> +                let start = *range.start();
> +                let end = *range.end();
> +
> +                if start >= end || end >= <$ty>::BITS {

The range is inclusive, so something like `0..=0` is valid (and returns
a mask of `0x1`). I think we want `start > end`?

Also, since you are using `checked_bit_*` below, I think the `end >=
<$ty>::BITS` check is redundant.

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

`start <= end`

> +                build_assert!(end < <$ty>::BITS);

Here also this check looks redundant, since `bit_*` will fail if the
shift doesn't fit into $ty.

> +
> +                let high = [<bit_ $ty>](end);
> +                let low = [<bit_ $ty>](start);
> +                (high | (high - 1)) & !(low - 1)
> +            }
> +        }
> +    };
> +}
> +
> +impl_genmask_fn!(
> +    u64,
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # use kernel::bits::genmask_u64;
> +    /// let mask = genmask_u64(21..=39);
> +    /// assert_eq!(mask, 0x000000ffffe00000);

I think we should also have 2 examples that show the behavior at the
limits (i.e. `0..=0` and `0..=63` here ; possibly others if you can see
interesting cases.). They are useful to understand how the function
actually works, and would also have caught the errors I pointed out
above.

Powered by blists - more mailing lists