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

["Danilo Krummrich" <dakr@kernel.org>]

On Tue Aug 26, 2025 at 5:18 PM CEST, Danilo Krummrich wrote:
> On Tue Aug 26, 2025 at 4:36 PM CEST, Alexandre Courbot wrote:
>> 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)
>
> That's not the reason this structure needs to be pinned. This is the reason for
> Devres itself needs to be pinned.
>
> In fact, I think RawSGTable by itself does not need to be pinned.

Just to expand on this a bit:

Eventually it does need to be pinned, because DmaMappedSgt keeps a pointer of
the underlying struct sg_table. But, this happens in Owned:

	Ok(try_pin_init!(&this in Self {
	    // SAFETY:
	    // - `page_vec` is a `KVec` of valid `struct page *` obtained from `pages`.
	    // - The pages contained in `pages` remain valid for the entire lifetime of the
	    //   `RawSGTable`.
	    sgt: unsafe { 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 `DmaMappedSgt::new` won't out-live
	        //   `sgt`.
	        // - `sgt` is never DMA unmapped manually.
	        Devres::new(dev, unsafe { DmaMappedSgt::new(sgt, dev, dir) })
	    },
	    _pages: pages,
	}))

So, it's fine to move RawSGTable around, *until* we obtain the address for
DmaMappedSgt (within Owned, Devres<DmaMappedSgt> is dropped before the
RawSGTable). However, this is an implementation detail of Owned and has nothing
to do with RawSGTable by itself.

Hence, we could also nuke #[pin_data] for RawSGTable and implement it as:

	/// A transparent wrapper around a `struct sg_table`.
	///
	/// While we could also create the `struct sg_table` in the constructor of [`Owned`], we can't tear
	/// down the `struct sg_table` in [`Owned::drop`]; the drop order in [`Owned`] matters.
	#[repr(transparent)]
	struct RawSGTable(Opaque<bindings::sg_table>);

	impl RawSGTable {
	    /// # Safety
	    ///
	    /// - `pages` must be a slice of valid `struct page *`.
	    /// - The pages pointed to by `pages` must remain valid for the entire lifetime of the returned
	    ///   [`RawSGTable`].
	    unsafe fn new(
	        pages: &mut [*mut bindings::page],
	        size: usize,
	        max_segment: u32,
	        flags: alloc::Flags,
	    ) -> Result<Self> {
	        // `sg_alloc_table_from_pages_segment()` expects at least one page, otherwise it
	        // produces a NPE.
	        if pages.is_empty() {
	            return Err(EINVAL);
	        }
	
	        let sgt = Opaque::zeroed();
	
	        // SAFETY:
	        // - `sgt.get()` 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(
	                sgt.get(),
	                pages.as_mut_ptr(),
	                pages.len().try_into()?,
	                0,
	                size,
	                max_segment,
	                flags.as_raw(),
	            )
	        })?;
	
	        Ok(Self(sgt))
	    }
	
	    #[inline]
	    fn as_raw(&self) -> *mut bindings::sg_table {
	        self.0.get()
	    }
	}
	
	impl Drop for RawSGTable {
	    #[inline]
	    fn drop(&mut self) {
	        // SAFETY: `sgt` is a valid and initialized `struct sg_table`.
	        unsafe { bindings::sg_free_table(self.0.get()) };
	    }
	}