[PATCH] rust: io: use const generics for read/write offsets

Alice Ryhl posted 1 patch 1 week, 6 days ago
drivers/gpu/drm/tyr/regs.rs     |  4 ++--
rust/kernel/devres.rs           |  4 ++--
rust/kernel/io.rs               | 18 ++++++++++--------
rust/kernel/io/mem.rs           |  6 +++---
samples/rust/rust_driver_pci.rs | 10 +++++-----
5 files changed, 22 insertions(+), 20 deletions(-)
[PATCH] rust: io: use const generics for read/write offsets
Posted by Alice Ryhl 1 week, 6 days ago
Using build_assert! to assert that offsets are in bounds is really
fragile and likely to result in spurious and hard-to-debug build
failures. Therefore, build_assert! should be avoided for this case.
Thus, update the code to perform the check in const evaluation instead.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 drivers/gpu/drm/tyr/regs.rs     |  4 ++--
 rust/kernel/devres.rs           |  4 ++--
 rust/kernel/io.rs               | 18 ++++++++++--------
 rust/kernel/io/mem.rs           |  6 +++---
 samples/rust/rust_driver_pci.rs | 10 +++++-----
 5 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
index f46933aaa2214ee0ac58b1ea2a6aa99506a35b70..e3c306e48e86d1d6047cab7944e0fe000901d48b 100644
--- a/drivers/gpu/drm/tyr/regs.rs
+++ b/drivers/gpu/drm/tyr/regs.rs
@@ -25,13 +25,13 @@
 impl<const OFFSET: usize> Register<OFFSET> {
     #[inline]
     pub(crate) fn read(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<u32> {
-        let value = (*iomem).access(dev)?.read32(OFFSET);
+        let value = (*iomem).access(dev)?.read32::<OFFSET>();
         Ok(value)
     }
 
     #[inline]
     pub(crate) fn write(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>, value: u32) -> Result {
-        (*iomem).access(dev)?.write32(value, OFFSET);
+        (*iomem).access(dev)?.write32::<OFFSET>(value);
         Ok(())
     }
 }
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index da18091143a67fcfbb247e7cb4f59f5a4932cac5..3e66e10c05fa078e42162c7a367161fbf735a07f 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -96,7 +96,7 @@ struct Inner<T: Send> {
 /// let devres = KBox::pin_init(Devres::new(dev, iomem), GFP_KERNEL)?;
 ///
 /// let res = devres.try_access().ok_or(ENXIO)?;
-/// res.write8(0x42, 0x0);
+/// res.write8::<0x0>(0x42);
 /// # Ok(())
 /// # }
 /// ```
@@ -232,7 +232,7 @@ pub fn device(&self) -> &Device {
     ///
     ///     // might_sleep()
     ///
-    ///     bar.write32(0x42, 0x0);
+    ///     bar.write32::<0x0>(0x42);
     ///
     ///     Ok(())
     /// }
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index 03b467722b8651ebecd660ac0e2d849cf88dc915..563ff8488100d9e07a7f4bffeb085db7bd7e9d6a 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -103,7 +103,7 @@ pub fn maxsize(&self) -> usize {
 ///# fn no_run() -> Result<(), Error> {
 /// // SAFETY: Invalid usage for example purposes.
 /// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
-/// iomem.write32(0x42, 0x0);
+/// iomem.write32::<0x0>(0x42);
 /// assert!(iomem.try_write32(0x42, 0x0).is_ok());
 /// assert!(iomem.try_write32(0x42, 0x4).is_err());
 /// # Ok(())
@@ -120,8 +120,8 @@ macro_rules! define_read {
         /// time, the build will fail.
         $(#[$attr])*
         #[inline]
-        pub fn $name(&self, offset: usize) -> $type_name {
-            let addr = self.io_addr_assert::<$type_name>(offset);
+        pub fn $name<const OFF: usize>(&self) -> $type_name {
+            let addr = self.io_addr_assert::<$type_name, OFF>();
 
             // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
             unsafe { bindings::$c_fn(addr as *const c_void) }
@@ -149,8 +149,8 @@ macro_rules! define_write {
         /// time, the build will fail.
         $(#[$attr])*
         #[inline]
-        pub fn $name(&self, value: $type_name, offset: usize) {
-            let addr = self.io_addr_assert::<$type_name>(offset);
+        pub fn $name<const OFF: usize>(&self, value: $type_name) {
+            let addr = self.io_addr_assert::<$type_name, OFF>();
 
             // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
             unsafe { bindings::$c_fn(value, addr as *mut c_void) }
@@ -217,10 +217,12 @@ fn io_addr<U>(&self, offset: usize) -> Result<usize> {
     }
 
     #[inline]
-    fn io_addr_assert<U>(&self, offset: usize) -> usize {
-        build_assert!(Self::offset_valid::<U>(offset, SIZE));
+    fn io_addr_assert<U, const OFF: usize>(&self) -> usize {
+        const {
+            build_assert!(Self::offset_valid::<U>(OFF, SIZE));
+        }
 
-        self.addr() + offset
+        self.addr() + OFF
     }
 
     define_read!(read8, try_read8, readb -> u8);
diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
index 6f99510bfc3a63dd72c1d47dc661dcd48fa7f54e..b73557f5f57c955ac251a46c9bdd6df0687411e2 100644
--- a/rust/kernel/io/mem.rs
+++ b/rust/kernel/io/mem.rs
@@ -54,7 +54,7 @@ pub(crate) unsafe fn new(device: &'a Device<Bound>, resource: &'a Resource) -> S
     ///       pdev: &platform::Device<Core>,
     ///       info: Option<&Self::IdInfo>,
     ///    ) -> Result<Pin<KBox<Self>>> {
-    ///       let offset = 0; // Some offset.
+    ///       const OFFSET: usize = 0; // Some offset.
     ///
     ///       // If the size is known at compile time, use [`Self::iomap_sized`].
     ///       //
@@ -66,9 +66,9 @@ pub(crate) unsafe fn new(device: &'a Device<Bound>, resource: &'a Resource) -> S
     ///       let io = iomem.access(pdev.as_ref())?;
     ///
     ///       // Read and write a 32-bit value at `offset`.
-    ///       let data = io.read32_relaxed(offset);
+    ///       let data = io.read32_relaxed::<OFFSET>();
     ///
-    ///       io.write32_relaxed(data, offset);
+    ///       io.write32_relaxed::<OFFSET>(data);
     ///
     ///       # Ok(KBox::new(SampleDriver, GFP_KERNEL)?.into())
     ///     }
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 606946ff4d7fd98e206ee6420a620d1c44eb0377..6f0388853e2b36e0800df5125a5dd8b20a6d5912 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -46,17 +46,17 @@ struct SampleDriver {
 impl SampleDriver {
     fn testdev(index: &TestIndex, bar: &Bar0) -> Result<u32> {
         // Select the test.
-        bar.write8(index.0, Regs::TEST);
+        bar.write8::<{ Regs::TEST }>(index.0);
 
-        let offset = u32::from_le(bar.read32(Regs::OFFSET)) as usize;
-        let data = bar.read8(Regs::DATA);
+        let offset = u32::from_le(bar.read32::<{ Regs::OFFSET }>()) as usize;
+        let data = bar.read8::<{ Regs::DATA }>();
 
         // Write `data` to `offset` to increase `count` by one.
         //
         // Note that we need `try_write8`, since `offset` can't be checked at compile-time.
         bar.try_write8(data, offset)?;
 
-        Ok(bar.read32(Regs::COUNT))
+        Ok(bar.read32::<{ Regs::COUNT }>())
     }
 }
 
@@ -98,7 +98,7 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
     fn unbind(pdev: &pci::Device<Core>, this: Pin<&Self>) {
         if let Ok(bar) = this.bar.access(pdev.as_ref()) {
             // Reset pci-testdev by writing a new test index.
-            bar.write8(this.index.0, Regs::TEST);
+            bar.write8::<{ Regs::TEST }>(this.index.0);
         }
     }
 }

---
base-commit: cf4fd52e323604ccfa8390917593e1fb965653ee
change-id: 20250918-write-offset-const-0b231c4282ea

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>
Re: [PATCH] rust: io: use const generics for read/write offsets
Posted by Joel Fernandes 1 week, 6 days ago
On Thu, Sep 18, 2025 at 03:02:11PM +0000, Alice Ryhl wrote:
> Using build_assert! to assert that offsets are in bounds is really
> fragile and likely to result in spurious and hard-to-debug build
> failures. Therefore, build_assert! should be avoided for this case.
> Thus, update the code to perform the check in const evaluation instead.

I really don't think this patch is a good idea (and nobody I spoke to thinks
so). Not only does it mess up the user's caller syntax completely, it is also
super confusing to pass both a generic and a function argument separately.

Sorry if I knew this would be the syntax, I would have objected even when we
spoke :)

I think the best fix (from any I've seen so far), is to move the bindings
calls of offending code into a closure and call the closure directly, as I
posted in the other thread. I also passed the closure idea by Gary and he
confirmed the compiler should behave correctly (I will check the code gen
with/without later). Gary also provided a brilliant suggestion that we can
call the closure directly instead of assigning it to a variable first. That
fix is also smaller, and does not screw up the users. APIs should fix issues
within them instead of relying on user to work around them.

So from my side, NAK. But I do appreciate you taking a look!

thanks,

 - Joel

> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  drivers/gpu/drm/tyr/regs.rs     |  4 ++--
>  rust/kernel/devres.rs           |  4 ++--
>  rust/kernel/io.rs               | 18 ++++++++++--------
>  rust/kernel/io/mem.rs           |  6 +++---
>  samples/rust/rust_driver_pci.rs | 10 +++++-----
>  5 files changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
> index f46933aaa2214ee0ac58b1ea2a6aa99506a35b70..e3c306e48e86d1d6047cab7944e0fe000901d48b 100644
> --- a/drivers/gpu/drm/tyr/regs.rs
> +++ b/drivers/gpu/drm/tyr/regs.rs
> @@ -25,13 +25,13 @@
>  impl<const OFFSET: usize> Register<OFFSET> {
>      #[inline]
>      pub(crate) fn read(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<u32> {
> -        let value = (*iomem).access(dev)?.read32(OFFSET);
> +        let value = (*iomem).access(dev)?.read32::<OFFSET>();
>          Ok(value)
>      }
>  
>      #[inline]
>      pub(crate) fn write(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>, value: u32) -> Result {
> -        (*iomem).access(dev)?.write32(value, OFFSET);
> +        (*iomem).access(dev)?.write32::<OFFSET>(value);
>          Ok(())
>      }
>  }
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index da18091143a67fcfbb247e7cb4f59f5a4932cac5..3e66e10c05fa078e42162c7a367161fbf735a07f 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -96,7 +96,7 @@ struct Inner<T: Send> {
>  /// let devres = KBox::pin_init(Devres::new(dev, iomem), GFP_KERNEL)?;
>  ///
>  /// let res = devres.try_access().ok_or(ENXIO)?;
> -/// res.write8(0x42, 0x0);
> +/// res.write8::<0x0>(0x42);
>  /// # Ok(())
>  /// # }
>  /// ```
> @@ -232,7 +232,7 @@ pub fn device(&self) -> &Device {
>      ///
>      ///     // might_sleep()
>      ///
> -    ///     bar.write32(0x42, 0x0);
> +    ///     bar.write32::<0x0>(0x42);
>      ///
>      ///     Ok(())
>      /// }
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 03b467722b8651ebecd660ac0e2d849cf88dc915..563ff8488100d9e07a7f4bffeb085db7bd7e9d6a 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -103,7 +103,7 @@ pub fn maxsize(&self) -> usize {
>  ///# fn no_run() -> Result<(), Error> {
>  /// // SAFETY: Invalid usage for example purposes.
>  /// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
> -/// iomem.write32(0x42, 0x0);
> +/// iomem.write32::<0x0>(0x42);
>  /// assert!(iomem.try_write32(0x42, 0x0).is_ok());
>  /// assert!(iomem.try_write32(0x42, 0x4).is_err());
>  /// # Ok(())
> @@ -120,8 +120,8 @@ macro_rules! define_read {
>          /// time, the build will fail.
>          $(#[$attr])*
>          #[inline]
> -        pub fn $name(&self, offset: usize) -> $type_name {
> -            let addr = self.io_addr_assert::<$type_name>(offset);
> +        pub fn $name<const OFF: usize>(&self) -> $type_name {
> +            let addr = self.io_addr_assert::<$type_name, OFF>();
>  
>              // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
>              unsafe { bindings::$c_fn(addr as *const c_void) }
> @@ -149,8 +149,8 @@ macro_rules! define_write {
>          /// time, the build will fail.
>          $(#[$attr])*
>          #[inline]
> -        pub fn $name(&self, value: $type_name, offset: usize) {
> -            let addr = self.io_addr_assert::<$type_name>(offset);
> +        pub fn $name<const OFF: usize>(&self, value: $type_name) {
> +            let addr = self.io_addr_assert::<$type_name, OFF>();
>  
>              // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
>              unsafe { bindings::$c_fn(value, addr as *mut c_void) }
> @@ -217,10 +217,12 @@ fn io_addr<U>(&self, offset: usize) -> Result<usize> {
>      }
>  
>      #[inline]
> -    fn io_addr_assert<U>(&self, offset: usize) -> usize {
> -        build_assert!(Self::offset_valid::<U>(offset, SIZE));
> +    fn io_addr_assert<U, const OFF: usize>(&self) -> usize {
> +        const {
> +            build_assert!(Self::offset_valid::<U>(OFF, SIZE));
> +        }
>  
> -        self.addr() + offset
> +        self.addr() + OFF
>      }
>  
>      define_read!(read8, try_read8, readb -> u8);
> diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
> index 6f99510bfc3a63dd72c1d47dc661dcd48fa7f54e..b73557f5f57c955ac251a46c9bdd6df0687411e2 100644
> --- a/rust/kernel/io/mem.rs
> +++ b/rust/kernel/io/mem.rs
> @@ -54,7 +54,7 @@ pub(crate) unsafe fn new(device: &'a Device<Bound>, resource: &'a Resource) -> S
>      ///       pdev: &platform::Device<Core>,
>      ///       info: Option<&Self::IdInfo>,
>      ///    ) -> Result<Pin<KBox<Self>>> {
> -    ///       let offset = 0; // Some offset.
> +    ///       const OFFSET: usize = 0; // Some offset.
>      ///
>      ///       // If the size is known at compile time, use [`Self::iomap_sized`].
>      ///       //
> @@ -66,9 +66,9 @@ pub(crate) unsafe fn new(device: &'a Device<Bound>, resource: &'a Resource) -> S
>      ///       let io = iomem.access(pdev.as_ref())?;
>      ///
>      ///       // Read and write a 32-bit value at `offset`.
> -    ///       let data = io.read32_relaxed(offset);
> +    ///       let data = io.read32_relaxed::<OFFSET>();
>      ///
> -    ///       io.write32_relaxed(data, offset);
> +    ///       io.write32_relaxed::<OFFSET>(data);
>      ///
>      ///       # Ok(KBox::new(SampleDriver, GFP_KERNEL)?.into())
>      ///     }
> diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
> index 606946ff4d7fd98e206ee6420a620d1c44eb0377..6f0388853e2b36e0800df5125a5dd8b20a6d5912 100644
> --- a/samples/rust/rust_driver_pci.rs
> +++ b/samples/rust/rust_driver_pci.rs
> @@ -46,17 +46,17 @@ struct SampleDriver {
>  impl SampleDriver {
>      fn testdev(index: &TestIndex, bar: &Bar0) -> Result<u32> {
>          // Select the test.
> -        bar.write8(index.0, Regs::TEST);
> +        bar.write8::<{ Regs::TEST }>(index.0);
>  
> -        let offset = u32::from_le(bar.read32(Regs::OFFSET)) as usize;
> -        let data = bar.read8(Regs::DATA);
> +        let offset = u32::from_le(bar.read32::<{ Regs::OFFSET }>()) as usize;
> +        let data = bar.read8::<{ Regs::DATA }>();
>  
>          // Write `data` to `offset` to increase `count` by one.
>          //
>          // Note that we need `try_write8`, since `offset` can't be checked at compile-time.
>          bar.try_write8(data, offset)?;
>  
> -        Ok(bar.read32(Regs::COUNT))
> +        Ok(bar.read32::<{ Regs::COUNT }>())
>      }
>  }
>  
> @@ -98,7 +98,7 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
>      fn unbind(pdev: &pci::Device<Core>, this: Pin<&Self>) {
>          if let Ok(bar) = this.bar.access(pdev.as_ref()) {
>              // Reset pci-testdev by writing a new test index.
> -            bar.write8(this.index.0, Regs::TEST);
> +            bar.write8::<{ Regs::TEST }>(this.index.0);
>          }
>      }
>  }
> 
> ---
> base-commit: cf4fd52e323604ccfa8390917593e1fb965653ee
> change-id: 20250918-write-offset-const-0b231c4282ea
> 
> Best regards,
> -- 
> Alice Ryhl <aliceryhl@google.com>
>
Re: [PATCH] rust: io: use const generics for read/write offsets
Posted by Danilo Krummrich 1 week, 6 days ago
On Thu Sep 18, 2025 at 8:13 PM CEST, Joel Fernandes wrote:
> On Thu, Sep 18, 2025 at 03:02:11PM +0000, Alice Ryhl wrote:
>> Using build_assert! to assert that offsets are in bounds is really
>> fragile and likely to result in spurious and hard-to-debug build
>> failures. Therefore, build_assert! should be avoided for this case.
>> Thus, update the code to perform the check in const evaluation instead.
>
> I really don't think this patch is a good idea (and nobody I spoke to thinks
> so). Not only does it mess up the user's caller syntax completely, it is also

I appreacite you raising the concern, but I rather have other people speak up
themselves.

> super confusing to pass both a generic and a function argument separately.

Why? We assert that the offset has to be const, whereas the value does not
have this requirement, so this makes perfect sense to me.

(I agree that it can look unfamiliar at the first glance though.)

> Sorry if I knew this would be the syntax, I would have objected even when we
> spoke :)
>
> I think the best fix (from any I've seen so far), is to move the bindings
> calls of offending code into a closure and call the closure directly, as I
> posted in the other thread. I also passed the closure idea by Gary and he
> confirmed the compiler should behave correctly (I will check the code gen
> with/without later). Gary also provided a brilliant suggestion that we can
> call the closure directly instead of assigning it to a variable first. That
> fix is also smaller, and does not screw up the users. APIs should fix issues
> within them instead of relying on user to work around them.

This is not a workaround, this is an idiomatic solution (which I probably should
have been doing already when I introduced the I/O code).

We do exactly the same thing for DmaMask::new() [1] and we agreed on doing the
same thing for Alignment as well [2].

[1] https://rust.docs.kernel.org/kernel/dma/struct.DmaMask.html#method.new
[2] https://lore.kernel.org/rust-for-linux/20250908-num-v5-1-c0f2f681ea96@nvidia.com/
Re: [PATCH] rust: io: use const generics for read/write offsets
Posted by Gary Guo 1 week, 5 days ago
On Fri, 19 Sep 2025 01:26:28 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:

> On Thu Sep 18, 2025 at 8:13 PM CEST, Joel Fernandes wrote:
> > On Thu, Sep 18, 2025 at 03:02:11PM +0000, Alice Ryhl wrote:  
> >> Using build_assert! to assert that offsets are in bounds is really
> >> fragile and likely to result in spurious and hard-to-debug build
> >> failures. Therefore, build_assert! should be avoided for this case.
> >> Thus, update the code to perform the check in const evaluation instead.  
> >
> > I really don't think this patch is a good idea (and nobody I spoke to thinks
> > so). Not only does it mess up the user's caller syntax completely, it is also  
> 
> I appreacite you raising the concern, but I rather have other people speak up
> themselves.

Joel asked me for opinion on this syntax during Kangrejos and I said
that I hated it.

> 
> > super confusing to pass both a generic and a function argument separately.  
> 
> Why? We assert that the offset has to be const, whereas the value does not
> have this requirement, so this makes perfect sense to me.

In some peripherals there are also cases where there are a window of
registers and you want to use a computed value to access them.
If I have a window of registers where the first address is `FOO` and
the last is `FOO_END`, then I might reasonable want to do access
registers in a loop.

> 
> (I agree that it can look unfamiliar at the first glance though.)
> 
> > Sorry if I knew this would be the syntax, I would have objected even when we
> > spoke :)
> >
> > I think the best fix (from any I've seen so far), is to move the bindings
> > calls of offending code into a closure and call the closure directly, as I
> > posted in the other thread. I also passed the closure idea by Gary and he
> > confirmed the compiler should behave correctly (I will check the code gen
> > with/without later). Gary also provided a brilliant suggestion that we can
> > call the closure directly instead of assigning it to a variable first. That
> > fix is also smaller, and does not screw up the users. APIs should fix issues
> > within them instead of relying on user to work around them.  
> 
> This is not a workaround, this is an idiomatic solution (which I probably should
> have been doing already when I introduced the I/O code).

I don't think this can be said to be idiomatic. Syntax churn is a real
issue, and quite a big one.

Turbofish is cumbersome to write with just magic numbers, and the
fact `{}` is needed to pass in constant expressions made this much
worse. If the drivers try hard to avoid magic numbers, you would
effective require  all code to be `::<{ ... }>()` and this is ugly.

In the design of generic atomics this is taken into consideration and
is why the `load`/`store` functions don't take `Ordering` type
parameter itself, but additionally takes a value of it and just throw it
away. This is so that you can still write `.load(Relaxed)` rather
than needing to write `.load::<Relaxed>()`.

If we have argument position const generics today, I'd say let's make
the switch. However with the tools that we have I don't think it's a
clear cut. I would even personally prefer a BUG than to having to do
turbofish every single time.

Best,
Gary

> 
> We do exactly the same thing for DmaMask::new() [1] and we agreed on doing the
> same thing for Alignment as well [2].
> 
> [1] https://rust.docs.kernel.org/kernel/dma/struct.DmaMask.html#method.new
> [2] https://lore.kernel.org/rust-for-linux/20250908-num-v5-1-c0f2f681ea96@nvidia.com/
Re: [PATCH] rust: io: use const generics for read/write offsets
Posted by Danilo Krummrich 1 week, 5 days ago
On Fri Sep 19, 2025 at 10:56 PM CEST, Gary Guo wrote:
> Turbofish is cumbersome to write with just magic numbers, and the
> fact `{}` is needed to pass in constant expressions made this much
> worse. If the drivers try hard to avoid magic numbers, you would
> effective require  all code to be `::<{ ... }>()` and this is ugly.

In the absolute majority of cases users won't see any of that anyways, since
they'll use the register!() macro generated types.

   // Master Control Register (MCR)
   //
   // Stores the offset as associated constant.
   let mcr = regs::MCR::default();

   // Set the enabled bit.
   mcr.enable();

   // Write the data on the bus.
   //
   // Calls `io.write<Self::OFFSET>(self.data())` internally.
   mcr.write(&io);

For indexed registers it is indeed a problem though.
Re: [PATCH] rust: io: use const generics for read/write offsets
Posted by Joel Fernandes 1 week, 5 days ago
Hello, Danilo,

> On Sep 19, 2025, at 1:26 AM, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Thu Sep 18, 2025 at 8:13 PM CEST, Joel Fernandes wrote:
>>> On Thu, Sep 18, 2025 at 03:02:11PM +0000, Alice Ryhl wrote:
>>> Using build_assert! to assert that offsets are in bounds is really
>>> fragile and likely to result in spurious and hard-to-debug build
>>> failures. Therefore, build_assert! should be avoided for this case.
>>> Thus, update the code to perform the check in const evaluation instead.
>> 
>> I really don't think this patch is a good idea (and nobody I spoke to thinks
>> so). Not only does it mess up the user's caller syntax completely, it is also
> 
> I appreacite you raising the concern,
> but I rather have other people speak up
> themselves.

I did not mean to speak for others, sorry it came across like that (and that is certainly not what I normally do). But I discussed the patch in person since we are at a conference and discussing it in person, and I did not get a lot of consensus on this. That is what I was trying to say. If it was a brilliant or great idea, I would have hoped for at least one person to tell me that this is exactly how we should do it.

> 
>> super confusing to pass both a generic and a function argument separately.
> 
> Why? We assert that the offset has to be const, whereas the value does not
> have this requirement, so this makes perfect sense to me.
> 
> (I agree that it can look unfamiliar at the first glance though.)

Yes the familiarity is the issue. I still do not feel using a generic for this looks ok to me and I think we can fix it differently, I will try to come up with an alternative fix unless we have already decided to use generics for this.

> 
>> Sorry if I knew this would be the syntax, I would have objected even when we
>> spoke :)
>> 
>> I think the best fix (from any I've seen so far), is to move the bindings
>> calls of offending code into a closure and call the closure directly, as I
>> posted in the other thread. I also passed the closure idea by Gary and he
>> confirmed the compiler should behave correctly (I will check the code gen
>> with/without later). Gary also provided a brilliant suggestion that we can
>> call the closure directly instead of assigning it to a variable first. That
>> fix is also smaller, and does not screw up the users. APIs should fix issues
>> within them instead of relying on user to work around them.
> 
> This is not a workaround, this is an idiomatic solution (which I probably should
> have been doing already when I introduced the I/O code).
> 
> We do exactly the same thing for DmaMask::new() [1] and we agreed on doing the
> same thing for Alignment as well [2].
> 
> [1] https://rust.docs.kernel.org/kernel/dma/struct.DmaMask.html#method.new
> [2] https://lore.kernel.org/rust-for-linux/20250908-num-v5-1-c0f2f681ea96@nvidia.com/

Ah ok. Since there is precedent, I am ok with it, especially since you feel so strongly about it and since you are the rust IO code maintainer.

But passing one parameter as a constant generic and another parameter as a function parameter, just seems weird to me even if there is precedent.

And one can argue that the value is also a constant right? It is confusing and makes callers unreadable IMHO.

Cheers,

 - Joel
Re: [PATCH] rust: io: use const generics for read/write offsets
Posted by Benno Lossin 1 week, 5 days ago
On Fri Sep 19, 2025 at 9:59 AM CEST, Joel Fernandes wrote:
> Hello, Danilo,
>
>> On Sep 19, 2025, at 1:26 AM, Danilo Krummrich <dakr@kernel.org> wrote:
>> 
>> On Thu Sep 18, 2025 at 8:13 PM CEST, Joel Fernandes wrote:
>>>> On Thu, Sep 18, 2025 at 03:02:11PM +0000, Alice Ryhl wrote:
>>>> Using build_assert! to assert that offsets are in bounds is really
>>>> fragile and likely to result in spurious and hard-to-debug build
>>>> failures. Therefore, build_assert! should be avoided for this case.
>>>> Thus, update the code to perform the check in const evaluation instead.
>>> 
>>> I really don't think this patch is a good idea (and nobody I spoke to thinks
>>> so). Not only does it mess up the user's caller syntax completely, it is also
>> 
>> I appreacite you raising the concern,
>> but I rather have other people speak up
>> themselves.
>
> I did not mean to speak for others, sorry it came across like that
> (and that is certainly not what I normally do). But I discussed the
> patch in person since we are at a conference and discussing it in
> person, and I did not get a lot of consensus on this. That is what I
> was trying to say. If it was a brilliant or great idea, I would have
> hoped for at least one person to tell me that this is exactly how we
> should do it.

I'm also not really thrilled to see lots more turbofish syntax. However,
if we can avoid the nasty build_assert errors then in my opinion it's
better. (yes we do have Gary's cool klint tool to handle them correctly,
but not every user will be aware of that tool).

Maybe we should ask Rust about adding `const` arguments in their normal
position again :)

---
Cheers,
Benno
Re: [PATCH] rust: io: use const generics for read/write offsets
Posted by Joel Fernandes 1 week, 5 days ago
On Fri, Sep 19, 2025 at 11:26:19AM +0200, Benno Lossin wrote:
> On Fri Sep 19, 2025 at 9:59 AM CEST, Joel Fernandes wrote:
> > Hello, Danilo,
> >
> >> On Sep 19, 2025, at 1:26 AM, Danilo Krummrich <dakr@kernel.org> wrote:
> >> 
> >> On Thu Sep 18, 2025 at 8:13 PM CEST, Joel Fernandes wrote:
> >>>> On Thu, Sep 18, 2025 at 03:02:11PM +0000, Alice Ryhl wrote:
> >>>> Using build_assert! to assert that offsets are in bounds is really
> >>>> fragile and likely to result in spurious and hard-to-debug build
> >>>> failures. Therefore, build_assert! should be avoided for this case.
> >>>> Thus, update the code to perform the check in const evaluation
> >>>> instead.
> >>> 
> >>> I really don't think this patch is a good idea (and nobody I spoke to
> >>> thinks so). Not only does it mess up the user's caller syntax
> >>> completely, it is also
> >> 
> >> I appreacite you raising the concern,
> >> but I rather have other people speak up
> >> themselves.
> >
> > I did not mean to speak for others, sorry it came across like that
> > (and that is certainly not what I normally do). But I discussed the
> > patch in person since we are at a conference and discussing it in
> > person, and I did not get a lot of consensus on this. That is what I
> > was trying to say. If it was a brilliant or great idea, I would have
> > hoped for at least one person to tell me that this is exactly how we
> > should do it.
> 
> I'm also not really thrilled to see lots more turbofish syntax. However,
> if we can avoid the nasty build_assert errors then in my opinion it's
> better. (yes we do have Gary's cool klint tool to handle them correctly,

Yes, thanks. Also I tried to apply this patch and it doesn't always work
because of array indexing usecase in nova, where we compute the offset based
on a runtime register index  (**/nova-core/**/macros.rs). Here idx is not a
constant:

            /// Read the array register at index `idx` from its address in `io`.
            #[inline(always)]
            pub(crate) fn read<const SIZE: usize, T>(
                io: &T,
                idx: usize,
            ) -> Self where
                T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,

In **/ga102.rs, we have the following usage where ucode_idx cannot be const:

regs::NV_FUSE_OPT_FPF_SEC2_UCODE1_VERSION::read(bar, ucode_idx).data()

So I am afraid this wont work. Also even if it did work, it means now we have
to also put idx as a const generic (turbofish syntax).

thanks,

 - Joel

Re: [PATCH] rust: io: use const generics for read/write offsets
Posted by Alexandre Courbot 1 week, 3 days ago
On Sat Sep 20, 2025 at 5:53 AM JST, Joel Fernandes wrote:
> On Fri, Sep 19, 2025 at 11:26:19AM +0200, Benno Lossin wrote:
>> On Fri Sep 19, 2025 at 9:59 AM CEST, Joel Fernandes wrote:
>> > Hello, Danilo,
>> >
>> >> On Sep 19, 2025, at 1:26 AM, Danilo Krummrich <dakr@kernel.org> wrote:
>> >> 
>> >> On Thu Sep 18, 2025 at 8:13 PM CEST, Joel Fernandes wrote:
>> >>>> On Thu, Sep 18, 2025 at 03:02:11PM +0000, Alice Ryhl wrote:
>> >>>> Using build_assert! to assert that offsets are in bounds is really
>> >>>> fragile and likely to result in spurious and hard-to-debug build
>> >>>> failures. Therefore, build_assert! should be avoided for this case.
>> >>>> Thus, update the code to perform the check in const evaluation
>> >>>> instead.
>> >>> 
>> >>> I really don't think this patch is a good idea (and nobody I spoke to
>> >>> thinks so). Not only does it mess up the user's caller syntax
>> >>> completely, it is also
>> >> 
>> >> I appreacite you raising the concern,
>> >> but I rather have other people speak up
>> >> themselves.
>> >
>> > I did not mean to speak for others, sorry it came across like that
>> > (and that is certainly not what I normally do). But I discussed the
>> > patch in person since we are at a conference and discussing it in
>> > person, and I did not get a lot of consensus on this. That is what I
>> > was trying to say. If it was a brilliant or great idea, I would have
>> > hoped for at least one person to tell me that this is exactly how we
>> > should do it.
>> 
>> I'm also not really thrilled to see lots more turbofish syntax. However,
>> if we can avoid the nasty build_assert errors then in my opinion it's
>> better. (yes we do have Gary's cool klint tool to handle them correctly,
>
> Yes, thanks. Also I tried to apply this patch and it doesn't always work
> because of array indexing usecase in nova, where we compute the offset based
> on a runtime register index  (**/nova-core/**/macros.rs). Here idx is not a
> constant:
>
>             /// Read the array register at index `idx` from its address in `io`.
>             #[inline(always)]
>             pub(crate) fn read<const SIZE: usize, T>(
>                 io: &T,
>                 idx: usize,
>             ) -> Self where
>                 T: ::core::ops::Deref<Target = ::kernel::io::Io<SIZE>>,
>
> In **/ga102.rs, we have the following usage where ucode_idx cannot be const:
>
> regs::NV_FUSE_OPT_FPF_SEC2_UCODE1_VERSION::read(bar, ucode_idx).data()
>
> So I am afraid this wont work. Also even if it did work, it means now we have
> to also put idx as a const generic (turbofish syntax).

We could always use the `try_read*` variant for these, but that would
introduce runtime checking for errors that can't happen. We have been
pretty successful in avoiding using `try_read*` in Nova so far, and I
think that's something we should try to preserve as it brings confidence
that our register accesses are correct.