Re: [PATCH v3 1/3] rust: device_id: split out index support into a separate trait
From: "Trevor Gross" <tmgross@umich.edu>
Date: 2025-07-09 03:11:03
Also in:
linux-devicetree, linux-pci, lkml, rust-for-linux
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:
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
@@ -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? :)
quoted hunk ↗ jump to hunk
@@ -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>