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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The unmanaged `enable_device_mem()` leaks the PCI enable count on unbind, breaking driver reload.
--

commit 1d6899aaf8110fc74f47f044741e7a507b4dfb2e
Author: Maurice Hieronymus <mhi@mailbox.org>

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

This commit adds a managed counterpart to Device::enable_device_mem() that
wraps pcim_enable_device(). It ensures the device's enable count is
balanced across unbind/rebind cycles by automatically registering a cleanup
action. It also documents the existing enable_device_mem() as unmanaged.

> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 5071cae6543fd..d076a36910910 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -454,11 +454,27 @@ pub fn pci_class(&self) -> Class {
>  
>  impl<'a> Device<device::Core<'a>> {
>      /// Enable 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`].
> +    #[inline]
>      pub fn enable_device_mem(&self) -> Result {

[Severity: High]
This is a pre-existing issue, but since the commit message explicitly notes
that leaking the enable count breaks driver reload and MSI allocation,
shouldn't existing callers of Device::enable_device_mem() be updated?

For example, NovaCoreDriver::probe() in drivers/gpu/nova-core/driver.rs and
SampleDriver::probe() in samples/rust/rust_driver_pci.rs still use the
unmanaged enable_device_mem().

Would it be better to either migrate these existing drivers to use the newly
introduced managed enable_device(), or provide a managed memory-only
equivalent (wrapping pcim_enable_device_mem()) for them to use?

>          // 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()) })
>      }
>  
> +    /// Enable I/O and memory resources for this device, with automatic cleanup.
> +    ///
> +    /// This is the managed version of `pci_enable_device()`: it enables the device's I/O and
> +    /// memory resources and registers a `pci_disable_device()` call that runs automatically
> +    /// when the device is unbound from its driver. In contrast, [`Device::enable_device_mem`]
> +    /// is unmanaged and only enables memory resources.
> +    #[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::pcim_enable_device(self.as_raw()) })
> +    }
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260620-b4-rust-pci-edu-driver-v2-0-6fd6684f2c14@mailbox.org?part=2