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

[Onur Özkan]

On Sun, 03 May 2026 13:30:50 +0300
x@xxxxxxx wrote:

> From: Favilances Noir <favilances@xxxxxxxxx>
> 
> From: Favilances Noir <favilances@xxxxxxxxx>
> 
> 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.
> 
> Signed-off-by: Favilances Noir <favilances@xxxxxxxxx>
> ---
>  rust/kernel/devres.rs        |  8 +++++---
>  rust/kernel/io.rs            | 30 ++++++++++++++++++++++++------
>  rust/kernel/io/mem.rs        |  4 ++--
>  rust/kernel/io/resource.rs   |  6 +++---
>  rust/kernel/iommu/pgtable.rs |  2 +-
>  5 files changed, 35 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..2d55ee93caae 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -21,9 +21,25 @@
>  
>  /// 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)]

Copy and Clone seem never used, please drop them (also see [1]).

[1]: https://rust-for-linux.zulipchat.com/#narrow/channel/509436-Nova/topic/clone.2Fcopy.20additions

Regards,
Onur

> +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 +117,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 +147,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
>