linux-kernel - Re: [PATCH v4 1/3] rust: i2c: add basic I2C device and driver 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: <655ca23c-1fe6-498a-80b8-1b75044d9db3@gmail.com>
Date: Tue, 9 Sep 2025 18:19:03 +0100
From: Igor Korotin <igor.korotin.linux@...il.com>
To: Daniel Almeida <daniel.almeida@...labora.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 1/3] rust: i2c: add basic I2C device and driver
 abstractions

Hi Daniel

On 8/27/2025 7:37 PM, Daniel Almeida wrote:
>> +
>> +        let i2c_table = match T::I2C_ID_TABLE {
>> +            Some(table) => table.as_ptr(),
>> +            None => core::ptr::null(),
>> +        };
>> +
>> +        let of_table = match T::OF_ID_TABLE {
>> +            Some(table) => table.as_ptr(),
>> +            None => core::ptr::null(),
>> +        };
>> +
>> +        let acpi_table = match T::ACPI_ID_TABLE {
>> +            Some(table) => table.as_ptr(),
>> +            None => core::ptr::null(),
>> +        };
>> +
>> +        // SAFETY: It's safe to set the fields of `struct i2c_client` on initialization.
>> +        unsafe {
>> +            (*idrv.get()).driver.name = name.as_char_ptr();
>> +            (*idrv.get()).probe = Some(Self::probe_callback);
>> +            (*idrv.get()).remove = Some(Self::remove_callback);
>> +            (*idrv.get()).shutdown = Some(Self::shutdown_callback);
>> +            (*idrv.get()).id_table = i2c_table;
>> +            (*idrv.get()).driver.of_match_table = of_table;
>> +            (*idrv.get()).driver.acpi_match_table = acpi_table;
>> +        }
>> +
>> +        // SAFETY: `idrv` is guaranteed to be a valid `RegType`.
>> +        to_result(unsafe { bindings::i2c_register_driver(module.0, idrv.get()) })
>> +    }
>> +
>> +    unsafe fn unregister(idrv: &Opaque<Self::RegType>) {
>> +        // SAFETY: `idrv` is guaranteed to be a valid `RegType`.
>> +        unsafe { bindings::i2c_del_driver(idrv.get()) }
>> +    }
>> +}
>> +
>> +impl<T: Driver + 'static> Adapter<T> {
>> +    extern "C" fn probe_callback(idev: *mut bindings::i2c_client) -> kernel::ffi::c_int {
>> +        // SAFETY: The I2C bus only ever calls the probe callback with a valid pointer to a
>> +        // `struct i2c_client`.
>> +        //
>> +        // INVARIANT: `idev` is valid for the duration of `probe_callback()`.
>> +        let idev = unsafe { &*idev.cast::<I2cClient<device::CoreInternal>>() };
>> +
>> +        let info =
>> +            Self::i2c_id_info(idev).or_else(|| <Self as driver::Adapter>::id_info(idev.as_ref()));
> 
> I wonder if these should be private member functions?
> 
>> +
>> +        from_result(|| {
>> +            let data = T::probe(idev, info)?;
>> +
>> +            idev.as_ref().set_drvdata(data);
>> +            Ok(0)
>> +        })
>> +    }
>> +
>> +    extern "C" fn remove_callback(idev: *mut bindings::i2c_client) {
>> +        // SAFETY: `idev` is a valid pointer to a `struct i2c_client`.
>> +        let idev = unsafe { &*idev.cast::<I2cClient<device::CoreInternal>>() };
> 
>> +
>> +        // SAFETY: `remove_callback` is only ever called after a successful call to
>> +        // `probe_callback`, hence it's guaranteed that `I2cClient::set_drvdata()` has been called
>> +        // and stored a `Pin<KBox<T>>`.
>> +        drop(unsafe { idev.as_ref().drvdata_obtain::<Pin<KBox<T>>>() });
>> +    }
>> +
>> +    extern "C" fn shutdown_callback(idev: *mut bindings::i2c_client) {
>> +        // SAFETY: `shutdown_callback` is only ever called for a valid `idev`
>> +        let idev = unsafe { &*idev.cast::<I2cClient<device::Core>>() };
>> +
>> +        T::shutdown(idev);
>> +    }
>> +
>> +    /// The [`i2c::IdTable`] of the corresponding driver.
>> +    fn i2c_id_table() -> Option<IdTable<<Self as driver::Adapter>::IdInfo>> {
>> +        T::I2C_ID_TABLE
>> +    }
>> +
>> +    /// Returns the driver's private data from the matching entry in the [`i2c::IdTable`], if any.
>> +    ///
>> +    /// If this returns `None`, it means there is no match with an entry in the [`i2c::IdTable`].
>> +    fn i2c_id_info(dev: &I2cClient) -> Option<&'static <Self as driver::Adapter>::IdInfo> {
> 
> Again, perhaps a private member function? I’m trying to simplify the syntax here.

Can you, please, kindly clarify what do you mean? If a function is not 
pub/pub(crate),
it is a private function.

Thanks,
Igor