Re: [PATCH v3 3/5] rust: scatterlist: Add abstraction for sg_table

[Alexandre Courbot]

On Mon Aug 25, 2025 at 10:24 PM 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 generic
> parameter 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@xxxxxxxxx>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@xxxxxxxxx>
> Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>

A few minor things below, but:

Reviewed-by: Alexandre Courbot <acourbot@xxxxxxxxxx>

Used successfully on nova-core:

Tested-by: Alexandre Courbot <acourbot@xxxxxxxxxx>

I still see mentions of "type state" in the code (and the commit
message), is this on purpose? I still think this is a misleading use of
the term, but your call.

<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`.

Shouldn't the scatterlist also have valid a dma_address and dma_len?

<snip>
> +#[repr(transparent)]
> +#[pin_data(PinnedDrop)]
> +struct RawSGTable {

Even if this is for internal use, I think a short comment explaining
what this is for, and why it needs to be pinned (pointed to by devres)
would be helpful to people looking at this code.