[PATCH 0/3] Additional fixes for dma coherent allocator

[PATCH 0/3] Additional fixes for dma coherent allocator

[Abdiel Janulgue]

Additional fixups to improve the documentation and make the read/write
macros return Result as suggested by Andreas Hindborg as well as support
for reading and writing large blocks of data.

Abdiel Janulgue (3):
  rust: dma: be consistent in using the `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       | 153 +++++++++++++++++++++++++++++++--------
 samples/rust/rust_dma.rs |  21 ++----
 2 files changed, 131 insertions(+), 43 deletions(-)


base-commit: e6ea10d5dbe082c54add289b44f08c9fcfe658af
-- 
2.43.0

[PATCH 1/3] rust: dma: be consistent in using the `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.

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 2/3] rust: dma: convert the read/write macros to return Result

[Abdiel Janulgue]

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@gmail.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<_> {
+            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) }
+        (|| -> Result {
+            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)
-        }
+        (|| -> Result {
+            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 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);
+        }
     }
 }
 
-- 
2.43.0

[PATCH 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 | 87 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 24a6f10370c4..24025ec602ff 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -218,6 +218,93 @@ 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.
+    ///
+    /// Due to the safety requirements of slice, the caller should consider that the region could
+    /// be modified by the device at anytime. For ringbuffer type of r/w access or use-cases where
+    /// the pointer to the live data is needed, `start_ptr()` or `start_ptr_mut()` could be
+    /// used instead.
+    ///
+    /// # Safety
+    ///
+    /// * Callers must ensure that no hardware operations that involve the buffer are currently
+    ///   taking place 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 no hardware operations that involve the buffer are currently
+    ///   taking place 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 no hardware operations that involve the buffer overlaps with
+    ///   this write.
+    /// * 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 0/3] Additional fixes for dma coherent allocator

[Miguel Ojeda]

On Wed, Mar 26, 2025 at 9:13 PM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
>
> Additional fixups to improve the documentation and make the read/write
> macros return Result as suggested by Andreas Hindborg as well as support
> for reading and writing large blocks of data.

As far as I can tell, these seem improvements more than fixes -- so
can they go through the normal process for the next cycle?

Thanks!

Cheers,
Miguel

Re: [PATCH 0/3] Additional fixes for dma coherent allocator

[Abdiel Janulgue]

Hi Miguel,

On 26/03/2025 22:18, Miguel Ojeda wrote:
> On Wed, Mar 26, 2025 at 9:13 PM Abdiel Janulgue
> <abdiel.janulgue@gmail.com> wrote:
>>
>> Additional fixups to improve the documentation and make the read/write
>> macros return Result as suggested by Andreas Hindborg as well as support
>> for reading and writing large blocks of data.
> 
> As far as I can tell, these seem improvements more than fixes -- so
> can they go through the normal process for the next cycle?
> 
Sure! I'm totally fine with it.

Regards,
Abdiel

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

[Miguel Ojeda]

On Wed, Mar 26, 2025 at 9:13 PM Abdiel Janulgue
<abdiel.janulgue@gmail.com> 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@gmail.com>

I would suggest:

Suggested-by: Andreas Hindborg <a.hindborg@kernel.org>
Link: https://lore.kernel.org/rust-for-linux/87h63qhz4q.fsf@kernel.org/

Maybe also consider if Co-developed-by etc. makes sense since he
provided the diff.

Thanks!

Cheers,
Miguel

Re: [PATCH 1/3] rust: dma: be consistent in using the `coherent` nomenclature

[Benno Lossin]

On Wed Mar 26, 2025 at 9:11 PM CET, Abdiel Janulgue wrote:
> 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.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

---
Cheers,
Benno

> ---
>  rust/kernel/dma.rs | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

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@gmail.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.

---
Cheers,
Benno

>      }
>  }
>  

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

[Benno Lossin]

On Wed Mar 26, 2025 at 9:11 PM CET, Abdiel Janulgue wrote:
> +    /// 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.
> +    ///
> +    /// Due to the safety requirements of slice, the caller should consider that the region could
> +    /// be modified by the device at anytime. For ringbuffer type of r/w access or use-cases where
> +    /// the pointer to the live data is needed, `start_ptr()` or `start_ptr_mut()` could be
> +    /// used instead.
> +    ///
> +    /// # Safety
> +    ///
> +    /// * Callers must ensure that no hardware operations that involve the buffer are currently
> +    ///   taking place 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) })

I vaguely recall that there was some discussion on why this is OK (ie
the value behind the reference being modified by the device), but I
haven't followed it. Can you add the reasoning for why that is fine to
some comment here?

I also am not really fond of the phrase "hardware operations that
involve the buffer":
* what do you mean with "buffer"? `self`?
* what are "hardware operations"? (I no nothing about hardware, so that
  might be a knowledge gap on my part)
* what does "involve" mean?

---
Cheers,
Benno

> +    }

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

[Miguel Ojeda]

Few doc nits...

On Wed, Mar 26, 2025 at 9:13 PM Abdiel Janulgue
<abdiel.janulgue@gmail.com> wrote:
>
> +    /// Due to the safety requirements of slice, the caller should consider that the region could

Which requirements in particular? Link?

> +    /// be modified by the device at anytime. For ringbuffer type of r/w access or use-cases where

at anytime -> "at any time" or "anytime"

> +    /// the pointer to the live data is needed, `start_ptr()` or `start_ptr_mut()` could be

Intra-doc links where possible.

> +    /// Writes data to the region starting from `offset`. `offset` is in units of `T`, not the
> +    /// number of bytes.

I would double-check how it looks in the rendered docs (same for the
other method) -- the second sentence may be best in another paragraph,
i.e. outside the title, since it is special in the rendered
documentation ("short description"), unless you want to e.g.
differentiate from another variant.

> +        // SAFETY:
> +        // - The pointer is valid due to type invariant on `CoherentAllocation`
> +        // and we've just checked that the range and index is within bounds.

Please indent the comments as you would the docs -- the docs ones are
correct, and Clippy should check those, but it doesn't check normal
comments, sadly.

Thanks!

Cheers,
Miguel

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

[Abdiel Janulgue]

On 26/03/2025 22:48, Miguel Ojeda wrote:
> On Wed, Mar 26, 2025 at 9:13 PM Abdiel Janulgue
> <abdiel.janulgue@gmail.com> 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@gmail.com>
> 
> I would suggest:
> 
> Suggested-by: Andreas Hindborg <a.hindborg@kernel.org>
> Link: https://lore.kernel.org/rust-for-linux/87h63qhz4q.fsf@kernel.org/
> 
> Maybe also consider if Co-developed-by etc. makes sense since he
> provided the diff.

Thanks! Andreas, would you be okay if I add the Co-developed-by tag?

Regards,
Abdiel

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

[Andreas Hindborg]

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

> On Wed Mar 26, 2025 at 9:11 PM CET, Abdiel Janulgue wrote:
>> +    /// 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.
>> +    ///
>> +    /// Due to the safety requirements of slice, the caller should consider that the region could
>> +    /// be modified by the device at anytime. For ringbuffer type of r/w access or use-cases where
>> +    /// the pointer to the live data is needed, `start_ptr()` or `start_ptr_mut()` could be
>> +    /// used instead.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// * Callers must ensure that no hardware operations that involve the buffer are currently
>> +    ///   taking place 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) })
>
> I vaguely recall that there was some discussion on why this is OK (ie
> the value behind the reference being modified by the device), but I
> haven't followed it. Can you add the reasoning for why that is fine to
> some comment here?

My objection was with the function being safe. In this version it is
unsafe, and the safety requirement is that there must be no concurrent
reads/writes to the memory being operated on. That seems fine to me.

> I also am not really fond of the phrase "hardware operations that
> involve the buffer":
> * what do you mean with "buffer"? `self`?
> * what are "hardware operations"? (I no nothing about hardware, so that
>   might be a knowledge gap on my part)
> * what does "involve" mean?

It is clear to me, but using terminology read/write to/from memory is
probably better. The operations being executed by hardware is not
strictly relevant. We could have a note cautioning that devices must
also obey this.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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

> On 26/03/2025 22:48, Miguel Ojeda wrote:
>> On Wed, Mar 26, 2025 at 9:13 PM Abdiel Janulgue
>> <abdiel.janulgue@gmail.com> 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@gmail.com>
>>
>> I would suggest:
>>
>> Suggested-by: Andreas Hindborg <a.hindborg@kernel.org>
>> Link: https://lore.kernel.org/rust-for-linux/87h63qhz4q.fsf@kernel.org/
>>
>> Maybe also consider if Co-developed-by etc. makes sense since he
>> provided the diff.
>
> Thanks! Andreas, would you be okay if I add the Co-developed-by tag?

Yes, fine with me 👍


Best regards,
Andreas Hindborg

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

[Alexandre Courbot]

Hi Abdiel,

On Thu Mar 27, 2025 at 5:11 AM 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 | 87 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
>
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index 24a6f10370c4..24025ec602ff 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
> @@ -218,6 +218,93 @@ 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.
> +    ///
> +    /// Due to the safety requirements of slice, the caller should consider that the region could
> +    /// be modified by the device at anytime. For ringbuffer type of r/w access or use-cases where
> +    /// the pointer to the live data is needed, `start_ptr()` or `start_ptr_mut()` could be
> +    /// used instead.
> +    ///
> +    /// # Safety
> +    ///
> +    /// * Callers must ensure that no hardware operations that involve the buffer are currently
> +    ///   taking place 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 {

IIUC this should be `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 no hardware operations that involve the buffer are currently
> +    ///   taking place 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 {

Ditto.

> +            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 no hardware operations that involve the buffer overlaps with
> +    ///   this write.
> +    /// * 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 {

Ditto.

> +            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())
> +        };

How about leveraging as_slice_mut() so this unsafe block can be removed?

Cheers,
Alex.

Re: [PATCH 1/3] rust: dma: be consistent in using the `coherent` nomenclature

[Andreas Hindborg]

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

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

This and the next two edits has nothing to do with coherent/consistent
nomenclature. Please update the commit message to reflect that.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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

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


Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.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@gmail.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

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

[Andreas Hindborg]

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

> 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 | 87 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
>
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index 24a6f10370c4..24025ec602ff 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
> @@ -218,6 +218,93 @@ 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.
> +    ///
> +    /// Due to the safety requirements of slice, the caller should consider that the region could
> +    /// be modified by the device at anytime.

The user does not need to consider this, because the safety requirements
make sure this is not a problem. The user only needs to consider the
safety requirements.

>> For ringbuffer type of r/w access or use-cases where
> +    /// the pointer to the live data is needed, `start_ptr()` or `start_ptr_mut()` could be
> +    /// used instead.
> +    ///
> +    /// # Safety
> +    ///
> +    /// * Callers must ensure that no hardware operations that involve the buffer are currently
> +    ///   taking place 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 no hardware operations that involve the buffer are currently
> +    ///   taking place 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.

Formatting nit: could you indent the paragraph under the bullet:

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



Best regards,
Andreas Hindborg

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

[Miguel Ojeda]

On Tue, Apr 8, 2025 at 2:40 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> But the `assert_eq!` would panic anyway if comparison fails, right?

Previously the `?` generated by the macro would return out of the
closure written by the sample, and thus it wouldn't reach the
`assert_eq!`.

Expanded:

    let _ = (|| -> Result {
        for (i, value) in TEST_VALUES.into_iter().enumerate() {
            match (
                &{
                    let item = ...::item_from_index(&self.ca, i)?;
                    unsafe { ... }
                },
                &value.0,
            ) {
                (left_val, right_val) => {
                    if !(*left_val == *right_val) {
                        ...::assert_failed(...);
                    }
                }
            };
        }
        Ok(())
    })();

Cheers,
Miguel

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

[Andreas Hindborg]

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Tue, Apr 8, 2025 at 2:40 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> But the `assert_eq!` would panic anyway if comparison fails, right?
>
> Previously the `?` generated by the macro would return out of the
> closure written by the sample, and thus it wouldn't reach the
> `assert_eq!`.

Right, I see. So the question is whether we want to have the assert
panic here or not, of we get an Err. I vote yes.


Best regards,
Andreas Hindborg

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

[Benno Lossin]

On Tue Apr 8, 2025 at 9:46 PM CEST, Andreas Hindborg wrote:
> "Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:
>> On Tue, Apr 8, 2025 at 2:40 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>
>>> But the `assert_eq!` would panic anyway if comparison fails, right?
>>
>> Previously the `?` generated by the macro would return out of the
>> closure written by the sample, and thus it wouldn't reach the
>> `assert_eq!`.
>
> Right, I see. So the question is whether we want to have the assert
> panic here or not, of we get an Err. I vote yes.

The assert wouldn't be the source of the panic though, it would be the
`.unwrap()`, but I think it's better to report the error. Although I
think it would be nicer if the example could use better error handling,
but this code is in a `drop` function, so no way to bubble up a
result...

---
Cheers,
Benno

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

[Danilo Krummrich]

On Tue, Apr 08, 2025 at 09:54:13PM +0000, Benno Lossin wrote:
> On Tue Apr 8, 2025 at 9:46 PM CEST, Andreas Hindborg wrote:
> > "Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:
> >> On Tue, Apr 8, 2025 at 2:40 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>>
> >>> But the `assert_eq!` would panic anyway if comparison fails, right?
> >>
> >> Previously the `?` generated by the macro would return out of the
> >> closure written by the sample, and thus it wouldn't reach the
> >> `assert_eq!`.
> >
> > Right, I see. So the question is whether we want to have the assert
> > panic here or not, of we get an Err. I vote yes.
> 
> The assert wouldn't be the source of the panic though, it would be the
> `.unwrap()`, but I think it's better to report the error. Although I
> think it would be nicer if the example could use better error handling,
> but this code is in a `drop` function, so no way to bubble up a
> result...

We could be more explicit and assert is_ok() instead and subsequently do the
assert_eq.

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

[Abdiel Janulgue]

Hi Alex,

On 08/04/2025 06:08, Alexandre Courbot wrote:
>> +        }
>> +        // 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())
>> +        };
> 
> How about leveraging as_slice_mut() so this unsafe block can be removed?

as_slice_mut is still unsafe, no? So we couldn't remove unsafe block still?

Thanks,
Abdiel

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

[Alexandre Courbot]

On Thu Apr 10, 2025 at 6:02 PM JST, Abdiel Janulgue wrote:
> Hi Alex,
>
> On 08/04/2025 06:08, Alexandre Courbot wrote:
>>> +        }
>>> +        // 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())
>>> +        };
>> 
>> How about leveraging as_slice_mut() so this unsafe block can be removed?
>
> as_slice_mut is still unsafe, no? So we couldn't remove unsafe block still?

Ah, that's right. In that case it would at least factorize the bound
check and make the method a bit shorter, unless I missed something else.