Message-Id: <712BC396-F34D-4594-BA28-E2F9847A6899@collabora.com>
Date: Wed, 27 Aug 2025 16:23:11 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Igor Korotin <igor.korotin.linux@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>,
 Wolfram Sang <wsa+renesas@...g-engineering.com>,
 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>,
 Danilo Krummrich <dakr@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Viresh Kumar <viresh.kumar@...aro.org>,
 Asahi Lina <lina+kernel@...hilina.net>,
 Wedson Almeida Filho <wedsonaf@...il.com>,
 Alex Hung <alex.hung@....com>,
 Tamir Duberstein <tamird@...il.com>,
 Xiangfei Ding <dingxiangfei2009@...il.com>,
 linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org,
 linux-i2c@...r.kernel.org
Subject: Re: [PATCH v4 2/3] rust: i2c: add manual I2C device creation
 abstractions

Hi Igor,

> On 20 Aug 2025, at 12:21, Igor Korotin <igor.korotin.linux@...il.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@...il.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
> 
> 

Powered by blists - more mailing lists