Message-Id: <20250623.220235.715005847758063914.fujita.tomonori@gmail.com>
Date: Mon, 23 Jun 2025 22:02:35 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: miguel.ojeda.sandonis@...il.com
Cc: fujita.tomonori@...il.com, alex.gaynor@...il.com, dakr@...nel.org,
 gregkh@...uxfoundation.org, ojeda@...nel.org, rafael@...nel.org,
 robh@...nel.org, saravanak@...gle.com, a.hindborg@...nel.org,
 aliceryhl@...gle.com, bhelgaas@...gle.com, bjorn3_gh@...tonmail.com,
 boqun.feng@...il.com, david.m.ertman@...el.com,
 devicetree@...r.kernel.org, gary@...yguo.net, ira.weiny@...el.com,
 kwilczynski@...nel.org, leon@...nel.org, linux-kernel@...r.kernel.org,
 linux-pci@...r.kernel.org, lossin@...nel.org, netdev@...r.kernel.org,
 rust-for-linux@...r.kernel.org, tmgross@...ch.edu
Subject: Re: [PATCH v1 3/3] rust: net::phy Change module_phy_driver macro
 to use module_device_table macro

On Mon, 23 Jun 2025 11:21:59 +0200
Miguel Ojeda <miguel.ojeda.sandonis@...il.com> wrote:

> On Mon, Jun 23, 2025 at 8:10 AM FUJITA Tomonori
> <fujita.tomonori@...il.com> wrote:
>>
>> +// SAFETY: [`DeviceId`] is a `#[repr(transparent)` wrapper of `struct mdio_device_id`
> 
> No need for intra-doc link in normal comments.

Oops. I'll fix.

> Also, missing bracket (which apparently comes from another comment, so
> probably copy-pasted).

Yeah, it was copied from the PCI code. The same typo exists in
multiple files, so I’ll send a fix for them later.

>> +    // This should never be called.
>> +    fn index(&self) -> usize {
>> +        0
>> +    }
> 
> Hmm... This isn't great. Could this perhaps be designed differently?
> e.g. split into two traits, possibly based one on another, or similar?

Yeah, I don’t really like it either.

I think we could split the RawDeviceId trait into two: RawDeviceId and
RawDeviceIdIndex. Device ID structures that do not contain
context/data field (like mdio_device for PHY) would implement only
RawDeviceId, while pci and others would implement both.

To avoid duplicate code, the new() function has been split into two as
follows. I'm also fine with simply adding an assert to index().

diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
index 3dc72ca8cfc2..f9205ea2a05e 100644
--- a/rust/kernel/device_id.rs
+++ b/rust/kernel/device_id.rs
@@ -35,7 +35,9 @@ pub unsafe trait RawDeviceId {
     ///
     /// Id tables created from [`Self`] are going to hold this type in its zero-terminated array.
     type RawType: Copy;
+}
 
+pub unsafe trait RawDeviceIdIndex {
     /// The offset to the context/data field.
     const DRIVER_DATA_OFFSET: usize;
 
@@ -65,10 +67,7 @@ pub struct IdArray<T: RawDeviceId, U, const N: usize> {
 }
 
 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 {
+    const fn init_ids(ids: [(T, U); N]) -> ([MaybeUninit<T::RawType>; N], [MaybeUninit<U>; N]) {
         let mut raw_ids = [const { MaybeUninit::<T::RawType>::uninit() }; N];
         let mut infos = [const { MaybeUninit::uninit() }; N];
 
@@ -77,24 +76,18 @@ 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);
-            }
 
             // SAFETY: this is effectively a move: `infos[i] = ids[i].1`. We make a copy here but
             // later forget `ids`.
             infos[i] = MaybeUninit::new(unsafe { core::ptr::read(&ids[i].1) });
             i += 1;
         }
-
         core::mem::forget(ids);
 
+        (raw_ids, infos)
+    }
+
+    const fn build_ids(raw_ids: [MaybeUninit<T::RawType>; N], infos: [MaybeUninit<U>; N]) -> Self {
         Self {
             raw_ids: RawIdArray {
                 // SAFETY: this is effectively `array_assume_init`, which is unstable, so we use
@@ -109,12 +102,47 @@ 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.
+    // once the specialization feature becomes stable, we can use `new()` name.
+    pub const fn new_without_index(ids: [(T, U); N]) -> Self {
+        let (raw_ids, infos) = Self::init_ids(ids);
+
+        Self::build_ids(raw_ids, infos)
+    }
+
     /// 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 {
+        let (mut raw_ids, infos) = Self::init_ids(ids);
+
+        let mut i = 0usize;
+        while i < N {
+            // 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);
+            }
+            i += 1;
+        }
+
+        Self::build_ids(raw_ids, infos)
+    }
+}
+
 /// A device id table.
 ///
 /// This trait is only implemented by `IdArray`.
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index f6b19764ad17..25ed7e5c56fa 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -8,6 +8,7 @@
     alloc::flags::*,
     bindings, container_of, device,
     device_id::RawDeviceId,
+    device_id::RawDeviceIdIndex,
     devres::Devres,
     driver,
     error::{to_result, Result},
@@ -167,7 +168,9 @@ pub const fn from_class(class: u32, class_mask: u32) -> Self {
 // * `DRIVER_DATA_OFFSET` is the offset to the `driver_data` field.
 unsafe impl RawDeviceId for DeviceId {
     type RawType = bindings::pci_device_id;
+}
 
+unsafe impl RawDeviceIdIndex for DeviceId {
     const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::pci_device_id, driver_data);
 
     fn index(&self) -> usize {

Powered by blists - more mailing lists