On Tue, 2024-05-21 at 00:32 +0200, Miguel Ojeda 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.
ACK
>
> > +/// 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...
Justified questions – it is public because the Drop implementation for
pci::Bar requires the ioptr to pass it to pci_iounmap().
The alternative would be to give pci::Bar a copy of ioptr (it's just an
integer after all), but that would also not be exactly beautiful.
The subsystem (as PCI does here) shall not make an instance of IoMem
mutable, so the driver programmer couldn't modify ioptr.
I'm very open for ideas for alternatives, though. See also the other
mail where Danilo brainstorms about making IoMem a trait.
>
> > + 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.
Only if IoMem is mutable, correct?
The commit message states (btw this file would get more extensive
comments soonish) that with this design its the subsystem that is
responsible for creating IoMem validly, because the subsystem is the
one who knows about the memory regions and lengths and stuff.
The driver should only ever take an IoMem through a subsystem, so that
would be safe.
> One could
> also trigger an overflow in `get_io_addr`.
Yes, if the addition violates the capacity of a usize. But that would
then be a bug we really want to notice, wouldn't we?
Only alternative I can think of would be to do a wrapping_add(), but
that would be even worse UB.
Ideas?
>
> 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.
Yes, we're aware of that one. We also did some experiments with it.
Will discuss it in the other thread where Dave and Wedson mention it.
>
> Also, there are missing `// SAFETY:` comments here. Documentation and
> examples would also be nice to have.
Oh yes, ACK, will do
Thx for the review!
>
> Thanks!
>
> Cheers,
> Miguel
>
