Nouveau - Sep 2025 - [PATCH v4 6/6] rust: bitfield: Use 'as' operator for setter type conversion

The bitfield macro's setter currently uses the From trait for type
conversion, which is overly restrictive and prevents use cases such as
narrowing conversions (e.g., u32 storage size to u8 field size) which
aren't supported by From.

Replace 'from' with 'as' in the setter implementation to support
this.

Suggested-by: Yury Norov <yury.norov at gmail.com>
Signed-off-by: Joel Fernandes <joel at joelfernandes.org>
---
 rust/kernel/bits/bitfield.rs | 46 +++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/bits/bitfield.rs b/rust/kernel/bits/bitfield.rs
index 99e443580b36..6342352891fa 100644
--- a/rust/kernel/bits/bitfield.rs
+++ b/rust/kernel/bits/bitfield.rs
@@ -300,7 +300,7 @@ 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;
+            let value = ((value as $storage) << SHIFT) & MASK;
             self.0 = (self.0 & !MASK) | value;
 
             self
@@ -452,6 +452,15 @@ struct TestPartialBits: u8 {
         }
     }
 
+    // For testing wide field types on narrow storage
+    bitfield! {
+        struct TestWideFields: u8 {
+            3:0       nibble      as u32;
+            7:4       high_nibble as u32;
+            7:0       full        as u64;
+        }
+    }
+
     #[test]
     fn test_single_bits() {
         let mut pte = TestPageTableEntry::default();
@@ -689,4 +698,39 @@ fn test_partial_bitfield() {
         bf2 = bf2.set_state(0x55);
         assert_eq!(bf2.state(), 0x1);
     }
+
+    #[test]
+    fn test_wide_field_types() {
+        let mut wf = TestWideFields::default();
+
+        wf = wf.set_nibble(0x0000000F_u32);
+        assert_eq!(wf.nibble(), 0x0000000F_u32);
+
+        wf = wf.set_high_nibble(0x00000007_u32);
+        assert_eq!(wf.high_nibble(), 0x00000007_u32);
+
+        wf = wf.set_nibble(0xDEADBEEF_u32);
+        assert_eq!(wf.nibble(), 0x0000000F_u32);
+
+        wf = wf.set_high_nibble(0xCAFEBABE_u32);
+        assert_eq!(wf.high_nibble(), 0x0000000E_u32);
+
+        wf = wf.set_full(0xDEADBEEFCAFEBABE_u64);
+        assert_eq!(wf.full(), 0xBE_u64);
+
+        assert_eq!(wf.raw(), 0xBE_u8);
+
+        wf = TestWideFields::default()
+            .set_nibble(0x5_u32)
+            .set_high_nibble(0xA_u32);
+        assert_eq!(wf.raw(), 0xA5_u8);
+        assert_eq!(wf.nibble(), 0x5_u32);
+        assert_eq!(wf.high_nibble(), 0xA_u32);
+
+        // Test builder pattern
+        let wf2 = TestWideFields::default()
+            .set_nibble(0x12345678_u32)    // truncated to 0x8
+            .set_high_nibble(0x9ABCDEF0_u32); // truncated to 0x0
+        assert_eq!(wf2.raw(), 0x08_u8);
+    }
 }
-- 
2.34.1

On Sat, Sep 20, 2025 at 8:23?PM Joel Fernandes <joelagnelf at nvidia.com>
wrote:
>
> The bitfield macro's setter currently uses the From trait for type
> conversion, which is overly restrictive and prevents use cases such as
> narrowing conversions (e.g., u32 storage size to u8 field size) which
> aren't supported by From.
Being restrictive is a good thing -- it would be nice to know more
context about this change, like Alexandre points out.

In particular, the line:

    .set_nibble(0x12345678_u32)    // truncated to 0x8

sounds fairly alarming, and not what we usually want. Why cannot the
caller cast on their side, if they really want that?

We avoid `as` for similar reasons and nowadays enable some Clippy
warnings to prevent its use where not needed.

(By the way, please follow our usual coding conventions for comments.)

Thanks!

Cheers,
Miguel

On Mon Sep 29, 2025 at 10:59 PM JST, Miguel Ojeda wrote:
> On Sat, Sep 20, 2025 at 8:23?PM Joel Fernandes <joelagnelf at
nvidia.com> wrote:
>>
>> The bitfield macro's setter currently uses the From trait for type
>> conversion, which is overly restrictive and prevents use cases such as
>> narrowing conversions (e.g., u32 storage size to u8 field size) which
>> aren't supported by From.
>
> Being restrictive is a good thing
On that note, I have been wondering whether we should not push the
restriction up to having bounded primitive types with only a set number
of bits valid, e.g. `bound_u8::<2>` is guaranteed to only contain values
in the range `0..=3`.

Getters and setters would use these types depending on the number of
bits of the field, meaning that a caller would have to validate the
value they want to write if it does not implement e.g.
`Into<bound_u8<2>>`.

A bit radical maybe, but correcness ensues. :)

On Mon, Sep 29, 2025 at 03:59:32PM +0200, Miguel Ojeda
wrote:
> On Sat, Sep 20, 2025 at 8:23?PM Joel Fernandes <joelagnelf at
nvidia.com> wrote:
> >
> > The bitfield macro's setter currently uses the From trait for type
> > conversion, which is overly restrictive and prevents use cases such as
> > narrowing conversions (e.g., u32 storage size to u8 field size) which
> > aren't supported by From.
> 
> Being restrictive is a good thing -- it would be nice to know more
> context about this change, like Alexandre points out.
Sure, I replied to that thread. Lets discuss there as well about the usecase.
> In particular, the line:
> 
>     .set_nibble(0x12345678_u32)    // truncated to 0x8
> 
> sounds fairly alarming, and not what we usually want. Why cannot the
> caller cast on their side, if they really want that?
The setter function generated in this example accepts a u32.

Actually my test case here is not good, I will fix it. In the new v5 series I
am going to post, set_nibble(0x12345678) will actually fail because the value
passed exceeds 4 bit range.

So in reality, there will be no truncation at all, this is just a bad 
test case (I developed the failure mode for when the value passed exceeds
the bit range, only by v5).
> We avoid `as` for similar reasons and nowadays enable some Clippy
> warnings to prevent its use where not needed.
Understood, I can add a comment here to explain why we do it. It is just to
make the code compile, in reality we're not really truncating anything. The
issue solved in this patch is the following line wont compile when narrowing
from 32 bit to 8 bit (even if the value passed does not exceed 8 bits):
  let val = (<$storage>::from(value) << SHIFT) & MASK;
> (By the way, please follow our usual coding conventions for comments.)
Sorry about that, will do.

Thanks!

 - Joel