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`.
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
