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