Re: [PATCH v3 1/3] rust: device_id: split out index support into a separate trait

["Trevor Gross" <tmgross@umich.edu>]

On Fri Jul 4, 2025 at 12:10 AM EDT, FUJITA Tomonori wrote:
> Introduce a new trait `RawDeviceIdIndex`, which extends `RawDeviceId`
> to provide support for device ID types that include an index or
> context field (e.g., `driver_data`). This separates the concerns of
> layout compatibility and index-based data embedding, and allows
> `RawDeviceId` to be implemented for types that do not contain a
> `driver_data` field. Several such structures are defined in
> include/linux/mod_devicetable.h.
>
> Refactor `IdArray::new()` into a generic `build()` function, which
> takes an optional offset. Based on the presence of `RawDeviceIdIndex`,
> index writing is conditionally enabled. A new `new_without_index()`
> constructor is also provided for use cases where no index should be
> written.
>
> This refactoring is a preparation for enabling the PHY abstractions to
> use device_id trait.
>
> Acked-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/auxiliary.rs | 11 ++---
>  rust/kernel/device_id.rs | 91 ++++++++++++++++++++++++++++------------
>  rust/kernel/of.rs        | 15 ++++---
>  rust/kernel/pci.rs       | 11 ++---
>  4 files changed, 87 insertions(+), 41 deletions(-)

Few small suggestions if you wind up spinning this again:

> diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
> [...]
> @@ -68,7 +77,14 @@ impl<T: RawDeviceId, U, const N: usize> IdArray<T, U, N> {
>      /// Creates a new instance of the array.
>      ///
>      /// The contents are derived from the given identifiers and context information.
> -    pub const fn new(ids: [(T, U); N]) -> Self {
> +    ///
> +    /// # Safety
> +    ///
> +    /// If `offset` is `Some(offset)`, then:
> +    /// - `offset` must be the correct offset (in bytes) to the context/data field
> +    ///   (e.g., the `driver_data` field) within the raw device ID structure.
> +    /// - The field at `offset` must be correctly sized to hold a `usize`.
> +    const unsafe fn build(ids: [(T, U); N], offset: Option<usize>) -> Self {

Could you mention that calling with `offset` as `None` is always safe?
Also calling the arg `data_offset` might be more clear.

> @@ -92,7 +111,6 @@ impl<T: RawDeviceId, U, const N: usize> IdArray<T, U, N> {
>              infos[i] = MaybeUninit::new(unsafe { core::ptr::read(&ids[i].1) });
>              i += 1;
>          }
> -
>          core::mem::forget(ids);

This removes the space between a block and an expression, possibly
unintentional? :)

> @@ -109,12 +127,33 @@ impl<T: RawDeviceId, U, const N: usize> IdArray<T, U, N> {
>          }
>      }
>  
> +    /// Creates a new instance of the array without writing index values.
> +    ///
> +    /// The contents are derived from the given identifiers and context information.

Maybe the docs here should crosslink:

    If the device implements [`RawDeviceIdIndex`], consider using
    [`new`] instead.

> +    pub const fn new_without_index(ids: [(T, U); N]) -> Self {
> +        // SAFETY: Calling `Self::build` with `offset = None` is always safe,
> +        // because no raw memory writes are performed in this case.
> +        unsafe { Self::build(ids, None) }
> +    }
> +

With those changes, or as-is if there winds up not being another
version:

Reviewed-by: Trevor Gross <tmgross@umich.edu>