Message-Id: <DC9URZWE8Z4B.2R7NDRMFKENGK@kernel.org>
Date: Sat, 23 Aug 2025 15:48:38 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Alexandre Courbot" <acourbot@...dia.com>
Cc: <akpm@...ux-foundation.org>, <ojeda@...nel.org>,
 <alex.gaynor@...il.com>, <boqun.feng@...il.com>, <gary@...yguo.net>,
 <bjorn3_gh@...tonmail.com>, <lossin@...nel.org>, <a.hindborg@...nel.org>,
 <aliceryhl@...gle.com>, <tmgross@...ch.edu>, <abdiel.janulgue@...il.com>,
 <jgg@...pe.ca>, <lyude@...hat.com>, <robin.murphy@....com>,
 <daniel.almeida@...labora.com>, <rust-for-linux@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction
 for sg_table

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!

Powered by blists - more mailing lists