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

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