| 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
| ||
|
Message-ID: <CAL6vTrga0bN591VT0JJq8cd-kUgedsmRCMu=ST=NyZijT7dXQA@mail.gmail.com>
Date: Thu, 19 Dec 2024 07:33:18 -0800
From: Fabien Parent <parent.f@...il.com>
To: Rob Herring <robh@...nel.org>
Cc: Saravana Kannan <saravanak@...gle.com>, Miguel Ojeda <ojeda@...nel.org>,
Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>,
Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <benno.lossin@...ton.me>, 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>,
Wolfram Sang <wsa+renesas@...g-engineering.com>, Mark Brown <broonie@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>, devicetree@...r.kernel.org,
rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-i2c@...r.kernel.org, linux-arm-msm@...r.kernel.org,
vinod.koul@...aro.org, Fabien Parent <fabien.parent@...aro.org>,
Fiona Behrens <me@...enk.dev>
Subject: Re: [PATCH 1/9] rust: i2c: add basic I2C client abstraction
On Thu, Dec 19, 2024 at 5:03 AM Rob Herring <robh@...nel.org> wrote:
>
> On Wed, Dec 18, 2024 at 03:36:31PM -0800, Fabien Parent wrote:
> > From: Fiona Behrens <me@...enk.dev>
> >
> > Implement an abstraction to write I2C device drivers. The abstraction
> > is pretty basic and provides just the infrastructure to probe
> > a device from I2C/OF device_id and abstract `i2c_client`.
> > The client will be used by the Regmap abstraction to perform
> > I/O on the I2C bus.
> >
> > Signed-off-by: Fiona Behrens <me@...enk.dev>
> > Co-developed-by: Fabien Parent <fabien.parent@...aro.org>
> > Signed-off-by: Fabien Parent <fabien.parent@...aro.org>
> > ---
> > MAINTAINERS | 1 +
> > rust/bindings/bindings_helper.h | 1 +
> > rust/helpers/helpers.c | 1 +
> > rust/helpers/i2c.c | 13 ++
> > rust/kernel/i2c.rs | 288 ++++++++++++++++++++++++++++++++++++++++
> > rust/kernel/lib.rs | 2 +
> > 6 files changed, 306 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 6b9e10551392c185b9314c9f94edeaf6e85af58f..961fe4ed39605bf489d1d9e473f47bccb692ff14 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -10796,6 +10796,7 @@ F: include/linux/i2c-smbus.h
> > F: include/linux/i2c.h
> > F: include/uapi/linux/i2c-*.h
> > F: include/uapi/linux/i2c.h
> > +F: rust/kernel/i2c.rs
> >
> > I2C SUBSYSTEM HOST DRIVERS
> > M: Andi Shyti <andi.shyti@...nel.org>
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index e9fdceb568b8f94e602ee498323e5768a40a6cba..a882efb90bfc27960ef1fd5f2dc8cc40533a1c27 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -16,6 +16,7 @@
> > #include <linux/file.h>
> > #include <linux/firmware.h>
> > #include <linux/fs.h>
> > +#include <linux/i2c.h>
> > #include <linux/jiffies.h>
> > #include <linux/jump_label.h>
> > #include <linux/mdio.h>
> > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> > index 0640b7e115be1553549312dcfdf842bcae3bde1b..630e903f516ee14a51f46ff0bcc68e8f9a64021a 100644
> > --- a/rust/helpers/helpers.c
> > +++ b/rust/helpers/helpers.c
> > @@ -15,6 +15,7 @@
> > #include "device.c"
> > #include "err.c"
> > #include "fs.c"
> > +#include "i2c.c"
> > #include "io.c"
> > #include "jump_label.c"
> > #include "kunit.c"
> > diff --git a/rust/helpers/i2c.c b/rust/helpers/i2c.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..8ffdc454e7597cc61909da5b3597057aeb5f7299
> > --- /dev/null
> > +++ b/rust/helpers/i2c.c
> > @@ -0,0 +1,13 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/i2c.h>
> > +
> > +void *rust_helper_i2c_get_clientdata(const struct i2c_client *client)
> > +{
> > + return i2c_get_clientdata(client);
> > +}
> > +
> > +void rust_helper_i2c_set_clientdata(struct i2c_client *client, void *data)
> > +{
> > + i2c_set_clientdata(client, data);
> > +}
> > diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..efa03335e5b59e72738380e94213976b2464c25b
> > --- /dev/null
> > +++ b/rust/kernel/i2c.rs
> > @@ -0,0 +1,288 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Abstractions for the I2C bus.
> > +//!
> > +//! C header: [`include/linux/i2c.h`](srctree/include/linux/i2c.h)
> > +
> > +use crate::{
> > + bindings, container_of,
> > + device::Device,
> > + device_id::{self, RawDeviceId},
> > + driver,
> > + error::{to_result, Result},
> > + of,
> > + prelude::*,
> > + str::CStr,
> > + types::{ARef, ForeignOwnable, Opaque},
> > + ThisModule,
> > +};
> > +
> > +/// Abstraction for `bindings::i2c_device_id`.
> > +#[repr(transparent)]
> > +#[derive(Clone, Copy)]
> > +pub struct DeviceId(bindings::i2c_device_id);
> > +
> > +impl DeviceId {
> > + /// Create a new device id from an I2C name.
> > + pub const fn new(name: &CStr) -> Self {
> > + let src = name.as_bytes_with_nul();
> > + // TODO: Replace with `bindings::i2c_device_id::default()` once stabilized for `const`.
> > + // SAFETY: FFI type is valid to be zero-initialized.
> > + let mut i2c: bindings::i2c_device_id = unsafe { core::mem::zeroed() };
> > +
> > + let mut i = 0;
> > + while i < src.len() {
> > + i2c.name[i] = src[i] as _;
> > + i += 1;
> > + }
>
> You can simplify this now that char maps to u8 (in rust next).
>
> > +
> > + Self(i2c)
> > + }
> > +}
> > +
> > +// SAFETY:
> > +// * `DeviceId` is a `#[repr(transparent)` wrapper of `i2c_device_id` and does not add
> > +// additional invariants, so it's safe to transmute to `RawType`.
> > +// * `DRIVER_DATA_OFFSET` is the offset to the `data` field.
> > +unsafe impl RawDeviceId for DeviceId {
> > + type RawType = bindings::i2c_device_id;
> > +
> > + const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::i2c_device_id, driver_data);
> > +
> > + fn index(&self) -> usize {
> > + self.0.driver_data as _
> > + }
> > +}
> > +
> > +/// I2C [`DeviceId`] table.
> > +pub type IdTable<T> = &'static dyn device_id::IdTable<DeviceId, T>;
> > +
> > +/// An adapter for the registration of I2C drivers.
> > +#[doc(hidden)]
> > +pub struct Adapter<T: Driver + 'static>(T);
> > +
> > +impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
> > + type RegType = bindings::i2c_driver;
> > +
> > + fn register(
> > + i2cdrv: &Opaque<Self::RegType>,
> > + name: &'static CStr,
> > + module: &'static ThisModule,
> > + ) -> Result {
> > + // SAFETY: It's safe to set the fields of `struct i2c_driver` on initialization.
> > + unsafe {
> > + (*i2cdrv.get()).driver.name = name.as_char_ptr();
> > + (*i2cdrv.get()).probe = Some(Self::probe_callback);
> > + (*i2cdrv.get()).remove = Some(Self::remove_callback);
> > + if let Some(t) = T::I2C_ID_TABLE {
> > + (*i2cdrv.get()).id_table = t.as_ptr();
> > + }
> > + if let Some(t) = T::OF_ID_TABLE {
> > + (*i2cdrv.get()).driver.of_match_table = t.as_ptr();
> > + }
> > + }
> > +
> > + // SAFETY: `i2cdrv` is guaranteed to be a valid `RegType`.
> > + to_result(unsafe { bindings::i2c_register_driver(module.0, i2cdrv.get()) })
> > + }
> > +
> > + fn unregister(i2cdrv: &Opaque<Self::RegType>) {
> > + // SAFETY: `i2cdrv` is guaranteed to be a valid `RegType`.
> > + unsafe { bindings::i2c_del_driver(i2cdrv.get()) };
> > + }
> > +}
> > +
> > +impl<T: Driver> Adapter<T> {
> > + /// Get the [`Self::IdInfo`] that matched during probe.
> > + fn id_info(client: &mut Client) -> Option<&'static T::IdInfo> {
> > + let id = <Self as driver::Adapter>::id_info(client.as_ref());
> > + if id.is_some() {
> > + return id;
> > + }
> > +
> > + // SAFETY: `client` and `client.as_raw()` are guaranteed to be valid.
> > + let id = unsafe { bindings::i2c_client_get_device_id(client.as_raw()) };
> > + if !id.is_null() {
> > + // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct i2c_device_id` and
> > + // does not add additional invariants, so it's safe to transmute.
> > + let id = unsafe { &*id.cast::<DeviceId>() };
> > + return Some(T::I2C_ID_TABLE?.info(id.index()));
> > + }
>
> You aren't handling the DT based matching.
It is handled with the first line of this function:
+ fn id_info(client: &mut Client) -> Option<&'static T::IdInfo> {
+ let id = <Self as driver::Adapter>::id_info(client.as_ref());
This Adapter driver::Adapter is implemented by the I2C bus:
+impl<T: Driver + 'static> driver::Adapter for Adapter<T> {
+ type IdInfo = T::IdInfo;
+
+ fn of_id_table() -> Option<of::IdTable<Self::IdInfo>> {
+ T::OF_ID_TABLE
+ }
+}
This function is first using the adapter to retrieve the IdInfo, and
if nothing is returned then we perform our own match based on
i2c::IdInfo.
>
> Rob
Powered by blists - more mailing lists