Message-Id: <DDUYV4ETTD50.3UCGLW45AK740@kernel.org>
Date: Wed, 29 Oct 2025 18:02:45 +0100
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Alice Ryhl" <aliceryhl@...gle.com>
Cc: <gregkh@...uxfoundation.org>, <rafael@...nel.org>,
 <bhelgaas@...gle.com>, <kwilczynski@...nel.org>,
 <david.m.ertman@...el.com>, <ira.weiny@...el.com>, <leon@...nel.org>,
 <acourbot@...dia.com>, <ojeda@...nel.org>, <alex.gaynor@...il.com>,
 <boqun.feng@...il.com>, <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>,
 <lossin@...nel.org>, <a.hindborg@...nel.org>, <tmgross@...ch.edu>,
 <pcolberg@...hat.com>, <rust-for-linux@...r.kernel.org>,
 <linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/8] rust: device: introduce Device::drvdata()

On Wed Oct 29, 2025 at 4:30 PM CET, Danilo Krummrich wrote:
> On Wed Oct 29, 2025 at 1:59 PM CET, Alice Ryhl wrote:
>> Are you going to open that docs PR to the Rust compiler about the size
>> of TypeID that we talked about? :)
>
> Yes, I will -- thanks for the reminder.
>
>> Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>
>>
>>> +// Compile-time checks.
>>> +const _: () = {
>>> +    // Assert that we can `read()` / `write()` a `TypeId` instance from / into `struct driver_type`.
>>> +    static_assert!(core::mem::size_of::<bindings::driver_type>() == core::mem::size_of::<TypeId>());
>>> +};
>>
>> You don't need the "const _: ()" part. See the definition of
>> static_assert! to see why.
>
> Indeed, good catch -- same for the suggestions below.
>
>> Also, I would not require equality. The Rust team did not think that it
>> would ever increase in size, but it may decrease.
>>
>>>  /// The core representation of a device in the kernel's driver model.
>>>  ///
>>>  /// This structure represents the Rust abstraction for a C `struct device`. A [`Device`] can either
>>> @@ -198,12 +204,29 @@ pub unsafe fn as_bound(&self) -> &Device<Bound> {
>>>  }
>>>  
>>>  impl Device<CoreInternal> {
>>> +    fn type_id_store<T: 'static>(&self) {
>>
>> This name isn't great. How about "set_type_id()" instead?

Here's the diff, including a missing check in case someone tries to call
Device::drvdata() from probe().

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 36c6eec0ceab..1a307be953c2 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -17,11 +17,8 @@

 pub mod property;

-// Compile-time checks.
-const _: () = {
-    // Assert that we can `read()` / `write()` a `TypeId` instance from / into `struct driver_type`.
-    static_assert!(core::mem::size_of::<bindings::driver_type>() == core::mem::size_of::<TypeId>());
-};
+// Assert that we can `read()` / `write()` a `TypeId` instance from / into `struct driver_type`.
+static_assert!(core::mem::size_of::<bindings::driver_type>() >= core::mem::size_of::<TypeId>());

 /// The core representation of a device in the kernel's driver model.
 ///
@@ -204,7 +201,7 @@ pub unsafe fn as_bound(&self) -> &Device<Bound> {
 }

 impl Device<CoreInternal> {
-    fn type_id_store<T: 'static>(&self) {
+    fn set_type_id<T: 'static>(&self) {
         // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
         let private = unsafe { (*self.as_raw()).p };

@@ -226,7 +223,7 @@ pub fn set_drvdata<T: 'static>(&self, data: impl PinInit<T, Error>) -> Result {

         // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
         unsafe { bindings::dev_set_drvdata(self.as_raw(), data.into_foreign().cast()) };
-        self.type_id_store::<T>();
+        self.set_type_id::<T>();

         Ok(())
     }
@@ -242,6 +239,9 @@ pub unsafe fn drvdata_obtain<T: 'static>(&self) -> Pin<KBox<T>> {
         // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
         let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };

+        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
+        unsafe { bindings::dev_set_drvdata(self.as_raw(), core::ptr::null_mut()) };
+
         // SAFETY:
         // - By the safety requirements of this function, `ptr` comes from a previous call to
         //   `into_foreign()`.
@@ -286,7 +286,7 @@ unsafe fn drvdata_unchecked<T: 'static>(&self) -> Pin<&T> {
         unsafe { Pin::<KBox<T>>::borrow(ptr.cast()) }
     }

-    fn type_id_match<T: 'static>(&self) -> Result {
+    fn match_type_id<T: 'static>(&self) -> Result {
         // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
         let private = unsafe { (*self.as_raw()).p };

@@ -311,11 +311,16 @@ fn type_id_match<T: 'static>(&self) -> Result {
     /// Returns a pinned reference to the driver's private data or [`EINVAL`] if it doesn't match
     /// the asserted type `T`.
     pub fn drvdata<T: 'static>(&self) -> Result<Pin<&T>> {
-        self.type_id_match::<T>()?;
+        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
+        if unsafe { bindings::dev_get_drvdata(self.as_raw()) }.is_null() {
+            return Err(ENOENT);
+        }
+
+        self.match_type_id::<T>()?;

         // SAFETY:
-        // - The `Bound` device context guarantees that this is only ever call after a call
-        //   to `set_drvdata()` and before `drvdata_obtain()`.
+        // - The above check of `dev_get_drvdata()` guarantees that we are called after
+        //   `set_drvdata()` and before `drvdata_obtain()`.
         // - We've just checked that the type of the driver's private data is in fact `T`.
         Ok(unsafe { self.drvdata_unchecked() })
     }

Powered by blists - more mailing lists