[PATCH 0/4] Rust infrastructure for sg_table and scatterlist

[PATCH 0/4] Rust infrastructure for sg_table and scatterlist

[Danilo Krummrich]

This patch series provides abstractions for struct sg_table and struct
scatterlist.

Abdiel and me agreed for me to take over his previous iterations on this topic.
I decided to send my patches as a new series rather than as a subsequent version
of Abdiel's previous iterations, since the changes I made turned out to be much
closer to a full rewrite.

The most notable differences in design are:

  - SGTable utilizes BorrowedPage, AsPageIter and VmallocPageIter from my patch
    series in [1].

  -  SGTable is a transparent wrapper over either struct Owned<P> (where P is
     the provider of the backing pages) or struct Borrowed, which by itself is a
     transparent wrapper over Opaque<bindings::sg_table>, i.e. either
     SGTable<Owned<P>> or just SGTable (which is equivalent to
     SGTable<Borrowed>.

     - `SGTable<Owned<P>>`: Represents a table whose resources are fully managed
       by Rust. It takes ownership of a page provider `P`, allocates the
       underlying `struct sg_table`, maps it for DMA, and handles all cleanup
       automatically upon drop. The DMA mapping's lifetime is tied to the
       associated device using `Devres`, ensuring it is correctly unmapped
       before the device is unbound.

     - `SGTable<Borrowed>` (or just `SGTable`): A zero-cost representation of an
       externally managed `struct sg_table`. It is created from a raw pointer
       using `SGTable::as_ref()` and provides a lifetime-bound reference
       (`&'a SGTable`) for operations like iteration.

     - As a consequence, a borrowed SG table can be created with
       SGTable::as_ref(), which returns a &'a SGTable, just like similar
       existing abstractions.

       An owned SGTable is created with SGTable::new(), which returns an
       impl PinInit<SGTable<Owned<P>>, Error>, such that it can be initialized
       directly within existing private data memory allocations while providing
       the required pin guarantees.

  - SGTable<Owned<P>> uses an inner type Devres<DmaMapSgt> to ensure that the
    DMA mapping can't out-live device unbind.

  - SGTable<Owned<P>> uses pin-init for initialization.

This patch series depends on [1] (branch containing the patches in [2]). A
branch containing this series (including dependencies) can be found in [3];
Abdiel's latest series can be found in [4].

[1] https://lore.kernel.org/rust-for-linux/20250814093427.19629-1-dakr@kernel.org/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=page-iter
[3] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=scatterlist
[4] https://lore.kernel.org/lkml/20250718103359.1026240-1-abdiel.janulgue@gmail.com/

Danilo Krummrich (4):
  rust: dma: implement DataDirection
  rust: scatterlist: Add type-state abstraction for sg_table
  samples: rust: dma: add sample code for SGTable
  MAINTAINERS: rust: dma: add scatterlist files

 MAINTAINERS                     |   4 +-
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/scatterlist.c      |  24 ++
 rust/kernel/dma.rs              |  41 +++
 rust/kernel/lib.rs              |   1 +
 rust/kernel/scatterlist.rs      | 433 ++++++++++++++++++++++++++++++++
 samples/rust/rust_dma.rs        |  35 ++-
 8 files changed, 530 insertions(+), 10 deletions(-)
 create mode 100644 rust/helpers/scatterlist.c
 create mode 100644 rust/kernel/scatterlist.rs


base-commit: a66548bf306b60441ed2ea6b034a0cd69f464e74
-- 
2.50.1

[PATCH 1/4] rust: dma: implement DataDirection

[Danilo Krummrich]

Add the `DataDirection` struct, a newtype wrapper around the C
`enum dma_data_direction`.

This provides a type-safe Rust interface for specifying the direction of
DMA transfers.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/bindings/bindings_helper.h |  1 +
 rust/kernel/dma.rs              | 41 +++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 0e140e07758b..c2cc52ee9945 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -47,6 +47,7 @@
 #include <linux/cpumask.h>
 #include <linux/cred.h>
 #include <linux/device/faux.h>
+#include <linux/dma-direction.h>
 #include <linux/dma-mapping.h>
 #include <linux/errname.h>
 #include <linux/ethtool.h>
diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
index 2bc8ab51ec28..b0950a2768a5 100644
--- a/rust/kernel/dma.rs
+++ b/rust/kernel/dma.rs
@@ -244,6 +244,47 @@ pub mod attrs {
     pub const DMA_ATTR_PRIVILEGED: Attrs = Attrs(bindings::DMA_ATTR_PRIVILEGED);
 }
 
+/// DMA data direction.
+///
+/// Corresponds to the C [`enum dma_data_direction`].
+///
+/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
+#[derive(Copy, Clone, PartialEq, Eq)]
+pub struct DataDirection(bindings::dma_data_direction);
+
+impl DataDirection {
+    /// The DMA mapping is for bidirectional data transfer.
+    ///
+    /// This is used when the buffer can be both read from and written to by the device.
+    /// The cache for the corresponding memory region is both flushed and invalidated.
+    pub const BIDIRECTIONAL: DataDirection =
+        DataDirection(bindings::dma_data_direction_DMA_BIDIRECTIONAL);
+
+    /// The DMA mapping is for data transfer from memory to the device (write).
+    ///
+    /// The CPU has prepared data in the buffer, and the device will read it.
+    /// The cache for the corresponding memory region is flushed.
+    pub const TO_DEVICE: DataDirection = DataDirection(bindings::dma_data_direction_DMA_TO_DEVICE);
+
+    /// The DMA mapping is for data transfer from the device to memory (read).
+    ///
+    /// The device will write data into the buffer for the CPU to read.
+    /// The cache for the corresponding memory region is invalidated before CPU access.
+    pub const FROM_DEVICE: DataDirection =
+        DataDirection(bindings::dma_data_direction_DMA_FROM_DEVICE);
+
+    /// The DMA mapping is not for data transfer.
+    ///
+    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
+    /// will not perform any cache coherency operations.
+    pub const NONE: DataDirection = DataDirection(bindings::dma_data_direction_DMA_NONE);
+
+    /// Returns the raw representation of [`enum dma_data_direction`].
+    pub fn as_raw(self) -> bindings::dma_data_direction {
+        self.0
+    }
+}
+
 /// An abstraction of the `dma_alloc_coherent` API.
 ///
 /// This is an abstraction around the `dma_alloc_coherent` API which is used to allocate and map
-- 
2.50.1

[PATCH 2/4] rust: scatterlist: Add type-state abstraction for sg_table

[Danilo Krummrich]

Add a safe Rust abstraction for the kernel's scatter-gather list
facilities (`struct scatterlist` and `struct sg_table`).

This commit introduces `SGTable<T>`, a wrapper that uses a type-state
pattern to provide compile-time guarantees about ownership and lifetime.

The abstraction provides two primary states:
- `SGTable<Owned<P>>`: Represents a table whose resources are fully
  managed by Rust. It takes ownership of a page provider `P`, allocates
  the underlying `struct sg_table`, maps it for DMA, and handles all
  cleanup automatically upon drop. The DMA mapping's lifetime is tied to
  the associated device using `Devres`, ensuring it is correctly unmapped
  before the device is unbound.
- `SGTable<Borrowed>` (or just `SGTable`): A zero-cost representation of
  an externally managed `struct sg_table`. It is created from a raw
  pointer using `SGTable::as_ref()` and provides a lifetime-bound
  reference (`&'a SGTable`) for operations like iteration.

The API exposes a safe iterator that yields `&SGEntry` references,
allowing drivers to easily access the DMA address and length of each
segment in the list.

Co-developed-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/helpers/helpers.c     |   1 +
 rust/helpers/scatterlist.c |  24 ++
 rust/kernel/lib.rs         |   1 +
 rust/kernel/scatterlist.rs | 433 +++++++++++++++++++++++++++++++++++++
 4 files changed, 459 insertions(+)
 create mode 100644 rust/helpers/scatterlist.c
 create mode 100644 rust/kernel/scatterlist.rs

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 7cf7fe95e41d..e94542bf6ea7 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -39,6 +39,7 @@
 #include "rcu.c"
 #include "refcount.c"
 #include "regulator.c"
+#include "scatterlist.c"
 #include "security.c"
 #include "signal.c"
 #include "slab.c"
diff --git a/rust/helpers/scatterlist.c b/rust/helpers/scatterlist.c
new file mode 100644
index 000000000000..80c956ee09ab
--- /dev/null
+++ b/rust/helpers/scatterlist.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/dma-direction.h>
+
+dma_addr_t rust_helper_sg_dma_address(struct scatterlist *sg)
+{
+	return sg_dma_address(sg);
+}
+
+unsigned int rust_helper_sg_dma_len(struct scatterlist *sg)
+{
+	return sg_dma_len(sg);
+}
+
+struct scatterlist *rust_helper_sg_next(struct scatterlist *sg)
+{
+	return sg_next(sg);
+}
+
+void rust_helper_dma_unmap_sgtable(struct device *dev, struct sg_table *sgt,
+				   enum dma_data_direction dir, unsigned long attrs)
+{
+	return dma_unmap_sgtable(dev, sgt, dir, attrs);
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ed53169e795c..55acbc893736 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -113,6 +113,7 @@
 pub mod rbtree;
 pub mod regulator;
 pub mod revocable;
+pub mod scatterlist;
 pub mod security;
 pub mod seq_file;
 pub mod sizes;
diff --git a/rust/kernel/scatterlist.rs b/rust/kernel/scatterlist.rs
new file mode 100644
index 000000000000..4caaf8cfbf83
--- /dev/null
+++ b/rust/kernel/scatterlist.rs
@@ -0,0 +1,433 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstractions for scatter-gather lists.
+//!
+//! C header: [`include/linux/scatterlist.h`](srctree/include/linux/scatterlist.h)
+//!
+//! Scatter-gather (SG) I/O is a memory access technique that allows devices to perform DMA
+//! operations on data buffers that are not physically contiguous in memory. It works by creating a
+//! "scatter-gather list", an array where each entry specifies the address and length of a
+//! physically contiguous memory segment.
+//!
+//! The device's DMA controller can then read this list and process the segments sequentially as
+//! part of one logical I/O request. This avoids the need for a single, large, physically contiguous
+//! memory buffer, which can be difficult or impossible to allocate.
+//!
+//! This module provides safe Rust abstractions over the kernel's `struct scatterlist` and
+//! `struct sg_table` types.
+//!
+//! The main entry point is the [`SGTable`] type, which represents a complete scatter-gather table.
+//! It can be either:
+//!
+//! - An owned table ([`SGTable<Owned<P>>`]), created from a Rust memory buffer (e.g., [`VVec`]).
+//!   This type manages the allocation of the `struct sg_table`, the DMA mapping of the buffer, and
+//!   the automatic cleanup of all resources.
+//! - A borrowed reference (&[`SGTable`]), which provides safe, read-only access to a table that was
+//!   allocated by other (e.g., C) code.
+//!
+//! Individual entries in the table are represented by [`SGEntry`], which can be accessed by
+//! iterating over an [`SGTable`].
+
+use crate::{
+    alloc,
+    alloc::allocator::VmallocPageIter,
+    bindings,
+    device::{Bound, Device},
+    devres::Devres,
+    dma, error, page,
+    prelude::*,
+    types::{ARef, Opaque},
+};
+use core::{ops::Deref, ptr::NonNull};
+
+/// A single entry in a scatter-gather list.
+///
+/// An `SGEntry` represents a single, physically contiguous segment of memory that has been mapped
+/// for DMA.
+///
+/// Instances of this struct are obtained by iterating over an [`SGTable`]. Drivers do not create
+/// or own [`SGEntry`] objects directly.
+#[repr(transparent)]
+pub struct SGEntry(Opaque<bindings::scatterlist>);
+
+impl SGEntry {
+    /// Convert a raw `struct scatterlist *` to a `&'a SGEntry`.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that the `struct scatterlist` pointed to by `ptr` is valid for the
+    /// lifetime `'a`.
+    unsafe fn as_ref<'a>(ptr: *mut bindings::scatterlist) -> &'a Self {
+        // SAFETY: The safety requirements of this function guarantee that `ptr` is a valid pointer
+        // to a `struct scatterlist` for the duration of `'a`.
+        unsafe { &*ptr.cast() }
+    }
+
+    /// Obtain the raw `struct scatterlist *`.
+    fn as_raw(&self) -> *mut bindings::scatterlist {
+        self.0.get()
+    }
+
+    /// Returns the DMA address of this SG entry.
+    ///
+    /// This is the address that the device should use to access the memory segment.
+    pub fn dma_address(&self) -> bindings::dma_addr_t {
+        // SAFETY: `self.as_raw()` is a valid pointer to a `struct scatterlist`.
+        unsafe { bindings::sg_dma_address(self.as_raw()) }
+    }
+
+    /// Returns the length of this SG entry in bytes.
+    pub fn dma_len(&self) -> u32 {
+        // SAFETY: `self.as_raw()` is a valid pointer to a `struct scatterlist`.
+        unsafe { bindings::sg_dma_len(self.as_raw()) }
+    }
+}
+
+/// The borrowed type state of an [`SGTable`], representing a borrowed or externally managed table.
+#[repr(transparent)]
+pub struct Borrowed(Opaque<bindings::sg_table>);
+
+// SAFETY: An instance of `Borrowed` can be send to any task.
+unsafe impl Send for Borrowed {}
+
+/// A scatter-gather table.
+///
+/// This struct is a wrapper around the kernel's `struct sg_table`. It manages a list of DMA-mapped
+/// memory segments that can be passed to a device for I/O operations.
+///
+/// The generic parameter `T` is used as a type state to distinguish between owned and borrowed
+/// tables.
+///
+///  - [`SGTable<Owned>`]: An owned table created and managed entirely by Rust code. It handles
+///    allocation, DMA mapping, and cleanup of all associated resources. See [`SGTable::new`].
+///  - [`SGTable<Borrowed>`} (or simply [`SGTable`]): Represents a table whose lifetime is managed
+///    externally. It can be used safely via a borrowed reference `&'a SGTable`, where `'a` is the
+///    external lifetime.
+///
+/// All [`SGTable`] variants can be iterated over the individual [`SGEntry`]s.
+#[repr(transparent)]
+#[pin_data]
+pub struct SGTable<T: private::Sealed = Borrowed> {
+    #[pin]
+    inner: T,
+}
+
+impl SGTable {
+    /// Creates a borrowed `&'a SGTable` from a raw `struct sg_table` pointer.
+    ///
+    /// This allows safe access to an `sg_table` that is managed elsewhere (for example, in C code).
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that the `struct sg_table` pointed to by `ptr` is valid for the entire
+    /// lifetime of `'a`.
+    pub unsafe fn as_ref<'a>(ptr: *mut bindings::sg_table) -> &'a Self {
+        // SAFETY: The safety requirements of this function guarantee that `ptr` is a valid pointer
+        // to a `struct sg_table` for the duration of `'a`.
+        unsafe { &*ptr.cast() }
+    }
+
+    fn as_raw(&self) -> *mut bindings::sg_table {
+        self.inner.0.get()
+    }
+
+    fn as_iter(&self) -> SGTableIter<'_> {
+        // SAFETY: `self.as_raw()` is a valid pointer to a `struct sg_table`.
+        let ptr = unsafe { (*self.as_raw()).sgl };
+
+        // SAFETY: `ptr` is guaranteed to be a valid pointer to a `struct scatterlist`.
+        let pos = Some(unsafe { SGEntry::as_ref(ptr) });
+
+        SGTableIter { pos }
+    }
+}
+
+/// # Invariants
+///
+/// `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].
+struct DmaMapSgt {
+    sgt: NonNull<bindings::sg_table>,
+    dev: ARef<Device>,
+    dir: dma::DataDirection,
+}
+
+// SAFETY: An instance of `DmaMapSgt` can be send to any task.
+unsafe impl Send for DmaMapSgt {}
+
+impl DmaMapSgt {
+    /// # Safety
+    ///
+    /// `sgt` must be a valid pointer to a `struct sg_table` for the entire lifetime of the
+    /// returned [`DmaMapSgt`].
+    unsafe fn new(
+        sgt: NonNull<bindings::sg_table>,
+        dev: &Device<Bound>,
+        dir: dma::DataDirection,
+    ) -> Result<Self> {
+        // SAFETY:
+        // - `dev.as_raw()` is a valid pointer to a `struct device`, which is guaranteed to be
+        //   bound to a driver for the duration of this call.
+        // - `sgt` is a valid pointer to a `struct sg_table`.
+        error::to_result(unsafe {
+            bindings::dma_map_sgtable(dev.as_raw(), sgt.as_ptr(), dir.as_raw(), 0)
+        })?;
+
+        // INVARIANT: By the safety requirements of this function it is guaranteed that `sgt` is
+        // valid for the entire lifetime of this object instance.
+        Ok(Self {
+            sgt,
+            dev: dev.into(),
+            dir,
+        })
+    }
+}
+
+impl Drop for DmaMapSgt {
+    fn drop(&mut self) {
+        // SAFETY:
+        // - `self.dev.as_raw()` is a pointer to a valid `struct device`.
+        // - `self.dev` is the same device the mapping has been created for in `Self::new()`.
+        // - `self.sgt.as_ptr()` is a valid pointer to a `struct sg_table` by the type invariants
+        //   of `Self`.
+        // - `self.dir` is the same `dma::DataDirection` the mapping has been created with in
+        //   `Self::new()`.
+        unsafe {
+            bindings::dma_unmap_sgtable(self.dev.as_raw(), self.sgt.as_ptr(), self.dir.as_raw(), 0)
+        };
+    }
+}
+
+#[repr(transparent)]
+#[pin_data(PinnedDrop)]
+struct RawSGTable {
+    #[pin]
+    sgt: Opaque<bindings::sg_table>,
+}
+
+impl RawSGTable {
+    fn new(
+        mut pages: KVec<*mut bindings::page>,
+        size: usize,
+        max_segment: u32,
+        flags: alloc::Flags,
+    ) -> impl PinInit<Self, Error> {
+        try_pin_init!(Self {
+            sgt <- Opaque::try_ffi_init(|slot: *mut bindings::sg_table| {
+                // `sg_alloc_table_from_pages_segment()` expects at least one page, otherwise it
+                // produces a NPE.
+                if pages.is_empty() {
+                    return Err(EINVAL);
+                }
+
+                // SAFETY:
+                // - `slot` is a valid pointer to uninitialized memory.
+                // - As by the check above, `pages` is not empty.
+                error::to_result(unsafe {
+                    bindings::sg_alloc_table_from_pages_segment(
+                        slot,
+                        pages.as_mut_ptr(),
+                        pages.len().try_into()?,
+                        0,
+                        size,
+                        max_segment,
+                        flags.as_raw(),
+                    )
+                })
+            }),
+        })
+    }
+
+    fn as_raw(&self) -> *mut bindings::sg_table {
+        self.sgt.get()
+    }
+}
+
+#[pinned_drop]
+impl PinnedDrop for RawSGTable {
+    fn drop(self: Pin<&mut Self>) {
+        // SAFETY: `sgt` is a valid and initialized `struct sg_table`.
+        unsafe { bindings::sg_free_table(self.sgt.get()) };
+    }
+}
+
+/// The [`Owned`] type state of an [`SGTable`].
+///
+/// A [`SGTable<Owned>`] signifies that the [`SGTable`] owns all associated resources:
+///
+/// - The backing memory pages.
+/// - The `struct sg_table` allocation (`sgt`).
+/// - The DMA mapping, managed through a [`Devres`]-managed `DmaMapSgt`.
+///
+/// Users interact with this type through the [`SGTable`] handle and do not need to manage
+/// [`Owned`] directly.
+#[pin_data]
+pub struct Owned<P> {
+    // Note: The drop order is relevant; we first have to unmap the `struct sg_table`, then free the
+    // `struct sg_table` and finally free the backing pages.
+    #[pin]
+    dma: Devres<DmaMapSgt>,
+    #[pin]
+    sgt: RawSGTable,
+    _pages: P,
+}
+
+// SAFETY: An instance of `Owned` can be send to any task if `P` can be send to any task.
+unsafe impl<P: Send> Send for Owned<P> {}
+
+impl<P> Owned<P>
+where
+    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
+{
+    fn new(
+        dev: &Device<Bound>,
+        mut pages: P,
+        dir: dma::DataDirection,
+        flags: alloc::Flags,
+    ) -> Result<impl PinInit<Self, Error> + use<'_, P>> {
+        let page_iter = pages.page_iter();
+        let size = page_iter.size();
+
+        let mut page_vec: KVec<*mut bindings::page> =
+            KVec::with_capacity(page_iter.page_count(), flags)?;
+
+        for page in page_iter {
+            page_vec.push(page.as_ptr(), flags)?;
+        }
+
+        // `dma_max_mapping_size` returns `size_t`, but `sg_alloc_table_from_pages_segment()` takes
+        // an `unsigned int`.
+        let max_segment = {
+            // SAFETY: `dev.as_raw()` is a valid pointer to a `struct device`.
+            let size = unsafe { bindings::dma_max_mapping_size(dev.as_raw()) };
+            if size == 0 {
+                u32::MAX
+            } else {
+                size.min(u32::MAX as usize) as u32
+            }
+        };
+
+        Ok(try_pin_init!(&this in Self {
+            sgt <- RawSGTable::new(page_vec, size, max_segment, flags),
+            dma <- {
+                // SAFETY: `this` is a valid pointer to uninitialized memory.
+                let sgt = unsafe { &raw mut (*this.as_ptr()).sgt }.cast();
+
+                // SAFETY: `sgt` is guaranteed to be non-null.
+                let sgt = unsafe { NonNull::new_unchecked(sgt) };
+
+                // SAFETY: It is guaranteed that the object returned by `DmaMapSgt::new` won't
+                // out-live `sgt`.
+                Devres::new(dev, unsafe { DmaMapSgt::new(sgt, dev, dir) })
+            },
+            _pages: pages,
+        }))
+    }
+}
+
+impl<P> SGTable<Owned<P>>
+where
+    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
+{
+    /// Allocates a new scatter-gather table from the given pages and maps it for DMA.
+    ///
+    /// This constructor creates a new [`SGTable<Owned>`] that takes ownership of `P`.
+    /// It allocates a `struct sg_table`, populates it with entries corresponding to the physical
+    /// pages of `P`, and maps the table for DMA with the specified [`Device`] and
+    /// [`dma::DataDirection`].
+    ///
+    /// The DMA mapping is managed through [`Devres`], ensuring that the DMA mapping is unmapped
+    /// once the associated [`Device`] is unbound, or when the [`SGTable<Owned>`] is dropped.
+    ///
+    /// # Parameters
+    ///
+    /// * `dev`: The [`Device`] that will be performing the DMA.
+    /// * `pages`: The entity providing the backing pages. It must implement [`page::AsPageIter`].
+    ///   The ownership of this entity is moved into the new [`SGTable<Owned>`].
+    /// * `dir`: The [`dma::DataDirection`] of the DMA transfer.
+    /// * `flags`: Allocation flags for internal allocations (e.g., [`GFP_KERNEL`]).
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// use kernel::{
+    ///     device::{Bound, Device},
+    ///     dma, page,
+    ///     prelude::*,
+    ///     scatterlist::*,
+    /// };
+    ///
+    /// fn test(dev: &Device<Bound>) -> Result {
+    ///     let size = 4 * page::PAGE_SIZE;
+    ///     let pages = VVec::<u8>::with_capacity(size, GFP_KERNEL)?;
+    ///
+    ///     let sgt = KBox::pin_init(SGTable::new(
+    ///         dev,
+    ///         pages,
+    ///         dma::DataDirection::TO_DEVICE,
+    ///         GFP_KERNEL,
+    ///     ), GFP_KERNEL)?;
+    ///
+    ///     Ok(())
+    /// }
+    /// ```
+    pub fn new(
+        dev: &Device<Bound>,
+        pages: P,
+        dir: dma::DataDirection,
+        flags: alloc::Flags,
+    ) -> impl PinInit<Self, Error> + use<'_, P> {
+        try_pin_init!(Self {
+            inner <- Owned::new(dev, pages, dir, flags)?
+        })
+    }
+}
+
+impl<P> Deref for SGTable<Owned<P>> {
+    type Target = SGTable;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: `self.inner.sgt.as_raw()` is a valid pointer to a `struct sg_table` for the
+        // entire lifetime of `self`.
+        unsafe { SGTable::as_ref(self.inner.sgt.as_raw()) }
+    }
+}
+
+mod private {
+    pub trait Sealed {}
+
+    impl Sealed for super::Borrowed {}
+    impl<P> Sealed for super::Owned<P> {}
+}
+
+impl<'a> IntoIterator for &'a SGTable {
+    type Item = &'a SGEntry;
+    type IntoIter = SGTableIter<'a>;
+
+    fn into_iter(self) -> Self::IntoIter {
+        self.as_iter()
+    }
+}
+
+/// An [`Iterator`] over the [`SGEntry`] items of an [`SGTable`].
+pub struct SGTableIter<'a> {
+    pos: Option<&'a SGEntry>,
+}
+
+impl<'a> Iterator for SGTableIter<'a> {
+    type Item = &'a SGEntry;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        let entry = self.pos?;
+
+        // SAFETY: `entry.as_raw()` is a valid pointer to a `struct scatterlist`.
+        let next = unsafe { bindings::sg_next(entry.as_raw()) };
+
+        self.pos = (!next.is_null()).then(|| {
+            // SAFETY: If `next` is not NULL, `sg_next()` guarantees to return a valid pointer to
+            // the next `struct scatterlist`.
+            unsafe { SGEntry::as_ref(next) }
+        });
+
+        Some(entry)
+    }
+}
-- 
2.50.1

[PATCH 3/4] samples: rust: dma: add sample code for SGTable

[Danilo Krummrich]

Add sample code for allocating and mapping a scatter-gather table
(`SGTable`).

Co-developed-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 samples/rust/rust_dma.rs | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
index c5e7cce68654..cc82327db0c2 100644
--- a/samples/rust/rust_dma.rs
+++ b/samples/rust/rust_dma.rs
@@ -7,15 +7,19 @@
 use kernel::{
     bindings,
     device::Core,
-    dma::{CoherentAllocation, Device, DmaMask},
-    pci,
+    dma::{CoherentAllocation, DataDirection, Device, DmaMask},
+    page, pci,
     prelude::*,
+    scatterlist::{Owned, SGTable},
     types::ARef,
 };
 
+#[pin_data(PinnedDrop)]
 struct DmaSampleDriver {
     pdev: ARef<pci::Device>,
     ca: CoherentAllocation<MyStruct>,
+    #[pin]
+    sgt: SGTable<Owned<VVec<u8>>>,
 }
 
 const TEST_VALUES: [(u32, u32); 5] = [
@@ -70,21 +74,30 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self
             kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1))?;
         }
 
-        let drvdata = KBox::new(
-            Self {
+        let size = 4 * page::PAGE_SIZE;
+        let pages = VVec::with_capacity(size, GFP_KERNEL)?;
+
+        let sgt = SGTable::new(pdev.as_ref(), pages, DataDirection::TO_DEVICE, GFP_KERNEL);
+
+        let drvdata = KBox::pin_init(
+            try_pin_init!(Self {
                 pdev: pdev.into(),
                 ca,
-            },
+                sgt <- sgt,
+            }),
             GFP_KERNEL,
         )?;
 
-        Ok(drvdata.into())
+        Ok(drvdata)
     }
 }
 
-impl Drop for DmaSampleDriver {
-    fn drop(&mut self) {
-        dev_info!(self.pdev.as_ref(), "Unload DMA test driver.\n");
+#[pinned_drop]
+impl PinnedDrop for DmaSampleDriver {
+    fn drop(self: Pin<&mut Self>) {
+        let dev = self.pdev.as_ref();
+
+        dev_info!(dev, "Unload DMA test driver.\n");
 
         for (i, value) in TEST_VALUES.into_iter().enumerate() {
             let val0 = kernel::dma_read!(self.ca[i].h);
@@ -99,6 +112,10 @@ fn drop(&mut self) {
                 assert_eq!(val1, value.1);
             }
         }
+
+        for (i, entry) in self.sgt.into_iter().enumerate() {
+            dev_info!(dev, "Entry[{}]: DMA address: {:#x}", i, entry.dma_address());
+        }
     }
 }
 
-- 
2.50.1

[PATCH 4/4] MAINTAINERS: rust: dma: add scatterlist files

[Danilo Krummrich]

Rename the "DMA MAPPING HELPERS DEVICE DRIVER API [RUST]" maintainers
entry to "DMA MAPPING & SCATTERLIST API [RUST]" and add the
corresponding scatterlist files.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 MAINTAINERS | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index fe168477caa4..65f676b2c304 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7238,7 +7238,7 @@ F:	include/linux/dma-mapping.h
 F:	include/linux/swiotlb.h
 F:	kernel/dma/
 
-DMA MAPPING HELPERS DEVICE DRIVER API [RUST]
+DMA MAPPING & SCATTERLIST API [RUST]
 M:	Abdiel Janulgue <abdiel.janulgue@gmail.com>
 M:	Danilo Krummrich <dakr@kernel.org>
 R:	Daniel Almeida <daniel.almeida@collabora.com>
@@ -7249,7 +7249,9 @@ S:	Supported
 W:	https://rust-for-linux.com
 T:	git https://github.com/Rust-for-Linux/linux.git alloc-next
 F:	rust/helpers/dma.c
+F:	rust/helpers/scatterlist.c
 F:	rust/kernel/dma.rs
+F:	rust/kernel/scatterlist.rs
 F:	samples/rust/rust_dma.rs
 
 DMA-BUF HEAPS FRAMEWORK
-- 
2.50.1

Re: [PATCH 1/4] rust: dma: implement DataDirection

[Alice Ryhl]

On Fri, Aug 15, 2025 at 07:10:02PM +0200, Danilo Krummrich wrote:
> Add the `DataDirection` struct, a newtype wrapper around the C
> `enum dma_data_direction`.
> 
> This provides a type-safe Rust interface for specifying the direction of
> DMA transfers.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

> +/// DMA data direction.
> +///
> +/// Corresponds to the C [`enum dma_data_direction`].
> +///
> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
> +#[derive(Copy, Clone, PartialEq, Eq)]
> +pub struct DataDirection(bindings::dma_data_direction);

Perhaps this should be a real Rust enum so that you can do an exhaustive
match?

> +impl DataDirection {
> +    /// The DMA mapping is for bidirectional data transfer.
> +    ///
> +    /// This is used when the buffer can be both read from and written to by the device.
> +    /// The cache for the corresponding memory region is both flushed and invalidated.
> +    pub const BIDIRECTIONAL: DataDirection =
> +        DataDirection(bindings::dma_data_direction_DMA_BIDIRECTIONAL);
> +
> +    /// The DMA mapping is for data transfer from memory to the device (write).
> +    ///
> +    /// The CPU has prepared data in the buffer, and the device will read it.
> +    /// The cache for the corresponding memory region is flushed.
> +    pub const TO_DEVICE: DataDirection = DataDirection(bindings::dma_data_direction_DMA_TO_DEVICE);
> +
> +    /// The DMA mapping is for data transfer from the device to memory (read).
> +    ///
> +    /// The device will write data into the buffer for the CPU to read.
> +    /// The cache for the corresponding memory region is invalidated before CPU access.
> +    pub const FROM_DEVICE: DataDirection =
> +        DataDirection(bindings::dma_data_direction_DMA_FROM_DEVICE);
> +
> +    /// The DMA mapping is not for data transfer.
> +    ///
> +    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
> +    /// will not perform any cache coherency operations.
> +    pub const NONE: DataDirection = DataDirection(bindings::dma_data_direction_DMA_NONE);
> +
> +    /// Returns the raw representation of [`enum dma_data_direction`].
> +    pub fn as_raw(self) -> bindings::dma_data_direction {
> +        self.0
> +    }
> +}

Re: [PATCH 2/4] rust: scatterlist: Add type-state abstraction for sg_table

[Alice Ryhl]

On Fri, Aug 15, 2025 at 07:10:03PM +0200, Danilo Krummrich wrote:
> Add a safe Rust abstraction for the kernel's scatter-gather list
> facilities (`struct scatterlist` and `struct sg_table`).
> 
> This commit introduces `SGTable<T>`, a wrapper that uses a type-state
> pattern to provide compile-time guarantees about ownership and lifetime.
> 
> The abstraction provides two primary states:
> - `SGTable<Owned<P>>`: Represents a table whose resources are fully
>   managed by Rust. It takes ownership of a page provider `P`, allocates
>   the underlying `struct sg_table`, maps it for DMA, and handles all
>   cleanup automatically upon drop. The DMA mapping's lifetime is tied to
>   the associated device using `Devres`, ensuring it is correctly unmapped
>   before the device is unbound.
> - `SGTable<Borrowed>` (or just `SGTable`): A zero-cost representation of
>   an externally managed `struct sg_table`. It is created from a raw
>   pointer using `SGTable::as_ref()` and provides a lifetime-bound
>   reference (`&'a SGTable`) for operations like iteration.
> 
> The API exposes a safe iterator that yields `&SGEntry` references,
> allowing drivers to easily access the DMA address and length of each
> segment in the list.
> 
> Co-developed-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/helpers/helpers.c     |   1 +
>  rust/helpers/scatterlist.c |  24 ++
>  rust/kernel/lib.rs         |   1 +
>  rust/kernel/scatterlist.rs | 433 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 459 insertions(+)
>  create mode 100644 rust/helpers/scatterlist.c
>  create mode 100644 rust/kernel/scatterlist.rs
> 
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 7cf7fe95e41d..e94542bf6ea7 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -39,6 +39,7 @@
>  #include "rcu.c"
>  #include "refcount.c"
>  #include "regulator.c"
> +#include "scatterlist.c"
>  #include "security.c"
>  #include "signal.c"
>  #include "slab.c"
> diff --git a/rust/helpers/scatterlist.c b/rust/helpers/scatterlist.c
> new file mode 100644
> index 000000000000..80c956ee09ab
> --- /dev/null
> +++ b/rust/helpers/scatterlist.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/dma-direction.h>
> +
> +dma_addr_t rust_helper_sg_dma_address(struct scatterlist *sg)
> +{
> +	return sg_dma_address(sg);
> +}
> +
> +unsigned int rust_helper_sg_dma_len(struct scatterlist *sg)
> +{
> +	return sg_dma_len(sg);
> +}
> +
> +struct scatterlist *rust_helper_sg_next(struct scatterlist *sg)
> +{
> +	return sg_next(sg);
> +}
> +
> +void rust_helper_dma_unmap_sgtable(struct device *dev, struct sg_table *sgt,
> +				   enum dma_data_direction dir, unsigned long attrs)
> +{
> +	return dma_unmap_sgtable(dev, sgt, dir, attrs);
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index ed53169e795c..55acbc893736 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -113,6 +113,7 @@
>  pub mod rbtree;
>  pub mod regulator;
>  pub mod revocable;
> +pub mod scatterlist;
>  pub mod security;
>  pub mod seq_file;
>  pub mod sizes;
> diff --git a/rust/kernel/scatterlist.rs b/rust/kernel/scatterlist.rs
> new file mode 100644
> index 000000000000..4caaf8cfbf83
> --- /dev/null
> +++ b/rust/kernel/scatterlist.rs
> @@ -0,0 +1,433 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Abstractions for scatter-gather lists.
> +//!
> +//! C header: [`include/linux/scatterlist.h`](srctree/include/linux/scatterlist.h)
> +//!
> +//! Scatter-gather (SG) I/O is a memory access technique that allows devices to perform DMA
> +//! operations on data buffers that are not physically contiguous in memory. It works by creating a
> +//! "scatter-gather list", an array where each entry specifies the address and length of a
> +//! physically contiguous memory segment.
> +//!
> +//! The device's DMA controller can then read this list and process the segments sequentially as
> +//! part of one logical I/O request. This avoids the need for a single, large, physically contiguous
> +//! memory buffer, which can be difficult or impossible to allocate.
> +//!
> +//! This module provides safe Rust abstractions over the kernel's `struct scatterlist` and
> +//! `struct sg_table` types.
> +//!
> +//! The main entry point is the [`SGTable`] type, which represents a complete scatter-gather table.
> +//! It can be either:
> +//!
> +//! - An owned table ([`SGTable<Owned<P>>`]), created from a Rust memory buffer (e.g., [`VVec`]).
> +//!   This type manages the allocation of the `struct sg_table`, the DMA mapping of the buffer, and
> +//!   the automatic cleanup of all resources.
> +//! - A borrowed reference (&[`SGTable`]), which provides safe, read-only access to a table that was
> +//!   allocated by other (e.g., C) code.
> +//!
> +//! Individual entries in the table are represented by [`SGEntry`], which can be accessed by
> +//! iterating over an [`SGTable`].
> +
> +use crate::{
> +    alloc,
> +    alloc::allocator::VmallocPageIter,
> +    bindings,
> +    device::{Bound, Device},
> +    devres::Devres,
> +    dma, error, page,
> +    prelude::*,
> +    types::{ARef, Opaque},
> +};
> +use core::{ops::Deref, ptr::NonNull};
> +
> +/// A single entry in a scatter-gather list.
> +///
> +/// An `SGEntry` represents a single, physically contiguous segment of memory that has been mapped
> +/// for DMA.
> +///
> +/// Instances of this struct are obtained by iterating over an [`SGTable`]. Drivers do not create
> +/// or own [`SGEntry`] objects directly.
> +#[repr(transparent)]
> +pub struct SGEntry(Opaque<bindings::scatterlist>);

Send/Sync?

> +impl SGEntry {
> +    /// Convert a raw `struct scatterlist *` to a `&'a SGEntry`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the `struct scatterlist` pointed to by `ptr` is valid for the
> +    /// lifetime `'a`.
> +    unsafe fn as_ref<'a>(ptr: *mut bindings::scatterlist) -> &'a Self {

Please call this from_raw.

> +        // SAFETY: The safety requirements of this function guarantee that `ptr` is a valid pointer
> +        // to a `struct scatterlist` for the duration of `'a`.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    /// Obtain the raw `struct scatterlist *`.
> +    fn as_raw(&self) -> *mut bindings::scatterlist {

Consider adding #[inline] to all these methods.

> +        self.0.get()
> +    }
> +
> +    /// Returns the DMA address of this SG entry.
> +    ///
> +    /// This is the address that the device should use to access the memory segment.
> +    pub fn dma_address(&self) -> bindings::dma_addr_t {

We might want a typedef on the Rust side for dma_addr_t, like we already
have for the phys_addr_t/resource_size_t.

> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct scatterlist`.
> +        unsafe { bindings::sg_dma_address(self.as_raw()) }
> +    }
> +
> +    /// Returns the length of this SG entry in bytes.
> +    pub fn dma_len(&self) -> u32 {

Is u32 really the right length type?

> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct scatterlist`.
> +        unsafe { bindings::sg_dma_len(self.as_raw()) }
> +    }
> +}
> +
> +/// The borrowed type state of an [`SGTable`], representing a borrowed or externally managed table.
> +#[repr(transparent)]
> +pub struct Borrowed(Opaque<bindings::sg_table>);
> +
> +// SAFETY: An instance of `Borrowed` can be send to any task.
> +unsafe impl Send for Borrowed {}

No Sync?

> +/// A scatter-gather table.
> +///
> +/// This struct is a wrapper around the kernel's `struct sg_table`. It manages a list of DMA-mapped
> +/// memory segments that can be passed to a device for I/O operations.
> +///
> +/// The generic parameter `T` is used as a type state to distinguish between owned and borrowed
> +/// tables.
> +///
> +///  - [`SGTable<Owned>`]: An owned table created and managed entirely by Rust code. It handles
> +///    allocation, DMA mapping, and cleanup of all associated resources. See [`SGTable::new`].
> +///  - [`SGTable<Borrowed>`} (or simply [`SGTable`]): Represents a table whose lifetime is managed
> +///    externally. It can be used safely via a borrowed reference `&'a SGTable`, where `'a` is the
> +///    external lifetime.
> +///
> +/// All [`SGTable`] variants can be iterated over the individual [`SGEntry`]s.
> +#[repr(transparent)]
> +#[pin_data]
> +pub struct SGTable<T: private::Sealed = Borrowed> {
> +    #[pin]
> +    inner: T,
> +}
>
> +impl SGTable {
> +    /// Creates a borrowed `&'a SGTable` from a raw `struct sg_table` pointer.
> +    ///
> +    /// This allows safe access to an `sg_table` that is managed elsewhere (for example, in C code).
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the `struct sg_table` pointed to by `ptr` is valid for the entire
> +    /// lifetime of `'a`.
> +    pub unsafe fn as_ref<'a>(ptr: *mut bindings::sg_table) -> &'a Self {

Please rename to from_raw().

> +        // SAFETY: The safety requirements of this function guarantee that `ptr` is a valid pointer
> +        // to a `struct sg_table` for the duration of `'a`.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    fn as_raw(&self) -> *mut bindings::sg_table {

Ditto about #[inline] for these.

> +        self.inner.0.get()
> +    }
> +
> +    fn as_iter(&self) -> SGTableIter<'_> {
> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct sg_table`.
> +        let ptr = unsafe { (*self.as_raw()).sgl };
> +
> +        // SAFETY: `ptr` is guaranteed to be a valid pointer to a `struct scatterlist`.
> +        let pos = Some(unsafe { SGEntry::as_ref(ptr) });
> +
> +        SGTableIter { pos }
> +    }
> +}
> +
> +/// # Invariants
> +///
> +/// `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].

I think we probably want an invariant for why it's safe to call
dma_unmap_sgtable in Drop.

> +struct DmaMapSgt {
> +    sgt: NonNull<bindings::sg_table>,
> +    dev: ARef<Device>,
> +    dir: dma::DataDirection,
> +}
> +
> +// SAFETY: An instance of `DmaMapSgt` can be send to any task.
> +unsafe impl Send for DmaMapSgt {}

No Sync?

> +impl DmaMapSgt {
> +    /// # Safety
> +    ///
> +    /// `sgt` must be a valid pointer to a `struct sg_table` for the entire lifetime of the
> +    /// returned [`DmaMapSgt`].
> +    unsafe fn new(
> +        sgt: NonNull<bindings::sg_table>,
> +        dev: &Device<Bound>,
> +        dir: dma::DataDirection,
> +    ) -> Result<Self> {
> +        // SAFETY:
> +        // - `dev.as_raw()` is a valid pointer to a `struct device`, which is guaranteed to be
> +        //   bound to a driver for the duration of this call.
> +        // - `sgt` is a valid pointer to a `struct sg_table`.
> +        error::to_result(unsafe {
> +            bindings::dma_map_sgtable(dev.as_raw(), sgt.as_ptr(), dir.as_raw(), 0)
> +        })?;
> +
> +        // INVARIANT: By the safety requirements of this function it is guaranteed that `sgt` is
> +        // valid for the entire lifetime of this object instance.
> +        Ok(Self {
> +            sgt,
> +            dev: dev.into(),
> +            dir,
> +        })
> +    }
> +}
> +
> +impl Drop for DmaMapSgt {
> +    fn drop(&mut self) {
> +        // SAFETY:
> +        // - `self.dev.as_raw()` is a pointer to a valid `struct device`.
> +        // - `self.dev` is the same device the mapping has been created for in `Self::new()`.
> +        // - `self.sgt.as_ptr()` is a valid pointer to a `struct sg_table` by the type invariants
> +        //   of `Self`.
> +        // - `self.dir` is the same `dma::DataDirection` the mapping has been created with in
> +        //   `Self::new()`.
> +        unsafe {
> +            bindings::dma_unmap_sgtable(self.dev.as_raw(), self.sgt.as_ptr(), self.dir.as_raw(), 0)
> +        };
> +    }
> +}
> +
> +#[repr(transparent)]
> +#[pin_data(PinnedDrop)]
> +struct RawSGTable {
> +    #[pin]
> +    sgt: Opaque<bindings::sg_table>,
> +}

Send/Sync?

> +impl RawSGTable {
> +    fn new(
> +        mut pages: KVec<*mut bindings::page>,
> +        size: usize,
> +        max_segment: u32,
> +        flags: alloc::Flags,
> +    ) -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {
> +            sgt <- Opaque::try_ffi_init(|slot: *mut bindings::sg_table| {
> +                // `sg_alloc_table_from_pages_segment()` expects at least one page, otherwise it
> +                // produces a NPE.
> +                if pages.is_empty() {
> +                    return Err(EINVAL);
> +                }
> +
> +                // SAFETY:
> +                // - `slot` is a valid pointer to uninitialized memory.
> +                // - As by the check above, `pages` is not empty.
> +                error::to_result(unsafe {
> +                    bindings::sg_alloc_table_from_pages_segment(
> +                        slot,
> +                        pages.as_mut_ptr(),
> +                        pages.len().try_into()?,

The `pages` vector is dropped immediately after this call to
sg_alloc_table_from_pages_segment. Is that ok?

If it's ok, then I would change `pages` to `&[*mut page]` so that the
caller can manage the allocation of the array.

(Or maybe a mutable array ...?)

> +                        0,
> +                        size,
> +                        max_segment,
> +                        flags.as_raw(),
> +                    )
> +                })
> +            }),
> +        })
> +    }
> +
> +    fn as_raw(&self) -> *mut bindings::sg_table {
> +        self.sgt.get()
> +    }
> +}
> +
> +#[pinned_drop]
> +impl PinnedDrop for RawSGTable {
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY: `sgt` is a valid and initialized `struct sg_table`.
> +        unsafe { bindings::sg_free_table(self.sgt.get()) };
> +    }
> +}
> +
> +/// The [`Owned`] type state of an [`SGTable`].
> +///
> +/// A [`SGTable<Owned>`] signifies that the [`SGTable`] owns all associated resources:
> +///
> +/// - The backing memory pages.
> +/// - The `struct sg_table` allocation (`sgt`).
> +/// - The DMA mapping, managed through a [`Devres`]-managed `DmaMapSgt`.
> +///
> +/// Users interact with this type through the [`SGTable`] handle and do not need to manage
> +/// [`Owned`] directly.
> +#[pin_data]
> +pub struct Owned<P> {
> +    // Note: The drop order is relevant; we first have to unmap the `struct sg_table`, then free the
> +    // `struct sg_table` and finally free the backing pages.
> +    #[pin]
> +    dma: Devres<DmaMapSgt>,
> +    #[pin]
> +    sgt: RawSGTable,
> +    _pages: P,
> +}
> +
> +// SAFETY: An instance of `Owned` can be send to any task if `P` can be send to any task.
> +unsafe impl<P: Send> Send for Owned<P> {}

Sync?

> +impl<P> Owned<P>
> +where
> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,

If you specifically require the iterator type to be VmallocPageIter,
then I would hard-code that in the trait instead of specifying it here.

But I think you just want `P: AsPageIter`.

> +{
> +    fn new(
> +        dev: &Device<Bound>,
> +        mut pages: P,
> +        dir: dma::DataDirection,
> +        flags: alloc::Flags,
> +    ) -> Result<impl PinInit<Self, Error> + use<'_, P>> {

We would probably want to move the logic into the initializer so that we
don't have the double Result here.

> +        let page_iter = pages.page_iter();
> +        let size = page_iter.size();
> +
> +        let mut page_vec: KVec<*mut bindings::page> =
> +            KVec::with_capacity(page_iter.page_count(), flags)?;
> +
> +        for page in page_iter {
> +            page_vec.push(page.as_ptr(), flags)?;
> +        }
> +
> +        // `dma_max_mapping_size` returns `size_t`, but `sg_alloc_table_from_pages_segment()` takes
> +        // an `unsigned int`.
> +        let max_segment = {
> +            // SAFETY: `dev.as_raw()` is a valid pointer to a `struct device`.
> +            let size = unsafe { bindings::dma_max_mapping_size(dev.as_raw()) };
> +            if size == 0 {
> +                u32::MAX
> +            } else {
> +                size.min(u32::MAX as usize) as u32

u32::try_from(size).unwrap_or(u32::MAX)

> +            }
> +        };
> +
> +        Ok(try_pin_init!(&this in Self {
> +            sgt <- RawSGTable::new(page_vec, size, max_segment, flags),
> +            dma <- {
> +                // SAFETY: `this` is a valid pointer to uninitialized memory.
> +                let sgt = unsafe { &raw mut (*this.as_ptr()).sgt }.cast();
> +
> +                // SAFETY: `sgt` is guaranteed to be non-null.
> +                let sgt = unsafe { NonNull::new_unchecked(sgt) };
> +
> +                // SAFETY: It is guaranteed that the object returned by `DmaMapSgt::new` won't
> +                // out-live `sgt`.
> +                Devres::new(dev, unsafe { DmaMapSgt::new(sgt, dev, dir) })
> +            },
> +            _pages: pages,
> +        }))
> +    }
> +}
> +
> +impl<P> SGTable<Owned<P>>
> +where
> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
> +{
> +    /// Allocates a new scatter-gather table from the given pages and maps it for DMA.
> +    ///
> +    /// This constructor creates a new [`SGTable<Owned>`] that takes ownership of `P`.
> +    /// It allocates a `struct sg_table`, populates it with entries corresponding to the physical
> +    /// pages of `P`, and maps the table for DMA with the specified [`Device`] and
> +    /// [`dma::DataDirection`].
> +    ///
> +    /// The DMA mapping is managed through [`Devres`], ensuring that the DMA mapping is unmapped
> +    /// once the associated [`Device`] is unbound, or when the [`SGTable<Owned>`] is dropped.
> +    ///
> +    /// # Parameters
> +    ///
> +    /// * `dev`: The [`Device`] that will be performing the DMA.
> +    /// * `pages`: The entity providing the backing pages. It must implement [`page::AsPageIter`].
> +    ///   The ownership of this entity is moved into the new [`SGTable<Owned>`].
> +    /// * `dir`: The [`dma::DataDirection`] of the DMA transfer.
> +    /// * `flags`: Allocation flags for internal allocations (e.g., [`GFP_KERNEL`]).
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::{
> +    ///     device::{Bound, Device},
> +    ///     dma, page,
> +    ///     prelude::*,
> +    ///     scatterlist::*,
> +    /// };
> +    ///
> +    /// fn test(dev: &Device<Bound>) -> Result {
> +    ///     let size = 4 * page::PAGE_SIZE;
> +    ///     let pages = VVec::<u8>::with_capacity(size, GFP_KERNEL)?;
> +    ///
> +    ///     let sgt = KBox::pin_init(SGTable::new(
> +    ///         dev,
> +    ///         pages,
> +    ///         dma::DataDirection::TO_DEVICE,
> +    ///         GFP_KERNEL,
> +    ///     ), GFP_KERNEL)?;
> +    ///
> +    ///     Ok(())
> +    /// }
> +    /// ```
> +    pub fn new(
> +        dev: &Device<Bound>,
> +        pages: P,
> +        dir: dma::DataDirection,
> +        flags: alloc::Flags,
> +    ) -> impl PinInit<Self, Error> + use<'_, P> {
> +        try_pin_init!(Self {
> +            inner <- Owned::new(dev, pages, dir, flags)?
> +        })
> +    }
> +}
> +
> +impl<P> Deref for SGTable<Owned<P>> {
> +    type Target = SGTable;
> +
> +    fn deref(&self) -> &Self::Target {

Also #[inline].

> +        // SAFETY: `self.inner.sgt.as_raw()` is a valid pointer to a `struct sg_table` for the
> +        // entire lifetime of `self`.
> +        unsafe { SGTable::as_ref(self.inner.sgt.as_raw()) }
> +    }
> +}
> +
> +mod private {
> +    pub trait Sealed {}
> +
> +    impl Sealed for super::Borrowed {}
> +    impl<P> Sealed for super::Owned<P> {}
> +}
> +
> +impl<'a> IntoIterator for &'a SGTable {
> +    type Item = &'a SGEntry;
> +    type IntoIter = SGTableIter<'a>;
> +
> +    fn into_iter(self) -> Self::IntoIter {
> +        self.as_iter()
> +    }
> +}
> +
> +/// An [`Iterator`] over the [`SGEntry`] items of an [`SGTable`].
> +pub struct SGTableIter<'a> {
> +    pos: Option<&'a SGEntry>,
> +}
> +
> +impl<'a> Iterator for SGTableIter<'a> {
> +    type Item = &'a SGEntry;
> +
> +    fn next(&mut self) -> Option<Self::Item> {
> +        let entry = self.pos?;
> +
> +        // SAFETY: `entry.as_raw()` is a valid pointer to a `struct scatterlist`.
> +        let next = unsafe { bindings::sg_next(entry.as_raw()) };
> +
> +        self.pos = (!next.is_null()).then(|| {
> +            // SAFETY: If `next` is not NULL, `sg_next()` guarantees to return a valid pointer to
> +            // the next `struct scatterlist`.
> +            unsafe { SGEntry::as_ref(next) }
> +        });
> +
> +        Some(entry)
> +    }
> +}
> -- 
> 2.50.1
> 

Re: [PATCH 2/4] rust: scatterlist: Add type-state abstraction for sg_table

[Danilo Krummrich]

On Mon Aug 18, 2025 at 11:52 AM CEST, Alice Ryhl wrote:
> On Fri, Aug 15, 2025 at 07:10:03PM +0200, Danilo Krummrich wrote:
>> +/// A single entry in a scatter-gather list.
>> +///
>> +/// An `SGEntry` represents a single, physically contiguous segment of memory that has been mapped
>> +/// for DMA.
>> +///
>> +/// Instances of this struct are obtained by iterating over an [`SGTable`]. Drivers do not create
>> +/// or own [`SGEntry`] objects directly.
>> +#[repr(transparent)]
>> +pub struct SGEntry(Opaque<bindings::scatterlist>);
>
> Send/Sync?

I think SGEntry doesn't need it, but both would be valid.

For the other types, I simply forgot to impl Sync.

>> +    /// Returns the DMA address of this SG entry.
>> +    ///
>> +    /// This is the address that the device should use to access the memory segment.
>> +    pub fn dma_address(&self) -> bindings::dma_addr_t {
>
> We might want a typedef on the Rust side for dma_addr_t, like we already
> have for the phys_addr_t/resource_size_t.

Yes, the idea was to add that with a subsequent patch, dma.rs already leaks
bindings::dma_addr_t and I didn't want to mix up this series with this cleanup.

>> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct scatterlist`.
>> +        unsafe { bindings::sg_dma_address(self.as_raw()) }
>> +    }
>> +
>> +    /// Returns the length of this SG entry in bytes.
>> +    pub fn dma_len(&self) -> u32 {
>
> Is u32 really the right length type?

The C type uses unsigned int unfortunately, and SG entries larger than u32
probably don't make sense.

Formally, bus addresses and hence this size, can exceed size_t. However, it
obviously makes no sense for this to happen, so size_t would be a reasonable
choice. ressource_size_t would be resonable too.

>> +/// # Invariants
>> +///
>> +/// `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].
>
> I think we probably want an invariant for why it's safe to call
> dma_unmap_sgtable in Drop.

I assume you're suggesting that the invariant should say that the SG table is
always mapped? And that we should add a safety requirement to DmaMapSgt::new()
that the caller must guarantee that the SG table is never unmapped other than by
dropping DmaMapSgt?

If so, that sounds reasonable.

>> +struct DmaMapSgt {
>> +    sgt: NonNull<bindings::sg_table>,
>> +    dev: ARef<Device>,
>> +    dir: dma::DataDirection,
>> +}

<snip>

>> +impl RawSGTable {
>> +    fn new(
>> +        mut pages: KVec<*mut bindings::page>,
>> +        size: usize,
>> +        max_segment: u32,
>> +        flags: alloc::Flags,
>> +    ) -> impl PinInit<Self, Error> {
>> +        try_pin_init!(Self {
>> +            sgt <- Opaque::try_ffi_init(|slot: *mut bindings::sg_table| {
>> +                // `sg_alloc_table_from_pages_segment()` expects at least one page, otherwise it
>> +                // produces a NPE.
>> +                if pages.is_empty() {
>> +                    return Err(EINVAL);
>> +                }
>> +
>> +                // SAFETY:
>> +                // - `slot` is a valid pointer to uninitialized memory.
>> +                // - As by the check above, `pages` is not empty.
>> +                error::to_result(unsafe {
>> +                    bindings::sg_alloc_table_from_pages_segment(
>> +                        slot,
>> +                        pages.as_mut_ptr(),
>> +                        pages.len().try_into()?,
>
> The `pages` vector is dropped immediately after this call to
> sg_alloc_table_from_pages_segment. Is that ok?

Yes, it's only needed during sg_alloc_table_from_pages_segment().

> If it's ok, then I would change `pages` to `&[*mut page]` so that the
> caller can manage the allocation of the array.

We can immediately drop it after sg_alloc_table_from_pages_segmen(), so why do
we want the caller to take care of the lifetime?

>> +impl<P> Owned<P>
>> +where
>> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
>
> If you specifically require the iterator type to be VmallocPageIter,
> then I would hard-code that in the trait instead of specifying it here.

I do not follow, hard-code in which trait?

> But I think you just want `P: AsPageIter`.

Yeah, I thought for now it's probably good enough to require VmallocPageIter and
revisit once we get more implementors of AsPageIter, but I think we can also do
it right away.

I think we'd need a trait PageIterator, which implements page_count(), since we
technically can't rely on Iterator::size_hint(). Though, in this case I think we
can also just make AsPageIter::Iter: ExactSizeIterator?

>> +{
>> +    fn new(
>> +        dev: &Device<Bound>,
>> +        mut pages: P,
>> +        dir: dma::DataDirection,
>> +        flags: alloc::Flags,
>> +    ) -> Result<impl PinInit<Self, Error> + use<'_, P>> {
>
> We would probably want to move the logic into the initializer so that we
> don't have the double Result here.

That'd be nice, but I think it's not possible.

We can't borrow from pages in the initializer closure while at the same time
store pages with another initializer, can we?

Either way, it's not that big a deal I think, since this constructor is not
exposed to the outside world. Which is also why it didn't bother me too much.

>> +        let page_iter = pages.page_iter();
>> +        let size = page_iter.size();
>> +
>> +        let mut page_vec: KVec<*mut bindings::page> =
>> +            KVec::with_capacity(page_iter.page_count(), flags)?;
>> +
>> +        for page in page_iter {
>> +            page_vec.push(page.as_ptr(), flags)?;
>> +        }
>> +
>> +        // `dma_max_mapping_size` returns `size_t`, but `sg_alloc_table_from_pages_segment()` takes
>> +        // an `unsigned int`.
>> +        let max_segment = {
>> +            // SAFETY: `dev.as_raw()` is a valid pointer to a `struct device`.
>> +            let size = unsafe { bindings::dma_max_mapping_size(dev.as_raw()) };
>> +            if size == 0 {
>> +                u32::MAX
>> +            } else {
>> +                size.min(u32::MAX as usize) as u32
>
> u32::try_from(size).unwrap_or(u32::MAX)
>
>> +            }
>> +        };
>> +
>> +        Ok(try_pin_init!(&this in Self {
>> +            sgt <- RawSGTable::new(page_vec, size, max_segment, flags),
>> +            dma <- {
>> +                // SAFETY: `this` is a valid pointer to uninitialized memory.
>> +                let sgt = unsafe { &raw mut (*this.as_ptr()).sgt }.cast();
>> +
>> +                // SAFETY: `sgt` is guaranteed to be non-null.
>> +                let sgt = unsafe { NonNull::new_unchecked(sgt) };
>> +
>> +                // SAFETY: It is guaranteed that the object returned by `DmaMapSgt::new` won't
>> +                // out-live `sgt`.
>> +                Devres::new(dev, unsafe { DmaMapSgt::new(sgt, dev, dir) })
>> +            },
>> +            _pages: pages,
>> +        }))
>> +    }
>> +}

Re: [PATCH 1/4] rust: dma: implement DataDirection

[Danilo Krummrich]

On Mon Aug 18, 2025 at 11:34 AM CEST, Alice Ryhl wrote:
> On Fri, Aug 15, 2025 at 07:10:02PM +0200, Danilo Krummrich wrote:
>> Add the `DataDirection` struct, a newtype wrapper around the C
>> `enum dma_data_direction`.
>> 
>> This provides a type-safe Rust interface for specifying the direction of
>> DMA transfers.
>> 
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>
>> +/// DMA data direction.
>> +///
>> +/// Corresponds to the C [`enum dma_data_direction`].
>> +///
>> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
>> +#[derive(Copy, Clone, PartialEq, Eq)]
>> +pub struct DataDirection(bindings::dma_data_direction);
>
> Perhaps this should be a real Rust enum so that you can do an exhaustive
> match?

	/// DMA data direction.
	///
	/// Corresponds to the C [`enum dma_data_direction`].
	///
	/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
	#[derive(Copy, Clone, PartialEq, Eq, Debug)]
	#[repr(i32)]

Does bindgen ever pick another type than i32 for C enums? If so, it'd be a
downside that we'd have to mess with the type either in the `repr` or by casting
the variants.

	pub enum DataDirection {
	    /// The DMA mapping is for bidirectional data transfer.
	    ///
	    /// This is used when the buffer can be both read from and written to by the device.
	    /// The cache for the corresponding memory region is both flushed and invalidated.
	    Bidirectional = bindings::dma_data_direction_DMA_BIDIRECTIONAL,
	
	    /// The DMA mapping is for data transfer from memory to the device (write).
	    ///
	    /// The CPU has prepared data in the buffer, and the device will read it.
	    /// The cache for the corresponding memory region is flushed.
	    ToDevice = bindings::dma_data_direction_DMA_TO_DEVICE,
	
	    /// The DMA mapping is for data transfer from the device to memory (read).
	    ///
	    /// The device will write data into the buffer for the CPU to read.
	    /// The cache for the corresponding memory region is invalidated before CPU access.
	    FromDevice = bindings::dma_data_direction_DMA_FROM_DEVICE,
	
	    /// The DMA mapping is not for data transfer.
	    ///
	    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
	    /// will not perform any cache coherency operations.
	    None = bindings::dma_data_direction_DMA_NONE,
	}
	
	impl From<DataDirection> for bindings::dma_data_direction {
	    /// Returns the raw representation of [`enum dma_data_direction`].
	    fn from(direction: DataDirection) -> Self {
	        direction as Self
	    }
	}

Re: [PATCH 1/4] rust: dma: implement DataDirection

[Miguel Ojeda]

On Mon, Aug 18, 2025 at 1:27 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> Does bindgen ever pick another type than i32 for C enums? If so, it'd be a
> downside that we'd have to mess with the type either in the `repr` or by casting
> the variants.

Yes, it can easily pick `u32` -- the variants are always `int` in C,
but the actual enum can be a type compatible with int, unsigned or
char; and `bindgen` generates each variant with a type alias to the
underlying type of the `enum`, not `int`.

So e.g. for

    enum uint_enum { uint_enum_a, uint_enum_b };
    enum int_enum { int_enum_a = -1, int_enum_b };

    _Static_assert(_Generic(uint_enum_a, int: 1, default: 0), "");
    _Static_assert(_Generic(uint_enum_b, int: 1, default: 0), "");
    _Static_assert(_Generic(int_enum_a, int: 1, default: 0), "");
    _Static_assert(_Generic(int_enum_b, int: 1, default: 0), "");

    _Static_assert(_Generic((enum uint_enum)0, unsigned: 1, default: 0), "");
    _Static_assert(_Generic((enum int_enum)0, int: 1, default: 0), "");

you get:

    pub const uint_enum_uint_enum_a: uint_enum = 0;
    pub const uint_enum_uint_enum_b: uint_enum = 1;
    pub type uint_enum = ffi::c_uint;

    pub const int_enum_int_enum_a: int_enum = -1;
    pub const int_enum_int_enum_b: int_enum = 0;
    pub type int_enum = ffi::c_int;

Then there is this issue I reported to be careful about, which can
change it back to `i32` if there is a forward reference:

    https://github.com/rust-lang/rust-bindgen/issues/3179

I hope that helps.

Cheers,
Miguel

Re: [PATCH 2/4] rust: scatterlist: Add type-state abstraction for sg_table

[Danilo Krummrich]

On Mon Aug 18, 2025 at 1:16 PM CEST, Danilo Krummrich wrote:
> On Mon Aug 18, 2025 at 11:52 AM CEST, Alice Ryhl wrote:
>> On Fri, Aug 15, 2025 at 07:10:03PM +0200, Danilo Krummrich wrote:
>>> +impl<P> Owned<P>
>>> +where
>>> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
>>
>> If you specifically require the iterator type to be VmallocPageIter,
>> then I would hard-code that in the trait instead of specifying it here.
>
> I do not follow, hard-code in which trait?
>
>> But I think you just want `P: AsPageIter`.
>
> Yeah, I thought for now it's probably good enough to require VmallocPageIter and
> revisit once we get more implementors of AsPageIter, but I think we can also do
> it right away.
>
> I think we'd need a trait PageIterator, which implements page_count(), since we
> technically can't rely on Iterator::size_hint(). Though, in this case I think we
> can also just make AsPageIter::Iter: ExactSizeIterator?

Forgot to mention this [1] as for why we expect VmallocPageIter (at least for
now).

[1] https://lore.kernel.org/rust-for-linux/958ef505-8713-4f88-9f24-5971ce8a08ce@kernel.org/

Re: [PATCH 1/4] rust: dma: implement DataDirection

[Alice Ryhl]

On Mon, Aug 18, 2025 at 01:27:44PM +0200, Danilo Krummrich wrote:
> On Mon Aug 18, 2025 at 11:34 AM CEST, Alice Ryhl wrote:
> > On Fri, Aug 15, 2025 at 07:10:02PM +0200, Danilo Krummrich wrote:
> >> Add the `DataDirection` struct, a newtype wrapper around the C
> >> `enum dma_data_direction`.
> >> 
> >> This provides a type-safe Rust interface for specifying the direction of
> >> DMA transfers.
> >> 
> >> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >
> >> +/// DMA data direction.
> >> +///
> >> +/// Corresponds to the C [`enum dma_data_direction`].
> >> +///
> >> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
> >> +#[derive(Copy, Clone, PartialEq, Eq)]
> >> +pub struct DataDirection(bindings::dma_data_direction);
> >
> > Perhaps this should be a real Rust enum so that you can do an exhaustive
> > match?
> 
> 	/// DMA data direction.
> 	///
> 	/// Corresponds to the C [`enum dma_data_direction`].
> 	///
> 	/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
> 	#[derive(Copy, Clone, PartialEq, Eq, Debug)]
> 	#[repr(i32)]
> 
> Does bindgen ever pick another type than i32 for C enums? If so, it'd be a
> downside that we'd have to mess with the type either in the `repr` or by casting
> the variants.
> 
> 	pub enum DataDirection {
> 	    /// The DMA mapping is for bidirectional data transfer.
> 	    ///
> 	    /// This is used when the buffer can be both read from and written to by the device.
> 	    /// The cache for the corresponding memory region is both flushed and invalidated.
> 	    Bidirectional = bindings::dma_data_direction_DMA_BIDIRECTIONAL,
> 	
> 	    /// The DMA mapping is for data transfer from memory to the device (write).
> 	    ///
> 	    /// The CPU has prepared data in the buffer, and the device will read it.
> 	    /// The cache for the corresponding memory region is flushed.
> 	    ToDevice = bindings::dma_data_direction_DMA_TO_DEVICE,
> 	
> 	    /// The DMA mapping is for data transfer from the device to memory (read).
> 	    ///
> 	    /// The device will write data into the buffer for the CPU to read.
> 	    /// The cache for the corresponding memory region is invalidated before CPU access.
> 	    FromDevice = bindings::dma_data_direction_DMA_FROM_DEVICE,
> 	
> 	    /// The DMA mapping is not for data transfer.
> 	    ///
> 	    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
> 	    /// will not perform any cache coherency operations.
> 	    None = bindings::dma_data_direction_DMA_NONE,
> 	}
> 	
> 	impl From<DataDirection> for bindings::dma_data_direction {
> 	    /// Returns the raw representation of [`enum dma_data_direction`].
> 	    fn from(direction: DataDirection) -> Self {
> 	        direction as Self
> 	    }
> 	}

My suggestion is to cast on the Rust-side.

#[repr(whateveryouwant)]
enum DataDirection {
    Bidirectional = bindings::dma_data_direction_DMA_BIDIRECTIONAL as _,
}

Re: [PATCH 1/4] rust: dma: implement DataDirection

[Miguel Ojeda]

On Mon, Aug 18, 2025 at 1:56 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Yes, it can easily pick `u32` -- the variants are always `int` in C,

And, for completeness, if the kernel eventually uses C23 enums (I
don't see any, from a quick grep), i.e. with an underlying type, then
in C the variants will actually have that type, not `int`:

    enum c23_enum : short { c23_enum_a, c23_enum_b };

    _Static_assert(_Generic(c23_enum_a, short: 1, default: 0), "");
    _Static_assert(_Generic(c23_enum_b, short: 1, default: 0), "");

    _Static_assert(_Generic((enum c23_enum)0, short: 1, default: 0), "");

And `bindgen` will have the underlying type too, so everything matches
in that case:

    pub const c23_enum_c23_enum_a: c23_enum = 0;
    pub const c23_enum_c23_enum_b: c23_enum = 1;
    pub type c23_enum = ffi::c_short;

So, in a way, what `bindgen` tries to do (for the normal ones) is
behave a bit like these new ones.

But I always wondered if it is good or not differing there.

Cheers,
Miguel

Re: [PATCH 2/4] rust: scatterlist: Add type-state abstraction for sg_table

[Alice Ryhl]

On Mon, Aug 18, 2025 at 01:16:55PM +0200, Danilo Krummrich wrote:
> On Mon Aug 18, 2025 at 11:52 AM CEST, Alice Ryhl wrote:
> > On Fri, Aug 15, 2025 at 07:10:03PM +0200, Danilo Krummrich wrote:
> >> +/// A single entry in a scatter-gather list.
> >> +///
> >> +/// An `SGEntry` represents a single, physically contiguous segment of memory that has been mapped
> >> +/// for DMA.
> >> +///
> >> +/// Instances of this struct are obtained by iterating over an [`SGTable`]. Drivers do not create
> >> +/// or own [`SGEntry`] objects directly.
> >> +#[repr(transparent)]
> >> +pub struct SGEntry(Opaque<bindings::scatterlist>);
> >
> > Send/Sync?
> 
> I think SGEntry doesn't need it, but both would be valid.
> 
> For the other types, I simply forgot to impl Sync.

I would add them when it's valid to do so.

> >> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct scatterlist`.
> >> +        unsafe { bindings::sg_dma_address(self.as_raw()) }
> >> +    }
> >> +
> >> +    /// Returns the length of this SG entry in bytes.
> >> +    pub fn dma_len(&self) -> u32 {
> >
> > Is u32 really the right length type?
> 
> The C type uses unsigned int unfortunately, and SG entries larger than u32
> probably don't make sense.
> 
> Formally, bus addresses and hence this size, can exceed size_t. However, it
> obviously makes no sense for this to happen, so size_t would be a reasonable
> choice. ressource_size_t would be resonable too.

resource_size_t is what makes sense in my mind.

> >> +/// # Invariants
> >> +///
> >> +/// `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].
> >
> > I think we probably want an invariant for why it's safe to call
> > dma_unmap_sgtable in Drop.
> 
> I assume you're suggesting that the invariant should say that the SG table is
> always mapped? And that we should add a safety requirement to DmaMapSgt::new()
> that the caller must guarantee that the SG table is never unmapped other than by
> dropping DmaMapSgt?
> 
> If so, that sounds reasonable.

Something like that, yeah.

> >> +struct DmaMapSgt {
> >> +    sgt: NonNull<bindings::sg_table>,
> >> +    dev: ARef<Device>,
> >> +    dir: dma::DataDirection,
> >> +}
> 
> <snip>
> 
> >> +impl RawSGTable {
> >> +    fn new(
> >> +        mut pages: KVec<*mut bindings::page>,
> >> +        size: usize,
> >> +        max_segment: u32,
> >> +        flags: alloc::Flags,
> >> +    ) -> impl PinInit<Self, Error> {
> >> +        try_pin_init!(Self {
> >> +            sgt <- Opaque::try_ffi_init(|slot: *mut bindings::sg_table| {
> >> +                // `sg_alloc_table_from_pages_segment()` expects at least one page, otherwise it
> >> +                // produces a NPE.
> >> +                if pages.is_empty() {
> >> +                    return Err(EINVAL);
> >> +                }
> >> +
> >> +                // SAFETY:
> >> +                // - `slot` is a valid pointer to uninitialized memory.
> >> +                // - As by the check above, `pages` is not empty.
> >> +                error::to_result(unsafe {
> >> +                    bindings::sg_alloc_table_from_pages_segment(
> >> +                        slot,
> >> +                        pages.as_mut_ptr(),
> >> +                        pages.len().try_into()?,
> >
> > The `pages` vector is dropped immediately after this call to
> > sg_alloc_table_from_pages_segment. Is that ok?
> 
> Yes, it's only needed during sg_alloc_table_from_pages_segment().
> 
> > If it's ok, then I would change `pages` to `&[*mut page]` so that the
> > caller can manage the allocation of the array.
> 
> We can immediately drop it after sg_alloc_table_from_pages_segmen(), so why do
> we want the caller to take care of the lifetime?

I think the API is easier to understand that way. With a slice, it's
clear that you only expect it to be alive for the duration of the
method. Whereas with a vector, it looks like you're going to keep the
allocation alive long-term, but that's not the case.

> >> +impl<P> Owned<P>
> >> +where
> >> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
> >
> > If you specifically require the iterator type to be VmallocPageIter,
> > then I would hard-code that in the trait instead of specifying it here.
> 
> I do not follow, hard-code in which trait?

By hard-code, I meant that you refer to `VmallocPageIter` directly in
`trait AsPageIter`. But I don't think that's the correct solution.

> > But I think you just want `P: AsPageIter`.
> 
> Yeah, I thought for now it's probably good enough to require VmallocPageIter and
> revisit once we get more implementors of AsPageIter, but I think we can also do
> it right away.
> 
> I think we'd need a trait PageIterator, which implements page_count(), since we
> technically can't rely on Iterator::size_hint(). Though, in this case I think we
> can also just make AsPageIter::Iter: ExactSizeIterator?

I mean, ExactSizeIterator is not an unsafe trait, so it's allowed to lie
about the length. But it doesn't look like getting it wrong has any
problematic consequences here. At most we allocate too much in the
vector, or we have to reallocate it.

> >> +{
> >> +    fn new(
> >> +        dev: &Device<Bound>,
> >> +        mut pages: P,
> >> +        dir: dma::DataDirection,
> >> +        flags: alloc::Flags,
> >> +    ) -> Result<impl PinInit<Self, Error> + use<'_, P>> {
> >
> > We would probably want to move the logic into the initializer so that we
> > don't have the double Result here.
> 
> That'd be nice, but I think it's not possible.
> 
> We can't borrow from pages in the initializer closure while at the same time
> store pages with another initializer, can we?
> 
> Either way, it's not that big a deal I think, since this constructor is not
> exposed to the outside world. Which is also why it didn't bother me too much.

Ok. Shrug.

Alice

Re: [PATCH 2/4] rust: scatterlist: Add type-state abstraction for sg_table

[Danilo Krummrich]

On Mon Aug 18, 2025 at 2:27 PM CEST, Alice Ryhl wrote:
> On Mon, Aug 18, 2025 at 01:16:55PM +0200, Danilo Krummrich wrote:
>> On Mon Aug 18, 2025 at 11:52 AM CEST, Alice Ryhl wrote:
>> > On Fri, Aug 15, 2025 at 07:10:03PM +0200, Danilo Krummrich wrote:
>> >> +{
>> >> +    fn new(
>> >> +        dev: &Device<Bound>,
>> >> +        mut pages: P,
>> >> +        dir: dma::DataDirection,
>> >> +        flags: alloc::Flags,
>> >> +    ) -> Result<impl PinInit<Self, Error> + use<'_, P>> {
>> >
>> > We would probably want to move the logic into the initializer so that we
>> > don't have the double Result here.
>> 
>> That'd be nice, but I think it's not possible.
>> 
>> We can't borrow from pages in the initializer closure while at the same time
>> store pages with another initializer, can we?
>> 
>> Either way, it's not that big a deal I think, since this constructor is not
>> exposed to the outside world. Which is also why it didn't bother me too much.
>
> Ok. Shrug.

I mean, don't get me wrong, if you see a way to avoid the double Result, I'm
happy to change it.

(What I meant is, given the above, I thought it's not possible. But at the same
time I did not spend too much brain cycles, since the constructor is private
anyways.)

Re: [PATCH 1/4] rust: dma: implement DataDirection

[Danilo Krummrich]

On Mon Aug 18, 2025 at 2:22 PM CEST, Alice Ryhl wrote:
> On Mon, Aug 18, 2025 at 01:27:44PM +0200, Danilo Krummrich wrote:
>> On Mon Aug 18, 2025 at 11:34 AM CEST, Alice Ryhl wrote:
>> > On Fri, Aug 15, 2025 at 07:10:02PM +0200, Danilo Krummrich wrote:
>> >> Add the `DataDirection` struct, a newtype wrapper around the C
>> >> `enum dma_data_direction`.
>> >> 
>> >> This provides a type-safe Rust interface for specifying the direction of
>> >> DMA transfers.
>> >> 
>> >> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> >
>> >> +/// DMA data direction.
>> >> +///
>> >> +/// Corresponds to the C [`enum dma_data_direction`].
>> >> +///
>> >> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
>> >> +#[derive(Copy, Clone, PartialEq, Eq)]
>> >> +pub struct DataDirection(bindings::dma_data_direction);
>> >
>> > Perhaps this should be a real Rust enum so that you can do an exhaustive
>> > match?
>> 
>> 	/// DMA data direction.
>> 	///
>> 	/// Corresponds to the C [`enum dma_data_direction`].
>> 	///
>> 	/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
>> 	#[derive(Copy, Clone, PartialEq, Eq, Debug)]
>> 	#[repr(i32)]
>> 
>> Does bindgen ever pick another type than i32 for C enums? If so, it'd be a
>> downside that we'd have to mess with the type either in the `repr` or by casting
>> the variants.
>> 
>> 	pub enum DataDirection {
>> 	    /// The DMA mapping is for bidirectional data transfer.
>> 	    ///
>> 	    /// This is used when the buffer can be both read from and written to by the device.
>> 	    /// The cache for the corresponding memory region is both flushed and invalidated.
>> 	    Bidirectional = bindings::dma_data_direction_DMA_BIDIRECTIONAL,
>> 	
>> 	    /// The DMA mapping is for data transfer from memory to the device (write).
>> 	    ///
>> 	    /// The CPU has prepared data in the buffer, and the device will read it.
>> 	    /// The cache for the corresponding memory region is flushed.
>> 	    ToDevice = bindings::dma_data_direction_DMA_TO_DEVICE,
>> 	
>> 	    /// The DMA mapping is for data transfer from the device to memory (read).
>> 	    ///
>> 	    /// The device will write data into the buffer for the CPU to read.
>> 	    /// The cache for the corresponding memory region is invalidated before CPU access.
>> 	    FromDevice = bindings::dma_data_direction_DMA_FROM_DEVICE,
>> 	
>> 	    /// The DMA mapping is not for data transfer.
>> 	    ///
>> 	    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
>> 	    /// will not perform any cache coherency operations.
>> 	    None = bindings::dma_data_direction_DMA_NONE,
>> 	}
>> 	
>> 	impl From<DataDirection> for bindings::dma_data_direction {
>> 	    /// Returns the raw representation of [`enum dma_data_direction`].
>> 	    fn from(direction: DataDirection) -> Self {
>> 	        direction as Self
>> 	    }
>> 	}
>
> My suggestion is to cast on the Rust-side.
>
> #[repr(whateveryouwant)]

What's your suggestion for whateveryouwant?

And I mean this in general, not only for this case of dma::DataDirection. If we
pick u32 than things break if a negative enum variant is added on the C side. If
we pick i32, it can happen that we overflow.

> enum DataDirection {
>     Bidirectional = bindings::dma_data_direction_DMA_BIDIRECTIONAL as _,

We have a clippy lint that warns about `as _` casts, but I guess you meant
`as whateveryouwant`. However, if bindgen picks whateveryouwant too, we also get
a warning.

Ultimately, we'd need to suppress a warning.

> }

Re: [PATCH 2/4] rust: scatterlist: Add type-state abstraction for sg_table

[Danilo Krummrich]

On Mon Aug 18, 2025 at 2:21 PM CEST, Danilo Krummrich wrote:
> On Mon Aug 18, 2025 at 1:16 PM CEST, Danilo Krummrich wrote:
>> On Mon Aug 18, 2025 at 11:52 AM CEST, Alice Ryhl wrote:
>>> On Fri, Aug 15, 2025 at 07:10:03PM +0200, Danilo Krummrich wrote:
>>>> +impl<P> Owned<P>
>>>> +where
>>>> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
>>>
>>> If you specifically require the iterator type to be VmallocPageIter,
>>> then I would hard-code that in the trait instead of specifying it here.
>>
>> I do not follow, hard-code in which trait?
>>
>>> But I think you just want `P: AsPageIter`.
>>
>> Yeah, I thought for now it's probably good enough to require VmallocPageIter and
>> revisit once we get more implementors of AsPageIter, but I think we can also do
>> it right away.
>>
>> I think we'd need a trait PageIterator, which implements page_count(), since we
>> technically can't rely on Iterator::size_hint(). Though, in this case I think we
>> can also just make AsPageIter::Iter: ExactSizeIterator?
>
> Forgot to mention this [1] as for why we expect VmallocPageIter (at least for
> now).

Actually, let me expand on this a bit:

What I mean is that for some generic page::AsPageIter we don't know anything
about the semantics of the order of the pages; this is implementation specific
to the actual Iterator implementation, such as VmallocPageIter.

(For instance, VmallocPageIter iterates pages in the order as they are
virtually contiguous mapped by Vmalloc. VmallocPageIter documents this in the
context of a guarantees section.)

Thus, before allowing any AsPageIter::Iter in SGTable::new(), I'd like to see
actual implementations and subsequently figure out if it makes sense to add
additional (empty) traits providing such kind of guarantees.

> [1] https://lore.kernel.org/rust-for-linux/958ef505-8713-4f88-9f24-5971ce8a08ce@kernel.org/

Re: [PATCH 1/4] rust: dma: implement DataDirection

[Alice Ryhl]

On Mon, Aug 18, 2025 at 2:57 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon Aug 18, 2025 at 2:22 PM CEST, Alice Ryhl wrote:
> > On Mon, Aug 18, 2025 at 01:27:44PM +0200, Danilo Krummrich wrote:
> >> On Mon Aug 18, 2025 at 11:34 AM CEST, Alice Ryhl wrote:
> >> > On Fri, Aug 15, 2025 at 07:10:02PM +0200, Danilo Krummrich wrote:
> >> >> Add the `DataDirection` struct, a newtype wrapper around the C
> >> >> `enum dma_data_direction`.
> >> >>
> >> >> This provides a type-safe Rust interface for specifying the direction of
> >> >> DMA transfers.
> >> >>
> >> >> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >> >
> >> >> +/// DMA data direction.
> >> >> +///
> >> >> +/// Corresponds to the C [`enum dma_data_direction`].
> >> >> +///
> >> >> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
> >> >> +#[derive(Copy, Clone, PartialEq, Eq)]
> >> >> +pub struct DataDirection(bindings::dma_data_direction);
> >> >
> >> > Perhaps this should be a real Rust enum so that you can do an exhaustive
> >> > match?
> >>
> >>      /// DMA data direction.
> >>      ///
> >>      /// Corresponds to the C [`enum dma_data_direction`].
> >>      ///
> >>      /// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
> >>      #[derive(Copy, Clone, PartialEq, Eq, Debug)]
> >>      #[repr(i32)]
> >>
> >> Does bindgen ever pick another type than i32 for C enums? If so, it'd be a
> >> downside that we'd have to mess with the type either in the `repr` or by casting
> >> the variants.
> >>
> >>      pub enum DataDirection {
> >>          /// The DMA mapping is for bidirectional data transfer.
> >>          ///
> >>          /// This is used when the buffer can be both read from and written to by the device.
> >>          /// The cache for the corresponding memory region is both flushed and invalidated.
> >>          Bidirectional = bindings::dma_data_direction_DMA_BIDIRECTIONAL,
> >>
> >>          /// The DMA mapping is for data transfer from memory to the device (write).
> >>          ///
> >>          /// The CPU has prepared data in the buffer, and the device will read it.
> >>          /// The cache for the corresponding memory region is flushed.
> >>          ToDevice = bindings::dma_data_direction_DMA_TO_DEVICE,
> >>
> >>          /// The DMA mapping is for data transfer from the device to memory (read).
> >>          ///
> >>          /// The device will write data into the buffer for the CPU to read.
> >>          /// The cache for the corresponding memory region is invalidated before CPU access.
> >>          FromDevice = bindings::dma_data_direction_DMA_FROM_DEVICE,
> >>
> >>          /// The DMA mapping is not for data transfer.
> >>          ///
> >>          /// This is primarily for debugging purposes. With this direction, the DMA mapping API
> >>          /// will not perform any cache coherency operations.
> >>          None = bindings::dma_data_direction_DMA_NONE,
> >>      }
> >>
> >>      impl From<DataDirection> for bindings::dma_data_direction {
> >>          /// Returns the raw representation of [`enum dma_data_direction`].
> >>          fn from(direction: DataDirection) -> Self {
> >>              direction as Self
> >>          }
> >>      }
> >
> > My suggestion is to cast on the Rust-side.
> >
> > #[repr(whateveryouwant)]
>
> What's your suggestion for whateveryouwant?
>
> And I mean this in general, not only for this case of dma::DataDirection. If we
> pick u32 than things break if a negative enum variant is added on the C side. If
> we pick i32, it can happen that we overflow.
>
> > enum DataDirection {
> >     Bidirectional = bindings::dma_data_direction_DMA_BIDIRECTIONAL as _,
>
> We have a clippy lint that warns about `as _` casts, but I guess you meant
> `as whateveryouwant`. However, if bindgen picks whateveryouwant too, we also get
> a warning.
>
> Ultimately, we'd need to suppress a warning.

In general, I think we want some sort of helper function to cast
between arbitrary integer types in const-evaluation that panics if the
cast is out of bounds. Both here and many other places.

Alice

Re: [PATCH 1/4] rust: dma: implement DataDirection

[Danilo Krummrich]

On Mon Aug 18, 2025 at 4:00 PM CEST, Alice Ryhl wrote:
> In general, I think we want some sort of helper function to cast
> between arbitrary integer types in const-evaluation that panics if the
> cast is out of bounds. Both here and many other places.

What exactly do you have in mind?

Re: [PATCH 1/4] rust: dma: implement DataDirection

[Alice Ryhl]

On Mon, Aug 18, 2025 at 7:23 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon Aug 18, 2025 at 4:00 PM CEST, Alice Ryhl wrote:
> > In general, I think we want some sort of helper function to cast
> > between arbitrary integer types in const-evaluation that panics if the
> > cast is out of bounds. Both here and many other places.
>
> What exactly do you have in mind?

        pub enum DataDirection {
            /// The DMA mapping is for bidirectional data transfer.
            ///
            /// This is used when the buffer can be both read from and
written to by the device.
            /// The cache for the corresponding memory region is both
flushed and invalidated.
            Bidirectional =
const_cast(bindings::dma_data_direction_DMA_BIDIRECTIONAL),

with no warnings and build-failure if out-of-bounds.

Alice

Re: [PATCH 1/4] rust: dma: implement DataDirection

[Danilo Krummrich]

On 8/18/25 1:56 PM, Miguel Ojeda wrote:
> I hope that helps.

It does -- thanks for writing this up!

Re: [PATCH 1/4] rust: dma: implement DataDirection

[Danilo Krummrich]

On Mon Aug 18, 2025 at 8:47 PM CEST, Alice Ryhl wrote:
> with no warnings and build-failure if out-of-bounds.

+/// DMA data direction.
+///
+/// Corresponds to the C [`enum dma_data_direction`].
+///
+/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+#[repr(u32)]
+pub enum DataDirection {
+    /// The DMA mapping is for bidirectional data transfer.
+    ///
+    /// This is used when the buffer can be both read from and written to by the device.
+    /// The cache for the corresponding memory region is both flushed and invalidated.
+    Bidirectional = Self::const_cast(bindings::dma_data_direction_DMA_BIDIRECTIONAL),
+
+    /// The DMA mapping is for data transfer from memory to the device (write).
+    ///
+    /// The CPU has prepared data in the buffer, and the device will read it.
+    /// The cache for the corresponding memory region is flushed.
+    ToDevice = Self::const_cast(bindings::dma_data_direction_DMA_TO_DEVICE),
+
+    /// The DMA mapping is for data transfer from the device to memory (read).
+    ///
+    /// The device will write data into the buffer for the CPU to read.
+    /// The cache for the corresponding memory region is invalidated before CPU access.
+    FromDevice = Self::const_cast(bindings::dma_data_direction_DMA_FROM_DEVICE),
+
+    /// The DMA mapping is not for data transfer.
+    ///
+    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
+    /// will not perform any cache coherency operations.
+    None = Self::const_cast(bindings::dma_data_direction_DMA_NONE),
+}
+
+impl DataDirection {
+    /// Casts the bindgen-generated enum type to a `u32` at compile time.
+    ///
+    /// This function will cause a compile-time error if the underlying value of the
+    /// C enum is out of bounds for `u32`.
+    const fn const_cast(val: bindings::dma_data_direction) -> u32 {
+        // CAST: The C standard allows compilers to choose different integer types for enums.
+        // To safely check the value, we cast it to a wide signed integer type (`i128`)
+        // which can hold any standard C integer enum type without truncation.
+        let wide_val = val as i128;
+
+        // Check if the value is outside the valid range for the target type `u32`.
+        // CAST: `u32::MAX` is cast to `i128` to match the type of `wide_val` for the comparison.
+        if wide_val < 0 || wide_val > u32::MAX as i128 {
+            // Trigger a compile-time error in a const context.
+            panic!("C enum value is out of bounds for the target type `u32`.");
+        }
+
+        // CAST: This cast is valid because the check above guarantees that `wide_val`
+        // is within the representable range of `u32`.
+        wide_val as u32
+    }
+}
+
+impl From<DataDirection> for bindings::dma_data_direction {
+    /// Returns the raw representation of [`enum dma_data_direction`].
+    fn from(direction: DataDirection) -> Self {
+        // CAST: `direction as u32` gets the underlying representation of our `#[repr(u32)]` enum.
+        // The subsequent cast to `Self` (the bindgen type) assumes the C enum is compatible
+        // with the enum variants of `DataDirection`, which is a valid assumption given our
+        // compile-time checks.
+        direction as u32 as Self
+    }
+}

Re: [PATCH 1/4] rust: dma: implement DataDirection

[Daniel Almeida]

> On 18 Aug 2025, at 18:03, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Mon Aug 18, 2025 at 8:47 PM CEST, Alice Ryhl wrote:
>> with no warnings and build-failure if out-of-bounds.
> 
> +/// DMA data direction.
> +///
> +/// Corresponds to the C [`enum dma_data_direction`].
> +///
> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
> +#[derive(Copy, Clone, PartialEq, Eq, Debug)]
> +#[repr(u32)]
> +pub enum DataDirection {
> +    /// The DMA mapping is for bidirectional data transfer.
> +    ///
> +    /// This is used when the buffer can be both read from and written to by the device.
> +    /// The cache for the corresponding memory region is both flushed and invalidated.
> +    Bidirectional = Self::const_cast(bindings::dma_data_direction_DMA_BIDIRECTIONAL),
> +
> +    /// The DMA mapping is for data transfer from memory to the device (write).
> +    ///
> +    /// The CPU has prepared data in the buffer, and the device will read it.
> +    /// The cache for the corresponding memory region is flushed.
> +    ToDevice = Self::const_cast(bindings::dma_data_direction_DMA_TO_DEVICE),
> +
> +    /// The DMA mapping is for data transfer from the device to memory (read).
> +    ///
> +    /// The device will write data into the buffer for the CPU to read.
> +    /// The cache for the corresponding memory region is invalidated before CPU access.
> +    FromDevice = Self::const_cast(bindings::dma_data_direction_DMA_FROM_DEVICE),
> +
> +    /// The DMA mapping is not for data transfer.
> +    ///
> +    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
> +    /// will not perform any cache coherency operations.
> +    None = Self::const_cast(bindings::dma_data_direction_DMA_NONE),
> +}
> +
> +impl DataDirection {
> +    /// Casts the bindgen-generated enum type to a `u32` at compile time.
> +    ///
> +    /// This function will cause a compile-time error if the underlying value of the
> +    /// C enum is out of bounds for `u32`.
> +    const fn const_cast(val: bindings::dma_data_direction) -> u32 {

This should be its own generic helper for similar enums, IMHO

> +        // CAST: The C standard allows compilers to choose different integer types for enums.
> +        // To safely check the value, we cast it to a wide signed integer type (`i128`)
> +        // which can hold any standard C integer enum type without truncation.
> +        let wide_val = val as i128;
> +
> +        // Check if the value is outside the valid range for the target type `u32`.
> +        // CAST: `u32::MAX` is cast to `i128` to match the type of `wide_val` for the comparison.
> +        if wide_val < 0 || wide_val > u32::MAX as i128 {
> +            // Trigger a compile-time error in a const context.
> +            panic!("C enum value is out of bounds for the target type `u32`.");
> +        }
> +
> +        // CAST: This cast is valid because the check above guarantees that `wide_val`
> +        // is within the representable range of `u32`.
> +        wide_val as u32
> +    }
> +}
> +
> +impl From<DataDirection> for bindings::dma_data_direction {
> +    /// Returns the raw representation of [`enum dma_data_direction`].
> +    fn from(direction: DataDirection) -> Self {
> +        // CAST: `direction as u32` gets the underlying representation of our `#[repr(u32)]` enum.
> +        // The subsequent cast to `Self` (the bindgen type) assumes the C enum is compatible
> +        // with the enum variants of `DataDirection`, which is a valid assumption given our
> +        // compile-time checks.
> +        direction as u32 as Self
> +    }
> +}

Re: [PATCH 1/4] rust: dma: implement DataDirection

[Danilo Krummrich]

On Wed Aug 20, 2025 at 3:17 PM CEST, Daniel Almeida wrote:
>
>
>> On 18 Aug 2025, at 18:03, Danilo Krummrich <dakr@kernel.org> wrote:
>> 
>> On Mon Aug 18, 2025 at 8:47 PM CEST, Alice Ryhl wrote:
>>> with no warnings and build-failure if out-of-bounds.
>> 
>> +/// DMA data direction.
>> +///
>> +/// Corresponds to the C [`enum dma_data_direction`].
>> +///
>> +/// [`enum dma_data_direction`]: srctree/include/linux/dma-direction.h
>> +#[derive(Copy, Clone, PartialEq, Eq, Debug)]
>> +#[repr(u32)]
>> +pub enum DataDirection {
>> +    /// The DMA mapping is for bidirectional data transfer.
>> +    ///
>> +    /// This is used when the buffer can be both read from and written to by the device.
>> +    /// The cache for the corresponding memory region is both flushed and invalidated.
>> +    Bidirectional = Self::const_cast(bindings::dma_data_direction_DMA_BIDIRECTIONAL),
>> +
>> +    /// The DMA mapping is for data transfer from memory to the device (write).
>> +    ///
>> +    /// The CPU has prepared data in the buffer, and the device will read it.
>> +    /// The cache for the corresponding memory region is flushed.
>> +    ToDevice = Self::const_cast(bindings::dma_data_direction_DMA_TO_DEVICE),
>> +
>> +    /// The DMA mapping is for data transfer from the device to memory (read).
>> +    ///
>> +    /// The device will write data into the buffer for the CPU to read.
>> +    /// The cache for the corresponding memory region is invalidated before CPU access.
>> +    FromDevice = Self::const_cast(bindings::dma_data_direction_DMA_FROM_DEVICE),
>> +
>> +    /// The DMA mapping is not for data transfer.
>> +    ///
>> +    /// This is primarily for debugging purposes. With this direction, the DMA mapping API
>> +    /// will not perform any cache coherency operations.
>> +    None = Self::const_cast(bindings::dma_data_direction_DMA_NONE),
>> +}
>> +
>> +impl DataDirection {
>> +    /// Casts the bindgen-generated enum type to a `u32` at compile time.
>> +    ///
>> +    /// This function will cause a compile-time error if the underlying value of the
>> +    /// C enum is out of bounds for `u32`.
>> +    const fn const_cast(val: bindings::dma_data_direction) -> u32 {
>
> This should be its own generic helper for similar enums, IMHO

In general, I agree, but it may not be exactly straight forward (considering
that it still needs to work from const context).

The function relies on the fact that both the argument and return type are
numeric primitives, which need to fit into an i128. We could probably define a
marker trait for such types, etc.

Yet, I don't know how we could do the casts from one generic type I to another
generic type O. I think we'd need to implement traits for that as well. However,
this would require the unstable const_trait_impl feature.

Hence, working this out (if even possible currently) and improving existing enum
abstractions should be done independent from this series.

>> +        // CAST: The C standard allows compilers to choose different integer types for enums.
>> +        // To safely check the value, we cast it to a wide signed integer type (`i128`)
>> +        // which can hold any standard C integer enum type without truncation.
>> +        let wide_val = val as i128;
>> +
>> +        // Check if the value is outside the valid range for the target type `u32`.
>> +        // CAST: `u32::MAX` is cast to `i128` to match the type of `wide_val` for the comparison.
>> +        if wide_val < 0 || wide_val > u32::MAX as i128 {
>> +            // Trigger a compile-time error in a const context.
>> +            panic!("C enum value is out of bounds for the target type `u32`.");
>> +        }
>> +
>> +        // CAST: This cast is valid because the check above guarantees that `wide_val`
>> +        // is within the representable range of `u32`.
>> +        wide_val as u32
>> +    }
>> +}
>> +
>> +impl From<DataDirection> for bindings::dma_data_direction {
>> +    /// Returns the raw representation of [`enum dma_data_direction`].
>> +    fn from(direction: DataDirection) -> Self {
>> +        // CAST: `direction as u32` gets the underlying representation of our `#[repr(u32)]` enum.
>> +        // The subsequent cast to `Self` (the bindgen type) assumes the C enum is compatible
>> +        // with the enum variants of `DataDirection`, which is a valid assumption given our
>> +        // compile-time checks.
>> +        direction as u32 as Self
>> +    }
>> +}

Re: [PATCH 2/4] rust: scatterlist: Add type-state abstraction for sg_table

[Daniel Almeida]

Hi Danilo,

I am OK with this patch. I see that Alice has a few comments already so on my
side I will just leave a few nits, but in general this looks good.

> On 15 Aug 2025, at 14:10, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> Add a safe Rust abstraction for the kernel's scatter-gather list
> facilities (`struct scatterlist` and `struct sg_table`).
> 
> This commit introduces `SGTable<T>`, a wrapper that uses a type-state
> pattern to provide compile-time guarantees about ownership and lifetime.
> 
> The abstraction provides two primary states:
> - `SGTable<Owned<P>>`: Represents a table whose resources are fully
>  managed by Rust. It takes ownership of a page provider `P`, allocates
>  the underlying `struct sg_table`, maps it for DMA, and handles all
>  cleanup automatically upon drop. The DMA mapping's lifetime is tied to
>  the associated device using `Devres`, ensuring it is correctly unmapped
>  before the device is unbound.
> - `SGTable<Borrowed>` (or just `SGTable`): A zero-cost representation of
>  an externally managed `struct sg_table`. It is created from a raw
>  pointer using `SGTable::as_ref()` and provides a lifetime-bound
>  reference (`&'a SGTable`) for operations like iteration.
> 
> The API exposes a safe iterator that yields `&SGEntry` references,
> allowing drivers to easily access the DMA address and length of each
> segment in the list.
> 
> Co-developed-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
> rust/helpers/helpers.c     |   1 +
> rust/helpers/scatterlist.c |  24 ++
> rust/kernel/lib.rs         |   1 +
> rust/kernel/scatterlist.rs | 433 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 459 insertions(+)
> create mode 100644 rust/helpers/scatterlist.c
> create mode 100644 rust/kernel/scatterlist.rs
> 
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 7cf7fe95e41d..e94542bf6ea7 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -39,6 +39,7 @@
> #include "rcu.c"
> #include "refcount.c"
> #include "regulator.c"
> +#include "scatterlist.c"
> #include "security.c"
> #include "signal.c"
> #include "slab.c"
> diff --git a/rust/helpers/scatterlist.c b/rust/helpers/scatterlist.c
> new file mode 100644
> index 000000000000..80c956ee09ab
> --- /dev/null
> +++ b/rust/helpers/scatterlist.c
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/dma-direction.h>
> +
> +dma_addr_t rust_helper_sg_dma_address(struct scatterlist *sg)
> +{
> + return sg_dma_address(sg);
> +}
> +
> +unsigned int rust_helper_sg_dma_len(struct scatterlist *sg)
> +{
> + return sg_dma_len(sg);
> +}
> +
> +struct scatterlist *rust_helper_sg_next(struct scatterlist *sg)
> +{
> + return sg_next(sg);
> +}
> +
> +void rust_helper_dma_unmap_sgtable(struct device *dev, struct sg_table *sgt,
> +   enum dma_data_direction dir, unsigned long attrs)
> +{
> + return dma_unmap_sgtable(dev, sgt, dir, attrs);
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index ed53169e795c..55acbc893736 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -113,6 +113,7 @@
> pub mod rbtree;
> pub mod regulator;
> pub mod revocable;
> +pub mod scatterlist;
> pub mod security;
> pub mod seq_file;
> pub mod sizes;
> diff --git a/rust/kernel/scatterlist.rs b/rust/kernel/scatterlist.rs
> new file mode 100644
> index 000000000000..4caaf8cfbf83
> --- /dev/null
> +++ b/rust/kernel/scatterlist.rs
> @@ -0,0 +1,433 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Abstractions for scatter-gather lists.
> +//!
> +//! C header: [`include/linux/scatterlist.h`](srctree/include/linux/scatterlist.h)
> +//!
> +//! Scatter-gather (SG) I/O is a memory access technique that allows devices to perform DMA
> +//! operations on data buffers that are not physically contiguous in memory. It works by creating a
> +//! "scatter-gather list", an array where each entry specifies the address and length of a
> +//! physically contiguous memory segment.
> +//!
> +//! The device's DMA controller can then read this list and process the segments sequentially as
> +//! part of one logical I/O request. This avoids the need for a single, large, physically contiguous
> +//! memory buffer, which can be difficult or impossible to allocate.
> +//!
> +//! This module provides safe Rust abstractions over the kernel's `struct scatterlist` and
> +//! `struct sg_table` types.
> +//!
> +//! The main entry point is the [`SGTable`] type, which represents a complete scatter-gather table.
> +//! It can be either:
> +//!
> +//! - An owned table ([`SGTable<Owned<P>>`]), created from a Rust memory buffer (e.g., [`VVec`]).
> +//!   This type manages the allocation of the `struct sg_table`, the DMA mapping of the buffer, and
> +//!   the automatic cleanup of all resources.
> +//! - A borrowed reference (&[`SGTable`]), which provides safe, read-only access to a table that was
> +//!   allocated by other (e.g., C) code.
> +//!
> +//! Individual entries in the table are represented by [`SGEntry`], which can be accessed by
> +//! iterating over an [`SGTable`].
> +
> +use crate::{
> +    alloc,
> +    alloc::allocator::VmallocPageIter,
> +    bindings,
> +    device::{Bound, Device},
> +    devres::Devres,
> +    dma, error, page,
> +    prelude::*,
> +    types::{ARef, Opaque},
> +};
> +use core::{ops::Deref, ptr::NonNull};
> +
> +/// A single entry in a scatter-gather list.
> +///
> +/// An `SGEntry` represents a single, physically contiguous segment of memory that has been mapped
> +/// for DMA.
> +///
> +/// Instances of this struct are obtained by iterating over an [`SGTable`]. Drivers do not create
> +/// or own [`SGEntry`] objects directly.
> +#[repr(transparent)]
> +pub struct SGEntry(Opaque<bindings::scatterlist>);
> +
> +impl SGEntry {
> +    /// Convert a raw `struct scatterlist *` to a `&'a SGEntry`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the `struct scatterlist` pointed to by `ptr` is valid for the
> +    /// lifetime `'a`.
> +    unsafe fn as_ref<'a>(ptr: *mut bindings::scatterlist) -> &'a Self {
> +        // SAFETY: The safety requirements of this function guarantee that `ptr` is a valid pointer
> +        // to a `struct scatterlist` for the duration of `'a`.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    /// Obtain the raw `struct scatterlist *`.
> +    fn as_raw(&self) -> *mut bindings::scatterlist {
> +        self.0.get()
> +    }
> +
> +    /// Returns the DMA address of this SG entry.
> +    ///
> +    /// This is the address that the device should use to access the memory segment.
> +    pub fn dma_address(&self) -> bindings::dma_addr_t {
> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct scatterlist`.
> +        unsafe { bindings::sg_dma_address(self.as_raw()) }
> +    }
> +
> +    /// Returns the length of this SG entry in bytes.
> +    pub fn dma_len(&self) -> u32 {
> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct scatterlist`.
> +        unsafe { bindings::sg_dma_len(self.as_raw()) }
> +    }
> +}
> +
> +/// The borrowed type state of an [`SGTable`], representing a borrowed or externally managed table.
> +#[repr(transparent)]
> +pub struct Borrowed(Opaque<bindings::sg_table>);
> +
> +// SAFETY: An instance of `Borrowed` can be send to any task.
> +unsafe impl Send for Borrowed {}
> +
> +/// A scatter-gather table.
> +///
> +/// This struct is a wrapper around the kernel's `struct sg_table`. It manages a list of DMA-mapped
> +/// memory segments that can be passed to a device for I/O operations.
> +///
> +/// The generic parameter `T` is used as a type state to distinguish between owned and borrowed
> +/// tables.
> +///
> +///  - [`SGTable<Owned>`]: An owned table created and managed entirely by Rust code. It handles
> +///    allocation, DMA mapping, and cleanup of all associated resources. See [`SGTable::new`].
> +///  - [`SGTable<Borrowed>`} (or simply [`SGTable`]): Represents a table whose lifetime is managed
> +///    externally. It can be used safely via a borrowed reference `&'a SGTable`, where `'a` is the
> +///    external lifetime.
> +///
> +/// All [`SGTable`] variants can be iterated over the individual [`SGEntry`]s.
> +#[repr(transparent)]
> +#[pin_data]
> +pub struct SGTable<T: private::Sealed = Borrowed> {

Am I the only one that think we should have an actual trait here instead of
using private::Sealed directly?

> +    #[pin]
> +    inner: T,
> +}
> +
> +impl SGTable {
> +    /// Creates a borrowed `&'a SGTable` from a raw `struct sg_table` pointer.
> +    ///
> +    /// This allows safe access to an `sg_table` that is managed elsewhere (for example, in C code).
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that the `struct sg_table` pointed to by `ptr` is valid for the entire
> +    /// lifetime of `'a`.
> +    pub unsafe fn as_ref<'a>(ptr: *mut bindings::sg_table) -> &'a Self {
> +        // SAFETY: The safety requirements of this function guarantee that `ptr` is a valid pointer
> +        // to a `struct sg_table` for the duration of `'a`.
> +        unsafe { &*ptr.cast() }
> +    }
> +
> +    fn as_raw(&self) -> *mut bindings::sg_table {
> +        self.inner.0.get()
> +    }
> +
> +    fn as_iter(&self) -> SGTableIter<'_> {

Perhaps just "iter()” ?

> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct sg_table`.
> +        let ptr = unsafe { (*self.as_raw()).sgl };
> +
> +        // SAFETY: `ptr` is guaranteed to be a valid pointer to a `struct scatterlist`.
> +        let pos = Some(unsafe { SGEntry::as_ref(ptr) });
> +
> +        SGTableIter { pos }
> +    }
> +}
> +
> +/// # Invariants
> +///
> +/// `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].
> +struct DmaMapSgt {

This is private, but some extra docs here wouldn’t hurt?

> +    sgt: NonNull<bindings::sg_table>,
> +    dev: ARef<Device>,
> +    dir: dma::DataDirection,
> +}
> +
> +// SAFETY: An instance of `DmaMapSgt` can be send to any task.
> +unsafe impl Send for DmaMapSgt {}
> +
> +impl DmaMapSgt {
> +    /// # Safety
> +    ///
> +    /// `sgt` must be a valid pointer to a `struct sg_table` for the entire lifetime of the
> +    /// returned [`DmaMapSgt`].
> +    unsafe fn new(
> +        sgt: NonNull<bindings::sg_table>,
> +        dev: &Device<Bound>,
> +        dir: dma::DataDirection,
> +    ) -> Result<Self> {
> +        // SAFETY:
> +        // - `dev.as_raw()` is a valid pointer to a `struct device`, which is guaranteed to be
> +        //   bound to a driver for the duration of this call.
> +        // - `sgt` is a valid pointer to a `struct sg_table`.
> +        error::to_result(unsafe {
> +            bindings::dma_map_sgtable(dev.as_raw(), sgt.as_ptr(), dir.as_raw(), 0)
> +        })?;
> +
> +        // INVARIANT: By the safety requirements of this function it is guaranteed that `sgt` is
> +        // valid for the entire lifetime of this object instance.
> +        Ok(Self {
> +            sgt,
> +            dev: dev.into(),
> +            dir,
> +        })
> +    }
> +}
> +
> +impl Drop for DmaMapSgt {
> +    fn drop(&mut self) {
> +        // SAFETY:
> +        // - `self.dev.as_raw()` is a pointer to a valid `struct device`.
> +        // - `self.dev` is the same device the mapping has been created for in `Self::new()`.
> +        // - `self.sgt.as_ptr()` is a valid pointer to a `struct sg_table` by the type invariants
> +        //   of `Self`.
> +        // - `self.dir` is the same `dma::DataDirection` the mapping has been created with in
> +        //   `Self::new()`.
> +        unsafe {
> +            bindings::dma_unmap_sgtable(self.dev.as_raw(), self.sgt.as_ptr(), self.dir.as_raw(), 0)
> +        };
> +    }
> +}
> +
> +#[repr(transparent)]
> +#[pin_data(PinnedDrop)]
> +struct RawSGTable {
> +    #[pin]
> +    sgt: Opaque<bindings::sg_table>,
> +}
> +
> +impl RawSGTable {
> +    fn new(
> +        mut pages: KVec<*mut bindings::page>,
> +        size: usize,
> +        max_segment: u32,
> +        flags: alloc::Flags,
> +    ) -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {
> +            sgt <- Opaque::try_ffi_init(|slot: *mut bindings::sg_table| {
> +                // `sg_alloc_table_from_pages_segment()` expects at least one page, otherwise it
> +                // produces a NPE.
> +                if pages.is_empty() {
> +                    return Err(EINVAL);
> +                }
> +
> +                // SAFETY:
> +                // - `slot` is a valid pointer to uninitialized memory.
> +                // - As by the check above, `pages` is not empty.
> +                error::to_result(unsafe {
> +                    bindings::sg_alloc_table_from_pages_segment(
> +                        slot,
> +                        pages.as_mut_ptr(),
> +                        pages.len().try_into()?,
> +                        0,
> +                        size,
> +                        max_segment,
> +                        flags.as_raw(),
> +                    )
> +                })
> +            }),
> +        })
> +    }
> +
> +    fn as_raw(&self) -> *mut bindings::sg_table {
> +        self.sgt.get()
> +    }
> +}
> +
> +#[pinned_drop]
> +impl PinnedDrop for RawSGTable {
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY: `sgt` is a valid and initialized `struct sg_table`.
> +        unsafe { bindings::sg_free_table(self.sgt.get()) };
> +    }
> +}
> +
> +/// The [`Owned`] type state of an [`SGTable`].
> +///
> +/// A [`SGTable<Owned>`] signifies that the [`SGTable`] owns all associated resources:
> +///
> +/// - The backing memory pages.
> +/// - The `struct sg_table` allocation (`sgt`).
> +/// - The DMA mapping, managed through a [`Devres`]-managed `DmaMapSgt`.
> +///
> +/// Users interact with this type through the [`SGTable`] handle and do not need to manage
> +/// [`Owned`] directly.
> +#[pin_data]
> +pub struct Owned<P> {
> +    // Note: The drop order is relevant; we first have to unmap the `struct sg_table`, then free the
> +    // `struct sg_table` and finally free the backing pages.
> +    #[pin]
> +    dma: Devres<DmaMapSgt>,
> +    #[pin]
> +    sgt: RawSGTable,
> +    _pages: P,
> +}
> +
> +// SAFETY: An instance of `Owned` can be send to any task if `P` can be send to any task.
> +unsafe impl<P: Send> Send for Owned<P> {}
> +
> +impl<P> Owned<P>
> +where
> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
> +{
> +    fn new(
> +        dev: &Device<Bound>,
> +        mut pages: P,
> +        dir: dma::DataDirection,
> +        flags: alloc::Flags,
> +    ) -> Result<impl PinInit<Self, Error> + use<'_, P>> {

I confess I have no idea what “use<‘_, P>” is.

> +        let page_iter = pages.page_iter();
> +        let size = page_iter.size();
> +
> +        let mut page_vec: KVec<*mut bindings::page> =
> +            KVec::with_capacity(page_iter.page_count(), flags)?;
> +
> +        for page in page_iter {
> +            page_vec.push(page.as_ptr(), flags)?;
> +        }
> +
> +        // `dma_max_mapping_size` returns `size_t`, but `sg_alloc_table_from_pages_segment()` takes
> +        // an `unsigned int`.
> +        let max_segment = {
> +            // SAFETY: `dev.as_raw()` is a valid pointer to a `struct device`.
> +            let size = unsafe { bindings::dma_max_mapping_size(dev.as_raw()) };
> +            if size == 0 {
> +                u32::MAX
> +            } else {
> +                size.min(u32::MAX as usize) as u32
> +            }
> +        };
> +
> +        Ok(try_pin_init!(&this in Self {
> +            sgt <- RawSGTable::new(page_vec, size, max_segment, flags),
> +            dma <- {
> +                // SAFETY: `this` is a valid pointer to uninitialized memory.
> +                let sgt = unsafe { &raw mut (*this.as_ptr()).sgt }.cast();
> +
> +                // SAFETY: `sgt` is guaranteed to be non-null.
> +                let sgt = unsafe { NonNull::new_unchecked(sgt) };
> +
> +                // SAFETY: It is guaranteed that the object returned by `DmaMapSgt::new` won't
> +                // out-live `sgt`.
> +                Devres::new(dev, unsafe { DmaMapSgt::new(sgt, dev, dir) })
> +            },
> +            _pages: pages,
> +        }))
> +    }
> +}
> +
> +impl<P> SGTable<Owned<P>>
> +where
> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
> +{
> +    /// Allocates a new scatter-gather table from the given pages and maps it for DMA.
> +    ///
> +    /// This constructor creates a new [`SGTable<Owned>`] that takes ownership of `P`.
> +    /// It allocates a `struct sg_table`, populates it with entries corresponding to the physical
> +    /// pages of `P`, and maps the table for DMA with the specified [`Device`] and
> +    /// [`dma::DataDirection`].
> +    ///
> +    /// The DMA mapping is managed through [`Devres`], ensuring that the DMA mapping is unmapped
> +    /// once the associated [`Device`] is unbound, or when the [`SGTable<Owned>`] is dropped.
> +    ///
> +    /// # Parameters
> +    ///
> +    /// * `dev`: The [`Device`] that will be performing the DMA.
> +    /// * `pages`: The entity providing the backing pages. It must implement [`page::AsPageIter`].
> +    ///   The ownership of this entity is moved into the new [`SGTable<Owned>`].
> +    /// * `dir`: The [`dma::DataDirection`] of the DMA transfer.
> +    /// * `flags`: Allocation flags for internal allocations (e.g., [`GFP_KERNEL`]).
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// use kernel::{
> +    ///     device::{Bound, Device},
> +    ///     dma, page,
> +    ///     prelude::*,
> +    ///     scatterlist::*,
> +    /// };
> +    ///
> +    /// fn test(dev: &Device<Bound>) -> Result {
> +    ///     let size = 4 * page::PAGE_SIZE;
> +    ///     let pages = VVec::<u8>::with_capacity(size, GFP_KERNEL)?;
> +    ///
> +    ///     let sgt = KBox::pin_init(SGTable::new(
> +    ///         dev,
> +    ///         pages,
> +    ///         dma::DataDirection::TO_DEVICE,
> +    ///         GFP_KERNEL,
> +    ///     ), GFP_KERNEL)?;
> +    ///
> +    ///     Ok(())
> +    /// }
> +    /// ```
> +    pub fn new(
> +        dev: &Device<Bound>,
> +        pages: P,
> +        dir: dma::DataDirection,
> +        flags: alloc::Flags,
> +    ) -> impl PinInit<Self, Error> + use<'_, P> {
> +        try_pin_init!(Self {
> +            inner <- Owned::new(dev, pages, dir, flags)?
> +        })
> +    }
> +}
> +
> +impl<P> Deref for SGTable<Owned<P>> {
> +    type Target = SGTable;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: `self.inner.sgt.as_raw()` is a valid pointer to a `struct sg_table` for the
> +        // entire lifetime of `self`.
> +        unsafe { SGTable::as_ref(self.inner.sgt.as_raw()) }
> +    }
> +}
> +
> +mod private {
> +    pub trait Sealed {}
> +
> +    impl Sealed for super::Borrowed {}
> +    impl<P> Sealed for super::Owned<P> {}
> +}
> +
> +impl<'a> IntoIterator for &'a SGTable {
> +    type Item = &'a SGEntry;
> +    type IntoIter = SGTableIter<'a>;
> +
> +    fn into_iter(self) -> Self::IntoIter {
> +        self.as_iter()
> +    }
> +}
> +
> +/// An [`Iterator`] over the [`SGEntry`] items of an [`SGTable`].
> +pub struct SGTableIter<'a> {
> +    pos: Option<&'a SGEntry>,
> +}
> +
> +impl<'a> Iterator for SGTableIter<'a> {
> +    type Item = &'a SGEntry;
> +
> +    fn next(&mut self) -> Option<Self::Item> {
> +        let entry = self.pos?;
> +
> +        // SAFETY: `entry.as_raw()` is a valid pointer to a `struct scatterlist`.
> +        let next = unsafe { bindings::sg_next(entry.as_raw()) };
> +
> +        self.pos = (!next.is_null()).then(|| {
> +            // SAFETY: If `next` is not NULL, `sg_next()` guarantees to return a valid pointer to
> +            // the next `struct scatterlist`.
> +            unsafe { SGEntry::as_ref(next) }
> +        });
> +
> +        Some(entry)
> +    }
> +}
> -- 
> 2.50.1
> 
> 

— Daniel

Re: [PATCH 2/4] rust: scatterlist: Add type-state abstraction for sg_table

[Danilo Krummrich]

On Wed Aug 20, 2025 at 7:08 PM CEST, Daniel Almeida wrote:
>> +/// A scatter-gather table.
>> +///
>> +/// This struct is a wrapper around the kernel's `struct sg_table`. It manages a list of DMA-mapped
>> +/// memory segments that can be passed to a device for I/O operations.
>> +///
>> +/// The generic parameter `T` is used as a type state to distinguish between owned and borrowed
>> +/// tables.
>> +///
>> +///  - [`SGTable<Owned>`]: An owned table created and managed entirely by Rust code. It handles
>> +///    allocation, DMA mapping, and cleanup of all associated resources. See [`SGTable::new`].
>> +///  - [`SGTable<Borrowed>`} (or simply [`SGTable`]): Represents a table whose lifetime is managed
>> +///    externally. It can be used safely via a borrowed reference `&'a SGTable`, where `'a` is the
>> +///    external lifetime.
>> +///
>> +/// All [`SGTable`] variants can be iterated over the individual [`SGEntry`]s.
>> +#[repr(transparent)]
>> +#[pin_data]
>> +pub struct SGTable<T: private::Sealed = Borrowed> {
>
> Am I the only one that think we should have an actual trait here instead of
> using private::Sealed directly?

I don't know. :)

I think this case perfectly fits the Sealed pattern. There isn't any semantics
behind that, other than "we want that only the sealed ones work".

>
>> +    #[pin]
>> +    inner: T,
>> +}
>> +
>> +impl SGTable {
>> +    /// Creates a borrowed `&'a SGTable` from a raw `struct sg_table` pointer.
>> +    ///
>> +    /// This allows safe access to an `sg_table` that is managed elsewhere (for example, in C code).
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// Callers must ensure that the `struct sg_table` pointed to by `ptr` is valid for the entire
>> +    /// lifetime of `'a`.
>> +    pub unsafe fn as_ref<'a>(ptr: *mut bindings::sg_table) -> &'a Self {
>> +        // SAFETY: The safety requirements of this function guarantee that `ptr` is a valid pointer
>> +        // to a `struct sg_table` for the duration of `'a`.
>> +        unsafe { &*ptr.cast() }
>> +    }
>> +
>> +    fn as_raw(&self) -> *mut bindings::sg_table {
>> +        self.inner.0.get()
>> +    }
>> +
>> +    fn as_iter(&self) -> SGTableIter<'_> {
>
> Perhaps just "iter()” ?

No strong opinition, I'm fine with either.

>
>> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct sg_table`.
>> +        let ptr = unsafe { (*self.as_raw()).sgl };
>> +
>> +        // SAFETY: `ptr` is guaranteed to be a valid pointer to a `struct scatterlist`.
>> +        let pos = Some(unsafe { SGEntry::as_ref(ptr) });
>> +
>> +        SGTableIter { pos }
>> +    }
>> +}
>> +
>> +/// # Invariants
>> +///
>> +/// `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].
>> +struct DmaMapSgt {
>
> This is private, but some extra docs here wouldn’t hurt?

The type just represents the DMA mapping state of the SGT, i.e. exists
corresponds to is mapped, is dropped corresponds to is unmapped.

I can probably add a few lines if it helps.

>> +impl<P> Owned<P>
>> +where
>> +    for<'a> P: page::AsPageIter<Iter<'a> = VmallocPageIter<'a>> + 'static,
>> +{
>> +    fn new(
>> +        dev: &Device<Bound>,
>> +        mut pages: P,
>> +        dir: dma::DataDirection,
>> +        flags: alloc::Flags,
>> +    ) -> Result<impl PinInit<Self, Error> + use<'_, P>> {
>
> I confess I have no idea what “use<‘_, P>” is.

It's a feature called #![feature(precise_capturing)], which, unfortunately, does
not exist in 1.78, so I removed this syntax in v2. Luckly, we don't actually
need it in this case.