On Tue, Oct 22, 2024 at 06:47:12PM -0500, Rob Herring wrote:
> On Tue, Oct 22, 2024 at 11:31:52PM +0200, Danilo Krummrich wrote:
> > 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@xxxxxxxxxx>
> > ---
> > MAINTAINERS | 1 +
> > rust/bindings/bindings_helper.h | 1 +
> > rust/helpers/helpers.c | 1 +
> > rust/helpers/platform.c | 13 ++
> > rust/kernel/lib.rs | 1 +
> > rust/kernel/platform.rs | 217 ++++++++++++++++++++++++++++++++
> > 6 files changed, 234 insertions(+)
> > create mode 100644 rust/helpers/platform.c
> > create mode 100644 rust/kernel/platform.rs
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 87eb9a7869eb..173540375863 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -6985,6 +6985,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@xxxxxx>
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index 312f03cbdce9..217c776615b9 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -18,6 +18,7 @@
> > #include <linux/of_device.h>
> > #include <linux/pci.h>
> > #include <linux/phy.h>
> > +#include <linux/platform_device.h>
> > #include <linux/refcount.h>
> > #include <linux/sched.h>
> > #include <linux/slab.h>
> > diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> > index 8bc6e9735589..663cdc2a45e0 100644
> > --- a/rust/helpers/helpers.c
> > +++ b/rust/helpers/helpers.c
> > @@ -17,6 +17,7 @@
> > #include "kunit.c"
> > #include "mutex.c"
> > #include "page.c"
> > +#include "platform.c"
> > #include "pci.c"
> > #include "rbtree.c"
> > #include "rcu.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 5946f59f1688..9e8dcd6d7c01 100644
> > --- a/rust/kernel/lib.rs
> > +++ b/rust/kernel/lib.rs
> > @@ -53,6 +53,7 @@
> > pub mod net;
> > pub mod of;
> > pub mod page;
> > +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..addf5356f44f
> > --- /dev/null
> > +++ b/rust/kernel/platform.rs
> > @@ -0,0 +1,217 @@
> > +// 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,
> > + device_id::RawDeviceId,
> > + 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::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> {
> > + fn id_info(pdev: &Device) -> Option<&'static T::IdInfo> {
> > + let table = T::ID_TABLE;
> > + let id = T::of_match_device(pdev)?;
> > +
> > + Some(table.info(id.index()))
> > + }
> > +
> > + extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> core::ffi::c_int {
> > + // SAFETY: The platform bus only ever calls the probe callback with a valid `pdev`.
> > + let dev = unsafe { device::Device::from_raw(&mut (*pdev).dev) };
> > + // SAFETY: `dev` is guaranteed to be embedded in a valid `struct platform_device` by the
> > + // call above.
> > + let mut pdev = unsafe { Device::from_dev(dev) };
> > +
> > + let info = Self::id_info(&pdev);
> > + match T::probe(&mut pdev, info) {
> > + Ok(data) => {
> > + // Let the `struct platform_device` own a reference of the driver's private data.
> > + // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
> > + // `struct platform_device`.
> > + unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> > + }
> > + Err(err) => return Error::to_errno(err),
> > + }
> > +
> > + 0
> > + }
> > +
> > + extern "C" fn remove_callback(pdev: *mut bindings::platform_device) {
> > + // SAFETY: `pdev` is a valid pointer to a `struct platform_device`.
> > + let ptr = unsafe { bindings::platform_get_drvdata(pdev) };
> > +
> > + // SAFETY: `remove_callback` is only ever called after a successful call to
> > + // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
> > + // `KBox<T>` pointer created through `KBox::into_foreign`.
> > + let _ = unsafe { KBox::<T>::from_foreign(ptr) };
> > + }
> > +}
> > +
> > +/// Declares a kernel module that exposes a single platform driver.
> > +///
> > +/// # Examples
> > +///
> > +/// ```ignore
> > +/// kernel::module_platform_driver! {
> > +/// type: MyDriver,
> > +/// name: "Module name",
> > +/// author: "Author name",
> > +/// description: "Description",
> > +/// license: "GPL v2",
> > +/// }
> > +/// ```
> > +#[macro_export]
> > +macro_rules! module_platform_driver {
> > + ($($f:tt)*) => {
> > + $crate::module_driver!(<T>, $crate::platform::Adapter<T>, { $($f)* });
> > + };
> > +}
> > +
> > +/// IdTable type for platform drivers.
> > +pub type IdTable<T> = &'static dyn kernel::device_id::IdTable<of::DeviceId, T>;
> > +
> > +/// The platform driver trait.
> > +///
> > +/// # Example
> > +///
> > +///```
> > +/// # use kernel::{bindings, c_str, of, platform};
> > +///
> > +/// struct MyDriver;
> > +///
> > +/// kernel::of_device_table!(
> > +/// OF_TABLE,
> > +/// MODULE_OF_TABLE,
> > +/// <MyDriver as platform::Driver>::IdInfo,
> > +/// [
> > +/// (of::DeviceId::new(c_str!("redhat,my-device")), ())
>
> All compatible strings have to be documented as do vendor prefixes and
> I don't think "redhat" is one. An exception is you can use
> "test,<whatever>" and not document it.
Yeah, I copied that from the sample driver, where it's probably wrong too.
I guess "vendor,device" would be illegal as well?
>
> There's a check for undocumented compatibles. I guess I'll have to add
> rust parsing to it...
>
> BTW, how do you compile this code in the kernel?
You mean this example? It gets compiled as a KUnit doctest, but it obvously
doesn't execute anything, so it's a compile only test.
>
> > +/// ]
> > +/// );
> > +///
> > +/// impl platform::Driver for MyDriver {
> > +/// type IdInfo = ();
> > +/// const ID_TABLE: platform::IdTable<Self::IdInfo> = &OF_TABLE;
> > +///
> > +/// fn probe(
> > +/// _pdev: &mut platform::Device,
> > +/// _id_info: Option<&Self::IdInfo>,
> > +/// ) -> Result<Pin<KBox<Self>>> {
> > +/// Err(ENODEV)
> > +/// }
> > +/// }
> > +///```
> > +/// Drivers must implement this trait in order to get a platform driver registered. Please refer to
> > +/// the `Adapter` documentation for an example.
> > +pub trait Driver {
> > + /// The type holding information about each device id supported by the driver.
> > + ///
> > + /// TODO: Use associated_type_defaults once stabilized:
> > + ///
> > + /// type IdInfo: 'static = ();
> > + type IdInfo: 'static;
> > +
> > + /// The table of device ids supported by the driver.
> > + const ID_TABLE: IdTable<Self::IdInfo>;
> > +
> > + /// Platform driver probe.
> > + ///
> > + /// Called when a new platform device is added or discovered.
> > + /// Implementers should attempt to initialize the device here.
> > + fn probe(dev: &mut Device, id_info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>>;
> > +
> > + /// Find the [`of::DeviceId`] within [`Driver::ID_TABLE`] matching the given [`Device`], if any.
> > + fn of_match_device(pdev: &Device) -> Option<&of::DeviceId> {
>
> Is this visible to drivers? It shouldn't be.
Yeah, I think we should just remove it. Looking at struct of_device_id, it
doesn't contain any useful information for a driver. I think when I added this I
was a bit in "autopilot" mode from the PCI stuff, where struct pci_device_id is
useful to drivers.
> I just removed most of the
> calls of the C version earlier this year. Drivers should only need the
> match data. The preferred driver C API is device_get_match_data(). That
> is firmware agnostic and works for DT, ACPI and old platform
> driver_data. Obviously, ACPI is not supported here, but it will be soon
> enough. We could perhaps get away with not supporting the platform
> driver_data because that's generally not used on anything in the last 10
> years.
Otherwise `of_match_device` is only used in `probe_callback` to get the device
info structure, which we indeed should use device_get_match_data() for.
>
> Another potential issue is keeping the match logic for probe and the
> match logic for the data in sync. If you implement your own logic here
> in rust and probe is using the C version, they might not be the same.
> Best case, we have 2 implementations of the same thing.
>
> > + let table = Self::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_dev().as_raw()`.
> > + let raw_id = unsafe { bindings::of_match_device(table.as_ptr(), pdev.as_dev().as_raw()) };
>
> of_match_device depends on CONFIG_OF. There's an empty static inline,
> but seems bindgen can't handle those. Prior versions added a helper
> function, but that's going to scale terribly. Can we use an annotation
> for CONFIG_OF here (assuming we can't get rid of using this directly)?
>
> > +
> > + 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.
> > + Some(unsafe { &*raw_id.cast::<of::DeviceId>() })
> > + }
> > + }
> > +}
>
