Re: [PATCH v2 1/8] rust: pci: add {enable,disable}_sriov(), to control SR-IOV capability

["Danilo Krummrich" <dakr@kernel.org>]

On Thu Sep 24, 2026 at 9:05 PM CEST, Zhi Wang wrote:
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 3ec897709e89..e6dac919f02d 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -133,6 +133,10 @@ extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) {
>          // INVARIANT: `pdev` is valid for the duration of `remove_callback()`.
>          let pdev = unsafe { &*pdev.cast::<Device<device::CoreInternal<'_>>>() };
>  
> +        // Keep PF data installed until all VF remove callbacks have completed.
> +        #[cfg(CONFIG_PCI_IOV)]
> +        pdev.disable_sriov();

I don't think we need this? The VfRegistration should guard against this
already.

> +
>          // SAFETY: `remove_callback` is only ever called after a successful call to
>          // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
>          // and stored a `Pin<KBox<T::Data<'_>>>`.
> @@ -472,6 +476,38 @@ pub fn set_master(&self) {
>          // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
>          unsafe { bindings::pci_set_master(self.as_raw()) };
>      }
> +
> +    /// Enable the Single Root I/O Virtualization (SR-IOV) capability for this device,
> +    /// where `nr_virtfn` is number of Virtual Functions (VF) to enable.
> +    #[cfg(CONFIG_PCI_IOV)]
> +    pub fn enable_sriov(&self, nr_virtfn: i32) -> Result {
> +        // SAFETY:
> +        // `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> +        //
> +        // `pci_enable_sriov()` checks that the enable operation is valid:
> +        // - the device is a Physical Function (PF),
> +        // - SR-IOV is currently disabled, and
> +        // - `nr_virtfn` does not exceed the total number of supported VFs.
> +        //
> +        // The Core device context inherits from the Bound device context,
> +        // which guarantees that the PF device is bound to a driver.
> +        to_result(unsafe { bindings::pci_enable_sriov(self.as_raw(), nr_virtfn) })
> +    }
> +
> +    /// Disable the Single Root I/O Virtualization (SR-IOV) capability for this device.
> +    #[cfg(CONFIG_PCI_IOV)]
> +    pub fn disable_sriov(&self) {
> +        // SAFETY:
> +        // `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> +        //
> +        // `pci_disable_sriov()` checks that the disable operation is valid:
> +        // - the device is a Physical Function (PF), and
> +        // - SR-IOV is currently enabled.
> +        //
> +        // The Core device context inherits from the Bound device context,
> +        // which guarantees that the PF device is bound to a driver.
> +        unsafe { bindings::pci_disable_sriov(self.as_raw()) };
> +    }
>  }

I think we do not need to expose those as functions on Device<Core>. We should
only need to call those from the sriov_configure() callback and should otherwise
be covered by the VfRegistration.

Hence, I suggest to expose those methods via a token type, which also helps to
use an RAII patterns for cleanup rather than manual enable/disable calls:

Instead of a single sriov_configure() callback that covers both cases, we can
have two callbacks.

	fn sriov_enable<'bound>(
	    dev: &'bound Device<device::Core<'_>>,
	    data: Pin<&'bound Self::Data<'bound>>,
	    token: SriovEnable<'_>,
	) -> Result<SriovEnabled<'_>>;

and

	fn sriov_disable<'bound>(
	    dev: &'a Device<device::Core<'_>>,
	    data: Pin<&'bound Self::Data<'bound>>,
	    token: SriovDisable<'_>,
	) -> Result;

Note the return type on sriov_enable(), which is obtained from token.enable().
It can serve as guard and automatically disable again of sriov_enable() fails
subsequently and returns an error. If it successfully returns SriovEnabled, the
PCI core can just discard it. The structs could look like this:

	pub struct SriovEnable<'a> {
	    dev: &'a Device<device::CoreInternal<'a>>,
	    num_vfs: u32,
	}
	
	impl<'a> SriovEnable<'a> {
	    pub fn num_vfs(&self) -> u32 {
	        self.num_vfs
	    }
	
	    pub fn enable(self, num_vfs: u32) -> Result<SriovEnabled<'a>> {
	        let ret = unsafe {
	            bindings::pci_enable_sriov(self.dev.as_raw(), num_vfs)
	        };
	        to_result(ret)?;
	
	        Ok(SriovEnabled { dev: self.dev, num_vfs })
	    }
	}

And the destructor of SriovEnabled could be:

	impl Drop for SriovEnabled<'_> {
	    fn drop(&mut self) {
	        unsafe { bindings::pci_disable_sriov(self.dev.as_raw()) }
	    }
	}

We could still have the helpers on Device<CoreInternal> so you can use them
safely in the guard types.