[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <03902487-5cd3-45ed-b3cb-cabfd0ac5cb7@gmail.com>
Date: Tue, 18 Mar 2025 20:42:39 +0200
From: Abdiel Janulgue <abdiel.janulgue@...il.com>
To: Andreas Hindborg <a.hindborg@...nel.org>
Cc: rust-for-linux@...r.kernel.org, daniel.almeida@...labora.com,
dakr@...nel.org, robin.murphy@....com, aliceryhl@...gle.com,
Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <benno.lossin@...ton.me>, Trevor Gross <tmgross@...ch.edu>,
Valentin Obst <kernel@...entinobst.de>,
open list <linux-kernel@...r.kernel.org>, Christoph Hellwig <hch@....de>,
Marek Szyprowski <m.szyprowski@...sung.com>, airlied@...hat.com,
"open list:DMA MAPPING HELPERS" <iommu@...ts.linux.dev>
Subject: Re: [PATCH v14 03/11] samples: rust: add Rust dma test sample driver
Hi,
On 18/03/2025 15:26, Andreas Hindborg wrote:
> Abdiel Janulgue <abdiel.janulgue@...il.com> writes:
>
>> Add a simple driver to excercise the basics of the Rust DMA
>> coherent allocator bindings.
>>
>> Suggested-by: Danilo Krummrich <dakr@...nel.org>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@...il.com>
>> ---
>> samples/rust/Kconfig | 11 +++++
>> samples/rust/Makefile | 1 +
>> samples/rust/rust_dma.rs | 97 ++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 109 insertions(+)
>> create mode 100644 samples/rust/rust_dma.rs
>>
>> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
>> index 3b6eae84b297..e2d14aa6beec 100644
>> --- a/samples/rust/Kconfig
>> +++ b/samples/rust/Kconfig
>> @@ -78,4 +78,15 @@ config SAMPLE_RUST_HOSTPROGS
>>
>> If unsure, say N.
>>
>> +config SAMPLE_RUST_DRIVER_DMA
>> + tristate "DMA Test Driver"
>> + depends on PCI
>> + help
>> + This option builds the Rust dma test driver sample.
>> +
>> + To compile this as a module, choose M here:
>> + the module will be called dma.
>> +
>> + If unsure, say N.
>> +
>> endif # SAMPLES_RUST
>> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
>> index 0dbc6d90f1ef..1a9aff6e8d6a 100644
>> --- a/samples/rust/Makefile
>> +++ b/samples/rust/Makefile
>> @@ -7,6 +7,7 @@ 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
>> obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX) += rust_driver_faux.o
>> +obj-$(CONFIG_SAMPLE_RUST_DRIVER_DMA) += rust_dma.o
>>
>> rust_print-y := rust_print_main.o rust_print_events.o
>>
>> diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
>> new file mode 100644
>> index 000000000000..1740140faba6
>> --- /dev/null
>> +++ b/samples/rust/rust_dma.rs
>> @@ -0,0 +1,97 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Rust DMA api test (based on QEMU's `pci-testdev`).
>> +//!
>> +//! To make this driver probe, QEMU must be run with `-device pci-testdev`.
>> +
>> +use kernel::{bindings, dma::CoherentAllocation, pci, prelude::*};
>> +
>> +struct DmaSampleDriver {
>> + pdev: pci::Device,
>> + ca: CoherentAllocation<MyStruct>,
>> +}
>> +
>> +const TEST_VALUES: [(u32, u32); 5] = [
>> + (0xa, 0xb),
>> + (0xc, 0xd),
>> + (0xe, 0xf),
>> + (0xab, 0xba),
>> + (0xcd, 0xef),
>> +];
>> +
>> +struct MyStruct {
>> + h: u32,
>> + b: u32,
>> +}
>> +
>> +impl MyStruct {
>> + fn new(h: u32, b: u32) -> Self {
>> + Self { h, b }
>> + }
>> +}
>> +// SAFETY: All bit patterns are acceptable values for `MyStruct`.
>> +unsafe impl kernel::transmute::AsBytes for MyStruct {}
>> +// SAFETY: Instances of `MyStruct` have no uninitialized portions.
>> +unsafe impl kernel::transmute::FromBytes for MyStruct {}
>> +
>> +kernel::pci_device_table!(
>> + PCI_TABLE,
>> + MODULE_PCI_TABLE,
>> + <DmaSampleDriver as pci::Driver>::IdInfo,
>> + [(
>> + pci::DeviceId::from_id(bindings::PCI_VENDOR_ID_REDHAT, 0x5),
>> + ()
>> + )]
>> +);
>> +
>> +impl pci::Driver for DmaSampleDriver {
>> + type IdInfo = ();
>> + const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE;
>> +
>> + fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
>> + dev_info!(pdev.as_ref(), "Probe DMA test driver.\n");
>> +
>> + let ca: CoherentAllocation<MyStruct> =
>> + CoherentAllocation::alloc_coherent(pdev.as_ref(), TEST_VALUES.len(), GFP_KERNEL)?;
>> +
>> + || -> Result {
>> + for (i, value) in TEST_VALUES.into_iter().enumerate() {
>> + kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1));
>> + }
>> +
>> + Ok(())
>> + }()?;
>
> Why is this placed in a closure? Left over from deferred error for pin-init?
>
The macro expands into a block which needs to validate the function
item_from_index()?
>> +
>> + let drvdata = KBox::new(
>> + Self {
>> + pdev: pdev.clone(),
>> + ca,
>> + },
>> + GFP_KERNEL,
>> + )?;
>> +
>> + Ok(drvdata.into())
>> + }
>> +}
>> +
>> +impl Drop for DmaSampleDriver {
>> + fn drop(&mut self) {
>> + dev_info!(self.pdev.as_ref(), "Unload DMA test driver.\n");
>> +
>> + let _ = || -> Result {
>> + for (i, value) in TEST_VALUES.into_iter().enumerate() {
>> + assert_eq!(kernel::dma_read!(self.ca[i].h), value.0);
>> + assert_eq!(kernel::dma_read!(self.ca[i].b), value.1);
>
> We should probably change `dma_read!`/`dma_write!` to return `Result`,
> so that we don't have to wrap these calls in a closure for obscure reasons.
>
Changing `dma_read!`/`dma_write!` to return `Result` is probably not a
trivial change, would it be okay to have this fixed in a subsequent patch?
Thanks!
/Abdiel
Powered by blists - more mailing lists