Re: [PATCH v4 12/13] rust: platform: add basic platform device / driver abstractions

On Mon, Dec 09, 2024 at 04:37:06PM -0600, Rob Herring wrote:

On Thu, Dec 05, 2024 at 03:14:43PM +0100, Danilo Krummrich wrote:

quoted

Implement the basic platform bus abstractions required to write a basic
platform driver. This includes the following data structures:

The `platform::Driver` trait represents the interface to the driver and
provides `pci::Driver::probe` for the driver to implement.

The `platform::Device` abstraction represents a `struct platform_device`.

In order to provide the platform bus specific parts to a generic
`driver::Registration` the `driver::RegistrationOps` trait is implemented
by `platform::Adapter`.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 MAINTAINERS                     |   1 +
 rust/bindings/bindings_helper.h |   2 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/platform.c         |  13 ++
 rust/kernel/lib.rs              |   1 +
 rust/kernel/platform.rs         | 222 ++++++++++++++++++++++++++++++++
 6 files changed, 240 insertions(+)
 create mode 100644 rust/helpers/platform.c
 create mode 100644 rust/kernel/platform.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 7d6bb4b15d2c..365fc48b7041 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -7034,6 +7034,7 @@ F:	rust/kernel/device.rs
 F:	rust/kernel/device_id.rs
 F:	rust/kernel/devres.rs
 F:	rust/kernel/driver.rs
+F:	rust/kernel/platform.rs
 
 DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS)
 M:	Nishanth Menon <nm@ti.com>

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 6d7a68e2ecb7..e9fdceb568b8 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h

@@ -20,9 +20,11 @@
 #include <linux/jump_label.h>
 #include <linux/mdio.h>
 #include <linux/miscdevice.h>
+#include <linux/of_device.h>
 #include <linux/pci.h>
 #include <linux/phy.h>
 #include <linux/pid_namespace.h>
+#include <linux/platform_device.h>
 #include <linux/poll.h>
 #include <linux/refcount.h>
 #include <linux/sched.h>

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 3fda33cd42d4..0640b7e115be 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c

@@ -20,6 +20,7 @@
 #include "kunit.c"
 #include "mutex.c"
 #include "page.c"
+#include "platform.c"
 #include "pci.c"
 #include "pid_namespace.c"
 #include "rbtree.c"

diff --git a/rust/helpers/platform.c b/rust/helpers/platform.c
new file mode 100644
index 000000000000..ab9b9f317301
--- /dev/null
+++ b/rust/helpers/platform.c

@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/platform_device.h>
+
+void *rust_helper_platform_get_drvdata(const struct platform_device *pdev)
+{
+	return platform_get_drvdata(pdev);
+}
+
+void rust_helper_platform_set_drvdata(struct platform_device *pdev, void *data)
+{
+	platform_set_drvdata(pdev, data);
+}

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 7a0e4c82ad0c..cc8f48aa162b 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs

@@ -59,6 +59,7 @@
 pub mod of;
 pub mod page;
 pub mod pid_namespace;
+pub mod platform;
 pub mod prelude;
 pub mod print;
 pub mod rbtree;

diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
new file mode 100644
index 000000000000..868cfddb75a2
--- /dev/null
+++ b/rust/kernel/platform.rs

@@ -0,0 +1,222 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstractions for the platform bus.
+//!
+//! C header: [`include/linux/platform_device.h`](srctree/include/linux/platform_device.h)
+
+use crate::{
+    bindings, container_of, device, driver,
+    error::{to_result, Result},
+    of,
+    prelude::*,
+    str::CStr,
+    types::{ARef, ForeignOwnable},
+    ThisModule,
+};
+
+/// An adapter for the registration of platform drivers.
+pub struct Adapter<T: Driver>(T);
+
+impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
+    type RegType = bindings::platform_driver;
+
+    fn register(
+        pdrv: &mut Self::RegType,
+        name: &'static CStr,
+        module: &'static ThisModule,
+    ) -> Result {
+        pdrv.driver.name = name.as_char_ptr();
+        pdrv.probe = Some(Self::probe_callback);
+
+        // Both members of this union are identical in data layout and semantics.
+        pdrv.__bindgen_anon_1.remove = Some(Self::remove_callback);
+        pdrv.driver.of_match_table = T::OF_ID_TABLE.as_ptr();
+
+        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        to_result(unsafe { bindings::__platform_driver_register(pdrv, module.0) })
+    }
+
+    fn unregister(pdrv: &mut Self::RegType) {
+        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        unsafe { bindings::platform_driver_unregister(pdrv) };
+    }
+}
+
+impl<T: Driver + 'static> Adapter<T> {
+    #[cfg(CONFIG_OF)]
+    fn of_id_info(pdev: &Device) -> Option<&'static T::IdInfo> {
+        let table = T::OF_ID_TABLE;
+
+        // SAFETY:
+        // - `table` has static lifetime, hence it's valid for read,
+        // - `dev` is guaranteed to be valid while it's alive, and so is `pdev.as_ref().as_raw()`.
+        let raw_id = unsafe { bindings::of_match_device(table.as_ptr(), pdev.as_ref().as_raw()) };
+
+        if raw_id.is_null() {
+            None
+        } else {
+            // SAFETY: `DeviceId` is a `#[repr(transparent)` wrapper of `struct of_device_id` and
+            // does not add additional invariants, so it's safe to transmute.
+            let id = unsafe { &*raw_id.cast::<of::DeviceId>() };
+
+            Some(table.info(<of::DeviceId as crate::device_id::RawDeviceId>::index(id)))
+        }
+    }
+
+    #[cfg(not(CONFIG_OF))]
+    fn of_id_info(_pdev: &Device) -> Option<&'static T::IdInfo> {
+        None
+    }
+
+    // Try to retrieve an `IdInfo` from any of the ID tables; if we can't find one for a particular
+    // table, it means we don't have a match in there. If we don't match any of the ID tables, it
+    // means we were matched by name.
+    fn id_info(pdev: &Device) -> Option<&'static T::IdInfo> {
+        let id = Self::of_id_info(pdev);
+        if id.is_some() {
+            return id;
+        }
+
+        None
+    }

These methods are going to have to be duplicated by every bus type which 
can do DT matching (and later ACPI). Can't this be moved to be part of 
the common Driver trait.

As mentioned in v3, I agree, but I'd prefer to do this in a follow up series.

I'll say it again for Greg to comment (doubtful he will look at v3 
again). Really, I think we should also align the probe method interface 
across bus types. That means getting rid of the 'id' in the PCI probe 
(or add it for everyone). Most drivers never need it. The typical case 
is needing nothing or the matched data. In a quick scan[1], there's 
only a handful of cases. So I think probe should match the common 
scenario and make retrieving the id match explicit if needed.

I agree, but I will keep it until Greg commented on it, since I explicitly
promised to add it on request.

Rob

[1] git grep -W 'const struct pci_device_id \*' drivers/ | grep -P 'id->(?!driver_data)'