Hi,
> On 28 Oct 2024, at 12:43, Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
>
> On Tue, Oct 22, 2024 at 11:33 PM Danilo Krummrich <dakr@xxxxxxxxxx> 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.
>
> And using zero works because the user then statically knows that zero
> bytes are available ... ?
>
>> `Io` 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
>> `Io` accordingly.
>>
>> To ensure that I/O mapped memory can't out-live the device it may be
>> bound to, subsystems should embedd the corresponding I/O memory type
>> (e.g. pci::Bar) into a `Devres` container, such that it gets revoked
>> once the device is unbound.
>
> I wonder if `Io` should be a reference type instead. That is:
>
> struct Io<'a, const SIZE: usize> {
> addr: usize,
> maxsize: usize,
> _lifetime: PhantomData<&'a ()>,
> }
>
> and then the constructor requires "addr must be valid I/O mapped
> memory for maxsize bytes for the duration of 'a". And instead of
> embedding it in another struct, the other struct creates an `Io` on
> each access?
Please let’s not add a lifetime here. Drivers will usually have this mapped
at probe time and it will usually remain mapped until remove(), so I think this
works fine the way it is.
>
>> Co-developed-by: Philipp Stanner <pstanner@xxxxxxxxxx>
>> Signed-off-by: Philipp Stanner <pstanner@xxxxxxxxxx>
>> Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
>
> [...]
>
>> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
>> new file mode 100644
>> index 000000000000..750af938f83e
>> --- /dev/null
>> +++ b/rust/kernel/io.rs
>> @@ -0,0 +1,234 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Memory-mapped IO.
>> +//!
>> +//! C header: [`include/asm-generic/io.h`](srctree/include/asm-generic/io.h)
>> +
>> +use crate::error::{code::EINVAL, Result};
>> +use crate::{bindings, build_assert};
>> +
>> +/// IO-mapped memory, starting at the base address @addr and spanning @maxlen bytes.
>> +///
>> +/// The creator (usually a subsystem / bus such as PCI) is responsible for creating the
>> +/// mapping, performing an additional region request etc.
>> +///
>> +/// # Invariant
>> +///
>> +/// `addr` is the start and `maxsize` the length of valid I/O mapped memory region of size
>> +/// `maxsize`.
>
> Do you not also need an invariant that `SIZE <= maxsize`?
>
> Alice
