Received-SPF: pass (google.com: domain of linux-kernel+bounces-184219-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240520172554.182094-1-dakr@redhat.com> <20240520172554.182094-11-dakr@redhat.com>
In-Reply-To: <20240520172554.182094-11-dakr@redhat.com>
From: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Date: Tue, 21 May 2024 00:32:02 +0200
Message-ID: <CANiq72kHrgOVrdw7rB9KpHvOMy244TgmEzAcL=v5O9rchs8T1g@mail.gmail.com>
Subject: Re: [RFC PATCH 10/11] rust: add basic abstractions for iomem operations
To: Danilo Krummrich <dakr@redhat.com>
Cc: gregkh@linuxfoundation.org, rafael@kernel.org, bhelgaas@google.com, 
	ojeda@kernel.org, alex.gaynor@gmail.com, wedsonaf@gmail.com, 
	boqun.feng@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com, 
	benno.lossin@proton.me, a.hindborg@samsung.com, aliceryhl@google.com, 
	airlied@gmail.com, fujita.tomonori@gmail.com, lina@asahilina.net, 
	pstanner@redhat.com, ajanulgu@redhat.com, lyude@redhat.com, 
	rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-pci@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, May 20, 2024 at 7:27=E2=80=AFPM Danilo Krummrich <dakr@redhat.com> =
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 =3D 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