Re: [PATCH v5 09/20] rust: io: use view types instead of addresses for `Io`

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The abstraction methods `io_read` and `io_write` for `ConfigSpaceBackend` are missing the `#[inline]` annotation.
--

commit 4b02e1c6f87f5cb689ea18ffd09a2fb450c97ee7
Author: Gary Guo <gary@garyguo.net>
Subject: rust: io: use view types instead of addresses for Io

This commit introduces a new IoBackend trait to avoid monomorphization
duplication and defines operations on it using view types instead of
addresses. IoCapable implementations are moved to backends, and io_read
and io_write are made safe by relying on view invariants.

> diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
> index 89f4bb483a7f6..e67c1e3694fb8 100644
> --- a/rust/kernel/pci/io.rs
> +++ b/rust/kernel/pci/io.rs

[ ... ]

> @@ -112,25 +138,24 @@ unsafe fn io_write(self, value: $ty, address: usize) {
>  /// Implements [`IoCapable`] on [`ConfigSpace`] for `$ty` using `$read_fn` and `$write_fn`.
>  macro_rules! impl_config_space_io_capable {
>      ($ty:ty, $read_fn:ident, $write_fn:ident) => {
> -        impl<'a, T: ?Sized> IoCapable<$ty> for ConfigSpace<'a, T> {
> -            unsafe fn io_read(self, address: usize) -> $ty {
> +        impl IoCapable<$ty> for ConfigSpaceBackend {
> +            fn io_read(view: ConfigSpace<'_, $ty>) -> $ty {

[Severity: Low]
this isn't a bug, but should this small forwarding function be annotated
with #[inline]? The Rust subsystem guidelines state that functions in
abstractions that are small or forwarding to a binding call should be
annotated with #[inline].

> +                // CAST: The offset is cast to `i32` because the C functions expect a 32-bit
> +                // signed offset parameter. PCI configuration space size is at most 4096 bytes,
> +                // so the value always fits within `i32` without truncation or sign change.
> +                let addr = view.ptr.addr() as i32;
> +
>                  let mut val: $ty = 0;
>  
>                  // Return value from C function is ignored in infallible accessors.
> -                let _ret =
> -                    // SAFETY: By the type invariant `self.pdev` is a valid address.
> -                    // CAST: The offset is cast to `i32` because the C functions expect a 32-bit
> -                    // signed offset parameter. PCI configuration space size is at most 4096 bytes,
> -                    // so the value always fits within `i32` without truncation or sign change.
> -                    unsafe { bindings::$read_fn(self.pdev.as_raw(), address as i32, &mut val) };
> -
> +                // SAFETY: By the type invariant `pdev` is a valid address.
> +                let _ = unsafe { bindings::$read_fn(view.pdev.as_raw(), addr, &mut val) };
>                  val
>              }
>  
> -            unsafe fn io_write(self, value: $ty, address: usize) {
> +            fn io_write(view: ConfigSpace<'_, $ty>, value: $ty) {

[Severity: Low]
this isn't a bug, but should this function also have the #[inline] annotation?

> +                // CAST: The offset is cast to `i32` because the C functions expect a 32-bit
> +                // signed offset parameter. PCI configuration space size is at most 4096 bytes,
> +                // so the value always fits within `i32` without truncation or sign change.
> +                let addr = view.ptr.addr() as i32;
> +
>                  // Return value from C function is ignored in infallible accessors.
> -                let _ret =
> -                    // SAFETY: By the type invariant `self.pdev` is a valid address.
> -                    // CAST: The offset is cast to `i32` because the C functions expect a 32-bit
> -                    // signed offset parameter. PCI configuration space size is at most 4096 bytes,
> -                    // so the value always fits within `i32` without truncation or sign change.
> -                    unsafe { bindings::$write_fn(self.pdev.as_raw(), address as i32, value) };
> +                // SAFETY: By the type invariant `pdev` is a valid address.
> +                let _ = unsafe { bindings::$write_fn(view.pdev.as_raw(), addr, value) };
>              }
>          }
>      };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626-io_projection-v5-0-d0961471ae50@garyguo.net?part=9