[PATCH v4 2/3] rust: i2c: add manual I2C device creation abstractions

Igor Korotin posted 3 patches 1 month, 2 weeks ago
There is a newer version of this series
[PATCH v4 2/3] rust: i2c: add manual I2C device creation abstractions
Posted by Igor Korotin 1 month, 2 weeks ago
In addition to the basic I2C device support, added rust abstractions
upon `i2c_new_client_device`/`i2c_unregister_device` C functions.

Implement the core abstractions needed for manual creation/deletion
of I2C devices, including:

 * `i2c::Registration` — a NonNull pointer created by the function
                          `i2c_new_client_device`

 * `i2c::I2cAdapter` — a ref counted wrapper around `struct i2c_adapter`

 * `i2c::I2cBoardInfo` — a safe wrapper around `struct i2c_board_info`

Signed-off-by: Igor Korotin <igor.korotin.linux@gmail.com>
---
 rust/kernel/i2c.rs | 175 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 174 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
index f5e8c4bc1b7d..851d20ec65b5 100644
--- a/rust/kernel/i2c.rs
+++ b/rust/kernel/i2c.rs
@@ -13,7 +13,10 @@
     types::Opaque,
 };
 
-use core::{marker::PhantomData, ptr::NonNull};
+use core::{
+    marker::PhantomData,
+    ptr::{from_ref, NonNull},
+};
 
 /// An I2C device id table.
 #[repr(transparent)]
@@ -316,6 +319,134 @@ fn shutdown(dev: &I2cClient<device::Core>) {
     }
 }
 
+/// The i2c adapter representation.
+///
+/// This structure represents the Rust abstraction for a C `struct i2c_adapter`. The
+/// implementation abstracts the usage of an existing C `struct i2c_adapter` that
+/// gets passed from the C side
+///
+/// # Invariants
+///
+/// A [`I2cAdapter`] instance represents a valid `struct i2c_adapter` created by the C portion of
+/// the kernel.
+#[repr(transparent)]
+pub struct I2cAdapter<Ctx: device::DeviceContext = device::Normal>(
+    Opaque<bindings::i2c_adapter>,
+    PhantomData<Ctx>,
+);
+
+impl<Ctx: device::DeviceContext> I2cAdapter<Ctx> {
+    fn as_raw(&self) -> *mut bindings::i2c_adapter {
+        self.0.get()
+    }
+
+    /// Gets pointer to an `i2c_adapter` by index.
+    pub fn get(index: i32) -> Result<&'static Self> {
+        // SAFETY: `index` must refer to a valid I2C adapter; the kernel
+        // guarantees that `i2c_get_adapter(index)` returns either a valid
+        // pointer or NULL. `NonNull::new` guarantees the correct check.
+        let adapter = NonNull::new(unsafe { bindings::i2c_get_adapter(index) }).ok_or(ENODEV)?;
+
+        // SAFETY: `adapter` is non-null and points to a live `i2c_adapter`.
+        // `I2cAdapter` is #[repr(transparent)], so this cast is valid.
+        Ok(unsafe { adapter.cast::<Self>().as_ref() })
+    }
+}
+
+impl<Ctx: device::DeviceContext> Drop for I2cAdapter<Ctx> {
+    fn drop(&mut self) {
+        // SAFETY: This `I2cAdapter` was obtained from `i2c_get_adapter`,
+        // and calling `i2c_put_adapter` exactly once will correctly release
+        // the reference count in the I2C core. It is safe to call from any context
+        unsafe { bindings::i2c_put_adapter(self.as_raw()) }
+    }
+}
+
+// SAFETY: `I2cAdapter` is a transparent wrapper of a type that doesn't depend on `I2cAdapter`'s
+// generic argument.
+kernel::impl_device_context_deref!(unsafe { I2cAdapter });
+kernel::impl_device_context_into_aref!(I2cAdapter);
+
+// SAFETY: Instances of `I2cAdapter` are always reference-counted.
+unsafe impl crate::types::AlwaysRefCounted for I2cAdapter {
+    fn inc_ref(&self) {
+        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
+        unsafe { bindings::get_device(self.as_ref().as_raw()) };
+    }
+
+    unsafe fn dec_ref(obj: NonNull<Self>) {
+        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
+        unsafe { bindings::put_device(&raw mut (*obj.as_ref().as_raw()).dev) }
+    }
+}
+
+impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for I2cAdapter<Ctx> {
+    fn as_ref(&self) -> &device::Device<Ctx> {
+        let raw = self.as_raw();
+        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
+        // `struct i2c_adapter`.
+        let dev = unsafe { &raw mut (*raw).dev };
+
+        // SAFETY: `dev` points to a valid `struct device`.
+        unsafe { device::Device::from_raw(dev) }
+    }
+}
+
+impl<Ctx: device::DeviceContext> TryFrom<&device::Device<Ctx>> for &I2cAdapter<Ctx> {
+    type Error = kernel::error::Error;
+
+    fn try_from(dev: &device::Device<Ctx>) -> Result<Self, Self::Error> {
+        // SAFETY: By the type invariant of `Device`, `dev.as_raw()` is a valid pointer to a
+        // `struct device`.
+        if unsafe { bindings::i2c_verify_adapter(dev.as_raw()).is_null() } {
+            return Err(EINVAL);
+        }
+
+        // SAFETY: We've just verified that the type of `dev` equals to
+        // `bindings::i2c_adapter_type`, hence `dev` must be embedded in a valid
+        // `struct i2c_adapter` as guaranteed by the corresponding C code.
+        let idev = unsafe { container_of!(dev.as_raw(), bindings::i2c_adapter, dev) };
+
+        // SAFETY: `idev` is a valid pointer to a `struct i2c_adapter`.
+        Ok(unsafe { &*idev.cast() })
+    }
+}
+
+/// The i2c board info representation
+///
+/// This structure represents the Rust abstraction for a C `struct i2c_board_info` structure,
+/// which is used for manual I2C client creation.
+#[repr(transparent)]
+pub struct I2cBoardInfo(bindings::i2c_board_info);
+
+impl I2cBoardInfo {
+    const I2C_TYPE_SIZE: usize = 20;
+    /// Create a new board‐info for a kernel driver.
+    #[inline(always)]
+    pub const fn new(type_: &'static CStr, addr: u16) -> Self {
+        build_assert!(
+            type_.len_with_nul() <= Self::I2C_TYPE_SIZE,
+            "Type exceeds 20 bytes"
+        );
+        let src = type_.as_bytes_with_nul();
+        // Replace with `bindings::acpi_device_id::default()` once stabilized for `const`.
+        // SAFETY: FFI type is valid to be zero-initialized.
+        let mut i2c_board_info: bindings::i2c_board_info = unsafe { core::mem::zeroed() };
+        let mut i: usize = 0;
+        while i < src.len() {
+            i2c_board_info.type_[i] = src[i];
+            i += 1;
+        }
+
+        i2c_board_info.addr = addr;
+        Self(i2c_board_info)
+    }
+
+    fn as_raw(&self) -> *const bindings::i2c_board_info {
+        from_ref(&self.0)
+    }
+}
+
 /// The i2c client representation.
 ///
 /// This structure represents the Rust abstraction for a C `struct i2c_client`. The
@@ -394,3 +525,45 @@ unsafe impl Send for I2cClient {}
 // SAFETY: `I2cClient` can be shared among threads because all methods of `I2cClient`
 // (i.e. `I2cClient<Normal>) are thread safe.
 unsafe impl Sync for I2cClient {}
+
+/// The registration of an i2c client device.
+///
+/// This type represents the registration of a [`struct i2c_client`]. When an instance of this
+/// type is dropped, its respective i2c client device will be unregistered from the system.
+///
+/// # Invariants
+///
+/// `self.0` always holds a valid pointer to an initialized and registered
+/// [`struct i2c_client`].
+#[repr(transparent)]
+pub struct Registration(NonNull<bindings::i2c_client>);
+
+impl Registration {
+    /// The C `i2c_new_client_device` function wrapper for manual I2C client creation.
+    pub fn new(i2c_adapter: &I2cAdapter, i2c_board_info: &I2cBoardInfo) -> Result<Self> {
+        // SAFETY: the kernel guarantees that `i2c_new_client_device()` returns either a valid
+        // pointer or NULL. `from_err_ptr` separates errors. Following `NonNull::new` checks
+        // for NULL.
+        let raw_dev = from_err_ptr(unsafe {
+            bindings::i2c_new_client_device(i2c_adapter.as_raw(), i2c_board_info.as_raw())
+        })?;
+
+        let dev_ptr = NonNull::new(raw_dev).ok_or(ENODEV)?;
+
+        Ok(Self(dev_ptr))
+    }
+}
+
+impl Drop for Registration {
+    fn drop(&mut self) {
+        // SAFETY: `Drop` is only called for a valid `Registration`, which by invariant
+        // always contains a non-null pointer to an `i2c_client`.
+        unsafe { bindings::i2c_unregister_device(self.0.as_ptr()) }
+    }
+}
+
+// SAFETY: A `Registration` of a `struct i2c_client` can be released from any thread.
+unsafe impl Send for Registration {}
+
+// SAFETY: `Registration` does not expose any methods or fields that need synchronization.
+unsafe impl Sync for Registration {}
-- 
2.43.0

Re: [PATCH v4 2/3] rust: i2c: add manual I2C device creation abstractions
Posted by Daniel Almeida 1 month, 1 week ago
Hi Igor,

> On 20 Aug 2025, at 12:21, Igor Korotin <igor.korotin.linux@gmail.com> wrote:
> 
> In addition to the basic I2C device support, added rust abstractions
> upon `i2c_new_client_device`/`i2c_unregister_device` C functions.

Can you use imperative voice here?

> 
> Implement the core abstractions needed for manual creation/deletion
> of I2C devices, including:

Like this ^

> 
> * `i2c::Registration` — a NonNull pointer created by the function
>                          `i2c_new_client_device`
> 
> * `i2c::I2cAdapter` — a ref counted wrapper around `struct i2c_adapter`
> 
> * `i2c::I2cBoardInfo` — a safe wrapper around `struct i2c_board_info`
> 
> Signed-off-by: Igor Korotin <igor.korotin.linux@gmail.com>
> ---
> rust/kernel/i2c.rs | 175 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 174 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
> index f5e8c4bc1b7d..851d20ec65b5 100644
> --- a/rust/kernel/i2c.rs
> +++ b/rust/kernel/i2c.rs
> @@ -13,7 +13,10 @@
>     types::Opaque,
> };
> 
> -use core::{marker::PhantomData, ptr::NonNull};
> +use core::{
> +    marker::PhantomData,
> +    ptr::{from_ref, NonNull},
> +};
> 
> /// An I2C device id table.
> #[repr(transparent)]
> @@ -316,6 +319,134 @@ fn shutdown(dev: &I2cClient<device::Core>) {
>     }
> }
> 
> +/// The i2c adapter representation.
> +///
> +/// This structure represents the Rust abstraction for a C `struct i2c_adapter`. The
> +/// implementation abstracts the usage of an existing C `struct i2c_adapter` that
> +/// gets passed from the C side
> +///
> +/// # Invariants
> +///
> +/// A [`I2cAdapter`] instance represents a valid `struct i2c_adapter` created by the C portion of
> +/// the kernel.
> +#[repr(transparent)]
> +pub struct I2cAdapter<Ctx: device::DeviceContext = device::Normal>(
> +    Opaque<bindings::i2c_adapter>,
> +    PhantomData<Ctx>,
> +);
> +
> +impl<Ctx: device::DeviceContext> I2cAdapter<Ctx> {
> +    fn as_raw(&self) -> *mut bindings::i2c_adapter {
> +        self.0.get()
> +    }
> +
> +    /// Gets pointer to an `i2c_adapter` by index.
> +    pub fn get(index: i32) -> Result<&'static Self> {

Hmm, perhaps I am misunderstanding what is going on, but I don’t think
’static is the right thing to have here.

Looking at i2c_get_adapter, it relies on: 

a) adap->nr actually being in i2c_adapter_idr, but even if it is, it may eventually be removed.
b) acquiring a refcount on adapter->dev,

Also, when this goes out of scope the refcount acquired above has to be
decremented, so this has to return an owned type and not a reference.

We should probably simply return ARef<I2cAdapter> here, or more succinctly,
ARef<Self>.

> +        // SAFETY: `index` must refer to a valid I2C adapter; the kernel
> +        // guarantees that `i2c_get_adapter(index)` returns either a valid
> +        // pointer or NULL. `NonNull::new` guarantees the correct check.
> +        let adapter = NonNull::new(unsafe { bindings::i2c_get_adapter(index) }).ok_or(ENODEV)?;
> +
> +        // SAFETY: `adapter` is non-null and points to a live `i2c_adapter`.
> +        // `I2cAdapter` is #[repr(transparent)], so this cast is valid.
> +        Ok(unsafe { adapter.cast::<Self>().as_ref() })
> +    }
> +}
> +
> +impl<Ctx: device::DeviceContext> Drop for I2cAdapter<Ctx> {
> +    fn drop(&mut self) {
> +        // SAFETY: This `I2cAdapter` was obtained from `i2c_get_adapter`,

Where? Note that drop() is called when a T goes out of scope, not when a &T
does so. Specially, you should not expect drop() to be called for T if
there’s a &’static T laying around.

> +        // and calling `i2c_put_adapter` exactly once will correctly release
> +        // the reference count in the I2C core. It is safe to call from any context
> +        unsafe { bindings::i2c_put_adapter(self.as_raw()) }
> +    }

Again, barring some misunderstanding on my end, remove this whole Drop impl in
favor of ARef<I2cAdapter>.

> +}
> +
> +// SAFETY: `I2cAdapter` is a transparent wrapper of a type that doesn't depend on `I2cAdapter`'s
> +// generic argument.
> +kernel::impl_device_context_deref!(unsafe { I2cAdapter });
> +kernel::impl_device_context_into_aref!(I2cAdapter);
> +
> +// SAFETY: Instances of `I2cAdapter` are always reference-counted.
> +unsafe impl crate::types::AlwaysRefCounted for I2cAdapter {
> +    fn inc_ref(&self) {
> +        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> +        unsafe { bindings::get_device(self.as_ref().as_raw()) };
> +    }
> +
> +    unsafe fn dec_ref(obj: NonNull<Self>) {
> +        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
> +        unsafe { bindings::put_device(&raw mut (*obj.as_ref().as_raw()).dev) }
> +    }
> +}

Shouldn’t these be `i2c_{get/put}_adapter` ?

> +
> +impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for I2cAdapter<Ctx> {
> +    fn as_ref(&self) -> &device::Device<Ctx> {
> +        let raw = self.as_raw();
> +        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> +        // `struct i2c_adapter`.
> +        let dev = unsafe { &raw mut (*raw).dev };
> +
> +        // SAFETY: `dev` points to a valid `struct device`.
> +        unsafe { device::Device::from_raw(dev) }
> +    }
> +}
> +
> +impl<Ctx: device::DeviceContext> TryFrom<&device::Device<Ctx>> for &I2cAdapter<Ctx> {
> +    type Error = kernel::error::Error;
> +
> +    fn try_from(dev: &device::Device<Ctx>) -> Result<Self, Self::Error> {
> +        // SAFETY: By the type invariant of `Device`, `dev.as_raw()` is a valid pointer to a
> +        // `struct device`.
> +        if unsafe { bindings::i2c_verify_adapter(dev.as_raw()).is_null() } {
> +            return Err(EINVAL);
> +        }
> +
> +        // SAFETY: We've just verified that the type of `dev` equals to
> +        // `bindings::i2c_adapter_type`, hence `dev` must be embedded in a valid
> +        // `struct i2c_adapter` as guaranteed by the corresponding C code.
> +        let idev = unsafe { container_of!(dev.as_raw(), bindings::i2c_adapter, dev) };
> +
> +        // SAFETY: `idev` is a valid pointer to a `struct i2c_adapter`.
> +        Ok(unsafe { &*idev.cast() })
> +    }
> +}
> +
> +/// The i2c board info representation
> +///
> +/// This structure represents the Rust abstraction for a C `struct i2c_board_info` structure,
> +/// which is used for manual I2C client creation.
> +#[repr(transparent)]
> +pub struct I2cBoardInfo(bindings::i2c_board_info);
> +
> +impl I2cBoardInfo {
> +    const I2C_TYPE_SIZE: usize = 20;
> +    /// Create a new board‐info for a kernel driver.

Nit: instead of `board-info` you can just say [`I2cBoardInfo`]. This will look
better in the docs.
 
> +    #[inline(always)]
> +    pub const fn new(type_: &'static CStr, addr: u16) -> Self {
> +        build_assert!(
> +            type_.len_with_nul() <= Self::I2C_TYPE_SIZE,
> +            "Type exceeds 20 bytes"
> +        );
> +        let src = type_.as_bytes_with_nul();
> +        // Replace with `bindings::acpi_device_id::default()` once stabilized for `const`.
> +        // SAFETY: FFI type is valid to be zero-initialized.
> +        let mut i2c_board_info: bindings::i2c_board_info = unsafe { core::mem::zeroed() };
> +        let mut i: usize = 0;
> +        while i < src.len() {
> +            i2c_board_info.type_[i] = src[i];
> +            i += 1;
> +        }
> +
> +        i2c_board_info.addr = addr;
> +        Self(i2c_board_info)
> +    }
> +
> +    fn as_raw(&self) -> *const bindings::i2c_board_info {
> +        from_ref(&self.0)
> +    }
> +}
> +
> /// The i2c client representation.
> ///
> /// This structure represents the Rust abstraction for a C `struct i2c_client`. The
> @@ -394,3 +525,45 @@ unsafe impl Send for I2cClient {}
> // SAFETY: `I2cClient` can be shared among threads because all methods of `I2cClient`
> // (i.e. `I2cClient<Normal>) are thread safe.
> unsafe impl Sync for I2cClient {}
> +
> +/// The registration of an i2c client device.
> +///
> +/// This type represents the registration of a [`struct i2c_client`]. When an instance of this
> +/// type is dropped, its respective i2c client device will be unregistered from the system.
> +///
> +/// # Invariants
> +///
> +/// `self.0` always holds a valid pointer to an initialized and registered
> +/// [`struct i2c_client`].
> +#[repr(transparent)]
> +pub struct Registration(NonNull<bindings::i2c_client>);
> +
> +impl Registration {
> +    /// The C `i2c_new_client_device` function wrapper for manual I2C client creation.
> +    pub fn new(i2c_adapter: &I2cAdapter, i2c_board_info: &I2cBoardInfo) -> Result<Self> {
> +        // SAFETY: the kernel guarantees that `i2c_new_client_device()` returns either a valid
> +        // pointer or NULL. `from_err_ptr` separates errors. Following `NonNull::new` checks
> +        // for NULL.
> +        let raw_dev = from_err_ptr(unsafe {
> +            bindings::i2c_new_client_device(i2c_adapter.as_raw(), i2c_board_info.as_raw())
> +        })?;
> +
> +        let dev_ptr = NonNull::new(raw_dev).ok_or(ENODEV)?;
> +
> +        Ok(Self(dev_ptr))
> +    }
> +}
> +
> +impl Drop for Registration {
> +    fn drop(&mut self) {
> +        // SAFETY: `Drop` is only called for a valid `Registration`, which by invariant
> +        // always contains a non-null pointer to an `i2c_client`.
> +        unsafe { bindings::i2c_unregister_device(self.0.as_ptr()) }
> +    }
> +}
> +
> +// SAFETY: A `Registration` of a `struct i2c_client` can be released from any thread.
> +unsafe impl Send for Registration {}
> +
> +// SAFETY: `Registration` does not expose any methods or fields that need synchronization.

Can you mention that `Registration` offers no interior mutability instead?

I think we should all align on the safety comments for Sync.

> +unsafe impl Sync for Registration {}
> -- 
> 2.43.0
> 
>