Re: [PATCH v6 07/20] rust: io: implement `Mmio` as view type
Daniel Almeida <[email protected]>
| Newsgroups | org.kernel.vger.linux-pwm,dev.linux.lists.driver-core,dev.linux.lists.nova-gpu,org.freedesktop.lists.dri-devel,org.kernel.vger.linux-kernel,org.kernel.vger.linux-pci,org.kernel.vger.rust-for-linux |
|---|---|
| Message-ID | <[email protected]> |
> On 6 Jul 2026, at 09:44, Gary Guo <[email protected]> wrote: > > Implement `Mmio` as view type and convert `RelaxedMmio` to view type as > well. I/O implementations of `MmioOwned` are changed to delegate to the > `Mmio` view type. > > All existing users of `MmioOwned` in the documentation which do not > actually reflect the owning semantics is converted. > > Reviewed-by: Alexandre Courbot <[email protected]> > Signed-off-by: Gary Guo <[email protected]> > --- > rust/kernel/io.rs | 176 ++++++++++++++++++++++++++++++++++----------- > rust/kernel/io/poll.rs | 10 +-- > rust/kernel/io/register.rs | 24 +++---- > 3 files changed, 153 insertions(+), 57 deletions(-) > > diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs > index f93be7f78069..8110b49aa430 100644 > --- a/rust/kernel/io.rs > +++ b/rust/kernel/io.rs > @@ -4,6 +4,10 @@ > //! > //! C header: [`include/asm-generic/io.h`](srctree/include/asm-generic/io.h) > > +use core::{ > + marker::PhantomData, // > +}; > + > use crate::{ > bindings, > prelude::*, > @@ -537,10 +541,11 @@ fn write64(self, value: u64, offset: usize) > /// ```no_run > /// use kernel::io::{ > /// Io, > - /// MmioOwned, > + /// Mmio, > + /// Region, > /// }; > /// > - /// fn do_reads(io: &MmioOwned) -> Result { > + /// fn do_reads(io: Mmio<'_, Region>) -> Result { > /// // 32-bit read from address `0x10`. > /// let v: u32 = io.try_read(0x10)?; > /// > @@ -571,10 +576,11 @@ fn try_read<T, L>(self, location: L) -> Result<T> > /// ```no_run > /// use kernel::io::{ > /// Io, > - /// MmioOwned, > + /// Mmio, > + /// Region, > /// }; > /// > - /// fn do_writes(io: &MmioOwned) -> Result { > + /// fn do_writes(io: Mmio<'_, Region>) -> Result { > /// // 32-bit write of value `1` at address `0x10`. > /// io.try_write(0x10, 1u32)?; > /// > @@ -609,7 +615,8 @@ fn try_write<T, L>(self, location: L, value: T) -> Result > /// use kernel::io::{ > /// register, > /// Io, > - /// MmioOwned, > + /// Mmio, > + /// Region, > /// }; > /// > /// register! { > @@ -625,7 +632,7 @@ fn try_write<T, L>(self, location: L, value: T) -> Result > /// } > /// } > /// > - /// fn do_write_reg(io: &MmioOwned) -> Result { > + /// fn do_write_reg(io: Mmio<'_, Region>) -> Result { > /// > /// io.try_write_reg(VERSION::new(1, 0)) > /// } > @@ -654,10 +661,11 @@ fn try_write_reg<T, L, V>(self, value: V) -> Result > /// ```no_run > /// use kernel::io::{ > /// Io, > - /// MmioOwned, > + /// Mmio, > + /// Region, > /// }; > /// > - /// fn do_update(io: &MmioOwned<0x1000>) -> Result { > + /// fn do_update(io: Mmio<'_, Region<0x1000>>) -> Result { > /// io.try_update(0x10, |v: u32| { > /// v + 1 > /// }) > @@ -691,10 +699,11 @@ fn try_update<T, L, F>(self, location: L, f: F) -> Result > /// ```no_run > /// use kernel::io::{ > /// Io, > - /// MmioOwned, > + /// Mmio, > + /// Region, > /// }; > /// > - /// fn do_reads(io: &MmioOwned<0x1000>) { > + /// fn do_reads(io: Mmio<'_, Region<0x1000>>) { > /// // 32-bit read from address `0x10`. > /// let v: u32 = io.read(0x10); > /// > @@ -723,10 +732,11 @@ fn read<T, L>(self, location: L) -> T > /// ```no_run > /// use kernel::io::{ > /// Io, > - /// MmioOwned, > + /// Mmio, > + /// Region, > /// }; > /// > - /// fn do_writes(io: &MmioOwned<0x1000>) { > + /// fn do_writes(io: Mmio<'_, Region<0x1000>>) { > /// // 32-bit write of value `1` at address `0x10`. > /// io.write(0x10, 1u32); > /// > @@ -757,7 +767,8 @@ fn write<T, L>(self, location: L, value: T) > /// use kernel::io::{ > /// register, > /// Io, > - /// MmioOwned, > + /// Mmio, > + /// Region, > /// }; > /// > /// register! { > @@ -773,7 +784,7 @@ fn write<T, L>(self, location: L, value: T) > /// } > /// } > /// > - /// fn do_write_reg(io: &MmioOwned<0x1000>) { > + /// fn do_write_reg(io: Mmio<'_, Region<0x1000>>) { > /// io.write_reg(VERSION::new(1, 0)); > /// } > /// ``` > @@ -801,10 +812,11 @@ fn write_reg<T, L, V>(self, value: V) > /// ```no_run > /// use kernel::io::{ > /// Io, > - /// MmioOwned, > + /// Mmio, > + /// Region, > /// }; > /// > - /// fn do_update(io: &MmioOwned<0x1000>) { > + /// fn do_update(io: Mmio<'_, Region<0x1000>>) { > /// io.update(0x10, |v: u32| { > /// v + 1 > /// }) > @@ -828,16 +840,72 @@ fn update<T, L, F>(self, location: L, f: F) > } > } > > +/// A view of memory-mapped I/O region. > +/// > +/// # Invariant > +/// > +/// `ptr` points to a valid and aligned memory-mapped I/O region for the duration lifetime `'a`. > +pub struct Mmio<'a, T: ?Sized> { > + ptr: *mut T, > + phantom: PhantomData<&'a ()>, > +} > + > +impl<T: ?Sized> Copy for Mmio<'_, T> {} > +impl<T: ?Sized> Clone for Mmio<'_, T> { > + #[inline] > + fn clone(&self) -> Self { > + *self > + } > +} > + > +impl<'a, T: ?Sized> Mmio<'a, T> { > + /// Create a `Mmio`, providing the accessors to the MMIO mapping. > + /// > + /// # Safety > + /// > + /// `raw` represents a valid and aligned memory-mapped I/O region while `'a` is alive. > + #[inline] > + pub unsafe fn from_raw(raw: MmioRaw<T>) -> Self { > + // INVARIANT: Per safety requirement. > + Self { > + ptr: raw.ptr, > + phantom: PhantomData, > + } > + } > +} > + > +// SAFETY: `Mmio<'_, T>` is conceptually `&T` but in I/O memory. > +unsafe impl<T: ?Sized + Sync> Send for Mmio<'_, T> {} > + > +// SAFETY: `Mmio<'_, T>` is conceptually `&T` but in I/O memory. > +unsafe impl<T: ?Sized + Sync> Sync for Mmio<'_, T> {} > + > +impl<T: ?Sized + KnownSize> Io for Mmio<'_, T> { > + type Target = T; > + > + #[inline] > + fn addr(self) -> usize { > + self.ptr.addr() > + } > + > + #[inline] > + fn maxsize(self) -> usize { > + KnownSize::size(self.ptr) > + } > +} > + > /// Implements [`IoCapable`] on `$mmio` for `$ty` using `$read_fn` and `$write_fn`. > macro_rules! impl_mmio_io_capable { > ($mmio:ident, $(#[$attr:meta])* $ty:ty, $read_fn:ident, $write_fn:ident) => { > $(#[$attr])* > - impl<const SIZE: usize> IoCapable<$ty> for &$mmio<SIZE> { > + impl<T: ?Sized> IoCapable<$ty> for $mmio<'_, T> { > + #[inline] > unsafe fn io_read(self, address: usize) -> $ty { > // SAFETY: By the trait invariant `address` is a valid address for MMIO operations. > unsafe { bindings::$read_fn(address as *const c_void) } > } > > + #[inline] > unsafe fn io_write(self, value: $ty, address: usize) { > // SAFETY: By the trait invariant `address` is a valid address for MMIO operations. > unsafe { bindings::$write_fn(value, address as *mut c_void) } > @@ -847,17 +915,12 @@ unsafe fn io_write(self, value: $ty, address: usize) { > } > > // MMIO regions support 8, 16, and 32-bit accesses. > -impl_mmio_io_capable!(MmioOwned, u8, readb, writeb); > -impl_mmio_io_capable!(MmioOwned, u16, readw, writew); > -impl_mmio_io_capable!(MmioOwned, u32, readl, writel); > +impl_mmio_io_capable!(Mmio, u8, readb, writeb); > +impl_mmio_io_capable!(Mmio, u16, readw, writew); > +impl_mmio_io_capable!(Mmio, u32, readl, writel); > // MMIO regions on 64-bit systems also support 64-bit accesses. > -impl_mmio_io_capable!( > - MmioOwned, > - #[cfg(CONFIG_64BIT)] > - u64, > - readq, > - writeq > -); > +#[cfg(CONFIG_64BIT)] > +impl_mmio_io_capable!(Mmio, u64, readq, writeq); > > impl<'a, const SIZE: usize> Io for &'a MmioOwned<SIZE> { > type Target = Region<SIZE>; > @@ -875,6 +938,23 @@ fn maxsize(self) -> usize { > } > } > > +impl<'a, const SIZE: usize, T> IoCapable<T> for &'a MmioOwned<SIZE> > +where > + Mmio<'a, Region<SIZE>>: IoCapable<T>, > +{ > + #[inline] > + unsafe fn io_read(self, address: usize) -> T { > + // SAFETY: Per safety requirement. > + unsafe { self.as_view().io_read(address) } > + } > + > + #[inline] > + unsafe fn io_write(self, value: T, address: usize) { > + // SAFETY: Per safety requirement. > + unsafe { self.as_view().io_write(value, address) } > + } > +} > + > impl<const SIZE: usize> MmioOwned<SIZE> { > /// Converts an `MmioRaw` into an `MmioOwned` instance, providing the accessors to the MMIO > /// mapping. > @@ -887,19 +967,33 @@ pub unsafe fn from_raw(raw: &MmioRaw<Region<SIZE>>) -> &Self { > // SAFETY: `MmioOwned` is a transparent wrapper around `MmioRaw`. > unsafe { &*core::ptr::from_ref(raw).cast() } > } > + > + /// Return a view that covers the full region. > + #[inline] > + pub fn as_view(&self) -> Mmio<'_, Region<SIZE>> { > + // SAFETY: `Mmio` has same invariant as `MmioOwned`. > + unsafe { Mmio::from_raw(self.0) } > + } > } > > -/// [`MmioOwned`] wrapper using relaxed accessors. > +/// [`Mmio`] but using relaxed accessors. > /// > /// This type provides an implementation of [`Io`] that uses relaxed I/O MMIO operands instead of > /// the regular ones. > /// > -/// See [`MmioOwned::relaxed`] for a usage example. > -#[repr(transparent)] > -pub struct RelaxedMmio<const SIZE: usize = 0>(MmioOwned<SIZE>); > +/// See [`Mmio::relaxed`] for a usage example. > +pub struct RelaxedMmio<'a, T: ?Sized>(Mmio<'a, T>); > > -impl<'a, const SIZE: usize> Io for &'a RelaxedMmio<SIZE> { > - type Target = Region<SIZE>; > +impl<T: ?Sized> Copy for RelaxedMmio<'_, T> {} > +impl<T: ?Sized> Clone for RelaxedMmio<'_, T> { > + #[inline] > + fn clone(&self) -> Self { > + *self > + } > +} > + > +impl<T: ?Sized + KnownSize> Io for RelaxedMmio<'_, T> { > + type Target = T; > > #[inline] > fn addr(self) -> usize { > @@ -912,8 +1006,8 @@ fn maxsize(self) -> usize { > } > } > > -impl<const SIZE: usize> MmioOwned<SIZE> { > - /// Returns a [`RelaxedMmio`] reference that performs relaxed I/O operations. > +impl<'a, T: ?Sized> Mmio<'a, T> { > + /// Returns a [`RelaxedMmio`] that performs relaxed I/O operations. > /// > /// Relaxed accessors do not provide ordering guarantees with respect to DMA or memory accesses > /// and can be used when such ordering is not required. > @@ -923,20 +1017,20 @@ impl<const SIZE: usize> MmioOwned<SIZE> { > /// ```no_run > /// use kernel::io::{ > /// Io, > - /// MmioOwned, > + /// Mmio, > + /// Region, > /// RelaxedMmio, > /// }; > /// > - /// fn do_io(io: &MmioOwned<0x100>) { > + /// fn do_io(io: Mmio<'_, Region<0x100>>) { > /// // The access is performed using `readl_relaxed` instead of `readl`. > /// let v = io.relaxed().read32(0x10); > /// } > /// > /// ``` > - pub fn relaxed(&self) -> &RelaxedMmio<SIZE> { > - // SAFETY: `RelaxedMmio` is `#[repr(transparent)]` over `MmioOwned`, so `MmioOwned<SIZE>` > - // and `RelaxedMmio<SIZE>` have identical layout. > - unsafe { core::mem::transmute(self) } > + #[inline] > + pub fn relaxed(self) -> RelaxedMmio<'a, T> { > + RelaxedMmio(self) > } > } > > diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs > index 79828a8006b5..d75f2fcf46f2 100644 > --- a/rust/kernel/io/poll.rs > +++ b/rust/kernel/io/poll.rs > @@ -47,14 +47,15 @@ > /// ```no_run > /// use kernel::io::{ > /// Io, > -/// MmioOwned, > +/// Mmio, > +/// Region, > /// poll::read_poll_timeout, // > /// }; > /// use kernel::time::Delta; > /// > /// const HW_READY: u16 = 0x01; > /// > -/// fn wait_for_hardware<const SIZE: usize>(io: &MmioOwned<SIZE>) -> Result { > +/// fn wait_for_hardware<const SIZE: usize>(io: Mmio<'_, Region<SIZE>>) -> Result { > /// read_poll_timeout( > /// // The `op` closure reads the value of a specific status register. > /// || io.try_read16(0x1000), > @@ -134,14 +135,15 @@ pub fn read_poll_timeout<Op, Cond, T>( > /// ```no_run > /// use kernel::io::{ > /// Io, > -/// MmioOwned, > +/// Mmio, > +/// Region, > /// poll::read_poll_timeout_atomic, // > /// }; > /// use kernel::time::Delta; > /// > /// const HW_READY: u16 = 0x01; > /// > -/// fn wait_for_hardware<const SIZE: usize>(io: &MmioOwned<SIZE>) -> Result { > +/// fn wait_for_hardware<const SIZE: usize>(io: Mmio<'_, Region<SIZE>>) -> Result { > /// read_poll_timeout_atomic( > /// // The `op` closure reads the value of a specific status register. > /// || io.try_read16(0x1000), > diff --git a/rust/kernel/io/register.rs b/rust/kernel/io/register.rs > index fd3bf6220f95..6cb07fc92cc3 100644 > --- a/rust/kernel/io/register.rs > +++ b/rust/kernel/io/register.rs > @@ -58,7 +58,7 @@ > //! }, > //! num::Bounded, > //! }; > -//! # use kernel::io::MmioOwned; > +//! # use kernel::io::{Mmio, Region}; > //! # register! { > //! # pub BOOT_0(u32) @ 0x00000100 { > //! # 15:8 vendor_id; > @@ -66,7 +66,7 @@ > //! # 3:0 minor_revision; > //! # } > //! # } > -//! # fn test(io: &MmioOwned<0x1000>) { > +//! # fn test(io: Mmio<'_, Region<0x1000>>) { > //! # fn obtain_vendor_id() -> u8 { 0xff } > //! > //! // Read from the register's defined offset (0x100). > @@ -446,7 +446,7 @@ fn into_io_op(self) -> (FixedRegisterLoc<T>, T) { > /// Io, > /// }, > /// }; > -/// # use kernel::io::MmioOwned; > +/// # use kernel::io::{Mmio, Region}; > /// > /// register! { > /// FIXED_REG(u32) @ 0x100 { > @@ -455,7 +455,7 @@ fn into_io_op(self) -> (FixedRegisterLoc<T>, T) { > /// } > /// } > /// > -/// # fn test(io: &MmioOwned<0x1000>) { > +/// # fn test(io: Mmio<'_, Region<0x1000>>) { > /// let val = io.read(FIXED_REG); > /// > /// // Write from an already-existing value. > @@ -559,7 +559,7 @@ fn into_io_op(self) -> (FixedRegisterLoc<T>, T) { > /// Io, > /// }, > /// }; > -/// # use kernel::io::MmioOwned; > +/// # use kernel::io::{Mmio, Region}; > /// > /// // Type used to identify the base. > /// pub struct CpuCtlBase; > @@ -584,7 +584,7 @@ fn into_io_op(self) -> (FixedRegisterLoc<T>, T) { > /// } > /// } > /// > -/// # fn test(io: MmioOwned<0x1000>) { > +/// # fn test(io: Mmio<'_, Region<0x1000>>) { > /// // Read the status of `Cpu0`. > /// let cpu0_started = io.read(CPU_CTL::of::<Cpu0>()); > /// > @@ -601,7 +601,7 @@ fn into_io_op(self) -> (FixedRegisterLoc<T>, T) { > /// } > /// } > /// > -/// # fn test2(io: MmioOwned<0x1000>) { > +/// # fn test2(io: Mmio<'_, Region<0x1000>>) { > /// // Start the aliased `CPU0`, leaving its other fields untouched. > /// io.update(CPU_CTL_ALIAS::of::<Cpu0>(), |r| r.with_alias_start(true)); > /// # } > @@ -638,7 +638,7 @@ fn into_io_op(self) -> (FixedRegisterLoc<T>, T) { > /// Io, > /// }, > /// }; > -/// # use kernel::io::MmioOwned; > +/// # use kernel::io::{Mmio, Region}; > /// # fn get_scratch_idx() -> usize { > /// # 0x15 > /// # } > @@ -651,7 +651,7 @@ fn into_io_op(self) -> (FixedRegisterLoc<T>, T) { > /// } > /// } > /// > -/// # fn test(io: &MmioOwned<0x1000>) > +/// # fn test(io: Mmio<'_, Region<0x1000>>) > /// # -> Result<(), Error>{ > /// // Read scratch register 0, i.e. I/O address `0x80`. > /// let scratch_0 = io.read(SCRATCH::at(0)).value(); > @@ -724,7 +724,7 @@ fn into_io_op(self) -> (FixedRegisterLoc<T>, T) { > /// Io, > /// }, > /// }; > -/// # use kernel::io::MmioOwned; > +/// # use kernel::io::{Mmio, Region}; > /// # fn get_scratch_idx() -> usize { > /// # 0x15 > /// # } > @@ -752,7 +752,7 @@ fn into_io_op(self) -> (FixedRegisterLoc<T>, T) { > /// } > /// } > /// > -/// # fn test(io: &MmioOwned<0x1000>) -> Result<(), Error> { > +/// # fn test(io: Mmio<'_, Region<0x1000>>) -> Result<(), Error> { > /// // Read scratch register 0 of CPU0. > /// let scratch = io.read(CPU_SCRATCH::of::<Cpu0>().at(0)); > /// > @@ -794,7 +794,7 @@ fn into_io_op(self) -> (FixedRegisterLoc<T>, T) { > /// } > /// } > /// > -/// # fn test2(io: &MmioOwned<0x1000>) -> Result<(), Error> { > +/// # fn test2(io: Mmio<'_, Region<0x1000>>) -> Result<(), Error> { > /// let cpu0_status = io.read(CPU_FIRMWARE_STATUS::of::<Cpu0>()).status(); > /// # Ok(()) > /// # } > > -- > 2.54.0 > Ran the tests again, they’re passing. Tested-by: Daniel Almeida <[email protected]> Reviewed-by: Daniel Almeida <[email protected]>