[PATCH 0/5] dma::Device trait and DMA mask

[PATCH 0/5] dma::Device trait and DMA mask

[Danilo Krummrich]

This patch series adds the dma::Device trait to be implemented by bus devices on
DMA capable busses.

The dma::Device trait implements methods to set the DMA mask for for such
devices.

The first two bus devices implementing the trait are PCI and platform.

Unfortunately, the DMA mask setters have to be unsafe for now, since, with
reasonable effort, we can't prevent drivers from data races writing and reading
the DMA mask fields concurrently (see also [1]).

Link: https://lore.kernel.org/lkml/DB6YTN5P23X3.2S0NH4YECP1CP@kernel.org/ [1]
Link: https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=rust/dma-mask

Danilo Krummrich (5):
  rust: dma: implement `dma::Device` trait
  rust: dma: add DMA addressing capabilities
  rust: pci: implement the `dma::Device` trait
  rust: platform: implement the `dma::Device` trait
  rust: samples: dma: set DMA mask

 rust/helpers/dma.c       |  5 +++
 rust/kernel/dma.rs       | 95 +++++++++++++++++++++++++++++++++++++---
 rust/kernel/pci.rs       |  2 +
 rust/kernel/platform.rs  |  2 +
 samples/rust/rust_dma.rs | 12 ++++-
 5 files changed, 109 insertions(+), 7 deletions(-)


base-commit: d49ac7744f578bcc8708a845cce24d3b91f86260
-- 
2.50.0

[PATCH 1/5] rust: dma: implement `dma::Device` trait

[Danilo Krummrich]

Add a trait that defines the DMA specific methods of devices.

The `dma::Device` trait is to be implemented by bus device
representations, where the underlying bus is capable of DMA, such as
`pci::Device` or `platform::Device`.

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

diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 2ac4c47aeed3..f0af23d08e8d 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -5,14 +5,21 @@
 //! C header: [`include/linux/dma-mapping.h`](srctree/include/linux/dma-mapping.h)
 
 use crate::{
-    bindings, build_assert,
-    device::{Bound, Device},
+    bindings, build_assert, device,
+    device::Bound,
     error::code::*,
     error::Result,
     transmute::{AsBytes, FromBytes},
     types::ARef,
 };
 
+/// Trait to be implemented by DMA capable bus devices.
+///
+/// The [`dma::Device`](Device) trait should be implemented by bus specific device representations,
+/// where the underlying bus is DMA capable, such as [`pci::Device`](::kernel::pci::Device) or
+/// [`platform::Device`](::kernel::platform::Device).
+pub trait Device: AsRef<device::Device<Core>> {}
+
 /// Possible attributes associated with a DMA mapping.
 ///
 /// They can be combined with the operators `|`, `&`, and `!`.
@@ -132,7 +139,7 @@ pub mod attrs {
 // Hence, find a way to revoke the device resources of a `CoherentAllocation`, but not the
 // entire `CoherentAllocation` including the allocated memory itself.
 pub struct CoherentAllocation<T: AsBytes + FromBytes> {
-    dev: ARef<Device>,
+    dev: ARef<device::Device>,
     dma_handle: bindings::dma_addr_t,
     count: usize,
     cpu_addr: *mut T,
@@ -154,7 +161,7 @@ impl<T: AsBytes + FromBytes> CoherentAllocation<T> {
     /// # Ok::<(), Error>(()) }
     /// ```
     pub fn alloc_attrs(
-        dev: &Device<Bound>,
+        dev: &device::Device<Bound>,
         count: usize,
         gfp_flags: kernel::alloc::Flags,
         dma_attrs: Attrs,
@@ -199,7 +206,7 @@ pub fn alloc_attrs(
     /// Performs the same functionality as [`CoherentAllocation::alloc_attrs`], except the
     /// `dma_attrs` is 0 by default.
     pub fn alloc_coherent(
-        dev: &Device<Bound>,
+        dev: &device::Device<Bound>,
         count: usize,
         gfp_flags: kernel::alloc::Flags,
     ) -> Result<CoherentAllocation<T>> {
-- 
2.50.0

[PATCH 2/5] rust: dma: add DMA addressing capabilities

[Danilo Krummrich]

Implement `dma_set_mask()`, `dma_set_coherent_mask()` and
`dma_set_mask_and_coherent()` in the `dma::Device` trait.

Those methods are used to set up the device's DMA addressing
capabilities.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/helpers/dma.c |  5 +++
 rust/kernel/dma.rs | 82 ++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 84 insertions(+), 3 deletions(-)

diff --git a/rust/helpers/dma.c b/rust/helpers/dma.c
index df8b8a77355a..6e741c197242 100644
--- a/rust/helpers/dma.c
+++ b/rust/helpers/dma.c
@@ -14,3 +14,8 @@ void rust_helper_dma_free_attrs(struct device *dev, size_t size, void *cpu_addr,
 {
 	dma_free_attrs(dev, size, cpu_addr, dma_handle, attrs);
 }
+
+int rust_helper_dma_set_mask_and_coherent(struct device *dev, u64 mask)
+{
+	return dma_set_mask_and_coherent(dev, mask);
+}
diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index f0af23d08e8d..4b27b8279941 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -6,9 +6,9 @@
 
 use crate::{
     bindings, build_assert, device,
-    device::Bound,
+    device::{Bound, Core},
     error::code::*,
-    error::Result,
+    error::{to_result, Result},
     transmute::{AsBytes, FromBytes},
     types::ARef,
 };
@@ -18,7 +18,83 @@
 /// The [`dma::Device`](Device) trait should be implemented by bus specific device representations,
 /// where the underlying bus is DMA capable, such as [`pci::Device`](::kernel::pci::Device) or
 /// [`platform::Device`](::kernel::platform::Device).
-pub trait Device: AsRef<device::Device<Core>> {}
+pub trait Device: AsRef<device::Device<Core>> {
+    /// Set up the device's DMA streaming addressing capabilities.
+    ///
+    /// This method is usually called once from `probe()` as soon as the device capabilities are
+    /// known.
+    ///
+    /// # Safety
+    ///
+    /// This method must not be called concurrently with any DMA allocation or mapping primitives,
+    /// such as [`CoherentAllocation::alloc_attrs`].
+    unsafe fn dma_set_mask(&self, mask: u64) -> Result {
+        // SAFETY:
+        // - By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
+        // - The safety requirement of this function guarantees that there are no concurrent calls
+        //   to DMA allocation and mapping primitives using this mask.
+        to_result(unsafe { bindings::dma_set_mask(self.as_ref().as_raw(), mask) })
+    }
+
+    /// Set up the device's DMA coherent addressing capabilities.
+    ///
+    /// This method is usually called once from `probe()` as soon as the device capabilities are
+    /// known.
+    ///
+    /// # Safety
+    ///
+    /// This method must not be called concurrently with any DMA allocation or mapping primitives,
+    /// such as [`CoherentAllocation::alloc_attrs`].
+    unsafe fn dma_set_coherent_mask(&self, mask: u64) -> Result {
+        // SAFETY:
+        // - By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
+        // - The safety requirement of this function guarantees that there are no concurrent calls
+        //   to DMA allocation and mapping primitives using this mask.
+        to_result(unsafe { bindings::dma_set_coherent_mask(self.as_ref().as_raw(), mask) })
+    }
+
+    /// Set up the device's DMA addressing capabilities.
+    ///
+    /// This is a combination of [`Device::dma_set_mask`] and [`Device::dma_set_coherent_mask`].
+    ///
+    /// This method is usually called once from `probe()` as soon as the device capabilities are
+    /// known.
+    ///
+    /// # Safety
+    ///
+    /// This method must not be called concurrently with any DMA allocation or mapping primitives,
+    /// such as [`CoherentAllocation::alloc_attrs`].
+    unsafe fn dma_set_mask_and_coherent(&self, mask: u64) -> Result {
+        // SAFETY:
+        // - By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
+        // - The safety requirement of this function guarantees that there are no concurrent calls
+        //   to DMA allocation and mapping primitives using this mask.
+        to_result(unsafe { bindings::dma_set_mask_and_coherent(self.as_ref().as_raw(), mask) })
+    }
+}
+
+/// Returns a bitmask with the lowest `n` bits set to `1`.
+///
+/// For `n` in `0..=64`, returns a mask with the lowest `n` bits set.
+/// For `n > 64`, returns `u64::MAX` (all bits set).
+///
+/// # Examples
+///
+/// ```
+/// use kernel::dma::dma_bit_mask;
+///
+/// assert_eq!(dma_bit_mask(0), 0);
+/// assert_eq!(dma_bit_mask(1), 0b1);
+/// assert_eq!(dma_bit_mask(64), u64::MAX);
+/// assert_eq!(dma_bit_mask(100), u64::MAX); // Saturates at all bits set.
+/// ```
+pub const fn dma_bit_mask(n: usize) -> u64 {
+    match n {
+        0 => 0,
+        1..=64 => u64::MAX >> (64 - n),
+        _ => u64::MAX,
+    }
+}
 
 /// Possible attributes associated with a DMA mapping.
 ///
-- 
2.50.0

[PATCH 3/5] rust: pci: implement the `dma::Device` trait

[Danilo Krummrich]

The PCI bus is potentially capable of performing DMA, hence implement
the `dma:Device` trait for `pci::Device`.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/pci.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 8435f8132e38..c0aef0e1faac 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -434,6 +434,8 @@ pub fn set_master(&self) {
 kernel::impl_device_context_deref!(unsafe { Device });
 kernel::impl_device_context_into_aref!(Device);
 
+impl crate::dma::Device for Device<device::Core> {}
+
 // SAFETY: Instances of `Device` are always reference-counted.
 unsafe impl crate::types::AlwaysRefCounted for Device {
     fn inc_ref(&self) {
-- 
2.50.0

[PATCH 4/5] rust: platform: implement the `dma::Device` trait

[Danilo Krummrich]

The platform bus is potentially capable of performing DMA, hence implement
the `dma:Device` trait for `platform::Device`.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/platform.rs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 5b21fa517e55..7236af6482be 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -195,6 +195,8 @@ fn as_raw(&self) -> *mut bindings::platform_device {
 kernel::impl_device_context_deref!(unsafe { Device });
 kernel::impl_device_context_into_aref!(Device);
 
+impl crate::dma::Device for Device<device::Core> {}
+
 // SAFETY: Instances of `Device` are always reference-counted.
 unsafe impl crate::types::AlwaysRefCounted for Device {
     fn inc_ref(&self) {
-- 
2.50.0

[PATCH 5/5] rust: samples: dma: set DMA mask

[Danilo Krummrich]

Set a DMA mask for the `pci::Device` in the Rust DMA sample driver.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 samples/rust/rust_dma.rs | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
index 9e05d5c0cdae..b295c346252f 100644
--- a/samples/rust/rust_dma.rs
+++ b/samples/rust/rust_dma.rs
@@ -4,7 +4,14 @@
 //!
 //! To make this driver probe, QEMU must be run with `-device pci-testdev`.
 
-use kernel::{bindings, device::Core, dma::CoherentAllocation, pci, prelude::*, types::ARef};
+use kernel::{
+    bindings,
+    device::Core,
+    dma::{CoherentAllocation, Device, dma_bit_mask},
+    pci,
+    prelude::*,
+    types::ARef,
+};
 
 struct DmaSampleDriver {
     pdev: ARef<pci::Device>,
@@ -51,6 +58,9 @@ impl pci::Driver for DmaSampleDriver {
     fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>>> {
         dev_info!(pdev.as_ref(), "Probe DMA test driver.\n");
 
+        // SAFETY: There are no concurrent calls to DMA allocation and mapping primitives.
+        unsafe { pdev.dma_set_mask_and_coherent(dma_bit_mask(64))? };
+
         let ca: CoherentAllocation<MyStruct> =
             CoherentAllocation::alloc_coherent(pdev.as_ref(), TEST_VALUES.len(), GFP_KERNEL)?;
 
-- 
2.50.0

Re: [PATCH 2/5] rust: dma: add DMA addressing capabilities

[Joel Fernandes]

Hi Danilo,

On Thu, Jul 10, 2025 at 09:45:44PM +0200, Danilo Krummrich wrote:
> Implement `dma_set_mask()`, `dma_set_coherent_mask()` and
> `dma_set_mask_and_coherent()` in the `dma::Device` trait.
> 
> Those methods are used to set up the device's DMA addressing
> capabilities.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

It looks good to me. A few high-level comments:

1. If we don't expect the concurrency issue for this in C code, why do we
expect it to happen in rust? 

2. Since the Rust code is wrapping around the C code, the data race is
happening entirely on the C side right? So can we just rely on KCSAN to catch
concurrency issues instead of marking the callers as unsafe? I feel the
unsafe { } really might make the driver code ugly.

3. Maybe we could document this issue than enforce it via unsafe? My concern
is wrapping unsafe { } makes the calling code ugly.

4. Are there other kernel APIs where Rust wraps potentially racy C code as
unsafe?

5. In theory, all rust bindings wrappers are unsafe and we do mark it around
the bindings call, right? But in this case, we're also making the calling
code of the unsafe caller as unsafe. C code is 'unsafe' obviously from Rust
PoV but I am not sure we worry about the internal implementation-unsafety of
the C code because then maybe most bindings wrappers would need to be unsafe,
not only these DMA ones.

thanks,

 - Joel


> ---
>  rust/helpers/dma.c |  5 +++
>  rust/kernel/dma.rs | 82 ++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 84 insertions(+), 3 deletions(-)
> 
> diff --git a/rust/helpers/dma.c b/rust/helpers/dma.c
> index df8b8a77355a..6e741c197242 100644
> --- a/rust/helpers/dma.c
> +++ b/rust/helpers/dma.c
> @@ -14,3 +14,8 @@ void rust_helper_dma_free_attrs(struct device *dev, size_t size, void *cpu_addr,
>  {
>  	dma_free_attrs(dev, size, cpu_addr, dma_handle, attrs);
>  }
> +
> +int rust_helper_dma_set_mask_and_coherent(struct device *dev, u64 mask)
> +{
> +	return dma_set_mask_and_coherent(dev, mask);
> +}
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index f0af23d08e8d..4b27b8279941 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
> @@ -6,9 +6,9 @@
>  
>  use crate::{
>      bindings, build_assert, device,
> -    device::Bound,
> +    device::{Bound, Core},
>      error::code::*,
> -    error::Result,
> +    error::{to_result, Result},
>      transmute::{AsBytes, FromBytes},
>      types::ARef,
>  };
> @@ -18,7 +18,83 @@
>  /// The [`dma::Device`](Device) trait should be implemented by bus specific device representations,
>  /// where the underlying bus is DMA capable, such as [`pci::Device`](::kernel::pci::Device) or
>  /// [`platform::Device`](::kernel::platform::Device).
> -pub trait Device: AsRef<device::Device<Core>> {}
> +pub trait Device: AsRef<device::Device<Core>> {
> +    /// Set up the device's DMA streaming addressing capabilities.
> +    ///
> +    /// This method is usually called once from `probe()` as soon as the device capabilities are
> +    /// known.
> +    ///
> +    /// # Safety
> +    ///
> +    /// This method must not be called concurrently with any DMA allocation or mapping primitives,
> +    /// such as [`CoherentAllocation::alloc_attrs`].
> +    unsafe fn dma_set_mask(&self, mask: u64) -> Result {
> +        // SAFETY:
> +        // - By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
> +        // - The safety requirement of this function guarantees that there are no concurrent calls
> +        //   to DMA allocation and mapping primitives using this mask.
> +        to_result(unsafe { bindings::dma_set_mask(self.as_ref().as_raw(), mask) })
> +    }
> +
> +    /// Set up the device's DMA coherent addressing capabilities.
> +    ///
> +    /// This method is usually called once from `probe()` as soon as the device capabilities are
> +    /// known.
> +    ///
> +    /// # Safety
> +    ///
> +    /// This method must not be called concurrently with any DMA allocation or mapping primitives,
> +    /// such as [`CoherentAllocation::alloc_attrs`].
> +    unsafe fn dma_set_coherent_mask(&self, mask: u64) -> Result {
> +        // SAFETY:
> +        // - By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
> +        // - The safety requirement of this function guarantees that there are no concurrent calls
> +        //   to DMA allocation and mapping primitives using this mask.
> +        to_result(unsafe { bindings::dma_set_coherent_mask(self.as_ref().as_raw(), mask) })
> +    }
> +
> +    /// Set up the device's DMA addressing capabilities.
> +    ///
> +    /// This is a combination of [`Device::dma_set_mask`] and [`Device::dma_set_coherent_mask`].
> +    ///
> +    /// This method is usually called once from `probe()` as soon as the device capabilities are
> +    /// known.
> +    ///
> +    /// # Safety
> +    ///
> +    /// This method must not be called concurrently with any DMA allocation or mapping primitives,
> +    /// such as [`CoherentAllocation::alloc_attrs`].
> +    unsafe fn dma_set_mask_and_coherent(&self, mask: u64) -> Result {
> +        // SAFETY:
> +        // - By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
> +        // - The safety requirement of this function guarantees that there are no concurrent calls
> +        //   to DMA allocation and mapping primitives using this mask.
> +        to_result(unsafe { bindings::dma_set_mask_and_coherent(self.as_ref().as_raw(), mask) })
> +    }
> +}
> +
> +/// Returns a bitmask with the lowest `n` bits set to `1`.
> +///
> +/// For `n` in `0..=64`, returns a mask with the lowest `n` bits set.
> +/// For `n > 64`, returns `u64::MAX` (all bits set).
> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::dma::dma_bit_mask;
> +///
> +/// assert_eq!(dma_bit_mask(0), 0);
> +/// assert_eq!(dma_bit_mask(1), 0b1);
> +/// assert_eq!(dma_bit_mask(64), u64::MAX);
> +/// assert_eq!(dma_bit_mask(100), u64::MAX); // Saturates at all bits set.
> +/// ```
> +pub const fn dma_bit_mask(n: usize) -> u64 {
> +    match n {
> +        0 => 0,
> +        1..=64 => u64::MAX >> (64 - n),
> +        _ => u64::MAX,
> +    }
> +}
>  
>  /// Possible attributes associated with a DMA mapping.
>  ///
> -- 
> 2.50.0
> 

Re: [PATCH 2/5] rust: dma: add DMA addressing capabilities

[Danilo Krummrich]

On Fri Jul 11, 2025 at 9:35 PM CEST, Joel Fernandes wrote:
> It looks good to me. A few high-level comments:
>
> 1. If we don't expect the concurrency issue for this in C code, why do we
> expect it to happen in rust? 

The race can happen in C as well, but people would probably argue that no one
ever calls the mask setter function concurrently to DMA allocation and mapping
primitives.

> 2. Since the Rust code is wrapping around the C code, the data race is
> happening entirely on the C side right? So can we just rely on KCSAN to catch
> concurrency issues instead of marking the callers as unsafe? I feel the
> unsafe { } really might make the driver code ugly.

If the function is declared safe in Rust it means that we have to guarantee that
any possible use won't lead to undefined behavior. This isn't the case here
unfortunately.

> 3. Maybe we could document this issue than enforce it via unsafe? My concern
> is wrapping unsafe { } makes the calling code ugly.

Not possible, that's exactly what unsafe is for. We can't violate the "safe
functions can't cause UB" guarantee.

> 4. Are there other kernel APIs where Rust wraps potentially racy C code as
> unsafe?

Sure, but we usually don't expose those to drivers. We always try to make things
safe for drivers.

This case is a bit special though. There is a way to avoid unsafe for the DMA
mask setter methods, but it would involve at least three new type states for
device structures and duplicated API interfaces for types that expect certain
type states.

The conclusion was to go with unsafe for now, since introducing this kind of
complexity is not worth for something that is (not entirely, but more of) a
formal problem.

In the long term I'd like to get those setters safe, e.g. by making the DMA mask
fields atomics.

> 5. In theory, all rust bindings wrappers are unsafe and we do mark it around
> the bindings call, right? But in this case, we're also making the calling
> code of the unsafe caller as unsafe. C code is 'unsafe' obviously from Rust
> PoV but I am not sure we worry about the internal implementation-unsafety of
> the C code because then maybe most bindings wrappers would need to be unsafe,
> not only these DMA ones.

No, the key is to design Rust APIs in a way that the abstraction can't call C
functions in a way that potentially causes undefined behavor.

As an example, let's have a look at a PCI device function:

	/// Enable bus-mastering for this device.
	pub fn set_master(&self) {
	    // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
	    unsafe { bindings::pci_set_master(self.as_raw()) };
	}

pci_set_master() is unsafe because it takes a pointer value, so the caller can
easily cause UB by passing an invalid pointer.

pci::Device::set_master() is safe, because once you have a pci::Device instance, there is no way
(other than due to a bug) that you obtain an invalid pointer from it to
subsequently call pci_set_master().

This means for the caller there is no way to call pci::Device::set_master() such
that it can cause UB.

Re: [PATCH 2/5] rust: dma: add DMA addressing capabilities

[Miguel Ojeda]

On Fri, Jul 11, 2025 at 9:35 PM Joel Fernandes <joelagnelf@nvidia.com> wrote:
>
> 2. Since the Rust code is wrapping around the C code, the data race is
> happening entirely on the C side right? So can we just rely on KCSAN to catch
> concurrency issues instead of marking the callers as unsafe? I feel the
> unsafe { } really might make the driver code ugly.
>
> 3. Maybe we could document this issue than enforce it via unsafe? My concern
> is wrapping unsafe { } makes the calling code ugly.

Yeah, this sort of dilemma comes up from time to time, yeah, e.g. see:

    https://lore.kernel.org/rust-for-linux/CANiq72k_NNFsQ=GGCsur34CTYhSFC0m=mHS83mTB8HQCDBcW=w@mail.gmail.com/
    https://lore.kernel.org/rust-for-linux/CANiq72m3WFj9Eb2iRUM3mLFibWW+cupAoNQt+cqtNa4O9=jq7Q@mail.gmail.com/

In short: that is the job of `unsafe {}` -- if we start to avoid it
just to make code prettier, then it loses its power.

There are few alternatives/notes, though:

  - If there is a way to somehow guarantee or check that something is
safe, perhaps with a tool like Klint, then that could allow us to
avoid `unsafe`.

  - If the blocks are very repetitive in a single user and don't add
any value, then one could consider having users write a single `unsafe
{}` where they promise to uphold X instead of requiring it in further
calls.

  - Worst case, we can promote something to the potential ASH list
idea ("Acknowledged Soundness Holes"): a list where we document things
that do not require `unsafe {}` that should require it but don't for
strong practical reasons.

> 5. In theory, all rust bindings wrappers are unsafe and we do mark it around
> the bindings call, right? But in this case, we're also making the calling
> code of the unsafe caller as unsafe. C code is 'unsafe' obviously from Rust
> PoV but I am not sure we worry about the internal implementation-unsafety of
> the C code because then maybe most bindings wrappers would need to be unsafe,
> not only these DMA ones.

It is orthogonal -- you may have a safe function that uses `unsafe {}`
inside (e.g. to call a C function), but also you will see unsafe
functions with just safe code inside. And, of course, safe functions
with only safe code inside and unsafe functions with `unsafe {}`
blocks inside.

In other words, a safe function does not mean unsafe code is used or
not inside. Similarly, an unsafe function does not mean unsafe code is
used (or not) inside either.

This is the usual "two meaning of `unsafe`" -- that of e.g. functions
(where it means there are safety preconditions for calling a function)
and that of e.g. `unsafe {}` blocks (where it means the caller must
play by the rules, e.g. satisfy the safety preconditions to call an
unsafe function).

So a Rust function that calls C functions (and thus uses `unsafe {}`)
may or may not need to be unsafe -- it all depends on the case. That
is, it depends on whether callers can cause UB or not. And similarly,
a Rust function that does not use unsafe (including not calling C
functions) can still be very much unsafe, because other code may rely
on that code for soundness (e.g. an unsafe method that assigns to an
internal pointer which then other safe methods rely on).

Cheers,
Miguel

Re: [PATCH 2/5] rust: dma: add DMA addressing capabilities

[Joel Fernandes]

On 7/11/2025 3:54 PM, Danilo Krummrich wrote:
> On Fri Jul 11, 2025 at 9:35 PM CEST, Joel Fernandes wrote:
>> It looks good to me. A few high-level comments:
>>
>> 1. If we don't expect the concurrency issue for this in C code, why do we
>> expect it to happen in rust? 
> 
> The race can happen in C as well, but people would probably argue that no one
> ever calls the mask setter function concurrently to DMA allocation and mapping
> primitives.
> 
After going through the DMA code some more, I am convinced the concurrency is an
issue so marking the higher-level rust wrapper as unsafe { } makes sense.

Thanks Danilo for the discussion and Miguel also for the clarifications on the
other thread.

 - Joel

Re: [PATCH 2/5] rust: dma: add DMA addressing capabilities

[Joel Fernandes]

On 7/11/2025 4:14 PM, Miguel Ojeda wrote:
> On Fri, Jul 11, 2025 at 9:35 PM Joel Fernandes <joelagnelf@nvidia.com> wrote:
>>
>> 2. Since the Rust code is wrapping around the C code, the data race is
>> happening entirely on the C side right? So can we just rely on KCSAN to catch
>> concurrency issues instead of marking the callers as unsafe? I feel the
>> unsafe { } really might make the driver code ugly.
>>
>> 3. Maybe we could document this issue than enforce it via unsafe? My concern
>> is wrapping unsafe { } makes the calling code ugly.
> 
> Yeah, this sort of dilemma comes up from time to time, yeah, e.g. see:
> 
>     https://lore.kernel.org/rust-for-linux/CANiq72k_NNFsQ=GGCsur34CTYhSFC0m=mHS83mTB8HQCDBcW=w@mail.gmail.com/
>     https://lore.kernel.org/rust-for-linux/CANiq72m3WFj9Eb2iRUM3mLFibWW+cupAoNQt+cqtNa4O9=jq7Q@mail.gmail.com/
> 
> In short: that is the job of `unsafe {}` -- if we start to avoid it
> just to make code prettier, then it loses its power.
> 
> There are few alternatives/notes, though:
> 
>   - If there is a way to somehow guarantee or check that something is
> safe, perhaps with a tool like Klint, then that could allow us to
> avoid `unsafe`.
> 
>   - If the blocks are very repetitive in a single user and don't add
> any value, then one could consider having users write a single `unsafe
> {}` where they promise to uphold X instead of requiring it in further
> calls.
> 
>   - Worst case, we can promote something to the potential ASH list
> idea ("Acknowledged Soundness Holes"): a list where we document things
> that do not require `unsafe {}` that should require it but don't for
> strong practical reasons.
> 
>> 5. In theory, all rust bindings wrappers are unsafe and we do mark it around
>> the bindings call, right? But in this case, we're also making the calling
>> code of the unsafe caller as unsafe. C code is 'unsafe' obviously from Rust
>> PoV but I am not sure we worry about the internal implementation-unsafety of
>> the C code because then maybe most bindings wrappers would need to be unsafe,
>> not only these DMA ones.
> 
> It is orthogonal -- you may have a safe function that uses `unsafe {}`
> inside (e.g. to call a C function), but also you will see unsafe
> functions with just safe code inside. And, of course, safe functions
> with only safe code inside and unsafe functions with `unsafe {}`
> blocks inside.
> 
> In other words, a safe function does not mean unsafe code is used or
> not inside. Similarly, an unsafe function does not mean unsafe code is
> used (or not) inside either.
> 
> This is the usual "two meaning of `unsafe`" -- that of e.g. functions
> (where it means there are safety preconditions for calling a function)
> and that of e.g. `unsafe {}` blocks (where it means the caller must
> play by the rules, e.g. satisfy the safety preconditions to call an
> unsafe function).
> 
> So a Rust function that calls C functions (and thus uses `unsafe {}`)
> may or may not need to be unsafe -- it all depends on the case. That
> is, it depends on whether callers can cause UB or not. And similarly,
> a Rust function that does not use unsafe (including not calling C
> functions) can still be very much unsafe, because other code may rely
> on that code for soundness (e.g. an unsafe method that assigns to an
> internal pointer which then other safe methods rely on).
> 
Thanks for this clarification and write up! I need it so thank you very much Miguel!

 - Joel

Re: [PATCH 0/5] dma::Device trait and DMA mask

[Abdiel Janulgue]

Thanks for completing this missing feature!

I've looked it into detail, and for patches 1, 2, and 5:

Reviewed-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>

On 7/10/25 22:45, Danilo Krummrich wrote:
> This patch series adds the dma::Device trait to be implemented by bus devices on
> DMA capable busses.
> 
> The dma::Device trait implements methods to set the DMA mask for for such
> devices.
> 
> The first two bus devices implementing the trait are PCI and platform.
> 
> Unfortunately, the DMA mask setters have to be unsafe for now, since, with
> reasonable effort, we can't prevent drivers from data races writing and reading
> the DMA mask fields concurrently (see also [1]).
> 
> Link: https://lore.kernel.org/lkml/DB6YTN5P23X3.2S0NH4YECP1CP@kernel.org/ [1]
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=rust/dma-mask
> 
> Danilo Krummrich (5):
>    rust: dma: implement `dma::Device` trait
>    rust: dma: add DMA addressing capabilities
>    rust: pci: implement the `dma::Device` trait
>    rust: platform: implement the `dma::Device` trait
>    rust: samples: dma: set DMA mask
> 
>   rust/helpers/dma.c       |  5 +++
>   rust/kernel/dma.rs       | 95 +++++++++++++++++++++++++++++++++++++---
>   rust/kernel/pci.rs       |  2 +
>   rust/kernel/platform.rs  |  2 +
>   samples/rust/rust_dma.rs | 12 ++++-
>   5 files changed, 109 insertions(+), 7 deletions(-)
> 
> 
> base-commit: d49ac7744f578bcc8708a845cce24d3b91f86260

Re: [PATCH 2/5] rust: dma: add DMA addressing capabilities

[Alexandre Courbot]

On Fri Jul 11, 2025 at 4:45 AM JST, Danilo Krummrich wrote:
> Implement `dma_set_mask()`, `dma_set_coherent_mask()` and
> `dma_set_mask_and_coherent()` in the `dma::Device` trait.
>
> Those methods are used to set up the device's DMA addressing
> capabilities.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/helpers/dma.c |  5 +++
>  rust/kernel/dma.rs | 82 ++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 84 insertions(+), 3 deletions(-)
>
> diff --git a/rust/helpers/dma.c b/rust/helpers/dma.c
> index df8b8a77355a..6e741c197242 100644
> --- a/rust/helpers/dma.c
> +++ b/rust/helpers/dma.c
> @@ -14,3 +14,8 @@ void rust_helper_dma_free_attrs(struct device *dev, size_t size, void *cpu_addr,
>  {
>  	dma_free_attrs(dev, size, cpu_addr, dma_handle, attrs);
>  }
> +
> +int rust_helper_dma_set_mask_and_coherent(struct device *dev, u64 mask)
> +{
> +	return dma_set_mask_and_coherent(dev, mask);
> +}
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index f0af23d08e8d..4b27b8279941 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
> @@ -6,9 +6,9 @@
>  
>  use crate::{
>      bindings, build_assert, device,
> -    device::Bound,
> +    device::{Bound, Core},
>      error::code::*,
> -    error::Result,
> +    error::{to_result, Result},
>      transmute::{AsBytes, FromBytes},
>      types::ARef,
>  };
> @@ -18,7 +18,83 @@
>  /// The [`dma::Device`](Device) trait should be implemented by bus specific device representations,
>  /// where the underlying bus is DMA capable, such as [`pci::Device`](::kernel::pci::Device) or
>  /// [`platform::Device`](::kernel::platform::Device).
> -pub trait Device: AsRef<device::Device<Core>> {}
> +pub trait Device: AsRef<device::Device<Core>> {
> +    /// Set up the device's DMA streaming addressing capabilities.
> +    ///
> +    /// This method is usually called once from `probe()` as soon as the device capabilities are
> +    /// known.
> +    ///
> +    /// # Safety
> +    ///
> +    /// This method must not be called concurrently with any DMA allocation or mapping primitives,
> +    /// such as [`CoherentAllocation::alloc_attrs`].

I'm a bit confused by the use of "concurrently" in this sentence. Do you
mean that it must be called *before* any DMA allocation of mapping
primitives? In this case, wouldn't it be clearer to make the order
explicit?

> +    unsafe fn dma_set_mask(&self, mask: u64) -> Result {

Do we want to allow any u64 as a valid mask? If not, shall we restrict
the accepted values by taking either the parameter to give to
`dma_bit_mask`, or a bit range (similarly to Daniel's bitmask series
[1], which it might make sense to leverage)?

[1]
https://lore.kernel.org/rust-for-linux/20250714-topics-tyr-genmask2-v9-1-9e6422cbadb6@collabora.com/

Re: [PATCH 2/5] rust: dma: add DMA addressing capabilities

[Danilo Krummrich]

On Wed Jul 16, 2025 at 5:18 AM CEST, Alexandre Courbot wrote:
> On Fri Jul 11, 2025 at 4:45 AM JST, Danilo Krummrich wrote:
>> @@ -18,7 +18,83 @@
>>  /// The [`dma::Device`](Device) trait should be implemented by bus specific device representations,
>>  /// where the underlying bus is DMA capable, such as [`pci::Device`](::kernel::pci::Device) or
>>  /// [`platform::Device`](::kernel::platform::Device).
>> -pub trait Device: AsRef<device::Device<Core>> {}
>> +pub trait Device: AsRef<device::Device<Core>> {
>> +    /// Set up the device's DMA streaming addressing capabilities.
>> +    ///
>> +    /// This method is usually called once from `probe()` as soon as the device capabilities are
>> +    /// known.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// This method must not be called concurrently with any DMA allocation or mapping primitives,
>> +    /// such as [`CoherentAllocation::alloc_attrs`].
>
> I'm a bit confused by the use of "concurrently" in this sentence. Do you
> mean that it must be called *before* any DMA allocation of mapping
> primitives? In this case, wouldn't it be clearer to make the order
> explicit?

Setting the mask before any DMA allocations might only be relevant from a
semantical point of view, but not safety wise.

We need to prevent concurrent accesses to dev->dma_mask and
dev->coherent_dma_mask.

>> +    unsafe fn dma_set_mask(&self, mask: u64) -> Result {
>
> Do we want to allow any u64 as a valid mask? If not, shall we restrict
> the accepted values by taking either the parameter to give to
> `dma_bit_mask`, or a bit range (similarly to Daniel's bitmask series
> [1], which it might make sense to leverage)?
>
> [1]
> https://lore.kernel.org/rust-for-linux/20250714-topics-tyr-genmask2-v9-1-9e6422cbadb6@collabora.com/

I think it would make sense to make dma_bit_mask() return a new type, e.g.
DmaMask and take this instead.

Taking the parameter dma_bit_mask() takes directly in dma_set_mask() etc. makes
sense, but changes the semantics of the mask parameter *subtly* compared to the
C versions, which I want to avoid.

Using the infrastructure in [1] doesn't seem to provide much value, since we
don't want a range [M..N], but [0..N], so we should rather only ask for N.

Re: [PATCH 2/5] rust: dma: add DMA addressing capabilities

[Alexandre Courbot]

On Wed Jul 16, 2025 at 5:04 PM JST, Danilo Krummrich wrote:
> On Wed Jul 16, 2025 at 5:18 AM CEST, Alexandre Courbot wrote:
>> On Fri Jul 11, 2025 at 4:45 AM JST, Danilo Krummrich wrote:
>>> @@ -18,7 +18,83 @@
>>>  /// The [`dma::Device`](Device) trait should be implemented by bus specific device representations,
>>>  /// where the underlying bus is DMA capable, such as [`pci::Device`](::kernel::pci::Device) or
>>>  /// [`platform::Device`](::kernel::platform::Device).
>>> -pub trait Device: AsRef<device::Device<Core>> {}
>>> +pub trait Device: AsRef<device::Device<Core>> {
>>> +    /// Set up the device's DMA streaming addressing capabilities.
>>> +    ///
>>> +    /// This method is usually called once from `probe()` as soon as the device capabilities are
>>> +    /// known.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// This method must not be called concurrently with any DMA allocation or mapping primitives,
>>> +    /// such as [`CoherentAllocation::alloc_attrs`].
>>
>> I'm a bit confused by the use of "concurrently" in this sentence. Do you
>> mean that it must be called *before* any DMA allocation of mapping
>> primitives? In this case, wouldn't it be clearer to make the order
>> explicit?
>
> Setting the mask before any DMA allocations might only be relevant from a
> semantical point of view, but not safety wise.
>
> We need to prevent concurrent accesses to dev->dma_mask and
> dev->coherent_dma_mask.
>
>>> +    unsafe fn dma_set_mask(&self, mask: u64) -> Result {
>>
>> Do we want to allow any u64 as a valid mask? If not, shall we restrict
>> the accepted values by taking either the parameter to give to
>> `dma_bit_mask`, or a bit range (similarly to Daniel's bitmask series
>> [1], which it might make sense to leverage)?
>>
>> [1]
>> https://lore.kernel.org/rust-for-linux/20250714-topics-tyr-genmask2-v9-1-9e6422cbadb6@collabora.com/
>
> I think it would make sense to make dma_bit_mask() return a new type, e.g.
> DmaMask and take this instead.
>
> Taking the parameter dma_bit_mask() takes directly in dma_set_mask() etc. makes
> sense, but changes the semantics of the mask parameter *subtly* compared to the
> C versions, which I want to avoid.
>
> Using the infrastructure in [1] doesn't seem to provide much value, since we
> don't want a range [M..N], but [0..N], so we should rather only ask for N.

I agree that a dedicated type limiting the possible values to inputs
that make sense would be nice.

Re: [PATCH 2/5] rust: dma: add DMA addressing capabilities

[Greg KH]

On Thu, Jul 10, 2025 at 09:45:44PM +0200, Danilo Krummrich wrote:
> +/// Returns a bitmask with the lowest `n` bits set to `1`.
> +///
> +/// For `n` in `0..=64`, returns a mask with the lowest `n` bits set.
> +/// For `n > 64`, returns `u64::MAX` (all bits set).
> +///
> +/// # Examples
> +///
> +/// ```
> +/// use kernel::dma::dma_bit_mask;
> +///
> +/// assert_eq!(dma_bit_mask(0), 0);
> +/// assert_eq!(dma_bit_mask(1), 0b1);
> +/// assert_eq!(dma_bit_mask(64), u64::MAX);
> +/// assert_eq!(dma_bit_mask(100), u64::MAX); // Saturates at all bits set.
> +/// ```
> +pub const fn dma_bit_mask(n: usize) -> u64 {
> +    match n {
> +        0 => 0,
> +        1..=64 => u64::MAX >> (64 - n),
> +        _ => u64::MAX,
> +    }
> +}

This is just the C macro DMA_BIT_MASK(), right?  If so, can that be said
here somewhere?  Or, how about turning DMA_BIT_MASK() into an inline
function which could then be just called by the rust code directly
instead?

Just a minor thing, but it stood out to me here.

thanks,

greg k-h

Re: [PATCH 2/5] rust: dma: add DMA addressing capabilities

[Danilo Krummrich]

On Wed Jul 16, 2025 at 11:15 AM CEST, Greg KH wrote:
> On Thu, Jul 10, 2025 at 09:45:44PM +0200, Danilo Krummrich wrote:
>> +/// Returns a bitmask with the lowest `n` bits set to `1`.
>> +///
>> +/// For `n` in `0..=64`, returns a mask with the lowest `n` bits set.
>> +/// For `n > 64`, returns `u64::MAX` (all bits set).
>> +///
>> +/// # Examples
>> +///
>> +/// ```
>> +/// use kernel::dma::dma_bit_mask;
>> +///
>> +/// assert_eq!(dma_bit_mask(0), 0);
>> +/// assert_eq!(dma_bit_mask(1), 0b1);
>> +/// assert_eq!(dma_bit_mask(64), u64::MAX);
>> +/// assert_eq!(dma_bit_mask(100), u64::MAX); // Saturates at all bits set.
>> +/// ```
>> +pub const fn dma_bit_mask(n: usize) -> u64 {
>> +    match n {
>> +        0 => 0,
>> +        1..=64 => u64::MAX >> (64 - n),
>> +        _ => u64::MAX,
>> +    }
>> +}
>
> This is just the C macro DMA_BIT_MASK(), right?  If so, can that be said
> here somewhere?

Yes, I think that'd be good.

> Or, how about turning DMA_BIT_MASK() into an inline
> function which could then be just called by the rust code directly
> instead?

Unfortunately, bindgen doesn't pick up either, so converting to an inline
function wouldn't help.

We could use it through a Rust helper though, but I considered this to be
unnecessary overhead. Whether that's relevant in this case is of course
questionable though. :)

Given that we also concluded that we want to return a new type (i.e. DmaMask)
rather than a u64, I feel like it's a bit cleaner to keep it self-contained.

Re: [PATCH 2/5] rust: dma: add DMA addressing capabilities

[Alice Ryhl]

On Wed, Jul 16, 2025 at 11:15 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Jul 10, 2025 at 09:45:44PM +0200, Danilo Krummrich wrote:
> > +/// Returns a bitmask with the lowest `n` bits set to `1`.
> > +///
> > +/// For `n` in `0..=64`, returns a mask with the lowest `n` bits set.
> > +/// For `n > 64`, returns `u64::MAX` (all bits set).
> > +///
> > +/// # Examples
> > +///
> > +/// ```
> > +/// use kernel::dma::dma_bit_mask;
> > +///
> > +/// assert_eq!(dma_bit_mask(0), 0);
> > +/// assert_eq!(dma_bit_mask(1), 0b1);
> > +/// assert_eq!(dma_bit_mask(64), u64::MAX);
> > +/// assert_eq!(dma_bit_mask(100), u64::MAX); // Saturates at all bits set.
> > +/// ```
> > +pub const fn dma_bit_mask(n: usize) -> u64 {
> > +    match n {
> > +        0 => 0,
> > +        1..=64 => u64::MAX >> (64 - n),
> > +        _ => u64::MAX,
> > +    }
> > +}
>
> This is just the C macro DMA_BIT_MASK(), right?  If so, can that be said
> here somewhere?  Or, how about turning DMA_BIT_MASK() into an inline
> function which could then be just called by the rust code directly
> instead?

Converting to an inline method would not make a difference for calling
it from Rust. We would need a helper in rust/helpers/ either way, and
it's also possible to define a helper for a macro.

Alice