[PATCH] rust: pci: rework device enabling API

[PATCH] rust: pci: rework device enabling API

[Maurice Hieronymus]

`enable_device_mem()` wraps the unmanaged `pci_enable_device_mem()` and has
no disable counterpart, so the enable count is leaked on driver unbind.

Replace it with:

- `enable_device_managed()`, wrapping `pcim_enable_device()`, which
  registers a `pci_disable_device()` cleanup that runs on unbind; this
  is what drivers should normally call in `probe()`.

- an unmanaged `enable_device()`/`disable_device()` pair, wrapping
  `pci_enable_device()`/`pci_disable_device()`, e.g. for runtime PM
  paths.

Unlike `pci_enable_device_mem()`, both variants enable I/O and memory
resources.

Convert nova-core and the rust_driver_pci sample, the only users of
`enable_device_mem()`, to `enable_device_managed()`; besides gaining the
automatic cleanup they now also enables I/O resources.

This is a prerequisite for the EDU PCI sample driver series [1], as
requested by Danilo in its review [2]; that series will be rebased on
top of this patch.

Link: https://lore.kernel.org/r/20260620-b4-rust-pci-edu-driver-v2-0-6fd6684f2c14@mailbox.org [1]
Link: https://lore.kernel.org/rust-for-linux/DJEPMYC7J6ZG.D0ODUD4YGFCT@kernel.org/ [2]
Suggested-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Maurice Hieronymus <mhi@mailbox.org>
---
Replace pci::Device::enable_device_mem() with a managed
enable_device_managed() plus an unmanaged enable_device()/
disable_device() pair, and convert the two users (nova-core and the
rust_driver_pci sample).

Requested by Danilo [2] while reviewing the EDU PCI sample driver
series [1]; that series will be rebased on top of this one.

[1] https://lore.kernel.org/r/20260620-b4-rust-pci-edu-driver-v2-0-6fd6684f2c14@mailbox.org
[2] https://lore.kernel.org/rust-for-linux/DJEPMYC7J6ZG.D0ODUD4YGFCT@kernel.org
---
 drivers/gpu/nova-core/driver.rs |  2 +-
 rust/kernel/pci.rs              | 26 +++++++++++++++++++++++---
 samples/rust/rust_driver_pci.rs |  2 +-
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
index 5738d4ac521b..b0711d98033b 100644
--- a/drivers/gpu/nova-core/driver.rs
+++ b/drivers/gpu/nova-core/driver.rs
@@ -75,7 +75,7 @@ fn probe<'bound>(
         pin_init::pin_init_scope(move || {
             dev_dbg!(pdev, "Probe Nova Core GPU driver.\n");
 
-            pdev.enable_device_mem()?;
+            pdev.enable_device_managed()?;
             pdev.set_master();
 
             Ok(try_pin_init!(NovaCore {
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 5071cae6543f..a408b3412077 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -453,10 +453,30 @@ pub fn pci_class(&self) -> Class {
 }
 
 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.
+    ///
+    /// This function is unmanaged and does not perform any cleanup when the device is unbound.
+    /// For a managed function take a look at [`Device::enable_device_managed`].
+    #[inline]
+    pub fn enable_device(&self) -> Result {
+        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
+        to_result(unsafe { bindings::pci_enable_device(self.as_raw()) })
+    }
+
+    /// Disable I/O and memory resources for this device.
+    ///
+    /// This function is the counterpart to [`Device::enable_device`].
+    #[inline]
+    pub fn disable_device(&self) {
+        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
+        unsafe { bindings::pci_disable_device(self.as_raw()) }
+    }
+
+    /// Enable I/O and memory resources for this device, with automatic cleanup.
+    #[inline]
+    pub fn enable_device_managed(&self) -> Result {
         // 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::pcim_enable_device(self.as_raw()) })
     }
 
     /// Enable bus-mastering for this device.
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 1aa8197d8698..213ef14386ca 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -154,7 +154,7 @@ fn probe<'bound>(
             pdev.device_id()
         );
 
-        pdev.enable_device_mem()?;
+        pdev.enable_device_managed()?;
         pdev.set_master();
 
         let bar = pdev.iomap_region_sized::<{ regs::END }>(0, c"rust_driver_pci")?;

---
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
change-id: 20260702-rust-pci-enable-device-managed-155eb5de8603

Best regards,
-- 
Maurice Hieronymus <mhi@mailbox.org>

Re: [PATCH] rust: pci: rework device enabling API

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The new `enable_device_managed` API uses `pcim_enable_device()`, which activates legacy automatic IRQ management that conflicts with Rust's `alloc_irq_vectors()` and violates a documented kernel API contract, leading to double-free issues.
- [High] `disable_device` and `enable_device` are exposed as safe functions, allowing safe Rust code to unbalance the PCI device's `enable_cnt` reference count, causing soundness holes that lead to DMA-after-free.
--

commit 47e71534c620f329a7271d6914a49b70de710719
Author: Maurice Hieronymus <mhi@mailbox.org>
Subject: rust: pci: rework device enabling API

This commit replaces enable_device_mem() with enable_device_managed() to
provide automatic cleanup on unbind via pcim_enable_device(). It also
introduces an unmanaged enable_device()/disable_device() pair for paths
like runtime PM, enabling both I/O and memory resources.

Link: https://lore.kernel.org/r/20260620-b4-rust-pci-edu-driver-v2-0-6fd6684f2c14@mailbox.org [1]
Link: https://lore.kernel.org/rust-for-linux/DJEPMYC7J6ZG.D0ODUD4YGFCT@kernel.org/ [2]

[ ... ]
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 5071cae6543fd..a408b34120777 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -453,10 +453,30 @@ pub fn pci_class(&self) -> Class {
>  }
>  
>  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.
> +    ///
> +    /// This function is unmanaged and does not perform any cleanup when the device is unbound.
> +    /// For a managed function take a look at [`Device::enable_device_managed`].
> +    #[inline]
> +    pub fn enable_device(&self) -> Result {
> +        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
> +        to_result(unsafe { bindings::pci_enable_device(self.as_raw()) })
> +    }
> +
> +    /// Disable I/O and memory resources for this device.
> +    ///
> +    /// This function is the counterpart to [`Device::enable_device`].
> +    #[inline]
> +    pub fn disable_device(&self) {

[Severity: High]
Is it safe to expose these as safe Rust functions? 

Since they directly modify the underlying PCI device's enable_cnt reference
count, safe Rust code could call them in an unbalanced way. For instance,
calling enable_device() multiple times would inflate the count. 

During driver unload, the cleanup path would only decrement it once, leaving
bus-mastering enabled and potentially leading to DMA-after-free when memory
buffers are freed. Should these be marked unsafe, or require an RAII guard
to track the enable count?

> +        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
> +        unsafe { bindings::pci_disable_device(self.as_raw()) }
> +    }
> +
> +    /// Enable I/O and memory resources for this device, with automatic cleanup.
> +    #[inline]
> +    pub fn enable_device_managed(&self) -> Result {
>          // 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::pcim_enable_device(self.as_raw()) })
>      }

[Severity: High]
Will wrapping pcim_enable_device() cause double-free issues with IRQ vectors?

This function activates legacy C devres management for the device by setting
pdev->is_managed = true. If a driver later calls alloc_irq_vectors(), devres
will automatically register pcim_msi_release() as a cleanup action.

However, Rust's IrqVectorRegistration object also explicitly calls
pci_free_irq_vectors() on Drop. When the driver is unbound, Rust will drop
the registration (freeing the vectors), and then the devres subsystem will
execute pcim_msi_release(), attempting to free them again.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-rust-pci-enable-device-managed-v1-1-75bc4ff2935c@mailbox.org?part=1

Re: [PATCH] rust: pci: rework device enabling API

[Danilo Krummrich]

(Cc: Beata, runtime PM consideration at the end)

On Thu Jul 2, 2026 at 11:32 AM CEST, Maurice Hieronymus wrote:
> - `enable_device_managed()`, wrapping `pcim_enable_device()`, which
>   registers a `pci_disable_device()` cleanup that runs on unbind; this
>   is what drivers should normally call in `probe()`.

The concern pointed out by Sashiko that pcim_enable_device() silently also sets
pdev->is_managed = true, which also influences the behavior of other unmanged
PCI paths is valid.

Of course, we could easily overcome this if we have to, but on second thought I
think it would be nice to just not have the managed version at all.

(Note that I also have a patch in my queue to convert IrqVectorRegistration to
use lifetimes instead of Devres, which will also address the topic in [1].)

The advantage of not having to store another object in the bus device private
data is minor, and a lifetime annotated guard is the more idiomatic solution
anyway.

	pub struct DeviceEnableGuard<'a> {
	    dev: &'a pci::Device<Bound>,
	}

Note that obtaining this guard would still require a &Device<Core>, but dropping
it can only be ensured from a bound scope, which &'a pci::Device<Bound> ensures.

This is the tricky part, as this would imply that pci_disable_device() can be
called concurrently with another DeviceEnableGuard being acquired and
concurrently with pci_set_master() and pci_clear_master(), etc., which given the
current implementation on the C side would be UB.

It would be interesting to dig into this and see if we can find a solution to
this problem (which might also include improvements on the C side).

For instance, struct pci_dev has a C bitfield that also includes the
is_busmaster field, which can race with all the other bits being accessed in the
same bitfield.

I think (most of) the fields should be in the same locking domain (the device
lock, which is held in bus callbacks and in Rust represented by the Core device
context state).

But there might already be issues with this, e.g. it seems to me that
block_cfg_access is protected through a different locking domain, the same goes
for a few other fields I think.

Bjorn, any insights on this?

(We had a similar C bitfield in the driver core, which we recently replaced by
using bitops, as there were subtle race conditions.)

> - an unmanaged `enable_device()`/`disable_device()` pair, wrapping
>   `pci_enable_device()`/`pci_disable_device()`, e.g. for runtime PM
>   paths.

If we could find a solution to the above, we could probably avoid having "raw"
accessors that can potentially lead to an unbalanced enable count and instead
could consider something like a DeviceDisableGuard<'a> which could be derived
from a DeviceEnableGuard<'a> for the duration of a suspend.

The runtime PM Rust infrastructure could provide a storage container which
lifetime wise spans suspend => resume, where this guard (and other data that
must be restored on resume) could be stored.

[1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Re-configuring.20interrupt.20vectors/with/577596401