Message-ID: <ZxibWpcswZxz5A07@pollux>
Date: Wed, 23 Oct 2024 08:44:42 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Rob Herring <robh@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com,
	ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com,
	gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	tmgross@...ch.edu, a.hindborg@...sung.com, aliceryhl@...gle.com,
	airlied@...il.com, fujita.tomonori@...il.com, lina@...hilina.net,
	pstanner@...hat.com, ajanulgu@...hat.com, lyude@...hat.com,
	daniel.almeida@...labora.com, saravanak@...gle.com,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 15/16] rust: platform: add basic platform device /
 driver abstractions

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@...nel.org>
> > ---
> >  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@...com>
> > 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>() })
> > +        }
> > +    }
> > +}
> 

Powered by blists - more mailing lists