[PATCH v2 0/3] Additional improvements for dma coherent allocator

[PATCH v2 0/3] Additional improvements for dma coherent allocator

[Abdiel Janulgue]

Changes since v1:
- Pull in reviewed-by tags and include links.
- Improve error handling in rust dma sample driver.
- Clarifications in documentation.
Link to v1: https://lore.kernel.org/all/20250326201230.3193329-1-abdiel.janulgue@gmail.com/

Abdiel Janulgue (3):
  rust: dma: clarify wording and consistency in `coherent` nomenclature
  rust: dma: convert the read/write macros to return Result
  rust: dma: add as_slice/write functions for CoherentAllocation

 rust/kernel/dma.rs       | 151 +++++++++++++++++++++++++++++++--------
 samples/rust/rust_dma.rs |  25 +++----
 2 files changed, 133 insertions(+), 43 deletions(-)


base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
-- 
2.43.0

[PATCH v2 1/3] rust: dma: clarify wording and be consistent in `coherent` nomenclature

[Abdiel Janulgue]

In the kernel, `consistent` and `coherent` are used interchangeably for the
region described in this api. Stick with `coherent` nomenclature
to show that dma_alloc_coherent() is being used, in addition to improving
the clarity in the DMA mapping attributes documentation.

Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/dma.rs | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 8cdc76043ee7..d3f448868457 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -89,15 +89,15 @@ pub mod attrs {
     /// Forces contiguous allocation of the buffer in physical memory.
     pub const DMA_ATTR_FORCE_CONTIGUOUS: Attrs = Attrs(bindings::DMA_ATTR_FORCE_CONTIGUOUS);
 
-    /// This is a hint to the DMA-mapping subsystem that it's probably not worth the time to try
+    /// Hints DMA-mapping subsystem that it's probably not worth the time to try
     /// to allocate memory to in a way that gives better TLB efficiency.
     pub const DMA_ATTR_ALLOC_SINGLE_PAGES: Attrs = Attrs(bindings::DMA_ATTR_ALLOC_SINGLE_PAGES);
 
-    /// This tells the DMA-mapping subsystem to suppress allocation failure reports (similarly to
+    /// Tells the DMA-mapping subsystem to suppress allocation failure reports (similarly to
     /// __GFP_NOWARN).
     pub const DMA_ATTR_NO_WARN: Attrs = Attrs(bindings::DMA_ATTR_NO_WARN);
 
-    /// Used to indicate that the buffer is fully accessible at an elevated privilege level (and
+    /// Indicates that the buffer is fully accessible at an elevated privilege level (and
     /// ideally inaccessible or at least read-only at lesser-privileged levels).
     pub const DMA_ATTR_PRIVILEGED: Attrs = Attrs(bindings::DMA_ATTR_PRIVILEGED);
 }
@@ -105,7 +105,7 @@ pub mod attrs {
 /// An abstraction of the `dma_alloc_coherent` API.
 ///
 /// This is an abstraction around the `dma_alloc_coherent` API which is used to allocate and map
-/// large consistent DMA regions.
+/// large coherent DMA regions.
 ///
 /// A [`CoherentAllocation`] instance contains a pointer to the allocated region (in the
 /// processor's virtual address space) and the device address which can be given to the device
@@ -115,7 +115,7 @@ pub mod attrs {
 /// # Invariants
 ///
 /// For the lifetime of an instance of [`CoherentAllocation`], the `cpu_addr` is a valid pointer
-/// to an allocated region of consistent memory and `dma_handle` is the DMA address base of
+/// to an allocated region of coherent memory and `dma_handle` is the DMA address base of
 /// the region.
 // TODO
 //
@@ -138,7 +138,7 @@ pub struct CoherentAllocation<T: AsBytes + FromBytes> {
 }
 
 impl<T: AsBytes + FromBytes> CoherentAllocation<T> {
-    /// Allocates a region of `size_of::<T> * count` of consistent memory.
+    /// Allocates a region of `size_of::<T> * count` of coherent memory.
     ///
     /// # Examples
     ///
-- 
2.43.0

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

[Abdiel Janulgue]

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.

Co-developed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
Suggested-by: Andreas Hindborg <a.hindborg@kernel.org>
Link: https://lore.kernel.org/rust-for-linux/87h63qhz4q.fsf@kernel.org/
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/dma.rs       | 54 +++++++++++++++++++++++-----------------
 samples/rust/rust_dma.rs | 25 ++++++++-----------
 2 files changed, 42 insertions(+), 37 deletions(-)

diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index d3f448868457..a61da5eeb017 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)
-        }
+        (|| -> ::core::result::Result<_, $crate::error::Error> {
+            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)*)
     };
 }
 
@@ -368,24 +370,30 @@ macro_rules! dma_read {
 #[macro_export]
 macro_rules! dma_write {
     ($dma:ident [ $idx:expr ] $($field:tt)*) => {{
-        $crate::dma_write!($dma, $idx, $($field)*);
+        $crate::dma_write!($dma, $idx, $($field)*)
     }};
     ($($dma:ident).* [ $idx:expr ] $($field:tt)* ) => {{
-        $crate::dma_write!($($dma).*, $idx, $($field)*);
+        $crate::dma_write!($($dma).*, $idx, $($field)*)
     }};
     ($dma:expr, $idx: expr, = $val:expr) => {
-        let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
-        // SAFETY: `item_from_index` ensures that `item` is always a valid item.
-        unsafe { $crate::dma::CoherentAllocation::field_write(&$dma, item, $val) }
+        (|| -> ::core::result::Result<_, $crate::error::Error> {
+            let item = $crate::dma::CoherentAllocation::item_from_index(&$dma, $idx)?;
+            // SAFETY: `item_from_index` ensures that `item` is always a valid item.
+            unsafe { $crate::dma::CoherentAllocation::field_write(&$dma, item, $val) }
+            ::core::result::Result::Ok(())
+        })()
     };
     ($dma:expr, $idx: expr, $(.$field:ident)* = $val:expr) => {
-        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_mut!((*item) $(.$field)*);
-            $crate::dma::CoherentAllocation::field_write(&$dma, ptr_field, $val)
-        }
+        (|| -> ::core::result::Result<_, $crate::error::Error> {
+            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_mut!((*item) $(.$field)*);
+                $crate::dma::CoherentAllocation::field_write(&$dma, ptr_field, $val)
+            }
+            ::core::result::Result::Ok(())
+        })()
     };
 }
diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
index 874c2c964afa..1e610545e100 100644
--- a/samples/rust/rust_dma.rs
+++ b/samples/rust/rust_dma.rs
@@ -54,13 +54,9 @@ fn probe(pdev: &pci::Device<Core>, _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,14 @@ 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() {
+            let val0 = kernel::dma_read!(self.ca[i].h);
+            let val1 = kernel::dma_read!(self.ca[i].b);
+            assert!(val0.is_ok());
+            assert!(val1.is_ok());
+            assert_eq!(val0.unwrap(), value.0);
+            assert_eq!(val1.unwrap(), value.1);
+        }
     }
 }
 
-- 
2.43.0

[PATCH v2 3/3] rust: dma: add as_slice/write functions for CoherentAllocation

[Abdiel Janulgue]

Add unsafe accessors for the region for reading or writing large
blocks of data.

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
---
 rust/kernel/dma.rs | 85 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index a61da5eeb017..880f6f04ba86 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -218,6 +218,91 @@ pub fn dma_handle(&self) -> bindings::dma_addr_t {
         self.dma_handle
     }
 
+    /// Returns the data from the region starting from `offset` as a slice.
+    /// `offset` and `count` are in units of `T`, not the number of bytes.
+    ///
+    /// For ringbuffer type of r/w access or use-cases where the pointer to the live data is needed,
+    /// [`CoherentAllocation::start_ptr`] or [`CoherentAllocation::start_ptr_mut`] could be used instead.
+    ///
+    /// # Safety
+    ///
+    /// * Callers must ensure that the device does not read/write to/from memory while the returned
+    ///   slice is live.
+    /// * Callers must ensure that this call does not race with a write to the same region while
+    ///   while the returned slice is live.
+    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
+        let end = offset.checked_add(count).ok_or(EOVERFLOW)?;
+        if end >= self.count {
+            return Err(EINVAL);
+        }
+        // SAFETY:
+        // - The pointer is valid due to type invariant on `CoherentAllocation`,
+        //   we've just checked that the range and index is within bounds. The immutability of the
+        //   of data is also guaranteed by the safety requirements of the function.
+        // - `offset` can't overflow since it is smaller than `self.count` and we've checked
+        //   that `self.count` won't overflow early in the constructor.
+        Ok(unsafe { core::slice::from_raw_parts(self.cpu_addr.add(offset), count) })
+    }
+
+    /// Performs the same functionality as [`CoherentAllocation::as_slice`], except that a mutable
+    /// slice is returned.
+    ///
+    /// # Safety
+    ///
+    /// * Callers must ensure that the device does not read/write to/from memory while the returned
+    ///   slice is live.
+    /// * Callers must ensure that this call does not race with a read or write to the same region
+    ///   while the returned slice is live.
+    pub unsafe fn as_slice_mut(&self, offset: usize, count: usize) -> Result<&mut [T]> {
+        let end = offset.checked_add(count).ok_or(EOVERFLOW)?;
+        if end >= self.count {
+            return Err(EINVAL);
+        }
+        // SAFETY:
+        // - The pointer is valid due to type invariant on `CoherentAllocation`,
+        //   we've just checked that the range and index is within bounds. The immutability of the
+        //   of data is also guaranteed by the safety requirements of the function.
+        // - `offset` can't overflow since it is smaller than `self.count` and we've checked
+        //   that `self.count` won't overflow early in the constructor.
+        Ok(unsafe { core::slice::from_raw_parts_mut(self.cpu_addr.add(offset), count) })
+    }
+
+    /// Writes data to the region starting from `offset`. `offset` is in units of `T`, not the
+    /// number of bytes.
+    ///
+    /// # Safety
+    ///
+    /// * Callers must ensure that the device does not read/write to/from memory while the returned
+    ///   slice is live.
+    /// * Callers must ensure that this call does not race with a read or write to the same region
+    ///   that overlaps with this write.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// # fn test(alloc: &mut kernel::dma::CoherentAllocation<u8>) -> Result {
+    /// let somedata: [u8; 4] = [0xf; 4];
+    /// let buf: &[u8] = &somedata;
+    /// // SAFETY: No hw operation on the device and no other r/w access to the region at this point.
+    /// unsafe { alloc.write(buf, 0)?; }
+    /// # Ok::<(), Error>(()) }
+    /// ```
+    pub unsafe fn write(&self, src: &[T], offset: usize) -> Result {
+        let end = offset.checked_add(src.len()).ok_or(EOVERFLOW)?;
+        if end >= self.count {
+            return Err(EINVAL);
+        }
+        // SAFETY:
+        // - The pointer is valid due to type invariant on `CoherentAllocation`
+        //   and we've just checked that the range and index is within bounds.
+        // - `offset` can't overflow since it is smaller than `self.count` and we've checked
+        //   that `self.count` won't overflow early in the constructor.
+        unsafe {
+            core::ptr::copy_nonoverlapping(src.as_ptr(), self.cpu_addr.add(offset), src.len())
+        };
+        Ok(())
+    }
+
     /// Returns a pointer to an element from the region with bounds checking. `offset` is in
     /// units of `T`, not the number of bytes.
     ///
-- 
2.43.0

Re: [PATCH v2 3/3] rust: dma: add as_slice/write functions for CoherentAllocation

[Alexandre Courbot]

Hi Abdiel,

On Thu Apr 10, 2025 at 5:58 PM JST, Abdiel Janulgue wrote:
> Add unsafe accessors for the region for reading or writing large
> blocks of data.
>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
>  rust/kernel/dma.rs | 85 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
>
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index a61da5eeb017..880f6f04ba86 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
> @@ -218,6 +218,91 @@ pub fn dma_handle(&self) -> bindings::dma_addr_t {
>          self.dma_handle
>      }
>  
> +    /// Returns the data from the region starting from `offset` as a slice.
> +    /// `offset` and `count` are in units of `T`, not the number of bytes.
> +    ///
> +    /// For ringbuffer type of r/w access or use-cases where the pointer to the live data is needed,
> +    /// [`CoherentAllocation::start_ptr`] or [`CoherentAllocation::start_ptr_mut`] could be used instead.
> +    ///
> +    /// # Safety
> +    ///
> +    /// * Callers must ensure that the device does not read/write to/from memory while the returned
> +    ///   slice is live.
> +    /// * Callers must ensure that this call does not race with a write to the same region while
> +    ///   while the returned slice is live.
> +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> +        let end = offset.checked_add(count).ok_or(EOVERFLOW)?;
> +        if end >= self.count {
> +            return Err(EINVAL);
> +        }

Not sure if you have overlooked my comment on the previous iteration or
if I completely missed the mark, but my understanding if that the bound
check should be `if end > self.count`. Also applies to the other methods
of this patch.

Re: [PATCH v2 3/3] rust: dma: add as_slice/write functions for CoherentAllocation

[Abdiel Janulgue]

On 10/04/2025 12:57, Alexandre Courbot wrote:
> Hi Abdiel,
> 
> On Thu Apr 10, 2025 at 5:58 PM JST, Abdiel Janulgue wrote:
>> Add unsafe accessors for the region for reading or writing large
>> blocks of data.
>>
>> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
>> ---
>>   rust/kernel/dma.rs | 85 ++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 85 insertions(+)
>>
>> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
>> index a61da5eeb017..880f6f04ba86 100644
>> --- a/rust/kernel/dma.rs
>> +++ b/rust/kernel/dma.rs
>> @@ -218,6 +218,91 @@ pub fn dma_handle(&self) -> bindings::dma_addr_t {
>>           self.dma_handle
>>       }
>>   
>> +    /// Returns the data from the region starting from `offset` as a slice.
>> +    /// `offset` and `count` are in units of `T`, not the number of bytes.
>> +    ///
>> +    /// For ringbuffer type of r/w access or use-cases where the pointer to the live data is needed,
>> +    /// [`CoherentAllocation::start_ptr`] or [`CoherentAllocation::start_ptr_mut`] could be used instead.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// * Callers must ensure that the device does not read/write to/from memory while the returned
>> +    ///   slice is live.
>> +    /// * Callers must ensure that this call does not race with a write to the same region while
>> +    ///   while the returned slice is live.
>> +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
>> +        let end = offset.checked_add(count).ok_or(EOVERFLOW)?;
>> +        if end >= self.count {
>> +            return Err(EINVAL);
>> +        }
> 
> Not sure if you have overlooked my comment on the previous iteration or
> if I completely missed the mark, but my understanding if that the bound
> check should be `if end > self.count`. Also applies to the other methods
> of this patch.
> 
Ah sorry about that, just missed that part! But will definitely do a 
follow-up that includes this (with other changes if needed).

/Abdiel

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

[Danilo Krummrich]

On Thu, Apr 10, 2025 at 11:58:17AM +0300, Abdiel Janulgue wrote:
> 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.
> 
> Co-developed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> Suggested-by: Andreas Hindborg <a.hindborg@kernel.org>
> Link: https://lore.kernel.org/rust-for-linux/87h63qhz4q.fsf@kernel.org/
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>

I think you can drop this and the Suggested-by tag, since Andreas is also a
co-author.

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

[Danilo Krummrich]

On Thu, Apr 10, 2025 at 11:58:17AM +0300, Abdiel Janulgue wrote:
> diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
> index 874c2c964afa..1e610545e100 100644
> --- a/samples/rust/rust_dma.rs
> +++ b/samples/rust/rust_dma.rs
> @@ -54,13 +54,9 @@ fn probe(pdev: &pci::Device<Core>, _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,14 @@ 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() {
> +            let val0 = kernel::dma_read!(self.ca[i].h);
> +            let val1 = kernel::dma_read!(self.ca[i].b);
> +            assert!(val0.is_ok());
> +            assert!(val1.is_ok());
> +            assert_eq!(val0.unwrap(), value.0);
> +            assert_eq!(val1.unwrap(), value.1);

Maybe use if-let to avoid the unwrap?

	if let Ok(val0) = val0 {
	   assert_eq!(val0, value.0);
	}

I know it's a bit pointless, since we know it must be ok, but the educational
message of the example should be to check and not to unwrap, so maybe that's
better.

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

[Benno Lossin]

On Thu Apr 10, 2025 at 1:54 PM CEST, Danilo Krummrich wrote:
> On Thu, Apr 10, 2025 at 11:58:17AM +0300, Abdiel Janulgue wrote:
>> @@ -78,13 +74,14 @@ 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() {
>> +            let val0 = kernel::dma_read!(self.ca[i].h);
>> +            let val1 = kernel::dma_read!(self.ca[i].b);
>> +            assert!(val0.is_ok());
>> +            assert!(val1.is_ok());
>> +            assert_eq!(val0.unwrap(), value.0);
>> +            assert_eq!(val1.unwrap(), value.1);
>
> Maybe use if-let to avoid the unwrap?
>
> 	if let Ok(val0) = val0 {
> 	   assert_eq!(val0, value.0);
> 	}
>
> I know it's a bit pointless, since we know it must be ok, but the educational
> message of the example should be to check and not to unwrap, so maybe that's
> better.

The if-let will silently ignore any errors, so I don't think that it's
fit for example code either.

---
Cheers,
Benno

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

[Danilo Krummrich]

On Thu, Apr 10, 2025 at 03:11:01PM +0000, Benno Lossin wrote:
> On Thu Apr 10, 2025 at 1:54 PM CEST, Danilo Krummrich wrote:
> > On Thu, Apr 10, 2025 at 11:58:17AM +0300, Abdiel Janulgue wrote:
> >> @@ -78,13 +74,14 @@ 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() {
> >> +            let val0 = kernel::dma_read!(self.ca[i].h);
> >> +            let val1 = kernel::dma_read!(self.ca[i].b);
> >> +            assert!(val0.is_ok());
> >> +            assert!(val1.is_ok());
> >> +            assert_eq!(val0.unwrap(), value.0);
> >> +            assert_eq!(val1.unwrap(), value.1);
> >
> > Maybe use if-let to avoid the unwrap?
> >
> > 	if let Ok(val0) = val0 {
> > 	   assert_eq!(val0, value.0);
> > 	}
> >
> > I know it's a bit pointless, since we know it must be ok, but the educational
> > message of the example should be to check and not to unwrap, so maybe that's
> > better.
> 
> The if-let will silently ignore any errors, so I don't think that it's
> fit for example code either.

Yes, but we still have the assert!() before, so the full sequence would be:

	assert!(val0.is_ok());

	if let Ok(val0) = val0 {
	   assert_eq!(val0, value.0);
	}

The intention would be to avoid patterns that shouldn't be used in "real" code;
assert!() should be obvious not to use for real code.

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

[Benno Lossin]

On Thu Apr 10, 2025 at 5:34 PM CEST, Danilo Krummrich wrote:
> On Thu, Apr 10, 2025 at 03:11:01PM +0000, Benno Lossin wrote:
>> On Thu Apr 10, 2025 at 1:54 PM CEST, Danilo Krummrich wrote:
>> > On Thu, Apr 10, 2025 at 11:58:17AM +0300, Abdiel Janulgue wrote:
>> >> @@ -78,13 +74,14 @@ 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() {
>> >> +            let val0 = kernel::dma_read!(self.ca[i].h);
>> >> +            let val1 = kernel::dma_read!(self.ca[i].b);
>> >> +            assert!(val0.is_ok());
>> >> +            assert!(val1.is_ok());
>> >> +            assert_eq!(val0.unwrap(), value.0);
>> >> +            assert_eq!(val1.unwrap(), value.1);
>> >
>> > Maybe use if-let to avoid the unwrap?
>> >
>> > 	if let Ok(val0) = val0 {
>> > 	   assert_eq!(val0, value.0);
>> > 	}
>> >
>> > I know it's a bit pointless, since we know it must be ok, but the educational
>> > message of the example should be to check and not to unwrap, so maybe that's
>> > better.
>> 
>> The if-let will silently ignore any errors, so I don't think that it's
>> fit for example code either.
>
> Yes, but we still have the assert!() before, so the full sequence would be:
>
> 	assert!(val0.is_ok());
>
> 	if let Ok(val0) = val0 {
> 	   assert_eq!(val0, value.0);
> 	}

Ah right, missed that.

> The intention would be to avoid patterns that shouldn't be used in "real" code;
> assert!() should be obvious not to use for real code.

Yeah, I'm not sure if this is that valuable. I think having "real code"
is better, but I don't have any idea what to do in this case.

Why does this sample do the validation in the `drop` method in the first
place? I guess the same code on the C side would do this in `remove` or
whatever the equivalent thing is there, but would there be the option to
report an error? Or is `remove` an infallible operation? In that case
`assert!` probably is still the best option.

---
Cheers,
Benno

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

[Danilo Krummrich]

On Thu, Apr 10, 2025 at 10:58:10PM +0000, Benno Lossin wrote:
> On Thu Apr 10, 2025 at 5:34 PM CEST, Danilo Krummrich wrote:
> > On Thu, Apr 10, 2025 at 03:11:01PM +0000, Benno Lossin wrote:
> >> On Thu Apr 10, 2025 at 1:54 PM CEST, Danilo Krummrich wrote:
> >> > On Thu, Apr 10, 2025 at 11:58:17AM +0300, Abdiel Janulgue wrote:
> >> >> @@ -78,13 +74,14 @@ 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() {
> >> >> +            let val0 = kernel::dma_read!(self.ca[i].h);
> >> >> +            let val1 = kernel::dma_read!(self.ca[i].b);
> >> >> +            assert!(val0.is_ok());
> >> >> +            assert!(val1.is_ok());
> >> >> +            assert_eq!(val0.unwrap(), value.0);
> >> >> +            assert_eq!(val1.unwrap(), value.1);
> >> >
> >> > Maybe use if-let to avoid the unwrap?
> >> >
> >> > 	if let Ok(val0) = val0 {
> >> > 	   assert_eq!(val0, value.0);
> >> > 	}
> >> >
> >> > I know it's a bit pointless, since we know it must be ok, but the educational
> >> > message of the example should be to check and not to unwrap, so maybe that's
> >> > better.
> >> 
> >> The if-let will silently ignore any errors, so I don't think that it's
> >> fit for example code either.
> >
> > Yes, but we still have the assert!() before, so the full sequence would be:
> >
> > 	assert!(val0.is_ok());
> >
> > 	if let Ok(val0) = val0 {
> > 	   assert_eq!(val0, value.0);
> > 	}
> 
> Ah right, missed that.
> 
> > The intention would be to avoid patterns that shouldn't be used in "real" code;
> > assert!() should be obvious not to use for real code.
> 
> Yeah, I'm not sure if this is that valuable. I think having "real code"
> is better, but I don't have any idea what to do in this case.
> 
> Why does this sample do the validation in the `drop` method in the first
> place?

I assume there is no specific reason, maybe Abdiel wanted to have a bit more
lifecycle for the allocation than just probe().

I guess we could just move it to probe(). Alternatively we can also keep it in a
closure or function and only assert! once for the returned Result.

> I guess the same code on the C side would do this in `remove` or
> whatever the equivalent thing is there, but would there be the option to
> report an error? Or is `remove` an infallible operation? In that case
> `assert!` probably is still the best option.

remove() is and has to be infallible, yes.