Message-ID: <20251215115521.1c85fe23.gary@garyguo.net>
Date: Mon, 15 Dec 2025 11:55:21 +0000
From: Gary Guo <gary@...yguo.net>
To: Filipe Xavier <felipeaggger@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>, 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>,
 daniel.almeida@...labora.com, rust-for-linux@...r.kernel.org,
 felipe_life@...e.com, linux-kernel@...r.kernel.org, Lyude Paul
 <lyude@...hat.com>
Subject: Re: [PATCH v6] rust: add new macro for common bitmap operations

On Fri, 05 Dec 2025 07:15:17 -0300
Filipe Xavier <felipeaggger@...il.com> wrote:

> We have seen a proliferation of mod_whatever::foo::Flags
> being defined with essentially the same implementation
> for BitAnd, BitOr, contains and etc.
> 
> This macro aims to bring a solution for this,
> allowing to generate these methods for user-defined structs.
> With some use cases in KMS and upcoming GPU drivers.
> 
> Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/We.20really.20need.20a.20common.20.60Flags.60.20type
> Signed-off-by: Filipe Xavier <felipeaggger@...il.com>
> Suggested-by: Daniel Almeida <daniel.almeida@...labora.com>
> Suggested-by: Lyude Paul <lyude@...hat.com>
> ---
> Changes in v6:
> - New methods: add new methods contains_any and contains_all.
> - Link to v5: https://lore.kernel.org/r/20251109-feat-add-bitmask-macro-v5-1-9d911b207ef4@gmail.com
> 
> Changes in v5:
> - Docs: improve macro documentation.
> - Link to v4: https://lore.kernel.org/r/20251026-feat-add-bitmask-macro-v4-1-e1b59b4762bc@gmail.com
> 
> Changes in v4:
> - Use enum: changed flag type from struct to enum.
> - Minor fix: airect casting (flag as $ty) instead of field access (.0).
> - Link to v3: https://lore.kernel.org/r/20250411-feat-add-bitmask-macro-v3-1-187bd3e4a03e@gmail.com
> 
> Changes in v3:
> - New Feat: added support to declare flags inside macro use.
> - Minor fixes: used absolute paths to refer to items, fix rustdoc and fix example cases.
> - Link to v2: https://lore.kernel.org/r/20250325-feat-add-bitmask-macro-v2-1-d3beabdad90f@gmail.com
> 
> Changes in v2:
> - rename: change macro and file name to impl_flags.
> - negation sign: change char for negation to `!`. 
> - transpose docs: add support to transpose user provided docs.
> - visibility: add support to use user defined visibility.
> - operations: add new operations for flag, 
> to support use between bit and bitmap, eg: flag & flags.
> - code style: small fixes to remove warnings.
> - Link to v1: https://lore.kernel.org/r/20250304-feat-add-bitmask-macro-v1-1-1c2d2bcb476b@gmail.com
> ---
>  rust/kernel/impl_flags.rs | 243 ++++++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs        |   2 +
>  rust/kernel/prelude.rs    |   1 +
>  3 files changed, 246 insertions(+)
> 
> diff --git a/rust/kernel/impl_flags.rs b/rust/kernel/impl_flags.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..aff6f14bc2f6097e78ba21bb13d124ff999b376a
> --- /dev/null
> +++ b/rust/kernel/impl_flags.rs
> @@ -0,0 +1,243 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/// Common helper for declaring bitflag and bitmask types.
> +///
> +/// This macro handles:
> +/// - A struct representing a bitmask, and an enumerator representing bitflags which
> +/// may be used in the aforementioned bitmask.
> +/// - Implementations of common bitmap op. ([`::core::ops::BitOr`], [`::core::ops::BitAnd`], etc.).
> +/// - Utility methods such as `.contains()` to check flags.
> +///
> +/// # Examples
> +///
> +/// Defining and using impl_flags:
> +///
> +/// ```
> +/// use kernel::impl_flags;
> +///
> +/// impl_flags!(
> +///     /// Represents multiple permissions.
> +///     #[derive(Debug, Clone, Default, Copy, PartialEq, Eq)]
> +///     pub struct Permissions(u32);
> +///     /// Represents a single permission.
> +///     #[derive(Debug, Clone, Copy, PartialEq, Eq)]
> +///     pub enum Permission {
> +///         Read    = 1 << 0,
> +///         Write   = 1 << 1,
> +///         Execute = 1 << 2,
> +///     }
> +/// );
> +///
> +/// // Combine multiple permissions using operation OR (`|`).
> +/// let read_write: Permissions = Permission::Read | Permission::Write;
> +///
> +/// assert!(read_write.contains(Permission::Read));
> +/// assert!(read_write.contains(Permission::Write));
> +/// assert!(!read_write.contains(Permission::Execute));
> +/// assert!(read_write.contains_any(Permission::Read | Permission::Execute));
> +/// assert!(read_write.contains_all(Permission::Read | Permission::Write));
> +///
> +/// // Removing a permission with operation AND (`&`).
> +/// let read_only: Permissions = read_write & Permission::Read;
> +/// assert!(read_only.contains(Permission::Read));
> +/// assert!(!read_only.contains(Permission::Write));
> +///
> +/// // Toggling permissions with XOR (`^`).
> +/// let toggled: Permissions = read_only ^ Permission::Read;
> +/// assert!(!toggled.contains(Permission::Read));
> +///
> +/// // Inverting permissions with negation (`!`).
> +/// let negated = !read_only;
> +/// assert!(negated.contains(Permission::Write));

Your implementation of `!` does not specify whether it may contain bit
flags that are not defined. In this example, `negated` contains bit 3~31
as well despite not defined anywhere. Same for `^` implementation.

AFAIK this is a similar behaviour to bitflags crate v1 and in v2 they're
changing the impl to unset unknown flags. For a bitflags that's only ever
used in the driver itself, either behaviour makes sense, but it might be a
safer approach to unset all unknown flags in flag operations.

bitflags 2.x provides a `_ = !0` approach to mark all bits as known.

(Yes I know you currently do not expose any methods to get raw values, but
we'll either have that in the future, or currently you can do `flags.0`
directly in the same module)

Best,
Gary

Powered by blists - more mailing lists