[PATCH v1 1/3] rust: device_id: make DRIVER_DATA_OFFSET optional

FUJITA Tomonori posted 3 patches 3 months, 2 weeks ago
There is a newer version of this series
[PATCH v1 1/3] rust: device_id: make DRIVER_DATA_OFFSET optional
Posted by FUJITA Tomonori 3 months, 2 weeks ago
Enable support for device ID structures that do not contain
context/data field (usually named `driver_data`), making the trait
usable in a wider range of subsystems and buses.

Several such structures are defined in
include/linux/mod_devicetable.h.

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/auxiliary.rs |  6 ++++--
 rust/kernel/device_id.rs | 26 +++++++++++++++-----------
 rust/kernel/of.rs        |  3 ++-
 rust/kernel/pci.rs       |  3 ++-
 4 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs
index d2cfe1eeefb6..7b8798599128 100644
--- a/rust/kernel/auxiliary.rs
+++ b/rust/kernel/auxiliary.rs
@@ -147,8 +147,10 @@ pub const fn new(modname: &'static CStr, name: &'static CStr) -> Self {
 unsafe impl RawDeviceId for DeviceId {
     type RawType = bindings::auxiliary_device_id;
 
-    const DRIVER_DATA_OFFSET: usize =
-        core::mem::offset_of!(bindings::auxiliary_device_id, driver_data);
+    const DRIVER_DATA_OFFSET: Option<usize> = Some(core::mem::offset_of!(
+        bindings::auxiliary_device_id,
+        driver_data
+    ));
 
     fn index(&self) -> usize {
         self.0.driver_data
diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
index 3dc72ca8cfc2..b7d00587a0e2 100644
--- a/rust/kernel/device_id.rs
+++ b/rust/kernel/device_id.rs
@@ -25,8 +25,9 @@
 ///     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.
+///   - If [`RawDeviceId::DRIVER_DATA_OFFSET`] is `Some(offset)`, it's 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.
@@ -37,7 +38,7 @@ pub unsafe trait RawDeviceId {
     type RawType: Copy;
 
     /// The offset to the context/data field.
-    const DRIVER_DATA_OFFSET: usize;
+    const DRIVER_DATA_OFFSET: Option<usize> = None;
 
     /// The index stored at `DRIVER_DATA_OFFSET` of the implementor of the [`RawDeviceId`] trait.
     fn index(&self) -> usize;
@@ -77,14 +78,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(data_offset) = T::DRIVER_DATA_OFFSET {
+                // 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(data_offset)
+                        .cast::<usize>()
+                        .write(i);
+                }
             }
 
             // SAFETY: this is effectively a move: `infos[i] = ids[i].1`. We make a copy here but
diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs
index 40d1bd13682c..0ca1692d61f3 100644
--- a/rust/kernel/of.rs
+++ b/rust/kernel/of.rs
@@ -19,7 +19,8 @@
 unsafe impl RawDeviceId for DeviceId {
     type RawType = bindings::of_device_id;
 
-    const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::of_device_id, data);
+    const DRIVER_DATA_OFFSET: Option<usize> =
+        Some(core::mem::offset_of!(bindings::of_device_id, data));
 
     fn index(&self) -> usize {
         self.0.data as usize
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index f6b19764ad17..dea49abe7cd3 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -168,7 +168,8 @@ pub const fn from_class(class: u32, class_mask: u32) -> Self {
 unsafe impl RawDeviceId for DeviceId {
     type RawType = bindings::pci_device_id;
 
-    const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::pci_device_id, driver_data);
+    const DRIVER_DATA_OFFSET: Option<usize> =
+        Some(core::mem::offset_of!(bindings::pci_device_id, driver_data));
 
     fn index(&self) -> usize {
         self.0.driver_data
-- 
2.43.0
Re: [PATCH v1 1/3] rust: device_id: make DRIVER_DATA_OFFSET optional
Posted by Danilo Krummrich 3 months, 1 week ago
On 6/23/25 8:09 AM, FUJITA Tomonori wrote:
> Enable support for device ID structures that do not contain
> context/data field (usually named `driver_data`), making the trait
> usable in a wider range of subsystems and buses.
> 
> Several such structures are defined in
> include/linux/mod_devicetable.h.
> 
> This refactoring is a preparation for enabling the PHY abstractions to
> use device_id trait.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Acked-by: Danilo Krummrich <dakr@kernel.org>
Re: [PATCH v1 1/3] rust: device_id: make DRIVER_DATA_OFFSET optional
Posted by FUJITA Tomonori 3 months, 1 week ago
On Fri, 4 Jul 2025 00:15:19 +0200
Danilo Krummrich <dakr@kernel.org> wrote:

> On 6/23/25 8:09 AM, FUJITA Tomonori wrote:
>> Enable support for device ID structures that do not contain
>> context/data field (usually named `driver_data`), making the trait
>> usable in a wider range of subsystems and buses.
>> Several such structures are defined in
>> include/linux/mod_devicetable.h.
>> This refactoring is a preparation for enabling the PHY abstractions to
>> use device_id trait.
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> Acked-by: Danilo Krummrich <dakr@kernel.org>

Thanks a lot!

Miguel suggested that splitting the RawDeviceId trait might lead to a
cleaner design, and I also tried that approach [v2]. But just to
confirm ― do you prefer the original v1 approach instead?

https://lore.kernel.org/lkml/CANiq72k0sdUoBxVYghgh50+ZRV2gbDkgVjuZgJLtj=4s9852xg@mail.gmail.com/

[v2]: https://lore.kernel.org/rust-for-linux/20250701141252.600113-1-fujita.tomonori@gmail.com/ 

Either way works for me.

Sorry Miguel, I forgot to fix the comment typo you pointed out in v1. I'll correct it in v3.
Re: [PATCH v1 1/3] rust: device_id: make DRIVER_DATA_OFFSET optional
Posted by Danilo Krummrich 3 months, 1 week ago
On Fri, Jul 04, 2025 at 08:41:59AM +0900, FUJITA Tomonori wrote:
> On Fri, 4 Jul 2025 00:15:19 +0200
> Danilo Krummrich <dakr@kernel.org> wrote:
> 
> > On 6/23/25 8:09 AM, FUJITA Tomonori wrote:
> >> Enable support for device ID structures that do not contain
> >> context/data field (usually named `driver_data`), making the trait
> >> usable in a wider range of subsystems and buses.
> >> Several such structures are defined in
> >> include/linux/mod_devicetable.h.
> >> This refactoring is a preparation for enabling the PHY abstractions to
> >> use device_id trait.
> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > 
> > Acked-by: Danilo Krummrich <dakr@kernel.org>
> 
> Thanks a lot!
> 
> Miguel suggested that splitting the RawDeviceId trait might lead to a
> cleaner design, and I also tried that approach [v2]. But just to
> confirm ― do you prefer the original v1 approach instead?

Sorry, I didn't catch v2.

From only looking at patch 1, I indeed prefer v1.

> https://lore.kernel.org/lkml/CANiq72k0sdUoBxVYghgh50+ZRV2gbDkgVjuZgJLtj=4s9852xg@mail.gmail.com/

Looking at this reply to patch 3, I can see why Miguel suggested it though.
Given that, I think splitting the trait is the correct thing to do. I'll reply
to v2.

> [v2]: https://lore.kernel.org/rust-for-linux/20250701141252.600113-1-fujita.tomonori@gmail.com/ 
> 
> Either way works for me.
> 
> Sorry Miguel, I forgot to fix the comment typo you pointed out in v1. I'll correct it in v3.