Re: [PATCH v2 7/8] rust: pci: add typed SR-IOV PF registration data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] rust: pci: missing #[inline] on C-binding forwarders and small accessors
--

commit f12a8908c8b58a535a5ede56fdb4d45436a0f800
Author: Zhi Wang <zhiw@nvidia.com>

    rust: pci: add typed SR-IOV PF registration data

This commit adds VfRegistration to register a pinned Rust object in the PF's
driver data. It provides accessors for VF drivers to borrow this object after
checking the requested Rust type.

> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index cb3ed2207075e..831b76744a30a 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs

[ ... ]

>      /// Returns the number of Virtual Functions (VF) enabled for a Physical Function (PF).
>      #[cfg(CONFIG_PCI_IOV)]
> -    pub(crate) fn num_vf(&self) -> i32 {
> +    pub fn num_vf(&self) -> i32 {

[Severity: Low]
Should this function have an #[inline] attribute?

According to the Rust subsystem guidelines, functions in the kernel
abstraction crate that are small or forward to a C binding call should be
annotated with #[inline] to allow the optimizer to inline them across crate
boundaries.

>          // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
>          unsafe { bindings::pci_num_vf(self.as_raw()) }
>      }

> diff --git a/rust/kernel/pci/sriov.rs b/rust/kernel/pci/sriov.rs
> new file mode 100644
> index 0000000000000..90abe878f67c5
> --- /dev/null
> +++ b/rust/kernel/pci/sriov.rs

[ ... ]

> +impl<Ctx: device::DeviceContext> PciDevice<Ctx> {
> +    /// Returns the raw `vf_registration_data_rust` pointer from this device.
> +    fn vf_registration_data_rust(&self) -> *mut core::ffi::c_void {

[Severity: Low]
Would it be appropriate to mark this small, trivial accessor with #[inline]?

> +        // SAFETY: `self.as_raw()` is valid.
> +        unsafe { (*self.as_raw()).vf_registration_data_rust }
> +    }
> +
> +    /// Sets the `vf_registration_data_rust` pointer on this device.
> +    fn set_vf_registration_data_rust(&self, ptr: *mut core::ffi::c_void) {

[Severity: Low]
Could this trivial setter also benefit from an #[inline] annotation?

> +        // SAFETY: `self.as_raw()` is valid. PCI probe publishes the data before enabling VFs;
> +        // teardown removes all VFs before withdrawing it.
> +        unsafe { (*self.as_raw()).vf_registration_data_rust = ptr };
> +    }
> +}
> +
> +impl PciDevice<device::Bound> {
> +    /// Returns the PF for this VF, or [`ENODEV`] if this is not a VF.
> +    fn physfn(&self) -> Result<&PciDevice> {

[Severity: Low]
Should #[inline] be added here?

This is a short function reading a C struct field within the abstraction crate.

> +        if !self.is_virtfn() {
> +            return Err(ENODEV);
> +        }

[ ... ]

> +    pub fn vf_registration_data_with<F: ForLt + 'static, R>(
> +        &self,
> +        f: impl for<'borrow, 'data> FnOnce(Pin<&'borrow F::Of<'data>>) -> R,
> +    ) -> Result<R> {

[Severity: Low]
Is it worth annotating this forwarding function with #[inline]?

> +        // SAFETY: The higher-ranked closure prevents the borrow from escaping or being stored in
> +        // invariant data by keeping its lifetime independent of the erased data lifetime.
> +        let pinned = unsafe { self.vf_registration_data_pinned::<F>()? };
> +        Ok(f(pinned))
> +    }
> +
> +    /// Returns a pinned reference to the VF registration data.
> +    ///
> +    /// Available only when `F` implements [`CovariantForLt`](trait@crate::types::CovariantForLt),
> +    /// guaranteeing that shortening the PF data lifetime is sound.
> +    ///
> +    /// For non-covariant types, use [`Self::vf_registration_data_with()`].
> +    ///
> +    /// It returns the same errors as [`Self::vf_registration_data_with()`].
> +    pub fn vf_registration_data<F: CovariantForLt + 'static>(&self) -> Result<Pin<&F::Of<'_>>> {

[Severity: Low]
Could this 1-line wrapper that delegates to vf_registration_data_pinned
also use #[inline]?

> +        // SAFETY: `CovariantForLt` permits shortening the encoded lifetime to this borrow.
> +        unsafe { self.vf_registration_data_pinned::<F>() }
> +    }
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924190556.1620886-1-zhiw@nvidia.com?part=7