Re: [PATCH 2/3] rust: dma: convert the read/write macros to return Result

[Benno Lossin]

On Wed Mar 26, 2025 at 9:11 PM CET, Abdiel Janulgue wrote:
> As suggested by Andreas Hindborg, we could do better here by
> having the macros return `Result`, so that we don't have to wrap
> these calls in a closure for validation which is confusing.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@xxxxxxxxx>
> ---
>  rust/kernel/dma.rs       | 54 +++++++++++++++++++++++-----------------
>  samples/rust/rust_dma.rs | 21 ++++++----------
>  2 files changed, 38 insertions(+), 37 deletions(-)
>
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index d3f448868457..24a6f10370c4 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
> @@ -328,20 +328,22 @@ unsafe impl<T: AsBytes + FromBytes + Send> Send for CoherentAllocation<T> {}
>  #[macro_export]
>  macro_rules! dma_read {
>      ($dma:expr, $idx: expr, $($field:tt)*) => {{
> -        let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
> -        // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
> -        // dereferenced. The compiler also further validates the expression on whether `field`
> -        // is a member of `item` when expanded by the macro.
> -        unsafe {
> -            let ptr_field = ::core::ptr::addr_of!((*item) $($field)*);
> -            $crate::dma::CoherentAllocation::field_read(&$dma, ptr_field)
> -        }
> +        (|| -> Result<_> {

Please use `::core::result::Result<_, _>` instead. If someone uses this
macro in a place with a different `Result` than the one from the kernel
crate, then this will result in a compile error. (also in the instances
below)

You might want to use `::core::result::Result<_, $crate::error::Error>`
instead though if the type inference can't figure out the error type.

> +            let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
> +            // SAFETY: `item_from_index` ensures that `item` is always a valid pointer and can be
> +            // dereferenced. The compiler also further validates the expression on whether `field`
> +            // is a member of `item` when expanded by the macro.
> +            unsafe {
> +                let ptr_field = ::core::ptr::addr_of!((*item) $($field)*);
> +                ::core::result::Result::Ok($crate::dma::CoherentAllocation::field_read(&$dma, ptr_field))
> +            }
> +        })()
>      }};
>      ($dma:ident [ $idx:expr ] $($field:tt)* ) => {
> -        $crate::dma_read!($dma, $idx, $($field)*);
> +        $crate::dma_read!($dma, $idx, $($field)*)
>      };
>      ($($dma:ident).* [ $idx:expr ] $($field:tt)* ) => {
> -        $crate::dma_read!($($dma).*, $idx, $($field)*);
> +        $crate::dma_read!($($dma).*, $idx, $($field)*)
>      };
>  }
>  

> diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
> index 908acd34b8db..cc09d49f2056 100644
> --- a/samples/rust/rust_dma.rs
> +++ b/samples/rust/rust_dma.rs
> @@ -54,13 +54,9 @@ fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>
>          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(())
> -        }()?;
> +        for (i, value) in TEST_VALUES.into_iter().enumerate() {
> +            kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1))?;
> +        }
>  
>          let drvdata = KBox::new(
>              Self {
> @@ -78,13 +74,10 @@ 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);
> -            }
> -            Ok(())
> -        }();
> +        for (i, value) in TEST_VALUES.into_iter().enumerate() {
> +            assert_eq!(kernel::dma_read!(self.ca[i].h).unwrap(), value.0);
> +            assert_eq!(kernel::dma_read!(self.ca[i].b).unwrap(), value.1);
> +        }

This changes the behavior from before: now an error will result in a
panic where before it was just ignored. Not sure what to do here since
it's a sample, but if you intend the functional change, I would mention
it in the commit message.

---
Cheers,
Benno

>      }
>  }
>