Re: [PATCH v4 06/13] rust: add `io::{Io, IoRaw}` base types

Alice Ryhl
2024-12-06T14:13:27Z

On Thu, Dec 5, 2024 at 3:16 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> I/O memory is typically either mapped through direct calls to ioremap()
> or subsystem / bus specific ones such as pci_iomap().
>
> Even though subsystem / bus specific functions to map I/O memory are
> based on ioremap() / iounmap() it is not desirable to re-implement them
> in Rust.
>
> Instead, implement a base type for I/O mapped memory, which generically
> provides the corresponding accessors, such as `Io::readb` or
> `Io:try_readb`.
>
> `Io` supports an optional const generic, such that a driver can indicate
> the minimal expected and required size of the mapping at compile time.
> Correspondingly, calls to the 'non-try' accessors, support compile time
> checks of the I/O memory offset to read / write, while the 'try'
> accessors, provide boundary checks on runtime.
>
> `IoRaw` is meant to be embedded into a structure (e.g. pci::Bar or
> io::IoMem) which creates the actual I/O memory mapping and initializes
> `IoRaw` accordingly.
>
> To ensure that I/O mapped memory can't out-live the device it may be
> bound to, subsystems must embed the corresponding I/O memory type (e.g.
> pci::Bar) into a `Devres` container, such that it gets revoked once the
> device is unbound.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

One nit below. With that addressed:

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

> +impl<const SIZE: usize> Io<SIZE> {
> + /// Converts an `IoRaw` into an `Io` instance, providing the accessors to the MMIO mapping.
> + ///
> + /// # Safety
> + ///
> + /// Callers must ensure that `addr` is the start of a valid I/O mapped memory region of size
> + /// `maxsize`.
> + pub unsafe fn from_raw<'a>(raw: &IoRaw<SIZE>) -> &'a Self {

I would use this signature:

pub unsafe fn from_raw(raw: &IoRaw<SIZE>) -> &Self;

Otherwise, you're saying that the returned reference is allowed to
outlive the IoRaw instance, which wouldn't be okay.

Alice