linux-kernel - Re: [PATCH v5 2/3] rust: i2c: add manual I2C device creation abstractions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DCQ9AXZ5APKN.1835AK0PVA3S5@kernel.org>
Date: Thu, 11 Sep 2025 22:34:17 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
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>, "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 v5 2/3] rust: i2c: add manual I2C device creation
 abstractions

On Thu Sep 11, 2025 at 5:50 PM CEST, Igor Korotin wrote:
> +impl I2cAdapter {
> +    /// Gets pointer to an `i2c_adapter` by index.
> +    pub fn get(index: i32) -> Result<ARef<Self>> {

Where do we get this index usually from? OF, ACPI, etc. I assume? I feel like it
could make sense to wrap it into a new type. Even though it is not safety
relevant it eliminates a source for mistakes.

> +        // 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.as_ptr().cast::<I2cAdapter<device::Normal>>()).into() })
> +    }
> +}
> +
> +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()) }
> +    }
> +}

The Drop implementation is not needed, you only ever give out an
ARef<I2cAdapter>, but never a "raw" I2cAdapter, which is the correct thing to
do.

> +
> +// 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::i2c_get_adapter((*self.as_raw()).nr) };

Please make accessing the nr field a separate inline function, or at least put
it in a separate unsafe block.

> +    }
> +
> +    unsafe fn dec_ref(obj: NonNull<Self>) {
> +        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
> +        unsafe { bindings::i2c_put_adapter(&raw mut (*obj.as_ref().as_raw())) }

Same here, separate unsafe blocks please.

> +    }
> +}
> +
> +impl<Ctx: device::DeviceContext> AsRef<I2cAdapter<Ctx>> for I2cAdapter<Ctx> {
> +    fn as_ref(&self) -> &I2cAdapter<Ctx> {
> +        &self
> +    }
> +}

This AsRef implementation doesn't seem to do anything?