[PATCH v5 5/9] rust: bitfield: Add a new() constructor and raw() accessor

[Joel Fernandes <joelagnelf@nvidia.com>]

In order to prevent the user from directly accessing/wrapping the inner
value of the struct, provide a new storage type to wrap the inner value.
The bitfield framework can then control access better. For instance, we
can zero out undefined bits to prevent undefined behavior of bits that
are not defined.

Further, we can somewhat prevent the user manipulating the bitfield's
inner storage directly using .0. They can still do so by using the new
bitfield storage type this patch defines, however it would not be by
accident and would have to be deliberate.

Suggested-by: Yury Norov <yury.norov@gmail.com>
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
 rust/kernel/bitfield.rs    | 123 +++++++++++++++++++++++++++++--------
 rust/kernel/io/register.rs |  16 ++---
 2 files changed, 106 insertions(+), 33 deletions(-)

diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs
index 09cd5741598c..fed19918c3b9 100644
--- a/rust/kernel/bitfield.rs
+++ b/rust/kernel/bitfield.rs
@@ -4,7 +4,33 @@
 //!
 //! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro.
 
-/// Defines a struct with accessors to access bits within an inner unsigned integer.
+/// Storage wrapper for bitfield values that prevents direct construction.
+///
+/// This type wraps the underlying storage value and ensures that bitfield
+/// structs can only be constructed through their `new()` method, which
+/// properly masks undefined bits.
+#[repr(transparent)]
+#[derive(Clone, Copy)]
+pub struct BitfieldInternalStorage<T: Copy> {
+    value: T,
+}
+
+impl<T: Copy> BitfieldInternalStorage<T> {
+    /// Creates a new storage wrapper with the given value.
+    #[inline(always)]
+    pub const fn from_raw(value: T) -> Self {
+        Self { value }
+    }
+
+    /// Returns the underlying raw value.
+    #[inline(always)]
+    pub const fn into_raw(self) -> T {
+        self.value
+    }
+}
+
+/// Bitfield macro definition.
+/// Define a struct with accessors to access bits within an inner unsigned integer.
 ///
 /// # Syntax
 ///
@@ -62,9 +88,23 @@
 ///         7:7 state as bool => State;
 ///     }
 /// }
+///
+/// // Create a bitfield from a raw value - undefined bits are zeroed
+/// let reg = ControlReg::new(0xDEADBEEF);
+/// // Only bits 0-3 and 7 are preserved (as defined by the fields)
+///
+/// // Get the raw underlying value
+/// let raw_value = reg.raw();
+///
+/// // Use the builder pattern with field setters
+/// let reg2 = ControlReg::default()
+///     .set_mode(Mode::Auto)
+///     .set_state(State::Active);
 /// ```
 ///
 /// This generates a struct with:
+/// - Constructor: `new(value)` - creates a bitfield from a raw value, zeroing undefined bits
+/// - Raw accessor: `raw()` - returns the underlying raw value
 /// - Field accessors: `mode()`, `state()`, etc.
 /// - Field setters: `set_mode()`, `set_state()`, etc. (supports chaining with builder pattern).
 ///   Note that the compiler will error out if the size of the setter's arg exceeds the
@@ -72,7 +112,7 @@
 /// - Debug and Default implementations.
 ///
 /// Note: Field accessors and setters inherit the same visibility as the struct itself.
-/// In the example above, both `mode()` and `set_mode()` methods will be `pub`.
+/// In the example above, `new()`, `raw()`, `mode()` and `set_mode()` methods will be `pub`.
 ///
 /// Fields are defined as follows:
 ///
@@ -99,19 +139,19 @@ macro_rules! bitfield {
         )?
         #[repr(transparent)]
         #[derive(Clone, Copy)]
-        $vis struct $name($storage);
+        $vis struct $name(::kernel::bitfield::BitfieldInternalStorage<$storage>);
 
         impl ::core::ops::BitOr for $name {
             type Output = Self;
 
             fn bitor(self, rhs: Self) -> Self::Output {
-                Self(self.0 | rhs.0)
+                Self::new(self.raw() | rhs.raw())
             }
         }
 
         impl ::core::convert::From<$name> for $storage {
             fn from(val: $name) -> $storage {
-                val.0
+                val.raw()
             }
         }
 
@@ -161,6 +201,53 @@ fn from(val: $name) -> $storage {
 
         #[allow(dead_code)]
         impl $name {
+            // Generate field constants to be used later
+            $(
+            ::kernel::macros::paste!(
+                const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
+                const [<$field:upper _MASK>]: $storage = {
+                    // Generate mask for shifting
+                    match ::core::mem::size_of::<$storage>() {
+                        1 => ::kernel::bits::genmask_u8($lo..=$hi) as $storage,
+                        2 => ::kernel::bits::genmask_u16($lo..=$hi) as $storage,
+                        4 => ::kernel::bits::genmask_u32($lo..=$hi) as $storage,
+                        8 => ::kernel::bits::genmask_u64($lo..=$hi) as $storage,
+                        _ => ::kernel::build_error!("Unsupported storage type size")
+                    }
+                };
+                const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros();
+            );
+            )*
+
+            /// Creates a new bitfield instance from a raw value.
+            ///
+            /// This constructor zeros out all bits that are not defined by any field,
+            /// ensuring only valid field bits are preserved. This is to prevent UB
+            /// when raw() is used to retrieve undefined bits.
+            #[inline(always)]
+            $vis fn new(value: $storage) -> Self {
+                // Calculate mask for all defined fields
+                let mut mask: $storage = 0;
+                $(
+                    ::kernel::macros::paste!(
+                        mask |= Self::[<$field:upper _MASK>];
+                    );
+                )*
+                // Zero out undefined bits and wrap in BitfieldInternalStorage
+                Self(::kernel::bitfield::BitfieldInternalStorage::from_raw(value & mask))
+            }
+
+            /// Returns the raw underlying value of the bitfield.
+            ///
+            /// This provides direct access to the storage value, useful for
+            /// debugging or when you need to work with the raw value.
+            /// Bits not defined are masked at construction time.
+            #[inline(always)]
+            $vis fn raw(&self) -> $storage {
+                self.0.into_raw()
+            }
+
+            // Generate field accessors
             $(
             ::kernel::bitfield!(@field_accessor $vis $name $storage, $hi:$lo $field as $type
                 $(?=> $try_into_type)?
@@ -249,21 +336,6 @@ impl $name {
         @leaf_accessor $vis:vis $name:ident $storage:ty, $hi:tt:$lo:tt $field:ident
             { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?;
     ) => {
-        ::kernel::macros::paste!(
-        const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
-        const [<$field:upper _MASK>]: $storage = {
-            // Generate mask for shifting
-            match ::core::mem::size_of::<$storage>() {
-                1 => ::kernel::bits::genmask_u8($lo..=$hi) as $storage,
-                2 => ::kernel::bits::genmask_u16($lo..=$hi) as $storage,
-                4 => ::kernel::bits::genmask_u32($lo..=$hi) as $storage,
-                8 => ::kernel::bits::genmask_u64($lo..=$hi) as $storage,
-                _ => ::kernel::build_error!("Unsupported storage type size")
-            }
-        };
-        const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros();
-        );
-
         $(
         #[doc="Returns the value of this field:"]
         #[doc=$comment]
@@ -274,7 +346,7 @@ impl $name {
             const MASK: $storage = $name::[<$field:upper _MASK>];
             const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
             );
-            let field = ((self.0 & MASK) >> SHIFT);
+            let field = ((self.raw() & MASK) >> SHIFT);
 
             $process(field)
         }
@@ -288,8 +360,9 @@ impl $name {
         $vis fn [<set_ $field>](mut self, value: $to_type) -> Self {
             const MASK: $storage = $name::[<$field:upper _MASK>];
             const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
-            let value = (<$storage>::from(value) << SHIFT) & MASK;
-            self.0 = (self.0 & !MASK) | value;
+            let val = (<$storage>::from(value) << SHIFT) & MASK;
+            let new_val = (self.raw() & !MASK) | val;
+            self.0 = ::kernel::bitfield::BitfieldInternalStorage::from_raw(new_val);
 
             self
         }
@@ -301,7 +374,7 @@ impl $name {
         impl ::core::fmt::Debug for $name {
             fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result {
                 f.debug_struct(stringify!($name))
-                    .field("<raw>", &format_args!("{:#x}", &self.0))
+                    .field("<raw>", &format_args!("{:#x}", &self.raw()))
                 $(
                     .field(stringify!($field), &self.$field())
                 )*
@@ -316,7 +389,7 @@ fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result {
         impl ::core::default::Default for $name {
             fn default() -> Self {
                 #[allow(unused_mut)]
-                let mut value = Self(Default::default());
+                let mut value = Self::new(Default::default());
 
                 ::kernel::macros::paste!(
                 $(
diff --git a/rust/kernel/io/register.rs b/rust/kernel/io/register.rs
index c24d956f122f..45c6ad1bfb9e 100644
--- a/rust/kernel/io/register.rs
+++ b/rust/kernel/io/register.rs
@@ -374,7 +374,7 @@ impl $name {
             pub(crate) fn read<const SIZE: usize, T>(io: &T) -> Self where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
             {
-                Self(io.read32($offset))
+                Self::new(io.read32($offset))
             }
 
             /// Write the value contained in `self` to the register address in `io`.
@@ -382,7 +382,7 @@ pub(crate) fn read<const SIZE: usize, T>(io: &T) -> Self where
             pub(crate) fn write<const SIZE: usize, T>(self, io: &T) where
                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
             {
-                io.write32(self.0, $offset)
+                io.write32(self.raw(), $offset)
             }
 
             /// Read the register from its address in `io` and run `f` on its value to obtain a new
@@ -424,7 +424,7 @@ pub(crate) fn read<const SIZE: usize, T, B>(
                     <B as ::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
                 );
 
-                Self(value)
+                Self::new(value)
             }
 
             /// Write the value contained in `self` to `io`, using the base address provided by
@@ -442,7 +442,7 @@ pub(crate) fn write<const SIZE: usize, T, B>(
                 const OFFSET: usize = $name::OFFSET;
 
                 io.write32(
-                    self.0,
+                    self.raw(),
                     <B as ::kernel::io::register::RegisterBase<$base>>::BASE + OFFSET
                 );
             }
@@ -487,7 +487,7 @@ pub(crate) fn read<const SIZE: usize, T>(
                 let offset = Self::OFFSET + (idx * Self::STRIDE);
                 let value = io.read32(offset);
 
-                Self(value)
+                Self::new(value)
             }
 
             /// Write the value contained in `self` to the array register with index `idx` in `io`.
@@ -503,7 +503,7 @@ pub(crate) fn write<const SIZE: usize, T>(
 
                 let offset = Self::OFFSET + (idx * Self::STRIDE);
 
-                io.write32(self.0, offset);
+                io.write32(self.raw(), offset);
             }
 
             /// Read the array register at index `idx` in `io` and run `f` on its value to obtain a
@@ -610,7 +610,7 @@ pub(crate) fn read<const SIZE: usize, T, B>(
                     Self::OFFSET + (idx * Self::STRIDE);
                 let value = io.read32(offset);
 
-                Self(value)
+                Self::new(value)
             }
 
             /// Write the value contained in `self` to `io`, using the base address provided by
@@ -631,7 +631,7 @@ pub(crate) fn write<const SIZE: usize, T, B>(
                 let offset = <B as ::kernel::io::register::RegisterBase<$base>>::BASE +
                     Self::OFFSET + (idx * Self::STRIDE);
 
-                io.write32(self.0, offset);
+                io.write32(self.raw(), offset);
             }
 
             /// Read the array register at index `idx` from `io`, using the base address provided
-- 
2.34.1