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

[Abdiel Janulgue <abdiel.janulgue@gmail.com>]

Hi,

On 18/03/2025 15:26, Andreas Hindborg wrote:
> 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?
> 

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