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

[Zhi Wang <zhiw@nvidia.com>]

On Sun, 27 Sep 2026 18:37:06 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:

> 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.
> 

Nice catch. This was already handled in the VfRegistration.

> > +
> >          // 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()) }; }
> > +

snip

> >  }  
> 
> 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:
> 

I see. With this, if something unexpected happens during the
sriov_enable callback in driver setup, the sriov_disable can be
automatically called then. Let me slightly change this patch to expose
the functions in Device<CoreInternal> and add a new patch for the below
idea.

Z.

> 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.

Got it.