Message-Id: <DEZKDEPMCQ7N.1VK1WNX9B1ZI7@kernel.org>
Date: Tue, 16 Dec 2025 11:20:39 +0100
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Matthew Maurer" <mmaurer@...gle.com>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Boqun Feng" <boqun.feng@...il.com>,
 "Gary Guo" <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "Greg Kroah-Hartman"
 <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
 <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v2 1/3] rust: Add soc_device support

On Tue Dec 16, 2025 at 1:43 AM CET, Matthew Maurer wrote:
> +/// Attributes for a SoC device.
> +///
> +/// These are both exported to userspace under /sys/devices/socX and provided to other drivers to
> +/// match against via `soc_device_match` (not yet available in Rust) to enable quirks or
> +/// device-specific support where necessary.
> +///
> +/// All fields are freeform - they have no specific formatting, just defined meanings.
> +/// For example, the [`machine`](`Attributes::machine`) field could be "DB8500" or
> +/// "Qualcomm Technologies, Inc. SM8560 HDK", but regardless it should identify a board or product.
> +pub struct Attributes {
> +    /// Should generally be a board ID or product ID. Examples
> +    /// include DB8500 (ST-Ericsson) or "Qualcomm Technologies, inc. SM8560 HDK".
> +    ///
> +    /// If this field is not populated, the SoC infrastructure will try to populate it from
> +    /// `/model` in the device tree.
> +    pub machine: Option<CString>,
> +    /// The broader class this SoC belongs to. Examples include ux500
> +    /// (for DB8500) or Snapdragon (for SM8650).
> +    ///
> +    /// On chips with ARM firmware supporting SMCCC v1.2+, this may be a JEDEC JEP106 manufacturer
> +    /// identification.
> +    pub family: Option<CString>,
> +    /// The manufacturing revision of the part. Frequently this is MAJOR.MINOR, but not always.
> +    pub revision: Option<CString>,
> +    /// Serial Number - uniquely identifies a specific SoC. If present, should be unique (buying a
> +    /// replacement part should change it if present). This field cannot be matched on and is
> +    /// solely present to export through /sys.
> +    pub serial_number: Option<CString>,
> +    /// SoC ID - identifies a specific SoC kind in question, sometimes more specifically than
> +    /// `machine` if the same SoC is used in multiple products. Some devices use this to specify a
> +    /// SoC name, e.g. "I.MX??", and others just print an ID number (e.g. Tegra and Qualcomm).
> +    ///
> +    /// On chips with ARM firmware supporting SMCCC v1.2+, this may be a JEDEC JEP106 manufacturer
> +    /// identification (the family value) followed by a colon and then a 4-digit ID value.
> +    pub soc_id: Option<CString>,
> +}

Thanks for expanding the documentation!

> +struct BuiltAttributes {
> +    // While `inner` has pointers to `_backing`, it is to the interior of the `CStrings`, not
> +    // `backing` itself, so it does not need to be pinned.
> +    _backing: Attributes,
> +    // `Opaque` makes us `!Unpin`, as the registration holds a pointer to `inner` when used.
> +    inner: Opaque<bindings::soc_device_attribute>,
> +}
> +
> +fn cstring_to_c(mcs: &Option<CString>) -> *const kernel::ffi::c_char {
> +    mcs.as_ref()
> +        .map(|cs| cs.as_char_ptr())
> +        .unwrap_or(core::ptr::null())
> +}
> +
> +impl BuiltAttributes {
> +    fn as_mut_ptr(&self) -> *mut bindings::soc_device_attribute {
> +        self.inner.get()
> +    }
> +}
> +
> +impl Attributes {
> +    fn build(self) -> BuiltAttributes {
> +        BuiltAttributes {
> +            inner: Opaque::new(bindings::soc_device_attribute {
> +                machine: cstring_to_c(&self.machine),
> +                family: cstring_to_c(&self.family),
> +                revision: cstring_to_c(&self.revision),
> +                serial_number: cstring_to_c(&self.serial_number),
> +                soc_id: cstring_to_c(&self.soc_id),
> +                data: core::ptr::null(),
> +                custom_attr_group: core::ptr::null(),
> +            }),
> +            _backing: self,
> +        }
> +    }
> +}
> +
> +/// # Safety
> +/// If a device is returned (e.g. no error), `attr` must remain valid for reads until the
> +/// returned pointer is released through `soc_device_unregister`.
> +unsafe fn register_device(attr: Pin<&BuiltAttributes>) -> Result<NonNull<bindings::soc_device>> {
> +    let raw_soc =
> +            // SAFETY:
> +            // * The struct provided through attr is backed by pinned data next to it, so as
> +            //   long as attr lives, the strings pointed to by the struct will too.
> +            // * `attr` is pinned, so the pinned data won't move.
> +            // * If it returns a device, and so others may try to read this data, by caller
> +            //   invariant, `attr` won't be released until the device is.
> +            error::from_err_ptr(unsafe { bindings::soc_device_register(attr.as_mut_ptr()) })?;
> +    // `soc_device_register` should not return NULL, but it doesn't hurt to be paranoid.
> +    NonNull::new(raw_soc).ok_or(EINVAL)
> +}

I think it turns out cleaner without this helper function, inlining the
contained code directly into Registration::new().

> +#[pin_data(PinnedDrop)]
> +/// Registration handle for your soc_dev. If you let it go out of scope, your soc_dev will be
> +/// unregistered.
> +pub struct Registration {
> +    #[pin]
> +    attr: BuiltAttributes,
> +    soc_dev: NonNull<bindings::soc_device>,
> +}
> +
> +// SAFETY: We provide no operations through `&Registration`.
> +unsafe impl Sync for Registration {}
> +
> +// SAFETY: All pointers are normal allocations, not thread-specific.
> +unsafe impl Send for Registration {}
> +
> +#[pinned_drop]
> +impl PinnedDrop for Registration {
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY: Device always contains a live pointer to a soc_device that can be unregistered
> +        unsafe { bindings::soc_device_unregister(self.soc_dev.as_ptr()) }
> +    }
> +}
> +
> +impl Registration {
> +    /// Register a new SoC device
> +    pub fn register(attr: Attributes) -> impl PinInit<Self, Error> {

Let's just call this Registration::new() please. We usually use new() if we
return a Registration object (or an initializer as in this case) and register()
if we do not return a Registration object, but rather automatically clean up the
registration silently, e.g. through devres.

> +        try_pin_init!(&this in Self {

You should be able to just access Self::attr directly, i.e. no need for &this.
(When you access attr within the code block of soc_dev it will be Self::attr and
not the attr from the function argument.)

Please find a diff [1] below.

> +                    attr: attr.build(),
> +                    // SAFETY: We have already initialized attr, and we are inside PinInit and Self
> +                    // is !Unpin, so attr won't be moved and is valid. If it returns success, attr
> +                    // will not be dropped until after our `PinnedDrop` implementation runs, so the
> +                    // device will be unregistered first.
> +                    soc_dev: unsafe {
> +                        register_device(Pin::new_unchecked(&(*this.as_ptr()).attr))?
> +                    },
> +        }? Error)
> +    }
> +}


[1]

diff --git a/rust/kernel/soc.rs b/rust/kernel/soc.rs
index fb9e46121878..242dcd09e7f5 100644
--- a/rust/kernel/soc.rs
+++ b/rust/kernel/soc.rs
@@ -89,22 +89,6 @@ fn build(self) -> BuiltAttributes {
     }
 }
 
-/// # Safety
-/// If a device is returned (e.g. no error), `attr` must remain valid for reads until the
-/// returned pointer is released through `soc_device_unregister`.
-unsafe fn register_device(attr: Pin<&BuiltAttributes>) -> Result<NonNull<bindings::soc_device>> {
-    let raw_soc =
-            // SAFETY:
-            // * The struct provided through attr is backed by pinned data next to it, so as
-            //   long as attr lives, the strings pointed to by the struct will too.
-            // * `attr` is pinned, so the pinned data won't move.
-            // * If it returns a device, and so others may try to read this data, by caller
-            //   invariant, `attr` won't be released until the device is.
-            error::from_err_ptr(unsafe { bindings::soc_device_register(attr.as_mut_ptr()) })?;
-    // `soc_device_register` should not return NULL, but it doesn't hurt to be paranoid.
-    NonNull::new(raw_soc).ok_or(EINVAL)
-}
-
 #[pin_data(PinnedDrop)]
 /// Registration handle for your soc_dev. If you let it go out of scope, your soc_dev will be
 /// unregistered.
@@ -131,14 +115,24 @@ fn drop(self: Pin<&mut Self>) {
 impl Registration {
     /// Register a new SoC device
     pub fn register(attr: Attributes) -> impl PinInit<Self, Error> {
-        try_pin_init!(&this in Self {
+        try_pin_init!(Self {
                     attr: attr.build(),
                     // SAFETY: We have already initialized attr, and we are inside PinInit and Self
                     // is !Unpin, so attr won't be moved and is valid. If it returns success, attr
                     // will not be dropped until after our `PinnedDrop` implementation runs, so the
                     // device will be unregistered first.
-                    soc_dev: unsafe {
-                        register_device(Pin::new_unchecked(&(*this.as_ptr()).attr))?
+                    soc_dev: {
+                        // SAFETY:
+                        // * The struct provided through attr is backed by pinned data next to it,
+                        //   so as long as attr lives, the strings pointed to by the struct will too.
+                        // * `attr` is pinned, so the pinned data won't move.
+                        // * If it returns a device, and so others may try to read this data, by
+                        //   caller invariant, `attr` won't be released until the device is.
+                        let raw_soc = error::from_err_ptr(unsafe {
+                            bindings::soc_device_register(attr.as_mut_ptr())
+                        })?;
+
+                        NonNull::new(raw_soc).ok_or(EINVAL)?
                     },
         }? Error)
     }

Powered by blists - more mailing lists