[PATCH v3 0/3] rust: Build PHY device tables by using module_device_table macro

[PATCH v3 0/3] rust: Build PHY device tables by using module_device_table macro

[FUJITA Tomonori]

Build PHY device tables by using module_device_table macro.

The PHY abstractions have been generating their own device tables
manually instead of using the module_device_table macro provided by
the device_id crate. However, the format of device tables occasionally
changes [1] [2], requiring updates to both the device_id crate and the custom
format used by the PHY abstractions, which is cumbersome to maintain.

[1]: https://lore.kernel.org/lkml/20241119235705.1576946-14-masahiroy@kernel.org/
[2]: https://lore.kernel.org/lkml/6e2f70b07a710e761eb68d089d96cee7b27bb2d5.1750511018.git.legion@kernel.org/

v3:
- Fix Safety comments and typo
v2: https://lore.kernel.org/lkml/20250701141252.600113-1-fujita.tomonori@gmail.com/
- Split off index-related parts of RawDeviceId into RawDeviceIdIndex
v1: https://lore.kernel.org/lkml/20250623060951.118564-1-fujita.tomonori@gmail.com/

FUJITA Tomonori (3):
  rust: device_id: split out index support into a separate trait
  rust: net::phy represent DeviceId as transparent wrapper over
    mdio_device_id
  rust: net::phy Change module_phy_driver macro to use
    module_device_table macro

 rust/kernel/auxiliary.rs |  11 +++--
 rust/kernel/device_id.rs |  91 ++++++++++++++++++++++++----------
 rust/kernel/net/phy.rs   | 104 +++++++++++++++++++--------------------
 rust/kernel/of.rs        |  15 ++++--
 rust/kernel/pci.rs       |  11 +++--
 5 files changed, 138 insertions(+), 94 deletions(-)


base-commit: 2009a2d5696944d85c34d75e691a6f3884e787c0
-- 
2.43.0

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

[FUJITA Tomonori]

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

[PATCH v3 2/3] rust: net::phy represent DeviceId as transparent wrapper over mdio_device_id

[FUJITA Tomonori]

Refactor the DeviceId struct to be a #[repr(transparent)] wrapper
around the C struct bindings::mdio_device_id.

This refactoring is a preparation for enabling the PHY abstractions to
use device_id trait.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/net/phy.rs | 53 +++++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 65ac4d59ad77..940972ffadae 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -507,7 +507,7 @@ pub const fn create_phy_driver<T: Driver>() -> DriverVTable {
     DriverVTable(Opaque::new(bindings::phy_driver {
         name: T::NAME.as_char_ptr().cast_mut(),
         flags: T::FLAGS,
-        phy_id: T::PHY_DEVICE_ID.id,
+        phy_id: T::PHY_DEVICE_ID.id(),
         phy_id_mask: T::PHY_DEVICE_ID.mask_as_int(),
         soft_reset: if T::HAS_SOFT_RESET {
             Some(Adapter::<T>::soft_reset_callback)
@@ -691,42 +691,41 @@ fn drop(&mut self) {
 ///
 /// Represents the kernel's `struct mdio_device_id`. This is used to find an appropriate
 /// PHY driver.
-pub struct DeviceId {
-    id: u32,
-    mask: DeviceMask,
-}
+#[repr(transparent)]
+#[derive(Clone, Copy)]
+pub struct DeviceId(bindings::mdio_device_id);
 
 impl DeviceId {
     /// Creates a new instance with the exact match mask.
     pub const fn new_with_exact_mask(id: u32) -> Self {
-        DeviceId {
-            id,
-            mask: DeviceMask::Exact,
-        }
+        Self(bindings::mdio_device_id {
+            phy_id: id,
+            phy_id_mask: DeviceMask::Exact.as_int(),
+        })
     }
 
     /// Creates a new instance with the model match mask.
     pub const fn new_with_model_mask(id: u32) -> Self {
-        DeviceId {
-            id,
-            mask: DeviceMask::Model,
-        }
+        Self(bindings::mdio_device_id {
+            phy_id: id,
+            phy_id_mask: DeviceMask::Model.as_int(),
+        })
     }
 
     /// Creates a new instance with the vendor match mask.
     pub const fn new_with_vendor_mask(id: u32) -> Self {
-        DeviceId {
-            id,
-            mask: DeviceMask::Vendor,
-        }
+        Self(bindings::mdio_device_id {
+            phy_id: id,
+            phy_id_mask: DeviceMask::Vendor.as_int(),
+        })
     }
 
     /// Creates a new instance with a custom match mask.
     pub const fn new_with_custom_mask(id: u32, mask: u32) -> Self {
-        DeviceId {
-            id,
-            mask: DeviceMask::Custom(mask),
-        }
+        Self(bindings::mdio_device_id {
+            phy_id: id,
+            phy_id_mask: DeviceMask::Custom(mask).as_int(),
+        })
     }
 
     /// Creates a new instance from [`Driver`].
@@ -734,18 +733,20 @@ pub const fn new_with_driver<T: Driver>() -> Self {
         T::PHY_DEVICE_ID
     }
 
+    /// Get a `phy_id` as u32.
+    pub const fn id(&self) -> u32 {
+        self.0.phy_id
+    }
+
     /// Get a `mask` as u32.
     pub const fn mask_as_int(&self) -> u32 {
-        self.mask.as_int()
+        self.0.phy_id_mask
     }
 
     // macro use only
     #[doc(hidden)]
     pub const fn mdio_device_id(&self) -> bindings::mdio_device_id {
-        bindings::mdio_device_id {
-            phy_id: self.id,
-            phy_id_mask: self.mask.as_int(),
-        }
+        self.0
     }
 }
 
-- 
2.43.0

[PATCH v3 3/3] rust: net::phy Change module_phy_driver macro to use module_device_table macro

[FUJITA Tomonori]

Change module_phy_driver macro to build device tables which are
exported to userspace by using module_device_table macro.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/net/phy.rs | 51 ++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 27 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 940972ffadae..338451a067fa 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -6,7 +6,7 @@
 //!
 //! C headers: [`include/linux/phy.h`](srctree/include/linux/phy.h).
 
-use crate::{error::*, prelude::*, types::Opaque};
+use crate::{device_id::RawDeviceId, error::*, prelude::*, types::Opaque};
 use core::{marker::PhantomData, ptr::addr_of_mut};
 
 pub mod reg;
@@ -750,6 +750,12 @@ pub const fn mdio_device_id(&self) -> bindings::mdio_device_id {
     }
 }
 
+// SAFETY: `DeviceId` is a `#[repr(transparent)]` wrapper of `struct mdio_device_id`
+// and does not add additional invariants, so it's safe to transmute to `RawType`.
+unsafe impl RawDeviceId for DeviceId {
+    type RawType = bindings::mdio_device_id;
+}
+
 enum DeviceMask {
     Exact,
     Model,
@@ -850,19 +856,18 @@ const fn as_int(&self) -> u32 {
 ///     }
 /// };
 ///
-/// const _DEVICE_TABLE: [::kernel::bindings::mdio_device_id; 2] = [
-///     ::kernel::bindings::mdio_device_id {
-///         phy_id: 0x00000001,
-///         phy_id_mask: 0xffffffff,
-///     },
-///     ::kernel::bindings::mdio_device_id {
-///         phy_id: 0,
-///         phy_id_mask: 0,
-///     },
-/// ];
-/// #[cfg(MODULE)]
-/// #[no_mangle]
-/// static __mod_device_table__mdio__phydev: [::kernel::bindings::mdio_device_id; 2] = _DEVICE_TABLE;
+/// const N: usize = 1;
+///
+/// const TABLE: ::kernel::device_id::IdArray<::kernel::net::phy::DeviceId, (), N> =
+///     ::kernel::device_id::IdArray::new_without_index([
+///         ::kernel::net::phy::DeviceId(
+///             ::kernel::bindings::mdio_device_id {
+///                 phy_id: 0x00000001,
+///                 phy_id_mask: 0xffffffff,
+///             }),
+///     ]);
+///
+/// ::kernel::module_device_table!("mdio", phydev, TABLE);
 /// ```
 #[macro_export]
 macro_rules! module_phy_driver {
@@ -873,20 +878,12 @@ macro_rules! module_phy_driver {
     };
 
     (@device_table [$($dev:expr),+]) => {
-        // SAFETY: C will not read off the end of this constant since the last element is zero.
-        const _DEVICE_TABLE: [$crate::bindings::mdio_device_id;
-            $crate::module_phy_driver!(@count_devices $($dev),+) + 1] = [
-            $($dev.mdio_device_id()),+,
-            $crate::bindings::mdio_device_id {
-                phy_id: 0,
-                phy_id_mask: 0
-            }
-        ];
+        const N: usize = $crate::module_phy_driver!(@count_devices $($dev),+);
+
+        const TABLE: $crate::device_id::IdArray<$crate::net::phy::DeviceId, (), N> =
+            $crate::device_id::IdArray::new_without_index([ $(($dev,())),+, ]);
 
-        #[cfg(MODULE)]
-        #[no_mangle]
-        static __mod_device_table__mdio__phydev: [$crate::bindings::mdio_device_id;
-            $crate::module_phy_driver!(@count_devices $($dev),+) + 1] = _DEVICE_TABLE;
+        $crate::module_device_table!("mdio", phydev, TABLE);
     };
 
     (drivers: [$($driver:ident),+ $(,)?], device_table: [$($dev:expr),+ $(,)?], $($f:tt)*) => {
-- 
2.43.0

Re: [PATCH v3 0/3] rust: Build PHY device tables by using module_device_table macro

[Jakub Kicinski]

On Fri,  4 Jul 2025 13:10:00 +0900 FUJITA Tomonori wrote:
> The PHY abstractions have been generating their own device tables
> manually instead of using the module_device_table macro provided by
> the device_id crate. However, the format of device tables occasionally
> changes [1] [2], requiring updates to both the device_id crate and the custom
> format used by the PHY abstractions, which is cumbersome to maintain.

Does not apply to networking trees so I suspect someone else will take
these:

Acked-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH v3 0/3] rust: Build PHY device tables by using module_device_table macro

[Miguel Ojeda]

On Tue, Jul 8, 2025 at 2:53 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> Does not apply to networking trees so I suspect someone else will take
> these:
>
> Acked-by: Jakub Kicinski <kuba@kernel.org>

Thanks! Happy to take it through Rust tree if that is best.

Greg, Rob/Saravana: Acked-by's for auxiliary and OF (in patch #1)
appreciated, thanks!

Cheers,
Miguel

Re: [PATCH v3 0/3] rust: Build PHY device tables by using module_device_table macro

[Danilo Krummrich]

On 7/8/25 2:53 AM, Jakub Kicinski wrote:
> On Fri,  4 Jul 2025 13:10:00 +0900 FUJITA Tomonori wrote:
>> The PHY abstractions have been generating their own device tables
>> manually instead of using the module_device_table macro provided by
>> the device_id crate. However, the format of device tables occasionally
>> changes [1] [2], requiring updates to both the device_id crate and the custom
>> format used by the PHY abstractions, which is cumbersome to maintain.
> 
> Does not apply to networking trees so I suspect someone else will take
> these:
> 
> Acked-by: Jakub Kicinski <kuba@kernel.org>

I gave an Acked-by: for patch 1, but can also take it through the driver-core
tree.

- Danilo

Re: [PATCH v3 0/3] rust: Build PHY device tables by using module_device_table macro

[Danilo Krummrich]

On 7/8/25 12:45 PM, Miguel Ojeda wrote:
> On Tue, Jul 8, 2025 at 2:53 AM Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> Does not apply to networking trees so I suspect someone else will take
>> these:
>>
>> Acked-by: Jakub Kicinski <kuba@kernel.org>
> 
> Thanks! Happy to take it through Rust tree if that is best.

Ah, didn't get this one before I replied in [1]. I gave an ACK because I thought 
it's going through the networking tree, but I can also take it through driver-core.

Taking it through the rust tree is fine for me too; just let me know. :)

[1] https://lore.kernel.org/lkml/1b9754c7-b9ad-4b01-8476-3d8367a49635@kernel.org/

Re: [PATCH v3 0/3] rust: Build PHY device tables by using module_device_table macro

[FUJITA Tomonori]

On Tue, 8 Jul 2025 12:45:20 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Tue, Jul 8, 2025 at 2:53 AM Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> Does not apply to networking trees so I suspect someone else will take
>> these:
>>
>> Acked-by: Jakub Kicinski <kuba@kernel.org>
> 
> Thanks! Happy to take it through Rust tree if that is best.

This is based on Rust tree. If I remember correctly, it can't be
applied cleanly to other trees because of Tamir's patch in Rust tree.


Thanks everyone!

Re: [PATCH v3 0/3] rust: Build PHY device tables by using module_device_table macro

[Danilo Krummrich]

On Tue Jul 8, 2025 at 12:59 PM CEST, FUJITA Tomonori wrote:
> On Tue, 8 Jul 2025 12:45:20 +0200
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>
>> On Tue, Jul 8, 2025 at 2:53 AM Jakub Kicinski <kuba@kernel.org> wrote:
>>>
>>> Does not apply to networking trees so I suspect someone else will take
>>> these:
>>>
>>> Acked-by: Jakub Kicinski <kuba@kernel.org>
>> 
>> Thanks! Happy to take it through Rust tree if that is best.
>
> This is based on Rust tree. If I remember correctly, it can't be
> applied cleanly to other trees because of Tamir's patch in Rust tree.

Had a brief look.

There will be two trivial conflicts with the driver-core tree, which fixed up
some of the safety comments you modify in this series as well.

The other way around, there is one trivial conflict with Tamir patch in the rust
tree fixing up an `as _` cast.

So, either way works fine.

Re: [PATCH v3 0/3] rust: Build PHY device tables by using module_device_table macro

[Miguel Ojeda]

On Tue, Jul 8, 2025 at 2:48 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> Had a brief look.
>
> There will be two trivial conflicts with the driver-core tree, which fixed up
> some of the safety comments you modify in this series as well.
>
> The other way around, there is one trivial conflict with Tamir patch in the rust
> tree fixing up an `as _` cast.
>
> So, either way works fine.

Thanks Danilo -- ditto. Even netdev could make sense as you said.

Since it touched several subsystems and it is based on rust-next, I am
happy to do so, but driver-core makes sense given that is the main
change after all.

So if I don't see you picking it, I will eventually do it.

Cheers,
Miguel

Re: [PATCH v3 0/3] rust: Build PHY device tables by using module_device_table macro

[Danilo Krummrich]

On Tue, Jul 08, 2025 at 08:47:13PM +0200, Miguel Ojeda wrote:
> Thanks Danilo -- ditto. Even netdev could make sense as you said.
> 
> Since it touched several subsystems and it is based on rust-next, I am
> happy to do so, but driver-core makes sense given that is the main
> change after all.
> 
> So if I don't see you picking it, I will eventually do it.

Checked again and the driver-core tree makes most sense, since we also need to
fix up the ACPI device ID code, which is queued up in driver-core-next.

I also caught a missing change in rust/kernel/driver.rs, which most likely
slipped through by not building with CONFIG_OF. :)

Here's the diff to fix up both, I already fixed it up on my end -- no need to
send a new version.

--

diff --git a/rust/kernel/acpi.rs b/rust/kernel/acpi.rs
index 2af4d4f92924..7ae317368b00 100644
--- a/rust/kernel/acpi.rs
+++ b/rust/kernel/acpi.rs
@@ -2,7 +2,11 @@

 //! Advanced Configuration and Power Interface abstractions.

-use crate::{bindings, device_id::RawDeviceId, prelude::*};
+use crate::{
+    bindings,
+    device_id::{RawDeviceId, RawDeviceIdIndex},
+    prelude::*,
+};

 /// IdTable type for ACPI drivers.
 pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<DeviceId, T>;
@@ -12,13 +16,14 @@
 #[derive(Clone, Copy)]
 pub struct DeviceId(bindings::acpi_device_id);

-// SAFETY:
-// * `DeviceId` is a `#[repr(transparent)` wrapper of `struct acpi_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 `acpi_device_id` and does not add
+// additional invariants, so it's safe to transmute to `RawType`.
 unsafe impl RawDeviceId for DeviceId {
     type RawType = bindings::acpi_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::acpi_device_id, driver_data);

     fn index(&self) -> usize {
diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
index f8dd7593e8dc..573d516b2f06 100644
--- a/rust/kernel/driver.rs
+++ b/rust/kernel/driver.rs
@@ -170,7 +170,7 @@ fn acpi_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
                 // and does not add additional invariants, so it's safe to transmute.
                 let id = unsafe { &*raw_id.cast::<acpi::DeviceId>() };

-                Some(table.info(<acpi::DeviceId as crate::device_id::RawDeviceId>::index(id)))
+                Some(table.info(<acpi::DeviceId as crate::device_id::RawDeviceIdIndex>::index(id)))
             }
         }
     }
@@ -204,7 +204,7 @@ fn of_id_info(dev: &device::Device) -> Option<&'static Self::IdInfo> {
                 // and does not add additional invariants, so it's safe to transmute.
                 let id = unsafe { &*raw_id.cast::<of::DeviceId>() };

-                Some(table.info(<of::DeviceId as crate::device_id::RawDeviceId>::index(id)))
+                Some(table.info(<of::DeviceId as crate::device_id::RawDeviceIdIndex>::index(id)))
             }
         }
     }

Re: [PATCH v3 0/3] rust: Build PHY device tables by using module_device_table macro

[FUJITA Tomonori]

On Wed, 9 Jul 2025 00:51:24 +0200
Danilo Krummrich <dakr@kernel.org> wrote:

>> Since it touched several subsystems and it is based on rust-next, I am
>> happy to do so, but driver-core makes sense given that is the main
>> change after all.
>> 
>> So if I don't see you picking it, I will eventually do it.
> 
> Checked again and the driver-core tree makes most sense, since we also need to
> fix up the ACPI device ID code, which is queued up in driver-core-next.
> 
> I also caught a missing change in rust/kernel/driver.rs, which most likely
> slipped through by not building with CONFIG_OF. :)

Oops, you are right. I mistakenly thought that CONFIG_OF was enabled
because kernel/of.rs was being compiled.


> Here's the diff to fix up both, I already fixed it up on my end -- no need to
> send a new version.

Thanks a lot!

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

[Trevor Gross]

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>

Re: [PATCH v3 2/3] rust: net::phy represent DeviceId as transparent wrapper over mdio_device_id

[Trevor Gross]

On Fri Jul 4, 2025 at 12:10 AM EDT, FUJITA Tomonori wrote:
> Refactor the DeviceId struct to be a #[repr(transparent)] wrapper
> around the C struct bindings::mdio_device_id.
>
> This refactoring is a preparation for enabling the PHY abstractions to
> use device_id trait.

Should this say "the `DeviceId` trait" (different case)?

> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/net/phy.rs | 53 +++++++++++++++++++++---------------------
>  1 file changed, 27 insertions(+), 26 deletions(-)
>
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 65ac4d59ad77..940972ffadae 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> [...]
> @@ -734,18 +733,20 @@ pub const fn new_with_driver<T: Driver>() -> Self {
>          T::PHY_DEVICE_ID
>      }
>  
> +    /// Get a `phy_id` as u32.
> +    pub const fn id(&self) -> u32 {
> +        self.0.phy_id
> +    }

For the docs maybe just:

    /// Get the MDIO device's phy ID.

Since `as u32` is slightly redundant (it's in the return type, and that
is how it is stored anyway).

>      /// Get a `mask` as u32.
>      pub const fn mask_as_int(&self) -> u32 {
> -        self.mask.as_int()
> +        self.0.phy_id_mask
>      }

One optional nit then:

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

Re: [PATCH v3 3/3] rust: net::phy Change module_phy_driver macro to use module_device_table macro

[Trevor Gross]

On Fri Jul 4, 2025 at 12:10 AM EDT, FUJITA Tomonori wrote:
> Change module_phy_driver macro to build device tables which are
> exported to userspace by using module_device_table macro.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

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

Re: [PATCH v3 0/3] rust: Build PHY device tables by using module_device_table macro

[Danilo Krummrich]

On 7/9/25 4:08 AM, FUJITA Tomonori wrote:
> On Wed, 9 Jul 2025 00:51:24 +0200
> Danilo Krummrich <dakr@kernel.org> wrote:
>> Here's the diff to fix up both, I already fixed it up on my end -- no need to
>> send a new version.
> 
> Thanks a lot!

Given the comments from Trevor, do you want me to wait for a respin, consider
the few nits when applying, or apply as is?

- Danilo

Re: [PATCH v3 0/3] rust: Build PHY device tables by using module_device_table macro

[FUJITA Tomonori]

On Thu, 10 Jul 2025 22:01:56 +0200
Danilo Krummrich <dakr@kernel.org> wrote:

> On 7/9/25 4:08 AM, FUJITA Tomonori wrote:
>> On Wed, 9 Jul 2025 00:51:24 +0200
>> Danilo Krummrich <dakr@kernel.org> wrote:
>>> Here's the diff to fix up both, I already fixed it up on my end -- no
>>> need to
>>> send a new version.
>> Thanks a lot!
> 
> Given the comments from Trevor, do you want me to wait for a respin,
> consider
> the few nits when applying, or apply as is?

Thanks, I'll send v4 shortly.

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

[FUJITA Tomonori]

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!

Re: [PATCH v3 2/3] rust: net::phy represent DeviceId as transparent wrapper over mdio_device_id

[FUJITA Tomonori]

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

> On Fri Jul 4, 2025 at 12:10 AM EDT, FUJITA Tomonori wrote:
>> Refactor the DeviceId struct to be a #[repr(transparent)] wrapper
>> around the C struct bindings::mdio_device_id.
>>
>> This refactoring is a preparation for enabling the PHY abstractions to
>> use device_id trait.
> 
> Should this say "the `DeviceId` trait" (different case)?

Ah, I changed it to the RawDeviceId trait.

>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> ---
>>  rust/kernel/net/phy.rs | 53 +++++++++++++++++++++---------------------
>>  1 file changed, 27 insertions(+), 26 deletions(-)
>>
>> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
>> index 65ac4d59ad77..940972ffadae 100644
>> --- a/rust/kernel/net/phy.rs
>> +++ b/rust/kernel/net/phy.rs
>> [...]
>> @@ -734,18 +733,20 @@ pub const fn new_with_driver<T: Driver>() -> Self {
>>          T::PHY_DEVICE_ID
>>      }
>>  
>> +    /// Get a `phy_id` as u32.
>> +    pub const fn id(&self) -> u32 {
>> +        self.0.phy_id
>> +    }
> 
> For the docs maybe just:
> 
>     /// Get the MDIO device's phy ID.
> 
> Since `as u32` is slightly redundant (it's in the return type, and that
> is how it is stored anyway).

Yeah, fixed. I used "PHY" for consistency with other comments.

>>      /// Get a `mask` as u32.
>>      pub const fn mask_as_int(&self) -> u32 {
>> -        self.mask.as_int()
>> +        self.0.phy_id_mask
>>      }

I also updated the above comment

/// Get the MDIO device's match mask.

> One optional nit then:
> 
> Reviewed-by: Trevor Gross <tmgross@umich.edu>

Thanks!