| Message ID | 20241022213221.2383-17-dakr@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Device / Driver PCI / Platform Rust abstractions | expand |
On Tue, Oct 22, 2024 at 11:31:53PM +0200, Danilo Krummrich wrote: > Add a sample Rust platform driver illustrating the usage of the platform > bus abstractions. > > This driver probes through either a match of device / driver name or a > match within the OF ID table. I know if rust compiles it works, but how does one actually use/test this? (I know ways, but I might be in the minority. :) ) The DT unittests already define test platform devices. I'd be happy to add a device node there. Then you don't have to muck with the DT on some device and it even works on x86 or UML. And I've started working on DT (fwnode really) property API bindings as well, and this will be great to test them with. > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > --- > MAINTAINERS | 1 + > samples/rust/Kconfig | 10 +++++ > samples/rust/Makefile | 1 + > samples/rust/rust_driver_platform.rs | 62 ++++++++++++++++++++++++++++ > 4 files changed, 74 insertions(+) > create mode 100644 samples/rust/rust_driver_platform.rs > > diff --git a/MAINTAINERS b/MAINTAINERS > index 173540375863..583b6588fd1e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -6986,6 +6986,7 @@ F: rust/kernel/device_id.rs > F: rust/kernel/devres.rs > F: rust/kernel/driver.rs > F: rust/kernel/platform.rs > +F: samples/rust/rust_driver_platform.rs > > DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS) > M: Nishanth Menon <nm@ti.com> > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig > index 6d468193cdd8..70126b750426 100644 > --- a/samples/rust/Kconfig > +++ b/samples/rust/Kconfig > @@ -41,6 +41,16 @@ config SAMPLE_RUST_DRIVER_PCI > > If unsure, say N. > > +config SAMPLE_RUST_DRIVER_PLATFORM > + tristate "Platform Driver" > + help > + This option builds the Rust Platform driver sample. > + > + To compile this as a module, choose M here: > + the module will be called rust_driver_platform. > + > + If unsure, say N. > + > config SAMPLE_RUST_HOSTPROGS > bool "Host programs" > help > diff --git a/samples/rust/Makefile b/samples/rust/Makefile > index b66767f4a62a..11fcb312ed36 100644 > --- a/samples/rust/Makefile > +++ b/samples/rust/Makefile > @@ -3,5 +3,6 @@ > obj-$(CONFIG_SAMPLE_RUST_MINIMAL) += rust_minimal.o > obj-$(CONFIG_SAMPLE_RUST_PRINT) += rust_print.o > obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI) += rust_driver_pci.o > +obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM) += rust_driver_platform.o > > subdir-$(CONFIG_SAMPLE_RUST_HOSTPROGS) += hostprogs > diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs > new file mode 100644 > index 000000000000..55caaaa4f216 > --- /dev/null > +++ b/samples/rust/rust_driver_platform.rs > @@ -0,0 +1,62 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Rust Platform driver sample. > + > +use kernel::{c_str, of, platform, prelude::*}; > + > +struct SampleDriver { > + pdev: platform::Device, > +} > + > +struct Info(u32); > + > +kernel::of_device_table!( > + OF_TABLE, > + MODULE_OF_TABLE, > + <SampleDriver as platform::Driver>::IdInfo, > + [( > + of::DeviceId::new(c_str!("redhat,rust-sample-platform-driver")), Perhaps use the same compatible as the commented example. Same comments on that apply to this. > + Info(42) Most of the time this is a pointer to a struct. It would be better to show how to do that. > + )] > +); > + > +impl platform::Driver for SampleDriver { > + type IdInfo = Info; > + const ID_TABLE: platform::IdTable<Self::IdInfo> = &OF_TABLE; Probably want to name this OF_ID_TABLE for when ACPI_ID_TABLE is added. > + > + fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>> { > + dev_dbg!(pdev.as_ref(), "Probe Rust Platform driver sample.\n"); > + > + match (Self::of_match_device(pdev), info) { That answers my question on being exposed to drivers. This is a big no for me. > + (Some(id), Some(info)) => { > + dev_info!( > + pdev.as_ref(), > + "Probed by OF compatible match: '{}' with info: '{}'.\n", > + id.compatible(), As I mentioned, "real" drivers don't need the compatible string. > + info.0 > + ); > + } > + _ => { > + dev_info!(pdev.as_ref(), "Probed by name.\n"); > + } > + }; > + > + let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?; > + > + Ok(drvdata.into()) > + } > +} > + > +impl Drop for SampleDriver { > + fn drop(&mut self) { > + dev_dbg!(self.pdev.as_ref(), "Remove Rust Platform driver sample.\n"); > + } > +} > + > +kernel::module_platform_driver! { > + type: SampleDriver, > + name: "rust_driver_platform", > + author: "Danilo Krummrich", > + description: "Rust Platform driver", > + license: "GPL v2", > +} > -- > 2.46.2 >
On Tue, Oct 22, 2024 at 07:04:08PM -0500, Rob Herring wrote: > On Tue, Oct 22, 2024 at 11:31:53PM +0200, Danilo Krummrich wrote: > > Add a sample Rust platform driver illustrating the usage of the platform > > bus abstractions. > > > > This driver probes through either a match of device / driver name or a > > match within the OF ID table. > > I know if rust compiles it works, but how does one actually use/test > this? (I know ways, but I might be in the minority. :) ) For testing a name match I just used platform_device_register_simple() in a separate module. Probing through the OF table is indeed a bit more tricky. Since I was too lazy to pull out a random ARM device of my cupboard I just used QEMU on x86 and did what drivers/of/unittest.c does. If you're smart you can also just enable those unit tests and change the compatible string to "unittest". :) > > The DT unittests already define test platform devices. I'd be happy to > add a device node there. Then you don't have to muck with the DT on some > device and it even works on x86 or UML. Sounds good, I'll add one in there for this sample driver -- any preferences? > > And I've started working on DT (fwnode really) property API bindings as > well, and this will be great to test them with. > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org> > > --- > > MAINTAINERS | 1 + > > samples/rust/Kconfig | 10 +++++ > > samples/rust/Makefile | 1 + > > samples/rust/rust_driver_platform.rs | 62 ++++++++++++++++++++++++++++ > > 4 files changed, 74 insertions(+) > > create mode 100644 samples/rust/rust_driver_platform.rs > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 173540375863..583b6588fd1e 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -6986,6 +6986,7 @@ F: rust/kernel/device_id.rs > > F: rust/kernel/devres.rs > > F: rust/kernel/driver.rs > > F: rust/kernel/platform.rs > > +F: samples/rust/rust_driver_platform.rs > > > > DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS) > > M: Nishanth Menon <nm@ti.com> > > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig > > index 6d468193cdd8..70126b750426 100644 > > --- a/samples/rust/Kconfig > > +++ b/samples/rust/Kconfig > > @@ -41,6 +41,16 @@ config SAMPLE_RUST_DRIVER_PCI > > > > If unsure, say N. > > > > +config SAMPLE_RUST_DRIVER_PLATFORM > > + tristate "Platform Driver" > > + help > > + This option builds the Rust Platform driver sample. > > + > > + To compile this as a module, choose M here: > > + the module will be called rust_driver_platform. > > + > > + If unsure, say N. > > + > > config SAMPLE_RUST_HOSTPROGS > > bool "Host programs" > > help > > diff --git a/samples/rust/Makefile b/samples/rust/Makefile > > index b66767f4a62a..11fcb312ed36 100644 > > --- a/samples/rust/Makefile > > +++ b/samples/rust/Makefile > > @@ -3,5 +3,6 @@ > > obj-$(CONFIG_SAMPLE_RUST_MINIMAL) += rust_minimal.o > > obj-$(CONFIG_SAMPLE_RUST_PRINT) += rust_print.o > > obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI) += rust_driver_pci.o > > +obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM) += rust_driver_platform.o > > > > subdir-$(CONFIG_SAMPLE_RUST_HOSTPROGS) += hostprogs > > diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs > > new file mode 100644 > > index 000000000000..55caaaa4f216 > > --- /dev/null > > +++ b/samples/rust/rust_driver_platform.rs > > @@ -0,0 +1,62 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +//! Rust Platform driver sample. > > + > > +use kernel::{c_str, of, platform, prelude::*}; > > + > > +struct SampleDriver { > > + pdev: platform::Device, > > +} > > + > > +struct Info(u32); > > + > > +kernel::of_device_table!( > > + OF_TABLE, > > + MODULE_OF_TABLE, > > + <SampleDriver as platform::Driver>::IdInfo, > > + [( > > + of::DeviceId::new(c_str!("redhat,rust-sample-platform-driver")), > > Perhaps use the same compatible as the commented example. Same comments > on that apply to this. > > > + Info(42) > > Most of the time this is a pointer to a struct. It would be better to > show how to do that. No, this should never be a raw pointer. There is no reason for a driver to perfom this kind of unsafe operation to store any ID info data. This ID info data is moved into the `IdArray` on compile time. And the bus abstraction takes care of providing a reference to this structure in `Driver::probe`. So, technically, this example is fine. But if you have ideas for more meaningful data to store there, I happy to change it. > > > + )] > > +); > > + > > +impl platform::Driver for SampleDriver { > > + type IdInfo = Info; > > + const ID_TABLE: platform::IdTable<Self::IdInfo> = &OF_TABLE; > > Probably want to name this OF_ID_TABLE for when ACPI_ID_TABLE is added. Yes, makes sense. > > > + > > + fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>> { > > + dev_dbg!(pdev.as_ref(), "Probe Rust Platform driver sample.\n"); > > + > > + match (Self::of_match_device(pdev), info) { > > That answers my question on being exposed to drivers. This is a big no > for me. Agreed, we don't need it. Please also see my previous reply in the platform bus abstraction. > > > + (Some(id), Some(info)) => { > > + dev_info!( > > + pdev.as_ref(), > > + "Probed by OF compatible match: '{}' with info: '{}'.\n", > > + id.compatible(), > > As I mentioned, "real" drivers don't need the compatible string. Same here. > > > + info.0 > > + ); > > + } > > + _ => { > > + dev_info!(pdev.as_ref(), "Probed by name.\n"); > > + } > > + }; > > + > > + let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?; > > + > > + Ok(drvdata.into()) > > + } > > +} > > + > > +impl Drop for SampleDriver { > > + fn drop(&mut self) { > > + dev_dbg!(self.pdev.as_ref(), "Remove Rust Platform driver sample.\n"); > > + } > > +} > > + > > +kernel::module_platform_driver! { > > + type: SampleDriver, > > + name: "rust_driver_platform", > > + author: "Danilo Krummrich", > > + description: "Rust Platform driver", > > + license: "GPL v2", > > +} > > -- > > 2.46.2 > > >
On Wed, Oct 23, 2024 at 08:59:48AM +0200, Danilo Krummrich wrote: > On Tue, Oct 22, 2024 at 07:04:08PM -0500, Rob Herring wrote: > > On Tue, Oct 22, 2024 at 11:31:53PM +0200, Danilo Krummrich wrote: > > > Add a sample Rust platform driver illustrating the usage of the platform > > > bus abstractions. > > > > > > This driver probes through either a match of device / driver name or a > > > match within the OF ID table. > > > > I know if rust compiles it works, but how does one actually use/test > > this? (I know ways, but I might be in the minority. :) ) > > For testing a name match I just used platform_device_register_simple() in a > separate module. > > Probing through the OF table is indeed a bit more tricky. Since I was too lazy > to pull out a random ARM device of my cupboard I just used QEMU on x86 and did > what drivers/of/unittest.c does. If you're smart you can also just enable those > unit tests and change the compatible string to "unittest". :) > > > > > The DT unittests already define test platform devices. I'd be happy to > > add a device node there. Then you don't have to muck with the DT on some > > device and it even works on x86 or UML. > > Sounds good, I'll add one in there for this sample driver -- any preferences? I gave this a spin and added the patch below in. Feel free to squash it into this one. 8<---------------------------------------------------------------- From: "Rob Herring (Arm)" <robh@kernel.org> Date: Wed, 23 Oct 2024 10:29:47 -0500 Subject: [PATCH] of: unittest: Add a platform device node for rust platform driver sample Signed-off-by: Rob Herring (Arm) <robh@kernel.org> diff --git a/drivers/of/unittest-data/tests-platform.dtsi b/drivers/of/unittest-data/tests-platform.dtsi index fa39611071b3..575ea260a877 100644 --- a/drivers/of/unittest-data/tests-platform.dtsi +++ b/drivers/of/unittest-data/tests-platform.dtsi @@ -33,6 +33,11 @@ dev@100 { reg = <0x100>; }; }; + + test-device@2 { + compatible = "test,rust-device"; + reg = <0x2>; + }; }; }; }; diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs index 55caaaa4f216..5cf4a8f86c13 100644 --- a/samples/rust/rust_driver_platform.rs +++ b/samples/rust/rust_driver_platform.rs @@ -15,7 +15,7 @@ struct SampleDriver { MODULE_OF_TABLE, <SampleDriver as platform::Driver>::IdInfo, [( - of::DeviceId::new(c_str!("redhat,rust-sample-platform-driver")), + of::DeviceId::new(c_str!("test,rust-device")), Info(42) )] );
On 23.10.2024 02:04, Rob Herring wrote: > On Tue, Oct 22, 2024 at 11:31:53PM +0200, Danilo Krummrich wrote: >> Add a sample Rust platform driver illustrating the usage of the platform >> bus abstractions. >> >> This driver probes through either a match of device / driver name or a >> match within the OF ID table. > > I know if rust compiles it works, but how does one actually use/test > this? (I know ways, but I might be in the minority. :) ) > > The DT unittests already define test platform devices. I'd be happy to > add a device node there. Then you don't have to muck with the DT on some > device and it even works on x86 or UML. Assuming being on x86, having CONFIG_OF and CONFIG_OF_UNITTEST enabled, seeing the ### dt-test ### running nicely at kernel startup and seeing the compiled in test device tree under /proc/device-tree: Would using a compatible from the test device tree (e.g. "test-device") in the Rust Platform driver sample [1] let the probe() of that driver sample run? Or is this a wrong/not sufficient understanding? I tried that, without success ;) Best regards Dirk --- a/samples/rust/rust_driver_platform.rs +++ b/samples/rust/rust_driver_platform.rs @@ -15,7 +15,7 @@ struct SampleDriver { SAMPLE_MODULE_OF_TABLE, <SampleDriver as platform::Driver>::IdInfo, [( - of::DeviceId::new(c_str!("redhat,rust-sample-platform-driver")), + of::DeviceId::new(c_str!("test-device")), Info(42) )] );
On Fri, Oct 25, 2024 at 5:33 AM Dirk Behme <dirk.behme@de.bosch.com> wrote: > > On 23.10.2024 02:04, Rob Herring wrote: > > On Tue, Oct 22, 2024 at 11:31:53PM +0200, Danilo Krummrich wrote: > >> Add a sample Rust platform driver illustrating the usage of the platform > >> bus abstractions. > >> > >> This driver probes through either a match of device / driver name or a > >> match within the OF ID table. > > > > I know if rust compiles it works, but how does one actually use/test > > this? (I know ways, but I might be in the minority. :) ) > > > > The DT unittests already define test platform devices. I'd be happy to > > add a device node there. Then you don't have to muck with the DT on some > > device and it even works on x86 or UML. > > Assuming being on x86, having CONFIG_OF and CONFIG_OF_UNITTEST enabled, > seeing the ### dt-test ### running nicely at kernel startup and seeing > the compiled in test device tree under /proc/device-tree: > > Would using a compatible from the test device tree (e.g. "test-device") > in the Rust Platform driver sample [1] let the probe() of that driver > sample run? No, because that binds with the platform driver within the unittest. Maybe it would work if you manually unbind the unittest driver and bind the rust sample. > Or is this a wrong/not sufficient understanding? > > I tried that, without success ;) Did you try the patch I sent in this thread? That works and only depends on kconfig options to work. Rob
diff --git a/MAINTAINERS b/MAINTAINERS index 173540375863..583b6588fd1e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6986,6 +6986,7 @@ F: rust/kernel/device_id.rs F: rust/kernel/devres.rs F: rust/kernel/driver.rs F: rust/kernel/platform.rs +F: samples/rust/rust_driver_platform.rs DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS) M: Nishanth Menon <nm@ti.com> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig index 6d468193cdd8..70126b750426 100644 --- a/samples/rust/Kconfig +++ b/samples/rust/Kconfig @@ -41,6 +41,16 @@ config SAMPLE_RUST_DRIVER_PCI If unsure, say N. +config SAMPLE_RUST_DRIVER_PLATFORM + tristate "Platform Driver" + help + This option builds the Rust Platform driver sample. + + To compile this as a module, choose M here: + the module will be called rust_driver_platform. + + If unsure, say N. + config SAMPLE_RUST_HOSTPROGS bool "Host programs" help diff --git a/samples/rust/Makefile b/samples/rust/Makefile index b66767f4a62a..11fcb312ed36 100644 --- a/samples/rust/Makefile +++ b/samples/rust/Makefile @@ -3,5 +3,6 @@ obj-$(CONFIG_SAMPLE_RUST_MINIMAL) += rust_minimal.o obj-$(CONFIG_SAMPLE_RUST_PRINT) += rust_print.o obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI) += rust_driver_pci.o +obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM) += rust_driver_platform.o subdir-$(CONFIG_SAMPLE_RUST_HOSTPROGS) += hostprogs diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs new file mode 100644 index 000000000000..55caaaa4f216 --- /dev/null +++ b/samples/rust/rust_driver_platform.rs @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Rust Platform driver sample. + +use kernel::{c_str, of, platform, prelude::*}; + +struct SampleDriver { + pdev: platform::Device, +} + +struct Info(u32); + +kernel::of_device_table!( + OF_TABLE, + MODULE_OF_TABLE, + <SampleDriver as platform::Driver>::IdInfo, + [( + of::DeviceId::new(c_str!("redhat,rust-sample-platform-driver")), + Info(42) + )] +); + +impl platform::Driver for SampleDriver { + type IdInfo = Info; + const ID_TABLE: platform::IdTable<Self::IdInfo> = &OF_TABLE; + + fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>> { + dev_dbg!(pdev.as_ref(), "Probe Rust Platform driver sample.\n"); + + match (Self::of_match_device(pdev), info) { + (Some(id), Some(info)) => { + dev_info!( + pdev.as_ref(), + "Probed by OF compatible match: '{}' with info: '{}'.\n", + id.compatible(), + info.0 + ); + } + _ => { + dev_info!(pdev.as_ref(), "Probed by name.\n"); + } + }; + + let drvdata = KBox::new(Self { pdev: pdev.clone() }, GFP_KERNEL)?; + + Ok(drvdata.into()) + } +} + +impl Drop for SampleDriver { + fn drop(&mut self) { + dev_dbg!(self.pdev.as_ref(), "Remove Rust Platform driver sample.\n"); + } +} + +kernel::module_platform_driver! { + type: SampleDriver, + name: "rust_driver_platform", + author: "Danilo Krummrich", + description: "Rust Platform driver", + license: "GPL v2", +}
Add a sample Rust platform driver illustrating the usage of the platform bus abstractions. This driver probes through either a match of device / driver name or a match within the OF ID table. Signed-off-by: Danilo Krummrich <dakr@kernel.org> --- MAINTAINERS | 1 + samples/rust/Kconfig | 10 +++++ samples/rust/Makefile | 1 + samples/rust/rust_driver_platform.rs | 62 ++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+) create mode 100644 samples/rust/rust_driver_platform.rs