(quick reply from my phone)
> On 21. May 2024, at 00:32, Miguel Ojeda <miguel.ojeda.sandonis@xxxxxxxxx> wrote:
>
> On Mon, May 20, 2024 at 7:27 PM Danilo Krummrich <dakr@xxxxxxxxxx> wrote:
>>
>> through its Drop() implementation.
>
> Nit: `Drop`, `Deref` and so on are traits -- what do the `()` mean
> here? I guess you may be referring to their method, but those are
> lowercase.
>
>> +/// IO-mapped memory, starting at the base pointer @ioptr and spanning @malxen bytes.
>
> Please use Markdown code spans instead (and intra-doc links where
> possible) -- we don't use the `@` notation. There is a typo on the
> variable name too.
>
>> +pub struct IoMem {
>> + pub ioptr: usize,
>
> This field is public, which raises some questions...
>
>> + pub fn readb(&self, offset: usize) -> Result<u8> {
>> + let ioptr: usize = self.get_io_addr(offset, 1)?;
>> +
>> + Ok(unsafe { bindings::readb(ioptr as _) })
>> + }
>
> These methods are unsound, since `ioptr` may end up being anything
> here, given `self.ioptr` it is controlled by the caller. One could
> also trigger an overflow in `get_io_addr`.
>
I think the only thing we really want from IoMem is a generic implementation of the read*() and write*() functions.
Everything else should be up the the resource itself, see e.g. pci::Bar. Hence I think we should just make IoMem a trait instead and just let the resource implement a getter for the MMIO pointer, etc.
> Wedson wrote a similar abstraction in the past
> (`rust/kernel/io_mem.rs` in the old `rust` branch), with a
> compile-time `SIZE` -- it is probably worth taking a look.
>
> Also, there are missing `// SAFETY:` comments here. Documentation and
> examples would also be nice to have.
>
> Thanks!
>
> Cheers,
> Miguel
>
