[RFC PATCH] rust: i2c: Remove use of bindings::i2c_match_id

[RFC PATCH] rust: i2c: Remove use of bindings::i2c_match_id

[Andrew Davis]

The C function i2c_match_id() is slated for removal, remove use of this
from Rust I2C by directly finding the matching ID for I2C clients.

Signed-off-by: Andrew Davis <afd@ti.com>
---

Hello all,

Looking for some help on this one. I'm working on removing i2c_match_id()
from the kernel for various reasons, and ran into the issue of it being
used by Rust through a binding.

Wanted to try my hand at some Rust code and so tried to recreate this
simple ~7 line C function in Rust. Didn't go so well, tried writing it
by hand and got something that built, but a quick AI code review told me
every single line I wrote was wrong :D After some more fixing this is what
I ended up with. Seems to be correct but having a hard time believing this
mess I came up with is really how Rust should be written. So any suggestions
or hints on a better way to do this would be very welcome!

Thanks,
Andrew

 rust/kernel/i2c.rs | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
index b4ff25c830fc0..acf96887253c8 100644
--- a/rust/kernel/i2c.rs
+++ b/rust/kernel/i2c.rs
@@ -197,12 +197,35 @@ fn i2c_id_info(dev: &I2cClient) -> Option<&'static <Self as driver::Adapter>::Id
         let table = Self::i2c_id_table()?;
 
         // SAFETY:
-        // - `table` has static lifetime, hence it's valid for reads
         // - `dev` is guaranteed to be valid while it's alive, and so is `dev.as_raw()`.
-        let raw_id = unsafe { bindings::i2c_match_id(table.as_ptr(), dev.as_raw()) };
-
-        if raw_id.is_null() {
-            return None;
+        // - `name` is filled with `strscpy()` when the client is created, so it is NUL-terminated,
+        //    and it is not modified for the lifetime of the client.
+        let client_name = unsafe { CStr::from_char_ptr((*dev.as_raw()).name.as_ptr()) };
+
+        // Get the raw pointer to the start of the C array
+        // It is never null as it points at the start of a static `IdArray`.
+        let mut raw_id = table.as_ptr();
+
+        // Iterate until hitting the sentinel entry or matching the client name
+        loop {
+            // SAFETY: `raw_id` points to an entry of `table`, which is terminated by a zeroed
+            // sentinel entry. We never advance past the sentinel (see below), so `raw_id` is
+            // valid for reads. `name` is NUL-terminated for every entry, including the
+            // sentinel, which is all zeroes.
+            let id_name = unsafe { CStr::from_char_ptr((*raw_id).name.as_ptr()) };
+
+            // An empty name is the sentinel: no match.
+            if id_name.is_empty() {
+                return None;
+            }
+
+            if id_name == client_name {
+                break;
+            }
+
+            // SAFETY: the current entry is not the sentinel, so the next entry is still
+            // within `table`.
+            raw_id = unsafe { raw_id.add(1) };
         }
 
         // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct i2c_device_id` and
-- 
2.43.0

[RFC PATCH v2] rust: i2c: Remove use of bindings::i2c_match_id

[Andrew Davis]

The C function i2c_match_id() is slated for removal, remove use of this
from Rust I2C by switching to bindings::i2c_get_match_data().

Signed-off-by: Andrew Davis <afd@ti.com>
---

Well Sashiko suggested I take the easy way and just re-use a different C
function instead of trying to open-code the loop in Rust. The C function
implements the same lookup as i2c_id_info did so we can drop that out and
its i2c_id_table helper. Still would like some help on checking if this
is actually correct though..

Thanks,
Andrew

 rust/kernel/i2c.rs | 39 +++++++--------------------------------
 1 file changed, 7 insertions(+), 32 deletions(-)

diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
index b4ff25c830fc0..37d914fcb3662 100644
--- a/rust/kernel/i2c.rs
+++ b/rust/kernel/i2c.rs
@@ -148,10 +148,13 @@ extern "C" fn probe_callback(idev: *mut bindings::i2c_client) -> kernel::ffi::c_
         // 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(|| {
-            // SAFETY: `idev` matched data is of type `Self::IdInfo`.
-            unsafe { <Self as driver::Adapter>::id_info(idev.as_ref()) }
-        });
+        // SAFETY: `idev` is a valid `struct i2c_client` bound to this driver, so
+        // `i2c_get_match_data()` may read `idev.dev.driver`.
+        let raw_info = unsafe { bindings::i2c_get_match_data(idev.as_raw()) };
+
+        // SAFETY: Non-NULL match data is either the OF/ACPI entry data or the `driver_data` of an
+        // I2C id entry. Both are built from `&'static Self::IdInfo` by this driver's tables.
+        let info = unsafe { raw_info.cast::<T::IdInfo>().as_ref() };
 
         from_result(|| {
             let data = T::probe(idev, info);
@@ -184,34 +187,6 @@ extern "C" fn shutdown_callback(idev: *mut bindings::i2c_client) {
 
         T::shutdown(idev, data);
     }
-
-    /// 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> {
-        let table = Self::i2c_id_table()?;
-
-        // SAFETY:
-        // - `table` has static lifetime, hence it's valid for reads
-        // - `dev` is guaranteed to be valid while it's alive, and so is `dev.as_raw()`.
-        let raw_id = unsafe { bindings::i2c_match_id(table.as_ptr(), dev.as_raw()) };
-
-        if raw_id.is_null() {
-            return None;
-        }
-
-        // 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 { &*raw_id.cast::<DeviceId>() };
-
-        // SAFETY: `id` comes from `table` which is of type `IdArray<_, Self::IdInfo>`.
-        Some(unsafe { id.info_unchecked::<T::IdInfo>() })
-    }
 }
 
 impl<T: Driver> driver::Adapter for Adapter<T> {
-- 
2.43.0