[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

[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

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

[Maurice Hieronymus]

On Thu Jul 2, 2026 at 11:47 PM CEST, Danilo Krummrich wrote:

> 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>,
> 	}
>
I agree. I'll implement the guard once the bitfield question below is
resolved.

> 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.
>
broken_parity_status can be set via sysfs at any time
(broken_parity_status_store() in drivers/pci/pci-sysfs.c), without any
lock the other writers of that bitfield word take. Racing that against
e.g. pci_set_master() from a runtime PM callback (which runs without
the device lock, e.g. nouveau_pmops_runtime_resume()) is a data race
on the shared word - triggerable from userspace today.

> (We had a similar C bitfield in the driver core, which we recently replaced by
> using bitops, as there were subtle race conditions.)
>
I looked at a7cc262a1135 ("driver core: Replace dev->offline +
->offline_disabled with accessors") and would offer to convert
pci_dev->is_busmaster as a first step, so the Rust device enabling
API can make progress; more bitfields could follow the same pattern
later.

The one question is where the bit should live. pci_dev already has
priv_flags, but its bit definitions and accessors are private to
drivers/pci, while is_busmaster is accessed directly from outside:
xen-pciback writes it, lpfc and sfc read it. So either priv_flags
grows public accessors for this bit, or struct pci_dev gets a public
flags bitmap with an accessor macro, like struct device.

Bjorn, would you take such a patch, and which of the two would you
prefer?

Best,

Maurice