`enable_device_mem()` wraps the unmanaged `pci_enable_device_mem()` and has no disable counterpart, so the enable count is leaked on driver unbind and the device does not come back up on a subsequent probe.
Replace it with `enable_device()`, which wraps `pci_enable_device()` and returns a `DeviceEnableGuard<'a>`: dropping the guard runs `pci_disable_device()`. The guard borrows the device's bound scope (`&'a Device<Bound>`), so it cannot outlive the driver binding, and since it is the only way to enable the device from safe code, the enable count always stays balanced. Obtaining the guard still requires a `&Device<Core>`, i.e. a bus callback. Unlike `pci_enable_device_mem()`, `pci_enable_device()` enables I/O and memory resources. Convert nova-core, the only user of `enable_device_mem()`, storing the guard as the last field of `NovaCore` so the device is disabled only after the GPU teardown. Link: https://lore.kernel.org/rust-for-linux/[email protected] Suggested-by: Danilo Krummrich <[email protected]> Signed-off-by: Maurice Hieronymus <[email protected]> --- drivers/gpu/nova-core/driver.rs | 5 ++++- rust/kernel/pci.rs | 31 ++++++++++++++++++++++++++++--- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs index 5738d4ac521b..99b15da59e81 100644 --- a/drivers/gpu/nova-core/driver.rs +++ b/drivers/gpu/nova-core/driver.rs @@ -30,6 +30,8 @@ pub(crate) struct NovaCore<'bound> { bar: pci::Bar<'bound, BAR0_SIZE>, #[allow(clippy::type_complexity)] _reg: auxiliary::Registration<'bound, ForLt!(())>, + // Declared last so the device stays enabled until everything above is torn down. + _enable: pci::DeviceEnableGuard<'bound>, } pub(crate) struct NovaCoreDriver; @@ -75,7 +77,7 @@ fn probe<'bound>( pin_init::pin_init_scope(move || { dev_dbg!(pdev, "Probe Nova Core GPU driver.\n"); - pdev.enable_device_mem()?; + let enable = pdev.enable_device()?; pdev.set_master(); Ok(try_pin_init!(NovaCore { @@ -95,6 +97,7 @@ fn probe<'bound>( crate::MODULE_NAME, (), )?, + _enable: enable, })) }) } diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs index 4def9ca1824c..bd9a8113af35 100644 --- a/rust/kernel/pci.rs +++ b/rust/kernel/pci.rs @@ -452,11 +452,36 @@ pub fn pci_class(&self) -> Class { } } +/// A guard that keeps the device's I/O and memory resources enabled. +/// +/// # Invariants +/// +/// The device's enable count was incremented once for this guard; dropping the guard decrements +/// it again. +pub struct DeviceEnableGuard<'a> { + dev: &'a Device<device::Bound>, +} + +impl Drop for DeviceEnableGuard<'_> { + fn drop(&mut self) { + // SAFETY: `self.dev.as_raw()` is a valid pointer to a `struct pci_dev`, and by the type + // invariant this guard holds one increment of the device's enable count. + unsafe { bindings::pci_disable_device(self.dev.as_raw()) }; + } +} + impl<'a> Device<device::Core<'a>> { - /// Enable memory resources for this device. - pub fn enable_device_mem(&self) -> Result { + /// Enable I/O and memory resources for this device. + /// + /// The device stays enabled for the lifetime of the returned guard; dropping the guard + /// disables the device again. The guard borrows the device's bound scope, so it cannot + /// outlive the driver binding. + pub fn enable_device(&self) -> Result<DeviceEnableGuard<'_>> { // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`. - to_result(unsafe { bindings::pci_enable_device_mem(self.as_raw()) }) + to_result(unsafe { bindings::pci_enable_device(self.as_raw()) })?; + + // INVARIANT: `pci_enable_device()` succeeded, so the enable count was incremented once. + Ok(DeviceEnableGuard { dev: self }) } /// Enable bus-mastering for this device. -- 2.54.0
