Received-SPF: pass (google.com: domain of linux-kernel+bounces-184297-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240520172554.182094-1-dakr@redhat.com> <20240520172554.182094-12-dakr@redhat.com>
In-Reply-To: <20240520172554.182094-12-dakr@redhat.com>
From: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Date: Tue, 21 May 2024 01:27:23 +0200
Message-ID: <CANiq72kObXP7-YtcXnWQXJpEQ=N+RtcSeM6A+scBK00VkFj5JA@mail.gmail.com>
Subject: Re: [RFC PATCH 11/11] rust: PCI: add BAR request and ioremap
To: pstanner@redhat.com, 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, 
	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

Hi Philipp,

A few quick nits I noticed for this one...

On Mon, May 20, 2024 at 7:27=E2=80=AFPM Danilo Krummrich <dakr@redhat.com> =
wrote:
>
> +/// A PCI BAR to perform IO-Operations on.

Some more documentation, examples, and/or references would be nice.

> +pub struct Bar {
> +    pdev: Device,
> +    iomem: IoMem,
> +    num: u8,
> +}
> +
> +impl Bar {
> +    fn new(pdev: Device, num: u8, name: &CStr) -> Result<Self> {
> +        let barnr =3D num as i32;

Would it make sense to use newtypes for `num`/`barnr`, perhaps?

> +        let barlen =3D pdev.resource_len(num)?;
> +        if barlen =3D=3D 0 {
> +            return Err(ENOMEM);
> +        }
> +
> +        // SAFETY:
> +        // `pdev` is always valid.

Please explain why it is always valid -- the point of a `SAFETY`
comment is not to say something is OK, but why it is so.

> +        // `barnr` is checked for validity at the top of the function.

Where was it checked? I may be missing something, but I only see a
widening cast.

> +        // SAFETY:
> +        // `pdev` is always valid.
> +        // `barnr` is checked for validity at the top of the function.
> +        // `name` is always valid.

Please use the convention we have elsewhere for this kind of lists,
i.e. use a list with the `-` bullet list marker.

> +        let ioptr: usize =3D unsafe { bindings::pci_iomap(pdev.as_raw(),=
 barnr, 0) } as usize;

Is the type needed, since there is an explicit cast?

> +        if ioptr =3D=3D 0 {
> +            // SAFETY:
> +            // `pdev` is always valid.
> +            // `barnr` is checked for validity at the top of the functio=
n.
> +            unsafe { bindings::pci_release_region(pdev.as_raw(), barnr) =
};
> +            return Err(ENOMEM);
> +        }

Should the region be managed in a RAII type instead?

> +    fn index_is_valid(i: u8) -> bool {
> +        // A pci_dev on the C side owns an array of resources with at mo=
st
> +        // PCI_NUM_RESOURCES entries.

Missing Markdown. There are other instances as well.

> +        if i as i32 >=3D bindings::PCI_NUM_RESOURCES as i32 {
> +            return false;
> +        }
> +
> +        true

The body of the function could just be `... < ...`, i.e. no `if` needed.

> +    // SAFETY: The caller should ensure that `ioptr` is valid.
> +    unsafe fn do_release(pdev: &Device, ioptr: usize, num: u8) {

This should not be a comment but documentation, and it should be a `#
Safety` section, not a `// SAFETY:` comment.

> +    /// Returns the size of the given PCI bar resource.
> +    pub fn resource_len(&self, bar: u8) -> Result<bindings::resource_siz=
e_t> {

Sometimes `bindings::` in signatures of public methods may be
justified -- is it the case here? Or should a newtype or similar be
provided instead? I only see this function being called inside the
module, anyway.

> +    /// Mapps an entire PCI-BAR after performing a region-request on it.

Typo.

Cheers,
Miguel