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 <alicer...@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 <alicer...@google.com>
>
