Re: [PATCH v2 3/5] rust: scatterlist: Add type-state abstraction for sg_table

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

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?