Re: [PATCH v3] rust: io: use newtype for PhysAddr

Re: [PATCH v3] rust: io: use newtype for PhysAddr

[Lorenzo Delgado]

Hi,

Are you still planning a v4? v3 no longer applies to driver-core-next
since ResourceSize became a newtype, and there's a new PhysAddr user in
drivers/gpu/drm/tyr/vm.rs.

If not, I'm happy to pick it up and credit your v3.

Thanks,
Lorenzo

[PATCH v2] rust: io: use newtype for PhysAddr

[x]

From: Favilances Noir <favilances@proton.me>

Currently, PhysAddr is a type alias for phys_addr_t. This means
that physical addresses can be mixed with unrelated integer values
without Rust noticing.

Use a transparent newtype for PhysAddr instead, and update the
affected call sites to perform explicit raw conversions at the FFI
boundary.

This keeps the existing phys_addr_t representation while making
physical addresses distinct in Rust type checking.

Drop the unused Clone and Copy derives from PhysAddr, as pointed out
in review.

Signed-off-by: Favilances Noir <favilances@proton.me>
---
 rust/kernel/devres.rs        |  8 +++++---
 rust/kernel/io.rs            | 29 +++++++++++++++++++++++------
 rust/kernel/io/mem.rs        |  4 ++--
 rust/kernel/io/resource.rs   |  6 +++---
 rust/kernel/iommu/pgtable.rs |  2 +-
 5 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 9e5f93aed20c..beb3723d2519 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -85,10 +85,10 @@ struct Inner<T> {
 ///     ///
 ///     /// [`paddr`, `paddr` + `SIZE`) must be a valid MMIO region that is mappable into the CPUs
 ///     /// virtual address space.
-///     unsafe fn new(paddr: usize) -> Result<Self>{
+///     unsafe fn new(paddr: PhysAddr) -> Result<Self>{
 ///         // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is
 ///         // valid for `ioremap`.
-///         let addr = unsafe { bindings::ioremap(paddr as PhysAddr, SIZE) };
+///         let addr = unsafe { bindings::ioremap(paddr.into_raw(), SIZE) };
 ///         if addr.is_null() {
 ///             return Err(ENOMEM);
 ///         }
@@ -114,7 +114,9 @@ struct Inner<T> {
 /// }
 /// # fn no_run(dev: &Device<Bound>) -> Result<(), Error> {
 /// // SAFETY: Invalid usage for example purposes.
-/// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
+/// let iomem = unsafe {
+///     IoMem::<{ core::mem::size_of::<u32>() }>::new(PhysAddr::from_raw(0xBAAAAAAD))?
+/// };
 /// let devres = Devres::new(dev, iomem)?;
 ///
 /// let res = devres.try_access().ok_or(ENXIO)?;
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index fcc7678fd9e3..999fa285a6ec 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -21,9 +21,24 @@
 
 /// Physical address type.
 ///
-/// This is a type alias to either `u32` or `u64` depending on the config option
-/// `CONFIG_PHYS_ADDR_T_64BIT`, and it can be a u64 even on 32-bit architectures.
-pub type PhysAddr = bindings::phys_addr_t;
+/// This wraps either `u32` or `u64` depending on the config option
+/// `CONFIG_PHYS_ADDR_T_64BIT`, and it can be a `u64` even on 32-bit architectures.
+#[repr(transparent)]
+pub struct PhysAddr(bindings::phys_addr_t);
+
+impl PhysAddr {
+    /// Creates a physical address from the raw C type.
+    #[inline]
+    pub const fn from_raw(value: bindings::phys_addr_t) -> Self {
+        Self(value)
+    }
+
+    /// Turns this physical address into the raw C type.
+    #[inline]
+    pub const fn into_raw(self) -> bindings::phys_addr_t {
+        self.0
+    }
+}
 
 /// Resource Size type.
 ///
@@ -101,10 +116,10 @@ pub fn maxsize(&self) -> usize {
 ///     ///
 ///     /// [`paddr`, `paddr` + `SIZE`) must be a valid MMIO region that is mappable into the CPUs
 ///     /// virtual address space.
-///     unsafe fn new(paddr: usize) -> Result<Self>{
+///     unsafe fn new(paddr: PhysAddr) -> Result<Self>{
 ///         // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is
 ///         // valid for `ioremap`.
-///         let addr = unsafe { bindings::ioremap(paddr as PhysAddr, SIZE) };
+///         let addr = unsafe { bindings::ioremap(paddr.into_raw(), SIZE) };
 ///         if addr.is_null() {
 ///             return Err(ENOMEM);
 ///         }
@@ -131,7 +146,9 @@ 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)? };
+/// let iomem = unsafe {
+///     IoMem::<{ core::mem::size_of::<u32>() }>::new(PhysAddr::from_raw(0xBAAAAAAD))?
+/// };
 /// iomem.write32(0x42, 0x0);
 /// assert!(iomem.try_write32(0x42, 0x0).is_ok());
 /// assert!(iomem.try_write32(0x42, 0x4).is_err());
diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
index 7dc78d547f7a..f21f9038b297 100644
--- a/rust/kernel/io/mem.rs
+++ b/rust/kernel/io/mem.rs
@@ -254,12 +254,12 @@ fn ioremap(resource: &Resource) -> Result<Self> {
             // SAFETY:
             // - `res_start` and `size` are read from a presumably valid `struct resource`.
             // - `size` is known not to be zero at this point.
-            unsafe { bindings::ioremap_np(res_start, size) }
+            unsafe { bindings::ioremap_np(res_start.into_raw(), size) }
         } else {
             // SAFETY:
             // - `res_start` and `size` are read from a presumably valid `struct resource`.
             // - `size` is known not to be zero at this point.
-            unsafe { bindings::ioremap(res_start, size) }
+            unsafe { bindings::ioremap(res_start.into_raw(), size) }
         };
 
         if addr.is_null() {
diff --git a/rust/kernel/io/resource.rs b/rust/kernel/io/resource.rs
index b7ac9faf141d..5dda1061db68 100644
--- a/rust/kernel/io/resource.rs
+++ b/rust/kernel/io/resource.rs
@@ -58,7 +58,7 @@ fn drop(&mut self) {
         };
 
         // SAFETY: Safe as per the invariant of `Region`.
-        unsafe { release_fn(start, size) };
+        unsafe { release_fn(start.into_raw(), size) };
     }
 }
 
@@ -113,7 +113,7 @@ pub fn request_region(
         let region = unsafe {
             bindings::__request_region(
                 self.0.get(),
-                start,
+                start.into_raw(),
                 size,
                 name.as_char_ptr(),
                 flags.0 as c_int,
@@ -137,7 +137,7 @@ pub fn size(&self) -> ResourceSize {
     pub fn start(&self) -> PhysAddr {
         let inner = self.0.get();
         // SAFETY: Safe as per the invariants of `Resource`.
-        unsafe { (*inner).start }
+        PhysAddr::from_raw(unsafe { (*inner).start })
     }
 
     /// Returns the name of the resource.
diff --git a/rust/kernel/iommu/pgtable.rs b/rust/kernel/iommu/pgtable.rs
index c88e38fd938a..ae724ca1e8ad 100644
--- a/rust/kernel/iommu/pgtable.rs
+++ b/rust/kernel/iommu/pgtable.rs
@@ -182,7 +182,7 @@ pub unsafe fn map_pages(
             (map_pages)(
                 self.raw_ops(),
                 iova,
-                paddr,
+                paddr.into_raw(),
                 pgsize,
                 pgcount,
                 prot as i32,
-- 
2.54.0

[PATCH v3] rust: io: use newtype for PhysAddr

[x]

From: Favilances Noir <favilances@proton.me>

Currently, PhysAddr is a type alias for phys_addr_t. This means
that physical addresses can be mixed with unrelated integer values
without Rust noticing.

Use a transparent newtype for PhysAddr instead, and update the
affected call sites to perform explicit raw conversions at the FFI
boundary.

This keeps the existing phys_addr_t representation while making
physical addresses distinct in Rust type checking.

Derive Clone, Copy, PartialEq, and Eq for PhysAddr, implement Debug
printing physical addresses in hexadecimal, and add From conversions
between PhysAddr and the raw phys_addr_t type for API ergonomics.

A fallible TryFrom<usize> conversion can be added later if needed.

Signed-off-by: Favilances Noir <favilances@proton.me>
---
v3:
- Re-add Clone and Copy derives as suggested
- Add PartialEq and Eq
- Add From impls for better ergonomics
- Add Debug impl printing hexadecimal values
- Keep explicit conversions at the FFI boundary
- Thanks to Onur and Danilo for the review and suggestions

 rust/kernel/devres.rs        |  8 +++---
 rust/kernel/io.rs            | 49 +++++++++++++++++++++++++++++++-----
 rust/kernel/io/mem.rs        |  4 +--
 rust/kernel/io/resource.rs   |  6 ++---
 rust/kernel/iommu/pgtable.rs |  2 +-
 5 files changed, 54 insertions(+), 15 deletions(-)

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 9e5f93aed20c..beb3723d2519 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -85,10 +85,10 @@ struct Inner<T> {
 ///     ///
 ///     /// [`paddr`, `paddr` + `SIZE`) must be a valid MMIO region that is mappable into the CPUs
 ///     /// virtual address space.
-///     unsafe fn new(paddr: usize) -> Result<Self>{
+///     unsafe fn new(paddr: PhysAddr) -> Result<Self>{
 ///         // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is
 ///         // valid for `ioremap`.
-///         let addr = unsafe { bindings::ioremap(paddr as PhysAddr, SIZE) };
+///         let addr = unsafe { bindings::ioremap(paddr.into_raw(), SIZE) };
 ///         if addr.is_null() {
 ///             return Err(ENOMEM);
 ///         }
@@ -114,7 +114,9 @@ struct Inner<T> {
 /// }
 /// # fn no_run(dev: &Device<Bound>) -> Result<(), Error> {
 /// // SAFETY: Invalid usage for example purposes.
-/// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
+/// let iomem = unsafe {
+///     IoMem::<{ core::mem::size_of::<u32>() }>::new(PhysAddr::from_raw(0xBAAAAAAD))?
+/// };
 /// let devres = Devres::new(dev, iomem)?;
 ///
 /// let res = devres.try_access().ok_or(ENXIO)?;
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index fcc7678fd9e3..e9e8944ddea6 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -6,6 +6,7 @@
 
 use crate::{
     bindings,
+    fmt,
     prelude::*, //
 };
 
@@ -21,9 +22,43 @@
 
 /// Physical address type.
 ///
-/// This is a type alias to either `u32` or `u64` depending on the config option
-/// `CONFIG_PHYS_ADDR_T_64BIT`, and it can be a u64 even on 32-bit architectures.
-pub type PhysAddr = bindings::phys_addr_t;
+/// This wraps either `u32` or `u64` depending on the config option
+/// `CONFIG_PHYS_ADDR_T_64BIT`, and it can be a `u64` even on 32-bit architectures.
+#[repr(transparent)]
+#[derive(Clone, Copy, PartialEq, Eq)]
+pub struct PhysAddr(bindings::phys_addr_t);
+
+impl PhysAddr {
+    /// Creates a physical address from the raw C type.
+    #[inline]
+    pub const fn from_raw(value: bindings::phys_addr_t) -> Self {
+        Self(value)
+    }
+
+    /// Turns this physical address into the raw C type.
+    #[inline]
+    pub const fn into_raw(self) -> bindings::phys_addr_t {
+        self.0
+    }
+}
+
+impl fmt::Debug for PhysAddr {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "{:#x}", self.0)
+    }
+}
+
+impl From<bindings::phys_addr_t> for PhysAddr {
+    fn from(value: bindings::phys_addr_t) -> Self {
+        Self::from_raw(value)
+    }
+}
+
+impl From<PhysAddr> for bindings::phys_addr_t {
+    fn from(value: PhysAddr) -> Self {
+        value.into_raw()
+    }
+}
 
 /// Resource Size type.
 ///
@@ -101,10 +136,10 @@ pub fn maxsize(&self) -> usize {
 ///     ///
 ///     /// [`paddr`, `paddr` + `SIZE`) must be a valid MMIO region that is mappable into the CPUs
 ///     /// virtual address space.
-///     unsafe fn new(paddr: usize) -> Result<Self>{
+///     unsafe fn new(paddr: PhysAddr) -> Result<Self>{
 ///         // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is
 ///         // valid for `ioremap`.
-///         let addr = unsafe { bindings::ioremap(paddr as PhysAddr, SIZE) };
+///         let addr = unsafe { bindings::ioremap(paddr.into_raw(), SIZE) };
 ///         if addr.is_null() {
 ///             return Err(ENOMEM);
 ///         }
@@ -131,7 +166,9 @@ 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)? };
+/// let iomem = unsafe {
+///     IoMem::<{ core::mem::size_of::<u32>() }>::new(PhysAddr::from_raw(0xBAAAAAAD))?
+/// };
 /// iomem.write32(0x42, 0x0);
 /// assert!(iomem.try_write32(0x42, 0x0).is_ok());
 /// assert!(iomem.try_write32(0x42, 0x4).is_err());
diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
index 7dc78d547f7a..f21f9038b297 100644
--- a/rust/kernel/io/mem.rs
+++ b/rust/kernel/io/mem.rs
@@ -254,12 +254,12 @@ fn ioremap(resource: &Resource) -> Result<Self> {
             // SAFETY:
             // - `res_start` and `size` are read from a presumably valid `struct resource`.
             // - `size` is known not to be zero at this point.
-            unsafe { bindings::ioremap_np(res_start, size) }
+            unsafe { bindings::ioremap_np(res_start.into_raw(), size) }
         } else {
             // SAFETY:
             // - `res_start` and `size` are read from a presumably valid `struct resource`.
             // - `size` is known not to be zero at this point.
-            unsafe { bindings::ioremap(res_start, size) }
+            unsafe { bindings::ioremap(res_start.into_raw(), size) }
         };
 
         if addr.is_null() {
diff --git a/rust/kernel/io/resource.rs b/rust/kernel/io/resource.rs
index b7ac9faf141d..5dda1061db68 100644
--- a/rust/kernel/io/resource.rs
+++ b/rust/kernel/io/resource.rs
@@ -58,7 +58,7 @@ fn drop(&mut self) {
         };
 
         // SAFETY: Safe as per the invariant of `Region`.
-        unsafe { release_fn(start, size) };
+        unsafe { release_fn(start.into_raw(), size) };
     }
 }
 
@@ -113,7 +113,7 @@ pub fn request_region(
         let region = unsafe {
             bindings::__request_region(
                 self.0.get(),
-                start,
+                start.into_raw(),
                 size,
                 name.as_char_ptr(),
                 flags.0 as c_int,
@@ -137,7 +137,7 @@ pub fn size(&self) -> ResourceSize {
     pub fn start(&self) -> PhysAddr {
         let inner = self.0.get();
         // SAFETY: Safe as per the invariants of `Resource`.
-        unsafe { (*inner).start }
+        PhysAddr::from_raw(unsafe { (*inner).start })
     }
 
     /// Returns the name of the resource.
diff --git a/rust/kernel/iommu/pgtable.rs b/rust/kernel/iommu/pgtable.rs
index c88e38fd938a..ae724ca1e8ad 100644
--- a/rust/kernel/iommu/pgtable.rs
+++ b/rust/kernel/iommu/pgtable.rs
@@ -182,7 +182,7 @@ pub unsafe fn map_pages(
             (map_pages)(
                 self.raw_ops(),
                 iova,
-                paddr,
+                paddr.into_raw(),
                 pgsize,
                 pgcount,
                 prot as i32,
-- 
2.54.0

Re: [PATCH v3] rust: io: use newtype for PhysAddr

[Miguel Ojeda]

On Tue, May 5, 2026 at 10:24 AM <x@2005.tr> wrote:
>
> From: Favilances Noir <favilances@proton.me>

It is great to see more strong types, thanks for this!

From the other parallel patch:

Suggested-by: Miguel Ojeda <ojeda@kernel.org>
Link: https://github.com/Rust-for-Linux/linux/issues/1204

By the way, please try to fix the email From header -- it can be quite
confusing to see "x" in the mailing list and other tooling... :)

We also probably want a few more `#[inline]`s as Sashiko mentions.

Cheers,
Miguel