Re: [PATCH v3 1/2] rust: pci: add capability lookup helpers

[Dirk Behme <dirk.behme@gmail.com>]

Hi Zijing,

some quite minor stylistic nits below:

On 31.01.26 16:17, Zijing Zhang wrote:
> Add thin wrappers around pci_find_capability() and
> pci_find_ext_capability().

pci_find_capability() -> `pci_find_capability()`

Same for the other cases and the 2nd commit message.

> The helpers return the capability offset in config space, or None if the
> capability is not present.
> 
> Introduce CapabilityId and ExtendedCapabilityId newtypes so drivers can use
> named constants without importing raw bindgen constants, while still
> allowing uncommon IDs via from_raw().
> 
> Signed-off-by: Zijing Zhang <zijing.zhang@ry.rs>
> ---
>  rust/kernel/pci.rs | 149 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 149 insertions(+)
> 
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 82e128431f08..213ff856f538 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -41,6 +41,118 @@
>      Vendor, //
>  };
>  pub use self::io::Bar;
> +
> +/// A standard PCI capability ID.
> +///
> +/// This is a thin wrapper around the underlying numeric capability ID.
> +#[repr(transparent)]
> +#[derive(Clone, Copy, Debug, Eq, PartialEq)]
> +pub struct CapabilityId(u8);
> +
> +impl CapabilityId {
> +    /// Creates a [`CapabilityId`] from a raw value.
> +    #[inline]
> +    pub const fn from_raw(id: u8) -> Self {
> +        Self(id)
> +    }
> +
> +    /// Returns the raw value.
> +    #[inline]
> +    pub const fn as_raw(self) -> u8 {
> +        self.0
> +    }
> +
> +    /// Power Management.
> +    pub const PM: Self = Self(bindings::PCI_CAP_ID_PM as u8);
> +    /// Accelerated Graphics Port.
> +    pub const AGP: Self = Self(bindings::PCI_CAP_ID_AGP as u8);
> +    /// Vital Product Data.
> +    pub const VPD: Self = Self(bindings::PCI_CAP_ID_VPD as u8);
> +    /// Slot Identification.
> +    pub const SLOTID: Self = Self(bindings::PCI_CAP_ID_SLOTID as u8);
> +    /// Message Signalled Interrupts.
> +    pub const MSI: Self = Self(bindings::PCI_CAP_ID_MSI as u8);
> +    /// CompactPCI HotSwap.
> +    pub const CHSWP: Self = Self(bindings::PCI_CAP_ID_CHSWP as u8);
> +    /// PCI-X.
> +    pub const PCIX: Self = Self(bindings::PCI_CAP_ID_PCIX as u8);
> +    /// HyperTransport.
> +    pub const HT: Self = Self(bindings::PCI_CAP_ID_HT as u8);
> +    /// Vendor-Specific.
> +    pub const VNDR: Self = Self(bindings::PCI_CAP_ID_VNDR as u8);
> +    /// Debug port.
> +    pub const DBG: Self = Self(bindings::PCI_CAP_ID_DBG as u8);
> +    /// CompactPCI Central Resource Control.
> +    pub const CCRC: Self = Self(bindings::PCI_CAP_ID_CCRC as u8);
> +    /// PCI Standard Hot-Plug Controller.
> +    pub const SHPC: Self = Self(bindings::PCI_CAP_ID_SHPC as u8);
> +    /// Bridge subsystem vendor/device ID.
> +    pub const SSVID: Self = Self(bindings::PCI_CAP_ID_SSVID as u8);
> +    /// AGP 8x.
> +    pub const AGP3: Self = Self(bindings::PCI_CAP_ID_AGP3 as u8);
> +    /// Secure Device.
> +    pub const SECDEV: Self = Self(bindings::PCI_CAP_ID_SECDEV as u8);
> +    /// PCI Express.
> +    pub const EXP: Self = Self(bindings::PCI_CAP_ID_EXP as u8);
> +    /// MSI-X.
> +    pub const MSIX: Self = Self(bindings::PCI_CAP_ID_MSIX as u8);
> +    /// Serial ATA Data/Index Configuration.
> +    pub const SATA: Self = Self(bindings::PCI_CAP_ID_SATA as u8);
> +    /// PCI Advanced Features.
> +    pub const AF: Self = Self(bindings::PCI_CAP_ID_AF as u8);
> +    /// PCI Enhanced Allocation.
> +    pub const EA: Self = Self(bindings::PCI_CAP_ID_EA as u8);
> +}
> +
> +/// A PCIe extended capability ID.
> +///
> +/// This is a thin wrapper around the underlying numeric capability ID.
> +#[repr(transparent)]
> +#[derive(Clone, Copy, Debug, Eq, PartialEq)]
> +pub struct ExtendedCapabilityId(u16);
> +
> +impl ExtendedCapabilityId {
> +    /// Creates an [`ExtendedCapabilityId`] from a raw value.
> +    #[inline]
> +    pub const fn from_raw(id: u16) -> Self {
> +        Self(id)
> +    }
> +
> +    /// Returns the raw value.
> +    #[inline]
> +    pub const fn as_raw(self) -> u16 {
> +        self.0
> +    }
> +
> +    /// Advanced Error Reporting.
> +    pub const ERR: Self = Self(bindings::PCI_EXT_CAP_ID_ERR as u16);
> +    /// Virtual Channel.
> +    pub const VC: Self = Self(bindings::PCI_EXT_CAP_ID_VC as u16);
> +    /// Device Serial Number.
> +    pub const DSN: Self = Self(bindings::PCI_EXT_CAP_ID_DSN as u16);
> +    /// Vendor-Specific Extended Capability.
> +    pub const VNDR: Self = Self(bindings::PCI_EXT_CAP_ID_VNDR as u16);
> +    /// Access Control Services.
> +    pub const ACS: Self = Self(bindings::PCI_EXT_CAP_ID_ACS as u16);
> +    /// Alternate Routing-ID Interpretation.
> +    pub const ARI: Self = Self(bindings::PCI_EXT_CAP_ID_ARI as u16);
> +    /// Address Translation Services.
> +    pub const ATS: Self = Self(bindings::PCI_EXT_CAP_ID_ATS as u16);
> +    /// Single Root I/O Virtualization.
> +    pub const SRIOV: Self = Self(bindings::PCI_EXT_CAP_ID_SRIOV as u16);
> +    /// Resizable BAR.
> +    pub const REBAR: Self = Self(bindings::PCI_EXT_CAP_ID_REBAR as u16);
> +    /// Latency Tolerance Reporting.
> +    pub const LTR: Self = Self(bindings::PCI_EXT_CAP_ID_LTR as u16);
> +    /// Downstream Port Containment.
> +    pub const DPC: Self = Self(bindings::PCI_EXT_CAP_ID_DPC as u16);
> +    /// L1 PM Substates.
> +    pub const L1SS: Self = Self(bindings::PCI_EXT_CAP_ID_L1SS as u16);
> +    /// Precision Time Measurement.
> +    pub const PTM: Self = Self(bindings::PCI_EXT_CAP_ID_PTM as u16);
> +    /// Designated Vendor-Specific Extended Capability.
> +    pub const DVSEC: Self = Self(bindings::PCI_EXT_CAP_ID_DVSEC as u16);
> +}

Missing empty line before `pub use self::irq` below?

>  pub use self::irq::{
>      IrqType,
>      IrqTypes,
> @@ -427,6 +539,43 @@ pub fn pci_class(&self) -> Class {
>          // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
>          Class::from_raw(unsafe { (*self.as_raw()).class })
>      }
> +
> +    /// Finds a PCI capability by ID and returns its config-space offset.
> +    ///
> +    /// Returns `None` if the capability is not present.
> +    ///
> +    /// This is a thin wrapper around `pci_find_capability()`.
> +    #[inline]
> +    pub fn find_capability(&self, id: CapabilityId) -> Option<u8> {

Depending on the future use of this function (and
`find_ext_capability()` below) would it be an option to introduce a
type for the returned u8/u16 offset?

> +        // SAFETY: By its type invariant `self.as_raw` is always a valid pointer to a
> +        // `struct pci_dev`.
> +        let offset = unsafe { bindings::pci_find_capability(self.as_raw(), id.as_raw().into()) };
> +
> +        if offset == 0 {
> +            None
> +        } else {
> +            Some(offset)
> +        }

In

https://lore.kernel.org/rust-for-linux/CANiq72kiscT5euAUjcSzvxMzM9Hdj8aQGeUN_pVF-vHf3DhBuQ@mail.gmail.com/

it was proposed to use the style

if offset == 0 {
    return None;
}

Some(offset)


Best regards

Dirk


> +    }
> +
> +    /// Finds an extended capability by ID and returns its config-space offset.
> +    ///
> +    /// Returns `None` if the capability is not present.
> +    ///
> +    /// This is a thin wrapper around `pci_find_ext_capability()`.
> +    #[inline]
> +    pub fn find_ext_capability(&self, id: ExtendedCapabilityId) -> Option<u16> {
> +        // SAFETY: By its type invariant `self.as_raw` is always a valid pointer to a
> +        // `struct pci_dev`.
> +        let offset =
> +            unsafe { bindings::pci_find_ext_capability(self.as_raw(), id.as_raw().into()) };
> +
> +        if offset == 0 {
> +            None
> +        } else {
> +            Some(offset)
> +        }
> +    }
>  }
>  
>  impl Device<device::Core> {