Re: [PATCH v14 03/11] samples: rust: add Rust dma test sample driver

[Andreas Hindborg <a.hindborg@kernel.org>]

Abdiel Janulgue <abdiel.janulgue@gmail.com> writes:

> Add a simple driver to excercise the basics of the Rust DMA
> coherent allocator bindings.
>
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.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?

> +
> +        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.


Best regards,
Andreas Hindborg