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

["Alexandre Courbot" <acourbot@nvidia.com>]

On Wed Jul 9, 2025 at 1:50 AM JST, Daniel Almeida wrote:
<snip>
> +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 or equal to the end or if the range is outside of

Should be: "if the start is greater than the end", as the two being
equal is valid.

> +            /// 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
> +    ///
> +    /// The example below highlights the default use case, i.e., when the range
> +    /// is being built from non-constant values, which are represented here as
> +    /// the function arguments `a` and `b`.
> +    ///
> +    /// ```
> +    /// fn build_mask(a: u32, b: u32) -> Option<u64> {
> +    ///     # use kernel::bits::genmask_checked_u64;
> +    ///     // Ensures that a valid mask can be constructed for the range
> +    ///     // `a..=b` by performing runtime checks.
> +    ///     genmask_checked_u64(a..=b)
> +    /// }
> +    /// ```

This example just wraps `genmask_checked` into another function and
doesn't test it at all. I think it would be more useful if it mirrored
the non-checked equivalent, and also showed cases where `None` is
returned:

    # use kernel::bits::genmask_u64;
    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);

Sure, here one should just use `genmask_u64`, but the function's
description already says that - here we just want to show how it works,
and test that it behaves as expected.

> +    ///
> +    /// This example tests an edge case where only the first bit is
> +    /// supposed to be set.

I don't think these comments are necessary as the examples are rather
obvious. You can put all these examples into the same code block,
separated by a blank line, without the `mask` temporary variable. Just
add an inline comment if some explanation is required for some of them.

With these last things:

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>