[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

[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

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

[Eliot Courtney]

On Wed Apr 1, 2026 at 5:53 AM JST, John Hubbard wrote:
> 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,

I think this entire file will be deleted so it'll be gone regardless.
I think we can skip it, just wrote it to test my change locally anyway.

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>
>