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(-)

diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
index d2cfe1eeefb6..526cb6dcad52 100644
--- a/rust/kernel/auxiliary.rs
+++ b/rust/kernel/auxiliary.rs
@@ -6,7 +6,7 @@
 
 use crate::{
     bindings, container_of, device,
-    device_id::RawDeviceId,
+    device_id::{RawDeviceId, RawDeviceIdIndex},
     driver,
     error::{to_result, Result},
     prelude::*,
@@ -140,13 +140,14 @@ pub const fn new(modname: &'static CStr, name: &'static CStr) -> Self {
     }
 }
 
-// SAFETY:
-// * `DeviceId` is a `#[repr(transparent)`] wrapper of `auxiliary_device_id` and does not add
-//   additional invariants, so it's safe to transmute to `RawType`.
-// * `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field.
+// SAFETY: `DeviceId` is a `#[repr(transparent)]` wrapper of `auxiliary_device_id` and does not add
+// additional invariants, so it's safe to transmute to `RawType`.
 unsafe impl RawDeviceId for DeviceId {
     type RawType = bindings::auxiliary_device_id;
+}
 
+// SAFETY: `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field.
+unsafe impl RawDeviceIdIndex for DeviceId {
     const DRIVER_DATA_OFFSET: usize =
         core::mem::offset_of!(bindings::auxiliary_device_id, driver_data);
 
diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
index 3dc72ca8cfc2..242666c2409c 100644
--- a/rust/kernel/device_id.rs
+++ b/rust/kernel/device_id.rs
@@ -14,32 +14,41 @@
 ///
 /// # Safety
 ///
-/// Implementers must ensure that:
-///   - `Self` is layout-compatible with [`RawDeviceId::RawType`]; i.e. it's safe to transmute to
-///     `RawDeviceId`.
+/// Implementers must ensure that `Self` is layout-compatible with [`RawDeviceId::RawType`];
+/// i.e. it's safe to transmute to `RawDeviceId`.
 ///
-///     This requirement is needed so `IdArray::new` can convert `Self` to `RawType` when building
-///     the ID table.
+/// This requirement is needed so `IdArray::new` can convert `Self` to `RawType` when building
+/// the ID table.
 ///
-///     Ideally, this should be achieved using a const function that does conversion instead of
-///     transmute; however, const trait functions relies on `const_trait_impl` unstable feature,
-///     which is broken/gone in Rust 1.73.
-///
-///   - `DRIVER_DATA_OFFSET` is the offset of context/data field of the device ID (usually named
-///     `driver_data`) of the device ID, the field is suitable sized to write a `usize` value.
-///
-///     Similar to the previous requirement, the data should ideally be added during `Self` to
-///     `RawType` conversion, but there's currently no way to do it when using traits in const.
+/// Ideally, this should be achieved using a const function that does conversion instead of
+/// transmute; however, const trait functions relies on `const_trait_impl` unstable feature,
+/// which is broken/gone in Rust 1.73.
 pub unsafe trait RawDeviceId {
     /// The raw type that holds the device id.
     ///
     /// Id tables created from [`Self`] are going to hold this type in its zero-terminated array.
     type RawType: Copy;
+}
 
-    /// The offset to the context/data field.
+/// Extension trait for [`RawDeviceId`] for devices that embed an index or context value.
+///
+/// This is typically used when the device ID struct includes a field like `driver_data`
+/// that is used to store a pointer-sized value (e.g., an index or context pointer).
+///
+/// # Safety
+///
+/// Implementers must ensure that `DRIVER_DATA_OFFSET` is the correct offset (in bytes) to
+/// the context/data field (e.g., the `driver_data` field) within the raw device ID structure.
+/// This field must be correctly sized to hold a `usize`.
+///
+/// Ideally, the data should be added during `Self` to `RawType` conversion,
+/// but there's currently no way to do it when using traits in const.
+pub unsafe trait RawDeviceIdIndex: RawDeviceId {
+    /// The offset (in bytes) to the context/data field in the raw device ID.
     const DRIVER_DATA_OFFSET: usize;
 
-    /// The index stored at `DRIVER_DATA_OFFSET` of the implementor of the [`RawDeviceId`] trait.
+    /// The index stored at `DRIVER_DATA_OFFSET` of the implementor of the [`RawDeviceIdIndex`]
+    /// trait.
     fn index(&self) -> usize;
 }
 
@@ -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 {
         let mut raw_ids = [const { MaybeUninit::<T::RawType>::uninit() }; N];
         let mut infos = [const { MaybeUninit::uninit() }; N];
 
@@ -77,14 +93,17 @@ impl<T: RawDeviceId, U, const N: usize> IdArray<T, U, N> {
             // SAFETY: by the safety requirement of `RawDeviceId`, we're guaranteed that `T` is
             // layout-wise compatible with `RawType`.
             raw_ids[i] = unsafe { core::mem::transmute_copy(&ids[i].0) };
-            // SAFETY: by the safety requirement of `RawDeviceId`, this would be effectively
-            // `raw_ids[i].driver_data = i;`.
-            unsafe {
-                raw_ids[i]
-                    .as_mut_ptr()
-                    .byte_add(T::DRIVER_DATA_OFFSET)
-                    .cast::<usize>()
-                    .write(i);
+
+            if let Some(offset) = offset {
+                // SAFETY: by the safety requirement of this function, this would be effectively
+                // `raw_ids[i].driver_data = i;`.
+                unsafe {
+                    raw_ids[i]
+                        .as_mut_ptr()
+                        .byte_add(offset)
+                        .cast::<usize>()
+                        .write(i);
+                }
             }
 
             // SAFETY: this is effectively a move: `infos[i] = ids[i].1`. We make a copy here but
@@ -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);
 
         Self {
@@ -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.
+    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) }
+    }
+
     /// Reference to the contained [`RawIdArray`].
     pub const fn raw_ids(&self) -> &RawIdArray<T, N> {
         &self.raw_ids
     }
 }
 
+impl<T: RawDeviceId + RawDeviceIdIndex, 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: by the safety requirement of `RawDeviceIdIndex`,
+        // `T::DRIVER_DATA_OFFSET` is guaranteed to be the correct offset (in bytes) to
+        // a field within `T::RawType`.
+        unsafe { Self::build(ids, Some(T::DRIVER_DATA_OFFSET)) }
+    }
+}
+
 /// A device id table.
 ///
 /// This trait is only implemented by `IdArray`.
diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs
index 40d1bd13682c..0c799a06371d 100644
--- a/rust/kernel/of.rs
+++ b/rust/kernel/of.rs
@@ -2,7 +2,11 @@
 
 //! Device Tree / Open Firmware abstractions.
 
-use crate::{bindings, device_id::RawDeviceId, prelude::*};
+use crate::{
+    bindings,
+    device_id::{RawDeviceId, RawDeviceIdIndex},
+    prelude::*,
+};
 
 /// IdTable type for OF drivers.
 pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<DeviceId, T>;
@@ -12,13 +16,14 @@
 #[derive(Clone, Copy)]
 pub struct DeviceId(bindings::of_device_id);
 
-// SAFETY:
-// * `DeviceId` is a `#[repr(transparent)` wrapper of `struct of_device_id` and does not add
-//   additional invariants, so it's safe to transmute to `RawType`.
-// * `DRIVER_DATA_OFFSET` is the offset to the `data` field.
+// SAFETY: `DeviceId` is a `#[repr(transparent)]` wrapper of `struct of_device_id` and does not add
+// additional invariants, so it's safe to transmute to `RawType`.
 unsafe impl RawDeviceId for DeviceId {
     type RawType = bindings::of_device_id;
+}
 
+// SAFETY: `DRIVER_DATA_OFFSET` is the offset to the `data` field.
+unsafe impl RawDeviceIdIndex for DeviceId {
     const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::of_device_id, data);
 
     fn index(&self) -> usize {
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 6b94fd7a3ce9..8012bfad3150 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -7,7 +7,7 @@
 use crate::{
     alloc::flags::*,
     bindings, container_of, device,
-    device_id::RawDeviceId,
+    device_id::{RawDeviceId, RawDeviceIdIndex},
     devres::Devres,
     driver,
     error::{to_result, Result},
@@ -161,13 +161,14 @@ pub const fn from_class(class: u32, class_mask: u32) -> Self {
     }
 }
 
-// SAFETY:
-// * `DeviceId` is a `#[repr(transparent)` wrapper of `pci_device_id` and does not add
-//   additional invariants, so it's safe to transmute to `RawType`.
-// * `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field.
+// SAFETY: `DeviceId` is a `#[repr(transparent)]` wrapper of `pci_device_id` and does not add
+// additional invariants, so it's safe to transmute to `RawType`.
 unsafe impl RawDeviceId for DeviceId {
     type RawType = bindings::pci_device_id;
+}
 
+// SAFETY: `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field.
+unsafe impl RawDeviceIdIndex for DeviceId {
     const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::pci_device_id, driver_data);
 
     fn index(&self) -> usize {
-- 
2.43.0

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>

On Tue, 08 Jul 2025 23:10:48 -0400
"Trevor Gross" <tmgross@umich.edu> wrote:

> 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?

Indeed, added.

> Also calling the arg `data_offset` might be more clear.

Yeah, changed.

>> @@ -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? :)

Oops, unintentional. Dropped the change.

>> @@ -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.

Looks nice, added. [`new`] doesn't work so I use [`IdArray::new`].

>> +    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>

Thanks!