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

[Andreas Hindborg <a.hindborg@kernel.org>]

"Gary Guo" <gary@garyguo.net> writes:

> On Thu Nov 5, 2026 at 3:42 PM GMT, Andreas Hindborg wrote:
>> 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.
>>
>> Drop the hand-written `TryFrom<u32> for VmMapFlags` in tyr, which now
>> conflicts with the implementation generated by `impl_flags!`. The two are
>> equivalent.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  drivers/gpu/drm/tyr/vm.rs | 13 -------------
>>  rust/kernel/impl_flags.rs | 31 +++++++++++++++++++++++++++++++
>>  2 files changed, 31 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tyr/vm.rs b/drivers/gpu/drm/tyr/vm.rs
>> index 1dd11b2b1253..0d6e841a31ed 100644
>> --- a/drivers/gpu/drm/tyr/vm.rs
>> +++ b/drivers/gpu/drm/tyr/vm.rs
>> @@ -141,19 +141,6 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>>      }
>>  }
>>  
>> -impl TryFrom<u32> for VmMapFlags {
>> -    type Error = Error;
>> -
>> -    fn try_from(value: u32) -> Result<Self, Self::Error> {
>> -        let valid = VmFlag::Readonly as u32 | VmFlag::Noexec as u32 | VmFlag::Uncached as u32;
>> -
>> -        if value & !valid != 0 {
>> -            return Err(EINVAL);
>> -        }
>> -        Ok(Self(value))
>> -    }
>> -}
>> -
>>  /// Arguments for a virtual memory map operation.
>>  struct VmMapArgs<'drm> {
>>      /// Access permissions and caching behavior for the mapping.
>> diff --git a/rust/kernel/impl_flags.rs b/rust/kernel/impl_flags.rs
>> index 6051487f6e28..052759b8038d 100644
>> --- a/rust/kernel/impl_flags.rs
>> +++ b/rust/kernel/impl_flags.rs
>> @@ -73,6 +73,17 @@
>>  /// // 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 {
>> @@ -115,6 +126,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 & !Self::all_bits() != 0 {
>> +                    return Err(::kernel::error::code::EINVAL);
>
> IMO this should be a new type, perhaps something like
> `kernel::bitflags::UnknownFlags`.

Is really useful? If this happens, you probably cannot recover anyway.

Best regards,
Andreas Hindborg