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>() }) > > + } > > + } > > +} >