Re: [PATCH 2/4] rust: pci: add managed Device::enable_device()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Wrapping `pcim_enable_device()` in `Device::enable_device()` triggers a legacy C-side side effect that conflicts with Rust's explicit IRQ management, causing a double-free of IRQ vectors on driver unbind.
- [Low] Missing `#[inline]` annotation on a small forwarding function in an abstraction crate.

Pre-existing issues:
- [High] The existing `Device::enable_device_mem()` method leaks the PCI device's enable count because it lacks a devres cleanup action or safe disable method.
--

commit 418855d3e0788282fef9f3b016a6200435cea33d
Author: Maurice Hieronymus <mhi@mailbox.org>

rust: pci: add managed Device::enable_device()

Add a managed counterpart to Device::enable_device_mem() that wraps
pcim_enable_device(). In addition to enabling the device, it registers a
pci_disable_device() cleanup that runs automatically when the device is
unbound from its driver, keeping the device's enable count balanced
across unbind/rebind cycles.

> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index af74ddff6114d..ca04548c82c31 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -452,6 +452,22 @@ pub fn enable_device_mem(&self) -> Result {
>          to_result(unsafe { bindings::pci_enable_device_mem(self.as_raw()) })
>      }

[Severity: High]
This isn't a bug introduced by this patch, but does the existing
Device::enable_device_mem() method leak the PCI device's enable count?

Since this method wraps the unmanaged pci_enable_device_mem(), it increments
the device's internal enable count. Safe Rust currently exposes no
disable_device_mem method, and enable_device_mem does not register a devres
cleanup action.

It appears that any driver using this safe method will permanently leak the
enable count on unbind. As noted in the commit message, this prevents the
device from transitioning back to the D0 power state on re-probe, causing
subsequent MSI allocations to fail. Should this be refactored to use devres
for automatic cleanup, or be marked unsafe?

> +
> +    /// Enable I/O and memory resources for this device, with automatic cleanup.
> +    ///

[ ... ]

> +    pub fn enable_device(&self) -> Result {

[Severity: Low]
Should this function have an #[inline] annotation?

The Rust guidelines for the kernel state that for abstractions, functions
that are small or forwarding to a binding call should be annotated with
#[inline]. Since Device::enable_device() simply forwards to the C binding
pcim_enable_device(), it seems to fit this criteria.

> +        // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
> +        to_result(unsafe { bindings::pcim_enable_device(self.as_raw()) })
> +    }

[Severity: High]
Does wrapping pcim_enable_device() here trigger a legacy C-side side effect
that conflicts with Rust's explicit IRQ management?

The function pcim_enable_device() sets pdev->is_managed = true. When a
driver later allocates IRQs via the Rust method Device::alloc_irq_vectors(),
the underlying C function pci_alloc_irq_vectors() detects this flag and
automatically registers a C-side devres action (pcim_msi_release) to free
the IRQ vectors.

However, the Rust method unconditionally registers its own devres action
which also drops them:

rust/kernel/pci/irq.rs:IrqVectorRegistration::drop() {
    unsafe { bindings::pci_free_irq_vectors(self.dev.as_raw()) };
}

During driver unbind, both devres actions would execute, causing
pci_free_irq_vectors() to be called twice. Could this be resolved by having
Device::enable_device() wrap the unmanaged pci_enable_device() instead, and
explicitly register a Rust-side devres cleanup handler that calls
pci_disable_device()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260614-b4-rust-pci-edu-driver-v1-0-e3f2471b595c@mailbox.org?part=2