[PATCH 1/2] rust: dma: make use of start_ptr() and start_ptr_mut()

[PATCH 1/2] rust: dma: make use of start_ptr() and start_ptr_mut()

[Danilo Krummrich]

Using start_ptr() and start_ptr_mut() has the advantage that we inherit
the requirements the a mutable or immutable reference from those
methods.

Hence, use them instead of self.cpu_addr.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/dma.rs | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 4e0af3e1a3b9..f066cbb53cfa 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -505,7 +505,7 @@ pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
         //   data is also guaranteed by the safety requirements of the function.
         // - `offset + count` 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) })
+        Ok(unsafe { core::slice::from_raw_parts(self.start_ptr().add(offset), count) })
     }
 
     /// Performs the same functionality as [`CoherentAllocation::as_slice`], except that a mutable
@@ -525,7 +525,7 @@ pub unsafe fn as_slice_mut(&mut self, offset: usize, count: usize) -> Result<&mu
         //   data is also guaranteed by the safety requirements of the function.
         // - `offset + count` 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) })
+        Ok(unsafe { core::slice::from_raw_parts_mut(self.start_ptr_mut().add(offset), count) })
     }
 
     /// Writes data to the region starting from `offset`. `offset` is in units of `T`, not the
@@ -557,7 +557,11 @@ pub unsafe fn write(&mut self, src: &[T], offset: usize) -> Result {
         // - `offset + count` 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())
+            core::ptr::copy_nonoverlapping(
+                src.as_ptr(),
+                self.start_ptr_mut().add(offset),
+                src.len(),
+            )
         };
         Ok(())
     }
@@ -576,7 +580,7 @@ pub fn item_from_index(&self, offset: usize) -> Result<*mut T> {
         // 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.
-        Ok(unsafe { self.cpu_addr.add(offset) })
+        Ok(unsafe { self.start_ptr().cast_mut().add(offset) })
     }
 
     /// Reads the value of `field` and ensures that its type is [`FromBytes`].
@@ -637,7 +641,7 @@ fn drop(&mut self) {
             bindings::dma_free_attrs(
                 self.dev.as_raw(),
                 size,
-                self.cpu_addr.cast(),
+                self.start_ptr_mut().cast(),
                 self.dma_handle,
                 self.dma_attrs.as_raw(),
             )
-- 
2.51.0

[PATCH 2/2] rust: dma: use NonNull<T> instead of *mut T

[Danilo Krummrich]

In struct CoherentAllocation, use NonNull<T> instead of a raw *mut T for
the CPU address; the CPU address of a valid CoherentAllocation won't
ever be NULL.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/dma.rs | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index f066cbb53cfa..f67c6686a440 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -12,6 +12,7 @@
     sync::aref::ARef,
     transmute::{AsBytes, FromBytes},
 };
+use core::ptr::NonNull;
 
 /// DMA address type.
 ///
@@ -358,7 +359,7 @@ pub struct CoherentAllocation<T: AsBytes + FromBytes> {
     dev: ARef<device::Device>,
     dma_handle: DmaAddress,
     count: usize,
-    cpu_addr: *mut T,
+    cpu_addr: NonNull<T>,
     dma_attrs: Attrs,
 }
 
@@ -392,7 +393,7 @@ pub fn alloc_attrs(
             .ok_or(EOVERFLOW)?;
         let mut dma_handle = 0;
         // SAFETY: Device pointer is guaranteed as valid by the type invariant on `Device`.
-        let ret = unsafe {
+        let addr = unsafe {
             bindings::dma_alloc_attrs(
                 dev.as_raw(),
                 size,
@@ -401,9 +402,7 @@ pub fn alloc_attrs(
                 dma_attrs.as_raw(),
             )
         };
-        if ret.is_null() {
-            return Err(ENOMEM);
-        }
+        let addr = NonNull::new(addr).ok_or(ENOMEM)?;
         // INVARIANT:
         // - We just successfully allocated a coherent region which is accessible for
         //   `count` elements, hence the cpu address is valid. We also hold a refcounted reference
@@ -414,7 +413,7 @@ pub fn alloc_attrs(
             dev: dev.into(),
             dma_handle,
             count,
-            cpu_addr: ret.cast::<T>(),
+            cpu_addr: addr.cast(),
             dma_attrs,
         })
     }
@@ -446,13 +445,13 @@ pub fn size(&self) -> usize {
 
     /// Returns the base address to the allocated region in the CPU's virtual address space.
     pub fn start_ptr(&self) -> *const T {
-        self.cpu_addr
+        self.cpu_addr.as_ptr()
     }
 
     /// Returns the base address to the allocated region in the CPU's virtual address space as
     /// a mutable pointer.
     pub fn start_ptr_mut(&mut self) -> *mut T {
-        self.cpu_addr
+        self.cpu_addr.as_ptr()
     }
 
     /// Returns a DMA handle which may be given to the device as the DMA address base of
-- 
2.51.0

Re: [PATCH 1/2] rust: dma: make use of start_ptr() and start_ptr_mut()

[Alice Ryhl]

On Mon, Nov 03, 2025 at 08:06:49PM +0100, Danilo Krummrich wrote:
> Using start_ptr() and start_ptr_mut() has the advantage that we inherit
> the requirements the a mutable or immutable reference from those
> methods.
> 
> Hence, use them instead of self.cpu_addr.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

>          // 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.
> -        Ok(unsafe { self.cpu_addr.add(offset) })
> +        Ok(unsafe { self.start_ptr().cast_mut().add(offset) })

I guess this shows that the mutable/immutable requirements we inherit
aren't actually what we need?

Alice

Re: [PATCH 2/2] rust: dma: use NonNull<T> instead of *mut T

[Alice Ryhl]

On Mon, Nov 03, 2025 at 08:06:50PM +0100, Danilo Krummrich wrote:
> In struct CoherentAllocation, use NonNull<T> instead of a raw *mut T for
> the CPU address; the CPU address of a valid CoherentAllocation won't
> ever be NULL.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH 1/2] rust: dma: make use of start_ptr() and start_ptr_mut()

[Danilo Krummrich]

On 11/4/25 9:39 AM, Alice Ryhl wrote:
> On Mon, Nov 03, 2025 at 08:06:49PM +0100, Danilo Krummrich wrote:
>> Using start_ptr() and start_ptr_mut() has the advantage that we inherit
>> the requirements the a mutable or immutable reference from those
>> methods.
>>
>> Hence, use them instead of self.cpu_addr.
>>
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> 
>>          // 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.
>> -        Ok(unsafe { self.cpu_addr.add(offset) })
>> +        Ok(unsafe { self.start_ptr().cast_mut().add(offset) })
> 
> I guess this shows that the mutable/immutable requirements we inherit
> aren't actually what we need?

item_from_index() is used for the dma_read!() and dma_write!() macros, hence
this one is on purpose.

Re: [PATCH 1/2] rust: dma: make use of start_ptr() and start_ptr_mut()

[Alexandre Courbot]

On Tue Nov 4, 2025 at 4:06 AM JST, Danilo Krummrich wrote:
> Using start_ptr() and start_ptr_mut() has the advantage that we inherit
> the requirements the a mutable or immutable reference from those

Typo: "the a"

Re: [PATCH 1/2] rust: dma: make use of start_ptr() and start_ptr_mut()

[Alice Ryhl]

On Tue, Nov 04, 2025 at 10:13:08AM +0100, Danilo Krummrich wrote:
> On 11/4/25 9:39 AM, Alice Ryhl wrote:
> > On Mon, Nov 03, 2025 at 08:06:49PM +0100, Danilo Krummrich wrote:
> >> Using start_ptr() and start_ptr_mut() has the advantage that we inherit
> >> the requirements the a mutable or immutable reference from those
> >> methods.
> >>
> >> Hence, use them instead of self.cpu_addr.
> >>
> >> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > 
> >>          // 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.
> >> -        Ok(unsafe { self.cpu_addr.add(offset) })
> >> +        Ok(unsafe { self.start_ptr().cast_mut().add(offset) })
> > 
> > I guess this shows that the mutable/immutable requirements we inherit
> > aren't actually what we need?
> 
> item_from_index() is used for the dma_read!() and dma_write!() macros, hence
> this one is on purpose.

I guess it's more that you don't really need mutable access to call
start_ptr_mut() for this particular case?

Alice

Re: [PATCH 1/2] rust: dma: make use of start_ptr() and start_ptr_mut()

[Danilo Krummrich]

On Tue Nov 4, 2025 at 3:31 PM CET, Alice Ryhl wrote:
> On Tue, Nov 04, 2025 at 10:13:08AM +0100, Danilo Krummrich wrote:
>> On 11/4/25 9:39 AM, Alice Ryhl wrote:
>> > On Mon, Nov 03, 2025 at 08:06:49PM +0100, Danilo Krummrich wrote:
>> >> Using start_ptr() and start_ptr_mut() has the advantage that we inherit
>> >> the requirements the a mutable or immutable reference from those
>> >> methods.
>> >>
>> >> Hence, use them instead of self.cpu_addr.
>> >>
>> >> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> > 
>> >>          // 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.
>> >> -        Ok(unsafe { self.cpu_addr.add(offset) })
>> >> +        Ok(unsafe { self.start_ptr().cast_mut().add(offset) })
>> > 
>> > I guess this shows that the mutable/immutable requirements we inherit
>> > aren't actually what we need?
>> 
>> item_from_index() is used for the dma_read!() and dma_write!() macros, hence
>> this one is on purpose.
>
> I guess it's more that you don't really need mutable access to call
> start_ptr_mut() for this particular case?

In general I think it's good to require a mutable reference for start_ptr_mut(),
as in the absolute majority of cases we actually want to inherit exclusive
access.

As you say, in this particular case we do not want to inherit exclusive access
on purpose.

Re: [PATCH 1/2] rust: dma: make use of start_ptr() and start_ptr_mut()

[Danilo Krummrich]

On Mon Nov 3, 2025 at 8:06 PM CET, Danilo Krummrich wrote:
> @@ -576,7 +580,7 @@ pub fn item_from_index(&self, offset: usize) -> Result<*mut T> {
>          // 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.
> -        Ok(unsafe { self.cpu_addr.add(offset) })
> +        Ok(unsafe { self.start_ptr().cast_mut().add(offset) })

In this specific case start_ptr().cast_mut() is indeed a bit odd, I will use the
following hunk instead and keep the raw access.

@@ -576,7 +580,7 @@ pub fn item_from_index(&self, offset: usize) -> Result<*mut T> {
         // 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.
-        Ok(unsafe { self.cpu_addr.add(offset) })
+        Ok(unsafe { self.cpu_addr.get().add(offset) })

Re: [PATCH 1/2] rust: dma: make use of start_ptr() and start_ptr_mut()

[Alice Ryhl]

On Thu, Nov 06, 2025 at 10:44:06AM +0100, Danilo Krummrich wrote:
> On Mon Nov 3, 2025 at 8:06 PM CET, Danilo Krummrich wrote:
> > @@ -576,7 +580,7 @@ pub fn item_from_index(&self, offset: usize) -> Result<*mut T> {
> >          // 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.
> > -        Ok(unsafe { self.cpu_addr.add(offset) })
> > +        Ok(unsafe { self.start_ptr().cast_mut().add(offset) })
> 
> In this specific case start_ptr().cast_mut() is indeed a bit odd, I will use the
> following hunk instead and keep the raw access.
> 
> @@ -576,7 +580,7 @@ pub fn item_from_index(&self, offset: usize) -> Result<*mut T> {
>          // 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.
> -        Ok(unsafe { self.cpu_addr.add(offset) })
> +        Ok(unsafe { self.cpu_addr.get().add(offset) })

Assuming you got rid of all cast_mut() instances:

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH 1/2] rust: dma: make use of start_ptr() and start_ptr_mut()

[Danilo Krummrich]

On Tue Nov 4, 2025 at 6:06 AM AEDT, Danilo Krummrich wrote:
> Using start_ptr() and start_ptr_mut() has the advantage that we inherit
> the requirements the a mutable or immutable reference from those
> methods.
>
> Hence, use them instead of self.cpu_addr.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Applied to driver-core-testing, thanks!

    [ Keep using self.cpu_addr in item_from_index(). - Danilo ]