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
