linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] dma::Device trait and DMA mask
@ 2025-07-16 15:02 Danilo Krummrich
  2025-07-16 15:02 ` [PATCH v2 1/5] rust: dma: implement `dma::Device` trait Danilo Krummrich
                   ` (7 more replies)
  0 siblings, 8 replies; 16+ messages in thread
From: Danilo Krummrich @ 2025-07-16 15:02 UTC (permalink / raw)
  To: abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg, ojeda,
	alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, aliceryhl,
	tmgross, bhelgaas, kwilczynski, gregkh, rafael
  Cc: rust-for-linux, linux-pci, linux-kernel, 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

Changes in v2:
  - replace dma_bit_mask() with a new type DmaMask
  - mention that DmaMask is the Rust equivalent of the C macro DMA_BIT_MASK()
  - make DmaMask::new() fallible
  - inline DmaMask methods

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       | 125 ++++++++++++++++++++++++++++++++++++---
 rust/kernel/pci.rs       |   2 +
 rust/kernel/platform.rs  |   2 +
 samples/rust/rust_dma.rs |  14 ++++-
 5 files changed, 140 insertions(+), 8 deletions(-)


base-commit: d49ac7744f578bcc8708a845cce24d3b91f86260
-- 
2.50.0


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 1/5] rust: dma: implement `dma::Device` trait
  2025-07-16 15:02 [PATCH v2 0/5] dma::Device trait and DMA mask Danilo Krummrich
@ 2025-07-16 15:02 ` Danilo Krummrich
  2025-07-16 15:02 ` [PATCH v2 2/5] rust: dma: add DMA addressing capabilities Danilo Krummrich
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 16+ messages in thread
From: Danilo Krummrich @ 2025-07-16 15:02 UTC (permalink / raw)
  To: abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg, ojeda,
	alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, aliceryhl,
	tmgross, bhelgaas, kwilczynski, gregkh, rafael
  Cc: rust-for-linux, linux-pci, linux-kernel, 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`.

Reviewed-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 2/5] rust: dma: add DMA addressing capabilities
  2025-07-16 15:02 [PATCH v2 0/5] dma::Device trait and DMA mask Danilo Krummrich
  2025-07-16 15:02 ` [PATCH v2 1/5] rust: dma: implement `dma::Device` trait Danilo Krummrich
@ 2025-07-16 15:02 ` Danilo Krummrich
  2025-07-16 17:32   ` Daniel Almeida
  2025-07-16 15:02 ` [PATCH v2 3/5] rust: pci: implement the `dma::Device` trait Danilo Krummrich
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 16+ messages in thread
From: Danilo Krummrich @ 2025-07-16 15:02 UTC (permalink / raw)
  To: abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg, ojeda,
	alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, aliceryhl,
	tmgross, bhelgaas, kwilczynski, gregkh, rafael
  Cc: rust-for-linux, linux-pci, linux-kernel, 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.

Reviewed-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/helpers/dma.c |   5 ++
 rust/kernel/dma.rs | 112 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 113 insertions(+), 4 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..afd3ba538e3c 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -6,9 +6,9 @@
 
 use crate::{
     bindings, build_assert, device,
-    device::Bound,
-    error::code::*,
-    error::Result,
+    device::{Bound, Core},
+    error::{to_result, Result},
+    prelude::*,
     transmute::{AsBytes, FromBytes},
     types::ARef,
 };
@@ -18,7 +18,111 @@
 /// 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: DmaMask) -> 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.value()) })
+    }
+
+    /// 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: DmaMask) -> 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.value()) })
+    }
+
+    /// 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: DmaMask) -> 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.value())
+        })
+    }
+}
+
+/// A DMA mask that holds a bitmask with the lowest `n` bits set.
+///
+/// Use [`DmaMask::new`] to construct a value. Values are guaranteed to
+/// never exceed the bit width of `u64`.
+///
+/// This is the Rust equivalent of the C macro `DMA_BIT_MASK()`.
+///
+/// # Examples
+///
+/// ```
+/// use kernel::dma::DmaMask;
+///
+/// let mask0 = DmaMask::new(0)?;
+/// assert_eq!(mask0.value(), 0);
+///
+/// let mask1 = DmaMask::new(1)?;
+/// assert_eq!(mask1.value(), 0b1);
+///
+/// let mask64 = DmaMask::new(64)?;
+/// assert_eq!(mask64.value(), u64::MAX);
+///
+/// let mask_overflow = DmaMask::new(100);
+/// assert!(mask_overflow.is_err());
+/// # Ok::<(), Error>(())
+/// ```
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct DmaMask(u64);
+
+impl DmaMask {
+    /// Constructs a `DmaMask` with the lowest `n` bits set to `1`.
+    ///
+    /// For `n <= 64`, sets exactly the lowest `n` bits.
+    /// For `n > 64`, returns [`EINVAL`].
+    #[inline]
+    pub const fn new(n: usize) -> Result<Self> {
+        Ok(Self(match n {
+            0 => 0,
+            1..=64 => u64::MAX >> (64 - n),
+            _ => return Err(EINVAL),
+        }))
+    }
+
+    /// Returns the underlying `u64` bitmask value.
+    #[inline]
+    pub const fn value(&self) -> u64 {
+        self.0
+    }
+}
 
 /// Possible attributes associated with a DMA mapping.
 ///
-- 
2.50.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 3/5] rust: pci: implement the `dma::Device` trait
  2025-07-16 15:02 [PATCH v2 0/5] dma::Device trait and DMA mask Danilo Krummrich
  2025-07-16 15:02 ` [PATCH v2 1/5] rust: dma: implement `dma::Device` trait Danilo Krummrich
  2025-07-16 15:02 ` [PATCH v2 2/5] rust: dma: add DMA addressing capabilities Danilo Krummrich
@ 2025-07-16 15:02 ` Danilo Krummrich
  2025-07-16 15:02 ` [PATCH v2 4/5] rust: platform: " Danilo Krummrich
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 16+ messages in thread
From: Danilo Krummrich @ 2025-07-16 15:02 UTC (permalink / raw)
  To: abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg, ojeda,
	alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, aliceryhl,
	tmgross, bhelgaas, kwilczynski, gregkh, rafael
  Cc: rust-for-linux, linux-pci, linux-kernel, 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 4/5] rust: platform: implement the `dma::Device` trait
  2025-07-16 15:02 [PATCH v2 0/5] dma::Device trait and DMA mask Danilo Krummrich
                   ` (2 preceding siblings ...)
  2025-07-16 15:02 ` [PATCH v2 3/5] rust: pci: implement the `dma::Device` trait Danilo Krummrich
@ 2025-07-16 15:02 ` Danilo Krummrich
  2025-07-16 15:02 ` [PATCH v2 5/5] rust: samples: dma: set DMA mask Danilo Krummrich
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 16+ messages in thread
From: Danilo Krummrich @ 2025-07-16 15:02 UTC (permalink / raw)
  To: abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg, ojeda,
	alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, aliceryhl,
	tmgross, bhelgaas, kwilczynski, gregkh, rafael
  Cc: rust-for-linux, linux-pci, linux-kernel, 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 5/5] rust: samples: dma: set DMA mask
  2025-07-16 15:02 [PATCH v2 0/5] dma::Device trait and DMA mask Danilo Krummrich
                   ` (3 preceding siblings ...)
  2025-07-16 15:02 ` [PATCH v2 4/5] rust: platform: " Danilo Krummrich
@ 2025-07-16 15:02 ` Danilo Krummrich
  2025-07-16 17:28 ` [PATCH v2 0/5] dma::Device trait and " Greg KH
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 16+ messages in thread
From: Danilo Krummrich @ 2025-07-16 15:02 UTC (permalink / raw)
  To: abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg, ojeda,
	alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, aliceryhl,
	tmgross, bhelgaas, kwilczynski, gregkh, rafael
  Cc: rust-for-linux, linux-pci, linux-kernel, Danilo Krummrich

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

Reviewed-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 samples/rust/rust_dma.rs | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
index 9e05d5c0cdae..9422ac68c139 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, DmaMask},
+    pci,
+    prelude::*,
+    types::ARef,
+};
 
 struct DmaSampleDriver {
     pdev: ARef<pci::Device>,
@@ -51,6 +58,11 @@ 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");
 
+        let mask = DmaMask::new(64)?;
+
+        // SAFETY: There are no concurrent calls to DMA allocation and mapping primitives.
+        unsafe { pdev.dma_set_mask_and_coherent(mask)? };
+
         let ca: CoherentAllocation<MyStruct> =
             CoherentAllocation::alloc_coherent(pdev.as_ref(), TEST_VALUES.len(), GFP_KERNEL)?;
 
-- 
2.50.0


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 0/5] dma::Device trait and DMA mask
  2025-07-16 15:02 [PATCH v2 0/5] dma::Device trait and DMA mask Danilo Krummrich
                   ` (4 preceding siblings ...)
  2025-07-16 15:02 ` [PATCH v2 5/5] rust: samples: dma: set DMA mask Danilo Krummrich
@ 2025-07-16 17:28 ` Greg KH
  2025-07-16 17:34 ` Daniel Almeida
  2025-07-20 14:33 ` Danilo Krummrich
  7 siblings, 0 replies; 16+ messages in thread
From: Greg KH @ 2025-07-16 17:28 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg, ojeda,
	alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, aliceryhl,
	tmgross, bhelgaas, kwilczynski, rafael, rust-for-linux, linux-pci,
	linux-kernel

On Wed, Jul 16, 2025 at 05:02:45PM +0200, 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
> 
> Changes in v2:
>   - replace dma_bit_mask() with a new type DmaMask
>   - mention that DmaMask is the Rust equivalent of the C macro DMA_BIT_MASK()
>   - make DmaMask::new() fallible
>   - inline DmaMask methods

I like the DmaMask stuff, nice!

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/5] rust: dma: add DMA addressing capabilities
  2025-07-16 15:02 ` [PATCH v2 2/5] rust: dma: add DMA addressing capabilities Danilo Krummrich
@ 2025-07-16 17:32   ` Daniel Almeida
  2025-07-16 17:41     ` Daniel Almeida
  2025-07-16 17:55     ` Danilo Krummrich
  0 siblings, 2 replies; 16+ messages in thread
From: Daniel Almeida @ 2025-07-16 17:32 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: abdiel.janulgue, robin.murphy, a.hindborg, ojeda, alex.gaynor,
	boqun.feng, gary, bjorn3_gh, lossin, aliceryhl, tmgross, bhelgaas,
	kwilczynski, gregkh, rafael, rust-for-linux, linux-pci,
	linux-kernel

Hi Danilo,

> +    #[inline]
> +    pub const fn new(n: usize) -> Result<Self> {
> +        Ok(Self(match n {
> +            0 => 0,
> +            1..=64 => u64::MAX >> (64 - n),
> +            _ => return Err(EINVAL),
> +        }))
> +    }
> +

Isn’t this equivalent to genmask_u64(0..=n) ? See [0].

You should also get a compile-time failure if n is out of bounds by default using
genmask.

— Daniel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 0/5] dma::Device trait and DMA mask
  2025-07-16 15:02 [PATCH v2 0/5] dma::Device trait and DMA mask Danilo Krummrich
                   ` (5 preceding siblings ...)
  2025-07-16 17:28 ` [PATCH v2 0/5] dma::Device trait and " Greg KH
@ 2025-07-16 17:34 ` Daniel Almeida
  2025-07-20 14:33 ` Danilo Krummrich
  7 siblings, 0 replies; 16+ messages in thread
From: Daniel Almeida @ 2025-07-16 17:34 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: abdiel.janulgue, robin.murphy, a.hindborg, ojeda, alex.gaynor,
	boqun.feng, gary, bjorn3_gh, lossin, aliceryhl, tmgross, bhelgaas,
	kwilczynski, gregkh, rafael, rust-for-linux, linux-pci,
	linux-kernel

Hi Danilo,

I think this looks good.

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/5] rust: dma: add DMA addressing capabilities
  2025-07-16 17:32   ` Daniel Almeida
@ 2025-07-16 17:41     ` Daniel Almeida
  2025-07-16 17:55     ` Danilo Krummrich
  1 sibling, 0 replies; 16+ messages in thread
From: Daniel Almeida @ 2025-07-16 17:41 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: abdiel.janulgue, robin.murphy, a.hindborg, ojeda, alex.gaynor,
	boqun.feng, gary, bjorn3_gh, lossin, aliceryhl, tmgross, bhelgaas,
	kwilczynski, gregkh, rafael, rust-for-linux, linux-pci,
	linux-kernel



> On 16 Jul 2025, at 14:32, Daniel Almeida <daniel.almeida@collabora.com> wrote:
> 
> Hi Danilo,
> 
>> +    #[inline]
>> +    pub const fn new(n: usize) -> Result<Self> {
>> +        Ok(Self(match n {
>> +            0 => 0,
>> +            1..=64 => u64::MAX >> (64 - n),
>> +            _ => return Err(EINVAL),
>> +        }))
>> +    }
>> +
> 
> Isn’t this equivalent to genmask_u64(0..=n) ? See [0].
> 
> You should also get a compile-time failure if n is out of bounds by default using
> genmask.
> 
> — Daniel
> 
> [0]: https://lore.kernel.org/rust-for-linux/20250714-topics-tyr-genmask2-v9-1-9e6422cbadb6@collabora.com/#r

Or genmask_u64(0..=n-1), if we disregard the previous off-by-one error

— Daniel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/5] rust: dma: add DMA addressing capabilities
  2025-07-16 17:32   ` Daniel Almeida
  2025-07-16 17:41     ` Daniel Almeida
@ 2025-07-16 17:55     ` Danilo Krummrich
  2025-07-16 18:32       ` Danilo Krummrich
  1 sibling, 1 reply; 16+ messages in thread
From: Danilo Krummrich @ 2025-07-16 17:55 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: abdiel.janulgue, robin.murphy, a.hindborg, ojeda, alex.gaynor,
	boqun.feng, gary, bjorn3_gh, lossin, aliceryhl, tmgross, bhelgaas,
	kwilczynski, gregkh, rafael, rust-for-linux, linux-pci,
	linux-kernel

On Wed Jul 16, 2025 at 7:32 PM CEST, Daniel Almeida wrote:
> Hi Danilo,
>
>> +    #[inline]
>> +    pub const fn new(n: usize) -> Result<Self> {
>> +        Ok(Self(match n {
>> +            0 => 0,
>> +            1..=64 => u64::MAX >> (64 - n),
>> +            _ => return Err(EINVAL),
>> +        }))
>> +    }
>> +
>
> Isn’t this equivalent to genmask_u64(0..=n) ? See [0].

Instead of the match this can use genmask_checked_u64() and convert the Option
to a Result, once genmask is upstream.

> You should also get a compile-time failure if n is out of bounds by default using
> genmask.

No, we can't use genmask_u64(), `n` is not guaranteed to be known at compile
time, so we'd need to use genmask_checked_u64().

Of course, we could have a separate DmaMask constructor, e.g. with a const
generic -- not sure that's worth though.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/5] rust: dma: add DMA addressing capabilities
  2025-07-16 17:55     ` Danilo Krummrich
@ 2025-07-16 18:32       ` Danilo Krummrich
  2025-07-16 21:13         ` Danilo Krummrich
  0 siblings, 1 reply; 16+ messages in thread
From: Danilo Krummrich @ 2025-07-16 18:32 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: abdiel.janulgue, robin.murphy, a.hindborg, ojeda, alex.gaynor,
	boqun.feng, gary, bjorn3_gh, lossin, aliceryhl, tmgross, bhelgaas,
	kwilczynski, gregkh, rafael, rust-for-linux, linux-pci,
	linux-kernel

On Wed Jul 16, 2025 at 7:55 PM CEST, Danilo Krummrich wrote:
> On Wed Jul 16, 2025 at 7:32 PM CEST, Daniel Almeida wrote:
>> Hi Danilo,
>>
>>> +    #[inline]
>>> +    pub const fn new(n: usize) -> Result<Self> {
>>> +        Ok(Self(match n {
>>> +            0 => 0,
>>> +            1..=64 => u64::MAX >> (64 - n),
>>> +            _ => return Err(EINVAL),
>>> +        }))
>>> +    }
>>> +
>>
>> Isn’t this equivalent to genmask_u64(0..=n) ? See [0].
>
> Instead of the match this can use genmask_checked_u64() and convert the Option
> to a Result, once genmask is upstream.
>
>> You should also get a compile-time failure if n is out of bounds by default using
>> genmask.
>
> No, we can't use genmask_u64(), `n` is not guaranteed to be known at compile
> time, so we'd need to use genmask_checked_u64().
>
> Of course, we could have a separate DmaMask constructor, e.g. with a const
> generic -- not sure that's worth though.

On the other hand, it doesn't hurt. Guess I will add another constructor with a
const generic. :)

I also quickly tried genmask and I have a few questions:

  (1) Why does genmask not use a const generic? I think this makes it more
      obvious that it's only intended to be used from const context.

  (2) Why is there no build_assert() when the range exceeds the number of bits
      of the target type? I would expect genmask_u64(0..100) to fail.

  (3) OOC, why did you choose u32 as argument type?

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/5] rust: dma: add DMA addressing capabilities
  2025-07-16 18:32       ` Danilo Krummrich
@ 2025-07-16 21:13         ` Danilo Krummrich
  2025-07-16 22:19           ` Daniel Almeida
  0 siblings, 1 reply; 16+ messages in thread
From: Danilo Krummrich @ 2025-07-16 21:13 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: abdiel.janulgue, robin.murphy, a.hindborg, ojeda, alex.gaynor,
	boqun.feng, gary, bjorn3_gh, lossin, aliceryhl, tmgross, bhelgaas,
	kwilczynski, gregkh, rafael, rust-for-linux, linux-pci,
	linux-kernel

On Wed Jul 16, 2025 at 8:32 PM CEST, Danilo Krummrich wrote:
> On Wed Jul 16, 2025 at 7:55 PM CEST, Danilo Krummrich wrote:
>> On Wed Jul 16, 2025 at 7:32 PM CEST, Daniel Almeida wrote:
>>> Hi Danilo,
>>>
>>>> +    #[inline]
>>>> +    pub const fn new(n: usize) -> Result<Self> {
>>>> +        Ok(Self(match n {
>>>> +            0 => 0,
>>>> +            1..=64 => u64::MAX >> (64 - n),
>>>> +            _ => return Err(EINVAL),
>>>> +        }))
>>>> +    }
>>>> +
>>>
>>> Isn’t this equivalent to genmask_u64(0..=n) ? See [0].
>>
>> Instead of the match this can use genmask_checked_u64() and convert the Option
>> to a Result, once genmask is upstream.
>>
>>> You should also get a compile-time failure if n is out of bounds by default using
>>> genmask.
>>
>> No, we can't use genmask_u64(), `n` is not guaranteed to be known at compile
>> time, so we'd need to use genmask_checked_u64().
>>
>> Of course, we could have a separate DmaMask constructor, e.g. with a const
>> generic -- not sure that's worth though.
>
> On the other hand, it doesn't hurt. Guess I will add another constructor with a
> const generic. :)

diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index afd3ba538e3c..ad69ef316295 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -81,25 +81,6 @@ unsafe fn dma_set_mask_and_coherent(&self, mask: DmaMask) -> Result {
 /// never exceed the bit width of `u64`.
 ///
 /// This is the Rust equivalent of the C macro `DMA_BIT_MASK()`.
-///
-/// # Examples
-///
-/// ```
-/// use kernel::dma::DmaMask;
-///
-/// let mask0 = DmaMask::new(0)?;
-/// assert_eq!(mask0.value(), 0);
-///
-/// let mask1 = DmaMask::new(1)?;
-/// assert_eq!(mask1.value(), 0b1);
-///
-/// let mask64 = DmaMask::new(64)?;
-/// assert_eq!(mask64.value(), u64::MAX);
-///
-/// let mask_overflow = DmaMask::new(100);
-/// assert!(mask_overflow.is_err());
-/// # Ok::<(), Error>(())
-/// ```
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub struct DmaMask(u64);

@@ -108,8 +89,27 @@ impl DmaMask {
     ///
     /// For `n <= 64`, sets exactly the lowest `n` bits.
     /// For `n > 64`, returns [`EINVAL`].
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::dma::DmaMask;
+    ///
+    /// let mask0 = DmaMask::try_new(0)?;
+    /// assert_eq!(mask0.value(), 0);
+    ///
+    /// let mask1 = DmaMask::try_new(1)?;
+    /// assert_eq!(mask1.value(), 0b1);
+    ///
+    /// let mask64 = DmaMask::try_new(64)?;
+    /// assert_eq!(mask64.value(), u64::MAX);
+    ///
+    /// let mask_overflow = DmaMask::try_new(100);
+    /// assert!(mask_overflow.is_err());
+    /// # Ok::<(), Error>(())
+    /// ```
     #[inline]
-    pub const fn new(n: usize) -> Result<Self> {
+    pub const fn try_new(n: u32) -> Result<Self> {
         Ok(Self(match n {
             0 => 0,
             1..=64 => u64::MAX >> (64 - n),
@@ -117,6 +117,38 @@ pub const fn new(n: usize) -> Result<Self> {
         }))
     }

+    /// Constructs a `DmaMask` with the lowest `n` bits set to `1`.
+    ///
+    /// For `n <= 64`, sets exactly the lowest `n` bits.
+    /// For `n > 64`, results in a build error.
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::dma::DmaMask;
+    /// use kernel::bits::genmask_u64;
+    ///
+    /// let mask0 = DmaMask::new::<0>();
+    /// assert_eq!(mask0.value(), 0);
+    ///
+    /// let mask1 = DmaMask::new::<1>();
+    /// assert_eq!(mask1.value(), 0b1);
+    ///
+    /// let mask64 = DmaMask::new::<64>();
+    /// assert_eq!(mask64.value(), u64::MAX);
+    ///
+    /// // Build failure.
+    /// // let mask_overflow = DmaMask::new::<100>();
+    /// ```
+    #[inline(always)]
+    pub const fn new<const N: u32>() -> Self {
+        let Ok(mask) = Self::try_new(N) else {
+            build_error!("Invalid DMA Mask.");
+        };
+
+        mask
+    }
+
     /// Returns the underlying `u64` bitmask value.
     #[inline]
     pub const fn value(&self) -> u64 {
diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
index 9422ac68c139..c5e7cce68654 100644
--- a/samples/rust/rust_dma.rs
+++ b/samples/rust/rust_dma.rs
@@ -58,7 +58,7 @@ 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");

-        let mask = DmaMask::new(64)?;
+        let mask = DmaMask::new::<64>();

         // SAFETY: There are no concurrent calls to DMA allocation and mapping primitives.
         unsafe { pdev.dma_set_mask_and_coherent(mask)? };


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/5] rust: dma: add DMA addressing capabilities
  2025-07-16 21:13         ` Danilo Krummrich
@ 2025-07-16 22:19           ` Daniel Almeida
  2025-07-16 22:32             ` Danilo Krummrich
  0 siblings, 1 reply; 16+ messages in thread
From: Daniel Almeida @ 2025-07-16 22:19 UTC (permalink / raw)
  To: Danilo Krummrich
  Cc: abdiel.janulgue, robin.murphy, a.hindborg, ojeda, alex.gaynor,
	boqun.feng, gary, bjorn3_gh, lossin, aliceryhl, tmgross, bhelgaas,
	kwilczynski, gregkh, rafael, rust-for-linux, linux-pci,
	linux-kernel

For some reason this diff did not apply on top of your latest commit (corrupt
patch on line 6)

> 
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index afd3ba538e3c..ad69ef316295 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
> @@ -81,25 +81,6 @@ unsafe fn dma_set_mask_and_coherent(&self, mask: DmaMask) -> Result {
> /// never exceed the bit width of `u64`.
> ///
> /// This is the Rust equivalent of the C macro `DMA_BIT_MASK()`.
> -///
> -/// # Examples
> -///
> -/// ```
> -/// use kernel::dma::DmaMask;
> -///
> -/// let mask0 = DmaMask::new(0)?;
> -/// assert_eq!(mask0.value(), 0);
> -///
> -/// let mask1 = DmaMask::new(1)?;
> -/// assert_eq!(mask1.value(), 0b1);
> -///
> -/// let mask64 = DmaMask::new(64)?;
> -/// assert_eq!(mask64.value(), u64::MAX);
> -///
> -/// let mask_overflow = DmaMask::new(100);
> -/// assert!(mask_overflow.is_err());
> -/// # Ok::<(), Error>(())
> -/// ```
> #[derive(Debug, Clone, Copy, PartialEq, Eq)]
> pub struct DmaMask(u64);
> 
> @@ -108,8 +89,27 @@ impl DmaMask {
>     ///
>     /// For `n <= 64`, sets exactly the lowest `n` bits.
>     /// For `n > 64`, returns [`EINVAL`].
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::dma::DmaMask;
> +    ///
> +    /// let mask0 = DmaMask::try_new(0)?;
> +    /// assert_eq!(mask0.value(), 0);
> +    ///
> +    /// let mask1 = DmaMask::try_new(1)?;
> +    /// assert_eq!(mask1.value(), 0b1);
> +    ///
> +    /// let mask64 = DmaMask::try_new(64)?;
> +    /// assert_eq!(mask64.value(), u64::MAX);
> +    ///
> +    /// let mask_overflow = DmaMask::try_new(100);
> +    /// assert!(mask_overflow.is_err());
> +    /// # Ok::<(), Error>(())
> +    /// ```
>     #[inline]
> -    pub const fn new(n: usize) -> Result<Self> {
> +    pub const fn try_new(n: u32) -> Result<Self> {
>         Ok(Self(match n {
>             0 => 0,
>             1..=64 => u64::MAX >> (64 - n),
> @@ -117,6 +117,38 @@ pub const fn new(n: usize) -> Result<Self> {
>         }))
>     }
> 
> +    /// Constructs a `DmaMask` with the lowest `n` bits set to `1`.
> +    ///
> +    /// For `n <= 64`, sets exactly the lowest `n` bits.
> +    /// For `n > 64`, results in a build error.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::dma::DmaMask;
> +    /// use kernel::bits::genmask_u64;

There is also this, which is not upstream (nor used in the patch)

> +    ///
> +    /// let mask0 = DmaMask::new::<0>();
> +    /// assert_eq!(mask0.value(), 0);
> +    ///
> +    /// let mask1 = DmaMask::new::<1>();
> +    /// assert_eq!(mask1.value(), 0b1);
> +    ///
> +    /// let mask64 = DmaMask::new::<64>();
> +    /// assert_eq!(mask64.value(), u64::MAX);
> +    ///
> +    /// // Build failure.
> +    /// // let mask_overflow = DmaMask::new::<100>();
> +    /// ```
> +    #[inline(always)]
> +    pub const fn new<const N: u32>() -> Self {
> +        let Ok(mask) = Self::try_new(N) else {
> +            build_error!("Invalid DMA Mask.");
> +        };
> +
> +        mask
> +    }
> +
>     /// Returns the underlying `u64` bitmask value.
>     #[inline]
>     pub const fn value(&self) -> u64 {
> diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
> index 9422ac68c139..c5e7cce68654 100644
> --- a/samples/rust/rust_dma.rs
> +++ b/samples/rust/rust_dma.rs
> @@ -58,7 +58,7 @@ 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");
> 
> -        let mask = DmaMask::new(64)?;
> +        let mask = DmaMask::new::<64>();
> 
>         // SAFETY: There are no concurrent calls to DMA allocation and mapping primitives.
>         unsafe { pdev.dma_set_mask_and_coherent(mask)? };
> 
> 

I hand-applied this and fixed the issue above. All doctests pass FYI.

— Daniel



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 2/5] rust: dma: add DMA addressing capabilities
  2025-07-16 22:19           ` Daniel Almeida
@ 2025-07-16 22:32             ` Danilo Krummrich
  0 siblings, 0 replies; 16+ messages in thread
From: Danilo Krummrich @ 2025-07-16 22:32 UTC (permalink / raw)
  To: Daniel Almeida
  Cc: abdiel.janulgue, robin.murphy, a.hindborg, ojeda, alex.gaynor,
	boqun.feng, gary, bjorn3_gh, lossin, aliceryhl, tmgross, bhelgaas,
	kwilczynski, gregkh, rafael, rust-for-linux, linux-pci,
	linux-kernel

On Thu Jul 17, 2025 at 12:19 AM CEST, Daniel Almeida wrote:
> I hand-applied this and fixed the issue above. All doctests pass FYI.

Yes, I noticed this in my testing as well, yet it seems I somehow managed to
post the wrong diff. In my branch [1] everything should be correct.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=rust/dma-mask

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 0/5] dma::Device trait and DMA mask
  2025-07-16 15:02 [PATCH v2 0/5] dma::Device trait and DMA mask Danilo Krummrich
                   ` (6 preceding siblings ...)
  2025-07-16 17:34 ` Daniel Almeida
@ 2025-07-20 14:33 ` Danilo Krummrich
  7 siblings, 0 replies; 16+ messages in thread
From: Danilo Krummrich @ 2025-07-20 14:33 UTC (permalink / raw)
  To: abdiel.janulgue, daniel.almeida, robin.murphy, a.hindborg, ojeda,
	alex.gaynor, boqun.feng, gary, bjorn3_gh, lossin, aliceryhl,
	tmgross, bhelgaas, kwilczynski, gregkh, rafael
  Cc: rust-for-linux, linux-pci, linux-kernel

On Wed Jul 16, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
> This patch series adds the dma::Device trait to be implemented by bus devices on
> DMA capable busses.

Applied to driver-core-testing, thanks!

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2025-07-20 14:33 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-16 15:02 [PATCH v2 0/5] dma::Device trait and DMA mask Danilo Krummrich
2025-07-16 15:02 ` [PATCH v2 1/5] rust: dma: implement `dma::Device` trait Danilo Krummrich
2025-07-16 15:02 ` [PATCH v2 2/5] rust: dma: add DMA addressing capabilities Danilo Krummrich
2025-07-16 17:32   ` Daniel Almeida
2025-07-16 17:41     ` Daniel Almeida
2025-07-16 17:55     ` Danilo Krummrich
2025-07-16 18:32       ` Danilo Krummrich
2025-07-16 21:13         ` Danilo Krummrich
2025-07-16 22:19           ` Daniel Almeida
2025-07-16 22:32             ` Danilo Krummrich
2025-07-16 15:02 ` [PATCH v2 3/5] rust: pci: implement the `dma::Device` trait Danilo Krummrich
2025-07-16 15:02 ` [PATCH v2 4/5] rust: platform: " Danilo Krummrich
2025-07-16 15:02 ` [PATCH v2 5/5] rust: samples: dma: set DMA mask Danilo Krummrich
2025-07-16 17:28 ` [PATCH v2 0/5] dma::Device trait and " Greg KH
2025-07-16 17:34 ` Daniel Almeida
2025-07-20 14:33 ` Danilo Krummrich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).