Message-ID: <87ldsahlxr.fsf@kernel.org>
Date: Tue, 08 Apr 2025 14:33:52 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Benno Lossin" <benno.lossin@...ton.me>
Cc: "Abdiel Janulgue" <abdiel.janulgue@...il.com>,  <ojeda@...nel.org>,
  "Danilo Krummrich" <dakr@...nel.org>,  "Daniel Almeida"
 <daniel.almeida@...labora.com>,  "Robin Murphy" <robin.murphy@....com>,
  "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>,  "Alice Ryhl" <aliceryhl@...gle.com>,  "Trevor
 Gross" <tmgross@...ch.edu>,  "open list:DMA MAPPING HELPERS DEVICE DRIVER
 API [RUST]" <rust-for-linux@...r.kernel.org>,  "Marek Szyprowski"
 <m.szyprowski@...sung.com>,  "open list:DMA MAPPING HELPERS"
 <iommu@...ts.linux.dev>,  "open list" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] rust: dma: convert the read/write macros to return
 Result

"Benno Lossin" <benno.lossin@...ton.me> writes:

> 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@...il.com>
>> ---
>>  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.

There is two sides to this. If we want this as a nice example that
people should copy in their drivers, using unwrap is bad. But for
testing and demonstration purposes, I think the unwrap is mandated.

But the `assert_eq!` would panic anyway if comparison fails, right?


Best regards,
Andreas Hindborg

Powered by blists - more mailing lists