Re: [PATCH v2 02/16] rust: io: add `IoRepr` trait

"Alexandre Courbot" <[email protected]>
Newsgroups 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 Thu Aug 6, 2026 at 1:35 AM JST, Gary Guo wrote:
> For types that are layout-compatible with an I/O capable type, we would
> want the ability to use them directly for I/O operations. E.g.
>
>     bitfield! {
>         pub struct Foo(u32) {
>             ...
>         }
>     }
>
>     #[repr(C)]
>     struct Bar {
>         foo: Foo,
>     }
>
>     let mmio: Mmio<'_, Bar> = ...;
>     io_read!(mmio, .foo)
>
> Currently this feature is available from `register!()` macro but not
> otherwise available with `io_read!`, `io_write!`. Support this by adding a
> `IoRepr` type to denote the underlying I/O type to use for a specific type.
>
> This makes the `IoLoc::IoType` and `Register::Storage` redundant; thus
> remove them; also convert register methods to use the `read_val` and
> `write_val` instead.

Ah that's nice, I was a bit bothered that `IoType` and `Storage`
basically defined the same thing.

>
> Signed-off-by: Gary Guo <[email protected]>
> ---
>  rust/kernel/bitfield.rs    |   5 ++
>  rust/kernel/io.rs          | 203 ++++++++++++++++++++++++++++++++-------------
>  rust/kernel/io/register.rs |  59 +++++--------
>  3 files changed, 170 insertions(+), 97 deletions(-)
>
> diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs
> index 35ede53f2b8e..4bc92c62da06 100644
> --- a/rust/kernel/bitfield.rs
> +++ b/rust/kernel/bitfield.rs
> @@ -308,6 +308,7 @@ macro_rules! bitfield {
>          $(#[$attr])*
>          #[repr(transparent)]
>          #[derive(Clone, Copy, PartialEq, Eq)]
> +        #[derive($crate::prelude::FromBytes, $crate::prelude::IntoBytes)]
>          $vis struct $name {
>              inner: $storage,
>          }
> @@ -346,6 +347,10 @@ fn from(val: $storage) -> $name {
>                  Self::from_raw(val)
>              }
>          }
> +
> +        impl $crate::io::IoRepr for $name {
> +            type Repr = $storage;
> +        }

This introduces a dependency from `bitfield` to `io`, which looks like
inconsistent layering. But thankfully there is an easy fix - see below.

>      };
>  
>      // Definitions requiring knowledge of individual fields: private and public field accessors,
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index adfc555de7d0..71c6180ed745 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -276,6 +276,91 @@ pub trait IoCapable<T>: IoBackend {
>      fn io_write<'a>(view: Self::View<'a, T>, value: T);
>  }
>  
> +/// Safe transmute that performs size check on monomorphization-time.
> +///
> +/// Can be considered as generic version of [`zerocopy::transmute!`] macro but using the unstable
> +/// `core::mem::transmute_neo` instead of [`core::mem::transmute`].
> +#[inline(always)] // This is a no-op.
> +fn transmute_neo<Src: IntoBytes, Dst: FromBytes>(val: Src) -> Dst {
> +    const_assert!(size_of::<Src>() == size_of::<Dst>());
> +
> +    // SAFETY: `Src: IntoBytes` and `Dst: FromBytes` and we've checked size is the same.
> +    unsafe { core::mem::transmute_copy(&core::mem::ManuallyDrop::new(val)) }
> +}

This is universally useful, so let's move this to the `transmute` module?

> +
> +/// Trait indicating the underlying primitive types to be used for I/O operations.
> +///
> +/// Implementing trait allows arbitrary types to be used for I/O operations, not just raw
> +/// primitives.
> +///
> +/// The layout of the type and the underlying primitive must match; this is enforced via const
> +/// assertions when I/O methods are used, as the type system cannot represent this.
> +/// [`IoRepr::from_repr`] and [`IoRepr::into_expr`] can be overridden for conversions, however it
> +/// should be noted that they are only invoked on value read/write operations and are not invoked
> +/// on byte operations such as [`Io::copy_read`].
> +///
> +/// # Examples
> +///
> +/// ```
> +/// # use kernel::io::*;
> +/// #[repr(transparent)]
> +/// #[derive(FromBytes, IntoBytes)]
> +/// pub struct MyNewType(u32);
> +///
> +/// impl IoRepr for MyNewType {
> +///     type Repr = u32;
> +/// }
> +///
> +/// #[repr(C)]
> +/// pub struct MyStruct {
> +///     raw: u32,
> +///     new_type: MyNewType,
> +/// }
> +///
> +/// # fn test(mmio: Mmio<'_, MyStruct>) {
> +/// // let mmio: Mmio<'_, MyStruct>;
> +/// let val: u32 = io_read!(mmio, .raw); // Raw primitive read
> +/// io_write!(mmio, .raw, val);          // Raw primitve write
> +/// let val: MyNewType = io_read!(mmio, .new_type); // Read via `IoRepr`.
> +/// io_write!(mmio, .new_type, val);                // Write via `IoRepr`.
> +/// # }
> +/// ```
> +pub trait IoRepr: FromBytes + IntoBytes + Sized {
> +    /// The backing I/O capable type.
> +    type Repr: FromBytes + IntoBytes;
> +
> +    /// Convert from [`IoRepr::Repr`] to `Self`.
> +    #[inline(always)]
> +    fn from_repr(repr: Self::Repr) -> Self {
> +        transmute_neo(repr)
> +    }
> +
> +    /// Convert from `Self` to [`IoRepr::Repr`].
> +    #[inline(always)]
> +    fn into_repr(this: Self) -> Self::Repr {
> +        transmute_neo(this)
> +    }
> +}
> +
> +macro_rules! impl_io_repr {
> +    ($($ty:ty => $backing:ty,)*) => {
> +        $(impl IoRepr for $ty {
> +            type Repr = $backing;
> +        })*
> +    };
> +}
> +
> +impl_io_repr! {
> +    u8 => u8,
> +    u16 => u16,
> +    u32 => u32,
> +    u64 => u64,
> +    i8 => u8,
> +    i16 => u16,
> +    i32 => u32,
> +    i64 => u64,
> +}

... and `IoRepr` and its implementations for primitive types should also
be part of `transmute` IMHO (after being renamed to e.g. `Repr`), for
there is nothing I/O exclusive to it. It just indicates that one type
can be represented by another, a property that is again useful outside
of I/O.

That way `bitfield` gets a dependency on `transmute` rather than `io`,
which doesn't break layering.

In order to avoid `Repr::Repr` we can also rename the associated type to
`Raw` and update the method names accordingly to `from_raw`/`into_raw` -
which would have allowed us to remove the `bitfield` methods of the same
name if they weren't needed in const context! But at least it makes
things align nicely.

<...>
> @@ -831,8 +816,8 @@ macro_rules! register {
>              { $($fields:tt)* }
>      ) => {
>          $crate::register!(@bitfield $(#[$attr])* $vis struct $name($storage) { $($fields)* });
> -        $crate::register!(@io_base $name($storage) @ $offset);
> -        $crate::register!(@io_fixed $(#[$attr])* $vis $name($storage));
> +        $crate::register!(@io_base $name @ $offset);
> +        $crate::register!(@io_fixed $(#[$attr])* $vis $name);
>      };
>  
>      // Creates an alias register of fixed offset register `alias` with its own fields.
> @@ -842,10 +827,10 @@ macro_rules! register {
>      ) => {
>          $crate::register!(@bitfield $(#[$attr])* $vis struct $name($storage) { $($fields)* });
>          $crate::register!(
> -            @io_base $name($storage) @
> +            @io_base $name @
>              <$alias as $crate::io::register::Register>::OFFSET
>          );
> -        $crate::register!(@io_fixed $(#[$attr])* $vis $name($storage));
> +        $crate::register!(@io_fixed $(#[$attr])* $vis $name);
>      };
>  
>      // Creates a register at a relative offset from a base address provider.
> @@ -854,8 +839,8 @@ macro_rules! register {
>              { $($fields:tt)* }
>      ) => {
>          $crate::register!(@bitfield $(#[$attr])* $vis struct $name($storage) { $($fields)* });
> -        $crate::register!(@io_base $name($storage) @ $offset);
> -        $crate::register!(@io_relative $vis $name($storage) @ $base);
> +        $crate::register!(@io_base $name @ $offset);
> +        $crate::register!(@io_relative $vis $name @ $base);
>      };
>  
>      // Creates an alias register of relative offset register `alias` with its own fields.
> @@ -865,9 +850,9 @@ macro_rules! register {
>      ) => {
>          $crate::register!(@bitfield $(#[$attr])* $vis struct $name($storage) { $($fields)* });
>          $crate::register!(
> -            @io_base $name($storage) @ <$alias as $crate::io::register::Register>::OFFSET
> +            @io_base $name @ <$alias as $crate::io::register::Register>::OFFSET
>          );
> -        $crate::register!(@io_relative $vis $name($storage) @ $base);
> +        $crate::register!(@io_relative $vis $name @ $base);
>      };
>  
>      // Creates an array of registers at a fixed offset of the MMIO space.
> @@ -878,8 +863,8 @@ macro_rules! register {
>          $crate::build_assert::static_assert!(::core::mem::size_of::<$storage>() <= $stride);
>  
>          $crate::register!(@bitfield $(#[$attr])* $vis struct $name($storage) { $($fields)* });
> -        $crate::register!(@io_base $name($storage) @ $offset);
> -        $crate::register!(@io_array $vis $name($storage) [ $size, stride = $stride ]);
> +        $crate::register!(@io_base $name @ $offset);
> +        $crate::register!(@io_array $vis $name [ $size, stride = $stride ]);
>      };
>  
>      // Shortcut for contiguous array of registers (stride == size of element).
> @@ -904,11 +889,11 @@ macro_rules! register {
>  
>          $crate::register!(@bitfield $(#[$attr])* $vis struct $name($storage) { $($fields)* });
>          $crate::register!(
> -            @io_base $name($storage) @
> +            @io_base $name @
>              <$alias as $crate::io::register::Register>::OFFSET
>                  + $idx * <$alias as $crate::io::register::RegisterArray>::STRIDE
>          );
> -        $crate::register!(@io_fixed $(#[$attr])* $vis $name($storage));
> +        $crate::register!(@io_fixed $(#[$attr])* $vis $name);
>      };
>  
>      // Creates an array of registers at a relative offset from a base address provider.
> @@ -920,9 +905,9 @@ macro_rules! register {
>          $crate::build_assert::static_assert!(::core::mem::size_of::<$storage>() <= $stride);
>  
>          $crate::register!(@bitfield $(#[$attr])* $vis struct $name($storage) { $($fields)* });
> -        $crate::register!(@io_base $name($storage) @ $offset);
> +        $crate::register!(@io_base $name @ $offset);
>          $crate::register!(
> -            @io_relative_array $vis $name($storage) [ $size, stride = $stride ] @ $base + $offset
> +            @io_relative_array $vis $name [ $size, stride = $stride ] @ $base + $offset
>          );
>      };
>  
> @@ -949,11 +934,11 @@ macro_rules! register {
>  
>          $crate::register!(@bitfield $(#[$attr])* $vis struct $name($storage) { $($fields)* });
>          $crate::register!(
> -            @io_base $name($storage) @
> +            @io_base $name @
>                  <$alias as $crate::io::register::Register>::OFFSET +
>                  $idx * <$alias as $crate::io::register::RegisterArray>::STRIDE
>          );
> -        $crate::register!(@io_relative $vis $name($storage) @ $base);
> +        $crate::register!(@io_relative $vis $name @ $base);
>      };
>  
>      // Generates the bitfield for the register.
> @@ -970,16 +955,14 @@ macro_rules! register {
>      };
>  
>      // Implementations shared by all registers types.
> -    (@io_base $name:ident($storage:ty) @ $offset:expr) => {
> +    (@io_base $name:ident @ $offset:expr) => {
>          impl $crate::io::register::Register for $name {
> -            type Storage = $storage;
> -
>              const OFFSET: usize = $offset;
>          }
>      };
>  
>      // Implementations of fixed registers.
> -    (@io_fixed $(#[$attr:meta])* $vis:vis $name:ident ($storage:ty)) => {
> +    (@io_fixed $(#[$attr:meta])* $vis:vis $name:ident) => {
>          impl $crate::io::register::FixedRegister for $name {}
>  
>          $(#[$attr])*
> @@ -988,7 +971,7 @@ impl $crate::io::register::FixedRegister for $name {}
>      };
>  
>      // Implementations of relative registers.
> -    (@io_relative $vis:vis $name:ident ($storage:ty) @ $base:ident) => {
> +    (@io_relative $vis:vis $name:ident @ $base:ident) => {
>          impl $crate::io::register::WithBase for $name {
>              type BaseFamily = $base;
>          }
> @@ -997,7 +980,7 @@ impl $crate::io::register::RelativeRegister for $name {}
>      };
>  
>      // Implementations of register arrays.
> -    (@io_array $vis:vis $name:ident ($storage:ty) [ $size:expr, stride = $stride:expr ]) => {
> +    (@io_array $vis:vis $name:ident [ $size:expr, stride = $stride:expr ]) => {
>          impl $crate::io::register::Array for $name {}
>  
>          impl $crate::io::register::RegisterArray for $name {
> @@ -1008,7 +991,7 @@ impl $crate::io::register::RegisterArray for $name {
>  
>      // Implementations of relative array registers.
>      (
> -        @io_relative_array $vis:vis $name:ident ($storage:ty) [ $size:expr, stride = $stride:expr ]
> +        @io_relative_array $vis:vis $name:ident [ $size:expr, stride = $stride:expr ]
>              @ $base:ident + $offset:literal
>      ) => {
>          impl $crate::io::register::WithBase for $name {

These appear to repeat quite a bit of [1] which is already in
`driver-core-next`. You'll want to rebase, I think the only difference
between the two is the removal of $storage from @io_base.

[1] https://lore.kernel.org/all/[email protected]/
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.