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 | 475 +++++++++++++++++++++++++++++++++++++
 4 files changed, 501 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..371c51222c5c
--- /dev/null
+++ b/rust/kernel/scatterlist.rs
@@ -0,0 +1,475 @@
+// 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,
+    io::resource::ResourceSize,
+    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>);
+
+// SAFETY: `SGEntry` can be send to any task.
+unsafe impl Send for SGEntry {}
+
+// SAFETY: `SGEntry` can be accessed concurrently.
+unsafe impl Sync for SGEntry {}
+
+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`.
+    #[inline]
+    unsafe fn from_raw<'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 *`.
+    #[inline]
+    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.
+    #[inline]
+    pub fn dma_address(&self) -> dma::DmaAddress {
+        // 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.
+    #[inline]
+    pub fn dma_len(&self) -> ResourceSize {
+        #[allow(clippy::useless_conversion)]
+        // SAFETY: `self.as_raw()` is a valid pointer to a `struct scatterlist`.
+        unsafe { bindings::sg_dma_len(self.as_raw()) }.into()
+    }
+}
+
+/// The borrowed type state of an [`SGTable`], representing a borrowed or externally managed table.
+#[repr(transparent)]
+pub struct Borrowed(Opaque<bindings::sg_table>);
+
+// SAFETY: `Borrowed` can be send to any task.
+unsafe impl Send for Borrowed {}
+
+// SAFETY: `Borrowed` can be accessed concurrently.
+unsafe impl Sync 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`,
+    /// - the data behind `ptr` is not modified concurrently for the duration of `'a`.
+    #[inline]
+    pub unsafe fn from_raw<'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() }
+    }
+
+    #[inline]
+    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::from_raw(ptr) });
+
+        SGTableIter { pos }
+    }
+}
+
+/// # Invariants
+///
+/// - `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].
+/// - `sgt` is always DMA mapped.
+struct DmaMapSgt {
+    sgt: NonNull<bindings::sg_table>,
+    dev: ARef<Device>,
+    dir: dma::DataDirection,
+}
+
+// SAFETY: `DmaMapSgt` can be send to any task.
+unsafe impl Send for DmaMapSgt {}
+
+// SAFETY: `DmaMapSgt` can be accessed concurrently.
+unsafe impl Sync for DmaMapSgt {}
+
+impl DmaMapSgt {
+    /// # Safety
+    ///
+    /// - `sgt` must be a valid pointer to a `struct sg_table` for the entire lifetime of the
+    ///   returned [`DmaMapSgt`].
+    /// - The caller must guarantee that `sgt` remains DMA mapped for the entire lifetime of
+    ///   [`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.into(), 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 {
+    #[inline]
+    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.into(), 0)
+        };
+    }
+}
+
+#[repr(transparent)]
+#[pin_data(PinnedDrop)]
+struct RawSGTable {
+    #[pin]
+    sgt: Opaque<bindings::sg_table>,
+}
+
+// SAFETY: `RawSGTable` can be send to any task.
+unsafe impl Send for RawSGTable {}
+
+// SAFETY: `RawSGTable` can be accessed concurrently.
+unsafe impl Sync for RawSGTable {}
+
+impl RawSGTable {
+    fn new(
+        pages: &mut [*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(),
+                    )
+                })
+            }),
+        })
+    }
+
+    #[inline]
+    fn as_raw(&self) -> *mut bindings::sg_table {
+        self.sgt.get()
+    }
+}
+
+#[pinned_drop]
+impl PinnedDrop for RawSGTable {
+    #[inline]
+    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: `Owned` can be send to any task if `P` can be send to any task.
+unsafe impl<P: Send> Send for Owned<P> {}
+
+// SAFETY: `Owned` can be accessed concurrently if `P` can be accessed concurrently.
+unsafe impl<P: Sync> Sync 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> + '_> {
+        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 {
+                u32::try_from(size).unwrap_or(u32::MAX)
+            }
+        };
+
+        Ok(try_pin_init!(&this in Self {
+            sgt <- RawSGTable::new(&mut 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`.
+                // - `sgt` is never DMA unmapped manually.
+                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::ToDevice,
+    ///         GFP_KERNEL,
+    ///     ), GFP_KERNEL)?;
+    ///
+    ///     Ok(())
+    /// }
+    /// ```
+    pub fn new(
+        dev: &Device<Bound>,
+        pages: P,
+        dir: dma::DataDirection,
+        flags: alloc::Flags,
+    ) -> impl PinInit<Self, Error> + '_ {
+        try_pin_init!(Self {
+            inner <- Owned::new(dev, pages, dir, flags)?
+        })
+    }
+}
+
+impl<P> Deref for SGTable<Owned<P>> {
+    type Target = SGTable;
+
+    #[inline]
+    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::from_raw(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>;
+
+    #[inline]
+    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::from_raw(next) }
+        });
+
+        Some(entry)
+    }
+}
-- 
2.50.1

Oops, forgot to mention a couple more things:

On Thu Aug 21, 2025 at 1:52 AM JST, 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.

Is this actually a typestate? From my understanding, the typestate
pattern implies transitions from one state to the other (such as
Unmapped -> Mapped), but in this version there are no such transitions
(the previous ones had, though). We are just using a generic parameter,
so mentioning typestate sounds a bit misleading to me.

Another random thought, in the owned case, do we want to provide an
accessor to the provider of the backing pages? Or do we expect the
caller to take dispositions to keep such a reference if they need to
access the backing buffer post-mapping?

On Sat Aug 23, 2025 at 3:47 PM CEST, Alexandre Courbot wrote:
> Oops, forgot to mention a couple more things:
>
> On Thu Aug 21, 2025 at 1:52 AM JST, 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.
>
> Is this actually a typestate? From my understanding, the typestate
> pattern implies transitions from one state to the other (such as
> Unmapped -> Mapped), but in this version there are no such transitions
> (the previous ones had, though). We are just using a generic parameter,
> so mentioning typestate sounds a bit misleading to me.

I'd argue that it's still kind of a typestate. You can derive &SGTable (i.e.
&SGTable<Borrowed>) from SGTabe<Owned>. So, technically there is an
uni-directional transition I guess.

> Another random thought, in the owned case, do we want to provide an
> accessor to the provider of the backing pages? Or do we expect the
> caller to take dispositions to keep such a reference if they need to
> access the backing buffer post-mapping?

That's not going to work that easily. Once the backing pages are DMA mapped, the
backing buffer can be accessed safely an more.

See also the safety requirements of dma::CoherentAllocation::as_slice() and
dma::CoherentAllocation::as_slice_mut().

If we want to support that, we have to provide a new type for this and maybe
want to define a common trait for DMA mapped memory accessors, etc.

Not the scope for this series, I believe. :)

On Sat Aug 23, 2025 at 10:57 PM JST, Danilo Krummrich wrote:
> On Sat Aug 23, 2025 at 3:47 PM CEST, Alexandre Courbot wrote:
>> Oops, forgot to mention a couple more things:
>>
>> On Thu Aug 21, 2025 at 1:52 AM JST, 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.
>>
>> Is this actually a typestate? From my understanding, the typestate
>> pattern implies transitions from one state to the other (such as
>> Unmapped -> Mapped), but in this version there are no such transitions
>> (the previous ones had, though). We are just using a generic parameter,
>> so mentioning typestate sounds a bit misleading to me.
>
> I'd argue that it's still kind of a typestate. You can derive &SGTable (i.e.
> &SGTable<Borrowed>) from SGTabe<Owned>. So, technically there is an
> uni-directional transition I guess.

That's technically correct, but is also not the intent of the design, at
least compared to something like Unmapped <-> Mapped. Not a big problem
if you prefer to keep the current naming though.

>
>> Another random thought, in the owned case, do we want to provide an
>> accessor to the provider of the backing pages? Or do we expect the
>> caller to take dispositions to keep such a reference if they need to
>> access the backing buffer post-mapping?
>
> That's not going to work that easily. Once the backing pages are DMA mapped, the
> backing buffer can be accessed safely an more.
>
> See also the safety requirements of dma::CoherentAllocation::as_slice() and
> dma::CoherentAllocation::as_slice_mut().

Yup. So couldn't similar accessors (marked unsafe of course) be
convenient?

>
> If we want to support that, we have to provide a new type for this and maybe
> want to define a common trait for DMA mapped memory accessors, etc.
>
> Not the scope for this series, I believe. :)

I've had a few thoughts in that direction as well, but completely agree
we should debate about this *after* this series is merged. :)

On Sat Aug 23, 2025 at 4:16 PM CEST, Alexandre Courbot wrote:
> On Sat Aug 23, 2025 at 10:57 PM JST, Danilo Krummrich wrote:
>> On Sat Aug 23, 2025 at 3:47 PM CEST, Alexandre Courbot wrote:
>>> Oops, forgot to mention a couple more things:
>>>
>>> On Thu Aug 21, 2025 at 1:52 AM JST, 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.
>>>
>>> Is this actually a typestate? From my understanding, the typestate
>>> pattern implies transitions from one state to the other (such as
>>> Unmapped -> Mapped), but in this version there are no such transitions
>>> (the previous ones had, though). We are just using a generic parameter,
>>> so mentioning typestate sounds a bit misleading to me.
>>
>> I'd argue that it's still kind of a typestate. You can derive &SGTable (i.e.
>> &SGTable<Borrowed>) from SGTabe<Owned>. So, technically there is an
>> uni-directional transition I guess.
>
> That's technically correct, but is also not the intent of the design, at
> least compared to something like Unmapped <-> Mapped. Not a big problem
> if you prefer to keep the current naming though.

I don't mind to name / call it differently, any suggestion?

>>
>>> Another random thought, in the owned case, do we want to provide an
>>> accessor to the provider of the backing pages? Or do we expect the
>>> caller to take dispositions to keep such a reference if they need to
>>> access the backing buffer post-mapping?
>>
>> That's not going to work that easily. Once the backing pages are DMA mapped, the
>> backing buffer can be accessed safely an more.
>>
>> See also the safety requirements of dma::CoherentAllocation::as_slice() and
>> dma::CoherentAllocation::as_slice_mut().
>
> Yup. So couldn't similar accessors (marked unsafe of course) be
> convenient?

Absolutely! But I think we want them represented by a common trait that can be
used by SGTable and dma::CoherentAllocation.

>>
>> If we want to support that, we have to provide a new type for this and maybe
>> want to define a common trait for DMA mapped memory accessors, etc.
>>
>> Not the scope for this series, I believe. :)
>
> I've had a few thoughts in that direction as well, but completely agree
> we should debate about this *after* this series is merged. :)

Yeah, let's add this feature subsequently.

On Sat Aug 23, 2025 at 11:20 PM JST, Danilo Krummrich wrote:
> On Sat Aug 23, 2025 at 4:16 PM CEST, Alexandre Courbot wrote:
>> On Sat Aug 23, 2025 at 10:57 PM JST, Danilo Krummrich wrote:
>>> On Sat Aug 23, 2025 at 3:47 PM CEST, Alexandre Courbot wrote:
>>>> Oops, forgot to mention a couple more things:
>>>>
>>>> On Thu Aug 21, 2025 at 1:52 AM JST, 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.
>>>>
>>>> Is this actually a typestate? From my understanding, the typestate
>>>> pattern implies transitions from one state to the other (such as
>>>> Unmapped -> Mapped), but in this version there are no such transitions
>>>> (the previous ones had, though). We are just using a generic parameter,
>>>> so mentioning typestate sounds a bit misleading to me.
>>>
>>> I'd argue that it's still kind of a typestate. You can derive &SGTable (i.e.
>>> &SGTable<Borrowed>) from SGTabe<Owned>. So, technically there is an
>>> uni-directional transition I guess.
>>
>> That's technically correct, but is also not the intent of the design, at
>> least compared to something like Unmapped <-> Mapped. Not a big problem
>> if you prefer to keep the current naming though.
>
> I don't mind to name / call it differently, any suggestion?

Simply using "generic parameter" would lift the possiblity for
misinterpretation IMHO.

On Thu Aug 21, 2025 at 1:52 AM JST, 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>
> ---

<snip>
> +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`.
> +    #[inline]
> +    unsafe fn from_raw<'a>(ptr: *mut bindings::scatterlist) -> &'a Self {
> +        // SAFETY: The safety requirements of this function guarantee that `ptr` is a valid pointer

nit: "guarantees".

<snip>
> +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).

nit: "to a".

> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that:
> +    ///
> +    /// - the `struct sg_table` pointed to by `ptr` is valid for the entire lifetime of `'a`,
> +    /// - the data behind `ptr` is not modified concurrently for the duration of `'a`.
> +    #[inline]
> +    pub unsafe fn from_raw<'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() }
> +    }
> +
> +    #[inline]
> +    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::from_raw(ptr) });
> +
> +        SGTableIter { pos }
> +    }
> +}
> +
> +/// # Invariants
> +///
> +/// - `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].

nit: "of the".

> +/// - `sgt` is always DMA mapped.
> +struct DmaMapSgt {

Minor point: I'd call this structure `DmaMappedSgt` to highlight the
fact that it is actively mapped. Or alternatively document it and its
members so that fact is clear.

<snip>
> +impl<'a> IntoIterator for &'a SGTable {
> +    type Item = &'a SGEntry;
> +    type IntoIter = SGTableIter<'a>;
> +
> +    #[inline]
> +    fn into_iter(self) -> Self::IntoIter {
> +        self.as_iter()
> +    }
> +}

While using this for Nova, I found it a bit unnatural having to call
`into_iter` on references intead of just having an `iter` method.
`into_iter` sounds like the passed object is consumed, while it is
actually its (copied) reference that is. Why not have a regular `iter`
method on `SGTable`? Actually we do have one, but it is called `as_iter`
and is private for some reason. :)

> +
> +/// 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::from_raw(next) }
> +        });

This might be missing a stop condition.

For reasons I am not completely clear about, the number of mapped
segments on the device side can be smaller than the number of
scatterlists provided by the sg_table. This is highlighted by the
documentation for `dma_map_sg_attrs` [1] ("Returns the number of mapped
entries (which can be less than nents) on success") and `sg_dma_address`
[2] ("You should only work with the number of sg entries dma_map_sg
returns, or alternatively stop on the first sg_dma_len(sg) which is 0.")

`dma_map_sgtable` stores the result of `dma_map_sg_attrs` into its
`nents` member, and makes use of it in its iterator macros. See how the
"regular" iterator and the "DMA" ones differ in their stop condition:

/*
 * Loop over each sg element in the given sg_table object.
 */
#define for_each_sgtable_sg(sgt, sg, i)		\
	for_each_sg((sgt)->sgl, sg, (sgt)->orig_nents, i)

and

/*
 * Loop over each sg element in the given *DMA mapped* sg_table object.
 * Please use sg_dma_address(sg) and sg_dma_len(sg) to extract DMA addresses
 * of the each element.
 */
#define for_each_sgtable_dma_sg(sgt, sg, i)	\
	for_each_sg((sgt)->sgl, sg, (sgt)->nents, i)

The DMA variant being the only one valid for accessing the DMA address
and DMA length (the C does not enforce this however).

So only calling `sg_next` until we reach the end of the list carries the
risk that we iterate on more items than we should, with the extra ones
having their length at 0 (which is likely to be a no-op, but is still
incorrect or at the very least inefficient). We can avoid this by either
storing the value of `nents` in the iterator, or, (which might be
simpler) follow the advice given by the documentation of
`sg_dma_address` and also stop if the DMA length of the next one is
zero.

[1] https://elixir.bootlin.com/linux/v6.16/source/kernel/dma/mapping.c#L233
[2] https://elixir.bootlin.com/linux/v6.16/source/include/linux/scatterlist.h#L31

On Sat, Aug 23, 2025 at 10:22:47PM +0900, Alexandre Courbot wrote:

> For reasons I am not completely clear about, the number of mapped
> segments on the device side can be smaller than the number of
> scatterlists provided by the sg_table. This is highlighted by the
> documentation for `dma_map_sg_attrs` [1] ("Returns the number of mapped
> entries (which can be less than nents) on success") and `sg_dma_address`
> [2] ("You should only work with the number of sg entries dma_map_sg
> returns, or alternatively stop on the first sg_dma_len(sg) which is 0.")

> So only calling `sg_next` until we reach the end of the list carries the
> risk that we iterate on more items than we should, with the extra ones
> having their length at 0

Correct, this is misusing the API, and I don't know if the lengths are
even guarenteed to be zero. To iterate the DMA list you must use the
length of the DMA list returned by dma_map_sg() and nothing else as
the stop condition.

To repeat again, the scatterlist data structure is "optimized" and
contains two completely different lists - the CPU list and the DMA
list. The DMA list is always <= the size of the CPU list.

For all purposes they are completely seperate things and we have a
unique set of iterators and accessors for the CPU vs DMA data.

Jason

On Sat Aug 23, 2025 at 4:32 PM CEST, Jason Gunthorpe wrote:
> Correct, this is misusing the API, and I don't know if the lengths are
> even guarenteed to be zero.

Hopefully it is. Otherwise, it's not only the documentation if sg_dma_address()
being wrong, but also a few drivers and maybe iommu_dma_unmap_sg(). However,
since it's a specific implementation of the API, it might be correct relying on
the sg_dma_len() == 0 check.

In any case, I think we can just use the nents field of struct sg_table, just
like for_each_sgtable_dma_sg() does.

On Sat Aug 23, 2025 at 3:22 PM CEST, Alexandre Courbot wrote:
> On Thu Aug 21, 2025 at 1:52 AM JST, Danilo Krummrich wrote:
>> +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`.
>> +    #[inline]
>> +    unsafe fn from_raw<'a>(ptr: *mut bindings::scatterlist) -> &'a Self {
>> +        // SAFETY: The safety requirements of this function guarantee that `ptr` is a valid pointer
>
> nit: "guarantees".

"guarantee" seems correct to me; it's "requirements" not "requirement".

(I think we commonly use the plural, i.e. "requirements" even if we end up
listing a single requirement only.)

> <snip>
>> +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).
>
> nit: "to a".

I'm not a native speaker, but I think "an" is correct, since "sg_table" is
pronounced with a vowel sound, /ɛs/, at the beginning.

>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// Callers must ensure that:
>> +    ///
>> +    /// - the `struct sg_table` pointed to by `ptr` is valid for the entire lifetime of `'a`,
>> +    /// - the data behind `ptr` is not modified concurrently for the duration of `'a`.
>> +    #[inline]
>> +    pub unsafe fn from_raw<'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() }
>> +    }
>> +
>> +    #[inline]
>> +    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::from_raw(ptr) });
>> +
>> +        SGTableIter { pos }SGEntry
>> +    }
>> +}
>> +
>> +/// # Invariants
>> +///
>> +/// - `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].
>
> nit: "of the".

This one I don't know for sure, maybe a native speaker can help.

I chose "for", since I think it indicates duration and "of" rather belonging,
but I honestly don't know. :)

>> +/// - `sgt` is always DMA mapped.
>> +struct DmaMapSgt {
>
> Minor point: I'd call this structure `DmaMappedSgt` to highlight the
> fact that it is actively mapped. Or alternatively document it and its
> members so that fact is clear.
>
> <snip>
>> +impl<'a> IntoIterator for &'a SGTable {
>> +    type Item = &'a SGEntry;
>> +    type IntoIter = SGTableIter<'a>;
>> +
>> +    #[inline]
>> +    fn into_iter(self) -> Self::IntoIter {
>> +        self.as_iter()
>> +    }
>> +}
>
> While using this for Nova, I found it a bit unnatural having to call
> `into_iter` on references intead of just having an `iter` method.
> `into_iter` sounds like the passed object is consumed, while it is
> actually its (copied) reference that is. Why not have a regular `iter`
> method on `SGTable`? Actually we do have one, but it is called `as_iter`
> and is private for some reason. :)

I think it makes sense to rename to SGTable::iter() and make it public.

I'm also fine removing the IntoIterator implementation, it seems pretty unlikely
that we'll have another type that provides an Iterator with SGEntry items we
need a generic interface for.

>> +
>> +/// 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::from_raw(next) }
>> +        });
>
> This might be missing a stop condition.

[...]

> follow the advice given by the documentation of
> `sg_dma_address` and also stop if the DMA length of the next one is
> zero.

Doh! I was even aware of this before sending the initial version and simply
forgot to add this stop condition after having been interrupted.

Thanks a lot for catching this!

On Sat Aug 23, 2025 at 10:48 PM JST, Danilo Krummrich wrote:
> On Sat Aug 23, 2025 at 3:22 PM CEST, Alexandre Courbot wrote:
>> On Thu Aug 21, 2025 at 1:52 AM JST, Danilo Krummrich wrote:
>>> +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`.
>>> +    #[inline]
>>> +    unsafe fn from_raw<'a>(ptr: *mut bindings::scatterlist) -> &'a Self {
>>> +        // SAFETY: The safety requirements of this function guarantee that `ptr` is a valid pointer
>>
>> nit: "guarantees".
>
> "guarantee" seems correct to me; it's "requirements" not "requirement".
>
> (I think we commonly use the plural, i.e. "requirements" even if we end up
> listing a single requirement only.)

Ah, you are correct! I missed the plural on "requirements".

>
>> <snip>
>>> +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).
>>
>> nit: "to a".
>
> I'm not a native speaker, but I think "an" is correct, since "sg_table" is
> pronounced with a vowel sound, /ɛs/, at the beginning.

TIL. :)

>
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// Callers must ensure that:
>>> +    ///
>>> +    /// - the `struct sg_table` pointed to by `ptr` is valid for the entire lifetime of `'a`,
>>> +    /// - the data behind `ptr` is not modified concurrently for the duration of `'a`.
>>> +    #[inline]
>>> +    pub unsafe fn from_raw<'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() }
>>> +    }
>>> +
>>> +    #[inline]
>>> +    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::from_raw(ptr) });
>>> +
>>> +        SGTableIter { pos }SGEntry
>>> +    }
>>> +}
>>> +
>>> +/// # Invariants
>>> +///
>>> +/// - `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].
>>
>> nit: "of the".
>
> This one I don't know for sure, maybe a native speaker can help.
>
> I chose "for", since I think it indicates duration and "of" rather belonging,
> but I honestly don't know. :)

I didn't give enough context. I meant "of the [`DmaMapSgt`]" (as in, it
cannot be any DmaMapSgt; it has to be this particular one).

>
>>> +/// - `sgt` is always DMA mapped.
>>> +struct DmaMapSgt {
>>
>> Minor point: I'd call this structure `DmaMappedSgt` to highlight the
>> fact that it is actively mapped. Or alternatively document it and its
>> members so that fact is clear.
>>
>> <snip>
>>> +impl<'a> IntoIterator for &'a SGTable {
>>> +    type Item = &'a SGEntry;
>>> +    type IntoIter = SGTableIter<'a>;
>>> +
>>> +    #[inline]
>>> +    fn into_iter(self) -> Self::IntoIter {
>>> +        self.as_iter()
>>> +    }
>>> +}
>>
>> While using this for Nova, I found it a bit unnatural having to call
>> `into_iter` on references intead of just having an `iter` method.
>> `into_iter` sounds like the passed object is consumed, while it is
>> actually its (copied) reference that is. Why not have a regular `iter`
>> method on `SGTable`? Actually we do have one, but it is called `as_iter`
>> and is private for some reason. :)
>
> I think it makes sense to rename to SGTable::iter() and make it public.
>
> I'm also fine removing the IntoIterator implementation, it seems pretty unlikely
> that we'll have another type that provides an Iterator with SGEntry items we
> need a generic interface for.

I assumed there was some Rust pattern to this `IntoIterator`
implementation on a reference, but I cannot see its usefulness when an
`iter` method also works on a reference anyway. So yeah if there is no
reason against it I think this would be more intuitive.

>
>>> +
>>> +/// 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::from_raw(next) }
>>> +        });
>>
>> This might be missing a stop condition.
>
> [...]
>
>> follow the advice given by the documentation of
>> `sg_dma_address` and also stop if the DMA length of the next one is
>> zero.
>
> Doh! I was even aware of this before sending the initial version and simply
> forgot to add this stop condition after having been interrupted.
>
> Thanks a lot for catching this!

A detail whose knowledge is typically acquired through considerable
suffering. :)

On Wed, Aug 20, 2025 at 06:52:57PM +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 | 475 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 501 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..371c51222c5c
> --- /dev/null
> +++ b/rust/kernel/scatterlist.rs
> @@ -0,0 +1,475 @@
> +// 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,
> +    io::resource::ResourceSize,
> +    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>);
> +
> +// SAFETY: `SGEntry` can be send to any task.
> +unsafe impl Send for SGEntry {}
> +
> +// SAFETY: `SGEntry` can be accessed concurrently.
> +unsafe impl Sync for SGEntry {}
> +
> +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`.
> +    #[inline]
> +    unsafe fn from_raw<'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 *`.
> +    #[inline]
> +    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.
> +    #[inline]
> +    pub fn dma_address(&self) -> dma::DmaAddress {
> +        // 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.
> +    #[inline]
> +    pub fn dma_len(&self) -> ResourceSize {
> +        #[allow(clippy::useless_conversion)]
> +        // SAFETY: `self.as_raw()` is a valid pointer to a `struct scatterlist`.
> +        unsafe { bindings::sg_dma_len(self.as_raw()) }.into()
> +    }
> +}
> +
> +/// The borrowed type state of an [`SGTable`], representing a borrowed or externally managed table.
> +#[repr(transparent)]
> +pub struct Borrowed(Opaque<bindings::sg_table>);
> +
> +// SAFETY: `Borrowed` can be send to any task.
> +unsafe impl Send for Borrowed {}
> +
> +// SAFETY: `Borrowed` can be accessed concurrently.
> +unsafe impl Sync 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`,
> +    /// - the data behind `ptr` is not modified concurrently for the duration of `'a`.
> +    #[inline]
> +    pub unsafe fn from_raw<'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() }
> +    }
> +
> +    #[inline]
> +    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::from_raw(ptr) });
> +
> +        SGTableIter { pos }
> +    }
> +}
> +
> +/// # Invariants
> +///
> +/// - `sgt` is a valid pointer to a `struct sg_table` for the entire lifetime of an [`DmaMapSgt`].
> +/// - `sgt` is always DMA mapped.
> +struct DmaMapSgt {
> +    sgt: NonNull<bindings::sg_table>,
> +    dev: ARef<Device>,
> +    dir: dma::DataDirection,
> +}
> +
> +// SAFETY: `DmaMapSgt` can be send to any task.
> +unsafe impl Send for DmaMapSgt {}
> +
> +// SAFETY: `DmaMapSgt` can be accessed concurrently.
> +unsafe impl Sync for DmaMapSgt {}
> +
> +impl DmaMapSgt {
> +    /// # Safety
> +    ///
> +    /// - `sgt` must be a valid pointer to a `struct sg_table` for the entire lifetime of the
> +    ///   returned [`DmaMapSgt`].
> +    /// - The caller must guarantee that `sgt` remains DMA mapped for the entire lifetime of
> +    ///   [`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.into(), 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 {
> +    #[inline]
> +    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.into(), 0)
> +        };
> +    }
> +}
> +
> +#[repr(transparent)]
> +#[pin_data(PinnedDrop)]
> +struct RawSGTable {
> +    #[pin]
> +    sgt: Opaque<bindings::sg_table>,
> +}
> +
> +// SAFETY: `RawSGTable` can be send to any task.
> +unsafe impl Send for RawSGTable {}
> +
> +// SAFETY: `RawSGTable` can be accessed concurrently.
> +unsafe impl Sync for RawSGTable {}
> +
> +impl RawSGTable {
> +    fn new(
> +        pages: &mut [*mut bindings::page],

This mutable reference is suspicious, IMO. Does it modify the list? You
don't read the values after calling `sg_alloc_table_from_pages_segment()`.

> +        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(),
> +                    )
> +                })
> +            }),
> +        })
> +    }
> +
> +    #[inline]
> +    fn as_raw(&self) -> *mut bindings::sg_table {
> +        self.sgt.get()
> +    }
> +}
> +
> +#[pinned_drop]
> +impl PinnedDrop for RawSGTable {
> +    #[inline]
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY: `sgt` is a valid and initialized `struct sg_table`.
> +        unsafe { bindings::sg_free_table(self.sgt.get()) };

It's weird that this is called free when the sg_table isn't freed by
this call.

> +    }
> +}
> +
> +/// 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: `Owned` can be send to any task if `P` can be send to any task.
> +unsafe impl<P: Send> Send for Owned<P> {}
> +
> +// SAFETY: `Owned` can be accessed concurrently if `P` can be accessed concurrently.
> +unsafe impl<P: Sync> Sync 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> + '_> {
> +        let page_iter = pages.page_iter();
> +        let size = page_iter.size();

Variable naming here is confusing. There's another variable called size
in an inner scope, and then afterwards in RawSGTable you use *this* size
variable again.

> +        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 {
> +                u32::try_from(size).unwrap_or(u32::MAX)
> +            }
> +        };
> +
> +        Ok(try_pin_init!(&this in Self {
> +            sgt <- RawSGTable::new(&mut 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`.
> +                // - `sgt` is never DMA unmapped manually.
> +                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::ToDevice,
> +    ///         GFP_KERNEL,
> +    ///     ), GFP_KERNEL)?;
> +    ///
> +    ///     Ok(())
> +    /// }
> +    /// ```
> +    pub fn new(
> +        dev: &Device<Bound>,
> +        pages: P,
> +        dir: dma::DataDirection,
> +        flags: alloc::Flags,
> +    ) -> impl PinInit<Self, Error> + '_ {
> +        try_pin_init!(Self {
> +            inner <- Owned::new(dev, pages, dir, flags)?
> +        })
> +    }
> +}
> +
> +impl<P> Deref for SGTable<Owned<P>> {
> +    type Target = SGTable;
> +
> +    #[inline]
> +    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::from_raw(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>;
> +
> +    #[inline]
> +    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::from_raw(next) }
> +        });
> +
> +        Some(entry)
> +    }
> +}
> -- 
> 2.50.1
>

On Fri Aug 22, 2025 at 1:44 PM CEST, Alice Ryhl wrote:
>> +#[pinned_drop]
>> +impl PinnedDrop for RawSGTable {
>> +    #[inline]
>> +    fn drop(self: Pin<&mut Self>) {
>> +        // SAFETY: `sgt` is a valid and initialized `struct sg_table`.
>> +        unsafe { bindings::sg_free_table(self.sgt.get()) };
>
> It's weird that this is called free when the sg_table isn't freed by
> this call.

It frees the entries contained in the sg_table.

>> +    }
>> +}
>> +
>> +/// 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: `Owned` can be send to any task if `P` can be send to any task.
>> +unsafe impl<P: Send> Send for Owned<P> {}
>> +
>> +// SAFETY: `Owned` can be accessed concurrently if `P` can be accessed concurrently.
>> +unsafe impl<P: Sync> Sync 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> + '_> {
>> +        let page_iter = pages.page_iter();
>> +        let size = page_iter.size();
>
> Variable naming here is confusing. There's another variable called size
> in an inner scope, and then afterwards in RawSGTable you use *this* size
> variable again.

I can change the size in the assignment block of max_segment to max_size, or do
you have other suggestions?

On Fri, Aug 22, 2025 at 01:48:47PM +0200, Danilo Krummrich wrote:
> On Fri Aug 22, 2025 at 1:44 PM CEST, Alice Ryhl wrote:
> >> +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> + '_> {
> >> +        let page_iter = pages.page_iter();
> >> +        let size = page_iter.size();
> >
> > Variable naming here is confusing. There's another variable called size
> > in an inner scope, and then afterwards in RawSGTable you use *this* size
> > variable again.
> 
> I can change the size in the assignment block of max_segment to max_size, or do
> you have other suggestions?

How about just this?

let max_segment = {
    // SAFETY: `dev.as_raw()` is a valid pointer to a `struct device`.
    let max_segment = unsafe { bindings::dma_max_mapping_size(dev.as_raw()) };
    if max_segment == 0 {
        u32::MAX
    } else {
        u32::try_from(max_segment).unwrap_or(u32::MAX)
    }
};

Alice

On 8/22/25 1:52 PM, Alice Ryhl wrote:
> On Fri, Aug 22, 2025 at 01:48:47PM +0200, Danilo Krummrich wrote:
>> On Fri Aug 22, 2025 at 1:44 PM CEST, Alice Ryhl wrote:
>>>> +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> + '_> {
>>>> +        let page_iter = pages.page_iter();
>>>> +        let size = page_iter.size();
>>>
>>> Variable naming here is confusing. There's another variable called size
>>> in an inner scope, and then afterwards in RawSGTable you use *this* size
>>> variable again.
>>
>> I can change the size in the assignment block of max_segment to max_size, or do
>> you have other suggestions?
> 
> How about just this?
> 
> let max_segment = {
>      // SAFETY: `dev.as_raw()` is a valid pointer to a `struct device`.
>      let max_segment = unsafe { bindings::dma_max_mapping_size(dev.as_raw()) };
>      if max_segment == 0 {
>          u32::MAX
>      } else {
>          u32::try_from(max_segment).unwrap_or(u32::MAX)
>      }
> };

Looks good, thanks!

On Fri Aug 22, 2025 at 8:54 PM JST, Danilo Krummrich wrote:
> On 8/22/25 1:52 PM, Alice Ryhl wrote:
>> On Fri, Aug 22, 2025 at 01:48:47PM +0200, Danilo Krummrich wrote:
>>> On Fri Aug 22, 2025 at 1:44 PM CEST, Alice Ryhl wrote:
>>>>> +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> + '_> {
>>>>> +        let page_iter = pages.page_iter();
>>>>> +        let size = page_iter.size();
>>>>
>>>> Variable naming here is confusing. There's another variable called size
>>>> in an inner scope, and then afterwards in RawSGTable you use *this* size
>>>> variable again.
>>>
>>> I can change the size in the assignment block of max_segment to max_size, or do
>>> you have other suggestions?
>> 
>> How about just this?
>> 
>> let max_segment = {
>>      // SAFETY: `dev.as_raw()` is a valid pointer to a `struct device`.
>>      let max_segment = unsafe { bindings::dma_max_mapping_size(dev.as_raw()) };
>>      if max_segment == 0 {
>>          u32::MAX
>>      } else {
>>          u32::try_from(max_segment).unwrap_or(u32::MAX)
>>      }
>> };
>
> Looks good, thanks!

Would this also work? It's a bit shorter.

// SAFETY: `dev.as_raw()` is a valid pointer to a `struct device`.
let max_segment = match unsafe { bindings::dma_max_mapping_size(dev.as_raw()) } {
    0 => u32::MAX,
    max_segment => u32::try_from(max_segment).unwrap_or(u32::MAX),
};

Hmm, I see that you’ve just posted a new version. I had v1 open the whole
morning so I missed that. Can you check my nits there?

— Daniel