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

[Fiona Behrens]

Maurice Hieronymus <mhi@xxxxxxxxxxx> writes:

> 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.
>
> The existing enable_device_mem() wraps the unmanaged
> pci_enable_device_mem() and has no disable counterpart, so the enable
> count is leaked on unbind. On the next probe pci_enable_device_flags()
> sees a non-zero enable count and returns early, skipping the power-state
> transition back to D0. For a device without a PCI power management
> capability the power state cannot be re-read from hardware and stays
> PCI_UNKNOWN, which makes __pci_enable_msi_range() reject the subsequent
> MSI allocation with -EINVAL.

I wonder if this is then also a good commit to further document this
behaviour in `enable_device_mem`, so that consumers are not only finding
the other variant and overlook this refcounting.

>
> Signed-off-by: Maurice Hieronymus <mhi@xxxxxxxxxxx>

Do favour the `#[inline]`, but for both version

Reviewed-by: Fiona Behrens <me@xxxxxxxxxx>

> ---
>  rust/kernel/pci.rs | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index af74ddff6114..ca04548c82c3 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()) })
>      }
>  
> +    /// 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.
> +    ///
> +    /// The automatic cleanup keeps the device's enable count balanced across driver
> +    /// unbind/rebind cycles. With an unbalanced (leaked) enable count, a re-probe skips the
> +    /// power-state transition back to `D0`, which makes subsequent MSI allocation fail with
> +    /// `EINVAL`.
> +    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()) })
> +    }
> +
>      /// Enable bus-mastering for this device.
>      #[inline]
>      pub fn set_master(&self) {