[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANiq72kObXP7-YtcXnWQXJpEQ=N+RtcSeM6A+scBK00VkFj5JA@mail.gmail.com>
Date: Tue, 21 May 2024 01:27:23 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: pstanner@...hat.com, Danilo Krummrich <dakr@...hat.com>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com,
ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com,
boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
benno.lossin@...ton.me, a.hindborg@...sung.com, aliceryhl@...gle.com,
airlied@...il.com, fujita.tomonori@...il.com, lina@...hilina.net,
ajanulgu@...hat.com, lyude@...hat.com, rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [RFC PATCH 11/11] rust: PCI: add BAR request and ioremap
Hi Philipp,
A few quick nits I noticed for this one...
On Mon, May 20, 2024 at 7:27 PM Danilo Krummrich <dakr@...hat.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 = num as i32;
Would it make sense to use newtypes for `num`/`barnr`, perhaps?
> + let barlen = pdev.resource_len(num)?;
> + if barlen == 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 = unsafe { bindings::pci_iomap(pdev.as_raw(), barnr, 0) } as usize;
Is the type needed, since there is an explicit cast?
> + if ioptr == 0 {
> + // SAFETY:
> + // `pdev` is always valid.
> + // `barnr` is checked for validity at the top of the function.
> + 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 most
> + // PCI_NUM_RESOURCES entries.
Missing Markdown. There are other instances as well.
> + if i as i32 >= 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_size_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
Powered by blists - more mailing lists