Message-Id: <16B126B0-DC90-480C-80F5-93EE9F922C71@collabora.com>
Date: Mon, 31 Mar 2025 19:35:02 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Lyude Paul <lyude@...hat.com>
Cc: Filipe Xavier <felipeaggger@...il.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>,
 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>,
 rust-for-linux@...r.kernel.org,
 felipe_life@...e.com,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] rust: add new macro for common bitmap operations

Hi Lyude,

> On 31 Mar 2025, at 19:29, Lyude Paul <lyude@...hat.com> wrote:
> 
> Sorry this took me a while to get back to, last week was a bit hectic. I
> realized there's a couple of changes we still need to make here (in addition
> to the other ones mentioned on the mailing list):
> 
> On Tue, 2025-03-25 at 10:10 -0300, Filipe Xavier 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 VideoCodecs.
>> 
>> Small use sample:
>> `
>> const READ: Permission = Permission(1 << 0);
>> const WRITE: Permission = Permission(1 << 1);
>> 
>> impl_flags!(Permissions, Permission, u32);
>> 
>> let read_write = Permissions::from(READ) | WRITE;
>> let read_only = read_write & READ;
>> `
>> 
>> 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 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 | 214 ++++++++++++++++++++++++++++++++++++++++++++++
>> rust/kernel/lib.rs        |   1 +
>> rust/kernel/prelude.rs    |   1 +
>> 3 files changed, 216 insertions(+)
>> 
>> diff --git a/rust/kernel/impl_flags.rs b/rust/kernel/impl_flags.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..e7cf00e14bdcd2acea47b8c158a984ac0206568b
>> --- /dev/null
>> +++ b/rust/kernel/impl_flags.rs
>> @@ -0,0 +1,214 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! impl_flags utilities for working with flags.
>> +
>> +/// Declares a impl_flags type with its corresponding flag type.
>> +///
>> +/// This macro generates:
>> +/// - Implementations of common bitmask operations ([`BitOr`], [`BitAnd`], etc.).
>> +/// - Utility methods such as `.contains()` to check flags.
>> +///
>> +/// # Examples
>> +///
>> +/// Defining and using impl_flags:
>> +///
>> +/// ```
>> +/// impl_flags!(
>> +///     /// Represents multiple permissions.
>> +///     pub Permissions,
>> +///     /// Represents a single permission.
>> +///     pub Permission,
>> +///     u32
>> +/// );
>> +///
>> +/// // Define some individual permissions.
>> +/// const READ: Permission = Permission(1 << 0);
>> +/// const WRITE: Permission = Permission(1 << 1);
>> +/// const EXECUTE: Permission = Permission(1 << 2);
>> +///
>> +/// // Combine multiple permissions using operation OR (`|`).
>> +/// let read_write = Permissions::from(READ) | WRITE;
>> +///
>> +/// assert!(read_write.contains(READ));
>> +/// assert!(read_write.contains(WRITE));
>> +/// assert!(!read_write.contains(EXECUTE));
>> +///
>> +/// // Removing a permission with operation AND (`&`).
>> +/// let read_only = read_write & READ;
>> +/// assert!(read_only.contains(READ));
>> +/// assert!(!read_only.contains(WRITE));
>> +///
>> +/// // Toggling permissions with XOR (`^`).
>> +/// let toggled = read_only ^ Permissions::from(READ);
>> +/// assert!(!toggled.contains(READ));
>> +///
>> +/// // Inverting permissions with negation (`!`).
>> +/// let negated = !read_only;
>> +/// assert!(negated.contains(WRITE));
>> +/// ```
>> +#[macro_export]
>> +macro_rules! impl_flags {
>> +    (
>> +        $(#[$outer_flags:meta])* $vis_flags:vis $flags:ident,
>> +        $(#[$outer_flag:meta])* $vis_flag:vis $flag:ident,
> 
> So we might want to make sure we have one of the other rfl folks look at this
> first but: ideally I'd like to be able to the type for an individual bitflag
> like this:
> 
> /// An enumerator representing a single flag in [`PlaneCommitFlags`].
> ///
> /// This is a non-exhaustive list, as the C side could add more later.
> #[derive(Copy, Clone, PartialEq, Eq)]
> #[repr(u32)]
> #[non_exhaustive]
> pub enum PlaneCommitFlag {
>    /// Don't notify applications of plane updates for newly-disabled planes. Drivers are encouraged
>    /// to set this flag by default, as otherwise they need to ignore plane updates for disabled
>    /// planes by hand.
>    ActiveOnly = (1 << 0),
>    /// Tell the DRM core that the display hardware requires that a [`Crtc`]'s planes must be
>    /// disabled when the [`Crtc`] is disabled. When not specified,
>    /// [`AtomicCommitTail::commit_planes`] will skip the atomic disable callbacks for a plane if
>    /// the [`Crtc`] in the old [`PlaneState`] needs a modesetting operation. It is still up to the
>    /// driver to disable said planes in their [`DriverCrtc::atomic_disable`] callback.
>    NoDisableAfterModeset = (1 << 1),
> }
> 
> It seems like we can pass through docs just fine, but could we get something
> to handle specifying actual discriminant values for the flag enum as well?
> 

This should be possible, as the bitflags crate lets you do that in userspace. Their syntax is a bit different than what
we currently have in `impl_flags` though.

— Daniel

Powered by blists - more mailing lists