Re: [RFC PATCH 11/11] rust: PCI: add BAR request and ioremap

[Miguel Ojeda]

Hi Philipp,

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

On Mon, May 20, 2024 at 7:27 PM Danilo Krummrich <dakr@xxxxxxxxxx> 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