[PATCH v2 0/2] rust: impl_flags: add convenience functions

[PATCH v2 0/2] rust: impl_flags: add convenience functions

[Andreas Hindborg]

Add a few convenience functions that makes it easier to work with the
`impl_flags` module and C APIs.

These conversion functions are useful when assigning flags to C structs in
Rust abstractions:

  let mut lim = bindings::queue_limits::zeroed();

  if self.rotational {
      lim.features = Feature::Rotational.into();
  }

  if self.write_cache {
      lim.features |= Feature::WriteCache;
  }

Or when providing flags from C to Rust:

  impl<T: Operations> RequestInner<T> {
      pub fn flags(&self) -> Flags {
          // SAFETY: By C API contract and type invariant, `cmd_flags` is valid for read
          let flags = unsafe { (*self.0.get()).cmd_flags & !((1 << bindings::REQ_OP_BITS) - 1) };
          Flags::try_from(flags).expect("Request should have valid falgs")
      }
  }

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
Changes in v2:
- Use an `as` cast instead of `core::mem::transmute` in
  `From<$flag> for $ty` (Miguel).
- Replace the `match` in `TryFrom::try_from` with an early-return
  `if` (Miguel)
- Construct the result directly instead of via `transmute` in `try_from`
  (Miguel).
- Use `Self` in the new conversion impls (Miguel).
- Mark `TryFrom::try_from` `#[inline]`.
- Extend the rustdoc example to demonstrate both new conversion
  impls (Miguel).
- Link to v1: https://msgid.link/20260215-impl-flags-additions-v1-0-6538c8fb841c@kernel.org

To: Miguel Ojeda <ojeda@kernel.org>
To: Boqun Feng <boqun@kernel.org>
To: Gary Guo <gary@garyguo.net>
To: Björn Roy Baron <bjorn3_gh@protonmail.com>
To: Benno Lossin <lossin@kernel.org>
To: Andreas Hindborg <a.hindborg@kernel.org>
To: Alice Ryhl <aliceryhl@google.com>
To: Trevor Gross <tmgross@umich.edu>
To: Danilo Krummrich <dakr@kernel.org>
Cc: rust-for-linux@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Andreas Hindborg (2):
      rust: impl_flags: add conversion functions
      rust: impl_flags: add bitwise operations with the underlying type

 rust/kernel/impl_flags.rs | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)
---
base-commit: 9e0898f1c0f134c6bad146ca8578f73c3e40ac0a
change-id: 20260215-impl-flags-additions-0340ffcba5b9
prerequisite-change-id: 20260212-impl-flags-inner-c61974b27b18:v2
prerequisite-patch-id: 379fb78c07b554278fae3c42d84d62bcfcfa0d45

Best regards,
--  
Andreas Hindborg <a.hindborg@kernel.org>

[PATCH v2 1/2] rust: impl_flags: add conversion functions

[Andreas Hindborg]

Add two conversion functions to the `impl_flags!` macro:

 - A `From<_>` implementation to convert from the flag value enum to
   underlying type.

 - A `TryFrom<_> implementation to convert from the underlying
   representation to flag container type.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/impl_flags.rs | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/rust/kernel/impl_flags.rs b/rust/kernel/impl_flags.rs
index 1d39d54540dc..04a4b0a356df 100644
--- a/rust/kernel/impl_flags.rs
+++ b/rust/kernel/impl_flags.rs
@@ -77,6 +77,17 @@
 /// // representation — useful for passing the value to a C API.
 /// let raw: u32 = read_only.bits();
 /// assert_eq!(raw, Permission::Read as u32);
+///
+/// // Convert a single flag value to its underlying integer representation.
+/// let raw_read: u32 = Permission::Read.into();
+/// assert_eq!(raw_read, Permission::Read as u32);
+///
+/// // Construct a `Permissions` value from a raw integer. `TryFrom` rejects
+/// // integers whose set bits do not all correspond to declared flags.
+/// let parsed = Permissions::try_from(raw_read | Permission::Write as u32).unwrap();
+/// assert!(parsed.contains(Permission::Read));
+/// assert!(parsed.contains(Permission::Write));
+/// assert!(Permissions::try_from(1 << 7).is_err());
 /// ```
 #[macro_export]
 macro_rules! impl_flags {
@@ -119,6 +130,26 @@ fn from(value: $flags) -> Self {
             }
         }
 
+        impl ::core::convert::From<$flag> for $ty {
+            #[inline]
+            fn from(value: $flag) -> Self {
+                value as Self
+            }
+        }
+
+        impl ::core::convert::TryFrom<$ty> for $flags {
+            type Error = ::kernel::error::Error;
+
+            #[inline]
+            fn try_from(value: $ty) -> Result<Self, Self::Error> {
+                if value & !$flags::all_bits() != 0 {
+                    return Err(::kernel::error::code::EINVAL);
+                }
+
+                Ok(Self(value))
+            }
+        }
+
         impl ::core::ops::BitOr for $flags {
             type Output = Self;
             #[inline]

-- 
2.51.2

Re: [PATCH v2 1/2] rust: impl_flags: add conversion functions

[Miguel Ojeda]

On Fri, Jun 5, 2026 at 3:05 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> +                if value & !$flags::all_bits() != 0 {

Can't this also be `Self`?

Cheers,
Miguel

[PATCH v2 2/2] rust: impl_flags: add bitwise operations with the underlying type

[Andreas Hindborg]

Add bitwise or operations between the flag value enum and the underlying
type. This is useful when manipulating flags from C API without round
tripping into the Rust flag container type:

  let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };
  ...
  if self.write_cache {
      lim.features |= request::Feature::WriteCache;
  }

The above code would be needlessly verbose without this direct assignment
option.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/impl_flags.rs | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/rust/kernel/impl_flags.rs b/rust/kernel/impl_flags.rs
index 04a4b0a356df..8340740b2efb 100644
--- a/rust/kernel/impl_flags.rs
+++ b/rust/kernel/impl_flags.rs
@@ -280,6 +280,22 @@ fn not(self) -> Self::Output {
             }
         }
 
+        impl ::core::ops::BitOr<$flag> for $ty {
+            type Output = Self;
+
+            #[inline]
+            fn bitor(self, rhs: $flag) -> Self::Output {
+                self | <$flag as Into<Self>>::into(rhs)
+            }
+        }
+
+        impl ::core::ops::BitOrAssign<$flag> for $ty {
+            #[inline]
+            fn bitor_assign(&mut self, rhs: $flag) {
+                *self = *self | <$flag as Into<Self>>::into(rhs)
+            }
+        }
+
         impl $flags {
             /// Returns an empty instance where no flags are set.
             #[inline]

-- 
2.51.2

Re: [PATCH v2 2/2] rust: impl_flags: add bitwise operations with the underlying type

[Miguel Ojeda]

On Fri, Jun 5, 2026 at 3:04 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Add bitwise or operations between the flag value enum and the underlying
> type. This is useful when manipulating flags from C API without round
> tripping into the Rust flag container type:
>
>   let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };

Can this use `zeroable` nowadays?

>   if self.write_cache {
>       lim.features |= request::Feature::WriteCache;
>   }
>
> The above code would be needlessly verbose without this direct assignment
> option.

v2 looks better overall, but I am wondering about Gary's question in
v1 -- not sure if you saw it:

  https://lore.kernel.org/rust-for-linux/DGPT710WN25X.1B9P21BE6X8P4@garyguo.net/

The commit message shows a converted case, but is it that much
verbose? Perhaps you can point to the use case / code that made you
think about adding these `impl`s?

Thanks!

Cheers,
Miguel

Re: [PATCH v2 2/2] rust: impl_flags: add bitwise operations with the underlying type

[Andreas Hindborg]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes:

> On Fri, Jun 5, 2026 at 3:04 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Add bitwise or operations between the flag value enum and the underlying
>> type. This is useful when manipulating flags from C API without round
>> tripping into the Rust flag container type:
>>
>>   let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };
>
> Can this use `zeroable` nowadays?

Yes, probably.

>
>>   if self.write_cache {
>>       lim.features |= request::Feature::WriteCache;
>>   }
>>
>> The above code would be needlessly verbose without this direct assignment
>> option.
>
> v2 looks better overall, but I am wondering about Gary's question in
> v1 -- not sure if you saw it:
>
>   https://lore.kernel.org/rust-for-linux/DGPT710WN25X.1B9P21BE6X8P4@garyguo.net/
>
> The commit message shows a converted case, but is it that much
> verbose? Perhaps you can point to the use case / code that made you
> think about adding these `impl`s?

I feel like we already discussed this. I don't understand the objection.
I have a C type integer that is a set of flags. In bindings, I
manipulate the flags (I will send this code within the next few days):

    /// Build a new `GenDisk` and add it to the VFS.
    pub fn build(
        self,
        name: fmt::Arguments<'_>,
        tagset: Arc<TagSet<T>>,
        queue_data: T::QueueData,
    ) -> Result<Arc<GenDisk<T>>> {
        let data = queue_data.into_foreign();
        let recover_data = ScopeGuard::new(|| {
            // SAFETY: T::QueueData was created by the call to `into_foreign()` above
            drop(unsafe { T::QueueData::from_foreign(data) });
        });

        let mut lim: bindings::queue_limits = pin_init::zeroed();

        lim.logical_block_size = self.logical_block_size;
        lim.physical_block_size = self.physical_block_size;
        lim.max_hw_discard_sectors = self.max_hw_discard_sectors;
        lim.max_sectors = self.max_sectors;
        lim.virt_boundary_mask = self.virt_boundary_mask;
        if self.rotational {
            lim.features = Feature::Rotational.into();
        }

        #[cfg(CONFIG_BLK_DEV_ZONED)]
        if self.zoned {
            if !T::HAS_REPORT_ZONES {
                return Err(error::code::EINVAL);
            }

            lim.features |= Feature::Zoned;
            lim.chunk_sectors = self.zone_size_sectors;
            lim.max_hw_zone_append_sectors = self.zone_append_max_sectors;
        }

        if self.write_cache {
            lim.features |= Feature::WriteCache;
        }

        if self.forced_unit_access {
            lim.features |= Feature::ForcedUnitAccess;
        }

        // SAFETY: `tagset.raw_tag_set()` points to a valid and initialized tag set
        let gendisk = from_err_ptr(unsafe {
            bindings::__blk_mq_alloc_disk(
                tagset.raw_tag_set(),
                &mut lim,
                data,
                static_lock_class!().as_ptr(),
            )
        })?;

        ...

I don't understand the rationale for writing these flags into a
temporary to then assigning later.


Best regards,
Andreas Hindborg

Re: [PATCH v2 2/2] rust: impl_flags: add bitwise operations with the underlying type

[Miguel Ojeda]

On Sat, Jun 6, 2026 at 10:09 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> I feel like we already discussed this. I don't understand the objection.

Now that you mention it, yeah. Hmm... it may be have been a call.

> I have a C type integer that is a set of flags. In bindings, I
> manipulate the flags (I will send this code within the next few days):

Yeah, so what I suggested is that providing the before-and-after
(rather than "after" only below as you do here and in the commit
message) can help to justify it.

That is, what exactly would you need to do before this patch? i.e. do
you really need a temporary? e.g. could you have written `as ...`
(putting aside that we try to avoid casts) or are we missing something
else from your use case? Would a `From` instead be good enough
instead?

My reading of Gary's comment is that, by providing too convenient
operators on the raw type, people may avoid actually using the newtype
when it makes sense to do so (which is a general principle that I
agree with).

In your case, I can see it is reasonable to have the operator there
(to avoid either casts or , and I think we are still in a better spot
than C here (since we only implement it for a particular primitive
type etc.), but yeah, it should have a better justification -- it is a
tradeoff to introduce more convenience, and the patch doesn't really
discuss it or its alternatives.

I hope this clarifies.

Cheers,
Miguel