| Message ID | 20241022213221.2383-16-dakr@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Device / Driver PCI / Platform Rust abstractions | expand |
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@kernel.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@ti.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. 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? > +/// ] > +/// ); > +/// > +/// 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. 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. 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>() }) > + } > + } > +}
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@kernel.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@ti.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>() }) > > + } > > + } > > +} >
On Wed, Oct 23, 2024 at 08:44:42AM +0200, Danilo Krummrich wrote: > 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@kernel.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@ti.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? Yes. > > 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. Yes. That's a question for my own education. > > > > > +/// ] > > > +/// ); > > > +/// > > > +/// 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>; Another thing. I don't think this is quite right. Well, this part is fine, but assigning the DT table to it is not. The underlying C code has 2 id tables in struct device_driver (DT and ACPI) and then the bus specific one in the struct ${bus}_driver. > > > + > > > + /// 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. TBC, you mean other than *data, right? If so, I agree. The DT type and name fields are pretty much legacy, so I don't think the rust bindings need to worry about them until someone converts Sparc and PowerMac drivers to rust (i.e. never). I would guess the PCI cases might be questionable, too. Like DT, drivers may be accessing the table fields, but that's not best practice. All the match fields are stored in pci_dev, so why get them from the match table? Rob
Hi Danilo, On 22.10.2024 23:31, 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@kernel.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/rust/kernel/platform.rs b/rust/kernel/platform.rs > new file mode 100644 > index 000000000000..addf5356f44f > --- /dev/null > +++ b/rust/kernel/platform.rs .... > +/// 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, It looks to me that OF_TABLE and MODULE_OF_TABLE are quite generic names used here. Shouldn't they be somehow driver specific, e.g. OF_TABLE_MYDRIVER and MODULE_OF_TABLE_MYDRIVER or whatever? Same for the other examples/samples in this patch series. Found that while using the *same* somewhere else ;) Best regards Dirk
Hi Danilo, On Tue Oct 22, 2024 at 2:31 PM PDT, Danilo Krummrich wrote: > +/// An adapter for the registration of platform drivers. > +pub struct Adapter<T: Driver>(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")), ()) > +/// ] > +/// ); > +/// > +/// 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. The `Adapter` doesn't have any examples, but there is one right above this paragraph. Anyway I think the example is in the right place and should not be moved over there. I think this paragraph should be moved above the example and the last sentence should be deleted. Fabien
On Wed, Oct 23, 2024 at 09:23:55AM -0500, Rob Herring wrote: > On Wed, Oct 23, 2024 at 08:44:42AM +0200, Danilo Krummrich wrote: > > 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: > > > > +/// ] > > > > +/// ); > > > > +/// > > > > +/// 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>; > > Another thing. I don't think this is quite right. Well, this part is > fine, but assigning the DT table to it is not. The underlying C code has > 2 id tables in struct device_driver (DT and ACPI) and then the bus > specific one in the struct ${bus}_driver. The assignment of this table in `Adapter::register` looks like this: `pdrv.driver.of_match_table = T::ID_TABLE.as_ptr();` What do you think is wrong with this assignment? > > > > > + > > > > + /// 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. > > TBC, you mean other than *data, right? If so, I agree. Yes. > > The DT type and name fields are pretty much legacy, so I don't think the > rust bindings need to worry about them until someone converts Sparc and > PowerMac drivers to rust (i.e. never). > > I would guess the PCI cases might be questionable, too. Like DT, drivers > may be accessing the table fields, but that's not best practice. All the > match fields are stored in pci_dev, so why get them from the match > table? Fair question, I'd like to forward it to Greg. IIRC, he explicitly requested to make the corresponding struct pci_device_id available in probe() at Kangrejos. > > Rob >
On Thu, Oct 24, 2024 at 11:11:50AM +0200, Dirk Behme wrote: > > +/// 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, > > It looks to me that OF_TABLE and MODULE_OF_TABLE are quite generic names > used here. Shouldn't they be somehow driver specific, e.g. OF_TABLE_MYDRIVER > and MODULE_OF_TABLE_MYDRIVER or whatever? Same for the other > examples/samples in this patch series. Found that while using the *same* > somewhere else ;) I think the names by themselves are fine. They're local to the module. However, we stringify `OF_TABLE` in `module_device_table` to build the export name, i.e. "__mod_of__OF_TABLE_device_table". Hence the potential duplicate symbols. I think we somehow need to build the module name into the symbol name as well. > > Best regards > > Dirk > >
On 22.10.2024 23:31, 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@kernel.org> > --- ... > 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 ... > +/// 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")), ()) > +/// ] > +/// ); > +/// > +/// 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) > +/// } > +/// } > +///``` Just in case it helps, having CONFIG_OF_UNITTEST with Rob's device tree add ons enabled adding something like [1] makes this example not compile only, but being executed as well: ... rust_example_platform_driver testcase-data:platform-tests:test-device@2: Rust example platform driver probe() called. ... # rust_doctest_kernel_platform_rs_0.location: rust/kernel/platform.rs:114 ok 63 rust_doctest_kernel_platform_rs_0 ... Best regards Dirk [1] diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs index addf5356f44f..a926233a789f 100644 --- a/rust/kernel/platform.rs +++ b/rust/kernel/platform.rs @@ -112,7 +112,8 @@ macro_rules! module_platform_driver { /// # Example /// ///``` -/// # use kernel::{bindings, c_str, of, platform}; +/// # mod module_example_platform_driver { +/// # use kernel::{bindings, c_str, of, platform, prelude::*}; /// /// struct MyDriver; /// @@ -121,7 +122,7 @@ macro_rules! module_platform_driver { /// MODULE_OF_TABLE, /// <MyDriver as platform::Driver>::IdInfo, /// [ -/// (of::DeviceId::new(c_str!("redhat,my-device")), ()) +/// (of::DeviceId::new(c_str!("test,rust-device")), ()) /// ] /// ); /// @@ -130,12 +131,22 @@ macro_rules! module_platform_driver { /// const ID_TABLE: platform::IdTable<Self::IdInfo> = &OF_TABLE; /// /// fn probe( -/// _pdev: &mut platform::Device, +/// pdev: &mut platform::Device, /// _id_info: Option<&Self::IdInfo>, /// ) -> Result<Pin<KBox<Self>>> { +/// dev_info!(pdev.as_ref(), "Rust example platform driver probe() called.\n"); /// Err(ENODEV) /// } /// } +/// +/// kernel::module_platform_driver! { +/// type: MyDriver, +/// name: "rust_example_platform_driver", +/// author: "Danilo Krummrich", +/// description: "Rust example platform driver", +/// license: "GPL v2", +/// } +/// # } ///``` /// Drivers must implement this trait in order to get a platform driver registered. Please refer to /// the `Adapter` documentation for an example.
On 28.10.2024 11:19, Danilo Krummrich wrote: > On Thu, Oct 24, 2024 at 11:11:50AM +0200, Dirk Behme wrote: >>> +/// 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, >> >> It looks to me that OF_TABLE and MODULE_OF_TABLE are quite generic names >> used here. Shouldn't they be somehow driver specific, e.g. OF_TABLE_MYDRIVER >> and MODULE_OF_TABLE_MYDRIVER or whatever? Same for the other >> examples/samples in this patch series. Found that while using the *same* >> somewhere else ;) > > I think the names by themselves are fine. They're local to the module. However, > we stringify `OF_TABLE` in `module_device_table` to build the export name, i.e. > "__mod_of__OF_TABLE_device_table". Hence the potential duplicate symbols. > > I think we somehow need to build the module name into the symbol name as well. Something like this? Subject: [PATCH] rust: device: Add the module name to the symbol name Make the symbol name unique by adding the module name to avoid duplicate symbol errors like ld.lld: error: duplicate symbol: __mod_of__OF_TABLE_device_table >>> defined at doctests_kernel_generated.ff18649a828ae8c4-cgu.0 >>> rust/doctests_kernel_generated.o:(__mod_of__OF_TABLE_device_table) in archive vmlinux.a >>> defined at rust_driver_platform.2308c4225c4e08b3-cgu.0 >>> samples/rust/rust_driver_platform.o:(.rodata+0x5A8) in archive vmlinux.a make[2]: *** [scripts/Makefile.vmlinux_o:65: vmlinux.o] Error 1 make[1]: *** [Makefile:1154: vmlinux_o] Error 2 __mod_of__OF_TABLE_device_table is too generic. Add the module name. Proposed-by: Danilo Krummrich <dakr@kernel.org> Link: https://lore.kernel.org/rust-for-linux/Zx9lFG1XKnC_WaG0@pollux/ Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> --- rust/kernel/device_id.rs | 4 ++-- rust/kernel/of.rs | 4 ++-- rust/kernel/pci.rs | 5 +++-- rust/kernel/platform.rs | 1 + samples/rust/rust_driver_pci.rs | 1 + samples/rust/rust_driver_platform.rs | 1 + 6 files changed, 10 insertions(+), 6 deletions(-) diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs index 5b1329fba528..231f34362da9 100644 --- a/rust/kernel/device_id.rs +++ b/rust/kernel/device_id.rs @@ -151,10 +151,10 @@ fn info(&self, index: usize) -> &U { /// Create device table alias for modpost. #[macro_export] macro_rules! module_device_table { - ($table_type: literal, $module_table_name:ident, $table_name:ident) => { + ($table_type: literal, $device_name: literal, $module_table_name:ident, $table_name:ident) => { #[rustfmt::skip] #[export_name = - concat!("__mod_", $table_type, "__", stringify!($table_name), "_device_table") + concat!("__mod_", $table_type, "__", stringify!($table_name), "_", $device_name, "_device_table") ] static $module_table_name: [core::mem::MaybeUninit<u8>; $table_name.raw_ids().size()] = unsafe { core::mem::transmute_copy($table_name.raw_ids()) }; diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs index a37629997974..77679c30638c 100644 --- a/rust/kernel/of.rs +++ b/rust/kernel/of.rs @@ -51,13 +51,13 @@ pub fn compatible<'a>(&self) -> &'a CStr { /// Create an OF `IdTable` with an "alias" for modpost. #[macro_export] macro_rules! of_device_table { - ($table_name:ident, $module_table_name:ident, $id_info_type: ty, $table_data: expr) => { + ($device_name: literal, $table_name:ident, $module_table_name:ident, $id_info_type: ty, $table_data: expr) => { const $table_name: $crate::device_id::IdArray< $crate::of::DeviceId, $id_info_type, { $table_data.len() }, > = $crate::device_id::IdArray::new($table_data); - $crate::module_device_table!("of", $module_table_name, $table_name); + $crate::module_device_table!("of", $device_name, $module_table_name, $table_name); }; } diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs index 58f7d9c0045b..806d192b9600 100644 --- a/rust/kernel/pci.rs +++ b/rust/kernel/pci.rs @@ -176,14 +176,14 @@ fn index(&self) -> usize { /// Create a PCI `IdTable` with its alias for modpost. #[macro_export] macro_rules! pci_device_table { - ($table_name:ident, $module_table_name:ident, $id_info_type: ty, $table_data: expr) => { + ($device_name: literal, $table_name:ident, $module_table_name:ident, $id_info_type: ty, $table_data: expr) => { const $table_name: $crate::device_id::IdArray< $crate::pci::DeviceId, $id_info_type, { $table_data.len() }, > = $crate::device_id::IdArray::new($table_data); - $crate::module_device_table!("pci", $module_table_name, $table_name); + $crate::module_device_table!("pci", $device_name, $module_table_name, $table_name); }; } @@ -197,6 +197,7 @@ macro_rules! pci_device_table { /// struct MyDriver; /// /// kernel::pci_device_table!( +/// "MyDriver", /// PCI_TABLE, /// MODULE_PCI_TABLE, /// <MyDriver as pci::Driver>::IdInfo, diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs index a926233a789f..fcdd3c5da0e5 100644 --- a/rust/kernel/platform.rs +++ b/rust/kernel/platform.rs @@ -118,6 +118,7 @@ macro_rules! module_platform_driver { /// struct MyDriver; /// /// kernel::of_device_table!( +/// "MyDriver", /// OF_TABLE, /// MODULE_OF_TABLE, /// <MyDriver as platform::Driver>::IdInfo, diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs index d24dc1fde9e8..6ee570b59233 100644 --- a/samples/rust/rust_driver_pci.rs +++ b/samples/rust/rust_driver_pci.rs @@ -31,6 +31,7 @@ struct SampleDriver { } kernel::pci_device_table!( + "SampleDriver", PCI_TABLE, MODULE_PCI_TABLE, <SampleDriver as pci::Driver>::IdInfo, diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs index fd7a5ad669fe..9dfbe3b9932b 100644 --- a/samples/rust/rust_driver_platform.rs +++ b/samples/rust/rust_driver_platform.rs @@ -11,6 +11,7 @@ struct SampleDriver { struct Info(u32); kernel::of_device_table!( + "SampleDriver", OF_TABLE, MODULE_OF_TABLE, <SampleDriver as platform::Driver>::IdInfo,
On Tue, Oct 29, 2024 at 08:20:55AM +0100, Dirk Behme wrote: > On 28.10.2024 11:19, Danilo Krummrich wrote: > > On Thu, Oct 24, 2024 at 11:11:50AM +0200, Dirk Behme wrote: > > > > +/// 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, > > > > > > It looks to me that OF_TABLE and MODULE_OF_TABLE are quite generic names > > > used here. Shouldn't they be somehow driver specific, e.g. OF_TABLE_MYDRIVER > > > and MODULE_OF_TABLE_MYDRIVER or whatever? Same for the other > > > examples/samples in this patch series. Found that while using the *same* > > > somewhere else ;) > > > > I think the names by themselves are fine. They're local to the module. However, > > we stringify `OF_TABLE` in `module_device_table` to build the export name, i.e. > > "__mod_of__OF_TABLE_device_table". Hence the potential duplicate symbols. > > > > I think we somehow need to build the module name into the symbol name as well. > > Something like this? No, I think we should just encode the Rust module name / path, which should make this a unique symbol name. diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs index 5b1329fba528..63e81ec2d6fd 100644 --- a/rust/kernel/device_id.rs +++ b/rust/kernel/device_id.rs @@ -154,7 +154,7 @@ macro_rules! module_device_table { ($table_type: literal, $module_table_name:ident, $table_name:ident) => { #[rustfmt::skip] #[export_name = - concat!("__mod_", $table_type, "__", stringify!($table_name), "_device_table") + concat!("__mod_", $table_type, "__", module_path!(), "_", stringify!($table_name), "_device_table") ] static $module_table_name: [core::mem::MaybeUninit<u8>; $table_name.raw_ids().size()] = unsafe { core::mem::transmute_copy($table_name.raw_ids()) }; For the doctests for instance this "__mod_of__OF_TABLE_device_table" becomes "__mod_of__doctests_kernel_generated_OF_TABLE_device_table". > > > Subject: [PATCH] rust: device: Add the module name to the symbol name > > Make the symbol name unique by adding the module name to avoid > duplicate symbol errors like > > ld.lld: error: duplicate symbol: __mod_of__OF_TABLE_device_table > >>> defined at doctests_kernel_generated.ff18649a828ae8c4-cgu.0 > >>> rust/doctests_kernel_generated.o:(__mod_of__OF_TABLE_device_table) in > archive vmlinux.a > >>> defined at rust_driver_platform.2308c4225c4e08b3-cgu.0 > >>> samples/rust/rust_driver_platform.o:(.rodata+0x5A8) in > archive vmlinux.a > make[2]: *** [scripts/Makefile.vmlinux_o:65: vmlinux.o] Error 1 > make[1]: *** [Makefile:1154: vmlinux_o] Error 2 > > __mod_of__OF_TABLE_device_table is too generic. Add the module name. > > Proposed-by: Danilo Krummrich <dakr@kernel.org> > Link: https://lore.kernel.org/rust-for-linux/Zx9lFG1XKnC_WaG0@pollux/ > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com> > --- > rust/kernel/device_id.rs | 4 ++-- > rust/kernel/of.rs | 4 ++-- > rust/kernel/pci.rs | 5 +++-- > rust/kernel/platform.rs | 1 + > samples/rust/rust_driver_pci.rs | 1 + > samples/rust/rust_driver_platform.rs | 1 + > 6 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs > index 5b1329fba528..231f34362da9 100644 > --- a/rust/kernel/device_id.rs > +++ b/rust/kernel/device_id.rs > @@ -151,10 +151,10 @@ fn info(&self, index: usize) -> &U { > /// Create device table alias for modpost. > #[macro_export] > macro_rules! module_device_table { > - ($table_type: literal, $module_table_name:ident, $table_name:ident) => > { > + ($table_type: literal, $device_name: literal, $module_table_name:ident, > $table_name:ident) => { > #[rustfmt::skip] > #[export_name = > - concat!("__mod_", $table_type, "__", stringify!($table_name), > "_device_table") > + concat!("__mod_", $table_type, "__", stringify!($table_name), > "_", $device_name, "_device_table") > ] > static $module_table_name: [core::mem::MaybeUninit<u8>; > $table_name.raw_ids().size()] = > unsafe { core::mem::transmute_copy($table_name.raw_ids()) }; > diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs > index a37629997974..77679c30638c 100644 > --- a/rust/kernel/of.rs > +++ b/rust/kernel/of.rs > @@ -51,13 +51,13 @@ pub fn compatible<'a>(&self) -> &'a CStr { > /// Create an OF `IdTable` with an "alias" for modpost. > #[macro_export] > macro_rules! of_device_table { > - ($table_name:ident, $module_table_name:ident, $id_info_type: ty, > $table_data: expr) => { > + ($device_name: literal, $table_name:ident, $module_table_name:ident, > $id_info_type: ty, $table_data: expr) => { > const $table_name: $crate::device_id::IdArray< > $crate::of::DeviceId, > $id_info_type, > { $table_data.len() }, > > = $crate::device_id::IdArray::new($table_data); > > - $crate::module_device_table!("of", $module_table_name, > $table_name); > + $crate::module_device_table!("of", $device_name, > $module_table_name, $table_name); > }; > } > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs > index 58f7d9c0045b..806d192b9600 100644 > --- a/rust/kernel/pci.rs > +++ b/rust/kernel/pci.rs > @@ -176,14 +176,14 @@ fn index(&self) -> usize { > /// Create a PCI `IdTable` with its alias for modpost. > #[macro_export] > macro_rules! pci_device_table { > - ($table_name:ident, $module_table_name:ident, $id_info_type: ty, > $table_data: expr) => { > + ($device_name: literal, $table_name:ident, $module_table_name:ident, > $id_info_type: ty, $table_data: expr) => { > const $table_name: $crate::device_id::IdArray< > $crate::pci::DeviceId, > $id_info_type, > { $table_data.len() }, > > = $crate::device_id::IdArray::new($table_data); > > - $crate::module_device_table!("pci", $module_table_name, > $table_name); > + $crate::module_device_table!("pci", $device_name, > $module_table_name, $table_name); > }; > } > > @@ -197,6 +197,7 @@ macro_rules! pci_device_table { > /// struct MyDriver; > /// > /// kernel::pci_device_table!( > +/// "MyDriver", > /// PCI_TABLE, > /// MODULE_PCI_TABLE, > /// <MyDriver as pci::Driver>::IdInfo, > diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs > index a926233a789f..fcdd3c5da0e5 100644 > --- a/rust/kernel/platform.rs > +++ b/rust/kernel/platform.rs > @@ -118,6 +118,7 @@ macro_rules! module_platform_driver { > /// struct MyDriver; > /// > /// kernel::of_device_table!( > +/// "MyDriver", > /// OF_TABLE, > /// MODULE_OF_TABLE, > /// <MyDriver as platform::Driver>::IdInfo, > diff --git a/samples/rust/rust_driver_pci.rs > b/samples/rust/rust_driver_pci.rs > index d24dc1fde9e8..6ee570b59233 100644 > --- a/samples/rust/rust_driver_pci.rs > +++ b/samples/rust/rust_driver_pci.rs > @@ -31,6 +31,7 @@ struct SampleDriver { > } > > kernel::pci_device_table!( > + "SampleDriver", > PCI_TABLE, > MODULE_PCI_TABLE, > <SampleDriver as pci::Driver>::IdInfo, > diff --git a/samples/rust/rust_driver_platform.rs > b/samples/rust/rust_driver_platform.rs > index fd7a5ad669fe..9dfbe3b9932b 100644 > --- a/samples/rust/rust_driver_platform.rs > +++ b/samples/rust/rust_driver_platform.rs > @@ -11,6 +11,7 @@ struct SampleDriver { > struct Info(u32); > > kernel::of_device_table!( > + "SampleDriver", > OF_TABLE, > MODULE_OF_TABLE, > <SampleDriver as platform::Driver>::IdInfo, > -- > 2.46.2 >
On 29.10.2024 09:50, Danilo Krummrich wrote: > On Tue, Oct 29, 2024 at 08:20:55AM +0100, Dirk Behme wrote: >> On 28.10.2024 11:19, Danilo Krummrich wrote: >>> On Thu, Oct 24, 2024 at 11:11:50AM +0200, Dirk Behme wrote: >>>>> +/// 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, >>>> >>>> It looks to me that OF_TABLE and MODULE_OF_TABLE are quite generic names >>>> used here. Shouldn't they be somehow driver specific, e.g. OF_TABLE_MYDRIVER >>>> and MODULE_OF_TABLE_MYDRIVER or whatever? Same for the other >>>> examples/samples in this patch series. Found that while using the *same* >>>> somewhere else ;) >>> >>> I think the names by themselves are fine. They're local to the module. However, >>> we stringify `OF_TABLE` in `module_device_table` to build the export name, i.e. >>> "__mod_of__OF_TABLE_device_table". Hence the potential duplicate symbols. >>> >>> I think we somehow need to build the module name into the symbol name as well. >> >> Something like this? > > No, I think we should just encode the Rust module name / path, which should make > this a unique symbol name. > > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs > index 5b1329fba528..63e81ec2d6fd 100644 > --- a/rust/kernel/device_id.rs > +++ b/rust/kernel/device_id.rs > @@ -154,7 +154,7 @@ macro_rules! module_device_table { > ($table_type: literal, $module_table_name:ident, $table_name:ident) => { > #[rustfmt::skip] > #[export_name = > - concat!("__mod_", $table_type, "__", stringify!($table_name), "_device_table") > + concat!("__mod_", $table_type, "__", module_path!(), "_", stringify!($table_name), "_device_table") > ] > static $module_table_name: [core::mem::MaybeUninit<u8>; $table_name.raw_ids().size()] = > unsafe { core::mem::transmute_copy($table_name.raw_ids()) }; > > For the doctests for instance this > > "__mod_of__OF_TABLE_device_table" > > becomes > > "__mod_of__doctests_kernel_generated_OF_TABLE_device_table". What implies *one* OF/PCI_TABLE per path (file)? For example adding a second FooDriver example to platform.rs won't be possible? +/// struct FooDriver; +/// +/// kernel::of_device_table!( +/// OF_TABLE, +/// MODULE_OF_TABLE, +/// <FooDriver as platform::Driver>::IdInfo, +/// [ +/// (of::DeviceId::new(c_str!("test,rust-device2")), ()) +/// ] +/// ); Best regards Dirk
On Tue, Oct 29, 2024 at 10:19:08AM +0100, Dirk Behme wrote: > On 29.10.2024 09:50, Danilo Krummrich wrote: > > On Tue, Oct 29, 2024 at 08:20:55AM +0100, Dirk Behme wrote: > > > On 28.10.2024 11:19, Danilo Krummrich wrote: > > > > On Thu, Oct 24, 2024 at 11:11:50AM +0200, Dirk Behme wrote: > > > > > > +/// 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, > > > > > > > > > > It looks to me that OF_TABLE and MODULE_OF_TABLE are quite generic names > > > > > used here. Shouldn't they be somehow driver specific, e.g. OF_TABLE_MYDRIVER > > > > > and MODULE_OF_TABLE_MYDRIVER or whatever? Same for the other > > > > > examples/samples in this patch series. Found that while using the *same* > > > > > somewhere else ;) > > > > > > > > I think the names by themselves are fine. They're local to the module. However, > > > > we stringify `OF_TABLE` in `module_device_table` to build the export name, i.e. > > > > "__mod_of__OF_TABLE_device_table". Hence the potential duplicate symbols. > > > > > > > > I think we somehow need to build the module name into the symbol name as well. > > > > > > Something like this? > > > > No, I think we should just encode the Rust module name / path, which should make > > this a unique symbol name. > > > > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs > > index 5b1329fba528..63e81ec2d6fd 100644 > > --- a/rust/kernel/device_id.rs > > +++ b/rust/kernel/device_id.rs > > @@ -154,7 +154,7 @@ macro_rules! module_device_table { > > ($table_type: literal, $module_table_name:ident, $table_name:ident) => { > > #[rustfmt::skip] > > #[export_name = > > - concat!("__mod_", $table_type, "__", stringify!($table_name), "_device_table") > > + concat!("__mod_", $table_type, "__", module_path!(), "_", stringify!($table_name), "_device_table") > > ] > > static $module_table_name: [core::mem::MaybeUninit<u8>; $table_name.raw_ids().size()] = > > unsafe { core::mem::transmute_copy($table_name.raw_ids()) }; > > > > For the doctests for instance this > > > > "__mod_of__OF_TABLE_device_table" > > > > becomes > > > > "__mod_of__doctests_kernel_generated_OF_TABLE_device_table". > > > What implies *one* OF/PCI_TABLE per path (file)? No, you can still have as many as you want for the same file, you just have to give them different identifier names -- you can't have two statics with the same name in one file anyways. Well, I guess you somehow can (just like the doctests do), but it does make sense to declare drivers in such a way. I think as long as we take care that separate Rust modules can't interfere with each other it's good enough. > > For example adding a second FooDriver example to platform.rs won't be > possible? Not unless you change the identifier name unfortunately. But that might be fixable by putting doctests in separate `mod $(DOCTEST) {}` blocks. > > +/// struct FooDriver; > +/// > +/// kernel::of_device_table!( > +/// OF_TABLE, > +/// MODULE_OF_TABLE, > +/// <FooDriver as platform::Driver>::IdInfo, > +/// [ > +/// (of::DeviceId::new(c_str!("test,rust-device2")), ()) > +/// ] > +/// ); > > Best regards > > Dirk > > > >
On Tue, Oct 29, 2024 at 10:50:11AM +0100, Danilo Krummrich wrote: > On Tue, Oct 29, 2024 at 10:19:08AM +0100, Dirk Behme wrote: > > On 29.10.2024 09:50, Danilo Krummrich wrote: > > > On Tue, Oct 29, 2024 at 08:20:55AM +0100, Dirk Behme wrote: > > > > On 28.10.2024 11:19, Danilo Krummrich wrote: > > > > > On Thu, Oct 24, 2024 at 11:11:50AM +0200, Dirk Behme wrote: > > > > > > > +/// 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, > > > > > > > > > > > > It looks to me that OF_TABLE and MODULE_OF_TABLE are quite generic names > > > > > > used here. Shouldn't they be somehow driver specific, e.g. OF_TABLE_MYDRIVER > > > > > > and MODULE_OF_TABLE_MYDRIVER or whatever? Same for the other > > > > > > examples/samples in this patch series. Found that while using the *same* > > > > > > somewhere else ;) > > > > > > > > > > I think the names by themselves are fine. They're local to the module. However, > > > > > we stringify `OF_TABLE` in `module_device_table` to build the export name, i.e. > > > > > "__mod_of__OF_TABLE_device_table". Hence the potential duplicate symbols. > > > > > > > > > > I think we somehow need to build the module name into the symbol name as well. > > > > > > > > Something like this? > > > > > > No, I think we should just encode the Rust module name / path, which should make > > > this a unique symbol name. > > > > > > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs > > > index 5b1329fba528..63e81ec2d6fd 100644 > > > --- a/rust/kernel/device_id.rs > > > +++ b/rust/kernel/device_id.rs > > > @@ -154,7 +154,7 @@ macro_rules! module_device_table { > > > ($table_type: literal, $module_table_name:ident, $table_name:ident) => { > > > #[rustfmt::skip] > > > #[export_name = > > > - concat!("__mod_", $table_type, "__", stringify!($table_name), "_device_table") > > > + concat!("__mod_", $table_type, "__", module_path!(), "_", stringify!($table_name), "_device_table") > > > ] > > > static $module_table_name: [core::mem::MaybeUninit<u8>; $table_name.raw_ids().size()] = > > > unsafe { core::mem::transmute_copy($table_name.raw_ids()) }; > > > > > > For the doctests for instance this > > > > > > "__mod_of__OF_TABLE_device_table" > > > > > > becomes > > > > > > "__mod_of__doctests_kernel_generated_OF_TABLE_device_table". > > > > > > What implies *one* OF/PCI_TABLE per path (file)? > > No, you can still have as many as you want for the same file, you just have to > give them different identifier names -- you can't have two statics with the same > name in one file anyways. > > Well, I guess you somehow can (just like the doctests do), but it does make > sense to declare drivers in such a way. > > I think as long as we take care that separate Rust modules can't interfere with > each other it's good enough. > > > > > For example adding a second FooDriver example to platform.rs won't be > > possible? > > Not unless you change the identifier name unfortunately. But that might be > fixable by putting doctests in separate `mod $(DOCTEST) {}` blocks. Another option would be to not only encode the module path, but also the line number: diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs index 5b1329fba528..7956edbbad52 100644 --- a/rust/kernel/device_id.rs +++ b/rust/kernel/device_id.rs @@ -154,7 +154,7 @@ macro_rules! module_device_table { ($table_type: literal, $module_table_name:ident, $table_name:ident) => { #[rustfmt::skip] #[export_name = - concat!("__mod_", $table_type, "__", stringify!($table_name), "_device_table") + concat!("__mod_", $table_type, "__", module_path!(), "_", line!(), "_", stringify!($table_name), "_device_table") ] static $module_table_name: [core::mem::MaybeUninit<u8>; $table_name.raw_ids().size()] = unsafe { core::mem::transmute_copy($table_name.raw_ids()) }; This way you'll get "__mod_of__doctests_kernel_generated_3875_OF_TABLE_device_table" "__mod_of__doctests_kernel_generated_3946_OF_TABLE_device_table" if you put identical doctests. > > > > > +/// struct FooDriver; > > +/// > > +/// kernel::of_device_table!( > > +/// OF_TABLE, > > +/// MODULE_OF_TABLE, > > +/// <FooDriver as platform::Driver>::IdInfo, > > +/// [ > > +/// (of::DeviceId::new(c_str!("test,rust-device2")), ()) > > +/// ] > > +/// ); > > > > Best regards > > > > Dirk > > > > > > > >
On 29.10.2024 10:55, Danilo Krummrich wrote: > On Tue, Oct 29, 2024 at 10:50:11AM +0100, Danilo Krummrich wrote: >> On Tue, Oct 29, 2024 at 10:19:08AM +0100, Dirk Behme wrote: >>> On 29.10.2024 09:50, Danilo Krummrich wrote: >>>> On Tue, Oct 29, 2024 at 08:20:55AM +0100, Dirk Behme wrote: >>>>> On 28.10.2024 11:19, Danilo Krummrich wrote: >>>>>> On Thu, Oct 24, 2024 at 11:11:50AM +0200, Dirk Behme wrote: >>>>>>>> +/// 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, >>>>>>> >>>>>>> It looks to me that OF_TABLE and MODULE_OF_TABLE are quite generic names >>>>>>> used here. Shouldn't they be somehow driver specific, e.g. OF_TABLE_MYDRIVER >>>>>>> and MODULE_OF_TABLE_MYDRIVER or whatever? Same for the other >>>>>>> examples/samples in this patch series. Found that while using the *same* >>>>>>> somewhere else ;) >>>>>> >>>>>> I think the names by themselves are fine. They're local to the module. However, >>>>>> we stringify `OF_TABLE` in `module_device_table` to build the export name, i.e. >>>>>> "__mod_of__OF_TABLE_device_table". Hence the potential duplicate symbols. >>>>>> >>>>>> I think we somehow need to build the module name into the symbol name as well. >>>>> >>>>> Something like this? >>>> >>>> No, I think we should just encode the Rust module name / path, which should make >>>> this a unique symbol name. >>>> >>>> diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs >>>> index 5b1329fba528..63e81ec2d6fd 100644 >>>> --- a/rust/kernel/device_id.rs >>>> +++ b/rust/kernel/device_id.rs >>>> @@ -154,7 +154,7 @@ macro_rules! module_device_table { >>>> ($table_type: literal, $module_table_name:ident, $table_name:ident) => { >>>> #[rustfmt::skip] >>>> #[export_name = >>>> - concat!("__mod_", $table_type, "__", stringify!($table_name), "_device_table") >>>> + concat!("__mod_", $table_type, "__", module_path!(), "_", stringify!($table_name), "_device_table") >>>> ] >>>> static $module_table_name: [core::mem::MaybeUninit<u8>; $table_name.raw_ids().size()] = >>>> unsafe { core::mem::transmute_copy($table_name.raw_ids()) }; >>>> >>>> For the doctests for instance this >>>> >>>> "__mod_of__OF_TABLE_device_table" >>>> >>>> becomes >>>> >>>> "__mod_of__doctests_kernel_generated_OF_TABLE_device_table". >>> >>> >>> What implies *one* OF/PCI_TABLE per path (file)? >> >> No, you can still have as many as you want for the same file, you just have to >> give them different identifier names -- you can't have two statics with the same >> name in one file anyways. >> >> Well, I guess you somehow can (just like the doctests do), but it does make >> sense to declare drivers in such a way. >> >> I think as long as we take care that separate Rust modules can't interfere with >> each other it's good enough. >> >>> >>> For example adding a second FooDriver example to platform.rs won't be >>> possible? >> >> Not unless you change the identifier name unfortunately. But that might be >> fixable by putting doctests in separate `mod $(DOCTEST) {}` blocks. > > Another option would be to not only encode the module path, but also the line > number: > > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs > index 5b1329fba528..7956edbbad52 100644 > --- a/rust/kernel/device_id.rs > +++ b/rust/kernel/device_id.rs > @@ -154,7 +154,7 @@ macro_rules! module_device_table { > ($table_type: literal, $module_table_name:ident, $table_name:ident) => { > #[rustfmt::skip] > #[export_name = > - concat!("__mod_", $table_type, "__", stringify!($table_name), "_device_table") > + concat!("__mod_", $table_type, "__", module_path!(), "_", line!(), "_", stringify!($table_name), "_device_table") > ] > static $module_table_name: [core::mem::MaybeUninit<u8>; $table_name.raw_ids().size()] = > unsafe { core::mem::transmute_copy($table_name.raw_ids()) }; > > This way you'll get > > "__mod_of__doctests_kernel_generated_3875_OF_TABLE_device_table" > "__mod_of__doctests_kernel_generated_3946_OF_TABLE_device_table" Yes, if we want to avoid adding a unique name that makes sense and is a good "automatic" option :) Thanks Dirk
On Tue, Oct 22, 2024 at 11:33 PM Danilo Krummrich <dakr@kernel.org> wrote: > + /// Find the [`of::DeviceId`] within [`Driver::ID_TABLE`] matching the given [`Device`], if any. > + fn of_match_device(pdev: &Device) -> Option<&of::DeviceId> { > + 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()) }; > + > + 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>() }) > + } > + } Sorry if this has already been mentioned, but this method should probably be marked #[cfg(CONFIG_OF)] so that you can use these abstractions even if OF is disabled. Alice
On Mon, Oct 28, 2024 at 5:15 AM Danilo Krummrich <dakr@kernel.org> wrote: > > On Wed, Oct 23, 2024 at 09:23:55AM -0500, Rob Herring wrote: > > On Wed, Oct 23, 2024 at 08:44:42AM +0200, Danilo Krummrich wrote: > > > 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: > > > > > +/// ] > > > > > +/// ); > > > > > +/// > > > > > +/// 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>; > > > > Another thing. I don't think this is quite right. Well, this part is > > fine, but assigning the DT table to it is not. The underlying C code has > > 2 id tables in struct device_driver (DT and ACPI) and then the bus > > specific one in the struct ${bus}_driver. > > The assignment of this table in `Adapter::register` looks like this: > > `pdrv.driver.of_match_table = T::ID_TABLE.as_ptr();` > > What do you think is wrong with this assignment? Every bus implementation will need the DT and ACPI tables, so they should not be declared and assigned in platform driver code, but in the generic device/driver abstractions just like the underlying C code. The one here should be for platform_device_id. You could put all 3 tables here, but that's going to be a lot of duplication I think. > > > > > > > + > > > > > + /// 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. > > > > TBC, you mean other than *data, right? If so, I agree. > > Yes. > > > > > The DT type and name fields are pretty much legacy, so I don't think the > > rust bindings need to worry about them until someone converts Sparc and > > PowerMac drivers to rust (i.e. never). > > > > I would guess the PCI cases might be questionable, too. Like DT, drivers > > may be accessing the table fields, but that's not best practice. All the > > match fields are stored in pci_dev, so why get them from the match > > table? > > Fair question, I'd like to forward it to Greg. IIRC, he explicitly requested to > make the corresponding struct pci_device_id available in probe() at Kangrejos. Which table gets passed in though? Is the IdInfo parameter generic and can be platform_device_id, of_device_id or acpi_device_id? Not sure if that's possible in rust or not. PCI is the exception, not the rule here, in that it only matches with pci_device_id. At least I think that is the case currently, but it is entirely possible we may want to do ACPI/DT matching like every other bus. There are cases where PCI devices are described in DT. Rob
On Tue, Oct 29, 2024 at 4:19 AM Dirk Behme <dirk.behme@de.bosch.com> wrote: > > On 29.10.2024 09:50, Danilo Krummrich wrote: > > On Tue, Oct 29, 2024 at 08:20:55AM +0100, Dirk Behme wrote: > >> On 28.10.2024 11:19, Danilo Krummrich wrote: > >>> On Thu, Oct 24, 2024 at 11:11:50AM +0200, Dirk Behme wrote: > >>>>> +/// 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, > >>>> > >>>> It looks to me that OF_TABLE and MODULE_OF_TABLE are quite generic names > >>>> used here. Shouldn't they be somehow driver specific, e.g. OF_TABLE_MYDRIVER > >>>> and MODULE_OF_TABLE_MYDRIVER or whatever? Same for the other > >>>> examples/samples in this patch series. Found that while using the *same* > >>>> somewhere else ;) > >>> > >>> I think the names by themselves are fine. They're local to the module. However, > >>> we stringify `OF_TABLE` in `module_device_table` to build the export name, i.e. > >>> "__mod_of__OF_TABLE_device_table". Hence the potential duplicate symbols. > >>> > >>> I think we somehow need to build the module name into the symbol name as well. > >> > >> Something like this? > > > > No, I think we should just encode the Rust module name / path, which should make > > this a unique symbol name. > > > > diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs > > index 5b1329fba528..63e81ec2d6fd 100644 > > --- a/rust/kernel/device_id.rs > > +++ b/rust/kernel/device_id.rs > > @@ -154,7 +154,7 @@ macro_rules! module_device_table { > > ($table_type: literal, $module_table_name:ident, $table_name:ident) => { > > #[rustfmt::skip] > > #[export_name = > > - concat!("__mod_", $table_type, "__", stringify!($table_name), "_device_table") > > + concat!("__mod_", $table_type, "__", module_path!(), "_", stringify!($table_name), "_device_table") > > ] > > static $module_table_name: [core::mem::MaybeUninit<u8>; $table_name.raw_ids().size()] = > > unsafe { core::mem::transmute_copy($table_name.raw_ids()) }; > > > > For the doctests for instance this > > > > "__mod_of__OF_TABLE_device_table" > > > > becomes > > > > "__mod_of__doctests_kernel_generated_OF_TABLE_device_table". > > > What implies *one* OF/PCI_TABLE per path (file)? It's generally one per module, but it's one per type because it is one type per driver. So platform (and most other) drivers can have $bus, DT, and ACPI tables. While you could have 1 module with N drivers, I don't think I've ever seen that case and certainly not something we'd encourage. Perhaps it is just not possible to disallow in C, but we can in rust? That may be a benefit, not a limitation. Rob
On Tue, Oct 22, 2024 at 11:33 PM Danilo Krummrich <dakr@kernel.org> wrote: > +/// 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>>>; This forces the user to put their driver data in a KBox, but they might want to use an Arc instead. You don't actually *need* a KBox - any ForeignOwnable seems to fit your purposes. Please see my miscdevice and shrinker patchsets for examples of how you can extend this to allow any ForeignOwnable. Alice
On Wed, Oct 30, 2024 at 04:50:43PM +0100, Alice Ryhl wrote: > On Tue, Oct 22, 2024 at 11:33 PM Danilo Krummrich <dakr@kernel.org> wrote: > > +/// 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>>>; > > This forces the user to put their driver data in a KBox, but they > might want to use an Arc instead. You don't actually *need* a KBox - > any ForeignOwnable seems to fit your purposes. This is intentional, I do need a `KBox` here. The reason is that I want to enforce that the returned `Pin<KBox<Self>>` has exactly the lifetime of the binding of the device and driver, i.e. from probe() until remove(). This is the lifetime the structure should actually represent. This way we can attach things like `Registration` objects to this structure, or anything else that should only exist from probe() until remove(). If a driver needs some private driver data that needs to be reference counted, it is usually attached to the class representation of the driver. For instance, in Nova the reference counted stuff is attached to the DRM device and then I just have the DRM device (which itself is reference counted) embedded in the `Driver` structure. In any case, drivers can always embed a separate `Arc` in their `Driver` structure if they really have a need for that. - Danilo > > Please see my miscdevice and shrinker patchsets for examples of how > you can extend this to allow any ForeignOwnable. > > Alice >
On Wed, Oct 30, 2024 at 7:07 PM Danilo Krummrich <dakr@kernel.org> wrote: > > On Wed, Oct 30, 2024 at 04:50:43PM +0100, Alice Ryhl wrote: > > On Tue, Oct 22, 2024 at 11:33 PM Danilo Krummrich <dakr@kernel.org> wrote: > > > +/// 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>>>; > > > > This forces the user to put their driver data in a KBox, but they > > might want to use an Arc instead. You don't actually *need* a KBox - > > any ForeignOwnable seems to fit your purposes. > > This is intentional, I do need a `KBox` here. > > The reason is that I want to enforce that the returned `Pin<KBox<Self>>` has > exactly the lifetime of the binding of the device and driver, i.e. from probe() > until remove(). This is the lifetime the structure should actually represent. > > This way we can attach things like `Registration` objects to this structure, or > anything else that should only exist from probe() until remove(). > > If a driver needs some private driver data that needs to be reference counted, > it is usually attached to the class representation of the driver. > > For instance, in Nova the reference counted stuff is attached to the DRM device > and then I just have the DRM device (which itself is reference counted) embedded > in the `Driver` structure. > > In any case, drivers can always embed a separate `Arc` in their `Driver` > structure if they really have a need for that. Is this needed for soundness of those registrations? Also, I've often seen drivers use devm_kzalloc or similar to allocate exactly this object. KBox doesn't allow for that. Alice
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@ti.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")), ()) +/// ] +/// ); +/// +/// 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> { + 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()) }; + + 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>() }) + } + } +} + +/// The platform device representation. +/// +/// A platform device is based on an always reference counted `device:Device` instance. Cloning a +/// platform device, hence, also increments the base device' reference count. +/// +/// # Invariants +/// +/// `Device` holds a valid reference of `ARef<device::Device>` whose underlying `struct device` is a +/// member of a `struct platform_device`. +#[derive(Clone)] +pub struct Device(ARef<device::Device>); + +impl Device { + /// Convert a raw kernel device into a `Device` + /// + /// # Safety + /// + /// `dev` must be an `Aref<device::Device>` whose underlying `bindings::device` is a member of a + /// `bindings::platform_device`. + unsafe fn from_dev(dev: ARef<device::Device>) -> Self { + Self(dev) + } + + fn as_dev(&self) -> &device::Device { + &self.0 + } + + fn as_raw(&self) -> *mut bindings::platform_device { + // SAFETY: By the type invariant `self.0.as_raw` is a pointer to the `struct device` + // embedded in `struct platform_device`. + unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut() + } +} + +impl AsRef<device::Device> for Device { + fn as_ref(&self) -> &device::Device { + &self.0 + } +}
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 | 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