[PATCH] gpu: nova-core: bitfield: fix broken Default implementation

[PATCH] gpu: nova-core: bitfield: fix broken Default implementation

[Eliot Courtney]

The current implementation does not actually set the default values for
the fields in the bitfield. Set it and add a test.

Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
It doesn't actually set the default values for each field. I know that
bitfields are being moved soon, so maybe this fix is unnecessary but I
saw it and it's a simple fix.
---
 drivers/gpu/nova-core/bitfield.rs | 50 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
index 16e143658c51..22d68fc21a1f 100644
--- a/drivers/gpu/nova-core/bitfield.rs
+++ b/drivers/gpu/nova-core/bitfield.rs
@@ -4,6 +4,8 @@
 //!
 //! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro.
 
+use kernel::prelude::*;
+
 /// Defines a struct with accessors to access bits within an inner unsigned integer.
 ///
 /// # Syntax
@@ -314,12 +316,11 @@ fn fmt(&self, f: &mut ::kernel::fmt::Formatter<'_>) -> ::kernel::fmt::Result {
         /// Returns a value for the bitfield where all fields are set to their default value.
         impl ::core::default::Default for $name {
             fn default() -> Self {
-                #[allow(unused_mut)]
-                let mut value = Self(Default::default());
+                let value = Self(Default::default());
 
                 ::kernel::macros::paste!(
                 $(
-                value.[<set_ $field>](Default::default());
+                let value = value.[<set_ $field>](Default::default());
                 )*
                 );
 
@@ -328,3 +329,46 @@ fn default() -> Self {
         }
     };
 }
+
+#[kunit_tests(nova_core_bitfield)]
+mod tests {
+    use super::*;
+
+    #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)]
+    enum State {
+        Inactive = 0,
+        #[default]
+        Active = 1,
+    }
+
+    impl From<bool> for State {
+        fn from(value: bool) -> Self {
+            if value {
+                State::Active
+            } else {
+                State::Inactive
+            }
+        }
+    }
+
+    impl From<State> for bool {
+        fn from(state: State) -> bool {
+            match state {
+                State::Inactive => false,
+                State::Active => true,
+            }
+        }
+    }
+
+    bitfield! {
+        struct TestBitfield(u32) {
+            0:0 state as bool => State;
+        }
+    }
+
+    #[test]
+    fn default_impl() -> Result {
+        assert_eq!(TestBitfield::default().state(), State::Active);
+        Ok(())
+    }
+}

---
base-commit: 7c50d748b4a635bc39802ea3f6b120e66b1b9067
change-id: 20260331-fix-bitfield-a03f6d1b9e00

Best regards,
--  
Eliot Courtney <ecourtney@nvidia.com>

Re: [PATCH] gpu: nova-core: bitfield: fix broken Default implementation

[Gary Guo]

On Tue Mar 31, 2026 at 8:56 AM BST, Eliot Courtney wrote:
> The current implementation does not actually set the default values for
> the fields in the bitfield. Set it and add a test.
>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
> It doesn't actually set the default values for each field. I know that
> bitfields are being moved soon, so maybe this fix is unnecessary but I
> saw it and it's a simple fix.

Good catch! This is why I think it's important to use `with_` for this sort of
methods as they are confusingly non-mutating :)

Could you add a Fixes tag?

Best,
Gary

> ---
>  drivers/gpu/nova-core/bitfield.rs | 50 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 47 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
> index 16e143658c51..22d68fc21a1f 100644
> --- a/drivers/gpu/nova-core/bitfield.rs
> +++ b/drivers/gpu/nova-core/bitfield.rs
> @@ -4,6 +4,8 @@
>  //!
>  //! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro.
>  
> +use kernel::prelude::*;
> +
>  /// Defines a struct with accessors to access bits within an inner unsigned integer.
>  ///
>  /// # Syntax
> @@ -314,12 +316,11 @@ fn fmt(&self, f: &mut ::kernel::fmt::Formatter<'_>) -> ::kernel::fmt::Result {
>          /// Returns a value for the bitfield where all fields are set to their default value.
>          impl ::core::default::Default for $name {
>              fn default() -> Self {
> -                #[allow(unused_mut)]
> -                let mut value = Self(Default::default());
> +                let value = Self(Default::default());
>  
>                  ::kernel::macros::paste!(
>                  $(
> -                value.[<set_ $field>](Default::default());
> +                let value = value.[<set_ $field>](Default::default());
>                  )*
>                  );
>  
> @@ -328,3 +329,46 @@ fn default() -> Self {
>          }
>      };
>  }
> +
> +#[kunit_tests(nova_core_bitfield)]
> +mod tests {
> +    use super::*;
> +
> +    #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)]
> +    enum State {
> +        Inactive = 0,
> +        #[default]
> +        Active = 1,
> +    }
> +
> +    impl From<bool> for State {
> +        fn from(value: bool) -> Self {
> +            if value {
> +                State::Active
> +            } else {
> +                State::Inactive
> +            }
> +        }
> +    }
> +
> +    impl From<State> for bool {
> +        fn from(state: State) -> bool {
> +            match state {
> +                State::Inactive => false,
> +                State::Active => true,
> +            }
> +        }
> +    }
> +
> +    bitfield! {
> +        struct TestBitfield(u32) {
> +            0:0 state as bool => State;
> +        }
> +    }
> +
> +    #[test]
> +    fn default_impl() -> Result {
> +        assert_eq!(TestBitfield::default().state(), State::Active);
> +        Ok(())
> +    }
> +}
>
> ---
> base-commit: 7c50d748b4a635bc39802ea3f6b120e66b1b9067
> change-id: 20260331-fix-bitfield-a03f6d1b9e00
>
> Best regards,
> --  
> Eliot Courtney <ecourtney@nvidia.com>

Re: [PATCH] gpu: nova-core: bitfield: fix broken Default implementation

[Danilo Krummrich]

On 3/31/26 9:56 AM, Eliot Courtney wrote:
> The current implementation does not actually set the default values for
> the fields in the bitfield. Set it and add a test.
> 
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>

Thanks for the fix!

Please add a Fixes: tag and separate the fix from the newly introduced Kunit test.

Maybe it is not worth adding the Kunit test in here as generic bitfields are
being worked on, but I don't mind.

- Danilo

Re: [PATCH] gpu: nova-core: bitfield: fix broken Default implementation

[Joel Fernandes]

On 3/31/2026 3:56 AM, Eliot Courtney wrote:
> The current implementation does not actually set the default values for
> the fields in the bitfield. Set it and add a test.
> 
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

thanks,

--
Joel Fernandes


> ---
> It doesn't actually set the default values for each field. I know that
> bitfields are being moved soon, so maybe this fix is unnecessary but I
> saw it and it's a simple fix.
> ---
>  drivers/gpu/nova-core/bitfield.rs | 50 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/bitfield.rs b/drivers/gpu/nova-core/bitfield.rs
> index 16e143658c51..22d68fc21a1f 100644
> --- a/drivers/gpu/nova-core/bitfield.rs
> +++ b/drivers/gpu/nova-core/bitfield.rs
> @@ -4,6 +4,8 @@
>  //!
>  //! Support for defining bitfields in Rust structures. Also used by the [`register!`] macro.
>  
> +use kernel::prelude::*;
> +
>  /// Defines a struct with accessors to access bits within an inner unsigned integer.
>  ///
>  /// # Syntax
> @@ -314,12 +316,11 @@ fn fmt(&self, f: &mut ::kernel::fmt::Formatter<'_>) -> ::kernel::fmt::Result {
>          /// Returns a value for the bitfield where all fields are set to their default value.
>          impl ::core::default::Default for $name {
>              fn default() -> Self {
> -                #[allow(unused_mut)]
> -                let mut value = Self(Default::default());
> +                let value = Self(Default::default());
>  
>                  ::kernel::macros::paste!(
>                  $(
> -                value.[<set_ $field>](Default::default());
> +                let value = value.[<set_ $field>](Default::default());
>                  )*
>                  );
>  
> @@ -328,3 +329,46 @@ fn default() -> Self {
>          }
>      };
>  }
> +
> +#[kunit_tests(nova_core_bitfield)]
> +mod tests {
> +    use super::*;
> +
> +    #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)]
> +    enum State {
> +        Inactive = 0,
> +        #[default]
> +        Active = 1,
> +    }
> +
> +    impl From<bool> for State {
> +        fn from(value: bool) -> Self {
> +            if value {
> +                State::Active
> +            } else {
> +                State::Inactive
> +            }
> +        }
> +    }
> +
> +    impl From<State> for bool {
> +        fn from(state: State) -> bool {
> +            match state {
> +                State::Inactive => false,
> +                State::Active => true,
> +            }
> +        }
> +    }
> +
> +    bitfield! {
> +        struct TestBitfield(u32) {
> +            0:0 state as bool => State;
> +        }
> +    }
> +
> +    #[test]
> +    fn default_impl() -> Result {
> +        assert_eq!(TestBitfield::default().state(), State::Active);
> +        Ok(())
> +    }
> +}
> 
> ---
> base-commit: 7c50d748b4a635bc39802ea3f6b120e66b1b9067
> change-id: 20260331-fix-bitfield-a03f6d1b9e00
> 
> Best regards,
> --  
> Eliot Courtney <ecourtney@nvidia.com>
> 

Re: [PATCH] gpu: nova-core: bitfield: fix broken Default implementation

[John Hubbard]

On 3/31/26 6:36 AM, Danilo Krummrich wrote:
> On 3/31/26 9:56 AM, Eliot Courtney wrote:
>> The current implementation does not actually set the default values for
>> the fields in the bitfield. Set it and add a test.
>>
>> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> 
> Thanks for the fix!
> 
> Please add a Fixes: tag and separate the fix from the newly introduced Kunit test.
> 
> Maybe it is not worth adding the Kunit test in here as generic bitfields are
> being worked on, but I don't mind.
> 

If there is a chance that we won't need the Kunit test eventually,
then please either delete it here, or add a TODO to delete it
when generic bitfields cover this.

(Kunit, like any other test suite, is easy to add to, but also easy
to avoid pruning--and so over time it risks being full of tests
that are redundant or no longer needed.)

thanks,
-- 
John Hubbard

Claude review: gpu: nova-core: bitfield: fix broken Default implementation

[Claude Code Review Bot]

Overall Series Review

Subject: gpu: nova-core: bitfield: fix broken Default implementation
Author: Eliot Courtney <ecourtney@nvidia.com>
Patches: 5
Reviewed: 2026-04-01T07:59:01.227236

---

This is a single-patch bugfix for a real bug in the `bitfield!` macro's `Default` implementation in the nova-core driver. The bug is clear: the `set_*` methods use a builder pattern (taking `self` by value and returning `Self`), but the `Default` implementation discards the return values, so field defaults are never actually applied. The fix is correct and minimal, and the added kunit test properly validates the fix. **Recommended for merging.**

---
Generated by Claude Code Patch Reviewer

Claude review: gpu: nova-core: bitfield: fix broken Default implementation

[Claude Code Review Bot]

Patch Review

**Bug analysis:** The existing code at line 317-323 of `bitfield.rs` is:

```rust
#[allow(unused_mut)]
let mut value = Self(Default::default());

::kernel::macros::paste!(
$(
value.[<set_ $field>](Default::default());
)*
);
```

The `set_*` methods have the signature `fn [<set_ $field>](mut self, value: $to_type) -> Self` — they take `self` by value, mutate the copy, and return it. Since the struct derives `Copy`, calling `value.set_field(...)` copies `value` into the method, but the returned (modified) copy is discarded. The original `value` retains the zero-initialized storage, so field defaults are silently lost.

**Fix correctness:** The patch changes to:

```rust
let value = Self(Default::default());

::kernel::macros::paste!(
$(
let value = value.[<set_ $field>](Default::default());
)*
);
```

This correctly rebinds `value` to the returned modified copy on each iteration. The removal of `mut` and `#[allow(unused_mut)]` is appropriate since shadowing replaces mutation.

**Test quality:** The kunit test is well-designed — it uses a `State` enum where `#[default]` is `Active = 1` (not the zero value), which means the test would fail without the fix (the u32 storage defaults to 0, which maps to `Inactive`). This is a good discriminating test.

**Minor notes:**

- The `use kernel::prelude::*;` import is added for the `#[kunit_tests]` attribute macro and `Result` type in the test. This is fine.
- The `From<bool>` / `From<State>` trait impls in the test mirror the pattern used in the module's doc examples, which is consistent.

**Nit (non-blocking):** The test only covers the `=> $into_type` path (bool-to-enum conversion). It doesn't test the plain `as $type` or `?=> $try_into_type` paths, but that's acceptable for a bugfix — the core issue (discarded return value) affects all paths equally, and one test is sufficient to demonstrate the fix works.

No issues found. This is a clean, correct bugfix.

---
Generated by Claude Code Patch Reviewer